From 3c6a4d63636ba41dad0ce63cf536761fc3b5ef64 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 24 Nov 2017 11:58:05 +0000 Subject: [PATCH] Ensure MRs always use branch refs for comparison If a merge request was created with a branch name that also matched a tag name, we'd generate a comparison to or from the tag respectively, rather than the branch. Merging would still use the branch, of course. To avoid this, ensure that when we get the branch heads, we prepend the reference prefix for branches, which will ensure that we generate the correct comparison. --- .../merge_requests/creations_controller.rb | 4 +-- app/models/merge_request.rb | 18 +++++++++-- app/services/compare_service.rb | 12 +++---- app/services/merge_requests/build_service.rb | 16 ++++++++-- ...when-branch-and-tag-have-the-same-name.yml | 5 +++ lib/gitlab/git/repository.rb | 8 ++++- spec/lib/gitlab/diff/position_tracer_spec.rb | 4 +-- spec/models/merge_request_spec.rb | 21 ++++++++++-- .../merge_requests/build_service_spec.rb | 32 ++++++++++++++++++- 9 files changed, 99 insertions(+), 21 deletions(-) create mode 100644 changelogs/unreleased/40530-merge-request-generates-wrong-diff-when-branch-and-tag-have-the-same-name.yml diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb index 764a9c7111e..1511fc08c89 100644 --- a/app/controllers/projects/merge_requests/creations_controller.rb +++ b/app/controllers/projects/merge_requests/creations_controller.rb @@ -65,7 +65,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap if params[:ref].present? @ref = params[:ref] - @commit = @repository.commit("refs/heads/#{@ref}") + @commit = @repository.commit(Gitlab::Git::BRANCH_REF_PREFIX + @ref) end render layout: false @@ -76,7 +76,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap if params[:ref].present? @ref = params[:ref] - @commit = @target_project.commit("refs/heads/#{@ref}") + @commit = @target_project.commit(Gitlab::Git::BRANCH_REF_PREFIX + @ref) end render layout: false diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 2753e4b16e5..57b169631f9 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -365,16 +365,28 @@ class MergeRequest < ActiveRecord::Base # We use these attributes to force these to the intended values. attr_writer :target_branch_sha, :source_branch_sha + def source_branch_ref + return @source_branch_sha if @source_branch_sha + return unless source_branch + + Gitlab::Git::BRANCH_REF_PREFIX + source_branch + end + + def target_branch_ref + return @target_branch_sha if @target_branch_sha + return unless target_branch + + Gitlab::Git::BRANCH_REF_PREFIX + target_branch + end + def source_branch_head return unless source_project - source_branch_ref = @source_branch_sha || source_branch source_project.repository.commit(source_branch_ref) if source_branch_ref end def target_branch_head - target_branch_ref = @target_branch_sha || target_branch - target_project.repository.commit(target_branch_ref) if target_branch_ref + target_project.repository.commit(target_branch_ref) end def branch_merge_base_commit diff --git a/app/services/compare_service.rb b/app/services/compare_service.rb index 53f16a236d2..1db91c3c90c 100644 --- a/app/services/compare_service.rb +++ b/app/services/compare_service.rb @@ -1,17 +1,17 @@ require 'securerandom' -# Compare 2 branches for one repo or between repositories +# Compare 2 refs for one repo or between repositories # and return Gitlab::Git::Compare object that responds to commits and diffs class CompareService - attr_reader :start_project, :start_branch_name + attr_reader :start_project, :start_ref_name - def initialize(new_start_project, new_start_branch_name) + def initialize(new_start_project, new_start_ref_name) @start_project = new_start_project - @start_branch_name = new_start_branch_name + @start_ref_name = new_start_ref_name end - def execute(target_project, target_branch, straight: false) - raw_compare = target_project.repository.compare_source_branch(target_branch, start_project.repository, start_branch_name, straight: straight) + def execute(target_project, target_ref, straight: false) + raw_compare = target_project.repository.compare_source_branch(target_ref, start_project.repository, start_ref_name, straight: straight) Compare.new(raw_compare, target_project, straight: straight) if raw_compare end diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index f3b99e1ec8c..8223e5fed7f 100644 --- a/app/services/merge_requests/build_service.rb +++ b/app/services/merge_requests/build_service.rb @@ -18,7 +18,17 @@ module MergeRequests attr_accessor :merge_request - delegate :target_branch, :source_branch, :source_project, :target_project, :compare_commits, :wip_title, :description, :errors, to: :merge_request + delegate :target_branch, + :target_branch_ref, + :target_project, + :source_branch, + :source_branch_ref, + :source_project, + :compare_commits, + :wip_title, + :description, + :errors, + to: :merge_request def find_source_project return source_project if source_project.present? && can?(current_user, :read_project, source_project) @@ -54,10 +64,10 @@ module MergeRequests def compare_branches compare = CompareService.new( source_project, - source_branch + source_branch_ref ).execute( target_project, - target_branch + target_branch_ref ) if compare diff --git a/changelogs/unreleased/40530-merge-request-generates-wrong-diff-when-branch-and-tag-have-the-same-name.yml b/changelogs/unreleased/40530-merge-request-generates-wrong-diff-when-branch-and-tag-have-the-same-name.yml new file mode 100644 index 00000000000..e9fae6fe0d7 --- /dev/null +++ b/changelogs/unreleased/40530-merge-request-generates-wrong-diff-when-branch-and-tag-have-the-same-name.yml @@ -0,0 +1,5 @@ +--- +title: Fix merge requests where the source or target branch name matches a tag name +merge_request: 15591 +author: +type: fixed diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index a6e7c410bdd..d399636bb28 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -1046,9 +1046,15 @@ module Gitlab end def with_repo_tmp_commit(start_repository, start_branch_name, sha) + source_ref = start_branch_name + + unless Gitlab::Git.branch_ref?(source_ref) + source_ref = "#{Gitlab::Git::BRANCH_REF_PREFIX}#{source_ref}" + end + tmp_ref = fetch_ref( start_repository, - source_ref: "#{Gitlab::Git::BRANCH_REF_PREFIX}#{start_branch_name}", + source_ref: source_ref, target_ref: "refs/tmp/#{SecureRandom.hex}" ) diff --git a/spec/lib/gitlab/diff/position_tracer_spec.rb b/spec/lib/gitlab/diff/position_tracer_spec.rb index e5138705443..ddc4f6c5b5c 100644 --- a/spec/lib/gitlab/diff/position_tracer_spec.rb +++ b/spec/lib/gitlab/diff/position_tracer_spec.rb @@ -1771,9 +1771,9 @@ describe Gitlab::Diff::PositionTracer do describe "merge of target branch" do let(:merge_commit) do - update_file_again_commit + second_create_file_commit - merge_request = create(:merge_request, source_branch: second_create_file_commit.sha, target_branch: branch_name, source_project: project) + merge_request = create(:merge_request, source_branch: second_branch_name, target_branch: branch_name, source_project: project) repository.merge(current_user, merge_request.diff_head_sha, merge_request, "Merge branches") diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 3cf8fc816ff..728028746d8 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -259,7 +259,7 @@ describe MergeRequest do end describe '#source_branch_sha' do - let(:last_branch_commit) { subject.source_project.repository.commit(subject.source_branch) } + let(:last_branch_commit) { subject.source_project.repository.commit(Gitlab::Git::BRANCH_REF_PREFIX + subject.source_branch) } context 'with diffs' do subject { create(:merge_request, :with_diffs) } @@ -273,6 +273,21 @@ describe MergeRequest do it 'returns the sha of the source branch last commit' do expect(subject.source_branch_sha).to eq(last_branch_commit.sha) end + + context 'when there is a tag name matching the branch name' do + let(:tag_name) { subject.source_branch } + + it 'returns the sha of the source branch last commit' do + subject.source_project.repository.add_tag(subject.author, + tag_name, + subject.target_branch_sha, + 'Add a tag') + + expect(subject.source_branch_sha).to eq(last_branch_commit.sha) + + subject.source_project.repository.rm_tag(subject.author, tag_name) + end + end end context 'when the merge request is being created' do @@ -933,7 +948,7 @@ describe MergeRequest do context 'with a completely different branch' do before do - subject.update(target_branch: 'v1.0.0') + subject.update(target_branch: 'csv') end it_behaves_like 'returning all SHA' @@ -941,7 +956,7 @@ describe MergeRequest do context 'with a branch having no difference' do before do - subject.update(target_branch: 'v1.1.0') + subject.update(target_branch: 'branch-merged') subject.reload # make sure commits were not cached end diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index b46c419de14..fee293760f5 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -29,13 +29,27 @@ describe MergeRequests::BuildService do before do project.team << [user, :guest] + end + def stub_compare allow(CompareService).to receive_message_chain(:new, :execute).and_return(compare) allow(project).to receive(:commit).and_return(commit_1) allow(project).to receive(:commit).and_return(commit_2) end - describe 'execute' do + describe '#execute' do + it 'calls the compare service with the correct arguments' do + expect(CompareService).to receive(:new) + .with(project, Gitlab::Git::BRANCH_REF_PREFIX + source_branch) + .and_call_original + + expect_any_instance_of(CompareService).to receive(:execute) + .with(project, Gitlab::Git::BRANCH_REF_PREFIX + target_branch) + .and_call_original + + merge_request + end + context 'missing source branch' do let(:source_branch) { '' } @@ -52,6 +66,10 @@ describe MergeRequests::BuildService do let(:target_branch) { nil } let(:commits) { Commit.decorate([commit_1], project) } + before do + stub_compare + end + it 'creates compare object with target branch as default branch' do expect(merge_request.compare).to be_present expect(merge_request.target_branch).to eq(project.default_branch) @@ -77,6 +95,10 @@ describe MergeRequests::BuildService do context 'no commits in the diff' do let(:commits) { [] } + before do + stub_compare + end + it 'allows the merge request to be created' do expect(merge_request.can_be_created).to eq(true) end @@ -89,6 +111,10 @@ describe MergeRequests::BuildService do context 'one commit in the diff' do let(:commits) { Commit.decorate([commit_1], project) } + before do + stub_compare + end + it 'allows the merge request to be created' do expect(merge_request.can_be_created).to eq(true) end @@ -149,6 +175,10 @@ describe MergeRequests::BuildService do context 'more than one commit in the diff' do let(:commits) { Commit.decorate([commit_1, commit_2], project) } + before do + stub_compare + end + it 'allows the merge request to be created' do expect(merge_request.can_be_created).to eq(true) end