Refactor git command stdio pipe (#36422)

Most potential deadlock problems should have been fixed, and new code is
unlikely to cause new problems with the new design.

Also raise the minimum Git version required to 2.6.0 (released in 2015)

services/gitdiff/gitdiff.go

@@ -1263,21 +1263,14 @@ func getDiffBasic(ctx context.Context, gitRepo *git.Repository, opts *DiffOption
 	cmdCtx, cmdCancel := context.WithCancel(ctx)
 	defer cmdCancel()
 
-	reader, writer := io.Pipe()
-	defer func() {
-		_ = reader.Close()
-		_ = writer.Close()
-	}()
-
+	reader, readerClose := cmdDiff.MakeStdoutPipe()
+	defer readerClose()
 	go func() {
 		if err := cmdDiff.
 			WithDir(repoPath).
-			WithStdout(writer).
 			RunWithStderr(cmdCtx); err != nil && !gitcmd.IsErrorCanceledOrKilled(err) {
 			log.Error("error during GetDiff(git diff dir: %s): %v", repoPath, err)
 		}
-
-		_ = writer.Close()
 	}()
 
 	diff, err := ParsePatch(cmdCtx, opts.MaxLines, opts.MaxLineCharacters, opts.MaxFiles, reader, parsePatchSkipToFile)

services/migrations/gitea_uploader.go

@@ -896,7 +896,7 @@ func (g *GiteaLocalUploader) CreateReviews(ctx context.Context, reviews ...*base
 			comment.TreePath = util.PathJoinRel(comment.TreePath)
 
 			var patch string
-			reader, writer := io.Pipe()
+			reader, writer := io.Pipe() // FIXME: use os.Pipe to avoid deadlock
 			defer func() {
 				_ = reader.Close()
 				_ = writer.Close()

services/mirror/mirror_push.go

@@ -192,7 +192,9 @@ func pushAllLFSObjects(ctx context.Context, gitRepo *git.Repository, lfsClient l
 
 	pointerChan := make(chan lfs.PointerBlob)
 	errChan := make(chan error, 1)
-	go lfs.SearchPointerBlobs(ctx, gitRepo, pointerChan, errChan)
+	go func() {
+		errChan <- lfs.SearchPointerBlobs(ctx, gitRepo, pointerChan)
+	}()
 
 	uploadObjects := func(pointers []lfs.Pointer) error {
 		err := lfsClient.Upload(ctx, pointers, func(p lfs.Pointer, objectError error) (io.ReadCloser, error) {
@@ -242,13 +244,12 @@ func pushAllLFSObjects(ctx context.Context, gitRepo *git.Repository, lfsClient l
 		}
 	}
 
-	err, has := <-errChan
-	if has {
+	err := <-errChan
+	if err != nil {
 		log.Error("Error enumerating LFS objects for repository: %v", err)
-		return err
 	}
 
-	return nil
+	return err
 }
 
 func syncPushMirrorWithSyncOnCommit(ctx context.Context, repoID int64) {

services/pull/lfs.go

@@ -7,15 +7,19 @@ package pull
 import (
 	"bufio"
 	"context"
+	"errors"
 	"io"
 	"strconv"
-	"sync"
 
 	git_model "code.gitea.io/gitea/models/git"
 	issues_model "code.gitea.io/gitea/models/issues"
+	"code.gitea.io/gitea/modules/git/gitcmd"
 	"code.gitea.io/gitea/modules/git/pipeline"
 	"code.gitea.io/gitea/modules/lfs"
 	"code.gitea.io/gitea/modules/log"
+	"code.gitea.io/gitea/modules/util"
+
+	"golang.org/x/sync/errgroup"
 )
 
 // LFSPush pushes lfs objects referred to in new commits in the head repository from the base repository
@@ -26,81 +30,82 @@ func LFSPush(ctx context.Context, tmpBasePath, mergeHeadSHA, mergeBaseSHA string
 	// ensure only blobs and <=1k size then pass in to git cat-file --batch
 	// to read each sha and check each as a pointer
 	// Then if they are lfs -> add them to the baseRepo
-	revListReader, revListWriter := io.Pipe()
-	shasToCheckReader, shasToCheckWriter := io.Pipe()
-	catFileCheckReader, catFileCheckWriter := io.Pipe()
-	shasToBatchReader, shasToBatchWriter := io.Pipe()
-	catFileBatchReader, catFileBatchWriter := io.Pipe()
-	errChan := make(chan error, 1)
-	wg := sync.WaitGroup{}
-	wg.Add(6)
-	// Create the go-routines in reverse order.
+
+	cmd1RevList, cmd3BathCheck, cmd5BatchContent := gitcmd.NewCommand(), gitcmd.NewCommand(), gitcmd.NewCommand()
+	cmd1RevListOut, cmd1RevListClose := cmd1RevList.MakeStdoutPipe()
+	defer cmd1RevListClose()
+
+	cmd3BatchCheckIn, cmd3BatchCheckOut, cmd3BatchCheckClose := cmd3BathCheck.MakeStdinStdoutPipe()
+	defer cmd3BatchCheckClose()
+
+	cmd5BatchContentIn, cmd5BatchContentOut, cmd5BatchContentClose := cmd5BatchContent.MakeStdinStdoutPipe()
+	defer cmd5BatchContentClose()
+
+	// Create the go-routines in reverse order (update: the order is not needed any more, the pipes are properly prepared)
+	wg := &errgroup.Group{}
 
 	// 6. Take the output of cat-file --batch and check if each file in turn
 	// to see if they're pointers to files in the LFS store associated with
 	// the head repo and add them to the base repo if so
-	go createLFSMetaObjectsFromCatFileBatch(ctx, catFileBatchReader, &wg, pr)
+	wg.Go(func() error {
+		return createLFSMetaObjectsFromCatFileBatch(ctx, cmd5BatchContentOut, pr)
+	})
 
 	// 5. Take the shas of the blobs and batch read them
-	go pipeline.CatFileBatch(ctx, shasToBatchReader, catFileBatchWriter, &wg, tmpBasePath)
+	wg.Go(func() error {
+		return pipeline.CatFileBatch(ctx, cmd5BatchContent, tmpBasePath)
+	})
 
 	// 4. From the provided objects restrict to blobs <=1k
-	go pipeline.BlobsLessThan1024FromCatFileBatchCheck(catFileCheckReader, shasToBatchWriter, &wg)
+	wg.Go(func() error {
+		return pipeline.BlobsLessThan1024FromCatFileBatchCheck(cmd3BatchCheckOut, cmd5BatchContentIn)
+	})
 
 	// 3. Run batch-check on the objects retrieved from rev-list
-	go pipeline.CatFileBatchCheck(ctx, shasToCheckReader, catFileCheckWriter, &wg, tmpBasePath)
+	wg.Go(func() error {
+		return pipeline.CatFileBatchCheck(ctx, cmd3BathCheck, tmpBasePath)
+	})
 
 	// 2. Check each object retrieved rejecting those without names as they will be commits or trees
-	go pipeline.BlobsFromRevListObjects(revListReader, shasToCheckWriter, &wg)
+	wg.Go(func() error {
+		return pipeline.BlobsFromRevListObjects(cmd1RevListOut, cmd3BatchCheckIn)
+	})
 
 	// 1. Run rev-list objects from mergeHead to mergeBase
-	go pipeline.RevListObjects(ctx, revListWriter, &wg, tmpBasePath, mergeHeadSHA, mergeBaseSHA, errChan)
+	wg.Go(func() error {
+		return pipeline.RevListObjects(ctx, cmd1RevList, tmpBasePath, mergeHeadSHA, mergeBaseSHA)
+	})
 
-	wg.Wait()
-	select {
-	case err, has := <-errChan:
-		if has {
-			return err
-		}
-	default:
-	}
-	return nil
+	return wg.Wait()
 }
 
-func createLFSMetaObjectsFromCatFileBatch(ctx context.Context, catFileBatchReader *io.PipeReader, wg *sync.WaitGroup, pr *issues_model.PullRequest) {
-	defer wg.Done()
+func createLFSMetaObjectsFromCatFileBatch(ctx context.Context, catFileBatchReader io.ReadCloser, pr *issues_model.PullRequest) error {
 	defer catFileBatchReader.Close()
 
 	contentStore := lfs.NewContentStore()
-
 	bufferedReader := bufio.NewReader(catFileBatchReader)
 	buf := make([]byte, 1025)
 	for {
 		// File descriptor line: sha
 		_, err := bufferedReader.ReadString(' ')
 		if err != nil {
-			_ = catFileBatchReader.CloseWithError(err)
-			break
+			return util.Iif(errors.Is(err, io.EOF), nil, err)
 		}
 		// Throw away the blob
 		if _, err := bufferedReader.ReadString(' '); err != nil {
-			_ = catFileBatchReader.CloseWithError(err)
-			break
+			return err
 		}
 		sizeStr, err := bufferedReader.ReadString('\n')
 		if err != nil {
-			_ = catFileBatchReader.CloseWithError(err)
-			break
+			return err
 		}
 		size, err := strconv.Atoi(sizeStr[:len(sizeStr)-1])
 		if err != nil {
-			_ = catFileBatchReader.CloseWithError(err)
-			break
+			return err
 		}
 		pointerBuf := buf[:size+1]
 		if _, err := io.ReadFull(bufferedReader, pointerBuf); err != nil {
-			_ = catFileBatchReader.CloseWithError(err)
-			break
+			return err
 		}
 		pointerBuf = pointerBuf[:size]
 		// Now we need to check if the pointerBuf is an LFS pointer
@@ -120,15 +125,13 @@ func createLFSMetaObjectsFromCatFileBatch(ctx context.Context, catFileBatchReade
 				log.Warn("During merge of: %d in %-v, there is a pointer to LFS Oid: %s which although present in the LFS store is not associated with the head repo %-v", pr.Index, pr.BaseRepo, pointer.Oid, pr.HeadRepo)
 				continue
 			}
-			_ = catFileBatchReader.CloseWithError(err)
-			break
+			return err
 		}
 		// OK we have a pointer that is associated with the head repo
 		// and is actually a file in the LFS
 		// Therefore it should be associated with the base repo
 		if _, err := git_model.NewLFSMetaObject(ctx, pr.BaseRepoID, pointer); err != nil {
-			_ = catFileBatchReader.CloseWithError(err)
-			break
+			return err
 		}
 	}
 }

services/pull/merge_prepare.go

@@ -41,7 +41,7 @@ func (ctx *mergeContext) PrepareGitCmd(cmd *gitcmd.Command) *gitcmd.Command {
 	return cmd.WithEnv(ctx.env).
 		WithDir(ctx.tmpBasePath).
 		WithParentCallerInfo().
-		WithStdout(ctx.outbuf)
+		WithStdoutBuffer(ctx.outbuf)
 }
 
 // ErrSHADoesNotMatch represents a "SHADoesNotMatch" kind of error.
@@ -219,11 +219,11 @@ func getDiffTree(ctx context.Context, repoPath, baseBranch, headBranch string, o
 		return 0, nil, nil
 	}
 
-	var diffOutReader io.ReadCloser
-	err := gitcmd.NewCommand("diff-tree", "--no-commit-id", "--name-only", "-r", "-r", "-z", "--root").
-		AddDynamicArguments(baseBranch, headBranch).
+	cmd := gitcmd.NewCommand("diff-tree", "--no-commit-id", "--name-only", "-r", "-r", "-z", "--root")
+	diffOutReader, diffOutReaderClose := cmd.MakeStdoutPipe()
+	defer diffOutReaderClose()
+	err := cmd.AddDynamicArguments(baseBranch, headBranch).
 		WithDir(repoPath).
-		WithStdoutReader(&diffOutReader).
 		WithPipelineFunc(func(ctx gitcmd.Context) error {
 			// Now scan the output from the command
 			scanner := bufio.NewScanner(diffOutReader)

services/pull/patch.go

@@ -414,13 +414,13 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo *
 	//   - alternatively we can do the equivalent of:
 	//  `git apply --check ... | grep ...`
 	//     meaning we don't store all the conflicts unnecessarily.
-	var stderrReader io.ReadCloser
+	stderrReader, stderrReaderClose := cmdApply.MakeStderrPipe()
+	defer stderrReaderClose()
 
 	// 8. Run the check command
 	conflict = false
 	err = cmdApply.
 		WithDir(tmpBasePath).
-		WithStderrReader(&stderrReader).
 		WithPipelineFunc(func(ctx gitcmd.Context) error {
 			const prefix = "error: patch failed:"
 			const errorPrefix = "error: "

services/pull/patch_unmerged.go

@@ -59,10 +59,10 @@ func readUnmergedLsFileLines(ctx context.Context, tmpBasePath string, outputChan
 		close(outputChan)
 	}()
 
-	var lsFilesReader io.ReadCloser
-	err := gitcmd.NewCommand("ls-files", "-u", "-z").
-		WithDir(tmpBasePath).
-		WithStdoutReader(&lsFilesReader).
+	cmd := gitcmd.NewCommand("ls-files", "-u", "-z")
+	lsFilesReader, lsFilesReaderClose := cmd.MakeStdoutPipe()
+	defer lsFilesReaderClose()
+	err := cmd.WithDir(tmpBasePath).
 		WithPipelineFunc(func(_ gitcmd.Context) error {
 			bufferedReader := bufio.NewReader(lsFilesReader)
 

services/pull/pull.go

@@ -526,9 +526,9 @@ func checkIfPRContentChanged(ctx context.Context, pr *issues_model.PullRequest,
 
 	cmd := gitcmd.NewCommand("diff", "--name-only", "-z").AddDynamicArguments(newCommitID, oldCommitID, mergeBase)
 
-	var stdoutReader io.ReadCloser
+	stdoutReader, stdoutReaderClose := cmd.MakeStdoutPipe()
+	defer stdoutReaderClose()
 	if err := cmd.WithDir(prCtx.tmpBasePath).
-		WithStdoutReader(&stdoutReader).
 		WithPipelineFunc(func(ctx gitcmd.Context) error {
 			return util.IsEmptyReader(stdoutReader)
 		}).

services/pull/review.go

@@ -274,7 +274,7 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo
 		if len(commitID) == 0 {
 			commitID = headCommitID
 		}
-		reader, writer := io.Pipe()
+		reader, writer := io.Pipe() // FIXME: use os.Pipe to avoid deadlock
 		defer func() {
 			_ = reader.Close()
 			_ = writer.Close()

services/pull/temp_repo.go

@@ -5,11 +5,11 @@
 package pull
 
 import (
+	"bytes"
 	"context"
 	"fmt"
 	"os"
 	"path/filepath"
-	"strings"
 
 	git_model "code.gitea.io/gitea/models/git"
 	issues_model "code.gitea.io/gitea/models/issues"
@@ -32,7 +32,7 @@ type prTmpRepoContext struct {
 	context.Context
 	tmpBasePath string
 	pr          *issues_model.PullRequest
-	outbuf      *strings.Builder // we keep these around to help reduce needless buffer recreation, any use should be preceded by a Reset and preferably after use
+	outbuf      *bytes.Buffer // we keep these around to help reduce needless buffer recreation, any use should be preceded by a Reset and preferably after use
 }
 
 // PrepareGitCmd prepares a git command with the correct directory, environment, and output buffers
@@ -40,7 +40,7 @@ type prTmpRepoContext struct {
 // Do NOT use it with gitcmd.RunStd*() functions, otherwise it will panic
 func (ctx *prTmpRepoContext) PrepareGitCmd(cmd *gitcmd.Command) *gitcmd.Command {
 	ctx.outbuf.Reset()
-	return cmd.WithDir(ctx.tmpBasePath).WithStdout(ctx.outbuf)
+	return cmd.WithDir(ctx.tmpBasePath).WithStdoutBuffer(ctx.outbuf)
 }
 
 // createTemporaryRepoForPR creates a temporary repo with "base" for pr.BaseBranch and "tracking" for  pr.HeadBranch
@@ -82,7 +82,7 @@ func createTemporaryRepoForPR(ctx context.Context, pr *issues_model.PullRequest)
 		Context:     ctx,
 		tmpBasePath: tmpBasePath,
 		pr:          pr,
-		outbuf:      &strings.Builder{},
+		outbuf:      &bytes.Buffer{},
 	}
 
 	baseRepoPath := pr.BaseRepo.RepoPath()

services/pull/update_rebase.go

@@ -82,7 +82,7 @@ func updateHeadByRebaseOnToBase(ctx context.Context, pr *issues_model.PullReques
 			pr.Index,
 		)).
 		WithDir(mergeCtx.tmpBasePath).
-		WithStdout(mergeCtx.outbuf).
+		WithStdoutBuffer(mergeCtx.outbuf).
 		RunWithStderr(ctx); err != nil {
 		if strings.Contains(err.Stderr(), "non-fast-forward") {
 			return &git.ErrPushOutOfDate{

services/repository/branch.go

@@ -264,12 +264,12 @@ func checkBranchName(ctx context.Context, repo *repo_model.Repository, name stri
 			return git_model.ErrBranchAlreadyExists{
 				BranchName: name,
 			}
-		// If branchRefName like a/b but we want to create a branch named a then we have a conflict
+		// If branchRefName like "a/b" but we want to create a branch named a then we have a conflict
 		case strings.HasPrefix(branchRefName, name+"/"):
 			return git_model.ErrBranchNameConflict{
 				BranchName: branchRefName,
 			}
-			// Conversely if branchRefName like a but we want to create a branch named a/b then we also have a conflict
+			// Conversely if branchRefName like "a" but we want to create a branch named "a/b" then we also have a conflict
 		case strings.HasPrefix(name, branchRefName+"/"):
 			return git_model.ErrBranchNameConflict{
 				BranchName: branchRefName,
@@ -281,7 +281,6 @@ func checkBranchName(ctx context.Context, repo *repo_model.Repository, name stri
 		}
 		return nil
 	})
-
 	return err
 }
 

services/repository/contributors_graph.go

@@ -8,7 +8,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"io"
 	"strconv"
 	"strings"
 	"sync"
@@ -122,10 +121,11 @@ func getExtendedCommitStats(repo *git.Repository, revision string /*, limit int
 	// AddOptionFormat("--max-count=%d", limit)
 	gitCmd.AddDynamicArguments(baseCommit.ID.String())
 
-	var stdoutReader io.ReadCloser
+	stdoutReader, stdoutReaderClose := gitCmd.MakeStdoutPipe()
+	defer stdoutReaderClose()
+
 	var extendedCommitStats []*ExtendedCommitStats
 	err = gitCmd.WithDir(repo.Path).
-		WithStdoutReader(&stdoutReader).
 		WithPipelineFunc(func(ctx gitcmd.Context) error {
 			scanner := bufio.NewScanner(stdoutReader)
 

services/repository/files/patch.go

@@ -170,7 +170,7 @@ func ApplyDiffPatch(ctx context.Context, repo *repo_model.Repository, doer *user
 	}
 
 	if err := cmdApply.WithDir(t.basePath).
-		WithStdin(strings.NewReader(opts.Content)).
+		WithStdinBytes([]byte(opts.Content)).
 		RunWithStderr(ctx); err != nil {
 		return nil, fmt.Errorf("git apply error: %w", err)
 	}

services/repository/files/temp_repo.go

@@ -117,7 +117,7 @@ func (t *TemporaryUploadRepository) LsFiles(ctx context.Context, filenames ...st
 	stdOut := new(bytes.Buffer)
 	if err := gitcmd.NewCommand("ls-files", "-z").AddDashesAndList(filenames...).
 		WithDir(t.basePath).
-		WithStdout(stdOut).
+		WithStdoutBuffer(stdOut).
 		RunWithStderr(ctx); err != nil {
 		return nil, fmt.Errorf("unable to run git ls-files for temporary repo of: %s, error: %w", t.repo.FullName(), err)
 	}
@@ -155,7 +155,7 @@ func (t *TemporaryUploadRepository) RemoveFilesFromIndex(ctx context.Context, fi
 
 	if err := gitcmd.NewCommand("update-index", "--remove", "-z", "--index-info").
 		WithDir(t.basePath).
-		WithStdin(stdIn).
+		WithStdinBytes(stdIn.Bytes()).
 		RunWithStderr(ctx); err != nil {
 		return fmt.Errorf("unable to update-index for temporary repo: %q, error: %w", t.repo.FullName(), err)
 	}
@@ -167,8 +167,8 @@ func (t *TemporaryUploadRepository) HashObjectAndWrite(ctx context.Context, cont
 	stdOut := new(bytes.Buffer)
 	if err := gitcmd.NewCommand("hash-object", "-w", "--stdin").
 		WithDir(t.basePath).
-		WithStdout(stdOut).
-		WithStdin(content).
+		WithStdoutBuffer(stdOut).
+		WithStdinCopy(content).
 		RunWithStderr(ctx); err != nil {
 		return "", fmt.Errorf("unable to hash-object to temporary repo: %s, error: %w", t.repo.FullName(), err)
 	}
@@ -330,8 +330,8 @@ func (t *TemporaryUploadRepository) CommitTree(ctx context.Context, opts *Commit
 	if err := cmdCommitTree.
 		WithEnv(env).
 		WithDir(t.basePath).
-		WithStdout(stdout).
-		WithStdin(messageBytes).
+		WithStdoutBuffer(stdout).
+		WithStdinBytes(messageBytes.Bytes()).
 		RunWithStderr(ctx); err != nil {
 		return "", fmt.Errorf("unable to commit-tree in temporary repo: %s Error: %w", t.repo.FullName(), err)
 	}
@@ -363,11 +363,12 @@ func (t *TemporaryUploadRepository) Push(ctx context.Context, doer *user_model.U
 // DiffIndex returns a Diff of the current index to the head
 func (t *TemporaryUploadRepository) DiffIndex(ctx context.Context) (*gitdiff.Diff, error) {
 	var diff *gitdiff.Diff
-	var stdoutReader io.ReadCloser
-	err := gitcmd.NewCommand("diff-index", "--src-prefix=\\a/", "--dst-prefix=\\b/", "--cached", "-p", "HEAD").
-		WithTimeout(30 * time.Second).
+	cmd := gitcmd.NewCommand("diff-index", "--src-prefix=\\a/", "--dst-prefix=\\b/", "--cached", "-p", "HEAD")
+	stdoutReader, stdoutReaderClose := cmd.MakeStdoutPipe()
+	defer stdoutReaderClose()
+
+	err := cmd.WithTimeout(30 * time.Second).
 		WithDir(t.basePath).
-		WithStdoutReader(&stdoutReader).
 		WithPipelineFunc(func(ctx gitcmd.Context) error {
 			var diffErr error
 			diff, diffErr = gitdiff.ParsePatch(ctx, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, stdoutReader, "")

services/repository/gitgraph/graph.go

@@ -6,7 +6,6 @@ package gitgraph
 import (
 	"bufio"
 	"bytes"
-	"io"
 
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/git/gitcmd"
@@ -45,10 +44,10 @@ func GetCommitGraph(r *git.Repository, page, maxAllowedColors int, hidePRRefs bo
 
 	commitsToSkip := setting.UI.GraphMaxCommitNum * (page - 1)
 
-	var stdoutReader io.ReadCloser
+	stdoutReader, stdoutReaderClose := graphCmd.MakeStdoutPipe()
+	defer stdoutReaderClose()
 	if err := graphCmd.
 		WithDir(r.Path).
-		WithStdoutReader(&stdoutReader).
 		WithPipelineFunc(func(ctx gitcmd.Context) error {
 			scanner := bufio.NewScanner(stdoutReader)
 			parser := &Parser{}

services/wiki/wiki.go

@@ -170,7 +170,7 @@ func updateWikiPage(ctx context.Context, doer *user_model.User, repo *repo_model
 
 	// FIXME: The wiki doesn't have lfs support at present - if this changes need to check attributes here
 
-	objectHash, err := gitRepo.HashObject(strings.NewReader(content))
+	objectHash, err := gitRepo.HashObjectBytes([]byte(content))
 	if err != nil {
 		log.Error("HashObject failed: %v", err)
 		return err