From 7cf6f50f01ef1285471e2e930d1f15b51e873666 Mon Sep 17 00:00:00 2001 From: Valery Sizov <valery@gitlab.com> Date: Fri, 15 Jul 2016 15:54:44 +0300 Subject: [PATCH] Fix of 'Commits being passed to custom hooks already reachable when using the UI' --- CHANGELOG | 1 + Gemfile | 2 +- Gemfile.lock | 18 ++++--- app/models/repository.rb | 76 +++++++++++---------------- app/services/create_branch_service.rb | 28 +++++----- 5 files changed, 57 insertions(+), 68 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index f254421a66b6f..c50c6640feb92 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -98,6 +98,7 @@ v 8.10.0 (unreleased) - Change status color and icon for running builds - Fix markdown rendering for: consecutive labels references, label references that begin with a digit or contains `.` - Fix last update timestamp on issues not preserved on gitlab.com and project imports + - Fix of 'Commits being passed to custom hooks are already reachable when using the UI' v 8.9.6 - Fix importing of events under notes for GitLab projects. !5154 diff --git a/Gemfile b/Gemfile index 8db71d91fb268..c8415d5e0f32a 100644 --- a/Gemfile +++ b/Gemfile @@ -56,7 +56,7 @@ gem 'browser', '~> 2.2' # Extracting information from a git repository # Provide access to Gitlab::Git library -gem 'gitlab_git', '~> 10.2' +gem 'gitlab_git', '~> 10.3' # LDAP Auth # GitLab fork with several improvements to original library. For full list of changes diff --git a/Gemfile.lock b/Gemfile.lock index 6366caa2b149f..2547e6f0585a4 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,3 +1,14 @@ +GIT + remote: https://gitlab.com/gitlab-org/gitlab_git.git + revision: 0df0cdd9bb1164a7595f1f69f5dfa79489e3eaf1 + branch: optional_branch_update + specs: + gitlab_git (10.3.0) + activesupport (~> 4.0) + charlock_holmes (~> 0.7.3) + github-linguist (~> 4.7.0) + rugged (~> 0.24.0) + GEM remote: https://rubygems.org/ specs: @@ -296,11 +307,6 @@ GEM mime-types (>= 1.16, < 3) posix-spawn (~> 0.3) gitlab-license (1.0.0) - gitlab_git (10.2.3) - activesupport (~> 4.0) - charlock_holmes (~> 0.7.3) - github-linguist (~> 4.7.0) - rugged (~> 0.24.0) gitlab_meta (7.0) gitlab_omniauth-ldap (1.2.1) net-ldap (~> 0.9) @@ -892,7 +898,7 @@ DEPENDENCIES gitlab-elasticsearch-git (~> 0.0.15) gitlab-flowdock-git-hook (~> 1.0.1) gitlab-license (~> 1.0) - gitlab_git (~> 10.2) + gitlab_git! gitlab_meta (= 7.0) gitlab_omniauth-ldap (~> 1.2.1) gollum-lib (~> 4.1.0) diff --git a/app/models/repository.rb b/app/models/repository.rb index f0d78f6116dd4..47887b24d8067 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -783,6 +783,7 @@ def commit_dir(user, path, message, branch) options[:commit] = { message: message, branch: ref, + update_ref: false } raw_repository.mkdir(path, options) @@ -798,6 +799,7 @@ def commit_file(user, path, content, message, branch, update) options[:commit] = { message: message, branch: ref, + update_ref: false } options[:file] = { @@ -818,7 +820,8 @@ def remove_file(user, path, message, branch) options[:author] = committer options[:commit] = { message: message, - branch: ref + branch: ref, + update_ref: false } options[:file] = { @@ -855,7 +858,7 @@ def ff_merge(user, source_sha, target_branch, options = {}) raise "Invalid merge target" if our_commit.nil? raise "Invalid merge source" if their_commit.nil? - commit_with_hooks(user, target_branch) do |ref| + commit_with_hooks(user, target_branch) do source_sha end end @@ -870,11 +873,10 @@ def merge(user, merge_request, options = {}) merge_index = rugged.merge_commits(our_commit, their_commit) return false if merge_index.conflicts? - commit_with_hooks(user, merge_request.target_branch) do |tmp_ref| + commit_with_hooks(user, merge_request.target_branch) do actual_options = options.merge( parents: [our_commit, their_commit], - tree: merge_index.write_tree(rugged), - update_ref: tmp_ref + tree: merge_index.write_tree(rugged) ) commit_id = Rugged::Commit.create(rugged, actual_options) @@ -889,15 +891,14 @@ def revert(user, commit, base_branch, revert_tree_id = nil) return false unless revert_tree_id - commit_with_hooks(user, base_branch) do |ref| + commit_with_hooks(user, base_branch) do committer = user_to_committer(user) source_sha = Rugged::Commit.create(rugged, message: commit.revert_message, author: committer, committer: committer, tree: revert_tree_id, - parents: [rugged.lookup(source_sha)], - update_ref: ref) + parents: [rugged.lookup(source_sha)]) end end @@ -907,7 +908,7 @@ def cherry_pick(user, commit, base_branch, cherry_pick_tree_id = nil) return false unless cherry_pick_tree_id - commit_with_hooks(user, base_branch) do |ref| + commit_with_hooks(user, base_branch) do committer = user_to_committer(user) source_sha = Rugged::Commit.create(rugged, message: commit.message, @@ -918,8 +919,7 @@ def cherry_pick(user, commit, base_branch, cherry_pick_tree_id = nil) }, committer: committer, tree: cherry_pick_tree_id, - parents: [rugged.lookup(source_sha)], - update_ref: ref) + parents: [rugged.lookup(source_sha)]) end end @@ -1119,20 +1119,6 @@ def fetch_ref(source_path, source_ref, target_ref) Gitlab::Popen.popen(args, path_to_repo) end - def with_tmp_ref(oldrev = nil) - random_string = SecureRandom.hex - tmp_ref = "refs/tmp/#{random_string}/head" - - if oldrev && !Gitlab::Git.blank_ref?(oldrev) - rugged.references.create(tmp_ref, oldrev) - end - - # Make commit in tmp ref - yield(tmp_ref) - ensure - rugged.references.delete(tmp_ref) rescue nil - end - def commit_with_hooks(current_user, branch) update_autocrlf_option @@ -1145,33 +1131,31 @@ def commit_with_hooks(current_user, branch) oldrev = target_branch.target end - with_tmp_ref(oldrev) do |tmp_ref| - # Make commit in tmp ref - newrev = yield(tmp_ref) + # Make commit + newrev = yield(ref) - unless newrev - raise CommitError.new('Failed to create commit') - end + unless newrev + raise CommitError.new('Failed to create commit') + end + + GitHooksService.new.execute(current_user, path_to_repo, oldrev, newrev, ref) do + if was_empty || !target_branch + # Create branch + rugged.references.create(ref, newrev) + else + # Update head + current_head = find_branch(branch).target - GitHooksService.new.execute(current_user, path_to_repo, oldrev, newrev, ref) do - if was_empty || !target_branch - # Create branch - rugged.references.create(ref, newrev) + # Make sure target branch was not changed during pre-receive hook + if current_head == oldrev + rugged.references.update(ref, newrev) else - # Update head - current_head = find_branch(branch).target - - # Make sure target branch was not changed during pre-receive hook - if current_head == oldrev - rugged.references.update(ref, newrev) - else - raise CommitError.new('Commit was rejected because branch received new push') - end + raise CommitError.new('Commit was rejected because branch received new push') end end - - newrev end + + newrev end def ls_files(ref) diff --git a/app/services/create_branch_service.rb b/app/services/create_branch_service.rb index d874582d54ffe..757fc35a78fdf 100644 --- a/app/services/create_branch_service.rb +++ b/app/services/create_branch_service.rb @@ -15,21 +15,19 @@ def execute(branch_name, ref, source_project: @project) return error('Branch already exists') end - new_branch = nil - - if source_project != @project - repository.with_tmp_ref do |tmp_ref| - repository.fetch_ref( - source_project.repository.path_to_repo, - "refs/heads/#{ref}", - tmp_ref - ) - - new_branch = repository.add_branch(current_user, branch_name, tmp_ref) - end - else - new_branch = repository.add_branch(current_user, branch_name, ref) - end + new_branch = if source_project != @project + repository.fetch_ref( + source_project.repository.path_to_repo, + "refs/heads/#{ref}", + "refs/heads/#{branch_name}" + ) + + repository.after_create_branch + + repository.find_branch(branch_name) + else + repository.add_branch(current_user, branch_name, ref) + end if new_branch success(new_branch) -- GitLab