Merge branch 'pks-receive-pack-write-reverse-index' into 'master'

git: Fix missing reverse indices for some Git commands

Closes #4352

See merge request gitlab-org/gitaly!4712

3 files changed, 87 insertions, 4 deletions

diff --git a/internal/git/command_description.go b/internal/git/command_description.go
index c0939a704..a56580e0b 100644
--- a/internal/git/command_description.go
+++ b/internal/git/command_description.go
@@ -85,7 +85,7 @@ var commandDescriptions = map[string]commandDescription{
 	"fetch": {
 		flags: 0,
 
-		opts: append([]GlobalOption{
+		opts: append(append([]GlobalOption{
 			// We've observed performance issues when fetching into big repositories
 			// part of an object pool. The root cause of this seems to be the
 			// connectivity check, which by default will also include references of any
@@ -119,7 +119,7 @@ var commandDescriptions = map[string]commandDescription{
 			// of time though because we never populate submodules at all. We thus
 			// disable recursion into submodules.
 			ConfigPair{Key: "fetch.recurseSubmodules", Value: "no"},
-		}, fsckConfiguration("fetch")...),
+		}, fsckConfiguration("fetch")...), packConfiguration()...),
 	},
 	"for-each-ref": {
 		flags: scNoRefUpdates,
@@ -215,7 +215,7 @@ var commandDescriptions = map[string]commandDescription{
 	},
 	"receive-pack": {
 		flags: 0,
-		opts: append(append([]GlobalOption{
+		opts: append(append(append([]GlobalOption{
 			// In case the repository belongs to an object pool, we want to prevent
 			// Git from including the pool's refs in the ref advertisement. We do
 			// this by rigging core.alternateRefsCommand to produce no output.
@@ -226,7 +226,7 @@ var commandDescriptions = map[string]commandDescription{
 			// Make git-receive-pack(1) advertise the push options
 			// capability to clients.
 			ConfigPair{Key: "receive.advertisePushOptions", Value: "true"},
-		}, hiddenReceivePackRefPrefixes()...), fsckConfiguration("receive")...),
+		}, hiddenReceivePackRefPrefixes()...), fsckConfiguration("receive")...), packConfiguration()...),
 	},
 	"remote": {
 		// While git-remote(1)'s `add` subcommand does support `--end-of-options`,
diff --git a/internal/git/localrepo/remote_test.go b/internal/git/localrepo/remote_test.go
index 93b06058e..0c622431e 100644
--- a/internal/git/localrepo/remote_test.go
+++ b/internal/git/localrepo/remote_test.go
@@ -184,6 +184,32 @@ func TestRepo_FetchRemote(t *testing.T) {
 		require.Contains(t, err.Error(), "fatal: 'doesnotexist' does not appear to be a git repository")
 		require.IsType(t, err, ErrFetchFailed{})
 	})
+
+	t.Run("generates reverse index", func(t *testing.T) {
+		repo, repoPath := initBareWithRemote(t, "origin")
+
+		// The repository has no objects yet, so there shouldn't be any packfile either.
+		packfiles, err := filepath.Glob(filepath.Join(repoPath, "objects", "pack", "*.pack"))
+		require.NoError(t, err)
+		require.Empty(t, packfiles)
+
+		// Same goes for reverse indices, naturally.
+		reverseIndices, err := filepath.Glob(filepath.Join(repoPath, "objects", "pack", "*.rev"))
+		require.NoError(t, err)
+		require.Empty(t, reverseIndices)
+
+		require.NoError(t, repo.FetchRemote(ctx, "origin", FetchOpts{}))
+
+		// After the fetch we should end up with a single packfile.
+		packfiles, err = filepath.Glob(filepath.Join(repoPath, "objects", "pack", "*.pack"))
+		require.NoError(t, err)
+		require.Len(t, packfiles, 1)
+
+		// And furthermore, that packfile should have a reverse index.
+		reverseIndices, err = filepath.Glob(filepath.Join(repoPath, "objects", "pack", "*.rev"))
+		require.NoError(t, err)
+		require.Len(t, reverseIndices, 1)
+	})
 }
 
 // captureGitSSHCommand creates a new intercepting command factory which captures the
diff --git a/internal/gitaly/service/smarthttp/receive_pack_test.go b/internal/gitaly/service/smarthttp/receive_pack_test.go
index eaa508709..21237a4a1 100644
--- a/internal/gitaly/service/smarthttp/receive_pack_test.go
+++ b/internal/gitaly/service/smarthttp/receive_pack_test.go
@@ -226,6 +226,63 @@ func TestPostReceivePack_protocolV2(t *testing.T) {
 	gittest.Exec(t, cfg, "-C", repoPath, "show", newCommitID.String())
 }
 
+func TestPostReceivePack_packfiles(t *testing.T) {
+	t.Parallel()
+
+	ctx := testhelper.Context(t)
+
+	cfg := testcfg.Build(t)
+	cfg.SocketPath = startSmartHTTPServer(t, cfg).Address()
+	testcfg.BuildGitalyHooks(t, cfg)
+
+	client, conn := newSmartHTTPClient(t, cfg.SocketPath, cfg.Auth.Token)
+	defer conn.Close()
+
+	repo, repoPath := gittest.CreateRepository(ctx, t, cfg, gittest.CreateRepositoryConfig{
+		Seed: gittest.SeedGitLabTest,
+	})
+
+	stream, err := client.PostReceivePack(ctx)
+	require.NoError(t, err)
+
+	// Verify the before-state of the repository. It should have a single packfile, ...
+	packfiles, err := filepath.Glob(filepath.Join(repoPath, "objects", "pack", "*.pack"))
+	require.NoError(t, err)
+	require.Len(t, packfiles, 1)
+
+	// ... but no reverse index. `gittest.CreateRepository()` uses `CreateRepositoryFromURL()`
+	// with a file path that doesn't use `file://` as prefix. As a result, Git uses the local
+	// transport and just copies objects over without generating a reverse index. This is thus
+	// expected as we don't want to perform a "real" clone, which would be a lot more expensive.
+	reverseIndices, err := filepath.Glob(filepath.Join(repoPath, "objects", "pack", "*.rev"))
+	require.NoError(t, err)
+	require.Empty(t, reverseIndices)
+
+	_, _, request := createPushRequest(t, cfg)
+	performPush(t, stream, &gitalypb.PostReceivePackRequest{
+		Repository:   repo,
+		GlId:         "user-123",
+		GlRepository: "project-123",
+		GitProtocol:  git.ProtocolV2,
+		// By default, Git would unpack the received packfile if it has less than
+		// 100 objects. Decrease this limit so that we indeed end up with another
+		// new packfile even though we only push a small set of objects.
+		GitConfigOptions: []string{
+			"receive.unpackLimit=0",
+		},
+	}, request)
+
+	// We now should have two packfiles, ...
+	packfiles, err = filepath.Glob(filepath.Join(repoPath, "objects", "pack", "*.pack"))
+	require.NoError(t, err)
+	require.Len(t, packfiles, 2)
+
+	// ... and one reverse index for the newly added packfile.
+	reverseIndices, err = filepath.Glob(filepath.Join(repoPath, "objects", "pack", "*.rev"))
+	require.NoError(t, err)
+	require.Len(t, reverseIndices, 1)
+}
+
 func TestPostReceivePack_rejectViaGitConfigOptions(t *testing.T) {
 	t.Parallel()