From bf218337ed5bd535856204ef1878a1495fa37dfe Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Mon, 4 Jul 2016 15:30:22 +0300 Subject: [PATCH] Better message for git hooks and file locks --- CHANGELOG | 1 + app/services/create_branch_service.rb | 4 ++-- app/services/create_tag_service.rb | 4 ++-- app/services/delete_branch_service.rb | 4 ++-- app/services/git_hooks_service.rb | 6 ++++-- lib/gitlab/git/hook.rb | 13 +++++++++---- spec/models/repository_spec.rb | 18 +++++++++--------- spec/services/create_tag_service_spec.rb | 4 ++-- spec/services/git_hooks_service_spec.rb | 10 +++++----- 9 files changed, 36 insertions(+), 28 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 5f593336895..3b89fa05801 100644 --- a/CHANGELOG +++ b/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 diff --git a/app/services/create_branch_service.rb b/app/services/create_branch_service.rb index 9f4481a8153..cc128563437 100644 --- a/app/services/create_branch_service.rb +++ b/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) diff --git a/app/services/create_tag_service.rb b/app/services/create_tag_service.rb index 91ed0e354d0..bd8d982e1fb 100644 --- a/app/services/create_tag_service.rb +++ b/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 diff --git a/app/services/delete_branch_service.rb b/app/services/delete_branch_service.rb index fae069ee4a5..752a7029952 100644 --- a/app/services/delete_branch_service.rb +++ b/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) diff --git a/app/services/git_hooks_service.rb b/app/services/git_hooks_service.rb index d7a0c25a044..172bd85dade 100644 --- a/app/services/git_hooks_service.rb +++ b/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 diff --git a/lib/gitlab/git/hook.rb b/lib/gitlab/git/hook.rb index 07b856ca64c..5415f4844d3 100644 --- a/lib/gitlab/git/hook.rb +++ b/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 diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index e753306a31f..7975fc64e59 100644 --- a/spec/models/repository_spec.rb +++ b/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 } diff --git a/spec/services/create_tag_service_spec.rb b/spec/services/create_tag_service_spec.rb index 91f9e663b66..7dc43c50b0d 100644 --- a/spec/services/create_tag_service_spec.rb +++ b/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 diff --git a/spec/services/git_hooks_service_spec.rb b/spec/services/git_hooks_service_spec.rb index 6367ac832e8..3fc37a315c0 100644 --- a/spec/services/git_hooks_service_spec.rb +++ b/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