Better message for git hooks and file locks

CHANGELOG

@@ -32,6 +32,7 @@ v 8.10.0 (unreleased)
   - Allow [ci skip] to be in any case and allow [skip ci]. !4785 (simon_w)
   - Add basic system information like memory and disk usage to the admin panel
   - Don't garbage collect commits that have related DB records like comments
+  - More descriptive message for git hooks and file locks
 
 v 8.9.5 (unreleased)
   - Improve the request / withdraw access button. !4860

app/services/create_branch_service.rb

@@ -34,8 +34,8 @@ class CreateBranchService < BaseService
     else
       error('Invalid reference name')
     end
-  rescue GitHooksService::PreReceiveError
-    error('Branch creation was rejected by Git hook')
+  rescue GitHooksService::PreReceiveError => ex
+    error(ex.message)
   end
 
   def success(branch)

app/services/create_tag_service.rb

@@ -13,8 +13,8 @@ class CreateTagService < BaseService
       new_tag = repository.add_tag(current_user, tag_name, target, message)
     rescue Rugged::TagError
       return error("Tag #{tag_name} already exists")
-    rescue GitHooksService::PreReceiveError
-      return error('Tag creation was rejected by Git hook')
+    rescue GitHooksService::PreReceiveError => ex
+      return error(ex.message)
     end
 
     if new_tag

app/services/delete_branch_service.rb

@@ -30,8 +30,8 @@ class DeleteBranchService < BaseService
     else
       error('Failed to remove branch')
     end
-  rescue GitHooksService::PreReceiveError
-    error('Branch deletion was rejected by Git hook')
+  rescue GitHooksService::PreReceiveError => ex
+    error(ex.message)
   end
 
   def error(message, return_code = 400)

app/services/git_hooks_service.rb

@@ -9,8 +9,10 @@ class GitHooksService
     @ref        = ref
 
     %w(pre-receive update).each do |hook_name|
-      unless run_hook(hook_name)
-        raise PreReceiveError.new("Git operation was rejected by #{hook_name} hook")
+      status, message = run_hook(hook_name)
+
+      unless status
+        raise PreReceiveError, message
       end
     end
 

lib/gitlab/git/hook.rb

@@ -29,8 +29,8 @@ module Gitlab
       def call_receive_hook(gl_id, oldrev, newrev, ref)
         changes = [oldrev, newrev, ref].join(" ")
 
-        # function  will return true if succesful
         exit_status = false
+        exit_message = nil
 
         vars = {
           'GL_ID' => gl_id,
@@ -41,7 +41,7 @@ module Gitlab
           chdir: repo_path
         }
 
-        Open3.popen2(vars, path, options) do |stdin, _, wait_thr|
+        Open3.popen3(vars, path, options) do |stdin, _, stderr, wait_thr|
           exit_status = true
           stdin.sync = true
 
@@ -60,16 +60,21 @@ module Gitlab
 
           unless wait_thr.value == 0
             exit_status = false
+            exit_message = stderr.gets
           end
         end
 
-        exit_status
+        [exit_status, exit_message]
       end
 
       def call_update_hook(gl_id, oldrev, newrev, ref)
+        status = nil
+
         Dir.chdir(repo_path) do
-          system({ 'GL_ID' => gl_id }, path, ref, oldrev, newrev)
+          status = system({ 'GL_ID' => gl_id }, path, ref, oldrev, newrev)
         end
+
+        [status, nil]
       end
     end
   end

spec/models/repository_spec.rb

@@ -308,14 +308,14 @@ describe Repository, models: true do
   describe :add_branch do
     context 'when pre hooks were successful' do
       it 'should run without errors' do
-        hook = double(trigger: true)
+        hook = double(trigger: [true, nil])
         expect(Gitlab::Git::Hook).to receive(:new).exactly(3).times.and_return(hook)
 
         expect { repository.add_branch(user, 'new_feature', 'master') }.not_to raise_error
       end
 
       it 'should create the branch' do
