Ensure MRs always use branch refs for comparison40530-merge-request-generates-wrong-diff-when-branch-and-tag-have-the-same-name

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.

9 files changed, 103 insertions, 21 deletions

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..b6b32be5211 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,25 @@ 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 }
+
+        before do
+          subject.source_project.repository.add_tag(subject.author,
+                                                    tag_name,
+                                                    subject.target_branch_sha,
+                                                    'Add a tag')
+        end
+
+        after do
+          subject.source_project.repository.rm_tag(subject.author, tag_name)
+        end
+
+        it 'returns the sha of the source branch last commit' do
+          expect(subject.source_branch_sha).to eq(last_branch_commit.sha)
+        end
+      end
     end
 
     context 'when the merge request is being created' do
@@ -933,7 +952,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 +960,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