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.

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

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

app/services/compare_service.rb

 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

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

changelogs/unreleased/40530-merge-request-generates-wrong-diff-when-branch-and-tag-have-the-same-name.yml

+---
+title: Fix merge requests where the source or target branch name matches a tag name
+merge_request: 15591
+author:
+type: fixed

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}"
         )
 

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")
 

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
 

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