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 e4d8f486c77..a6b10d5349c 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -364,16 +364,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