Merge branch 'sh-make-prune-optional-in-git-fetch' into 'master'

Make --prune a configurable parameter in fetching a git remote

See merge request gitlab-org/gitlab-ce!17346

app/models/repository.rb

@@ -866,20 +866,20 @@ class Repository
     raw_repository.ancestor?(ancestor_id, descendant_id)
   end
 
-  def fetch_as_mirror(url, forced: false, refmap: :all_refs, remote_name: nil)
+  def fetch_as_mirror(url, forced: false, refmap: :all_refs, remote_name: nil, prune: true)
     unless remote_name
       remote_name = "tmp-#{SecureRandom.hex}"
       tmp_remote_name = true
     end
 
     add_remote(remote_name, url, mirror_refmap: refmap)
-    fetch_remote(remote_name, forced: forced)
+    fetch_remote(remote_name, forced: forced, prune: prune)
   ensure
     remove_remote(remote_name) if tmp_remote_name
   end
 
-  def fetch_remote(remote, forced: false, ssh_auth: nil, no_tags: false)
-    gitlab_shell.fetch_remote(raw_repository, remote, ssh_auth: ssh_auth, forced: forced, no_tags: no_tags)
+  def fetch_remote(remote, forced: false, ssh_auth: nil, no_tags: false, prune: true)
+    gitlab_shell.fetch_remote(raw_repository, remote, ssh_auth: ssh_auth, forced: forced, no_tags: no_tags, prune: prune)
   end
 
   def fetch_source_branch!(source_repository, source_branch, local_ref)

changelogs/unreleased/sh-make-prune-optional-in-git-fetch.yml

+---
+title: Make --prune a configurable parameter in fetching a git remote
+merge_request:
+author:
+type: performance

lib/gitlab/git/gitlab_projects.rb

@@ -63,11 +63,12 @@ module Gitlab
         end
       end
 
-      def fetch_remote(name, timeout, force:, tags:, ssh_key: nil, known_hosts: nil)
+      def fetch_remote(name, timeout, force:, tags:, ssh_key: nil, known_hosts: nil, prune: true)
         tags_option = tags ? '--tags' : '--no-tags'
 
         logger.info "Fetching remote #{name} for repository #{repository_absolute_path}."
