Add option to explicitly disable transactions for fetch

Since fetching without transactions should be the exception, we make the
default to fetch with transactions.

4 files changed, 55 insertions, 9 deletions

diff --git a/internal/git/localrepo/remote.go b/internal/git/localrepo/remote.go
index 949bcd26f..a490473a9 100644
--- a/internal/git/localrepo/remote.go
+++ b/internal/git/localrepo/remote.go
@@ -217,6 +217,8 @@ type FetchOpts struct {
 	Tags FetchOptsTags
 	// Stderr if set it would be used to redirect stderr stream into it.
 	Stderr io.Writer
+	// DisableTransactions will disable the reference-transaction hook and atomic transactions.
+	DisableTransactions bool
 }
 
 // ErrFetchFailed indicates that the fetch has failed.
@@ -244,7 +246,11 @@ func (repo *Repo) FetchRemote(ctx context.Context, remoteName string, opts Fetch
 	commandOptions := []git.CmdOpt{
 		git.WithEnv(opts.Env...),
 		git.WithStderr(opts.Stderr),
-		git.WithDisabledHooks(),
+	}
+	if opts.DisableTransactions {
+		commandOptions = append(commandOptions, git.WithDisabledHooks())
+	} else {
+		commandOptions = append(commandOptions, git.WithRefTxHook(ctx, repo, repo.cfg))
 	}
 	commandOptions = append(commandOptions, opts.CommandOptions...)
 
@@ -295,7 +301,6 @@ func (repo *Repo) FetchInternal(
 	commandOptions := []git.CmdOpt{
 		git.WithEnv(append(env, opts.Env...)...),
 		git.WithStderr(opts.Stderr),
-		git.WithRefTxHook(ctx, repo, repo.cfg),
 		// 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 alternates. Given that object pools
@@ -305,12 +310,17 @@ func (repo *Repo) FetchInternal(
 		// matter in the connectivity check either.
 		git.WithConfig(git.ConfigPair{Key: "core.alternateRefsCommand", Value: "exit 0 #"}),
 	}
+	if opts.DisableTransactions {
+		commandOptions = append(commandOptions, git.WithDisabledHooks())
+	} else {
+		commandOptions = append(commandOptions, git.WithRefTxHook(ctx, repo, repo.cfg))
+	}
 	commandOptions = append(commandOptions, opts.CommandOptions...)
 
 	if err := repo.ExecAndWait(ctx,
 		git.SubCmd{
 			Name:  "fetch",
-			Flags: append(opts.buildFlags(), git.Flag{Name: "--atomic"}),
+			Flags: opts.buildFlags(),
 			Args:  append([]string{gitalyssh.GitalyInternalURL}, refspecs...),
 		},
 		commandOptions...,
@@ -340,6 +350,10 @@ func (opts FetchOpts) buildFlags() []git.Option {
 		flags = append(flags, git.Flag{Name: opts.Tags.String()})
 	}
 
+	if !opts.DisableTransactions {
+		flags = append(flags, git.Flag{Name: "--atomic"})
+	}
+
 	return flags
 }
 
diff --git a/internal/git/localrepo/remote_extra_test.go b/internal/git/localrepo/remote_extra_test.go
index f80be863b..2bbcd69f0 100644
--- a/internal/git/localrepo/remote_extra_test.go
+++ b/internal/git/localrepo/remote_extra_test.go
@@ -147,7 +147,22 @@ 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")
+		require.Contains(t, stderr.String(), "trace: built-in: git fetch --quiet --atomic --end-of-options")
+	})
+
+	t.Run("with disabled transactions", func(t *testing.T) {
+		ctx := testhelper.MergeIncomingMetadata(ctx, testhelper.GitalyServersMetadataFromCfg(t, cfg))
+
+		repoProto, _ := gittest.InitRepo(t, cfg, cfg.Storages[0])
+		repo := localrepo.NewTestRepo(t, cfg, repoProto)
+
+		var stderr bytes.Buffer
+		err := repo.FetchInternal(
+			ctx, remoteRepoProto, []string{"refs/heads/master"},
+			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")
 	})
 
 	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 4f1a8276a..6b165f0c9 100644
--- a/internal/git/localrepo/remote_test.go
+++ b/internal/git/localrepo/remote_test.go
@@ -361,6 +361,22 @@ 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")
+	})
+
+	t.Run("with disabled transactions", func(t *testing.T) {
+		_, sourceRepoPath := gittest.CloneRepo(t, cfg, cfg.Storages[0])
+		testRepo, testRepoPath := gittest.CloneRepo(t, cfg, cfg.Storages[0])
+
+		repo := New(remoteCmd.repo.gitCmdFactory, remoteCmd.repo.catfileCache, testRepo, cfg)
+		gittest.Exec(t, cfg, "-C", testRepoPath, "remote", "add", "source", sourceRepoPath)
+
+		var stderr bytes.Buffer
+		require.NoError(t, repo.FetchRemote(ctx, "source", FetchOpts{
+			Stderr:              &stderr,
+			Env:                 []string{"GIT_TRACE=1"},
+			DisableTransactions: true,
+		}))
 		require.Contains(t, stderr.String(), "trace: built-in: git fetch --quiet --end-of-options source")
 	})
 
diff --git a/internal/gitaly/service/repository/fetch_remote.go b/internal/gitaly/service/repository/fetch_remote.go
index 34a9c90fa..0a18c9140 100644
--- a/internal/gitaly/service/repository/fetch_remote.go
+++ b/internal/gitaly/service/repository/fetch_remote.go
@@ -26,11 +26,12 @@ func (s *server) FetchRemote(ctx context.Context, req *gitalypb.FetchRemoteReque
 
 	var stderr bytes.Buffer
 	opts := localrepo.FetchOpts{
-		Stderr:  &stderr,
-		Force:   req.Force,
-		Prune:   !req.NoPrune,
-		Tags:    localrepo.FetchOptsTagsAll,
-		Verbose: req.GetCheckTagsChanged(),
+		Stderr:              &stderr,
+		Force:               req.Force,
+		Prune:               !req.NoPrune,
+		Tags:                localrepo.FetchOptsTagsAll,
+		Verbose:             req.GetCheckTagsChanged(),
+		DisableTransactions: true,
 	}
 
 	if req.GetNoTags() {