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

* 36089-handle-ref-failure-better:
  Skip creating the merge request if repo is empty
  Just use repository would fix the test
  Since now fetch_ref is reliable, we could just rely on it
  Just use the repo. Not sure why master could pass
  Fix more tests
  Don't try to create diffs if one of the branch is missing
  Try to show exception information in the test
  Introduce MergeRequest#write_ref and Repository#write_ref
  Avoid ambiguity, which happened in a single test run
  Just detect exit status rather than check ref
  Fix some tests and report the error message
  Fake out Repository#fetch_ref for merge request if
  Detect if we didn't create the ref sooner

app/controllers/projects/issues_controller.rb

@@ -215,7 +215,7 @@ def realtime_changes
   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

@@ -467,7 +467,8 @@ def version_params_for(diff_refs)
   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
@@ -817,11 +818,7 @@ def state_icon_name
   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
@@ -968,4 +965,18 @@ def mergeable_with_quick_action?(current_user, autocomplete_precheck: false, las
   def base_pipeline
     @base_pipeline ||= project.pipelines.find_by(sha: merge_request_diff&.base_commit_sha)
   end
+
+  private
+
+  def write_ref
+    if for_fork?
+      target_project.repository.fetch_ref(
+        source_project.repository.path_to_repo,
+        "refs/heads/#{source_branch}",
+        ref_path
+      )
+    else
+      source_project.repository.write_ref(ref_path, source_branch_sha)
+    end
+  end
 end

app/models/project.rb

@@ -1048,9 +1048,7 @@ def unarchive!
   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

@@ -231,7 +231,7 @@ def keep_around(sha)
     # 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
@@ -244,6 +244,10 @@ def kept_around?(sha)
     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
@@ -1059,12 +1063,10 @@ def with_repo_branch_commit(start_repository, start_branch_name)
       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
@@ -1095,7 +1097,12 @@ def fetch_remote(remote, forced: false, ssh_auth: nil, no_tags: false)
   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 @@
   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 @@ def post_spam
   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 @@ def post_spam
   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 @@
     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

@@ -74,6 +74,17 @@
       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 @@
   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 @@
     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 @@
   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 @@ def expect_assignee_label_visual_tokens
   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 @@ def expect_assignee_label_visual_tokens
         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 @@ def expect_assignee_label_visual_tokens
     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 @@
   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 @@
            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 @@
     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 @@
   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 @@
     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 @@
   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 @@
   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 @@
     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

@@ -967,6 +967,27 @@ def expect_to_raise_storage_error
       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

@@ -318,15 +318,17 @@
       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
@@ -337,7 +339,7 @@
         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
@@ -351,25 +353,25 @@
              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
@@ -380,7 +382,7 @@
                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
@@ -390,14 +392,14 @@
                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

spec/services/ci/create_pipeline_service_spec.rb

@@ -66,7 +66,7 @@ def execute_service(
         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 @@
   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 @@ def initialize(*args)
   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) }