Merge branch 'pks-objectpool-sha256' into 'master'

objectpool: Compatibility with SHA256 object format

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

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

7 files changed, 37 insertions, 20 deletions

diff --git a/internal/git/objectpool/create.go b/internal/git/objectpool/create.go
index eb96458d9..f4e2e2c8f 100644
--- a/internal/git/objectpool/create.go
+++ b/internal/git/objectpool/create.go
@@ -50,6 +50,11 @@ func Create(
 		return nil, fmt.Errorf("getting source repository path: %w", err)
 	}
 
+	objectHash, err := sourceRepo.ObjectHash(ctx)
+	if err != nil {
+		return nil, fmt.Errorf("detecting source repo object hash: %w", err)
+	}
+
 	var stderr bytes.Buffer
 	cmd, err := gitCmdFactory.NewWithoutRepo(ctx,
 		git.Command{
@@ -63,6 +68,11 @@ func Create(
 		},
 		git.WithRefTxHook(sourceRepo),
 		git.WithStderr(&stderr),
+		// When cloning an empty repository then Git isn't capable to figure out the correct
+		// object hash that the new repository needs to use and just uses the default object
+		// format. To work around this shortcoming we thus set the default object hash to
+		// match the source repository's object hash.
+		git.WithEnv("GIT_DEFAULT_HASH="+objectHash.Format),
 	)
 	if err != nil {
 		return nil, fmt.Errorf("spawning clone: %w", err)
diff --git a/internal/git/objectpool/create_test.go b/internal/git/objectpool/create_test.go
index 0241b360b..14350a136 100644
--- a/internal/git/objectpool/create_test.go
+++ b/internal/git/objectpool/create_test.go
@@ -1,5 +1,3 @@
-//go:build !gitaly_test_sha256
-
 package objectpool
 
 import (
diff --git a/internal/git/objectpool/fetch.go b/internal/git/objectpool/fetch.go
index 8cfece74a..c1334ddae 100644
--- a/internal/git/objectpool/fetch.go
+++ b/internal/git/objectpool/fetch.go
@@ -158,6 +158,11 @@ func (o *ObjectPool) pruneReferences(ctx context.Context, origin *localrepo.Repo
 		return fmt.Errorf("start reference transaction: %w", err)
 	}
 
+	objectHash, err := o.ObjectHash(ctx)
+	if err != nil {
+		return fmt.Errorf("detecting object hash: %w", err)
+	}
+
 	// We need to manually compute a vote because all deletions we queue up here are
 	// force-deletions. We are forced to filter out force-deletions because these may also
 	// happen when evicting references from the packed-refs file.
@@ -182,7 +187,7 @@ func (o *ObjectPool) pruneReferences(ctx context.Context, origin *localrepo.Repo
 			// of every reference here.
 			deletedRef := "refs/remotes/" + string(bytes.TrimPrefix(line, []byte(" * [would prune] ")))
 
-			if _, err := io.Copy(voteHash, strings.NewReader(fmt.Sprintf("%[1]s %[1]s %s\n", git.ObjectHashSHA1.ZeroOID, deletedRef))); err != nil {
+			if _, err := io.Copy(voteHash, strings.NewReader(fmt.Sprintf("%[1]s %[1]s %s\n", objectHash.ZeroOID, deletedRef))); err != nil {
 				return fmt.Errorf("hashing reference deletion: %w", err)
 			}
 
@@ -264,6 +269,11 @@ func (o *ObjectPool) rescueDanglingObjects(ctx context.Context) (returnedErr err
 		return fmt.Errorf("start reference transaction: %w", err)
 	}
 
+	objectHash, err := o.ObjectHash(ctx)
+	if err != nil {
+		return fmt.Errorf("detecting object hash: %w", err)
+	}
+
 	scanner := bufio.NewScanner(fsck)
 	for scanner.Scan() {
 		split := strings.SplitN(scanner.Text(), " ", 3)
@@ -275,7 +285,7 @@ func (o *ObjectPool) rescueDanglingObjects(ctx context.Context) (returnedErr err
 			continue
 		}
 
-		danglingObjectID, err := git.ObjectHashSHA1.FromHex(split[2])
+		danglingObjectID, err := objectHash.FromHex(split[2])
 		if err != nil {
 			return fmt.Errorf("parsing object ID %q: %w", split[2], err)
 		}
diff --git a/internal/git/objectpool/fetch_test.go b/internal/git/objectpool/fetch_test.go
index 8ca529aa3..9df238646 100644
--- a/internal/git/objectpool/fetch_test.go
+++ b/internal/git/objectpool/fetch_test.go
@@ -1,5 +1,3 @@
-//go:build !gitaly_test_sha256
-
 package objectpool
 
 import (
@@ -67,24 +65,24 @@ func testFetchFromOriginDangling(t *testing.T, ctx context.Context) {
 
 	// git-fsck(1) should report the newly created unreachable objects as dangling.
 	fsckBefore := gittest.Exec(t, cfg, "-C", poolPath, "fsck", "--connectivity-only", "--dangling")
-	require.Equal(t, strings.Join([]string{
+	require.ElementsMatch(t, []string{
 		fmt.Sprintf("dangling blob %s", unreachableBlob),
 		fmt.Sprintf("dangling tag %s", unreachableTag),
 		fmt.Sprintf("dangling commit %s", unreachableCommit),
 		fmt.Sprintf("dangling tree %s", unreachableTree),
-	}, "\n"), text.ChompBytes(fsckBefore))
+	}, strings.Split(text.ChompBytes(fsckBefore), "\n"))
 
 	// We expect this second run to convert the dangling objects into non-dangling objects.
 	require.NoError(t, pool.FetchFromOrigin(ctx, repo))
 
 	// Each of the dangling objects should have gotten a new dangling reference.
 	danglingRefs := gittest.Exec(t, cfg, "-C", poolPath, "for-each-ref", "--format=%(refname) %(objectname)", "refs/dangling/")
-	require.Equal(t, strings.Join([]string{
+	require.ElementsMatch(t, []string{
 		fmt.Sprintf("refs/dangling/%[1]s %[1]s", unreachableBlob),
 		fmt.Sprintf("refs/dangling/%[1]s %[1]s", unreachableTree),
 		fmt.Sprintf("refs/dangling/%[1]s %[1]s", unreachableTag),
 		fmt.Sprintf("refs/dangling/%[1]s %[1]s", unreachableCommit),
-	}, "\n"), text.ChompBytes(danglingRefs))
+	}, strings.Split(text.ChompBytes(danglingRefs), "\n"))
 	// And git-fsck(1) shouldn't report the objects as dangling anymore.
 	require.Empty(t, gittest.Exec(t, cfg, "-C", poolPath, "fsck", "--connectivity-only", "--dangling"))
 }
@@ -103,8 +101,8 @@ func TestFetchFromOrigin_fsck(t *testing.T) {
 	// allows us to create these trees just fine, but git-fsck(1) complains.
 	gittest.WriteCommit(t, cfg, repoPath,
 		gittest.WithTreeEntries(
-			gittest.TreeEntry{OID: "4b825dc642cb6eb9a060e54bf8d69288fbee4904", Path: "dup", Mode: "040000"},
-			gittest.TreeEntry{OID: "4b825dc642cb6eb9a060e54bf8d69288fbee4904", Path: "dup", Mode: "040000"},
+			gittest.TreeEntry{OID: gittest.DefaultObjectHash.EmptyTreeOID, Path: "dup", Mode: "040000"},
+			gittest.TreeEntry{OID: gittest.DefaultObjectHash.EmptyTreeOID, Path: "dup", Mode: "040000"},
 		),
 		gittest.WithBranch("branch"),
 	)
@@ -334,7 +332,7 @@ func TestObjectPool_logStats(t *testing.T) {
 					IsObjectPool: true,
 					LooseObjects: stats.LooseObjectsInfo{
 						Count: 2,
-						Size:  142,
+						Size:  hashDependentSize(142, 158),
 					},
 					References: stats.ReferencesInfo{
 						LooseReferencesCount: 1,
@@ -358,7 +356,7 @@ func TestObjectPool_logStats(t *testing.T) {
 					IsObjectPool: true,
 					LooseObjects: stats.LooseObjectsInfo{
 						Count: 2,
-						Size:  142,
+						Size:  hashDependentSize(142, 158),
 					},
 					References: stats.ReferencesInfo{
 						LooseReferencesCount: 1,
diff --git a/internal/git/objectpool/link_test.go b/internal/git/objectpool/link_test.go
index c44f73cbe..2c691164c 100644
--- a/internal/git/objectpool/link_test.go
+++ b/internal/git/objectpool/link_test.go
@@ -1,5 +1,3 @@
-//go:build !gitaly_test_sha256
-
 package objectpool
 
 import (
diff --git a/internal/git/objectpool/pool_test.go b/internal/git/objectpool/pool_test.go
index 0f2c9a734..550199f30 100644
--- a/internal/git/objectpool/pool_test.go
+++ b/internal/git/objectpool/pool_test.go
@@ -1,5 +1,3 @@
-//go:build !gitaly_test_sha256
-
 package objectpool
 
 import (
diff --git a/internal/git/objectpool/testhelper_test.go b/internal/git/objectpool/testhelper_test.go
index c33b72cfe..20e9c7e61 100644
--- a/internal/git/objectpool/testhelper_test.go
+++ b/internal/git/objectpool/testhelper_test.go
@@ -1,5 +1,3 @@
-//go:build !gitaly_test_sha256
-
 package objectpool
 
 import (
@@ -58,3 +56,10 @@ func setupObjectPool(t *testing.T, ctx context.Context) (config.Cfg, *ObjectPool
 
 	return cfg, pool, repo
 }
+
+func hashDependentSize(sha1Size, sha256Size uint64) uint64 {
+	if gittest.ObjectHashIsSHA256() {
+		return sha256Size
+	}
+	return sha1Size
+}