-        cmd = %W(git fetch #{name} --prune --quiet)
+        cmd = %W(git fetch #{name} --quiet)
+        cmd << '--prune' if prune
         cmd << '--force' if force
         cmd << tags_option
 

lib/gitlab/gitaly_client/repository_service.rb

@@ -45,10 +45,10 @@ module Gitlab
         GitalyClient.call(@storage, :repository_service, :apply_gitattributes, request)
       end
 
-      def fetch_remote(remote, ssh_auth:, forced:, no_tags:, timeout:)
+      def fetch_remote(remote, ssh_auth:, forced:, no_tags:, timeout:, prune: true)
         request = Gitaly::FetchRemoteRequest.new(
           repository: @gitaly_repo, remote: remote, force: forced,
-          no_tags: no_tags, timeout: timeout
+          no_tags: no_tags, timeout: timeout, no_prune: !prune
         )
 
         if ssh_auth&.ssh_import?

lib/gitlab/shell.rb

@@ -125,13 +125,13 @@ module Gitlab
     # Ex.
     #   fetch_remote(my_repo, "upstream")
     #
-    def fetch_remote(repository, remote, ssh_auth: nil, forced: false, no_tags: false)
+    def fetch_remote(repository, remote, ssh_auth: nil, forced: false, no_tags: false, prune: true)
       gitaly_migrate(:fetch_remote) do |is_enabled|
         if is_enabled
-          repository.gitaly_repository_client.fetch_remote(remote, ssh_auth: ssh_auth, forced: forced, no_tags: no_tags, timeout: git_timeout)
+          repository.gitaly_repository_client.fetch_remote(remote, ssh_auth: ssh_auth, forced: forced, no_tags: no_tags, timeout: git_timeout, prune: prune)
         else
           storage_path = Gitlab.config.repositories.storages[repository.storage]["path"]
-          local_fetch_remote(storage_path, repository.relative_path, remote, ssh_auth: ssh_auth, forced: forced, no_tags: no_tags)
+          local_fetch_remote(storage_path, repository.relative_path, remote, ssh_auth: ssh_auth, forced: forced, no_tags: no_tags, prune: prune)
         end
       end
     end
@@ -428,8 +428,8 @@ module Gitlab
       )
     end
 
-    def local_fetch_remote(storage_path, repository_relative_path, remote, ssh_auth: nil, forced: false, no_tags: false)
-      vars = { force: forced, tags: !no_tags }
+    def local_fetch_remote(storage_path, repository_relative_path, remote, ssh_auth: nil, forced: false, no_tags: false, prune: true)
+      vars = { force: forced, tags: !no_tags, prune: prune }
 
       if ssh_auth&.ssh_import?
         if ssh_auth.ssh_key_auth? && ssh_auth.ssh_private_key.present?

spec/lib/gitlab/git/gitlab_projects_spec.rb

@@ -61,10 +61,11 @@ describe Gitlab::Git::GitlabProjects do
     let(:remote_name) { 'remote-name' }
     let(:branch_name) { 'master' }
     let(:force) { false }
+    let(:prune) { true }
     let(:tags) { true }
-    let(:args) { { force: force, tags: tags }.merge(extra_args) }
+    let(:args) { { force: force, tags: tags, prune: prune }.merge(extra_args) }
     let(:extra_args) { {} }
-    let(:cmd) { %W(git fetch #{remote_name} --prune --quiet --tags) }
+    let(:cmd) { %W(git fetch #{remote_name} --quiet --prune --tags) }
 
     subject { gl_projects.fetch_remote(remote_name, 600, args) }
 
@@ -97,7 +98,7 @@ describe Gitlab::Git::GitlabProjects do
 
     context 'with --force' do
       let(:force) { true }
-      let(:cmd) { %W(git fetch #{remote_name} --prune --quiet --force --tags) }
+      let(:cmd) { %W(git fetch #{remote_name} --quiet --prune --force --tags) }
 
       it 'executes the command with forced option' do
         stub_spawn(cmd, 600, tmp_repo_path, {}, success: true)
@@ -108,7 +109,18 @@ describe Gitlab::Git::GitlabProjects do
 
     context 'with --no-tags' do
       let(:tags) { false }
-      let(:cmd) { %W(git fetch #{remote_name} --prune --quiet --no-tags) }
+      let(:cmd) { %W(git fetch #{remote_name} --quiet --prune --no-tags) }
+
+      it 'executes the command' do
+        stub_spawn(cmd, 600, tmp_repo_path, {}, success: true)
+
+        is_expected.to be_truthy
+      end
+    end
+
+    context 'with no prune' do
+      let(:prune) { false }
+      let(:cmd) { %W(git fetch #{remote_name} --quiet --tags) }
 
       it 'executes the command' do
         stub_spawn(cmd, 600, tmp_repo_path, {}, success: true)

spec/lib/gitlab/gitaly_client/repository_service_spec.rb

@@ -85,6 +85,20 @@ describe Gitlab::GitalyClient::RepositoryService do
     end
   end
 
+  describe '#fetch_remote' do
+    let(:ssh_auth) { double(:ssh_auth, ssh_import?: true, ssh_key_auth?: false, ssh_known_hosts: nil) }
+    let(:import_url) { 'ssh://example.com' }
+
+    it 'sends a fetch_remote_request message' do
+      expect_any_instance_of(Gitaly::RepositoryService::Stub)
+        .to receive(:fetch_remote)
+        .with(gitaly_request_with_params(no_prune: false), kind_of(Hash))
+        .and_return(double(value: true))
+
+      client.fetch_remote(import_url, ssh_auth: ssh_auth, forced: false, no_tags: false, timeout: 60)
+    end
+  end
+
   describe '#rebase_in_progress?' do
     let(:rebase_id) { 1 }
 

spec/lib/gitlab/shell_spec.rb

@@ -508,8 +508,8 @@ describe Gitlab::Shell do
     end
 
     shared_examples 'fetch_remote' do |gitaly_on|
-      def fetch_remote(ssh_auth = nil)
-        gitlab_shell.fetch_remote(repository.raw_repository, 'remote-name', ssh_auth: ssh_auth)
+      def fetch_remote(ssh_auth = nil, prune = true)
+        gitlab_shell.fetch_remote(repository.raw_repository, 'remote-name', ssh_auth: ssh_auth, prune: prune)
       end
 
       def expect_gitlab_projects(fail = false, options = {})
@@ -555,27 +555,33 @@ describe Gitlab::Shell do
       end
 
       it 'returns true when the command succeeds' do
-        expect_call(false, force: false, tags: true)
+        expect_call(false, force: false, tags: true, prune: true)
 
         expect(fetch_remote).to be_truthy
       end
 
+      it 'returns true when the command succeeds' do
+        expect_call(false, force: false, tags: true, prune: false)
+
+        expect(fetch_remote(nil, false)).to be_truthy
+      end
+
       it 'raises an exception when the command fails' do
-        expect_call(true, force: false, tags: true)
+        expect_call(true, force: false, tags: true, prune: true)
 
         expect { fetch_remote }.to raise_error(Gitlab::Shell::Error)
       end
 
       it 'allows forced and no_tags to be changed' do
-        expect_call(false, force: true, tags: false)
+        expect_call(false, force: true, tags: false, prune: true)
 
-        result = gitlab_shell.fetch_remote(repository.raw_repository, 'remote-name', forced: true, no_tags: true)
+        result = gitlab_shell.fetch_remote(repository.raw_repository, 'remote-name', forced: true, no_tags: true, prune: true)
         expect(result).to be_truthy
       end
 
       context 'SSH auth' do
         it 'passes the SSH key if specified' do
-          expect_call(false, force: false, tags: true, ssh_key: 'foo')
+          expect_call(false, force: false, tags: true, prune: true, ssh_key: 'foo')
 
           ssh_auth = build_ssh_auth(ssh_key_auth?: true, ssh_private_key: 'foo')
 
@@ -583,7 +589,7 @@ describe Gitlab::Shell do
         end
 
         it 'does not pass an empty SSH key' do
-          expect_call(false, force: false, tags: true)
+          expect_call(false, force: false, tags: true, prune: true)
 
           ssh_auth = build_ssh_auth(ssh_key_auth: true, ssh_private_key: '')
 
@@ -591,7 +597,7 @@ describe Gitlab::Shell do
         end
 
         it 'does not pass the key unless SSH key auth is to be used' do
-          expect_call(false, force: false, tags: true)
+          expect_call(false, force: false, tags: true, prune: true)
 
           ssh_auth = build_ssh_auth(ssh_key_auth: false, ssh_private_key: 'foo')
 
@@ -599,7 +605,7 @@ describe Gitlab::Shell do
         end
 
         it 'passes the known_hosts data if specified' do
-          expect_call(false, force: false, tags: true, known_hosts: 'foo')
+          expect_call(false, force: false, tags: true, prune: true, known_hosts: 'foo')
 
           ssh_auth = build_ssh_auth(ssh_known_hosts: 'foo')
 
@@ -607,7 +613,7 @@ describe Gitlab::Shell do
         end
 
         it 'does not pass empty known_hosts data' do
-          expect_call(false, force: false, tags: true)
+          expect_call(false, force: false, tags: true, prune: true)
 
           ssh_auth = build_ssh_auth(ssh_known_hosts: '')
 
@@ -615,7 +621,7 @@ describe Gitlab::Shell do
         end
 
         it 'does not pass known_hosts data unless SSH is to be used' do
-          expect_call(false, force: false, tags: true)
+          expect_call(false, force: false, tags: true, prune: true)
 
           ssh_auth = build_ssh_auth(ssh_import?: false, ssh_known_hosts: 'foo')
 
@@ -642,7 +648,7 @@ describe Gitlab::Shell do
 
         it 'passes the correct params to the gitaly service' do
           expect(repository.gitaly_repository_client).to receive(:fetch_remote)
-            .with(remote_name, ssh_auth: ssh_auth, forced: true, no_tags: true, timeout: timeout)
+            .with(remote_name, ssh_auth: ssh_auth, forced: true, no_tags: true, prune: true, timeout: timeout)
 
           subject
         end