Merge branch 'git_hooks_better_message' into 'master'

More descriptive message for git hooks and file locks

## What does this MR do?
Most of the benefits will be in GitLab EE because we have git hooks there (push rules) and file lock features.
Instead of showing "Git operation was rejected by #{hook_name} hook" the actual problem description will be shown.
Also you will see a clear message when you edit locked file.

## Why was this MR needed?

Because we don't want to confuse our users

## What are the relevant issue numbers?

https://gitlab.com/gitlab-org/gitlab-ee/issues/713


See merge request !5067

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