repoutil: Add option to skip initializing repository in Create

The `Create()` helper will make sure to call the repository-seeding
function with an already initialized Git repository so that we can skip
the step of having to call git-init(1) at all the callsites. While this
is generally useful, some of the users of `Create()` need to create the
target repository by calling git-clone(1), which isn't happy in case the
target directory exists and is not an empty directory. These callers
thus manually `os.RemoveAll()` the target directory right now before
doing the clone.

Provide a new option `WithSkipInit()` that allows callers to skip the
call to git-init(1). Convert callsites to use the new option.

4 files changed, 56 insertions, 34 deletions

diff --git a/internal/gitaly/repoutil/create.go b/internal/gitaly/repoutil/create.go
index 6f9b63345..3cbc6f441 100644
--- a/internal/gitaly/repoutil/create.go
+++ b/internal/gitaly/repoutil/create.go
@@ -22,6 +22,7 @@ import (
 
 type createConfig struct {
 	gitOptions []git.Option
+	skipInit   bool
 }
 
 // CreateOption is an option that can be passed to Create.
@@ -46,6 +47,15 @@ func WithObjectHash(hash git.ObjectHash) CreateOption {
 	}
 }
 
+// WithSkipInit causes Create to skip calling git-init(1) so that the seeding function will be
+// called with a nonexistent target directory. This can be useful when using git-clone(1) to seed
+// the repository.
+func WithSkipInit() CreateOption {
+	return func(cfg *createConfig) {
+		cfg.skipInit = true
+	}
+}
+
 // Create will create a new repository in a race-free way with proper transactional semantics. The
 // repository will only be created if it doesn't yet exist and if nodes which take part in the
 // transaction reach quorum. Otherwise, the target path of the new repository will not be modified.
@@ -95,20 +105,27 @@ func Create(
 		option(&cfg)
 	}
 
-	stderr := &bytes.Buffer{}
-	cmd, err := gitCmdFactory.NewWithoutRepo(ctx, git.Command{
-		Name: "init",
-		Flags: append([]git.Option{
-			git.Flag{Name: "--bare"},
-			git.Flag{Name: "--quiet"},
-		}, cfg.gitOptions...),
-		Args: []string{newRepoDir.Path()},
-	}, git.WithStderr(stderr))
-	if err != nil {
-		return fmt.Errorf("spawning git-init: %w", err)
-	}
-	if err := cmd.Wait(); err != nil {
-		return fmt.Errorf("creating repository: %w, stderr: %q", err, stderr.String())
+	if !cfg.skipInit {
+		stderr := &bytes.Buffer{}
+		cmd, err := gitCmdFactory.NewWithoutRepo(ctx, git.Command{
+			Name: "init",
+			Flags: append([]git.Option{
+				git.Flag{Name: "--bare"},
+				git.Flag{Name: "--quiet"},
+			}, cfg.gitOptions...),
+			Args: []string{newRepoDir.Path()},
+		}, git.WithStderr(stderr))
+		if err != nil {
+			return fmt.Errorf("spawning git-init: %w", err)
+		}
+
+		if err := cmd.Wait(); err != nil {
+			return fmt.Errorf("creating repository: %w, stderr: %q", err, stderr.String())
+		}
+	} else {
+		if err := os.Remove(newRepoDir.Path()); err != nil {
+			return fmt.Errorf("removing precreated directory: %w", err)
+		}
 	}
 
 	if err := seedRepository(newRepo); err != nil {
diff --git a/internal/gitaly/repoutil/create_test.go b/internal/gitaly/repoutil/create_test.go
index 5264cb9d8..e4b7b38d2 100644
--- a/internal/gitaly/repoutil/create_test.go
+++ b/internal/gitaly/repoutil/create_test.go
@@ -266,6 +266,24 @@ func TestCreate(t *testing.T) {
 				require.Equal(t, "sha256", objectFormat)
 			},
 		},
+		{
+			desc: "skip initialization",
+			opts: []CreateOption{
+				WithSkipInit(),
+			},
+			seed: func(t *testing.T, repo *gitalypb.Repository, repoPath string) error {
+				require.NoDirExists(t, repoPath)
+				gittest.Exec(t, cfg, "init", "--bare", repoPath)
+				return nil
+			},
+			verify: func(t *testing.T, tempRepo *gitalypb.Repository, tempRepoPath string, realRepo *gitalypb.Repository, realRepoPath string) {
+				require.NoDirExists(t, tempRepoPath)
+
+				// But the new repository must exist.
+				isBareRepo := gittest.Exec(t, cfg, "-C", realRepoPath, "rev-parse", "--is-bare-repository")
+				require.Equal(t, "true", text.ChompBytes(isBareRepo))
+			},
+		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
 			// Make sure that we don't leak either the context or the mocked transaction
@@ -301,17 +319,17 @@ func TestCreate(t *testing.T) {
 				require.Equal(t, repo.StorageName, tempRepo.StorageName)
 				require.True(t, strings.HasPrefix(tempRepo.RelativePath, "+gitaly/tmp/repo"))
 
-				// Verify that the temporary repository exists and is a real Git
-				// repository.
-				tempRepoPath, err := locator.GetRepoPath(tempRepo)
+				tempRepoPath, err := locator.GetPath(tempRepo)
 				require.NoError(t, err)
-				isBareRepo := gittest.Exec(t, cfg, "-C", tempRepoPath, "rev-parse", "--is-bare-repository")
-				require.Equal(t, "true", text.ChompBytes(isBareRepo))
 
 				if tc.seed != nil {
 					return tc.seed(t, tempRepo, tempRepoPath)
 				}
 
+				// Verify that the repository exists now and is a real repository.
+				isBareRepo := gittest.Exec(t, cfg, "-C", tempRepoPath, "rev-parse", "--is-bare-repository")
+				require.Equal(t, "true", text.ChompBytes(isBareRepo))
+
 				return nil
 			}, tc.opts...))
 
