Merge branch 'jv-too-many-branches' into 'master'

UpdateRemoteMirror: handle large number of branches

Closes #1878

See merge request gitlab-org/gitaly!1745

3 files changed, 56 insertions, 25 deletions

diff --git a/changelogs/unreleased/jv-too-many-branches.yml b/changelogs/unreleased/jv-too-many-branches.yml
new file mode 100644
index 000000000..3f4a0d004
--- /dev/null
+++ b/changelogs/unreleased/jv-too-many-branches.yml
@@ -0,0 +1,5 @@
+---
+title: 'UpdateRemoteMirror: handle large number of branches'
+merge_request: 1745
+author:
+type: fixed
diff --git a/ruby/lib/gitlab/git/gitlab_projects.rb b/ruby/lib/gitlab/git/gitlab_projects.rb
index bc3859fb4..a23cdfa91 100644
--- a/ruby/lib/gitlab/git/gitlab_projects.rb
+++ b/ruby/lib/gitlab/git/gitlab_projects.rb
@@ -6,6 +6,8 @@ module Gitlab
       include Gitlab::Git::Popen
       include Gitlab::Utils::StrongMemoize
 
+      BRANCHES_PER_PUSH = 10
+
       # Relative path is a directory name for repository with .git at the end.
       # Example: gitlab-org/gitlab-test.git
       attr_reader :repository_relative_path
@@ -67,29 +69,36 @@ module Gitlab
       end
 
       def push_branches(remote_name, timeout, force, branch_names, env: {})
-        logger.info "Pushing branches from #{repository_absolute_path} to remote #{remote_name}: #{branch_names}"
-        cmd = %W(#{Gitlab.config.git.bin_path} push)
-        cmd << '--force' if force
-        cmd += %W(-- #{remote_name}).concat(branch_names)
+        branch_names.each_slice(BRANCHES_PER_PUSH) do |branches|
+          logger.info "Pushing #{branches.count} branches from #{repository_absolute_path} to remote #{remote_name}"
 
-        success = run_with_timeout(cmd, timeout, repository_absolute_path, env)
+          cmd = %W(#{Gitlab.config.git.bin_path} push)
+          cmd << '--force' if force
+          cmd += %W(-- #{remote_name}).concat(branches)
 
-        logger.error("Pushing branches to remote #{remote_name} failed.") unless success
+          unless run_with_timeout(cmd, timeout, repository_absolute_path, env)
+            logger.error("Pushing branches to remote #{remote_name} failed.")
+            return false
+          end
+        end
 
-        success
+        true
       end
 
       def delete_remote_branches(remote_name, branch_names, env: {})
-        branches = branch_names.map { |branch_name| ":#{branch_name}" }
+        branch_names.each_slice(BRANCHES_PER_PUSH) do |branches|
+          logger.info "Pushing #{branches.count} deleted branches from #{repository_absolute_path} to remote #{remote_name}"
 
-        logger.info "Pushing deleted branches from #{repository_absolute_path} to remote #{remote_name}: #{branch_names}"
-        cmd = %W(#{Gitlab.config.git.bin_path} push -- #{remote_name}).concat(branches)
+          cmd = %W(#{Gitlab.config.git.bin_path} push -- #{remote_name})
+          branches.each { |branch| cmd << ":#{branch}" }
 
-        success = run(cmd, repository_absolute_path, env)
-
-        logger.error("Pushing deleted branches to remote #{remote_name} failed.") unless success
+          unless run(cmd, repository_absolute_path, env)
+            logger.error("Pushing deleted branches to remote #{remote_name} failed.")
+            return false
+          end
+        end
 
-        success
+        true
       end
 
       protected
diff --git a/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb b/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb
index 85122ede6..a1ef8b51b 100644
--- a/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb
+++ b/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb
@@ -56,27 +56,35 @@ describe Gitlab::Git::GitlabProjects do
 
   describe '#push_branches' do
     let(:remote_name) { 'remote-name' }
-    let(:branch_name) { 'master' }
     let(:env) { { 'GIT_SSH_COMMAND' => 'foo-command bar' } }
-    let(:cmd) { %W(#{Gitlab.config.git.bin_path} push -- #{remote_name} #{branch_name}) }
     let(:force) { false }
+    let(:branch_names) { 20.times.map { |i| "branch#{i}" } }
+    let(:cmd1) do
+      %W(#{Gitlab.config.git.bin_path} push -- #{remote_name}) + branch_names[0, 10]
+    end
+    let(:cmd2) do
+      %W(#{Gitlab.config.git.bin_path} push -- #{remote_name}) + branch_names[10, 10]
+    end
 
-    subject { gl_projects.push_branches(remote_name, 600, force, [branch_name], env: env) }
+    subject { gl_projects.push_branches(remote_name, 600, force, branch_names, env: env) }
 
     it 'executes the command' do
-      stub_spawn(cmd, 600, tmp_repo_path, env, success: true)
+      stub_spawn(cmd1, 600, tmp_repo_path, env, success: true)
+      stub_spawn(cmd2, 600, tmp_repo_path, env, success: true)
 
       is_expected.to be_truthy
     end
 
     it 'fails' do
-      stub_spawn(cmd, 600, tmp_repo_path, env, success: false)
+      stub_spawn(cmd1, 600, tmp_repo_path, env, success: true)
+      stub_spawn(cmd2, 600, tmp_repo_path, env, success: false)
 
       is_expected.to be_falsy
     end
 
     context 'with --force' do
-      let(:cmd) { %W(#{Gitlab.config.git.bin_path} push --force -- #{remote_name} #{branch_name}) }
+      let(:branch_names) { ['master'] }
+      let(:cmd) { %W(#{Gitlab.config.git.bin_path} push --force -- #{remote_name} #{branch_names[0]}) }
       let(:force) { true }
 
       it 'executes the command' do
@@ -150,20 +158,29 @@ describe Gitlab::Git::GitlabProjects do
 
   describe '#delete_remote_branches' do
     let(:remote_name) { 'remote-name' }
-    let(:branch_name) { 'master' }
+    let(:branch_names) { 20.times.map { |i| "branch#{i}" } }
     let(:env) { { 'GIT_SSH_COMMAND' => 'foo-command bar' } }
-    let(:cmd) { %W(#{Gitlab.config.git.bin_path} push -- #{remote_name} :#{branch_name}) }
+    let(:cmd1) do
+      %W(#{Gitlab.config.git.bin_path} push -- #{remote_name}) +
+        branch_names[0, 10].map { |b| ':' + b }
+    end
+    let(:cmd2) do
+      %W(#{Gitlab.config.git.bin_path} push -- #{remote_name}) +
+        branch_names[10, 10].map { |b| ':' + b }
+    end
 
-    subject { gl_projects.delete_remote_branches(remote_name, [branch_name], env: env) }
+    subject { gl_projects.delete_remote_branches(remote_name, branch_names, env: env) }
 
     it 'executes the command' do
-      stub_unlimited_spawn(cmd, tmp_repo_path, env, success: true)
+      stub_unlimited_spawn(cmd1, tmp_repo_path, env, success: true)
+      stub_unlimited_spawn(cmd2, tmp_repo_path, env, success: true)
 
       is_expected.to be_truthy
     end
 
     it 'fails' do
-      stub_unlimited_spawn(cmd, tmp_repo_path, env, success: false)
+      stub_unlimited_spawn(cmd1, tmp_repo_path, env, success: true)
+      stub_unlimited_spawn(cmd2, tmp_repo_path, env, success: false)
 
       is_expected.to be_falsy
     end