gitaly_client: Add ability to fetch remotes by URL

When fetching remotes, we currently first create it and then fetch the
created remote. This is an inefficient interface and awkward to use, as
we have to assert that the remote exists each time we fetch. It's also
vulnerable to races as we now depend on on-disk state of Gitaly.

Some time ago, Gitaly has introduces a new `RemoteParams` field to the
`FetchRemote()` RPC which enables callers to provide configuration of
the remote directly instead of passing the previously created remote.
As a first step towards switching to that function, this commit
introduces two new parameters `url` and `refmap` to `fetch_remote()`
which switch to this new behaviour if given.

lib/gitlab/git/repository.rb

@@ -797,15 +797,19 @@ module Gitlab
       # Fetch remote for repository
       #
       # remote - remote name
+      # url - URL of the remote to fetch. `remote` is not used in this case.
+      # refmap - if url is given, determines which references should get fetched where
       # ssh_auth - SSH known_hosts data and a private key to use for public-key authentication
       # forced - should we use --force flag?
       # no_tags - should we use --no-tags flag?
       # prune - should we use --prune flag?
       # check_tags_changed - should we ask gitaly to calculate whether any tags changed?
-      def fetch_remote(remote, ssh_auth: nil, forced: false, no_tags: false, prune: true, check_tags_changed: false)
+      def fetch_remote(remote, url: nil, refmap: nil, ssh_auth: nil, forced: false, no_tags: false, prune: true, check_tags_changed: false)
         wrapped_gitaly_errors do
           gitaly_repository_client.fetch_remote(
             remote,
+            url: url,
+            refmap: refmap,
             ssh_auth: ssh_auth,
             forced: forced,
             no_tags: no_tags,

lib/gitlab/gitaly_client/repository_service.rb

@@ -70,13 +70,21 @@ module Gitlab
         end.join
       end
 
-      def fetch_remote(remote, ssh_auth:, forced:, no_tags:, timeout:, prune: true, check_tags_changed: false)
+      # rubocop: disable Metrics/ParameterLists
+      # The `remote` parameter is going away soonish anyway, at which point the
+      # Rubocop warning can be enabled again.
+      def fetch_remote(remote, url:, refmap:, ssh_auth:, forced:, no_tags:, timeout:, prune: true, check_tags_changed: false)
         request = Gitaly::FetchRemoteRequest.new(
           repository: @gitaly_repo, remote: remote, force: forced,
           no_tags: no_tags, timeout: timeout, no_prune: !prune,
           check_tags_changed: check_tags_changed
         )
 
+        if url
+          request.remote_params = Gitaly::Remote.new(url: url,
+                                                     mirror_refmaps: Array.wrap(refmap).map(&:to_s))
+        end
+
         if ssh_auth&.ssh_mirror_url?
           if ssh_auth.ssh_key_auth? && ssh_auth.ssh_private_key.present?
             request.ssh_key = ssh_auth.ssh_private_key
@@ -89,6 +97,7 @@ module Gitlab
 
         GitalyClient.call(@storage, :repository_service, :fetch_remote, request, timeout: GitalyClient.long_timeout)
       end
+      # rubocop: enable Metrics/ParameterLists
 
       def create_repository
         request = Gitaly::CreateRepositoryRequest.new(repository: @gitaly_repo)

spec/lib/gitlab/git/repository_spec.rb

@@ -521,7 +521,9 @@ RSpec.describe Gitlab::Git::Repository, :seed_helper do
         no_tags: true,
         timeout: described_class::GITLAB_PROJECTS_TIMEOUT,
         prune: false,
-        check_tags_changed: false
+        check_tags_changed: false,
+        url: nil,
+        refmap: nil
       }
 
       expect(repository.gitaly_repository_client).to receive(:fetch_remote).with('remote-name', expected_opts)

spec/lib/gitlab/gitaly_client/repository_service_spec.rb

@@ -122,67 +122,89 @@ RSpec.describe Gitlab::GitalyClient::RepositoryService do
   end
 
   describe '#fetch_remote' do
-    let(:remote) { 'remote-name' }
-
-    it 'sends a fetch_remote_request message' do
-      expected_request = gitaly_request_with_params(
-        remote: remote,
-        ssh_key: '',
-        known_hosts: '',
-        force: false,
-        no_tags: false,
-        no_prune: false,
-        check_tags_changed: false
-      )
+    shared_examples 'a fetch' do
+      it 'sends a fetch_remote_request message' do
+        expected_remote_params = Gitaly::Remote.new(
+          url: url, http_authorization_header: "", mirror_refmaps: [])
+
+        expected_request = gitaly_request_with_params(
+          remote: remote,
+          remote_params: url ? expected_remote_params : nil,
+          ssh_key: '',
+          known_hosts: '',
+          force: false,
+          no_tags: false,
+          no_prune: false,
+          check_tags_changed: false
+        )
+
+        expect_any_instance_of(Gitaly::RepositoryService::Stub)
+          .to receive(:fetch_remote)
+          .with(expected_request, kind_of(Hash))
+          .and_return(double(value: true))
+
+        client.fetch_remote(remote, url: url, refmap: nil, ssh_auth: nil, forced: false, no_tags: false, timeout: 1, check_tags_changed: false)
+      end
 
