Refactor git command stdio pipe (#36393)

And remove the incorrect `ensureValidGitRepository`

Author: wxiaoguang

modules/git/catfile_batch_command.go

@@ -5,8 +5,11 @@ package git
 
 import (
 	"context"
+	"os"
+	"path/filepath"
 
 	"code.gitea.io/gitea/modules/git/gitcmd"
+	"code.gitea.io/gitea/modules/util"
 )
 
 // catFileBatchCommand implements the CatFileBatch interface using the "cat-file --batch-command" command
@@ -21,8 +24,8 @@ type catFileBatchCommand struct {
 var _ CatFileBatch = (*catFileBatchCommand)(nil)
 
 func newCatFileBatchCommand(ctx context.Context, repoPath string) (*catFileBatchCommand, error) {
-	if err := ensureValidGitRepository(ctx, repoPath); err != nil {
-		return nil, err
+	if _, err := os.Stat(repoPath); err != nil {
+		return nil, util.NewNotExistErrorf("repo %q doesn't exist", filepath.Base(repoPath))
 	}
 	return &catFileBatchCommand{ctx: ctx, repoPath: repoPath}, nil
 }

modules/git/catfile_batch_legacy.go

@@ -6,8 +6,11 @@ package git
 import (
 	"context"
 	"io"
+	"os"
+	"path/filepath"
 
 	"code.gitea.io/gitea/modules/git/gitcmd"
+	"code.gitea.io/gitea/modules/util"
 )
 
 // catFileBatchLegacy implements the CatFileBatch interface using the "cat-file --batch" command and "cat-file --batch-check" command
@@ -24,8 +27,8 @@ type catFileBatchLegacy struct {
 var _ CatFileBatchCloser = (*catFileBatchLegacy)(nil)
 
 func newCatFileBatchLegacy(ctx context.Context, repoPath string) (*catFileBatchLegacy, error) {
-	if err := ensureValidGitRepository(ctx, repoPath); err != nil {
-		return nil, err
+	if _, err := os.Stat(repoPath); err != nil {
+		return nil, util.NewNotExistErrorf("repo %q doesn't exist", filepath.Base(repoPath))
 	}
 	return &catFileBatchLegacy{ctx: ctx, repoPath: repoPath}, nil
 }

modules/git/catfile_batch_reader.go

@@ -16,16 +16,12 @@ import (
 	"code.gitea.io/gitea/modules/log"
 )
 
-// writeCloserError wraps an io.WriteCloser with an additional CloseWithError function (for nio.Pipe)
-type writeCloserError interface {
-	io.WriteCloser
-	CloseWithError(err error) error
-}
-
 type catFileBatchCommunicator struct {
 	cancel context.CancelFunc
 	reader *bufio.Reader
-	writer writeCloserError
+	writer io.Writer
+
+	debugGitCmd *gitcmd.Command
 }
 
 func (b *catFileBatchCommunicator) Close() {
@@ -37,63 +33,41 @@ func (b *catFileBatchCommunicator) Close() {
 	}
 }
 
-// ensureValidGitRepository runs git rev-parse in the repository path - thus ensuring that the repository is a valid repository.
-// Run before opening git cat-file.
-// This is needed otherwise the git cat-file will hang for invalid repositories.
-// FIXME: the comment is from https://github.com/go-gitea/gitea/pull/17991 but it doesn't seem to be true.
-// The real problem is that Golang's Cmd.Wait hangs because it waits for the pipes to be closed, but we can't close the pipes before Wait returns
-// Need to refactor to use StdinPipe and StdoutPipe
-func ensureValidGitRepository(ctx context.Context, repoPath string) error {
-	stderr := strings.Builder{}
-	err := gitcmd.NewCommand("rev-parse").
-		WithDir(repoPath).
-		WithStderr(&stderr).
-		Run(ctx)
-	if err != nil {
-		return gitcmd.ConcatenateError(err, (&stderr).String())
-	}
-	return 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.
+	ctx, ctxCancel := context.WithCancelCause(ctx)
 
-	// so let's create a batch stdin and stdout
-	batchStdinReader, batchStdinWriter := io.Pipe()
-	batchStdoutReader, batchStdoutWriter := io.Pipe()
-	ctx, ctxCancel := context.WithCancel(ctx)
-	closed := make(chan struct{})
-	cancel := func() {
-		ctxCancel()
-		_ = batchStdinWriter.Close()
-		_ = batchStdoutReader.Close()
-		<-closed
-	}
+	var batchStdinWriter io.WriteCloser
+	var batchStdoutReader io.ReadCloser
+	stderr := strings.Builder{}
+	cmdCatFile = cmdCatFile.
+		WithDir(repoPath).
+		WithStdinWriter(&batchStdinWriter).
+		WithStdoutReader(&batchStdoutReader).
+		WithStderr(&stderr).
+		WithUseContextTimeout(true)
 
-	// Ensure cancel is called as soon as the provided context is cancelled
-	go func() {
-		<-ctx.Done()
-		cancel()
-	}()
+	err := cmdCatFile.Start(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)
+			},
+		}
+	}
 
 	go func() {
-		stderr := strings.Builder{}
-		err := cmdCatFile.
-			WithDir(repoPath).
-			WithStdin(batchStdinReader).
-			WithStdout(batchStdoutWriter).
-			WithStderr(&stderr).
-			WithUseContextTimeout(true).
-			Run(ctx)
+		err := cmdCatFile.Wait()
 		if err != nil {
-			_ = batchStdoutWriter.CloseWithError(gitcmd.ConcatenateError(err, (&stderr).String()))
-			_ = batchStdinReader.CloseWithError(gitcmd.ConcatenateError(err, (&stderr).String()))
-		} else {
-			_ = batchStdoutWriter.Close()
-			_ = batchStdinReader.Close()
+			log.Error("cat-file --batch command failed in repo %s: %v - stderr: %s", repoPath, err, stderr.String())
 		}
-		close(closed)
+		ctxCancel(err)
 	}()
 
 	// use a buffered reader to read from the cat-file --batch (StringReader.ReadString)
@@ -102,7 +76,10 @@ func newCatFileBatch(ctx context.Context, repoPath string, cmdCatFile *gitcmd.Co
 	return &catFileBatchCommunicator{
 		writer: batchStdinWriter,
 		reader: batchReader,
-		cancel: cancel,
+		cancel: func() {
+			ctxCancel(nil)
+		},
+		debugGitCmd: cmdCatFile,
 	}
 }
 

modules/git/catfile_batch_test.go

@@ -6,6 +6,7 @@ package git
 import (
 	"io"
 	"path/filepath"
+	"sync"
 	"testing"
 
 	"code.gitea.io/gitea/modules/test"
@@ -25,7 +26,14 @@ func TestCatFileBatch(t *testing.T) {
 func testCatFileBatch(t *testing.T) {
 	t.Run("CorruptedGitRepo", func(t *testing.T) {
 		tmpDir := t.TempDir()
-		_, err := NewBatch(t.Context(), tmpDir)
+		batch, err := NewBatch(t.Context(), tmpDir)
+		// as long as the directory exists, no error, because we can't really know whether the git repo is valid until we run commands
+		require.NoError(t, err)
+		defer batch.Close()
+
+		_, err = batch.QueryInfo("e2129701f1a4d54dc44f03c93bca0a2aec7c5449")
+		require.Error(t, err)
+		_, err = batch.QueryInfo("e2129701f1a4d54dc44f03c93bca0a2aec7c5449")
 		require.Error(t, err)
 	})
 
@@ -52,4 +60,30 @@ func testCatFileBatch(t *testing.T) {
 		require.NoError(t, err)
 		require.Equal(t, "file1\n", string(content))
 	})
+
+	t.Run("QueryTerminated", func(t *testing.T) {
+		var c *catFileBatchCommunicator
+		switch b := batch.(type) {
+		case *catFileBatchLegacy:
+			c = b.batchCheck
+			_, _ = c.writer.Write([]byte("in-complete-line-"))
+		case *catFileBatchCommand:
+			c = b.batch
+			_, _ = c.writer.Write([]byte("info"))
+		default:
+			t.FailNow()
+			return
+		}
+
+		wg := sync.WaitGroup{}
+		wg.Go(func() {
+			buf := make([]byte, 100)
+			_, _ = c.reader.Read(buf)
+			n, errRead := c.reader.Read(buf)
+			assert.Zero(t, n)
+			assert.ErrorIs(t, errRead, io.EOF) // the pipe is closed due to command being killed
+		})
+		c.debugGitCmd.DebugKill()
+		wg.Wait()
+	})
 }