Merge branch 'pks-git-stop-writing-fetch-head' into 'master'

fetch: Stop writing FETCH_HEAD

See merge request gitlab-org/gitaly!4305

6 files changed, 43 insertions, 31 deletions

diff --git a/internal/git/localrepo/remote.go b/internal/git/localrepo/remote.go
index 69ffb29d8..1e0ff9b3e 100644
--- a/internal/git/localrepo/remote.go
+++ b/internal/git/localrepo/remote.go
@@ -153,7 +153,10 @@ func (repo *Repo) FetchInternal(
 		git.SubCmd{
 			Name:  "fetch",
 			Flags: opts.buildFlags(),
-			Args:  append([]string{git.InternalGitalyURL}, refspecs...),
+			Args: append(
+				[]string{git.InternalGitalyURL},
+				refspecs...,
+			),
 		},
 		commandOptions...,
 	); err != nil {
@@ -164,7 +167,11 @@ func (repo *Repo) FetchInternal(
 }
 
 func (opts FetchOpts) buildFlags() []git.Option {
-	flags := []git.Option{}
+	flags := []git.Option{
+		// We don't need FETCH_HEAD, and it can potentially be hundreds of megabytes when
+		// doing a mirror-sync of repos with huge numbers of references.
+		git.Flag{Name: "--no-write-fetch-head"},
+	}
 
 	if !opts.Verbose {
 		flags = append(flags, git.Flag{Name: "--quiet"})
diff --git a/internal/git/localrepo/remote_extra_test.go b/internal/git/localrepo/remote_extra_test.go
index 1b473c06e..1603a896d 100644
--- a/internal/git/localrepo/remote_extra_test.go
+++ b/internal/git/localrepo/remote_extra_test.go
@@ -2,7 +2,6 @@ package localrepo_test
 
 import (
 	"bytes"
-	"fmt"
 	"path/filepath"
 	"testing"
 
@@ -13,7 +12,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service/hook"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service/ssh"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testserver"
@@ -66,15 +64,13 @@ func TestRepo_FetchInternal(t *testing.T) {
 			localrepo.FetchOpts{},
 		))
 
-		fetchHead := testhelper.MustReadFile(t, filepath.Join(repoPath, "FETCH_HEAD"))
-		expectedFetchHead := fmt.Sprintf("%s\t\tbranch 'master' of ssh://gitaly/internal\n", remoteOID.String())
-		expectedFetchHead += fmt.Sprintf("%s\tnot-for-merge\ttag 'v1.0.0' of ssh://gitaly/internal\n", tagV100OID.String())
-		expectedFetchHead += fmt.Sprintf("%s\tnot-for-merge\ttag 'v1.1.0' of ssh://gitaly/internal", tagV110OID.String())
-		require.Equal(t, expectedFetchHead, text.ChompBytes(fetchHead))
-
-		oid, err := repo.ResolveRevision(ctx, git.Revision("refs/heads/master"))
-		require.NoError(t, err, "the object from remote should exists in local after fetch done")
-		require.Equal(t, remoteOID, oid)
+		refs, err := repo.GetReferences(ctx)
+		require.NoError(t, err)
+		require.Equal(t, []git.Reference{
+			{Name: "refs/heads/master", Target: remoteOID.String()},
+			{Name: "refs/tags/v1.0.0", Target: tagV100OID.String()},
+			{Name: "refs/tags/v1.1.0", Target: tagV110OID.String()},
+		}, refs)
 
 		// Even if the gitconfig says we should write a commit graph, Gitaly should refuse
 		// to do so.
@@ -83,12 +79,14 @@ func TestRepo_FetchInternal(t *testing.T) {
 
 		// Assert that we're using the expected Git protocol version, which is protocol v2.
 		require.Equal(t, "GIT_PROTOCOL=version=2\n", readGitProtocol())
+
+		require.NoFileExists(t, filepath.Join(repoPath, "FETCH_HEAD"))
 	})
 
 	t.Run("refspec without tags", func(t *testing.T) {
 		ctx := testhelper.MergeIncomingMetadata(ctx, testcfg.GitalyServersMetadataFromCfg(t, cfg))
 
-		repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0])
+		repoProto, _ := gittest.InitRepo(t, cfg, cfg.Storages[0])
 		repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
 		require.NoError(t, repo.FetchInternal(
@@ -96,13 +94,11 @@ func TestRepo_FetchInternal(t *testing.T) {
 			localrepo.FetchOpts{Tags: localrepo.FetchOptsTagsNone},
 		))
 
-		fetchHead := testhelper.MustReadFile(t, filepath.Join(repoPath, "FETCH_HEAD"))
-		expectedFetchHead := fmt.Sprintf("%s\t\tbranch 'master' of ssh://gitaly/internal", remoteOID.String())
-		require.Equal(t, expectedFetchHead, text.ChompBytes(fetchHead))
-
-		oid, err := repo.ResolveRevision(ctx, git.Revision("refs/heads/master"))
-		require.NoError(t, err, "the object from remote should exists in local after fetch done")
-		require.Equal(t, remoteOID, oid)
+		refs, err := repo.GetReferences(ctx)
+		require.NoError(t, err)
+		require.Equal(t, []git.Reference{
+			{Name: "refs/heads/master", Target: remoteOID.String()},
+		}, refs)
 	})
 
 	t.Run("object ID", func(t *testing.T) {
@@ -149,7 +145,7 @@ func TestRepo_FetchInternal(t *testing.T) {
 			localrepo.FetchOpts{Stderr: &stderr, Env: []string{"GIT_TRACE=1"}},
 		)
 		require.NoError(t, err)
-		require.Contains(t, stderr.String(), "trace: built-in: git fetch --quiet --atomic --end-of-options")
+		require.Contains(t, stderr.String(), "trace: built-in: git fetch --no-write-fetch-head --quiet --atomic --end-of-options")
 	})
 
 	t.Run("with disabled transactions", func(t *testing.T) {
@@ -164,7 +160,7 @@ func TestRepo_FetchInternal(t *testing.T) {
 			localrepo.FetchOpts{Stderr: &stderr, Env: []string{"GIT_TRACE=1"}, DisableTransactions: true},
 		)
 		require.NoError(t, err)
-		require.Contains(t, stderr.String(), "trace: built-in: git fetch --quiet --end-of-options")
+		require.Contains(t, stderr.String(), "trace: built-in: git fetch --no-write-fetch-head --quiet --end-of-options")
 	})
 
 	t.Run("invalid remote repo", func(t *testing.T) {
diff --git a/internal/git/localrepo/remote_test.go b/internal/git/localrepo/remote_test.go
index 0c5ef9c75..ba9259104 100644
--- a/internal/git/localrepo/remote_test.go
+++ b/internal/git/localrepo/remote_test.go
@@ -63,22 +63,23 @@ func TestRepo_FetchRemote(t *testing.T) {
 	})
 
 	t.Run("ok", func(t *testing.T) {
-		repo, testRepoPath := initBareWithRemote(t, "origin")
+		repo, repoPath := initBareWithRemote(t, "origin")
 
 		var stderr bytes.Buffer
 		require.NoError(t, repo.FetchRemote(ctx, "origin", FetchOpts{Stderr: &stderr}))
 
 		require.Empty(t, stderr.String(), "it should not produce output as it is called with --quite flag by default")
 
-		fetchHeadData := testhelper.MustReadFile(t, filepath.Join(testRepoPath, "FETCH_HEAD"))
-
-		fetchHead := string(fetchHeadData)
-		require.Contains(t, fetchHead, "e56497bb5f03a90a51293fc6d516788730953899	not-for-merge	branch ''test''")
-		require.Contains(t, fetchHead, "8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b	not-for-merge	tag 'v1.1.0'")
+		refs, err := repo.GetReferences(ctx)
+		require.NoError(t, err)
+		require.Contains(t, refs, git.Reference{Name: "refs/remotes/origin/'test'", Target: "e56497bb5f03a90a51293fc6d516788730953899"})
+		require.Contains(t, refs, git.Reference{Name: "refs/tags/v1.1.0", Target: "8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b"})
 
 		sha, err := repo.ResolveRevision(ctx, git.Revision("refs/remotes/origin/master^{commit}"))
 		require.NoError(t, err, "the object from remote should exists in local after fetch done")
 		require.Equal(t, git.ObjectID("1e292f8fedd741b75372e19097c76d327140c312"), sha)
+
+		require.NoFileExists(t, filepath.Join(repoPath, "FETCH_HEAD"))
 	})
 
 	t.Run("with env", func(t *testing.T) {
@@ -90,7 +91,7 @@ func TestRepo_FetchRemote(t *testing.T) {
 
 		var stderr bytes.Buffer
 		require.NoError(t, repo.FetchRemote(ctx, "source", FetchOpts{Stderr: &stderr, Env: []string{"GIT_TRACE=1"}}))
-		require.Contains(t, stderr.String(), "trace: built-in: git fetch --quiet --atomic --end-of-options source")
+		require.Contains(t, stderr.String(), "trace: built-in: git fetch --no-write-fetch-head --quiet --atomic --end-of-options source")
 	})
 
 	t.Run("with disabled transactions", func(t *testing.T) {
@@ -106,7 +107,7 @@ func TestRepo_FetchRemote(t *testing.T) {
 			Env:                 []string{"GIT_TRACE=1"},
 			DisableTransactions: true,
 		}))
-		require.Contains(t, stderr.String(), "trace: built-in: git fetch --quiet --end-of-options source")
+		require.Contains(t, stderr.String(), "trace: built-in: git fetch --no-write-fetch-head --quiet --end-of-options source")
 	})
 
 	t.Run("with globals", func(t *testing.T) {
diff --git a/internal/git/objectpool/fetch.go b/internal/git/objectpool/fetch.go
index 8bb807184..9fd35efe9 100644
--- a/internal/git/objectpool/fetch.go
+++ b/internal/git/objectpool/fetch.go
@@ -55,6 +55,10 @@ func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *gitalypb.Repos
 				// want to fetch them a second time via Git's default
 				// tag refspec.
 				git.Flag{Name: "--no-tags"},
+				// We don't need FETCH_HEAD, and it can potentially be hundreds of
+				// megabytes when doing a mirror-sync of repos with huge numbers of
+				// references.
+				git.Flag{Name: "--no-write-fetch-head"},
 			},
 			Args: []string{originPath, refSpec},
 		},
diff --git a/internal/git/objectpool/fetch_test.go b/internal/git/objectpool/fetch_test.go
index d650be1da..79e7d3887 100644
--- a/internal/git/objectpool/fetch_test.go
+++ b/internal/git/objectpool/fetch_test.go
@@ -226,6 +226,8 @@ func TestFetchFromOrigin_refs(t *testing.T) {
 		},
 		strings.Split(text.ChompBytes(gittest.Exec(t, cfg, "-C", poolPath, "for-each-ref", "--format=%(refname)")), "\n"),
 	)
+
+	require.NoFileExists(t, filepath.Join(poolPath, "FETCH_HEAD"))
 }
 
 func resolveRef(t *testing.T, cfg config.Cfg, repo string, ref string) string {
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index 1538da9b2..d42e1e6a9 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -79,6 +79,8 @@ func SkipWithPraefect(t testing.TB, reason string) {
 
 // MustReadFile returns the content of a file or fails at once.
 func MustReadFile(t testing.TB, filename string) []byte {
+	t.Helper()
+
 	content, err := os.ReadFile(filename)
 	if err != nil {
 		t.Fatal(err)