-      expect_any_instance_of(Gitaly::RepositoryService::Stub)
-        .to receive(:fetch_remote)
-        .with(expected_request, kind_of(Hash))
-        .and_return(double(value: true))
+      context 'SSH auth' do
+        where(:ssh_mirror_url, :ssh_key_auth, :ssh_private_key, :ssh_known_hosts, :expected_params) do
+          false | false | 'key' | 'known_hosts' | {}
+          false | true  | 'key' | 'known_hosts' | {}
+          true  | false | 'key' | 'known_hosts' | { known_hosts: 'known_hosts' }
+          true  | true  | 'key' | 'known_hosts' | { ssh_key: 'key', known_hosts: 'known_hosts' }
+          true  | true  | 'key' | nil           | { ssh_key: 'key' }
+          true  | true  | nil   | 'known_hosts' | { known_hosts: 'known_hosts' }
+          true  | true  | nil   | nil           | {}
+          true  | true  | ''    | ''            | {}
+        end
 
-      client.fetch_remote(remote, ssh_auth: nil, forced: false, no_tags: false, timeout: 1, check_tags_changed: false)
+        with_them do
+          let(:ssh_auth) do
+            double(
+              :ssh_auth,
+              ssh_mirror_url?: ssh_mirror_url,
+              ssh_key_auth?: ssh_key_auth,
+              ssh_private_key: ssh_private_key,
+              ssh_known_hosts: ssh_known_hosts
+            )
+          end
+
+          it do
+            expected_remote_params = Gitaly::Remote.new(
+              url: url, http_authorization_header: "", mirror_refmaps: [])
+
+            expected_request = gitaly_request_with_params({
+              remote: remote,
+              remote_params: url ? expected_remote_params : nil,
+              ssh_key: '',
+              known_hosts: '',
+              force: false,
+              no_tags: false,
+              no_prune: false
+            }.update(expected_params))
+
+            expect_any_instance_of(Gitaly::RepositoryService::Stub)
+              .to receive(:fetch_remote)
+              .with(expected_request, kind_of(Hash))
+              .and_return(double(value: true))
+
+            client.fetch_remote(remote, url: url, refmap: nil, ssh_auth: ssh_auth, forced: false, no_tags: false, timeout: 1)
+          end
+        end
+      end
     end
 
-    context 'SSH auth' do
-      where(:ssh_mirror_url, :ssh_key_auth, :ssh_private_key, :ssh_known_hosts, :expected_params) do
-        false | false | 'key' | 'known_hosts' | {}
-        false | true  | 'key' | 'known_hosts' | {}
-        true  | false | 'key' | 'known_hosts' | { known_hosts: 'known_hosts' }
-        true  | true  | 'key' | 'known_hosts' | { ssh_key: 'key', known_hosts: 'known_hosts' }
-        true  | true  | 'key' | nil           | { ssh_key: 'key' }
-        true  | true  | nil   | 'known_hosts' | { known_hosts: 'known_hosts' }
-        true  | true  | nil   | nil           | {}
-        true  | true  | ''    | ''            | {}
+    context 'with remote' do
+      it_behaves_like 'a fetch' do
+        let(:remote) { 'remote-name' }
+        let(:url) { nil }
       end
+    end
 
-      with_them do
-        let(:ssh_auth) do
-          double(
-            :ssh_auth,
-            ssh_mirror_url?: ssh_mirror_url,
-            ssh_key_auth?: ssh_key_auth,
-            ssh_private_key: ssh_private_key,
-            ssh_known_hosts: ssh_known_hosts
-          )
-        end
-
-        it do
-          expected_request = gitaly_request_with_params({
-            remote: remote,
-            ssh_key: '',
-            known_hosts: '',
-            force: false,
-            no_tags: false,
-            no_prune: false
-          }.update(expected_params))
-
-          expect_any_instance_of(Gitaly::RepositoryService::Stub)
-            .to receive(:fetch_remote)
-            .with(expected_request, kind_of(Hash))
-            .and_return(double(value: true))
-
-          client.fetch_remote(remote, ssh_auth: ssh_auth, forced: false, no_tags: false, timeout: 1)
-        end
+    context 'with URL' do
+      it_behaves_like 'a fetch' do
+        let(:remote) { "" }
+        let(:url) { 'https://example.com/git/repo.git' }
       end
     end
   end