Merge branch 'jt-bump-git-version' into 'master'

git: Bump minimum version of Git to 2.41

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

Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Reviewed-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Reviewed-by: Justin Tobler <jtobler@gitlab.com>
Co-authored-by: Justin Tobler <jtobler@gitlab.com>

10 files changed, 45 insertions, 229 deletions

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 86aaeed98..e5fd79e81 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -186,7 +186,7 @@ build:
     matrix:
       - GO_VERSION: [ "1.19", "1.20" ]
         TEST_BOOT_ARGS: "--bundled-git"
-      - GIT_VERSION: "v2.40.0"
+      - GIT_VERSION: "v2.41.0"
 
 build:binaries:
   needs: []
@@ -223,7 +223,7 @@ test:
       # We also verify that things work as expected with a non-bundled Git
       # version matching our minimum required Git version.
       - TEST_TARGET: test
-        GIT_VERSION: "v2.40.0"
+        GIT_VERSION: "v2.41.0"
       # Execute tests with our minimum required Postgres version, as well. If
       # the minimum version changes, please change this to the new minimum
       # version. Furthermore, please make sure to update the minimum required
diff --git a/README.md b/README.md
index 7e0c77962..649b70693 100644
--- a/README.md
+++ b/README.md
@@ -58,7 +58,7 @@ Most users won't install Gitaly on its own. It is already included in
 Gitaly requires Go 1.19 or Go 1.20. Run `make` to compile the executables
 required by Gitaly.
 
-Gitaly uses `git`. Versions `2.40.0` and newer are supported.
+Gitaly uses `git`. Versions `2.41.0` and newer are supported.
 
 ## Configuration
 
diff --git a/internal/git/housekeeping/objects_test.go b/internal/git/housekeeping/objects_test.go
index 4f800b256..37ed2fc89 100644
--- a/internal/git/housekeeping/objects_test.go
+++ b/internal/git/housekeeping/objects_test.go
@@ -28,9 +28,6 @@ func TestRepackObjects(t *testing.T) {
 	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t)
 
-	gitVersion, err := gittest.NewCommandFactory(t, cfg).GitVersion(ctx)
-	require.NoError(t, err)
-
 	// repack is a custom helper function that repacks while explicitly disabling the update of
 	// server info. This is done so that we assert that the actual repacking logic doesn't write
 	// the server info.
@@ -262,16 +259,8 @@ func TestRepackObjects(t *testing.T) {
 				hasBitmap:    true,
 			},
 			stateAfterRepack: objectsState{
-				packfiles: 2,
-				// Git v2.38.0 does not yet remove redundant pack-based bitmaps.
-				// This is getting fixed via 55d902cd61 (builtin/repack.c: remove
-				// redundant pack-based bitmaps, 2022-10-17), which is part of Git
-				// v2.39.0 and newer.
-				//
-				// Local tests don't show that this is a problem. Most importantly,
-				// Git does not seem to warn about these bitmaps. So let's just
-				// ignore them for now.
-				hasBitmap:               !gitVersion.MidxDeletesRedundantBitmaps(),
+				packfiles:               2,
+				hasBitmap:               false,
 				hasMultiPackIndex:       true,
 				hasMultiPackIndexBitmap: true,
 			},
diff --git a/internal/git/housekeeping/optimization_strategy.go b/internal/git/housekeeping/optimization_strategy.go
index 37775bca3..7ac3563f7 100644
--- a/internal/git/housekeeping/optimization_strategy.go
+++ b/internal/git/housekeeping/optimization_strategy.go
@@ -62,22 +62,7 @@ func (s HeuristicalOptimizationStrategy) ShouldRepackObjects(ctx context.Context
 		return false, RepackObjectsConfig{}
 	}
 
