Refactor CatFile batch implementation and introduce batch-command for git 2.36

modules/git/batch.go

@@ -8,40 +8,153 @@ import (
 	"context"
 )
 
-type Batch struct {
+type batchCatFile struct {
 	cancel context.CancelFunc
 	Reader *bufio.Reader
 	Writer WriteCloserError
 }
 
-// NewBatch creates a new batch for the given repository, the Close must be invoked before release the batch
-func NewBatch(ctx context.Context, repoPath string) (*Batch, error) {
+func (b *batchCatFile) Close() {
+	if b.cancel != nil {
+		b.cancel()
+		b.Reader = nil
+		b.Writer = nil
+		b.cancel = nil
+	}
+}
+
+type Batch interface {
+	Write([]byte) (int, error)
+	WriteCheck([]byte) (int, error)
+	Reader() *bufio.Reader
+	CheckReader() *bufio.Reader
+	Close()
+}
+
+// batchCatFileWithCheck implements the Batch interface using the "cat-file --batch" command and "cat-file --batch-check" command
+// ref: https://git-scm.com/docs/git-cat-file#Documentation/git-cat-file.txt---batch
+// To align with --batch-command, we creates the two commands both at the same time if git version is lower than 2.36
+type batchCatFileWithCheck struct {
+	ctx        context.Context
+	repoPath   string
+	batch      *batchCatFile
+	batchCheck *batchCatFile
+}
+
+var _ Batch = &batchCatFileWithCheck{}
+
+// newBatchCatFileWithCheck creates a new batch and a new batch check for the given repository, the Close must be invoked before release the batch
+func newBatchCatFileWithCheck(ctx context.Context, repoPath string) (*batchCatFileWithCheck, error) {
 	// Now because of some insanity with git cat-file not immediately failing if not run in a valid git directory we need to run git rev-parse first!
 	if err := ensureValidGitRepository(ctx, repoPath); err != nil {
 		return nil, err
 	}
 
-	var batch Batch
-	batch.Writer, batch.Reader, batch.cancel = catFileBatch(ctx, repoPath)
-	return &batch, nil
+	return &batchCatFileWithCheck{
+		ctx:      ctx,
+		repoPath: repoPath,
+	}, nil
+}
+
+func (b *batchCatFileWithCheck) getBatch() *batchCatFile {
+	if b.batch != nil {
+		return b.batch
+	}
+	b.batch = newCatFileBatch(b.ctx, b.repoPath, BatchArg)
+	return b.batch
+}
+
+func (b *batchCatFileWithCheck) getBatchCheck() *batchCatFile {
+	if b.batchCheck != nil {
+		return b.batchCheck
+	}
+	b.batchCheck = newCatFileBatch(b.ctx, b.repoPath, BatchCheckArg)
+	return b.batchCheck
+}
+
+func (b *batchCatFileWithCheck) Write(bs []byte) (int, error) {
+	return b.getBatch().Writer.Write(bs)
+}
+
+func (b *batchCatFileWithCheck) WriteCheck(bs []byte) (int, error) {
+	return b.getBatchCheck().Writer.Write(bs)
+}
+
+func (b *batchCatFileWithCheck) Reader() *bufio.Reader {
+	return b.getBatch().Reader
+}
+
+func (b *batchCatFileWithCheck) CheckReader() *bufio.Reader {
+	return b.getBatchCheck().Reader
 }
 
-func NewBatchCheck(ctx context.Context, repoPath string) (*Batch, error) {
+func (b *batchCatFileWithCheck) Close() {
+	if b.batch != nil {
+		b.batch.Close()
+		b.batch = nil
+	}
+	if b.batchCheck != nil {
+		b.batchCheck.Close()
+		b.batchCheck = nil
+	}
+}
+
+// batchCommandCatFile implements the Batch interface using the "cat-file --batch-command" command
+// ref: https://git-scm.com/docs/git-cat-file#Documentation/git-cat-file.txt---batch-command
+type batchCommandCatFile struct {
+	ctx      context.Context
+	repoPath string
+	batch    *batchCatFile
+}
+
+var _ Batch = &batchCommandCatFile{}
+
+func newBatchCommandCatFile(ctx context.Context, repoPath string) (*batchCommandCatFile, error) {
 	// Now because of some insanity with git cat-file not immediately failing if not run in a valid git directory we need to run git rev-parse first!
 	if err := ensureValidGitRepository(ctx, repoPath); err != nil {
 		return nil, err
 	}
 
-	var check Batch
-	check.Writer, check.Reader, check.cancel = catFileBatchCheck(ctx, repoPath)
-	return &check, nil
+	return &batchCommandCatFile{
+		ctx:      ctx,
+		repoPath: repoPath,
+	}, nil
 }
 
-func (b *Batch) Close() {
-	if b.cancel != nil {
-		b.cancel()
-		b.Reader = nil
-		b.Writer = nil
-		b.cancel = nil
+func (b *batchCommandCatFile) getBatch() *batchCatFile {
+	if b.batch != nil {
+		return b.batch
+	}
+	b.batch = newCatFileBatch(b.ctx, b.repoPath, BatchCommandArg)
+	return b.batch
+}
+
+func (b *batchCommandCatFile) Write(bs []byte) (int, error) {
+	return b.getBatch().Writer.Write(append([]byte("contents "), bs...))
+}
+
+func (b *batchCommandCatFile) WriteCheck(bs []byte) (int, error) {
+	return b.getBatch().Writer.Write(append([]byte("info "), bs...))
+}
+
+func (b *batchCommandCatFile) Reader() *bufio.Reader {
+	return b.getBatch().Reader
+}
+
+func (b *batchCommandCatFile) CheckReader() *bufio.Reader {
+	return b.getBatch().Reader
+}
+
+func (b *batchCommandCatFile) Close() {
+	if b.batch != nil {
+		b.batch.Close()
+		b.batch = nil
+	}
+}
+
+func NewBatch(ctx context.Context, repoPath string) (Batch, error) {
+	if DefaultFeatures().SupportCatFileBatchCommand {
+		return newBatchCommandCatFile(ctx, repoPath)
 	}
+	return newBatchCatFileWithCheck(ctx, repoPath)
 }

modules/git/batch_reader.go

@@ -7,6 +7,7 @@ import (
 	"bufio"
 	"bytes"
 	"context"
+	"fmt"
 	"io"
 	"math"
 	"strconv"
@@ -40,52 +41,23 @@ func ensureValidGitRepository(ctx context.Context, repoPath string) error {
 	return nil
 }
 
-// catFileBatchCheck opens git cat-file --batch-check in the provided repo and returns a stdin pipe, a stdout reader and cancel function
-func catFileBatchCheck(ctx context.Context, repoPath string) (WriteCloserError, *bufio.Reader, func()) {
-	batchStdinReader, batchStdinWriter := io.Pipe()
-	batchStdoutReader, batchStdoutWriter := io.Pipe()
-	ctx, ctxCancel := context.WithCancel(ctx)
-	closed := make(chan struct{})
-	cancel := func() {
-		ctxCancel()
-		_ = batchStdoutReader.Close()
-		_ = batchStdinWriter.Close()
-		<-closed
-	}
-
-	// Ensure cancel is called as soon as the provided context is cancelled
-	go func() {
-		<-ctx.Done()
-		cancel()
-	}()
-
-	go func() {
-		stderr := strings.Builder{}
-		err := gitcmd.NewCommand("cat-file", "--batch-check").
-			WithDir(repoPath).
-			WithStdin(batchStdinReader).
-			WithStdout(batchStdoutWriter).
-			WithStderr(&stderr).
-			WithUseContextTimeout(true).
-			Run(ctx)
-		if err != nil {
-			_ = batchStdoutWriter.CloseWithError(gitcmd.ConcatenateError(err, (&stderr).String()))
-			_ = batchStdinReader.CloseWithError(gitcmd.ConcatenateError(err, (&stderr).String()))
-		} else {
-			_ = batchStdoutWriter.Close()
-			_ = batchStdinReader.Close()
-		}
-		close(closed)
-	}()
-
-	// For simplicities sake we'll use a buffered reader to read from the cat-file --batch-check
-	batchReader := bufio.NewReader(batchStdoutReader)
+const (
+	BatchArg = iota
+	BatchCheckArg
+	BatchCommandArg
+)
 
-	return batchStdinWriter, batchReader, cancel
+func isValidBatchArg(arg int) bool {
+	return arg == BatchArg || arg == BatchCheckArg || arg == BatchCommandArg
 }
 
-// catFileBatch opens git cat-file --batch in the provided repo and returns a stdin pipe, a stdout reader and cancel function
-func catFileBatch(ctx context.Context, repoPath string) (WriteCloserError, *bufio.Reader, func()) {
+// newCatFileBatch opens git cat-file --batch in the provided repo and returns a stdin pipe, a stdout reader and cancel function
+// batchArg is the argument to pass to cat-file --batch, it could be "--batch", "--batch-command" or "--batch-check".
+func newCatFileBatch(ctx context.Context, repoPath string, batchArg int) *batchCatFile {
+	if !isValidBatchArg(batchArg) {
+		panic(fmt.Sprintf("invalid batchArg: %d", batchArg))
+	}
+
 	// We often want to feed the commits in order into cat-file --batch, followed by their trees and sub trees as necessary.
 	// so let's create a batch stdin and stdout
 	batchStdinReader, batchStdinWriter := io.Pipe()
@@ -107,7 +79,16 @@ func catFileBatch(ctx context.Context, repoPath string) (WriteCloserError, *bufi
 
 	go func() {
 		stderr := strings.Builder{}
-		err := gitcmd.NewCommand("cat-file", "--batch").
+		cmd := gitcmd.NewCommand("cat-file")
+		switch batchArg {
+		case BatchArg:
+			cmd.AddArguments("--batch")
+		case BatchCheckArg:
+			cmd.AddArguments("--batch-check")
+		case BatchCommandArg:
+			cmd.AddArguments("--batch-command")
+		}
+		err := cmd.
 			WithDir(repoPath).
 			WithStdin(batchStdinReader).
 			WithStdout(batchStdoutWriter).
@@ -127,7 +108,11 @@ func catFileBatch(ctx context.Context, repoPath string) (WriteCloserError, *bufi
 	// For simplicities sake we'll us a buffered reader to read from the cat-file --batch
 	batchReader := bufio.NewReaderSize(batchStdoutReader, 32*1024)
 
-	return batchStdinWriter, batchReader, cancel
+	return &batchCatFile{
+		Writer: batchStdinWriter,
+		Reader: batchReader,
+		cancel: cancel,
+	}
 }
 
 // ReadBatchLine reads the header line from cat-file --batch

modules/git/blob_nogogit.go

@@ -26,16 +26,17 @@ type Blob struct {
 // DataAsync gets a ReadCloser for the contents of a blob without reading it all.
 // Calling the Close function on the result will discard all unread output.
 func (b *Blob) DataAsync() (io.ReadCloser, error) {
-	wr, rd, cancel, err := b.repo.CatFileBatch(b.repo.Ctx)
+	batch, cancel, err := b.repo.CatFileBatch(b.repo.Ctx)
 	if err != nil {
 		return nil, err
 	}
 
-	_, err = wr.Write([]byte(b.ID.String() + "\n"))
+	_, err = batch.Write([]byte(b.ID.String() + "\n"))
 	if err != nil {
 		cancel()
 		return nil, err
 	}
+	rd := batch.Reader()
 	_, _, size, err := ReadBatchLine(rd)
 	if err != nil {
 		cancel()
@@ -67,18 +68,18 @@ func (b *Blob) Size() int64 {
 		return b.size
 	}
 
-	wr, rd, cancel, err := b.repo.CatFileBatchCheck(b.repo.Ctx)
+	batch, cancel, err := b.repo.CatFileBatch(b.repo.Ctx)
 	if err != nil {
 		log.Debug("error whilst reading size for %s in %s. Error: %v", b.ID.String(), b.repo.Path, err)
 		return 0
 	}
 	defer cancel()
-	_, err = wr.Write([]byte(b.ID.String() + "\n"))
+	_, err = batch.WriteCheck([]byte(b.ID.String() + "\n"))
 	if err != nil {
 		log.Debug("error whilst reading size for %s in %s. Error: %v", b.ID.String(), b.repo.Path, err)
 		return 0
 	}
-	_, _, b.size, err = ReadBatchLine(rd)
+	_, _, b.size, err = ReadBatchLine(batch.CheckReader())
 	if err != nil {
 		log.Debug("error whilst reading size for %s in %s. Error: %v", b.ID.String(), b.repo.Path, err)
 		return 0

modules/git/blob_test.go

@@ -9,11 +9,25 @@ import (
 	"path/filepath"
 	"testing"
 
+	"code.gitea.io/gitea/modules/test"
+
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
-func TestBlob_Data(t *testing.T) {
+func TestBlob_Data_Batch(t *testing.T) {
+	defer test.MockVariableValue(&DefaultFeatures().SupportCatFileBatchCommand, false)()
+
+	testBlobData(t)
+}
+
+func TestBlob_Data_BatchCommand(t *testing.T) {
+	defer test.MockVariableValue(&DefaultFeatures().SupportCatFileBatchCommand, true)()
+
+	testBlobData(t)
+}
+
+func testBlobData(t *testing.T) {
 	output := "file2\n"
 	bareRepo1Path := filepath.Join(testReposDir, "repo1_bare")
 	repo, err := OpenRepository(t.Context(), bareRepo1Path)