kotovalexarian-likes-gitlab/gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Merge branch '40530-merge-request-generates-wrong-diff-when-branch-and-tag-have-the-same-name' into 'master'

Browse source 
Resolve "Merge request generates wrong diff when branch and tag have the same name"

Closes #40530

See merge request gitlab-org/gitlab-ce!15591
This commit is contained in:
Rémy Coutable 2017-11-28 18:01:09 +00:00
commit becf29e3ab
9 changed files with 99 additions and 21 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
app/controllers/projects/merge_requests/creations_controller.rb
View file

@ -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

18
app/models/merge_request.rb
View file

@ -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

12
app/services/compare_service.rb
View file

@ -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

16
app/services/merge_requests/build_service.rb
View file

@ -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

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

@ -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

8
lib/gitlab/git/repository.rb
View file

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

4
spec/lib/gitlab/diff/position_tracer_spec.rb
View file

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

21
spec/models/merge_request_spec.rb
View file

@ -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

32
spec/services/merge_requests/build_service_spec.rb
View file

@ -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