-	// There is a bug in Git that causes geometric repacking to fail in some circumstances when
-	// the repository is connected to an object pool. While we're upstreaming the fix via
-	// https://gitlab.com/gitlab-org/git/-/issues/152 we thus disable geometric repacks in any
-	// repository that has alternates.
-	//
-	// While this is kind of annoying, it ultimately shouldn't be too bad in most contexts as
-	// the majority of objects of a repository with object pool should be in the object pool
-	// anyway. We should eventually remove this condition though once the fix has landed.
-	//
-	// We have upstreamed fixes for this that are about to arrive in Git v2.41. Furthermore, we
-	// have backported them into Git v2.40.0.gl1. So if we detect that the current Git version
-	// does indeed support geometric repacking then we can enable this even when the repository
-	// is part of an object pool.
-	canUseGeometricRepacking := len(s.info.Alternates.ObjectDirectories) == 0 || s.gitVersion.GeometricRepackingSupportsAlternates()
-
-	if canUseGeometricRepacking && featureflag.GeometricRepacking.IsEnabled(ctx) {
+	if featureflag.GeometricRepacking.IsEnabled(ctx) {
 		nonCruftPackfilesCount := s.info.Packfiles.Count - s.info.Packfiles.CruftCount
 		timeSinceLastFullRepack := time.Since(s.info.Packfiles.LastFullRepack)
 
diff --git a/internal/git/housekeeping/optimization_strategy_test.go b/internal/git/housekeeping/optimization_strategy_test.go
index 165f38ff2..7075300f7 100644
--- a/internal/git/housekeeping/optimization_strategy_test.go
+++ b/internal/git/housekeeping/optimization_strategy_test.go
@@ -76,7 +76,10 @@ func testHeuristicalOptimizationStrategyShouldRepackObjects(t *testing.T, ctx co
 			// exist in pooled repositories.
 			expectedNeeded: true,
 			expectedConfig: RepackObjectsConfig{
-				Strategy:            RepackObjectsStrategyIncremental,
+				Strategy: geometricOrIncremental(ctx,
+					RepackObjectsStrategyGeometric,
+					RepackObjectsStrategyIncremental,
+				),
 				WriteMultiPackIndex: true,
 			},
 		},
@@ -383,31 +386,6 @@ func testHeuristicalOptimizationStrategyShouldRepackObjects(t *testing.T, ctx co
 			),
 		},
 		{
-			desc: "no geometric repack in object pool member with old Git version",
-			strategy: HeuristicalOptimizationStrategy{
-				gitVersion: git.NewVersion(2, 39, 0, 0),
-				info: stats.RepositoryInfo{
-					Packfiles: stats.PackfilesInfo{
-						Count:          9,
-						LastFullRepack: time.Now(),
-						MultiPackIndex: stats.MultiPackIndexInfo{
-							Exists:        true,
-							PackfileCount: 1,
-						},
-					},
-					Alternates: stats.AlternatesInfo{
-						ObjectDirectories: []string{"object-pool"},
-					},
-				},
-			},
-			expectedNeeded: true,
-			expectedConfig: RepackObjectsConfig{
-				Strategy:            RepackObjectsStrategyFullWithCruft,
-				WriteBitmap:         false,
-				WriteMultiPackIndex: true,
-			},
-		},
-		{
 			desc: "geometric repack in object pool member with recent Git version",
 			strategy: HeuristicalOptimizationStrategy{
 				gitVersion: git.NewVersion(2, 40, 0, 1),
diff --git a/internal/git/housekeeping/optimize_repository_test.go b/internal/git/housekeeping/optimize_repository_test.go
index dad4830fa..84b878f29 100644
--- a/internal/git/housekeeping/optimize_repository_test.go
+++ b/internal/git/housekeeping/optimize_repository_test.go
@@ -457,9 +457,6 @@ func testOptimizeRepository(t *testing.T, ctx context.Context) {
 	cfg := testcfg.Build(t)
 	txManager := transaction.NewManager(cfg, backchannel.NewRegistry())
 
-	gitVersion, err := gittest.NewCommandFactory(t, cfg).GitVersion(ctx)
-	require.NoError(t, err)
-
 	earlierDate := time.Date(2022, 12, 1, 0, 0, 0, 0, time.Local)
 	laterDate := time.Date(2022, 12, 1, 12, 0, 0, 0, time.Local)
 
@@ -489,11 +486,6 @@ func testOptimizeRepository(t *testing.T, ctx context.Context) {
 
 	geometricOrIncrementalMetric := geometricOrIncremental(ctx, "packed_objects_geometric", "packed_objects_incremental")
 
-	geometricIfSupported := geometricOrIncrementalMetric
-	if !gitVersion.GeometricRepackingSupportsAlternates() {
-		geometricIfSupported = "packed_objects_incremental"
-	}
-
 	type metric struct {
 		name, status string
 		count        int
@@ -836,7 +828,7 @@ func testOptimizeRepository(t *testing.T, ctx context.Context) {
 				return setupData{
 					repo: localrepo.NewTestRepo(t, cfg, repo),
 					expectedMetrics: []metric{
-						{name: geometricIfSupported, status: "success", count: 1},
+						{name: geometricOrIncrementalMetric, status: "success", count: 1},
 						{name: "written_commit_graph_full", status: "success", count: 1},
 						{name: "written_multi_pack_index", status: "success", count: 1},
 						{name: "total", status: "success", count: 1},
@@ -892,7 +884,7 @@ func testOptimizeRepository(t *testing.T, ctx context.Context) {
 				return setupData{
 					repo: localrepo.NewTestRepo(t, cfg, repo),
 					expectedMetrics: []metric{
-						{name: geometricIfSupported, status: "success", count: 1},
+						{name: geometricOrIncrementalMetric, status: "success", count: 1},
 						{name: "written_commit_graph_full", status: "success", count: 1},
 						{name: "written_multi_pack_index", status: "success", count: 1},
 						{name: "total", status: "success", count: 1},
@@ -928,12 +920,11 @@ func testOptimizeRepository(t *testing.T, ctx context.Context) {
 						{name: "total", status: "success", count: 1},
 					},
 					expectedMetricsForPool: []metric{
-						{name: func() string {
-							if gitVersion.GeometricRepackingSupportsAlternates() {
-								return geometricOrIncremental(ctx, "packed_objects_full_with_unreachable", "packed_objects_full_with_loose_unreachable")
-							}
-							return "packed_objects_full_with_loose_unreachable"
-						}(), status: "success", count: 1},
+						{
+							name:   geometricOrIncremental(ctx, "packed_objects_full_with_unreachable", "packed_objects_full_with_loose_unreachable"),
+							status: "success",
+							count:  1,
+						},
 						{name: "written_multi_pack_index", status: "success", count: 1},
 						{name: "total", status: "success", count: 1},
 					},
@@ -1015,7 +1006,7 @@ func testOptimizeRepository(t *testing.T, ctx context.Context) {
 				return setupData{
 					repo: localrepo.NewTestRepo(t, cfg, repo),
 					expectedMetrics: []metric{
-						{name: geometricIfSupported, status: "success", count: 1},
+						{name: geometricOrIncrementalMetric, status: "success", count: 1},
 						{name: "written_commit_graph_full", status: "success", count: 1},
 						{name: "written_multi_pack_index", status: "success", count: 1},
 						{name: "total", status: "success", count: 1},
diff --git a/internal/git/localrepo/tree_test.go b/internal/git/localrepo/tree_test.go
index a907a22e7..7a4f3fcdd 100644
--- a/internal/git/localrepo/tree_test.go
+++ b/internal/git/localrepo/tree_test.go
@@ -22,9 +22,6 @@ func TestTreeEntry_Write(t *testing.T) {
 	cfg := testcfg.Build(t)
 	ctx := testhelper.Context(t)
 
-	gitVersion, err := gittest.NewCommandFactory(t, cfg).GitVersion(ctx)
-	require.NoError(t, err)
-
 	repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
 		SkipCreationViaService: true,
 	})
@@ -201,14 +198,12 @@ func TestTreeEntry_Write(t *testing.T) {
 			err := tree.Write(ctx, repo)
 			oid := tree.OID
 			if tc.expectedErrString != "" {
-				if gitVersion.HashObjectFsck() {
-					switch e := err.(type) {
-					case structerr.Error:
-						stderr := e.Metadata()["stderr"].(string)
-						strings.Contains(stderr, tc.expectedErrString)
-					default:
-						strings.Contains(err.Error(), tc.expectedErrString)
-					}
+				switch e := err.(type) {
+				case structerr.Error:
+					stderr := e.Metadata()["stderr"].(string)
+					strings.Contains(stderr, tc.expectedErrString)
+				default:
+					strings.Contains(err.Error(), tc.expectedErrString)
 				}
 				return
 			}
diff --git a/internal/git/version.go b/internal/git/version.go
index 4f7908fca..831ac3029 100644
--- a/internal/git/version.go
+++ b/internal/git/version.go
@@ -14,9 +14,9 @@ import (
 // - https://docs.gitlab.com/ee/install/installation.html#software-requirements
 // - https://docs.gitlab.com/ee/update/ (see e.g. https://docs.gitlab.com/ee/update/#1440)
 var minimumVersion = Version{
-	versionString: "2.40.0",
+	versionString: "2.41.0",
 	major:         2,
-	minor:         40,
+	minor:         41,
 	patch:         0,
 	rc:            false,
 
@@ -75,49 +75,6 @@ func (v Version) IsSupported() bool {
 	return !v.LessThan(minimumVersion)
 }
 
-// HashObjectFsck detects whether or not the given Git version will do fsck
-// checks when git-hash-object writes objects.
-func (v Version) HashObjectFsck() bool {
-	return !v.LessThan(Version{
-		major: 2, minor: 40, patch: 0,
-	})
-}
-
-// PatchIDRespectsBinaries detects whether the given Git version correctly handles binary diffs when
-// computing a patch ID. Previous to Git v2.39.0, git-patch-id(1) just completely ignored any binary
-// diffs and thus would consider two diffs the same even if a binary changed.
-func (v Version) PatchIDRespectsBinaries() bool {
-	return !v.LessThan(Version{
-		major: 2, minor: 39, patch: 0,
-	})
-}
-
-// MidxDeletesRedundantBitmaps detects whether the given Git version deletes redundant pack-based
-// bitmaps when writing multi-pack-indices. This feature has been added via 55d902cd61
-// (builtin/repack.c: remove redundant pack-based bitmaps, 2022-10-17), which is part of Git
-// v2.39.0 and newer.
-func (v Version) MidxDeletesRedundantBitmaps() bool {
-	return !v.LessThan(Version{
-		major: 2, minor: 39, patch: 0,
-	})
-}
-
-// GeometricRepackingSupportsAlternates detects whether the given Git version knows to perform
-// geometric repacking in repositories which are connected to an alternate object database. This
-// used to not work due to various different bugs which have been fixed via de56e80363 (Merge branch
-// 'ps/fix-geom-repack-with-alternates' into next, 2023-04-18).
-//
-// The patches will be part of Git v2.41.0 and have been backported to Git v2.40.0.gl1.
-func (v Version) GeometricRepackingSupportsAlternates() bool {
-	if v.major == 2 && v.minor == 40 && v.gl > 0 {
-		return true
-	}
-
-	return !v.LessThan(Version{
-		major: 2, minor: 41,
-	})
-}
-
 // CatfileSupportsNulTerminatedOutput detects whether git-cat-file(1) knows the `-Z` switch, which causes it to
 // NUL-terminate both stdin and stdout. This new switch has been introduced upstream via a9ea4c23dc (Merge branch
 // 'ps/cat-file-null-output', 2023-06-22).
diff --git a/internal/git/version_test.go b/internal/git/version_test.go
index feb29a1c6..83af74e3c 100644
--- a/internal/git/version_test.go
+++ b/internal/git/version_test.go
@@ -111,79 +111,17 @@ func TestVersion_IsSupported(t *testing.T) {
 		{"2.39.0", false},
 		{"2.39.0.gl0", false},
 		{"2.39.0.gl3", false},
-		{"2.40.0", true},
-		{"2.40.0.gl1", true},
-		{"2.40.1", true},
-		{"3.0.0", true},
-		{"3.0.0.gl5", true},
-	} {
-		t.Run(tc.version, func(t *testing.T) {
-			version, err := parseVersion(tc.version)
-			require.NoError(t, err)
-			require.Equal(t, tc.expect, version.IsSupported())
-		})
-	}
-}
-
-func TestVersion_PatchIDRespectsBinaries(t *testing.T) {
-	for _, tc := range []struct {
-		version string
-		expect  bool
-	}{
-		{"1.0.0", false},
-		{"2.38.2", false},
-		{"2.39.0", 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.PatchIDRespectsBinaries())
-		})
-	}
-}
-
-func TestVersion_MidxDeletesRedundantBitmaps(t *testing.T) {
-	for _, tc := range []struct {
-		version string
-		expect  bool
-	}{
-		{"1.0.0", false},
-		{"2.38.2", false},
-		{"2.39.0", 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.MidxDeletesRedundantBitmaps())
-		})
-	}
-}
-
-func TestVersion_GeometricRepackingSupportsAlternates(t *testing.T) {
-	t.Parallel()
-
-	for _, tc := range []struct {
-		version string
-		expect  bool
-	}{
-		{"1.0.0", false},
-		{"2.39.2", false},
 		{"2.40.0", false},
+		{"2.40.0.gl1", false},
 		{"2.40.1", false},
-		{"2.40.0.gl1", true},
-		{"2.40.0.gl2", true},
-		{"2.40.1.gl1", true},
-		{"2.40.1.gl2", true},
 		{"2.41.0", true},
 		{"3.0.0", true},
+		{"3.0.0.gl5", true},
 	} {
 		t.Run(tc.version, func(t *testing.T) {
 			version, err := parseVersion(tc.version)
 			require.NoError(t, err)
-			require.Equal(t, tc.expect,
-				version.GeometricRepackingSupportsAlternates())
+			require.Equal(t, tc.expect, version.IsSupported())
 		})
 	}
 }
diff --git a/internal/gitaly/service/diff/patch_id_test.go b/internal/gitaly/service/diff/patch_id_test.go
index 00b89a1dd..75ec74a59 100644
--- a/internal/gitaly/service/diff/patch_id_test.go
+++ b/internal/gitaly/service/diff/patch_id_test.go
@@ -19,9 +19,6 @@ func TestGetPatchID(t *testing.T) {
 	ctx := testhelper.Context(t)
 	cfg, client := setupDiffServiceWithoutRepo(t)
 
-	gitVersion, err := gittest.NewCommandFactory(t, cfg).GitVersion(ctx)
-	require.NoError(t, err)
-
 	type setupData struct {
 		request          *gitalypb.GetPatchIDRequest
 		expectedResponse *gitalypb.GetPatchIDResponse
@@ -155,19 +152,15 @@ func TestGetPatchID(t *testing.T) {
 							// This was fixed in Git v2.39.0 so that "index" lines will
 							// now be hashed to correctly account for binary changes. As
 							// a result, the patch ID has changed.
-							if gitVersion.PatchIDRespectsBinaries() {
-								switch gittest.DefaultObjectHash.Format {
-								case "sha1":
-									return "13e4e9b9cd44ec511bac24fdbdeab9b74ba3000b"
-								case "sha256":
-									return "32f6beb9a210ac89a3e15e44dcd174c87c904e9d"
-								default:
-									require.FailNow(t, "unsupported object hash")
-									return ""
-								}
+							switch gittest.DefaultObjectHash.Format {
+							case "sha1":
+								return "13e4e9b9cd44ec511bac24fdbdeab9b74ba3000b"
+							case "sha256":
+								return "32f6beb9a210ac89a3e15e44dcd174c87c904e9d"
+							default:
+								require.FailNow(t, "unsupported object hash")
+								return ""
 							}
-
-							return "715883c1b90a5b4450072e22fefec769ad346266"
 						}(),
 					},
 				}
@@ -200,25 +193,15 @@ func TestGetPatchID(t *testing.T) {
 					},
 					expectedResponse: &gitalypb.GetPatchIDResponse{
 						PatchId: func() string {
-							if gitVersion.PatchIDRespectsBinaries() {
-								// When respecting binary diffs we indeed have a
-								// different patch ID compared to the preceding
-								// testcase.
-								switch gittest.DefaultObjectHash.Format {
-								case "sha1":
-									return "f678855867b112ac2c5466260b3b3a5e75fca875"
-								case "sha256":
-									return "10443cf318b577ea41526825ba034aaaedfeaa4b"
-								default:
-									require.FailNow(t, "unsupported object hash")
-									return ""
-								}
+							switch gittest.DefaultObjectHash.Format {
+							case "sha1":
+								return "f678855867b112ac2c5466260b3b3a5e75fca875"
+							case "sha256":
+								return "10443cf318b577ea41526825ba034aaaedfeaa4b"
+							default:
+								require.FailNow(t, "unsupported object hash")
+								return ""
 							}
-
-							// But when git-patch-id(1) is not paying respect to binary
-							// diffs we incorrectly return the same patch ID. This is
-							// nothing we can easily fix though.
-							return "715883c1b90a5b4450072e22fefec769ad346266"
 						}(),
 					},
 				}