Merge branch 'zj-catfile-git-dsl' into 'master'

Use the Git DSL for catfile processes

Closes #1933 and #1934

See merge request gitlab-org/gitaly!1624

3 files changed, 30 insertions, 20 deletions

diff --git a/internal/git/catfile/batch.go b/internal/git/catfile/batch.go
index d37c9c718..0099ef6dd 100644
--- a/internal/git/catfile/batch.go
+++ b/internal/git/catfile/batch.go
@@ -9,6 +9,8 @@ import (
 	"sync"
 
 	"gitlab.com/gitlab-org/gitaly/internal/git"
+	"gitlab.com/gitlab-org/gitaly/internal/git/alternates"
+	"gitlab.com/gitlab-org/gitaly/internal/git/repository"
 )
 
 // batch encapsulates a 'git cat-file --batch' process
@@ -30,16 +32,21 @@ type batchProcess struct {
 	sync.Mutex
 }
 
-func newBatchProcess(ctx context.Context, repoPath string, env []string) (*batchProcess, error) {
-	totalCatfileProcesses.Inc()
+func newBatchProcess(ctx context.Context, repo repository.GitRepo) (*batchProcess, error) {
+	repoPath, env, err := alternates.PathAndEnv(repo)
+	if err != nil {
+		return nil, err
+	}
 
+	totalCatfileProcesses.Inc()
 	b := &batchProcess{}
 
 	var stdinReader io.Reader
 	stdinReader, b.w = io.Pipe()
-	batchCmdArgs := []string{"--git-dir", repoPath, "cat-file", "--batch"}
 
-	batchCmd, err := git.BareCommand(ctx, stdinReader, nil, nil, env, batchCmdArgs...)
+	batchCmd, err := git.SafeBareCmd(ctx, stdinReader, nil, nil, env,
+		[]git.Option{git.ValueFlag{Name: "--git-dir", Value: repoPath}},
+		git.SubCmd{Name: "cat-file", Flags: []git.Option{git.Flag{"--batch"}}})
 	if err != nil {
 		return nil, err
 	}
diff --git a/internal/git/catfile/batchcheck.go b/internal/git/catfile/batchcheck.go
index 3734e2293..305ca14b1 100644
--- a/internal/git/catfile/batchcheck.go
+++ b/internal/git/catfile/batchcheck.go
@@ -8,6 +8,8 @@ import (
 	"sync"
 
 	"gitlab.com/gitlab-org/gitaly/internal/git"
+	"gitlab.com/gitlab-org/gitaly/internal/git/alternates"
+	"gitlab.com/gitlab-org/gitaly/internal/git/repository"
 )
 
 // batchCheck encapsulates a 'git cat-file --batch-check' process
@@ -17,13 +19,20 @@ type batchCheck struct {
 	sync.Mutex
 }
 
-func newBatchCheck(ctx context.Context, repoPath string, env []string) (*batchCheck, error) {
+func newBatchCheck(ctx context.Context, repo repository.GitRepo) (*batchCheck, error) {
+	repoPath, env, err := alternates.PathAndEnv(repo)
+	if err != nil {
+		return nil, err
+	}
+
 	bc := &batchCheck{}
 
 	var stdinReader io.Reader
 	stdinReader, bc.w = io.Pipe()
-	batchCmdArgs := []string{"--git-dir", repoPath, "cat-file", "--batch-check"}
-	batchCmd, err := git.BareCommand(ctx, stdinReader, nil, nil, env, batchCmdArgs...)
+
+	batchCmd, err := git.SafeBareCmd(ctx, stdinReader, nil, nil, env,
+		[]git.Option{git.ValueFlag{Name: "--git-dir", Value: repoPath}},
+		git.SubCmd{Name: "cat-file", Flags: []git.Option{git.Flag{"--batch-check"}}})
 	if err != nil {
 		return nil, err
 	}
diff --git a/internal/git/catfile/catfile.go b/internal/git/catfile/catfile.go
index 897282836..1226971bb 100644
--- a/internal/git/catfile/catfile.go
+++ b/internal/git/catfile/catfile.go
@@ -6,9 +6,8 @@ import (
 	"sync"
 
 	"github.com/prometheus/client_golang/prometheus"
-	"gitlab.com/gitlab-org/gitaly/internal/git/alternates"
+	"gitlab.com/gitlab-org/gitaly/internal/git/repository"
 	"gitlab.com/gitlab-org/gitaly/internal/metadata"
-	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 )
 
 var catfileCacheCounter = prometheus.NewCounterVec(
@@ -139,19 +138,14 @@ func (c *Batch) isClosed() bool {
 // New returns a new Batch instance. It is important that ctx gets canceled
 // somewhere, because if it doesn't the cat-file processes spawned by
 // New() never terminate.
-func New(ctx context.Context, repo *gitalypb.Repository) (*Batch, error) {
+func New(ctx context.Context, repo repository.GitRepo) (*Batch, error) {
 	if ctx.Done() == nil {
 		panic("empty ctx.Done() in catfile.Batch.New()")
 	}
 
-	repoPath, env, err := alternates.PathAndEnv(repo)
-	if err != nil {
-		return nil, err
-	}
-
 	sessionID := metadata.GetValue(ctx, SessionIDField)
 	if sessionID == "" {
-		return newBatch(ctx, repoPath, env)
+		return newBatch(ctx, repo)
 	}
 
 	cacheKey := newCacheKey(sessionID, repo)
@@ -165,7 +159,7 @@ func New(ctx context.Context, repo *gitalypb.Repository) (*Batch, error) {
 	// if we are using caching, create a fresh context for the new batch
 	// and initialize the new batch with a cache key and cancel function
 	cacheCtx, cacheCancel := context.WithCancel(context.Background())
-	c, err := newBatch(cacheCtx, repoPath, env)
+	c, err := newBatch(cacheCtx, repo)
 	if err != nil {
 		return nil, err
 	}
@@ -198,7 +192,7 @@ type simulatedBatchSpawnError struct{}
 
 func (simulatedBatchSpawnError) Error() string { return "simulated spawn error" }
 
-func newBatch(_ctx context.Context, repoPath string, env []string) (_ *Batch, err error) {
+func newBatch(_ctx context.Context, repo repository.GitRepo) (_ *Batch, err error) {
 	ctx, cancel := context.WithCancel(_ctx)
 	defer func() {
 		if err != nil {
@@ -206,12 +200,12 @@ func newBatch(_ctx context.Context, repoPath string, env []string) (_ *Batch, er
 		}
 	}()
 
-	batch, err := newBatchProcess(ctx, repoPath, env)
+	batch, err := newBatchProcess(ctx, repo)
 	if err != nil {
 		return nil, err
 	}
 
-	batchCheck, err := newBatchCheck(ctx, repoPath, env)
+	batchCheck, err := newBatchCheck(ctx, repo)
 	if err != nil {
 		return nil, err
 	}