From 885a4da2083991ef66f149446e7f41624b04b0bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Mon, 20 Nov 2017 15:00:38 -0300 Subject: [PATCH 1/2] Add feature flag to use gitaly-ssh mirroring when cloning internal repos This also allows us to simplify the naming since we can make some fetching methods private. --- lib/gitlab/git/repository.rb | 32 ++++++++++++++++++++------ lib/gitlab/git/repository_mirroring.rb | 13 ----------- spec/lib/gitlab/git/repository_spec.rb | 21 +++-------------- 3 files changed, 28 insertions(+), 38 deletions(-) diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 1468069a991..b1a6dbfe8d3 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -20,6 +20,7 @@ module Gitlab SEARCH_CONTEXT_LINES = 3 REBASE_WORKTREE_PREFIX = 'rebase'.freeze SQUASH_WORKTREE_PREFIX = 'squash'.freeze + GITALY_INTERNAL_URL = 'ssh://gitaly/internal.git'.freeze NoRepository = Class.new(StandardError) InvalidBlobName = Class.new(StandardError) @@ -1140,12 +1141,25 @@ module Gitlab @has_visible_content = has_local_branches? end - # Like all public `Gitlab::Git::Repository` methods, this method is part - # of `Repository`'s interface through `method_missing`. - # `Repository` has its own `fetch_remote` which uses `gitlab-shell` and - # takes some extra attributes, so we qualify this method name to prevent confusion. - def fetch_remote_without_shell(remote = 'origin') - run_git(['fetch', remote]).last.zero? + def fetch_repository_as_mirror(repository) + remote_name = "tmp-#{SecureRandom.hex}" + + # Notice that this feature flag is not for `fetch_repository_as_mirror` + # as a whole but for the fetching mechanism (file path or gitaly-ssh). + url, env = gitaly_migrate(:fetch_internal) do |is_enabled| + if is_enabled + repository = RemoteRepository.new(repository) unless repository.is_a?(RemoteRepository) + [GITALY_INTERNAL_URL, repository.fetch_env] + else + [repository.path, nil] + end + end + + add_remote(remote_name, url) + set_remote_as_mirror(remote_name) + fetch_remote(remote_name, env: env) + ensure + remove_remote(remote_name) end def blob_at(sha, path) @@ -1849,7 +1863,7 @@ module Gitlab end def gitaly_fetch_ref(source_repository, source_ref:, target_ref:) - args = %W(fetch --no-tags -f ssh://gitaly/internal.git #{source_ref}:#{target_ref}) + args = %W(fetch --no-tags -f #{GITALY_INTERNAL_URL} #{source_ref}:#{target_ref}) run_git(args, env: source_repository.fetch_env) end @@ -1869,6 +1883,10 @@ module Gitlab rescue Rugged::ReferenceError raise ArgumentError, 'Invalid merge source' end + + def fetch_remote(remote_name = 'origin', env: nil) + run_git(['fetch', remote_name], env: env).last.zero? + end end end end diff --git a/lib/gitlab/git/repository_mirroring.rb b/lib/gitlab/git/repository_mirroring.rb index 392bef69e80..30c1e522e8e 100644 --- a/lib/gitlab/git/repository_mirroring.rb +++ b/lib/gitlab/git/repository_mirroring.rb @@ -31,19 +31,6 @@ module Gitlab end end - # Like all_refs public `Gitlab::Git::Repository` methods, this method is part - # of `Repository`'s interface through `method_missing`. - # `Repository` has its own `fetch_as_mirror` which uses `gitlab-shell` and - # takes some extra attributes, so we qualify this method name to prevent confusion. - def fetch_as_mirror_without_shell(url) - remote_name = "tmp-#{SecureRandom.hex}" - add_remote(remote_name, url) - set_remote_as_mirror(remote_name) - fetch_remote_without_shell(remote_name) - ensure - remove_remote(remote_name) if remote_name - end - def remote_tags(remote) # Each line has this format: "dc872e9fa6963f8f03da6c8f6f264d0845d6b092\trefs/tags/v1.10.0\n" # We want to convert it to: [{ 'v1.10.0' => 'dc872e9fa6963f8f03da6c8f6f264d0845d6b092' }, ...] diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 08dd6ea80ff..4f24ca3c25f 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -588,12 +588,12 @@ describe Gitlab::Git::Repository, seed_helper: true do end end - describe '#fetch_as_mirror_without_shell' do + describe '#fetch_repository_as_mirror' do let(:new_repository) do Gitlab::Git::Repository.new('default', 'my_project.git', '') end - subject { new_repository.fetch_as_mirror_without_shell(repository.path) } + subject { new_repository.fetch_repository_as_mirror(repository) } before do Gitlab::Shell.new.add_repository('default', 'my_project') @@ -603,7 +603,7 @@ describe Gitlab::Git::Repository, seed_helper: true do Gitlab::Shell.new.remove_repository(TestEnv.repos_path, 'my_project') end - it 'fetches a url as a mirror remote' do + it 'fetches a repository as a mirror remote' do subject expect(refs(new_repository.path)).to eq(refs(repository.path)) @@ -1662,21 +1662,6 @@ describe Gitlab::Git::Repository, seed_helper: true do end end - describe '#fetch_remote_without_shell' do - let(:git_path) { Gitlab.config.git.bin_path } - let(:remote_name) { 'my_remote' } - - subject { repository.fetch_remote_without_shell(remote_name) } - - it 'fetches the remote and returns true if the command was successful' do - expect(repository).to receive(:popen) - .with(%W(#{git_path} fetch #{remote_name}), repository.path, {}) - .and_return(['', 0]) - - expect(subject).to be(true) - end - end - describe '#merge' do let(:repository) do Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') From 95009cefc2fe237cde72c93a0634e5dbc638cd1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Wed, 6 Dec 2017 19:08:29 -0300 Subject: [PATCH 2/2] Unify mirror addition operations to prepare for Gitaly migration --- app/models/repository.rb | 3 +-- lib/gitlab/git/repository.rb | 8 +++++--- lib/gitlab/git/repository_mirroring.rb | 28 +++++++++++++------------- 3 files changed, 20 insertions(+), 19 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index 82af299ec5e..1429774f0a9 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -972,8 +972,7 @@ class Repository tmp_remote_name = true end - add_remote(remote_name, url) - set_remote_as_mirror(remote_name, refmap: refmap) + add_remote(remote_name, url, mirror_refmap: refmap) fetch_remote(remote_name, forced: forced) ensure remove_remote(remote_name) if tmp_remote_name diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index b1a6dbfe8d3..cb787cff7b0 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -884,8 +884,11 @@ module Gitlab end end - def add_remote(remote_name, url) + # If `mirror_refmap` is present the remote is set as mirror with that mapping + def add_remote(remote_name, url, mirror_refmap: nil) rugged.remotes.create(remote_name, url) + + set_remote_as_mirror(remote_name, refmap: mirror_refmap) if mirror_refmap rescue Rugged::ConfigError remote_update(remote_name, url: url) end @@ -1155,8 +1158,7 @@ module Gitlab end end - add_remote(remote_name, url) - set_remote_as_mirror(remote_name) + add_remote(remote_name, url, mirror_refmap: :all_refs) fetch_remote(remote_name, env: env) ensure remove_remote(remote_name) diff --git a/lib/gitlab/git/repository_mirroring.rb b/lib/gitlab/git/repository_mirroring.rb index 30c1e522e8e..effb1f0ca19 100644 --- a/lib/gitlab/git/repository_mirroring.rb +++ b/lib/gitlab/git/repository_mirroring.rb @@ -17,20 +17,6 @@ module Gitlab rugged.config["remote.#{remote_name}.prune"] = true end - def set_remote_refmap(remote_name, refmap) - Array(refmap).each_with_index do |refspec, i| - refspec = REFMAPS[refspec] || refspec - - # We need multiple `fetch` entries, but Rugged only allows replacing a config, not adding to it. - # To make sure we start from scratch, we set the first using rugged, and use `git` for any others - if i == 0 - rugged.config["remote.#{remote_name}.fetch"] = refspec - else - run_git(%W[config --add remote.#{remote_name}.fetch #{refspec}]) - end - end - end - def remote_tags(remote) # Each line has this format: "dc872e9fa6963f8f03da6c8f6f264d0845d6b092\trefs/tags/v1.10.0\n" # We want to convert it to: [{ 'v1.10.0' => 'dc872e9fa6963f8f03da6c8f6f264d0845d6b092' }, ...] @@ -72,6 +58,20 @@ module Gitlab private + def set_remote_refmap(remote_name, refmap) + Array(refmap).each_with_index do |refspec, i| + refspec = REFMAPS[refspec] || refspec + + # We need multiple `fetch` entries, but Rugged only allows replacing a config, not adding to it. + # To make sure we start from scratch, we set the first using rugged, and use `git` for any others + if i == 0 + rugged.config["remote.#{remote_name}.fetch"] = refspec + else + run_git(%W[config --add remote.#{remote_name}.fetch #{refspec}]) + end + end + end + def list_remote_tags(remote) tag_list, exit_code, error = nil cmd = %W(#{Gitlab.config.git.bin_path} --git-dir=#{path} ls-remote --tags #{remote})