Make behaviour of update Hook response consistentavoid-deadlocks

Previously the update Hook would return any standard error output, even
if the script exited successfully. This is different from how the
pre-receive and post-receive scripts behave, and from what is
documented.

2 files changed, 47 insertions, 4 deletions

diff --git a/ruby/lib/gitlab/git/hook.rb b/ruby/lib/gitlab/git/hook.rb
index 437de575d..e1c4932d3 100644
--- a/ruby/lib/gitlab/git/hook.rb
+++ b/ruby/lib/gitlab/git/hook.rb
@@ -81,7 +81,7 @@ module Gitlab
 
           unless wait_thr.value == 0
             exit_status = false
-            exit_message = stderr_messages.presence || stdout_messages
+            exit_message = retrieve_error_messages(stderr_messages, stdout_messages)
           end
         end
 
@@ -98,7 +98,18 @@ module Gitlab
         vars = env_base_vars(gl_id, gl_username)
 
         stdout, stderr, status = Open3.capture3(vars, path, *args, options)
-        [status.success?, stderr.presence || stdout]
+
+        exit_message = if status.success?
+                         nil
+                       else
+                         retrieve_error_messages(stderr, stdout)
+                       end
+
+        [status.success?, exit_message]
+      end
+
+      def retrieve_error_messages(stderr, stdout)
+        stderr.presence || stdout
       end
 
       def env_base_vars(gl_id, gl_username)
diff --git a/ruby/spec/lib/gitlab/git/hook_spec.rb b/ruby/spec/lib/gitlab/git/hook_spec.rb
index 2e1551ba1..1678fe7ee 100644
--- a/ruby/spec/lib/gitlab/git/hook_spec.rb
+++ b/ruby/spec/lib/gitlab/git/hook_spec.rb
@@ -68,7 +68,14 @@ describe Gitlab::Git::Hook do
     end
 
     context 'when the hooks are successful' do
-      let(:script) { "#!/bin/sh\nexit 0\n" }
+      let(:script) do
+        <<-SCRIPT
+          #!/bin/sh
+          echo "message";
+          1>&2 echo "error";
+          exit 0;
+        SCRIPT
+      end
 
       it 'returns true' do
         hook_names.each do |hook|
@@ -78,10 +85,26 @@ describe Gitlab::Git::Hook do
           expect(trigger_result.first).to be(true)
         end
       end
+
+      it 'returns no messages' do
+        hook_names.each do |hook|
+          trigger_result = described_class.new(hook, repo)
+                                          .trigger('user-456', 'admin', '0' * 40, 'a' * 40, 'master')
+
+          expect(trigger_result.last).to be_blank
+        end
+      end
     end
 
     context 'when the hooks fail' do
-      let(:script) { "#!/bin/sh\nexit 1\n" }
+      let(:script) do
+        <<-SCRIPT
+          #!/bin/sh
+          echo "message";
+          1>&2 echo "error";
+          exit 1;
+        SCRIPT
+      end
 
       it 'returns false' do
         hook_names.each do |hook|
@@ -91,6 +114,15 @@ describe Gitlab::Git::Hook do
           expect(trigger_result.first).to be(false)
         end
       end
+
+      it 'returns messages' do
+        hook_names.each do |hook|
+          trigger_result = described_class.new(hook, repo)
+                                          .trigger('user-1', 'admin', '0' * 40, 'a' * 40, 'master')
+
+          expect(trigger_result.last).to eq("error\n")
+        end
+      end
     end
   end
 end