Merge branch 'pks-git-minimum-v2.31.0' into 'master'

git: Bump minimum git version to git v2.31.0

See merge request gitlab-org/gitaly!3340

11 files changed, 30 insertions, 228 deletions

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 23668f9d4..a69c2f1c7 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -119,8 +119,6 @@ build:
     matrix:
       - GO_VERSION: [ "1.14", "1.15" ]
         GIT_VERSION: [ "v2.31.1" ]
-      - GO_VERSION: "1.15"
-        GIT_VERSION: "v2.29.0"
 
 binaries:
   <<: *cache_definition
@@ -167,9 +165,6 @@ test:
       - GO_VERSION: "1.15"
         GIT_VERSION: "v2.31.1"
         TARGET: [ test-with-proxies, test-with-praefect, race-go ]
-      - GO_VERSION: "1.15"
-        GIT_VERSION: "v2.29.0"
-        TARGET: test
 
 nightly:git:
   <<: *test_definition
diff --git a/README.md b/README.md
index 43d072161..3d7070ac0 100644
--- a/README.md
+++ b/README.md
@@ -58,7 +58,7 @@ Gitaly requires Go 1.13.9 or newer and Ruby 2.7. Run `make` to download
 and compile Ruby dependencies, and to compile the Gitaly Go
 executable.
 
-Gitaly uses `git`. Versions `2.29.0` and newer are supported.
+Gitaly uses `git`. Versions `2.31.0` and newer are supported.
 
 ## Configuration
 
