Merge branch 'rs-push-with-porcelain' into 'master'

Push with the --porcelain flag and parse output of failed pushes

See merge request gitlab-org/gitaly!1845

7 files changed, 235 insertions, 4 deletions

diff --git a/changelogs/unreleased/rs-push-with-porcelain.yml b/changelogs/unreleased/rs-push-with-porcelain.yml
new file mode 100644
index 000000000..9e45166ba
--- /dev/null
+++ b/changelogs/unreleased/rs-push-with-porcelain.yml
@@ -0,0 +1,5 @@
+---
+title: Push with the --porcelain flag and parse output of failed pushes
+merge_request: 1845
+author:
+type: added
diff --git a/ruby/lib/gitlab/git/gitlab_projects.rb b/ruby/lib/gitlab/git/gitlab_projects.rb
index a23cdfa91..aa1a27cd2 100644
--- a/ruby/lib/gitlab/git/gitlab_projects.rb
+++ b/ruby/lib/gitlab/git/gitlab_projects.rb
@@ -73,6 +73,7 @@ module Gitlab
           logger.info "Pushing #{branches.count} branches from #{repository_absolute_path} to remote #{remote_name}"
 
           cmd = %W(#{Gitlab.config.git.bin_path} push)
+          cmd << '--porcelain'
           cmd << '--force' if force
           cmd += %W(-- #{remote_name}).concat(branches)
 
diff --git a/ruby/lib/gitlab/git/push_results.rb b/ruby/lib/gitlab/git/push_results.rb
new file mode 100644
index 000000000..6f18fa751
--- /dev/null
+++ b/ruby/lib/gitlab/git/push_results.rb
@@ -0,0 +1,75 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module Git
+    # Parses the output of a `git push --porcelain` command
+    class PushResults
+      attr_reader :all
+
+      def initialize(raw_output)
+        # If --porcelain is used, then each line of the output is of the form:
+        #     <flag> \t <from>:<to> \t <summary> (<reason>)
+        #
+        # See https://git-scm.com/docs/git-push#_output
+        @all = raw_output.each_line.map do |line|
+          line.chomp!
+
+          fields = line.split(" \t ", 3)
+
+          # Sanity check for porcelain output
+          next unless fields.size >= 3
+
+          flag = fields.shift
+          next unless Result.valid_flag?(flag)
+
+          from, to = fields.shift.split(':')
+          summary = fields.shift
+
+          Result.new(flag, from, to, summary)
+        end.compact
+      end
+
+      # Returns an Array of branch names that were not rejected nor up-to-date
+      def accepted_branches
+        all.select(&:accepted?).collect(&:branch_name)
+      end
+
+      # Returns an Array of branch names that were rejected
+      def rejected_branches
+        all.select(&:rejected?).collect(&:branch_name)
+      end
+
+      Result = Struct.new(:flag_char, :from, :to, :summary) do
+        # A single character indicating the status of the ref
+        FLAGS = {
+          ' ' => :fast_forward, # (space) for a successfully pushed fast-forward;
+          '+' => :forced,       # +       for a successful forced update;
+          '-' => :deleted,      # -       for a successfully deleted ref;
+          '*' => :new,          # *       for a successfully pushed new ref;
+          '!' => :rejected,     # !       for a ref that was rejected or failed to push; and
+          '=' => :up_to_date    # =       for a ref that was up to date and did not need pushing.
+        }.freeze
+
+        def self.valid_flag?(flag)
+          FLAGS.key?(flag)
+        end
+
+        def flag
+          FLAGS[flag_char]
+        end
+
+        def rejected?
+          flag == :rejected
+        end
+
+        def accepted?
+          !rejected? && flag != :up_to_date
+        end
+
+        def branch_name
+          to.delete_prefix('refs/heads/')
+        end
+      end
+    end
+  end
+end
diff --git a/ruby/lib/gitlab/git/repository_mirroring.rb b/ruby/lib/gitlab/git/repository_mirroring.rb
index 26534051d..7e0cb99d2 100644
--- a/ruby/lib/gitlab/git/repository_mirroring.rb
+++ b/ruby/lib/gitlab/git/repository_mirroring.rb
@@ -1,3 +1,5 @@
+require_relative 'push_results'
+
 module Gitlab
   module Git
     module RepositoryMirroring
@@ -32,7 +34,20 @@ module Gitlab
       def push_remote_branches(remote_name, branch_names, forced: true, env: {})
         success = @gitlab_projects.push_branches(remote_name, GITLAB_PROJECTS_TIMEOUT, forced, branch_names, env: env)
 
-        success || gitlab_projects_error
+        return true if success
+
+        results = PushResults.new(@gitlab_projects.output)
+
+        accepted_branches = results.accepted_branches.join(', ')
+        rejected_branches = results.rejected_branches.join(', ')
+
+        @gitlab_projects.logger.info(
+          "Failed to push to remote #{remote_name}. " \
+          "Accepted: #{accepted_branches} / " \
+          "Rejected: #{rejected_branches}"
+        )
+
+        gitlab_projects_error
       end
 
       def delete_remote_branches(remote_name, branch_names, env: {})
diff --git a/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb b/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb
index a1ef8b51b..8ff497f87 100644
--- a/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb
+++ b/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb
@@ -60,10 +60,10 @@ describe Gitlab::Git::GitlabProjects do
     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]
+      %W(#{Gitlab.config.git.bin_path} push --porcelain -- #{remote_name}) + branch_names[0, 10]
     end
     let(:cmd2) do
-      %W(#{Gitlab.config.git.bin_path} push -- #{remote_name}) + branch_names[10, 10]
+      %W(#{Gitlab.config.git.bin_path} push --porcelain -- #{remote_name}) + branch_names[10, 10]
     end
 
     subject { gl_projects.push_branches(remote_name, 600, force, branch_names, env: env) }
@@ -84,7 +84,7 @@ describe Gitlab::Git::GitlabProjects do
 
     context 'with --force' do
       let(:branch_names) { ['master'] }
-      let(:cmd) { %W(#{Gitlab.config.git.bin_path} push --force -- #{remote_name} #{branch_names[0]}) }
+      let(:cmd) { %W(#{Gitlab.config.git.bin_path} push --porcelain --force -- #{remote_name} #{branch_names[0]}) }
       let(:force) { true }
 
       it 'executes the command' do
diff --git a/ruby/spec/lib/gitlab/git/push_results_spec.rb b/ruby/spec/lib/gitlab/git/push_results_spec.rb
new file mode 100644
index 000000000..438172180
--- /dev/null
+++ b/ruby/spec/lib/gitlab/git/push_results_spec.rb
@@ -0,0 +1,70 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Git::PushResults do
+  it 'parses porcelain output' do
+    output = <<~OUTPUT
+      To gitlab.com:gitlab-org/security/gitlab-foss.git
+      = \t refs/heads/12-5-stable:refs/heads/12-5-stable \t [up to date]
+      = \t refs/heads/12-6-stable:refs/heads/12-6-stable \t [up to date]
+      * \t refs/heads/rs-some-new-branch:refs/heads/rs-some-new-branch \t [new branch]
+        \t refs/heads/rs-fast-forward:refs/heads/rs-fast-forward \t [fast-forward]
+      - \t refs/heads/rs-deleted:refs/heads/rs-deleted \t [deleted]
+      + \t refs/heads/rs-forced:refs/heads/rs-forced \t [forced]
+      ! \t refs/heads/12-7-stable:refs/heads/12-7-stable \t [rejected] (fetch first)
+      Done
+      error: failed to push some refs to 'git@gitlab.com:gitlab-org/security/gitlab-foss.git'
+      hint: Updates were rejected because the remote contains work that you do
+      hint: not have locally. This is usually caused by another repository pushing
+      hint: to the same ref. You may want to first integrate the remote changes
+      hint: (e.g., 'git pull ...') before pushing again.
+      hint: See the 'Note about fast-forwards' in 'git push --help' for details.
+    OUTPUT
+
+    results = described_class.new(output)
+
+    expect(results.all.size).to eq(7)
+    expect(results.accepted_branches).to contain_exactly(
+      'rs-some-new-branch',
+      'rs-fast-forward',
+      'rs-forced',
+      'rs-deleted'
+    )
+    expect(results.rejected_branches).to contain_exactly('12-7-stable')
+  end
+
+  it 'ignores non-porcelain output' do
+    output = <<~OUTPUT
+      remote: GitLab: You are not allowed to force push code to a protected branch on this project.
+      To
+      ! [remote rejected]         12-5-stable -> 12-5-stable (pre-receive hook declined)
+      ! [remote rejected]         12-6-stable -> 12-6-stable (pre-receive hook declined)
+      ! [remote rejected]         12-7-stable -> 12-7-stable (pre-receive hook declined)
+      ! [remote rejected]         master -> master (pre-receive hook declined)
+      error: failed to push some refs to '[FILTERED]@gitlab.com/gitlab-org/security/gitlab-foss.git'
+    OUTPUT
+
+    expect(described_class.new(output).all).to eq([])
+  end
+
+  it 'handles output without any recognizable flags' do
+    output = <<~OUTPUT
+      To gitlab.com:gitlab-org/security/gitlab-foss.git
+      Done
+      hint: Updates were rejected because the remote contains work that you do
+      hint: not have locally. This is usually caused by another repository pushing
+      hint: to the same ref. You may want to first integrate the remote changes
+      hint: (e.g., 'git pull ...') before pushing again.
+      hint: See the 'Note about fast-forwards' in 'git push --help' for details.
+    OUTPUT
+
+    expect(described_class.new(output).all).to eq([])
+  end
+
+  it 'handles invalid output' do
+    output = 'You get nothing!'
+
+    expect(described_class.new(output).all).to eq([])
+  end
+end
diff --git a/ruby/spec/lib/gitlab/git/repository_mirroring_spec.rb b/ruby/spec/lib/gitlab/git/repository_mirroring_spec.rb
new file mode 100644
index 000000000..57edf5af4
--- /dev/null
+++ b/ruby/spec/lib/gitlab/git/repository_mirroring_spec.rb
@@ -0,0 +1,65 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Git::RepositoryMirroring do
+  class FakeRepository
+    include Gitlab::Git::RepositoryMirroring
+
+    def initialize(projects_stub)
+      @gitlab_projects = projects_stub
+    end
+
+    def gitlab_projects_error
+      raise Gitlab::Git::CommandError, @gitlab_projects.output
+    end
+  end
+
+  describe '#push_remote_branches' do
+    let(:projects_stub) { double.as_null_object }
+
+    subject(:repository) { FakeRepository.new(projects_stub) }
+
+    context 'with a successful first push' do
+      it 'returns true' do
+        expect(projects_stub).to receive(:push_branches)
+          .with('remote_a', anything, true, %w[master], env: {})
+          .once
+          .and_return(true)
+
+        expect(projects_stub).not_to receive(:output)
+
+        expect(repository.push_remote_branches('remote_a', %w[master])).to eq(true)
+      end
+    end
+
+    context 'with a failed push' do
+      it 'logs a list of branch results and raises CommandError' do
+        output = "Oh no, push mirroring failed!"
+        logger = spy
+
+        # Once for parsing, once for the exception
+        allow(projects_stub).to receive(:output).twice.and_return(output)
+        allow(projects_stub).to receive(:logger).and_return(logger)
+
+        push_results = double(
+          accepted_branches: %w[develop],
+          rejected_branches: %w[master]
+        )
+        expect(Gitlab::Git::PushResults).to receive(:new)
+          .with(output)
+          .and_return(push_results)
+
+        # Push fails
+        expect(projects_stub).to receive(:push_branches).and_return(false)
+
+        # The CommandError gets re-raised, matching existing behavior
+        expect { repository.push_remote_branches('remote_a', %w[master develop]) }
+          .to raise_error(Gitlab::Git::CommandError, output)
+
+        # Ensure we logged a message with the PushResults info
+        expect(logger).to have_received(:info).with(%r{Accepted: develop / Rejected: master})
+      end
+    end
+  end
+end