Automatic merge of gitlab-org/gitlab-ce master

app/services/compare_service.rb

@@ -3,7 +3,7 @@
 require 'securerandom'
 
 # Compare 2 refs for one repo or between repositories
-# and return Gitlab::Git::Compare object that responds to commits and diffs
+# and return Compare object that responds to commits and diffs
 class CompareService
   attr_reader :start_project, :start_ref_name
 
@@ -15,7 +15,7 @@ class CompareService
   def execute(target_project, target_ref, base_sha: nil, straight: false)
     raw_compare = target_project.repository.compare_source_branch(target_ref, start_project.repository, start_ref_name, straight: straight)
 
-    return unless raw_compare
+    return unless raw_compare && raw_compare.base && raw_compare.head
 
     Compare.new(raw_compare,
                 target_project,

app/views/projects/issues/new.html.haml

-- add_to_breadcrumbs "Issues", project_issues_path(@project)
-- breadcrumb_title "New"
-- page_title "New Issue"
+- add_to_breadcrumbs _("Issues"), project_issues_path(@project)
+- breadcrumb_title _("New")
+- page_title _("New Issue")
 
 %h3.page-title
-  New Issue
+  _("New Issue")
 %hr
 
 = render "form"

app/views/projects/issues/show.html.haml

 - @content_class = "limit-container-width" unless fluid_layout
-- add_to_breadcrumbs "Issues", project_issues_path(@project)
+- add_to_breadcrumbs _("Issues"), project_issues_path(@project)
 - breadcrumb_title @issue.to_reference
-- page_title           "#{@issue.title} (#{@issue.to_reference})", "Issues"
+- page_title           "#{@issue.title} (#{@issue.to_reference})", _("Issues")
 - page_description     @issue.description
 - page_card_attributes @issue.card_attributes
 

changelogs/unreleased/sh-avoid-fetching-temp-refs-within-project.yml

+---
+title: Don't create a temp reference for branch comparisons within project
+merge_request: 24038
+author:
+type: fixed

lib/gitlab/checks/branch_check.rb

@@ -48,7 +48,7 @@ module Gitlab
 
         if project.empty_repo?
           protected_branch_push_checks
-        elsif creation? && protected_branch_creation_enabled?
+        elsif creation?
           protected_branch_creation_checks
         elsif deletion?
           protected_branch_deletion_checks
@@ -124,10 +124,6 @@ module Gitlab
         Gitlab::Routing.url_helpers.project_project_members_url(project)
       end
 
-      def protected_branch_creation_enabled?
-        Feature.enabled?(:protected_branch_creation, project, default_enabled: true)
-      end
-
       def matching_merge_request?
         Checks::MatchingMergeRequest.new(newrev, branch_name, project).match?
       end

lib/gitlab/git/repository.rb

@@ -732,18 +732,29 @@ module Gitlab
       end
 
       def compare_source_branch(target_branch_name, source_repository, source_branch_name, straight:)
+        reachable_ref =
+          if source_repository == self
+            source_branch_name
+          else
+            # If a tmp ref was created before for a separate repo comparison (forks),
+            # we're able to short-circuit the tmp ref re-creation:
+            # 1. Take the SHA from the source repo
+            # 2. Read that in the current "target" repo
+            # 3. If that SHA is still known (readable), it means GC hasn't
+            # cleaned it up yet, so we can use it instead re-writing the tmp ref.
+            source_commit_id = source_repository.commit(source_branch_name)&.sha
+            commit(source_commit_id)&.sha if source_commit_id
+          end
+
+        return compare(target_branch_name, reachable_ref, straight: straight) if reachable_ref
+
         tmp_ref = "refs/tmp/#{SecureRandom.hex}"
 
         return unless fetch_source_branch!(source_repository, source_branch_name, tmp_ref)
 
-        Gitlab::Git::Compare.new(
-          self,
-          target_branch_name,
-          tmp_ref,
-          straight: straight
-        )
+        compare(target_branch_name, tmp_ref, straight: straight)
       ensure
-        delete_refs(tmp_ref)
+        delete_refs(tmp_ref) if tmp_ref
       end
 
       def write_ref(ref_path, ref, old_ref: nil)
@@ -999,6 +1010,13 @@ module Gitlab
 
       private
 
+      def compare(base_ref, head_ref, straight:)
+        Gitlab::Git::Compare.new(self,
+                                 base_ref,
+                                 head_ref,
+                                 straight: straight)
+      end
+
       def empty_diff_stats
         Gitlab::Git::DiffStatsCollection.new([])
       end

spec/controllers/projects/compare_controller_spec.rb

@@ -82,7 +82,7 @@ describe Projects::CompareController do
         show_request
 
         expect(response).to be_success
-        expect(assigns(:diffs).diff_files.to_a).to eq([])
+        expect(assigns(:diffs)).to eq([])
         expect(assigns(:commits)).to eq([])
       end
     end

spec/lib/gitlab/checks/branch_check_spec.rb

@@ -77,117 +77,85 @@ describe Gitlab::Checks::BranchCheck do
         let(:oldrev) { '0000000000000000000000000000000000000000' }
         let(:ref) { 'refs/heads/feature' }
 
-        context 'protected branch creation feature is disabled' do
+        context 'user can push to branch' do
           before do
-            stub_feature_flags(protected_branch_creation: false)
+            allow(user_access)
+              .to receive(:can_push_to_branch?)
+              .with('feature')
+              .and_return(true)
           end
 
-          context 'user is not allowed to push to protected branch' do
-            before do
-              allow(user_access)
-                .to receive(:can_push_to_branch?)
-                .and_return(false)
-            end
-
-            it 'raises an error' do
-              expect { subject.validate! }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to push code to protected branches on this project.')
-            end
+          it 'does not raise an error' do
+            expect { subject.validate! }.not_to raise_error
           end
+        end
 
-          context 'user is allowed to push to protected branch' do
-            before do
-              allow(user_access)
-                .to receive(:can_push_to_branch?)
-                .and_return(true)
-            end
-
-            it 'does not raise an error' do
-              expect { subject.validate! }.not_to raise_error
-            end
+        context 'user cannot push to branch' do
+          before do
+            allow(user_access)
+              .to receive(:can_push_to_branch?)
+              .with('feature')
+              .and_return(false)
           end
-        end
 
-        context 'protected branch creation feature is enabled' do
-          context 'user can push to branch' do
+          context 'user cannot merge to branch' do
             before do
               allow(user_access)
-                .to receive(:can_push_to_branch?)
+                .to receive(:can_merge_to_branch?)
                 .with('feature')
-                .and_return(true)
+                .and_return(false)
             end
 
-            it 'does not raise an error' do
-              expect { subject.validate! }.not_to raise_error
+            it 'raises an error' do
+              expect { subject.validate! }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to create protected branches on this project.')
             end
           end
 
-          context 'user cannot push to branch' do
+          context 'user can merge to branch' do
             before do
               allow(user_access)
-                .to receive(:can_push_to_branch?)
+                .to receive(:can_merge_to_branch?)
                 .with('feature')
-                .and_return(false)
+                .and_return(true)
+
+              allow(project.repository)
+                .to receive(:branch_names_contains_sha)
+                .with(newrev)
+                .and_return(['branch'])
             end
 
-            context 'user cannot merge to branch' do
+            context "newrev isn't in any protected branches" do
               before do
-                allow(user_access)
-                  .to receive(:can_merge_to_branch?)
-                  .with('feature')
+                allow(ProtectedBranch)
+                  .to receive(:any_protected?)
+                  .with(project, ['branch'])
                   .and_return(false)
               end
 
               it 'raises an error' do
-                expect { subject.validate! }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to create protected branches on this project.')
+                expect { subject.validate! }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You can only use an existing protected branch ref as the basis of a new protected branch.')
               end
             end
 
-            context 'user can merge to branch' do
+            context 'newrev is included in a protected branch' do
               before do
-                allow(user_access)
-                  .to receive(:can_merge_to_branch?)
-                  .with('feature')
+                allow(ProtectedBranch)
+                  .to receive(:any_protected?)
+                  .with(project, ['branch'])
                   .and_return(true)
-
-                allow(project.repository)
-                  .to receive(:branch_names_contains_sha)
-                  .with(newrev)
-                  .and_return(['branch'])
               end
 
-              context "newrev isn't in any protected branches" do
-                before do
-                  allow(ProtectedBranch)
-                    .to receive(:any_protected?)
-                    .with(project, ['branch'])
-                    .and_return(false)
-                end
+              context 'via web interface' do
+                let(:protocol) { 'web' }
 
-                it 'raises an error' do
-                  expect { subject.validate! }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You can only use an existing protected branch ref as the basis of a new protected branch.')
+                it 'allows branch creation' do
+                  expect { subject.validate! }.not_to raise_error
                 end
               end
 
-              context 'newrev is included in a protected branch' do
-                before do
-                  allow(ProtectedBranch)
-                    .to receive(:any_protected?)
-                    .with(project, ['branch'])
-                    .and_return(true)
-                end
-
-                context 'via web interface' do
-                  let(:protocol) { 'web' }
-
-                  it 'allows branch creation' do
-                    expect { subject.validate! }.not_to raise_error
-                  end
-                end
-
-                context 'via SSH' do
-                  it 'raises an error' do
-                    expect { subject.validate! }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You can only create protected branches using the web interface and API.')
-                  end
+              context 'via SSH' do
+                it 'raises an error' do
+                  expect { subject.validate! }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You can only create protected branches using the web interface and API.')
                 end
               end
             end

spec/lib/gitlab/git/repository_spec.rb

@@ -1966,6 +1966,70 @@ describe Gitlab::Git::Repository, :seed_helper do
     end
   end
 
+  describe '#compare_source_branch' do
+    let(:repository) { Gitlab::Git::Repository.new('default', TEST_GITATTRIBUTES_REPO_PATH, '', 'group/project') }
+
+    context 'within same repository' do
+      it 'does not create a temp ref' do
+        expect(repository).not_to receive(:fetch_source_branch!)
+        expect(repository).not_to receive(:delete_refs)
+
+        compare = repository.compare_source_branch('master', repository, 'feature', straight: false)
+        expect(compare).to be_a(Gitlab::Git::Compare)
+        expect(compare.commits.count).to be > 0
+      end
+
+      it 'returns empty commits when source ref does not exist' do
+        compare = repository.compare_source_branch('master', repository, 'non-existent-branch', straight: false)
+
+        expect(compare.commits).to be_empty
+      end
+    end
+
+    context 'with different repositories' do
+      context 'when ref is known by source repo, but not by target' do
+        before do
+          mutable_repository.write_ref('another-branch', 'feature')
+        end
+
+        it 'creates temp ref' do
+          expect(repository).not_to receive(:fetch_source_branch!)
+          expect(repository).not_to receive(:delete_refs)
+
+          compare = repository.compare_source_branch('master', mutable_repository, 'another-branch', straight: false)
+          expect(compare).to be_a(Gitlab::Git::Compare)
+          expect(compare.commits.count).to be > 0
+        end
+      end
+
+      context 'when ref is known by source and target repos' do
+        before do
+          mutable_repository.write_ref('another-branch', 'feature')
+          repository.write_ref('another-branch', 'feature')
+        end
+
+        it 'does not create a temp ref' do
+          expect(repository).not_to receive(:fetch_source_branch!)
+          expect(repository).not_to receive(:delete_refs)
+
+          compare = repository.compare_source_branch('master', mutable_repository, 'another-branch', straight: false)
+          expect(compare).to be_a(Gitlab::Git::Compare)
+          expect(compare.commits.count).to be > 0
+        end
+      end
+
+      context 'when ref is unknown by source repo' do
+        it 'returns nil when source ref does not exist' do
+          expect(repository).to receive(:fetch_source_branch!).and_call_original
+          expect(repository).to receive(:delete_refs).and_call_original
+
+          compare = repository.compare_source_branch('master', mutable_repository, 'non-existent-branch', straight: false)
+          expect(compare).to be_nil
+        end
+      end
+    end
+  end
+
   describe '#checksum' do
     it 'calculates the checksum for non-empty repo' do
       expect(repository.checksum).to eq '51d0a9662681f93e1fee547a6b7ba2bcaf716059'

spec/services/compare_service_spec.rb

@@ -19,5 +19,18 @@ describe CompareService do
 
       it { expect(subject.diffs.size).to eq(3) }
     end
+
+    context 'compare with target branch that does not exist' do
+      subject { service.execute(project, 'non-existent-ref') }
+
+      it { expect(subject).to be_nil }
+    end
+
+    context 'compare with source branch that does not exist' do
+      let(:service) { described_class.new(project, 'non-existent-branch') }
+      subject { service.execute(project, 'non-existent-ref') }
+
+      it { expect(subject).to be_nil }
+    end
   end
 end