Log custom hook stderr output

2 files changed, 66 insertions, 0 deletions

diff --git a/ruby/lib/gitlab/git/hook.rb b/ruby/lib/gitlab/git/hook.rb
index 68cb76f7d..88f4868ec 100644
--- a/ruby/lib/gitlab/git/hook.rb
+++ b/ruby/lib/gitlab/git/hook.rb
@@ -12,6 +12,8 @@ module Gitlab
       end
 
       GL_PROTOCOL = 'web'
+      ERROR_LOG_FORMAT = '%s hook error in repository %s: %s'
+
       attr_reader :name, :path, :repository
 
       def initialize(name, repository)
@@ -52,6 +54,8 @@ module Gitlab
 
         stdout, stderr, exit_status = Open3.capture3(vars, path, options)
 
+        log_errors(stderr)
+
         exit_message = retrieve_output(stdout, stderr, exit_status)
 
         [exit_status.success?, exit_message]
@@ -64,6 +68,8 @@ module Gitlab
 
         stdout, stderr, exit_status = Open3.capture3(vars, path, *args, options)
 
+        log_errors(stderr)
+
         exit_message = retrieve_output(stdout, stderr, exit_status)
 
         [exit_status.success?, exit_message]
@@ -75,6 +81,21 @@ module Gitlab
         (stdout + stderr).strip
       end
 
+      def log_errors(errors)
+        return unless errors.present?
+
+        errors.split("\n").each do |error|
+          message = format(
+            ERROR_LOG_FORMAT,
+            name,
+            repository.relative_path,
+            error
+          )
+
+          Gitlab::GitLogger.error(message)
+        end
+      end
+
       def env_base_vars(gl_id, gl_username)
         {
           'GITLAB_SHELL_DIR' => Gitlab.config.gitlab_shell.path,
diff --git a/ruby/spec/lib/gitlab/git/hook_spec.rb b/ruby/spec/lib/gitlab/git/hook_spec.rb
index 0260d5b40..5b6e4f1ca 100644
--- a/ruby/spec/lib/gitlab/git/hook_spec.rb
+++ b/ruby/spec/lib/gitlab/git/hook_spec.rb
@@ -79,6 +79,8 @@ describe Gitlab::Git::Hook do
 
       it 'returns true' do
         hook_names.each do |hook|
+          silence_error_log
+
           trigger_result = described_class.new(hook, repo)
                                           .trigger('user-456', 'admin', '0' * 40, 'a' * 40, 'master')
 
@@ -88,12 +90,29 @@ describe Gitlab::Git::Hook do
 
       it 'does not return a message' do
         hook_names.each do |hook|
+          silence_error_log
+
           trigger_result = described_class.new(hook, repo)
                                           .trigger('user-456', 'admin', '0' * 40, 'a' * 40, 'master')
 
           expect(trigger_result.last).to eq(nil)
         end
       end
+
+      it 'logs all stderr to the error log' do
+        hook_names.each do |hook|
+          error_message = format(
+            Gitlab::Git::Hook::ERROR_LOG_FORMAT,
+            hook,
+            repo.relative_path,
+            'msg to STDERR'
+          )
+
+          expect(Gitlab::GitLogger).to receive(:error).with(error_message)
+
+          described_class.new(hook, repo).trigger('user-456', 'admin', '0' * 40, 'a' * 40, 'master')
+        end
+      end
     end
 
     context 'when the hooks fail' do
@@ -108,6 +127,8 @@ describe Gitlab::Git::Hook do
 
       it 'returns false' do
         hook_names.each do |hook|
+          silence_error_log
+
           trigger_result = described_class.new(hook, repo)
                                           .trigger('user-456', 'admin', '0' * 40, 'a' * 40, 'master')
 
@@ -117,12 +138,36 @@ describe Gitlab::Git::Hook do
 
       it 'returns all stdout and stderr messages' do
         hook_names.each do |hook|
+          silence_error_log
+
           trigger_result = described_class.new(hook, repo)
                                           .trigger('user-456', 'admin', '0' * 40, 'a' * 40, 'master')
 
           expect(trigger_result.last).to eq("msg to STDOUT\nmsg to STDERR")
         end
       end
+
+      it 'logs all stderr to the error log' do
+        hook_names.each do |hook|
+          error_message = format(
+            Gitlab::Git::Hook::ERROR_LOG_FORMAT,
+            hook,
+            repo.relative_path,
+            'msg to STDERR'
+          )
+
+          expect(Gitlab::GitLogger).to receive(:error).with(error_message)
+
+          described_class.new(hook, repo).trigger('user-456', 'admin', '0' * 40, 'a' * 40, 'master')
+        end
+      end
     end
   end
+
+  # Call before tests of scripts that write to stderr, when stderr is
+  # not a subject of a test. This prevents the error from appearing in
+  # rspec's output when rspec is running
+  def silence_error_log
+    expect(Gitlab::GitLogger).to receive(:error)
+  end
 end