Refactor git command stdio pipe

go.mod

@@ -39,8 +39,6 @@ require (
 	github.com/charmbracelet/git-lfs-transfer v0.1.1-0.20251013092601-6327009efd21
 	github.com/chi-middleware/proxy v1.1.1
 	github.com/dimiro1/reply v0.0.0-20200315094148-d0136a4c9e21
-	github.com/djherbis/buffer v1.2.0
-	github.com/djherbis/nio/v3 v3.0.1
 	github.com/dsnet/compress v0.0.2-0.20230904184137-39efe44ab707
 	github.com/dustin/go-humanize v1.0.1
 	github.com/editorconfig/editorconfig-core-go/v2 v2.6.3

go.sum

@@ -260,11 +260,6 @@ github.com/dgryski/go-rendezvous v0.0.0-20200823014737-9f7001d12a5f h1:lO4WD4F/r
 github.com/dgryski/go-rendezvous v0.0.0-20200823014737-9f7001d12a5f/go.mod h1:cuUVRXasLTGF7a8hSLbxyZXjz+1KgoB3wDUb6vlszIc=
 github.com/dimiro1/reply v0.0.0-20200315094148-d0136a4c9e21 h1:PdsjTl0Cg+ZJgOx/CFV5NNgO1ThTreqdgKYiDCMHJwA=
 github.com/dimiro1/reply v0.0.0-20200315094148-d0136a4c9e21/go.mod h1:xJvkyD6Y2rZapGvPJLYo9dyx1s5dxBEDPa8T3YTuOk0=
-github.com/djherbis/buffer v1.1.0/go.mod h1:VwN8VdFkMY0DCALdY8o00d3IZ6Amz/UNVMWcSaJT44o=
-github.com/djherbis/buffer v1.2.0 h1:PH5Dd2ss0C7CRRhQCZ2u7MssF+No9ide8Ye71nPHcrQ=
-github.com/djherbis/buffer v1.2.0/go.mod h1:fjnebbZjCUpPinBRD+TDwXSOeNQ7fPQWLfGQqiAiUyE=
-github.com/djherbis/nio/v3 v3.0.1 h1:6wxhnuppteMa6RHA4L81Dq7ThkZH8SwnDzXDYy95vB4=
-github.com/djherbis/nio/v3 v3.0.1/go.mod h1:Ng4h80pbZFMla1yKzm61cF0tqqilXZYrogmWgZxOcmg=
 github.com/dlclark/regexp2 v1.2.0/go.mod h1:2pZnwuY/m+8K6iRw6wQdMtk+rH5tNGR1i55kozfMjCc=
 github.com/dlclark/regexp2 v1.4.0/go.mod h1:2pZnwuY/m+8K6iRw6wQdMtk+rH5tNGR1i55kozfMjCc=
 github.com/dlclark/regexp2 v1.7.0/go.mod h1:DHkYz0B9wPfa6wondMfaivmHpzrQ3v9q8cnmRbL6yW8=

modules/git/attribute/batch.go

@@ -7,7 +7,7 @@ import (
 	"bytes"
 	"context"
 	"fmt"
-	"os"
+	"io"
 	"path/filepath"
 	"time"
 
@@ -20,7 +20,7 @@ import (
 type BatchChecker struct {
 	attributesNum int
 	repo          *git.Repository
-	stdinWriter   *os.File
+	stdinWriter   io.WriteCloser
 	stdOut        *nulSeparatedAttributeWriter
 	ctx           context.Context
 	cancel        context.CancelFunc
@@ -60,32 +60,27 @@ func NewBatchChecker(repo *git.Repository, treeish string, attributes []string)
 		},
 	}
 
-	stdinReader, stdinWriter, err := os.Pipe()
-	if err != nil {
-		return nil, err
-	}
+	stdinWriter, stdinWriterClose := cmd.MakeStdinPipe()
 	checker.stdinWriter = stdinWriter
 
 	lw := new(nulSeparatedAttributeWriter)
 	lw.attributes = make(chan attributeTriple, len(attributes))
 	lw.closed = make(chan struct{})
 	checker.stdOut = lw
 
+	cmd.WithEnv(envs).
+		WithDir(repo.Path).
+		WithStdoutCopy(lw)
+
 	go func() {
-		defer func() {
-			_ = stdinReader.Close()
-			_ = lw.Close()
-		}()
-		err := cmd.WithEnv(envs).
-			WithDir(repo.Path).
-			WithStdin(stdinReader).
-			WithStdout(lw).
-			RunWithStderr(ctx)
+		defer stdinWriterClose()
+		defer checker.cancel()
+		defer lw.Close()
 
+		err := cmd.RunWithStderr(ctx)
 		if err != nil && !gitcmd.IsErrorCanceledOrKilled(err) {
 			log.Error("Attribute checker for commit %s exits with error: %v", treeish, err)
 		}
-		checker.cancel()
 	}()
 
 	return checker, nil

modules/git/attribute/checker.go

@@ -68,15 +68,14 @@ func CheckAttributes(ctx context.Context, gitRepo *git.Repository, treeish strin
 	}
 	defer cancel()
 
-	stdOut := new(bytes.Buffer)
-	if err := cmd.WithEnv(append(os.Environ(), envs...)).
+	stdout, _, err := cmd.WithEnv(append(os.Environ(), envs...)).
 		WithDir(gitRepo.Path).
-		WithStdout(stdOut).
-		RunWithStderr(ctx); err != nil {
+		RunStdBytes(ctx)
+	if err != nil {
 		return nil, fmt.Errorf("failed to run check-attr: %w", err)
 	}
 
-	fields := bytes.Split(stdOut.Bytes(), []byte{'\000'})
+	fields := bytes.Split(stdout, []byte{'\000'})
 	if len(fields)%3 != 1 {
 		return nil, errors.New("wrong number of fields in return from check-attr")
 	}

modules/git/catfile_batch_command.go

@@ -39,23 +39,23 @@ func (b *catFileBatchCommand) getBatch() *catFileBatchCommunicator {
 }
 
 func (b *catFileBatchCommand) QueryContent(obj string) (*CatFileObject, BufferedReader, error) {
-	_, err := b.getBatch().writer.Write([]byte("contents " + obj + "\n"))
+	_, err := b.getBatch().reqWriter.Write([]byte("contents " + obj + "\n"))
 	if err != nil {
 		return nil, nil, err
 	}
-	info, err := catFileBatchParseInfoLine(b.getBatch().reader)
+	info, err := catFileBatchParseInfoLine(b.getBatch().respReader)
 	if err != nil {
 		return nil, nil, err
 	}
-	return info, b.getBatch().reader, nil
+	return info, b.getBatch().respReader, nil
 }
 
 func (b *catFileBatchCommand) QueryInfo(obj string) (*CatFileObject, error) {
-	_, err := b.getBatch().writer.Write([]byte("info " + obj + "\n"))
+	_, err := b.getBatch().reqWriter.Write([]byte("info " + obj + "\n"))
 	if err != nil {
 		return nil, err
 	}
-	return catFileBatchParseInfoLine(b.getBatch().reader)
+	return catFileBatchParseInfoLine(b.getBatch().respReader)
 }
 
 func (b *catFileBatchCommand) Close() {

modules/git/catfile_batch_legacy.go

@@ -50,23 +50,23 @@ func (b *catFileBatchLegacy) getBatchCheck() *catFileBatchCommunicator {
 }
 
 func (b *catFileBatchLegacy) QueryContent(obj string) (*CatFileObject, BufferedReader, error) {
-	_, err := io.WriteString(b.getBatchContent().writer, obj+"\n")
+	_, err := io.WriteString(b.getBatchContent().reqWriter, obj+"\n")
 	if err != nil {
 		return nil, nil, err
 	}
-	info, err := catFileBatchParseInfoLine(b.getBatchContent().reader)
+	info, err := catFileBatchParseInfoLine(b.getBatchContent().respReader)
 	if err != nil {
 		return nil, nil, err
 	}
-	return info, b.getBatchContent().reader, nil
+	return info, b.getBatchContent().respReader, nil
 }
 
 func (b *catFileBatchLegacy) QueryInfo(obj string) (*CatFileObject, error) {
-	_, err := io.WriteString(b.getBatchCheck().writer, obj+"\n")
+	_, err := io.WriteString(b.getBatchCheck().reqWriter, obj+"\n")
 	if err != nil {
 		return nil, err
 	}
-	return catFileBatchParseInfoLine(b.getBatchCheck().reader)
+	return catFileBatchParseInfoLine(b.getBatchCheck().respReader)
 }
 
 func (b *catFileBatchLegacy) Close() {

modules/git/catfile_batch_reader.go

@@ -18,46 +18,40 @@ import (
 )
 
 type catFileBatchCommunicator struct {
-	cancel context.CancelFunc
-	reader *bufio.Reader
-	writer io.Writer
-
+	cancel      context.CancelFunc
+	reqWriter   io.Writer
+	respReader  *bufio.Reader
 	debugGitCmd *gitcmd.Command
 }
 
 func (b *catFileBatchCommunicator) Close() {
 	if b.cancel != nil {
 		b.cancel()
-		b.reader = nil
-		b.writer = nil
 		b.cancel = nil
 	}
 }
 
 // newCatFileBatch opens git cat-file --batch in the provided repo and returns a stdin pipe, a stdout reader and cancel function
-func newCatFileBatch(ctx context.Context, repoPath string, cmdCatFile *gitcmd.Command) *catFileBatchCommunicator {
-	// We often want to feed the commits in order into cat-file --batch, followed by their trees and subtrees as necessary.
+func newCatFileBatch(ctx context.Context, repoPath string, cmdCatFile *gitcmd.Command) (ret *catFileBatchCommunicator) {
 	ctx, ctxCancel := context.WithCancelCause(ctx)
 
-	var batchStdinWriter io.WriteCloser
-	var batchStdoutReader io.ReadCloser
-	cmdCatFile = cmdCatFile.
-		WithDir(repoPath).
-		WithStdinWriter(&batchStdinWriter).
-		WithStdoutReader(&batchStdoutReader)
+	// We often want to feed the commits in order into cat-file --batch, followed by their trees and subtrees as necessary.
+	stdinWriter, stdoutReader, pipeClose := cmdCatFile.MakeStdinStdoutPipe()
+	ret = &catFileBatchCommunicator{
+		debugGitCmd: cmdCatFile,
+		cancel:      func() { ctxCancel(nil) },
+		reqWriter:   stdinWriter,
+		respReader:  bufio.NewReaderSize(stdoutReader, 32*1024), // use a buffered reader for rich operations
+	}
 
-	err := cmdCatFile.StartWithStderr(ctx)
+	err := cmdCatFile.WithDir(repoPath).StartWithStderr(ctx)
 	if err != nil {
 		log.Error("Unable to start git command %v: %v", cmdCatFile.LogString(), err)
 		// ideally here it should return the error, but it would require refactoring all callers
 		// so just return a dummy communicator that does nothing, almost the same behavior as before, not bad
-		return &catFileBatchCommunicator{
-			writer: io.Discard,
-			reader: bufio.NewReader(bytes.NewReader(nil)),
-			cancel: func() {
-				ctxCancel(err)
-			},
-		}
+		ctxCancel(err)
+		pipeClose()
+		return ret
 	}
 
 	go func() {
@@ -66,19 +60,10 @@ func newCatFileBatch(ctx context.Context, repoPath string, cmdCatFile *gitcmd.Co
 			log.Error("cat-file --batch command failed in repo %s, error: %v", repoPath, err)
 		}
 		ctxCancel(err)
+		pipeClose()
 	}()
 
-	// use a buffered reader to read from the cat-file --batch (StringReader.ReadString)
-	batchReader := bufio.NewReaderSize(batchStdoutReader, 32*1024)
-
-	return &catFileBatchCommunicator{
-		writer: batchStdinWriter,
-		reader: batchReader,
-		cancel: func() {
-			ctxCancel(nil)
-		},
-		debugGitCmd: cmdCatFile,
-	}
+	return ret
 }
 
 // catFileBatchParseInfoLine reads the header line from cat-file --batch

modules/git/catfile_batch_test.go

@@ -66,10 +66,10 @@ func testCatFileBatch(t *testing.T) {
 		switch b := batch.(type) {
 		case *catFileBatchLegacy:
 			c = b.batchCheck
-			_, _ = c.writer.Write([]byte("in-complete-line-"))
+			_, _ = c.reqWriter.Write([]byte("in-complete-line-"))
 		case *catFileBatchCommand:
 			c = b.batch
-			_, _ = c.writer.Write([]byte("info"))
+			_, _ = c.reqWriter.Write([]byte("info"))
 		default:
 			t.FailNow()
 			return
@@ -78,8 +78,8 @@ func testCatFileBatch(t *testing.T) {
 		wg := sync.WaitGroup{}
 		wg.Go(func() {
 			buf := make([]byte, 100)
-			_, _ = c.reader.Read(buf)
-			n, errRead := c.reader.Read(buf)
+			_, _ = c.respReader.Read(buf)
+			n, errRead := c.respReader.Read(buf)
 			assert.Zero(t, n)
 			assert.ErrorIs(t, errRead, io.EOF) // the pipe is closed due to command being killed
 		})

modules/git/diff.go

@@ -78,7 +78,7 @@ func GetRepoRawDiffForFile(repo *Repository, startCommit, endCommit string, diff
 	}
 
 	return cmd.WithDir(repo.Path).
-		WithStdout(writer).
+		WithStdoutCopy(writer).
 		RunWithStderr(repo.Ctx)
 }
 
@@ -286,11 +286,10 @@ func GetAffectedFiles(repo *Repository, branchName, oldCommitID, newCommitID str
 	affectedFiles := make([]string, 0, 32)
 
 	// Run `git diff --name-only` to get the names of the changed files
-	var stdoutReader io.ReadCloser
-	err := gitcmd.NewCommand("diff", "--name-only").AddDynamicArguments(oldCommitID, newCommitID).
-		WithEnv(env).
-		WithDir(repo.Path).
-		WithStdoutReader(&stdoutReader).
+	cmd := gitcmd.NewCommand("diff", "--name-only").AddDynamicArguments(oldCommitID, newCommitID)
+	stdoutReader, stdoutReaderClose := cmd.MakeStdoutPipe()
+	defer stdoutReaderClose()
+	err := cmd.WithEnv(env).WithDir(repo.Path).
 		WithPipelineFunc(func(ctx gitcmd.Context) error {
 			// Now scan the output from the command
 			scanner := bufio.NewScanner(stdoutReader)

modules/git/git.go

@@ -21,7 +21,7 @@ import (
 	"github.com/hashicorp/go-version"
 )
 
-const RequiredVersion = "2.0.0" // the minimum Git version required
+const RequiredVersion = "2.6.0" // the minimum Git version required
 
 type Features struct {
 	gitVersion *version.Version