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

app/controllers/projects/issues_controller.rb

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

app/models/merge_request.rb

@@ -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
@@ -792,11 +793,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
 
@@ -939,4 +936,17 @@ class MergeRequest < ActiveRecord::Base
 
     true
   end
+
+  private
+
+  def write_ref
+    target_project.repository.with_repo_branch_commit(
+      source_project.repository, source_branch) do |commit|
+        if commit
+          target_project.repository.write_ref(ref_path, commit.sha)
+        else
+          raise Rugged::ReferenceError, 'source repository is empty'
+        end
+      end
+  end
 end

app/models/project.rb

@@ -1048,9 +1048,7 @@ class Project < ActiveRecord::Base
   def change_head(branch)
     if repository.branch_exists?(branch)
       repository.before_change_head
-      repository.rugged.references.create('HEAD',
-                                          "refs/heads/#{branch}",
-                                          force: true)
+      repository.write_ref('HEAD', "refs/heads/#{branch}")
       repository.copy_gitattributes(branch)
       repository.after_change_head
       reload_default_branch

app/models/repository.rb

@@ -224,7 +224,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
@@ -237,6 +237,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
@@ -985,12 +989,10 @@ class Repository
       if start_repository == self
         start_branch_name
       else
-        tmp_ref = "refs/tmp/#{SecureRandom.hex}/head"
-
-        fetch_ref(
+        tmp_ref = fetch_ref(
           start_repository.path_to_repo,
           "#{Gitlab::Git::BRANCH_REF_PREFIX}#{start_branch_name}",
-          tmp_ref
+          "refs/tmp/#{SecureRandom.hex}/head"
         )
 
         start_repository.commit(start_branch_name).sha
@@ -1021,7 +1023,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)

db/fixtures/development/10_merge_requests.rb

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

spec/controllers/projects/issues_controller_spec.rb

 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)

spec/factories/deployments.rb

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

spec/factories/merge_requests.rb

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

spec/features/merge_requests/filter_by_labels_spec.rb

 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)

spec/features/merge_requests/filter_merge_requests_spec.rb

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

spec/features/merge_requests/reset_filters_spec.rb

 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' }

spec/features/merge_requests/user_lists_merge_requests_spec.rb

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

spec/features/task_lists_spec.rb

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

spec/finders/environments_finder_spec.rb

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

spec/models/repository_spec.rb

@@ -961,6 +961,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' }
 

spec/requests/api/v3/merge_requests_spec.rb

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

spec/services/ci/create_pipeline_service_spec.rb

@@ -66,7 +66,7 @@ describe Ci::CreatePipelineService do
 
         context 'when there is no pipeline for source branch' do
           it "does not update merge request head pipeline" do
-            merge_request = create(:merge_request, source_branch: 'other_branch', target_branch: "branch_1", source_project: project)
+            merge_request = create(:merge_request, source_branch: 'feature', target_branch: "branch_1", source_project: project)
 
             head_pipeline = pipeline
 

spec/services/create_deployment_service_spec.rb

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

spec/services/issues/resolve_discussions_spec.rb

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

spec/services/merge_requests/get_urls_service_spec.rb

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

spec/support/issuables_list_metadata_shared_examples.rb

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