diff options
author | Timothy Andrew <mail@timothyandrew.net> | 2016-06-17 11:21:17 +0530 |
---|---|---|
committer | Timothy Andrew <mail@timothyandrew.net> | 2016-07-05 11:05:16 +0530 |
commit | eb16e1e3c2614f385c4b992c919fd26768cfc3d8 (patch) | |
tree | edfa0da64f11cb814bde94eb0e1ea69674298cd6 | |
parent | 2a5cb7ec5259123cbbecb0577b9b4afacaf7546a (diff) | |
download | gitlab-ce-eb16e1e3c2614f385c4b992c919fd26768cfc3d8.tar.gz |
Improve the error message displayed when branch creation fails.
Note: This feature was developed independently on master while this was
in review. I've removed the conflicting bits and left the relevant
additions, mainly a test for `Gitlab::Git::Hook`. The original commit
message follows:
1. `gitlab-shell` outputs errors to `stderr`, but we weren't using this
information, prior to this commit. Now we capture the `stderr`, and
display it in the flash message when branch creation fails.
2. This can be used to display better errors for other git operation
failures with small tweaks.
3. The return value of `Gitlab::Git::Hook#trigger` is changed from a
simple `true`/`false` to a tuple of `[status, errors]`. All usages
and tests have been updated to reflect this change.
4. This is only relevant to branch creation _from the Web UI_, since SSH
and HTTP pushes access `gitlab-shell` either directly or through
`gitlab-workhorse`.
5. A few minor changes need to be made on the `gitlab-shell` end. Right
now, the `stderr` message it outputs is prefixed by "GitLab: ", which
shows up in our flash message. This is better removed.
-rw-r--r-- | app/views/projects/protected_branches/index.html.haml | 14 | ||||
-rw-r--r-- | lib/gitlab/git/hook.rb | 9 | ||||
-rw-r--r-- | spec/lib/gitlab/git/hook_spec.rb | 70 | ||||
-rw-r--r-- | spec/support/test_env.rb | 2 |
4 files changed, 81 insertions, 14 deletions
diff --git a/app/views/projects/protected_branches/index.html.haml b/app/views/projects/protected_branches/index.html.haml index 8eaef1f2904..75c27d85e9f 100644 --- a/app/views/projects/protected_branches/index.html.haml +++ b/app/views/projects/protected_branches/index.html.haml @@ -4,17 +4,17 @@ .col-lg-3 %h4.prepend-top-0 = page_title - %p Keep stable branches secure and force developers to use Merge Requests - .col-lg-9 - %h5.prepend-top-0 - Protect a branch - .account-well.append-bottom-default - %p.light-header.append-bottom-0 Protected branches are designed to + %p Keep stable branches secure and force developers to use merge requests. + %p.prepend-top-20 + Protected branches are designed to: %ul %li prevent pushes from everybody except #{link_to "masters", help_page_path("permissions", "permissions"), class: "vlink"} %li prevent anyone from force pushing to the branch %li prevent anyone from deleting the branch %p.append-bottom-0 Read more about #{link_to "project permissions", help_page_path("permissions", "permissions"), class: "underlined-link"} + .col-lg-9 + %h5.prepend-top-0 + Protect a branch - if can? current_user, :admin_project, @project = form_for [@project.namespace.becomes(Namespace), @project, @protected_branch] do |f| = form_errors(@protected_branch) @@ -35,7 +35,7 @@ = f.label :developers_can_push, "Developers can push", class: "label-light append-bottom-0" %p.light.append-bottom-0 Allow developers to push to this branch - = f.submit "Protect", class: "btn-create btn" + = f.submit "Protect", class: "btn-create btn protect-branch-btn", disabled: true %hr = render "branches_list" diff --git a/lib/gitlab/git/hook.rb b/lib/gitlab/git/hook.rb index 420c6883c45..db87d447358 100644 --- a/lib/gitlab/git/hook.rb +++ b/lib/gitlab/git/hook.rb @@ -14,7 +14,7 @@ module Gitlab end def trigger(gl_id, oldrev, newrev, ref) - return true unless exists? + return [true, nil] unless exists? case name when "pre-receive", "post-receive" @@ -68,13 +68,10 @@ module Gitlab end def call_update_hook(gl_id, oldrev, newrev, ref) - status = nil - Dir.chdir(repo_path) do - status = system({ 'GL_ID' => gl_id }, path, ref, oldrev, newrev) + stdout, stderr, status = Open3.capture3({ 'GL_ID' => gl_id }, path, ref, oldrev, newrev) + [status.success?, stderr.presence || stdout] end - - [status, nil] end def retrieve_error_message(stderr, stdout) diff --git a/spec/lib/gitlab/git/hook_spec.rb b/spec/lib/gitlab/git/hook_spec.rb new file mode 100644 index 00000000000..a15aa173fbd --- /dev/null +++ b/spec/lib/gitlab/git/hook_spec.rb @@ -0,0 +1,70 @@ +require 'spec_helper' +require 'fileutils' + +describe Gitlab::Git::Hook, lib: true do + describe "#trigger" do + let(:project) { create(:project) } + let(:user) { create(:user) } + + def create_hook(name) + FileUtils.mkdir_p(File.join(project.repository.path, 'hooks')) + File.open(File.join(project.repository.path, 'hooks', name), 'w', 0755) do |f| + f.write('exit 0') + end + end + + def create_failing_hook(name) + FileUtils.mkdir_p(File.join(project.repository.path, 'hooks')) + File.open(File.join(project.repository.path, 'hooks', name), 'w', 0755) do |f| + f.write(<<-HOOK) + echo 'regular message from the hook' + echo 'error message from the hook' 1>&2 + exit 1 + HOOK + end + end + + ['pre-receive', 'post-receive', 'update'].each do |hook_name| + + context "when triggering a #{hook_name} hook" do + context "when the hook is successful" do + it "returns success with no errors" do + create_hook(hook_name) + hook = Gitlab::Git::Hook.new(hook_name, project.repository.path) + blank = Gitlab::Git::BLANK_SHA + ref = Gitlab::Git::BRANCH_REF_PREFIX + 'new_branch' + + status, errors = hook.trigger(Gitlab::GlId.gl_id(user), blank, blank, ref) + expect(status).to be true + expect(errors).to be_blank + end + end + + context "when the hook is unsuccessful" do + it "returns failure with errors" do + create_failing_hook(hook_name) + hook = Gitlab::Git::Hook.new(hook_name, project.repository.path) + blank = Gitlab::Git::BLANK_SHA + ref = Gitlab::Git::BRANCH_REF_PREFIX + 'new_branch' + + status, errors = hook.trigger(Gitlab::GlId.gl_id(user), blank, blank, ref) + expect(status).to be false + expect(errors).to eq("error message from the hook\n") + end + end + end + end + + context "when the hook doesn't exist" do + it "returns success with no errors" do + hook = Gitlab::Git::Hook.new('unknown_hook', project.repository.path) + blank = Gitlab::Git::BLANK_SHA + ref = Gitlab::Git::BRANCH_REF_PREFIX + 'new_branch' + + status, errors = hook.trigger(Gitlab::GlId.gl_id(user), blank, blank, ref) + expect(status).to be true + expect(errors).to be_nil + end + end + end +end diff --git a/spec/support/test_env.rb b/spec/support/test_env.rb index 9f9ef20f99b..6b99b0f24cb 100644 --- a/spec/support/test_env.rb +++ b/spec/support/test_env.rb @@ -63,7 +63,7 @@ module TestEnv end def disable_pre_receive - 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]) end # Clean /tmp/tests |