diff --git a/changelogs/unreleased/pks-git-minimum-v2-31-0.yml b/changelogs/unreleased/pks-git-minimum-v2-31-0.yml
new file mode 100644
index 000000000..28634a514
--- /dev/null
+++ b/changelogs/unreleased/pks-git-minimum-v2-31-0.yml
@@ -0,0 +1,5 @@
+---
+title: 'git: Bump minimum git version to git v2.31.0'
+merge_request: 3340
+author:
+type: added
diff --git a/internal/git/command_options_test.go b/internal/git/command_options_test.go
index 5b26c9f16..9ccd1d964 100644
--- a/internal/git/command_options_test.go
+++ b/internal/git/command_options_test.go
@@ -277,13 +277,6 @@ func TestWithConfigEnv(t *testing.T) {
 
 	gitCmdFactory := NewExecCommandFactory(cfg)
 
-	version, err := CurrentVersion(ctx, gitCmdFactory)
-	require.NoError(t, err)
-
-	if !version.SupportsConfigEnv() {
-		t.Skip("git does not support config env")
-	}
-
 	for _, tc := range []struct {
 		desc           string
 		configPairs    []ConfigPair
diff --git a/internal/git/objectpool/fetch.go b/internal/git/objectpool/fetch.go
index 106ee6a9e..ad7e78df3 100644
--- a/internal/git/objectpool/fetch.go
+++ b/internal/git/objectpool/fetch.go
@@ -63,14 +63,9 @@ func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *gitalypb.Repos
 		return err
 	}
 
-	gitVersion, err := git.CurrentVersion(ctx, o.gitCmdFactory)
-	if err != nil {
-		return err
-	}
-
 	var opts []git.CmdOpt
 	flags := []git.Option{git.Flag{Name: "--quiet"}}
-	if gitVersion.SupportsAtomicFetches() && featureflag.IsEnabled(ctx, featureflag.AtomicFetch) {
+	if featureflag.IsEnabled(ctx, featureflag.AtomicFetch) {
 		flags = append(flags, git.Flag{
 			Name: "--atomic",
 		})
diff --git a/internal/git/version.go b/internal/git/version.go
index 923aa0135..db31701ea 100644
--- a/internal/git/version.go
+++ b/internal/git/version.go
@@ -13,14 +13,12 @@ var (
 	// also update the following locations:
 	// - https://gitlab.com/gitlab-org/gitaly/blob/master/README.md#installation
 	// - https://gitlab.com/gitlab-org/gitaly/blob/master/.gitlab-ci.yml
-	// - https://gitlab.com/gitlab-org/gitlab-build-images/blob/master/.gitlab-ci.yml
 	// - https://gitlab.com/gitlab-org/gitlab-foss/blob/master/.gitlab-ci.yml
 	// - https://gitlab.com/gitlab-org/gitlab-foss/blob/master/lib/system_check/app/git_version_check.rb
-	// - https://gitlab.com/gitlab-org/build/CNG/blob/master/ci_files/variables.yml
 	minimumVersion = Version{
-		versionString: "2.29.0",
+		versionString: "2.31.0",
 		major:         2,
-		minor:         29,
+		minor:         31,
 		patch:         0,
 		rc:            false,
 	}
@@ -76,21 +74,6 @@ func (v Version) IsSupported() bool {
 	return !v.LessThan(minimumVersion)
 }
 
-// SupportsConfigEnv checks whether git supports the config environment variables GIT_CONFIG_COUNT,
-// GIT_CONFIG_KEY and GIT_CONFIG_VALUE.
-func (v Version) SupportsConfigEnv() bool {
-	return !v.LessThan(Version{
-		major: 2, minor: 31, patch: 0,
-	})
-}
-
-// SupportsAtomicFetches checks whether git-fetch supports the `--atomic` flag.
-func (v Version) SupportsAtomicFetches() bool {
-	return !v.LessThan(Version{
-		major: 2, minor: 31, patch: 0,
-	})
-}
-
 // LessThan determines whether the version is older than another version.
 func (v Version) LessThan(other Version) bool {
 	switch {
diff --git a/internal/git/version_test.go b/internal/git/version_test.go
index 57d8d3098..e0c705d2f 100644
--- a/internal/git/version_test.go
+++ b/internal/git/version_test.go
@@ -117,45 +117,7 @@ func TestVersion_IsSupported(t *testing.T) {
 		{"2.24.0-rc0", false},
 		{"2.24.0", false},
 		{"2.25.0", false},
-		{"2.29.0-rc0", false},
-		{"2.29.0", true},
-		{"2.29.1", true},
-		{"3.0.0", true},
-	} {
-		version, err := parseVersion(tc.version)
-		require.NoError(t, err)
-		require.Equal(t, tc.expect, version.IsSupported())
-	}
-}
-
-func TestVersion_SupportsConfigEnv(t *testing.T) {
-	for _, tc := range []struct {
-		version string
-		expect  bool
-	}{
-		{"2.20.0", false},
-		{"2.24.0-rc0", false},
-		{"2.24.0", false},
-		{"2.25.0", false},
-		{"2.31.0", true},
-		{"2.31.1", true},
-		{"3.0.0", true},
-	} {
-		t.Run(tc.version, func(t *testing.T) {
-			version, err := parseVersion(tc.version)
-			require.NoError(t, err)
-			require.Equal(t, tc.expect, version.SupportsConfigEnv())
-		})
-	}
-}
-
-func TestVersion_SupportsAtomicFetches(t *testing.T) {
-	for _, tc := range []struct {
-		version string
-		expect  bool
-	}{
-		{"2.25.0", false},
-		{"2.30.1", false},
+		{"2.30.0", false},
 		{"2.31.0-rc0", false},
 		{"2.31.0", true},
 		{"2.31.1", true},
@@ -163,6 +125,6 @@ func TestVersion_SupportsAtomicFetches(t *testing.T) {
 	} {
 		version, err := parseVersion(tc.version)
 		require.NoError(t, err)
-		require.Equal(t, tc.expect, version.SupportsAtomicFetches())
+		require.Equal(t, tc.expect, version.IsSupported())
 	}
 }
diff --git a/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go b/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go
index 00dfcbb3d..04cc1be5b 100644
--- a/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go
+++ b/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go
@@ -112,11 +112,8 @@ func TestFetchIntoObjectPool_hooks(t *testing.T) {
 			Repack:     true,
 		}
 
-		gitVersion, err := git.CurrentVersion(ctx, gitCmdFactory)
-		require.NoError(t, err)
-
 		_, err = client.FetchIntoObjectPool(ctx, req)
-		if gitVersion.SupportsAtomicFetches() && featureflag.IsEnabled(ctx, featureflag.AtomicFetch) {
+		if featureflag.IsEnabled(ctx, featureflag.AtomicFetch) {
 			require.Equal(t, status.Error(codes.Internal, "exit status 128"), err)
 		} else {
 			require.NoError(t, err)
diff --git a/internal/gitaly/service/remote/fetch_internal_remote.go b/internal/gitaly/service/remote/fetch_internal_remote.go
index 7f5c49bb9..e0483db91 100644
--- a/internal/gitaly/service/remote/fetch_internal_remote.go
+++ b/internal/gitaly/service/remote/fetch_internal_remote.go
@@ -27,11 +27,6 @@ func (s *server) FetchInternalRemote(ctx context.Context, req *gitalypb.FetchInt
 		return nil, status.Errorf(codes.InvalidArgument, "FetchInternalRemote: %v", err)
 	}
 
-	gitVersion, err := git.CurrentVersion(ctx, s.gitCmdFactory)
-	if err != nil {
-		return nil, status.Errorf(codes.Internal, "cannot determine current git version: %v", err)
-	}
-
 	env, err := gitalyssh.UploadPackEnv(ctx, s.cfg, &gitalypb.SSHUploadPackRequest{Repository: req.RemoteRepository})
 	if err != nil {
 		return nil, fmt.Errorf("upload pack environment: %w", err)
@@ -45,7 +40,7 @@ func (s *server) FetchInternalRemote(ctx context.Context, req *gitalypb.FetchInt
 		git.WithStderr(stderr),
 	}
 
-	if gitVersion.SupportsAtomicFetches() && featureflag.IsEnabled(ctx, featureflag.AtomicFetch) {
+	if featureflag.IsEnabled(ctx, featureflag.AtomicFetch) {
 		flags = append(flags, git.Flag{
 			Name: "--atomic",
 		})
diff --git a/internal/gitaly/service/remote/fetch_internal_remote_test.go b/internal/gitaly/service/remote/fetch_internal_remote_test.go
index c6f387608..b34dda6e8 100644
--- a/internal/gitaly/service/remote/fetch_internal_remote_test.go
+++ b/internal/gitaly/service/remote/fetch_internal_remote_test.go
@@ -130,10 +130,7 @@ func testSuccessfulFetchInternalRemote(t *testing.T, ctx context.Context) {
 		},
 	)
 
-	gitVersion, err := git.CurrentVersion(ctx, gitCmdFactory)
-	require.NoError(t, err)
-
-	if gitVersion.SupportsAtomicFetches() && featureflag.IsEnabled(ctx, featureflag.AtomicFetch) {
+	if featureflag.IsEnabled(ctx, featureflag.AtomicFetch) {
 		require.Equal(t, 2, hookManager.called)
 	} else {
 		require.Equal(t, 0, hookManager.called)
diff --git a/internal/gitaly/service/repository/fetch_remote.go b/internal/gitaly/service/repository/fetch_remote.go
index 39d38fab9..751999eaa 100644
--- a/internal/gitaly/service/repository/fetch_remote.go
+++ b/internal/gitaly/service/repository/fetch_remote.go
@@ -9,15 +9,10 @@ import (
 	"strings"
 	"time"
 
-	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
-	"github.com/sirupsen/logrus"
-	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/errors"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/localrepo"
-	"gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc/status"
 )
@@ -44,120 +39,29 @@ func (s *server) FetchRemote(ctx context.Context, req *gitalypb.FetchRemoteReque
 	remoteName := req.GetRemote()
 
 	if params := req.GetRemoteParams(); params != nil {
-		gitVersion, err := git.CurrentVersion(ctx, s.gitCmdFactory)
-		if err != nil {
-			return nil, fmt.Errorf("cannot determine current git version: %w", err)
-		}
+		remoteName = "inmemory"
 
 		remoteURL := params.GetUrl()
 		refspecs := s.getRefspecs(params.GetMirrorRefmaps())
 
-		if gitVersion.SupportsConfigEnv() {
-			remoteName = "inmemory"
-
-			config := []git.ConfigPair{
-				{Key: "remote.inmemory.url", Value: remoteURL},
-			}
-
-			for _, refspec := range refspecs {
-				config = append(config, git.ConfigPair{
-					Key: "remote.inmemory.fetch", Value: refspec,
-				})
-			}
-
-			if authHeader := params.GetHttpAuthorizationHeader(); authHeader != "" {
-				config = append(config, git.ConfigPair{
-					Key:   fmt.Sprintf("http.%s.extraHeader", remoteURL),
-					Value: "Authorization: " + authHeader,
-				})
-			}
-
-			opts.CommandOptions = append(opts.CommandOptions, git.WithConfigEnv(config...))
-		} else {
-			remoteSuffix, err := text.RandomHex(16)
-			if err != nil {
-				return nil, fmt.Errorf("cannot generate remote suffix: %w", err)
-			}
-
-			// We're generating a random name for the remote. We do not use a static name here
-			// and neither do we use a user-provided one as we'll be removing the remote after
-			// this RPC again, which also removes all its contained references.
-			//
-			// Ideally, we'd just use an in-memory remote either by fetching via URL directly or
-			// by injecting an in-memory remote via git configuration. But the former is not
-			// possible as it may leak credentials stored in URLs, while the latter is not
-			// possible yet because git has no official way to inject configuration via the
-			// environment yet. That is about to change though with git v2.31.0, so we can
-			// migrate this to use anonymous remotes as soon as we require that as minimum
-			// version.
-			remoteName = fmt.Sprintf("tmp-%s", remoteSuffix)
-
-			if err := s.setRemote(ctx, repo, remoteName, remoteURL); err != nil {
-				return nil, fmt.Errorf("set remote: %w", err)
-			}
-
-			defer func(parentCtx context.Context) {
-				ctx, cancel := context.WithCancel(command.SuppressCancellation(parentCtx))
-				defer cancel()
-
-				// we pass context as it may be overridden in case timeout is set for the call
-				if err := s.removeRemote(ctx, repo, remoteName); err != nil {
-					ctxlogrus.Extract(ctx).WithError(err).WithFields(logrus.Fields{
-						"remote":  remoteName,
-						"storage": req.GetRepository().GetStorageName(),
-						"path":    req.GetRepository().GetRelativePath(),
-					}).Error("removal of remote failed")
-				}
-			}(ctx)
-
-			for _, refspec := range refspecs {
-				opts.CommandOptions = append(opts.CommandOptions,
-					git.WithConfig(git.ConfigPair{Key: "remote." + remoteName + ".fetch", Value: refspec}),
-				)
-			}
-
-			if params.GetHttpAuthorizationHeader() != "" {
-				client, err := s.ruby.RepositoryServiceClient(ctx)
-				if err != nil {
-					return nil, err
-				}
-
-				clientCtx, err := rubyserver.SetHeaders(ctx, s.locator, req.GetRepository())
-				if err != nil {
-					return nil, err
-				}
-
-				// currently it is only possible way to set config value without exposing it to outside (won't be listed in 'ps')
-				extraHeaderKey := "http." + remoteURL + ".extraHeader"
-
-				if _, err := client.SetConfig(clientCtx, &gitalypb.SetConfigRequest{
-					Repository: req.GetRepository(),
-					Entries: []*gitalypb.SetConfigRequest_Entry{{
-						Key:   extraHeaderKey,
-						Value: &gitalypb.SetConfigRequest_Entry_ValueStr{ValueStr: "Authorization: " + params.GetHttpAuthorizationHeader()},
-					}},
-				}); err != nil {
-					return nil, helper.ErrInternal(fmt.Errorf("set extra header: %w", err))
-				}
+		config := []git.ConfigPair{
+			{Key: "remote.inmemory.url", Value: remoteURL},
+		}
 
-				defer func() {
-					ctx, cancel := context.WithCancel(command.SuppressCancellation(clientCtx))
-					defer cancel()
+		for _, refspec := range refspecs {
+			config = append(config, git.ConfigPair{
+				Key: "remote.inmemory.fetch", Value: refspec,
+			})
+		}
 
-					// currently it is only possible way to set config value without exposing it to outside (won't be listed in 'ps')
-					if _, err := client.DeleteConfig(ctx, &gitalypb.DeleteConfigRequest{
-						Repository: req.Repository,
-						Keys:       []string{extraHeaderKey},
-					}); err != nil {
-						ctxlogrus.Extract(ctx).WithError(err).WithFields(logrus.Fields{
-							"remote":  remoteName,
-							"storage": req.GetRepository().GetStorageName(),
-							"path":    req.GetRepository().GetRelativePath(),
-						}).Error("removal of extra header config failed")
-					}
-				}()
-			}
+		if authHeader := params.GetHttpAuthorizationHeader(); authHeader != "" {
+			config = append(config, git.ConfigPair{
+				Key:   fmt.Sprintf("http.%s.extraHeader", remoteURL),
+				Value: "Authorization: " + authHeader,
+			})
 		}
+
+		opts.CommandOptions = append(opts.CommandOptions, git.WithConfigEnv(config...))
 	} else {
 		sshCommand, cleanup, err := git.BuildSSHInvocation(ctx, req.GetSshKey(), req.GetKnownHosts())
 		if err != nil {
@@ -266,27 +170,3 @@ func (s *server) getRefspecs(refmaps []string) []string {
 	}
 	return refspecs
 }
-
-func (s *server) setRemote(ctx context.Context, repo *localrepo.Repo, name, url string) error {
-	if err := repo.Remote().Remove(ctx, name); err != nil {
-		if err != git.ErrNotFound {
-			return fmt.Errorf("remove remote: %w", err)
-		}
-	}
-
-	if err := repo.Remote().Add(ctx, name, url, git.RemoteAddOpts{}); err != nil {
-		return fmt.Errorf("add remote: %w", err)
-	}
-
-	return nil
-}
-
-func (s *server) removeRemote(ctx context.Context, repo *localrepo.Repo, name string) error {
-	if err := repo.Remote().Remove(ctx, name); err != nil {
-		if err != git.ErrNotFound {
-			return fmt.Errorf("remove remote: %w", err)
-		}
-	}
-
-	return nil
-}