Commit eac5c565 authored by Douwe Maan's avatar Douwe Maan Committed by Lin Jen-Shin

Merge branch '36089-handle-ref-failure-better' into 'master'

Refactor how we fetch ref for merge requests

Closes #36089 and #36296

See merge request !13416
parent 4a75aac0
......@@ -212,7 +212,7 @@ class Projects::IssuesController < Projects::ApplicationController
end
def create_merge_request
result = MergeRequests::CreateFromIssueService.new(project, current_user, issue_iid: issue.iid).execute
result = ::MergeRequests::CreateFromIssueService.new(project, current_user, issue_iid: issue.iid).execute
if result[:status] == :success
render json: MergeRequestCreateSerializer.new.represent(result[:merge_request])
......
......@@ -443,7 +443,8 @@ class MergeRequest < ActiveRecord::Base
end
def reload_diff_if_branch_changed
if source_branch_changed? || target_branch_changed?
if (source_branch_changed? || target_branch_changed?) &&
(source_branch_head && target_branch_head)
reload_diff
end
end
......@@ -794,11 +795,7 @@ class MergeRequest < ActiveRecord::Base
end
def fetch_ref
target_project.repository.fetch_ref(
source_project.repository.path_to_repo,
"refs/heads/#{source_branch}",
ref_path
)
write_ref
update_column(:ref_fetched, true)
end
......
......@@ -1061,13 +1061,16 @@ class Project < ActiveRecord::Base
end
def change_head(branch)
repository.before_change_head
repository.rugged.references.create('HEAD',
"refs/heads/#{branch}",
force: true)
repository.copy_gitattributes(branch)
repository.after_change_head
reload_default_branch
if repository.branch_exists?(branch)
repository.before_change_head
repository.write_ref('HEAD', "refs/heads/#{branch}")
repository.copy_gitattributes(branch)
repository.after_change_head
reload_default_branch
else
errors.add(:base, "Could not change HEAD: branch '#{branch}' does not exist")
false
end
end
def forked_from?(project)
......
......@@ -232,7 +232,7 @@ class Repository
# This will still fail if the file is corrupted (e.g. 0 bytes)
begin
rugged.references.create(keep_around_ref_name(sha), sha, force: true)
write_ref(keep_around_ref_name(sha), sha)
rescue Rugged::ReferenceError => ex
Rails.logger.error "Unable to create keep-around reference for repository #{path}: #{ex}"
rescue Rugged::OSError => ex
......@@ -245,6 +245,10 @@ class Repository
ref_exists?(keep_around_ref_name(sha))
end
def write_ref(ref_path, sha)
rugged.references.create(ref_path, sha, force: true)
end
def diverging_commit_counts(branch)
root_ref_hash = raw_repository.rev_parse_target(root_ref).oid
cache.fetch(:"diverging_commit_counts_#{branch.name}") do
......@@ -1024,7 +1028,12 @@ class Repository
def fetch_ref(source_path, source_ref, target_ref)
args = %W(fetch --no-tags -f #{source_path} #{source_ref}:#{target_ref})
run_git(args)
message, status = run_git(args)
# Make sure ref was created, and raise Rugged::ReferenceError when not
raise Rugged::ReferenceError, message if status != 0
target_ref
end
def create_ref(ref, ref_path)
......
......@@ -28,6 +28,8 @@ Gitlab::Seeder.quiet do
project = Project.find_by_full_path('gitlab-org/gitlab-test')
next if project.empty_repo? # We don't have repository on CI
params = {
source_branch: 'feature',
target_branch: 'master',
......
require('spec_helper')
describe Projects::IssuesController do
let(:project) { create(:project_empty_repo) }
let(:project) { create(:project) }
let(:user) { create(:user) }
let(:issue) { create(:issue, project: project) }
......@@ -841,7 +841,7 @@ describe Projects::IssuesController do
describe 'POST #toggle_award_emoji' do
before do
sign_in(user)
project.team << [user, :developer]
project.add_developer(user)
end
it "toggles the award emoji" do
......@@ -855,6 +855,8 @@ describe Projects::IssuesController do
end
describe 'POST create_merge_request' do
let(:project) { create(:project, :repository) }
before do
project.add_developer(user)
sign_in(user)
......
......@@ -10,6 +10,10 @@ FactoryGirl.define do
after(:build) do |deployment, evaluator|
deployment.project ||= deployment.environment.project
unless deployment.project.repository_exists?
allow(deployment.project.repository).to receive(:fetch_ref)
end
end
end
end
......@@ -68,6 +68,17 @@ FactoryGirl.define do
merge_user author
end
after(:build) do |merge_request|
target_project = merge_request.target_project
source_project = merge_request.source_project
# Fake `write_ref` if we don't have repository
# We have too many existing tests replying on this behaviour
unless [target_project, source_project].all?(&:repository_exists?)
allow(merge_request).to receive(:write_ref)
end
end
factory :merged_merge_request, traits: [:merged]
factory :closed_merge_request, traits: [:closed]
factory :reopened_merge_request, traits: [:opened]
......
require 'rails_helper'
feature 'Merge Request filtering by Labels', js: true do
feature 'Merge Request filtering by Labels', :js do
include FilteredSearchHelpers
include MergeRequestHelpers
......@@ -12,9 +12,9 @@ feature 'Merge Request filtering by Labels', js: true do
let!(:feature) { create(:label, project: project, title: 'feature') }
let!(:enhancement) { create(:label, project: project, title: 'enhancement') }
let!(:mr1) { create(:merge_request, title: "Bugfix1", source_project: project, target_project: project, source_branch: "bugfix1") }
let!(:mr2) { create(:merge_request, title: "Bugfix2", source_project: project, target_project: project, source_branch: "bugfix2") }
let!(:mr3) { create(:merge_request, title: "Feature1", source_project: project, target_project: project, source_branch: "feature1") }
let!(:mr1) { create(:merge_request, title: "Bugfix1", source_project: project, target_project: project, source_branch: "fix") }
let!(:mr2) { create(:merge_request, title: "Bugfix2", source_project: project, target_project: project, source_branch: "wip") }
let!(:mr3) { create(:merge_request, title: "Feature1", source_project: project, target_project: project, source_branch: "improve/awesome") }
before do
mr1.labels << bug
......@@ -25,7 +25,7 @@ feature 'Merge Request filtering by Labels', js: true do
mr3.title = "Feature1"
mr3.labels << feature
project.team << [user, :master]
project.add_master(user)
sign_in(user)
visit project_merge_requests_path(project)
......
......@@ -12,7 +12,7 @@ describe 'Filter merge requests' do
let!(:wontfix) { create(:label, project: project, title: "Won't fix") }
before do
project.team << [user, :master]
project.add_master(user)
group.add_developer(user)
sign_in(user)
create(:merge_request, source_project: project, target_project: project)
......@@ -170,7 +170,7 @@ describe 'Filter merge requests' do
describe 'filter merge requests by text' do
before do
create(:merge_request, title: "Bug", source_project: project, target_project: project, source_branch: "bug")
create(:merge_request, title: "Bug", source_project: project, target_project: project, source_branch: "wip")
bug_label = create(:label, project: project, title: 'bug')
milestone = create(:milestone, title: "8", project: project)
......@@ -179,7 +179,7 @@ describe 'Filter merge requests' do
title: "Bug 2",
source_project: project,
target_project: project,
source_branch: "bug2",
source_branch: "fix",
milestone: milestone,
author: user,
assignee: user)
......@@ -259,12 +259,12 @@ describe 'Filter merge requests' do
end
end
describe 'filter merge requests and sort', js: true do
describe 'filter merge requests and sort', :js do
before do
bug_label = create(:label, project: project, title: 'bug')
mr1 = create(:merge_request, title: "Frontend", source_project: project, target_project: project, source_branch: "Frontend")
mr2 = create(:merge_request, title: "Bug 2", source_project: project, target_project: project, source_branch: "bug2")
mr1 = create(:merge_request, title: "Frontend", source_project: project, target_project: project, source_branch: "wip")
mr2 = create(:merge_request, title: "Bug 2", source_project: project, target_project: project, source_branch: "fix")
mr1.labels << bug_label
mr2.labels << bug_label
......
require 'rails_helper'
feature 'Merge requests filter clear button', js: true do
feature 'Merge requests filter clear button', :js do
include FilteredSearchHelpers
include MergeRequestHelpers
include IssueHelpers
......@@ -9,8 +9,8 @@ feature 'Merge requests filter clear button', js: true do
let!(:user) { create(:user) }
let!(:milestone) { create(:milestone, project: project) }
let!(:bug) { create(:label, project: project, name: 'bug')}
let!(:mr1) { create(:merge_request, title: "Feature", source_project: project, target_project: project, source_branch: "Feature", milestone: milestone, author: user, assignee: user) }
let!(:mr2) { create(:merge_request, title: "Bugfix1", source_project: project, target_project: project, source_branch: "Bugfix1") }
let!(:mr1) { create(:merge_request, title: "Feature", source_project: project, target_project: project, source_branch: "improve/awesome", milestone: milestone, author: user, assignee: user) }
let!(:mr2) { create(:merge_request, title: "Bugfix1", source_project: project, target_project: project, source_branch: "fix") }
let(:merge_request_css) { '.merge-request' }
let(:clear_search_css) { '.filtered-search-box .clear-search' }
......
......@@ -24,12 +24,10 @@ describe 'Projects > Merge requests > User lists merge requests' do
milestone: create(:milestone, due_date: '2013-12-12'),
created_at: 2.minutes.ago,
updated_at: 2.minutes.ago)
# lfs in itself is not a great choice for the title if one wants to match the whole body content later on
# just think about the scenario when faker generates 'Chester Runolfsson' as the user's name
create(:merge_request,
title: 'merge_lfs',
title: 'merge-test',
source_project: project,
source_branch: 'merge_lfs',
source_branch: 'merge-test',
created_at: 3.minutes.ago,
updated_at: 10.seconds.ago)
end
......@@ -38,7 +36,7 @@ describe 'Projects > Merge requests > User lists merge requests' do
visit_merge_requests(project, assignee_id: IssuableFinder::NONE)
expect(current_path).to eq(project_merge_requests_path(project))
expect(page).to have_content 'merge_lfs'
expect(page).to have_content 'merge-test'
expect(page).not_to have_content 'fix'
expect(page).not_to have_content 'markdown'
expect(count_merge_requests).to eq(1)
......@@ -47,7 +45,7 @@ describe 'Projects > Merge requests > User lists merge requests' do
it 'filters on a specific assignee' do
visit_merge_requests(project, assignee_id: user.id)
expect(page).not_to have_content 'merge_lfs'
expect(page).not_to have_content 'merge-test'
expect(page).to have_content 'fix'
expect(page).to have_content 'markdown'
expect(count_merge_requests).to eq(2)
......@@ -57,14 +55,14 @@ describe 'Projects > Merge requests > User lists merge requests' do
visit_merge_requests(project, sort: sort_value_recently_created)
expect(first_merge_request).to include('fix')
expect(last_merge_request).to include('merge_lfs')
expect(last_merge_request).to include('merge-test')
expect(count_merge_requests).to eq(3)
end
it 'sorts by oldest' do
visit_merge_requests(project, sort: sort_value_oldest_created)
expect(first_merge_request).to include('merge_lfs')
expect(first_merge_request).to include('merge-test')
expect(last_merge_request).to include('fix')
expect(count_merge_requests).to eq(3)
end
......@@ -72,7 +70,7 @@ describe 'Projects > Merge requests > User lists merge requests' do
it 'sorts by last updated' do
visit_merge_requests(project, sort: sort_value_recently_updated)
expect(first_merge_request).to include('merge_lfs')
expect(first_merge_request).to include('merge-test')
expect(count_merge_requests).to eq(3)
end
......
......@@ -52,8 +52,8 @@ feature 'Task Lists' do
before do
Warden.test_mode!
project.team << [user, :master]
project.team << [user2, :guest]
project.add_master(user)
project.add_guest(user2)
login_as(user)
end
......
......@@ -12,7 +12,7 @@ describe EnvironmentsFinder do
context 'tagged deployment' do
before do
create(:deployment, environment: environment, ref: '1.0', tag: true, sha: project.commit.id)
create(:deployment, environment: environment, ref: 'v1.1.0', tag: true, sha: project.commit.id)
end
it 'returns environment when with_tags is set' do
......
......@@ -995,6 +995,27 @@ describe Repository, models: true do
end
end
context 'when temporary ref failed to be created from other project' do
let(:target_project) { create(:project, :empty_repo) }
before do
expect(target_project.repository).to receive(:run_git)
end
it 'raises Rugged::ReferenceError' do
raise_reference_error = raise_error(Rugged::ReferenceError) do |err|
expect(err.cause).to be_nil
end
expect do
GitOperationService.new(user, target_project.repository)
.with_branch('feature',
start_project: project,
&:itself)
end.to raise_reference_error
end
end
context 'when the update adds more than one commit' do
let(:old_rev) { '33f3729a45c02fc67d00adb1b8bca394b0e761d9' }
......
This diff is collapsed.
......@@ -315,15 +315,17 @@ describe API::MergeRequests do
let!(:fork_project) { create(:project, forked_from_project: project, namespace: user2.namespace, creator_id: user2.id) }
let!(:unrelated_project) { create(:project, namespace: create(:user).namespace, creator_id: user2.id) }
before :each do |each|
fork_project.team << [user2, :reporter]
before do
fork_project.add_reporter(user2)
allow_any_instance_of(MergeRequest).to receive(:write_ref)
end
it "returns merge_request" do
post v3_api("/projects/#{fork_project.id}/merge_requests", user2),
title: 'Test merge_request', source_branch: "feature_conflict", target_branch: "master",
author: user2, target_project_id: project.id, description: 'Test description for Test merge_request'
expect(response).to have_http_status(201)
expect(response).to have_gitlab_http_status(201)
expect(json_response['title']).to eq('Test merge_request')
expect(json_response['description']).to eq('Test description for Test merge_request')
end
......@@ -334,7 +336,7 @@ describe API::MergeRequests do
expect(fork_project.forked_from_project).to eq(project)
post v3_api("/projects/#{fork_project.id}/merge_requests", user2),
title: 'Test merge_request', source_branch: "master", target_branch: "master", author: user2, target_project_id: project.id
expect(response).to have_http_status(201)
expect(response).to have_gitlab_http_status(201)
expect(json_response['title']).to eq('Test merge_request')
end
......@@ -348,25 +350,25 @@ describe API::MergeRequests do
author: user2,
target_project_id: project.id
expect(response).to have_http_status(422)
expect(response).to have_gitlab_http_status(422)
end
it "returns 400 when source_branch is missing" do
post v3_api("/projects/#{fork_project.id}/merge_requests", user2),
title: 'Test merge_request', target_branch: "master", author: user2, target_project_id: project.id
expect(response).to have_http_status(400)
expect(response).to have_gitlab_http_status(400)
end
it "returns 400 when target_branch is missing" do
post v3_api("/projects/#{fork_project.id}/merge_requests", user2),
title: 'Test merge_request', target_branch: "master", author: user2, target_project_id: project.id
expect(response).to have_http_status(400)
expect(response).to have_gitlab_http_status(400)
end
it "returns 400 when title is missing" do
post v3_api("/projects/#{fork_project.id}/merge_requests", user2),
target_branch: 'master', source_branch: 'markdown', author: user2, target_project_id: project.id
expect(response).to have_http_status(400)
expect(response).to have_gitlab_http_status(400)
end
context 'when target_branch is specified' do
......@@ -377,7 +379,7 @@ describe API::MergeRequests do
source_branch: 'markdown',
author: user,
target_project_id: fork_project.id
expect(response).to have_http_status(422)
expect(response).to have_gitlab_http_status(422)
end
it 'returns 422 if targeting a different fork' do
......@@ -387,14 +389,14 @@ describe API::MergeRequests do
source_branch: 'markdown',
author: user2,
target_project_id: unrelated_project.id
expect(response).to have_http_status(422)
expect(response).to have_gitlab_http_status(422)
end
end
it "returns 201 when target_branch is specified and for the same project" do
post v3_api("/projects/#{fork_project.id}/merge_requests", user2),
title: 'Test merge_request', target_branch: 'master', source_branch: 'markdown', author: user2, target_project_id: fork_project.id
expect(response).to have_http_status(201)
expect(response).to have_gitlab_http_status(201)
end
end
end
......
......@@ -20,6 +20,10 @@ describe CreateDeploymentService do
let(:service) { described_class.new(job) }
before do
allow_any_instance_of(Deployment).to receive(:create_ref)
end
describe '#execute' do
subject { service.execute }
......
......@@ -20,7 +20,7 @@ describe Issues::ResolveDiscussions do
describe "for resolving discussions" do
let(:discussion) { create(:diff_note_on_merge_request, project: project, note: "Almost done").to_discussion }
let(:merge_request) { discussion.noteable }
let(:other_merge_request) { create(:merge_request, source_project: project, source_branch: "other") }
let(:other_merge_request) { create(:merge_request, source_project: project, source_branch: "fix") }
describe "#merge_request_for_resolving_discussion" do
let(:service) { DummyService.new(project, user, merge_request_to_resolve_discussions_of: merge_request.iid) }
......
......@@ -3,7 +3,7 @@ require "spec_helper"
describe MergeRequests::GetUrlsService do
let(:project) { create(:project, :public, :repository) }
let(:service) { described_class.new(project) }
let(:source_branch) { "my_branch" }
let(:source_branch) { "merge-test" }
let(:new_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=#{source_branch}" }
let(:show_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/#{merge_request.iid}" }
let(:new_branch_changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" }
......@@ -111,9 +111,9 @@ describe MergeRequests::GetUrlsService do
end
context 'pushing new branch and existing branch (with merge request created) at once' do
let!(:merge_request) { create(:merge_request, source_project: project, source_branch: "existing_branch") }
let!(:merge_request) { create(:merge_request, source_project: project, source_branch: "markdown") }
let(:new_branch_changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/new_branch" }
let(:existing_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/existing_branch" }
let(:existing_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/markdown" }
let(:changes) { "#{new_branch_changes}\n#{existing_branch_changes}" }
let(:new_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch" }
......@@ -124,7 +124,7 @@ describe MergeRequests::GetUrlsService do
url: new_merge_request_url,
new_merge_request: true
}, {
branch_name: "existing_branch",
branch_name: "markdown",
url: show_merge_request_url,
new_merge_request: false
}])
......
......@@ -2,12 +2,12 @@ shared_examples 'issuables list meta-data' do |issuable_type, action = nil|
before do
@issuable_ids = []
2.times do |n|
%w[fix improve/awesome].each do |source_branch|
issuable =
if issuable_type == :issue
create(issuable_type, project: project)
else
create(issuable_type, source_project: project, source_branch: "#{n}-feature")
create(issuable_type, source_project: project, source_branch: source_branch)
end
@issuable_ids << issuable.id
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment