Merge branch 'rs-remove-push-results' into 'master'

Remove PushResult parser and logging

Closes #2687

See merge request gitlab-org/gitaly!2085

6 files changed, 6 insertions, 199 deletions

diff --git a/ruby/lib/gitlab/git/gitlab_projects.rb b/ruby/lib/gitlab/git/gitlab_projects.rb
index aa1a27cd2..a23cdfa91 100644
--- a/ruby/lib/gitlab/git/gitlab_projects.rb
+++ b/ruby/lib/gitlab/git/gitlab_projects.rb
@@ -73,7 +73,6 @@ 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
deleted file mode 100644
index dd4ee2795..000000000
--- a/ruby/lib/gitlab/git/push_results.rb
+++ /dev/null
@@ -1,76 +0,0 @@
-# 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
-        # and https://github.com/git/git/blob/v2.25.1/transport.c#L466-L475
-        @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 ref names that were not rejected nor up-to-date
-      def accepted_refs
-        all.select(&:accepted?).collect(&:ref_name)
-      end
-
-      # Returns an Array of ref names that were rejected
-      def rejected_refs
-        all.select(&:rejected?).collect(&:ref_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 ref_name
-          to.sub(%r{\Arefs/(heads|tags)/}, '')
-        end
-      end
-    end
-  end
-end
diff --git a/ruby/lib/gitlab/git/repository_mirroring.rb b/ruby/lib/gitlab/git/repository_mirroring.rb
index ce6faec36..e564a4cbd 100644
--- a/ruby/lib/gitlab/git/repository_mirroring.rb
+++ b/ruby/lib/gitlab/git/repository_mirroring.rb
@@ -1,5 +1,3 @@
-require_relative 'push_results'
-
 module Gitlab
   module Git
     module RepositoryMirroring
@@ -34,20 +32,7 @@ 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)
 
-        return true if success
-
-        results = PushResults.new(@gitlab_projects.output)
-
-        accepted_refs = results.accepted_refs.join(', ')
-        rejected_refs = results.rejected_refs.join(', ')
-
-        @gitlab_projects.logger.info(
-          "Failed to push to remote #{remote_name}. " \
-          "Accepted: #{accepted_refs} / " \
-          "Rejected: #{rejected_refs}"
-        )
-
-        gitlab_projects_error
+        success || 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 8ff497f87..a1ef8b51b 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 --porcelain -- #{remote_name}) + branch_names[0, 10]
+      %W(#{Gitlab.config.git.bin_path} push -- #{remote_name}) + branch_names[0, 10]
     end
     let(:cmd2) do
-      %W(#{Gitlab.config.git.bin_path} push --porcelain -- #{remote_name}) + branch_names[10, 10]
+      %W(#{Gitlab.config.git.bin_path} push -- #{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 --porcelain --force -- #{remote_name} #{branch_names[0]}) }
+      let(:cmd) { %W(#{Gitlab.config.git.bin_path} push --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
deleted file mode 100644
index 4a2cd2e02..000000000
--- a/ruby/spec/lib/gitlab/git/push_results_spec.rb
+++ /dev/null
@@ -1,84 +0,0 @@
-# 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
-      =\trefs/heads/12-5-stable:refs/heads/12-5-stable\t[up to date]
-      =\trefs/heads/12-6-stable:refs/heads/12-6-stable\t[up to date]
-      *\trefs/heads/rs-some-new-branch:refs/heads/rs-some-new-branch\t[new branch]
-       \trefs/heads/rs-fast-forward:refs/heads/rs-fast-forward\t[fast-forward]
-      -\trefs/heads/rs-deleted:refs/heads/rs-deleted\t[deleted]
-      +\trefs/heads/rs-forced:refs/heads/rs-forced\t[forced]
-      !\trefs/heads/12-7-stable:refs/heads/12-7-stable\t[rejected] (fetch first)
-      *\trefs/tags/v1.2.3:refs/tags/v1.2.3\t[new tag]
-      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(8)
-    expect(results.accepted_refs).to contain_exactly(
-      'rs-some-new-branch',
-      'rs-fast-forward',
-      'rs-forced',
-      'rs-deleted',
-      'v1.2.3'
-    )
-    expect(results.rejected_refs).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
-
-  describe Gitlab::Git::PushResults::Result do
-    describe '#ref_name' do
-      it 'deletes only one prefix' do
-        # It's  valid (albeit insane) for a branch to be named `refs/tags/foo`
-        ref_name = 'refs/heads/refs/tags/branch'
-        result = described_class.new('!', ref_name, ref_name, '')
-
-        expect(result.ref_name).to eq('refs/tags/branch')
-      end
-    end
-  end
-end
diff --git a/ruby/spec/lib/gitlab/git/repository_mirroring_spec.rb b/ruby/spec/lib/gitlab/git/repository_mirroring_spec.rb
index 927cb863e..a0efc83f8 100644
--- a/ruby/spec/lib/gitlab/git/repository_mirroring_spec.rb
+++ b/ruby/spec/lib/gitlab/git/repository_mirroring_spec.rb
@@ -34,31 +34,14 @@ describe Gitlab::Git::RepositoryMirroring do
     end
 
     context 'with a failed push' do
-      it 'logs a list of branch results and raises CommandError' do
+      it 'raises an error' do
         output = "Oh no, push mirroring failed!"
-        logger = spy
+        allow(projects_stub).to receive(:output).and_return(output)
 
-        # 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_refs: %w[develop],
-          rejected_refs: %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