-        allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return(true)
+        allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([true, nil])
 
         branch = repository.add_branch(user, 'new_feature', 'master')
 
@@ -331,7 +331,7 @@ describe Repository, models: true do
 
     context 'when pre hooks failed' do
       it 'should get an error' do
-        allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return(false)
+        allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, ''])
 
         expect do
           repository.add_branch(user, 'new_feature', 'master')
@@ -339,7 +339,7 @@ describe Repository, models: true do
       end
 
       it 'should not create the branch' do
-        allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return(false)
+        allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, ''])
 
         expect do
           repository.add_branch(user, 'new_feature', 'master')
@@ -352,13 +352,13 @@ describe Repository, models: true do
   describe :rm_branch do
     context 'when pre hooks were successful' do
       it 'should run without errors' do
-        allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return(true)
+        allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([true, nil])
 
         expect { repository.rm_branch(user, 'feature') }.not_to raise_error
       end
 
       it 'should delete the branch' do
-        allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return(true)
+        allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([true, nil])
 
         expect { repository.rm_branch(user, 'feature') }.not_to raise_error
 
@@ -368,7 +368,7 @@ describe Repository, models: true do
 
     context 'when pre hooks failed' do
       it 'should get an error' do
-        allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return(false)
+        allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, ''])
 
         expect do
           repository.rm_branch(user, 'new_feature')
@@ -376,7 +376,7 @@ describe Repository, models: true do
       end
 
       it 'should not delete the branch' do
-        allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return(false)
+        allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, ''])
 
         expect do
           repository.rm_branch(user, 'feature')
@@ -408,7 +408,7 @@ describe Repository, models: true do
 
     context 'when pre hooks failed' do
       it 'should get an error' do
-        allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return(false)
+        allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, ''])
 
         expect do
           repository.commit_with_hooks(user, 'feature') { sample_commit.id }

spec/services/create_tag_service_spec.rb

@@ -41,12 +41,12 @@ describe CreateTagService, services: true do
       it 'returns an error' do
         expect(repository).to receive(:add_tag).
           with(user, 'v1.1.0', 'master', 'Foo').
-          and_raise(GitHooksService::PreReceiveError)
+          and_raise(GitHooksService::PreReceiveError, 'something went wrong')
 
         response = service.execute('v1.1.0', 'master', 'Foo')
 
         expect(response).to eq(status: :error,
-                               message: 'Tag creation was rejected by Git hook')
+                               message: 'something went wrong')
       end
     end
   end

spec/services/git_hooks_service_spec.rb

@@ -18,16 +18,16 @@ describe GitHooksService, services: true do
   describe '#execute' do
     context 'when receive hooks were successful' do
       it 'should call post-receive hook' do
-        hook = double(trigger: true)
+        hook = double(trigger: [true, nil])
         expect(Gitlab::Git::Hook).to receive(:new).exactly(3).times.and_return(hook)
 
-        expect(service.execute(user, @repo_path, @blankrev, @newrev, @ref) { }).to eq(true)
+        expect(service.execute(user, @repo_path, @blankrev, @newrev, @ref) { }).to eq([true, nil])
       end
     end
 
     context 'when pre-receive hook failed' do
       it 'should not call post-receive hook' do
-        expect(service).to receive(:run_hook).with('pre-receive').and_return(false)
+        expect(service).to receive(:run_hook).with('pre-receive').and_return([false, ''])
         expect(service).not_to receive(:run_hook).with('post-receive')
 
         expect do
@@ -38,8 +38,8 @@ describe GitHooksService, services: true do
 
     context 'when update hook failed' do
       it 'should not call post-receive hook' do
-        expect(service).to receive(:run_hook).with('pre-receive').and_return(true)
-        expect(service).to receive(:run_hook).with('update').and_return(false)
+        expect(service).to receive(:run_hook).with('pre-receive').and_return([true, nil])
+        expect(service).to receive(:run_hook).with('update').and_return([false, ''])
         expect(service).not_to receive(:run_hook).with('post-receive')
 
         expect do