diff --git a/internal/gitaly/service/repository/create_fork.go b/internal/gitaly/service/repository/create_fork.go
index d0a666463..bb172eac5 100644
--- a/internal/gitaly/service/repository/create_fork.go
+++ b/internal/gitaly/service/repository/create_fork.go
@@ -3,7 +3,6 @@ package repository
 import (
 	"context"
 	"fmt"
-	"os"
 	"strings"
 
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
@@ -30,12 +29,6 @@ func (s *server) CreateFork(ctx context.Context, req *gitalypb.CreateForkRequest
 			return err
 		}
 
-		// git-clone(1) doesn't allow for the target path to exist, so we have to
-		// remove it first.
-		if err := os.RemoveAll(targetPath); err != nil {
-			return fmt.Errorf("removing target path: %w", err)
-		}
-
 		// Ideally we'd just fetch into the already-created repo, but that wouldn't
 		// allow us to easily set up HEAD to point to the correct ref. We thus have
 		// no easy choice but to use git-clone(1).
@@ -78,7 +71,7 @@ func (s *server) CreateFork(ctx context.Context, req *gitalypb.CreateForkRequest
 		}
 
 		return nil
-	}); err != nil {
+	}, repoutil.WithSkipInit()); err != nil {
 		return nil, structerr.NewInternal("creating fork: %w", err)
 	}
 
diff --git a/internal/gitaly/service/repository/create_repository_from_url.go b/internal/gitaly/service/repository/create_repository_from_url.go
index d084501dc..46c421104 100644
--- a/internal/gitaly/service/repository/create_repository_from_url.go
+++ b/internal/gitaly/service/repository/create_repository_from_url.go
@@ -6,7 +6,6 @@ import (
 	"encoding/base64"
 	"fmt"
 	"net/url"
-	"os"
 
 	"gitlab.com/gitlab-org/gitaly/v15/internal/command"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
@@ -95,11 +94,6 @@ func (s *server) CreateRepositoryFromURL(ctx context.Context, req *gitalypb.Crea
 			return fmt.Errorf("getting temporary repository path: %w", err)
 		}
 
-		// We need to remove the target path first so git-clone(1) doesn't complain.
-		if err := os.RemoveAll(targetPath); err != nil {
-			return fmt.Errorf("removing temporary repository: %w", err)
-		}
-
 		var stderr bytes.Buffer
 		cmd, err := s.cloneFromURLCommand(ctx,
 			req.GetUrl(),
@@ -125,7 +119,7 @@ func (s *server) CreateRepositoryFromURL(ctx context.Context, req *gitalypb.Crea
 		}
 
 		return nil
-	}); err != nil {
+	}, repoutil.WithSkipInit()); err != nil {
 		return nil, structerr.NewInternal("creating repository: %w", err)
 	}