Refactor git command context & pipeline (#36406)

Less and simpler code, fewer bugs

services/cron/tasks_basic.go

@@ -54,7 +54,7 @@ func registerRepoHealthCheck() {
 			RunAtStart: false,
 			Schedule:   "@midnight",
 		},
-		Timeout: time.Duration(setting.Git.Timeout.Default) * time.Second,
+		Timeout: time.Duration(setting.Git.Timeout.GC) * time.Second,
 		Args:    []string{},
 	}, func(ctx context.Context, _ *user_model.User, config Config) error {
 		rhcConfig := config.(*RepoHealthCheckConfig)

services/gitdiff/gitdiff.go

@@ -16,7 +16,6 @@ import (
 	"path"
 	"sort"
 	"strings"
-	"time"
 
 	"code.gitea.io/gitea/models/db"
 	git_model "code.gitea.io/gitea/models/git"
@@ -1271,10 +1270,10 @@ func getDiffBasic(ctx context.Context, gitRepo *git.Repository, opts *DiffOption
 	}()
 
 	go func() {
-		if err := cmdDiff.WithTimeout(time.Duration(setting.Git.Timeout.Default) * time.Second).
+		if err := cmdDiff.
 			WithDir(repoPath).
 			WithStdout(writer).
-			RunWithStderr(cmdCtx); err != nil && !git.IsErrCanceledOrKilled(err) {
+			RunWithStderr(cmdCtx); err != nil && !gitcmd.IsErrorCanceledOrKilled(err) {
 			log.Error("error during GetDiff(git diff dir: %s): %v", repoPath, err)
 		}
 

services/pull/merge_prepare.go

@@ -206,16 +206,6 @@ func prepareTemporaryRepoForMerge(ctx *mergeContext) error {
 // getDiffTree returns a string containing all the files that were changed between headBranch and baseBranch
 // the filenames are escaped so as to fit the format required for .git/info/sparse-checkout
 func getDiffTree(ctx context.Context, repoPath, baseBranch, headBranch string, out io.Writer) error {
-	diffOutReader, diffOutWriter, err := os.Pipe()
-	if err != nil {
-		log.Error("Unable to create os.Pipe for %s", repoPath)
-		return err
-	}
-	defer func() {
-		_ = diffOutReader.Close()
-		_ = diffOutWriter.Close()
-	}()
-
 	scanNullTerminatedStrings := func(data []byte, atEOF bool) (advance int, token []byte, err error) {
 		if atEOF && len(data) == 0 {
 			return 0, nil, nil
@@ -229,27 +219,23 @@ func getDiffTree(ctx context.Context, repoPath, baseBranch, headBranch string, o
 		return 0, nil, nil
 	}
 
-	err = gitcmd.NewCommand("diff-tree", "--no-commit-id", "--name-only", "-r", "-r", "-z", "--root").
+	var diffOutReader io.ReadCloser
+	err := gitcmd.NewCommand("diff-tree", "--no-commit-id", "--name-only", "-r", "-r", "-z", "--root").
 		AddDynamicArguments(baseBranch, headBranch).
 		WithDir(repoPath).
-		WithStdout(diffOutWriter).
-		WithPipelineFunc(func(ctx context.Context, cancel context.CancelFunc) error {
-			// Close the writer end of the pipe to begin processing
-			_ = diffOutWriter.Close()
-			defer func() {
-				// Close the reader on return to terminate the git command if necessary
-				_ = diffOutReader.Close()
-			}()
-
+		WithStdoutReader(&diffOutReader).
+		WithPipelineFunc(func(ctx gitcmd.Context) error {
 			// Now scan the output from the command
 			scanner := bufio.NewScanner(diffOutReader)
 			scanner.Split(scanNullTerminatedStrings)
 			for scanner.Scan() {
-				filepath := scanner.Text()
+				treePath := scanner.Text()
 				// escape '*', '?', '[', spaces and '!' prefix
-				filepath = escapedSymbols.ReplaceAllString(filepath, `\$1`)
+				treePath = escapedSymbols.ReplaceAllString(treePath, `\$1`)
 				// no necessary to escape the first '#' symbol because the first symbol is '/'
-				fmt.Fprintf(out, "/%s\n", filepath)
+				if _, err := fmt.Fprintf(out, "/%s\n", treePath); err != nil {
+					return err
+				}
 			}
 			return scanner.Err()
 		}).

services/pull/patch.go

@@ -421,12 +421,7 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo *
 	err = cmdApply.
 		WithDir(tmpBasePath).
 		WithStderrReader(&stderrReader).
-		WithPipelineFunc(func(ctx context.Context, cancel context.CancelFunc) error {
-			defer func() {
-				// Close the reader on return to terminate the git command if necessary
-				_ = stderrReader.Close()
-			}()
-
+		WithPipelineFunc(func(ctx gitcmd.Context) error {
 			const prefix = "error: patch failed:"
 			const errorPrefix = "error: "
 			const threewayFailed = "Failed to perform three-way merge..."

services/pull/patch_unmerged.go

@@ -9,7 +9,6 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"os"
 	"strconv"
 	"strings"
 
@@ -60,25 +59,11 @@ func readUnmergedLsFileLines(ctx context.Context, tmpBasePath string, outputChan
 		close(outputChan)
 	}()
 
-	lsFilesReader, lsFilesWriter, err := os.Pipe()
-	if err != nil {
-		log.Error("Unable to open stderr pipe: %v", err)
-		outputChan <- &lsFileLine{err: fmt.Errorf("unable to open stderr pipe: %w", err)}
-		return
-	}
-	defer func() {
-		_ = lsFilesWriter.Close()
-		_ = lsFilesReader.Close()
-	}()
-
-	err = gitcmd.NewCommand("ls-files", "-u", "-z").
+	var lsFilesReader io.ReadCloser
+	err := gitcmd.NewCommand("ls-files", "-u", "-z").
 		WithDir(tmpBasePath).
-		WithStdout(lsFilesWriter).
-		WithPipelineFunc(func(_ context.Context, _ context.CancelFunc) error {
-			_ = lsFilesWriter.Close()
-			defer func() {
-				_ = lsFilesReader.Close()
-			}()
+		WithStdoutReader(&lsFilesReader).
+		WithPipelineFunc(func(_ gitcmd.Context) error {
 			bufferedReader := bufio.NewReader(lsFilesReader)
 
 			for {

services/pull/pull.go

@@ -8,7 +8,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"os"
 	"regexp"
 	"strings"
 	"time"
@@ -526,18 +525,11 @@ func checkIfPRContentChanged(ctx context.Context, pr *issues_model.PullRequest,
 	}
 
 	cmd := gitcmd.NewCommand("diff", "--name-only", "-z").AddDynamicArguments(newCommitID, oldCommitID, mergeBase)
-	stdoutReader, stdoutWriter, err := os.Pipe()
-	if err != nil {
-		return false, mergeBase, fmt.Errorf("unable to open pipe for to run diff: %w", err)
-	}
 
+	var stdoutReader io.ReadCloser
 	if err := cmd.WithDir(prCtx.tmpBasePath).
-		WithStdout(stdoutWriter).
-		WithPipelineFunc(func(ctx context.Context, cancel context.CancelFunc) error {
-			_ = stdoutWriter.Close()
-			defer func() {
-				_ = stdoutReader.Close()
-			}()
+		WithStdoutReader(&stdoutReader).
+		WithPipelineFunc(func(ctx gitcmd.Context) error {
 			return util.IsEmptyReader(stdoutReader)
 		}).
 		RunWithStderr(ctx); err != nil {

services/pull/review.go

@@ -9,7 +9,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"regexp"
 	"strings"
 
 	"code.gitea.io/gitea/models/db"
@@ -17,6 +16,7 @@ import (
 	repo_model "code.gitea.io/gitea/models/repo"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/git"
+	"code.gitea.io/gitea/modules/git/gitcmd"
 	"code.gitea.io/gitea/modules/gitrepo"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/optional"
@@ -26,7 +26,15 @@ import (
 	notify_service "code.gitea.io/gitea/services/notify"
 )
 
-var notEnoughLines = regexp.MustCompile(`fatal: file .* has only \d+ lines?`)
+func isErrBlameNotFoundOrNotEnoughLines(err error) bool {
+	stdErr, ok := gitcmd.ErrorAsStderr(err)
+	if !ok {
+		return false
+	}
+	notFound := strings.HasPrefix(stdErr, "fatal: no such path")
+	notEnoughLines := strings.HasPrefix(stdErr, "fatal: file ") && strings.Contains(stdErr, " has only ") && strings.Contains(stdErr, " lines?")
+	return notFound || notEnoughLines
+}
 
 // ErrDismissRequestOnClosedPR represents an error when an user tries to dismiss a review associated to a closed or merged PR.
 type ErrDismissRequestOnClosedPR struct{}
@@ -67,7 +75,7 @@ func lineBlame(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Re
 func checkInvalidation(ctx context.Context, c *issues_model.Comment, repo *repo_model.Repository, gitRepo *git.Repository, branch string) error {
 	// FIXME differentiate between previous and proposed line
 	commit, err := lineBlame(ctx, repo, gitRepo, branch, c.TreePath, uint(c.UnsignedLine()))
-	if err != nil && (strings.Contains(err.Error(), "fatal: no such path") || notEnoughLines.MatchString(err.Error())) {
+	if isErrBlameNotFoundOrNotEnoughLines(err) {
 		c.Invalidated = true
 		return issues_model.UpdateCommentInvalidate(ctx, c)
 	}
@@ -251,7 +259,7 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo
 			commit, err := lineBlame(ctx, pr.BaseRepo, gitRepo, head, treePath, uint(line))
 			if err == nil {
 				commitID = commit.ID.String()
-			} else if !(strings.Contains(err.Error(), "exit status 128 - fatal: no such path") || notEnoughLines.MatchString(err.Error())) {
+			} else if !isErrBlameNotFoundOrNotEnoughLines(err) {
 				return nil, fmt.Errorf("LineBlame[%s, %s, %s, %d]: %w", pr.GetGitHeadRefName(), gitRepo.Path, treePath, line, err)
 			}
 		}

services/repository/contributors_graph.go

@@ -8,7 +8,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"os"
+	"io"
 	"strconv"
 	"strings"
 	"sync"
@@ -117,24 +117,16 @@ func getExtendedCommitStats(repo *git.Repository, revision string /*, limit int
 	if err != nil {
 		return nil, err
 	}
-	stdoutReader, stdoutWriter, err := os.Pipe()
-	if err != nil {
-		return nil, err
-	}
-	defer func() {
-		_ = stdoutReader.Close()
-		_ = stdoutWriter.Close()
-	}()
 
 	gitCmd := gitcmd.NewCommand("log", "--shortstat", "--no-merges", "--pretty=format:---%n%aN%n%aE%n%as", "--reverse")
 	// AddOptionFormat("--max-count=%d", limit)
 	gitCmd.AddDynamicArguments(baseCommit.ID.String())
 
+	var stdoutReader io.ReadCloser
 	var extendedCommitStats []*ExtendedCommitStats
 	err = gitCmd.WithDir(repo.Path).
-		WithStdout(stdoutWriter).
-		WithPipelineFunc(func(ctx context.Context, cancel context.CancelFunc) error {
-			_ = stdoutWriter.Close()
+		WithStdoutReader(&stdoutReader).
+		WithPipelineFunc(func(ctx gitcmd.Context) error {
 			scanner := bufio.NewScanner(stdoutReader)
 
 			for scanner.Scan() {
@@ -186,7 +178,6 @@ func getExtendedCommitStats(repo *git.Repository, revision string /*, limit int
 				}
 				extendedCommitStats = append(extendedCommitStats, res)
 			}
-			_ = stdoutReader.Close()
 			return nil
 		}).
 		RunWithStderr(repo.Ctx)

services/repository/files/temp_repo.go

@@ -362,25 +362,15 @@ 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) {
-	stdoutReader, stdoutWriter, err := os.Pipe()
-	if err != nil {
-		return nil, fmt.Errorf("unable to open stdout pipe: %w", err)
-	}
-	defer func() {
-		_ = stdoutReader.Close()
-		_ = stdoutWriter.Close()
-	}()
 	var diff *gitdiff.Diff
-	err = gitcmd.NewCommand("diff-index", "--src-prefix=\\a/", "--dst-prefix=\\b/", "--cached", "-p", "HEAD").
+	var stdoutReader io.ReadCloser
+	err := gitcmd.NewCommand("diff-index", "--src-prefix=\\a/", "--dst-prefix=\\b/", "--cached", "-p", "HEAD").
 		WithTimeout(30 * time.Second).
 		WithDir(t.basePath).
-		WithStdout(stdoutWriter).
-		WithPipelineFunc(func(ctx context.Context, cancel context.CancelFunc) error {
-			_ = stdoutWriter.Close()
-			defer cancel()
+		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, "")
-			_ = stdoutReader.Close()
 			if diffErr != nil {
 				// if the diffErr is not nil, it will be returned as the error of "Run()"
 				return fmt.Errorf("ParsePatch: %w", diffErr)
@@ -388,7 +378,7 @@ func (t *TemporaryUploadRepository) DiffIndex(ctx context.Context) (*gitdiff.Dif
 			return nil
 		}).
 		RunWithStderr(ctx)
-	if err != nil && !git.IsErrCanceledOrKilled(err) {
+	if err != nil && !gitcmd.IsErrorCanceledOrKilled(err) {
 		return nil, fmt.Errorf("unable to run diff-index pipeline in temporary repo: %w", err)
 	}
 

services/repository/gitgraph/graph.go

@@ -6,8 +6,7 @@ package gitgraph
 import (
 	"bufio"
 	"bytes"
-	"context"
-	"os"
+	"io"
 
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/git/gitcmd"
@@ -44,20 +43,14 @@ func GetCommitGraph(r *git.Repository, page, maxAllowedColors int, hidePRRefs bo
 	}
 	graph := NewGraph()
 
-	stdoutReader, stdoutWriter, err := os.Pipe()
-	if err != nil {
-		return nil, err
-	}
 	commitsToSkip := setting.UI.GraphMaxCommitNum * (page - 1)
 
-	scanner := bufio.NewScanner(stdoutReader)
-
+	var stdoutReader io.ReadCloser
 	if err := graphCmd.
 		WithDir(r.Path).
-		WithStdout(stdoutWriter).
-		WithPipelineFunc(func(ctx context.Context, cancel context.CancelFunc) error {
-			_ = stdoutWriter.Close()
-			defer stdoutReader.Close()
+		WithStdoutReader(&stdoutReader).
+		WithPipelineFunc(func(ctx gitcmd.Context) error {
+			scanner := bufio.NewScanner(stdoutReader)
 			parser := &Parser{}
 			parser.firstInUse = -1
 			parser.maxAllowedColors = maxAllowedColors
@@ -89,8 +82,7 @@ func GetCommitGraph(r *git.Repository, page, maxAllowedColors int, hidePRRefs bo
 				line := scanner.Bytes()
 				if bytes.IndexByte(line, '*') >= 0 {
 					if err := parser.AddLineToGraph(graph, row, line); err != nil {
-						cancel()
-						return err
+						return ctx.CancelWithCause(err)
 					}
 					break
 				}
@@ -101,8 +93,7 @@ func GetCommitGraph(r *git.Repository, page, maxAllowedColors int, hidePRRefs bo
 				row++
 				line := scanner.Bytes()
 				if err := parser.AddLineToGraph(graph, row, line); err != nil {
-					cancel()
-					return err
+					return ctx.CancelWithCause(err)
 				}
 			}
 			return scanner.Err()