Merge branch 'sh-fix-issue-1329' into 'master'

Fix 503 errors when Git outputs warnings to stderr

Closes #1329

See merge request gitlab-org/gitaly!1024

6 files changed, 79 insertions, 15 deletions

diff --git a/changelogs/unreleased/sh-fix-issue-1329.yml b/changelogs/unreleased/sh-fix-issue-1329.yml
new file mode 100644
index 000000000..16f9503eb
--- /dev/null
+++ b/changelogs/unreleased/sh-fix-issue-1329.yml
@@ -0,0 +1,5 @@
+---
+title: Fix 503 errors when Git outputs warnings to stderr
+merge_request: 1024
+author:
+type: fixed
diff --git a/internal/service/commit/find_commits_test.go b/internal/service/commit/find_commits_test.go
index 4027414e7..51147bd41 100644
--- a/internal/service/commit/find_commits_test.go
+++ b/internal/service/commit/find_commits_test.go
@@ -367,6 +367,52 @@ func TestSuccessfulFindCommitsRequestWithAltGitObjectDirs(t *testing.T) {
 	}
 }
 
+func TestSuccessfulFindCommitsRequestWithAmbiguousRef(t *testing.T) {
+	server, serverSocketPath := startTestServices(t)
+	defer server.Stop()
+
+	client, conn := newCommitServiceClient(t, serverSocketPath)
+	defer conn.Close()
+
+	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepoWithWorktree(t)
+	defer cleanupFn()
+
+	// These are arbitrary SHAs in the repository. The important part is
+	// that we create a branch using one of them with a different SHA so
+	// that Git detects an ambiguous reference.
+	branchName := "1e292f8fedd741b75372e19097c76d327140c312"
+	commitSha := "6907208d755b60ebeacb2e9dfea74c92c3449a1f"
+
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "checkout", "-b", branchName, commitSha)
+
+	request := &gitalypb.FindCommitsRequest{
+		Repository: testRepo,
+		Revision:   []byte(branchName),
+		Limit:      1,
+	}
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	c, err := client.FindCommits(ctx, request)
+	require.NoError(t, err)
+
+	receivedCommits := []*gitalypb.GitCommit{}
+
+	for {
+		resp, err := c.Recv()
+		if err == io.EOF {
+			break
+		} else if err != nil {
+			t.Fatal(err)
+		}
+
+		receivedCommits = append(receivedCommits, resp.GetCommits()...)
+	}
+
+	require.Equal(t, 1, len(receivedCommits), "number of commits received")
+}
+
 func TestFailureFindCommitsRequest(t *testing.T) {
 	server, serverSocketPath := startTestServices(t)
 	defer server.Stop()
diff --git a/ruby/lib/gitlab/git/popen.rb b/ruby/lib/gitlab/git/popen.rb
index cd86e52e1..5fb595cef 100644
--- a/ruby/lib/gitlab/git/popen.rb
+++ b/ruby/lib/gitlab/git/popen.rb
@@ -5,7 +5,7 @@ module Gitlab
     module Popen
       FAST_GIT_PROCESS_TIMEOUT = 15.seconds
 
-      def popen(cmd, path, vars = {}, lazy_block: nil)
+      def popen(cmd, path, vars = {}, include_stderr: true, lazy_block: nil)
         raise "System commands must be given as an array of strings" unless cmd.is_a?(Array)
 
         path ||= Dir.pwd
@@ -33,7 +33,7 @@ module Gitlab
             cmd_output << stdout.read
           end
 
-          cmd_output << err_reader.value
+          cmd_output << err_reader.value if include_stderr
           cmd_status = wait_thr.value.exitstatus
         end
 
diff --git a/ruby/lib/gitlab/git/repository.rb b/ruby/lib/gitlab/git/repository.rb
index e0c3f9b82..4d1dc744e 100644
--- a/ruby/lib/gitlab/git/repository.rb
+++ b/ruby/lib/gitlab/git/repository.rb
@@ -450,7 +450,7 @@ module Gitlab
         with_worktree(rebase_path, branch, env: env) do
           run_git!(
             %W[pull --rebase #{remote_repo_path} #{remote_branch}],
-            chdir: rebase_path, env: env
+            chdir: rebase_path, env: env, include_stderr: true
           )
 
           rebase_sha = run_git!(%w[rev-parse HEAD], chdir: rebase_path, env: env).strip
@@ -475,13 +475,13 @@ module Gitlab
         with_worktree(squash_path, branch, sparse_checkout_files: diff_files, env: env) do
           # Apply diff of the `diff_range` to the worktree
           diff = run_git!(%W[diff --binary #{diff_range}])
-          run_git!(%w[apply --index --whitespace=nowarn], chdir: squash_path, env: env) do |stdin|
+          run_git!(%w[apply --index --whitespace=nowarn], chdir: squash_path, env: env, include_stderr: true) do |stdin|
             stdin.binmode
             stdin.write(diff)
           end
 
           # Commit the `diff_range` diff
-          run_git!(%W[commit --no-verify --message #{message}], chdir: squash_path, env: env)
+          run_git!(%W[commit --no-verify --message #{message}], chdir: squash_path, env: env, include_stderr: true)
 
           # Return the squash sha. May print a warning for ambiguous refs, but
           # we can ignore that with `--quiet` and just take the SHA, if present.
@@ -715,7 +715,7 @@ module Gitlab
         source_repository = RemoteRepository.new(source_repository) unless source_repository.is_a?(RemoteRepository)
 
         args = %W[fetch --no-tags -f #{GITALY_INTERNAL_URL} #{source_ref}:#{target_ref}]
-        message, status = run_git(args, env: source_repository.fetch_env)
+        message, status = run_git(args, env: source_repository.fetch_env, include_stderr: true)
         raise Gitlab::Git::CommandError, message if status != 0
 
         target_ref
@@ -838,18 +838,18 @@ module Gitlab
 
       private
 
-      def run_git(args, chdir: path, env: {}, nice: false, lazy_block: nil, &block)
+      def run_git(args, chdir: path, env: {}, nice: false, include_stderr: false, lazy_block: nil, &block)
         cmd = [Gitlab.config.git.bin_path, *args]
         cmd.unshift("nice") if nice
 
         object_directories = alternate_object_directories
         env['GIT_ALTERNATE_OBJECT_DIRECTORIES'] = object_directories.join(File::PATH_SEPARATOR) if object_directories.any?
 
-        popen(cmd, chdir, env, lazy_block: lazy_block, &block)
+        popen(cmd, chdir, env, include_stderr: include_stderr, lazy_block: lazy_block, &block)
       end
 
-      def run_git!(args, chdir: path, env: {}, nice: false, lazy_block: nil, &block)
-        output, status = run_git(args, chdir: chdir, env: env, nice: nice, lazy_block: lazy_block, &block)
+      def run_git!(args, chdir: path, env: {}, nice: false, include_stderr: false, lazy_block: nil, &block)
+        output, status = run_git(args, chdir: chdir, env: env, nice: nice, include_stderr: include_stderr, lazy_block: lazy_block, &block)
 
         raise GitError, output unless status.zero?
 
@@ -920,7 +920,8 @@ module Gitlab
           cmd += Array(options[:path])
         end
 
-        raw_output, _status = run_git(cmd)
+        # Disable stderr because Git can show warnings that corrupt the output stream
+        raw_output, _status = run_git(cmd, include_stderr: false)
         lines = offset_in_ruby ? raw_output.lines.drop(offset) : raw_output.lines
 
         lines.map! { |c| Rugged::Commit.new(rugged, c.strip) }
@@ -956,7 +957,7 @@ module Gitlab
           "delete #{ref}\x00\x00"
         end
 
-        message, status = run_git(%w[update-ref --stdin -z]) do |stdin|
+        message, status = run_git(%w[update-ref --stdin -z], include_stderr: true) do |stdin|
           stdin.write(instructions.join)
         end
 
@@ -1045,7 +1046,7 @@ module Gitlab
       # this can be very expensive, so set up sparse checkout for the worktree
       # to only check out the files we're interested in.
       def configure_sparse_checkout(worktree_git_path, files)
-        run_git!(%w[config core.sparseCheckout true])
+        run_git!(%w[config core.sparseCheckout true], include_stderr: true)
 
         return if files.empty?
 
diff --git a/ruby/spec/lib/gitlab/git/popen_spec.rb b/ruby/spec/lib/gitlab/git/popen_spec.rb
index 25cc8cf69..8b09dd313 100644
--- a/ruby/spec/lib/gitlab/git/popen_spec.rb
+++ b/ruby/spec/lib/gitlab/git/popen_spec.rb
@@ -30,6 +30,15 @@ describe 'Gitlab::Git::Popen' do
       it { expect(output).to include('No such file or directory') }
     end
 
+    context 'when stderr is not included' do
+      let(:result) { klass.new.popen(%w(cat NOTHING), path, include_stderr: false) }
+      let(:output) { result.first }
+      let(:status) { result.last }
+
+      it { expect(status).to eq(1) }
+      it { expect(output).to eq('') }
+    end
+
     context 'unsafe string command' do
       it 'raises an error when it gets called with a string argument' do
         expect { klass.new.popen('ls', path) }.to raise_error(RuntimeError)
diff --git a/ruby/spec/lib/gitlab/git/rev_list_spec.rb b/ruby/spec/lib/gitlab/git/rev_list_spec.rb
index c6601afb9..fd0cb45f2 100644
--- a/ruby/spec/lib/gitlab/git/rev_list_spec.rb
+++ b/ruby/spec/lib/gitlab/git/rev_list_spec.rb
@@ -17,14 +17,17 @@ describe Gitlab::Git::RevList do
       args_for_popen(additional_args),
       repo_path,
       {},
-      hash_including(lazy_block: with_lazy_block ? anything : nil)
+      hash_including(include_stderr: false,
+                     lazy_block: with_lazy_block ? anything : nil)
     ]
 
-    expect(repository).to receive(:popen).with(*params) do |*_, lazy_block:|
+    # rubocop:disable Lint/UnusedBlockArgument
+    expect(repository).to receive(:popen).with(*params) do |*_, include_stderr:, lazy_block:|
       output = lazy_block.call(output.lines.lazy.map(&:chomp)) if with_lazy_block
 
       [output, 0]
     end
+    # rubocop:enable Lint/UnusedBlockArgument
   end
 
   context "#new_refs" do