Merge branch 'pks-git-more-robust-version-parsing' into 'master'

git: Fix race between cancelling git-version(1) and reading its output

Closes #4740

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5344

Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Pavlo Strokov <pstrokov@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Reviewed-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>

2 files changed, 16 insertions, 20 deletions

diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go
index a7ceca341..e130cc630 100644
--- a/internal/git/command_factory.go
+++ b/internal/git/command_factory.go
@@ -1,6 +1,7 @@
 package git
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"fmt"
@@ -362,20 +363,24 @@ func (cf *ExecCommandFactory) GitVersion(ctx context.Context) (Version, error) {
 	// Furthermore, note that we're not using `newCommand()` but instead hand-craft the command.
 	// This is required to avoid a cyclic dependency when we need to check the version in
 	// `newCommand()` itself.
-	cmd, err := command.New(ctx, []string{execEnv.BinaryPath, "version"}, command.WithEnvironment(execEnv.EnvironmentVariables))
+	var versionBuffer bytes.Buffer
+	cmd, err := command.New(ctx, []string{execEnv.BinaryPath, "version"},
+		command.WithEnvironment(execEnv.EnvironmentVariables),
+		command.WithStdout(&versionBuffer),
+	)
 	if err != nil {
 		return Version{}, fmt.Errorf("spawning version command: %w", err)
 	}
 
-	gitVersion, err := parseVersionFromCommand(cmd)
-	if err != nil {
-		return Version{}, err
-	}
-
 	if err := cmd.Wait(); err != nil {
 		return Version{}, fmt.Errorf("waiting for version: %w", err)
 	}
 
+	gitVersion, err := parseVersionOutput(versionBuffer.Bytes())
+	if err != nil {
+		return Version{}, err
+	}
+
 	cf.cachedGitVersionByBinary[gitBinary] = cachedGitVersion{
 		version: gitVersion,
 		stat:    stat,
diff --git a/internal/git/version.go b/internal/git/version.go
index 2d13c1187..4690e96aa 100644
--- a/internal/git/version.go
+++ b/internal/git/version.go
@@ -1,12 +1,10 @@
 package git
 
 import (
+	"bytes"
 	"fmt"
-	"io"
 	"strconv"
 	"strings"
-
-	"gitlab.com/gitlab-org/gitaly/v15/internal/command"
 )
 
 // minimumVersion is the minimum required Git version. If updating this version, be sure to
@@ -38,17 +36,10 @@ type Version struct {
 	gl                  uint32
 }
 
-func parseVersionFromCommand(cmd *command.Command) (Version, error) {
-	versionOutput, err := io.ReadAll(cmd)
-	if err != nil {
-		return Version{}, fmt.Errorf("reading version output: %w", err)
-	}
-
-	if err := cmd.Wait(); err != nil {
-		return Version{}, fmt.Errorf("waiting for version command: %w", err)
-	}
-
-	trimmedVersionOutput := strings.Trim(string(versionOutput), " \n")
+// parseVersionOutput parses output returned by git-version(1). It is expected to be in the format
+// "git version 2.39.1.gl1".
+func parseVersionOutput(versionOutput []byte) (Version, error) {
+	trimmedVersionOutput := string(bytes.Trim(versionOutput, " \n"))
 	versionString := strings.SplitN(trimmedVersionOutput, " ", 3)
 	if len(versionString) != 3 {
 		return Version{}, fmt.Errorf("invalid version format: %q", string(versionOutput))