Fix 503 errors when Git outputs warnings to stderr

Git can emit warnings when a command is run, such as when an ambiguous
ref is used or a deprecated feature is used (e.g. `/info/grafts`).
Gitaly will parse these warnings as OIDs and attempt to deference them,
resulting in an error, "2:TypeError: The given OID is too long".

To fix this, we provide an optional parameter to control the
concatenation of the stderr stream to the resulting output. In cases
where we want to see the full message (e.g. when we log an error
message), we include the stderr output. Otherwise, by default for
Repository commands, we disable this flag.

Closes https://gitlab.com/gitlab-org/gitaly/issues/1329

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