Merge remote-tracking branch 'upstream/main' into limit-repo-size

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