From 9ea91e036fc62d7558453e5da9e652ce3f6e1af7 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 21 Jan 2026 09:35:14 +0800 Subject: [PATCH] Refactor git command context & pipeline (#36406) Less and simpler code, fewer bugs --- custom/conf/app.example.ini | 3 - modules/git/attribute/batch.go | 2 +- modules/git/catfile_batch_reader.go | 3 +- modules/git/diff.go | 24 +-- modules/git/error.go | 9 -- modules/git/git.go | 5 - modules/git/gitcmd/command.go | 172 +++++++--------------- modules/git/gitcmd/command_race_test.go | 38 ----- modules/git/gitcmd/command_test.go | 14 ++ modules/git/gitcmd/context.go | 28 ++++ modules/git/gitcmd/error.go | 78 ++++++++++ modules/git/grep.go | 7 +- modules/git/remote.go | 6 +- modules/git/repo_stats.go | 18 +-- modules/git/submodule.go | 18 +-- modules/gitrepo/blame.go | 2 +- modules/log/init.go | 6 +- modules/nosql/manager.go | 2 +- modules/process/manager.go | 51 ++++--- modules/process/process.go | 3 +- modules/queue/workerqueue.go | 2 +- modules/setting/git.go | 9 -- options/locale/locale_en-US.json | 2 - routers/private/hook_verification.go | 49 ++---- routers/web/repo/githttp.go | 6 +- services/cron/tasks_basic.go | 2 +- services/gitdiff/gitdiff.go | 5 +- services/pull/merge_prepare.go | 32 ++-- services/pull/patch.go | 7 +- services/pull/patch_unmerged.go | 23 +-- services/pull/pull.go | 14 +- services/pull/review.go | 16 +- services/repository/contributors_graph.go | 17 +-- services/repository/files/temp_repo.go | 20 +-- services/repository/gitgraph/graph.go | 23 +-- templates/admin/config.tmpl | 4 - 36 files changed, 286 insertions(+), 434 deletions(-) delete mode 100644 modules/git/gitcmd/command_race_test.go create mode 100644 modules/git/gitcmd/context.go create mode 100644 modules/git/gitcmd/error.go diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index 40c066c2b1..a1228f8dbf 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -737,11 +737,8 @@ LEVEL = Info ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; Git Operation timeout in seconds ;[git.timeout] -;DEFAULT = 360 ;MIGRATE = 600 ;MIRROR = 300 -;CLONE = 300 -;PULL = 300 ;GC = 60 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; diff --git a/modules/git/attribute/batch.go b/modules/git/attribute/batch.go index 6038815d82..184d9027ee 100644 --- a/modules/git/attribute/batch.go +++ b/modules/git/attribute/batch.go @@ -82,7 +82,7 @@ func NewBatchChecker(repo *git.Repository, treeish string, attributes []string) WithStdout(lw). RunWithStderr(ctx) - if err != nil && !git.IsErrCanceledOrKilled(err) { + if err != nil && !gitcmd.IsErrorCanceledOrKilled(err) { log.Error("Attribute checker for commit %s exits with error: %v", treeish, err) } checker.cancel() diff --git a/modules/git/catfile_batch_reader.go b/modules/git/catfile_batch_reader.go index aede3cf422..9039edcd20 100644 --- a/modules/git/catfile_batch_reader.go +++ b/modules/git/catfile_batch_reader.go @@ -44,8 +44,7 @@ func newCatFileBatch(ctx context.Context, repoPath string, cmdCatFile *gitcmd.Co cmdCatFile = cmdCatFile. WithDir(repoPath). WithStdinWriter(&batchStdinWriter). - WithStdoutReader(&batchStdoutReader). - WithUseContextTimeout(true) + WithStdoutReader(&batchStdoutReader) err := cmdCatFile.StartWithStderr(ctx) if err != nil { diff --git a/modules/git/diff.go b/modules/git/diff.go index 6fa5e6f2bc..e4229c07ee 100644 --- a/modules/git/diff.go +++ b/modules/git/diff.go @@ -5,10 +5,8 @@ package git import ( "bufio" - "context" "fmt" "io" - "os" "regexp" "strconv" "strings" @@ -284,30 +282,16 @@ func GetAffectedFiles(repo *Repository, branchName, oldCommitID, newCommitID str } oldCommitID = startCommitID } - stdoutReader, stdoutWriter, err := os.Pipe() - if err != nil { - log.Error("Unable to create os.Pipe for %s", repo.Path) - return nil, err - } - defer func() { - _ = stdoutReader.Close() - _ = stdoutWriter.Close() - }() affectedFiles := make([]string, 0, 32) // Run `git diff --name-only` to get the names of the changed files - err = gitcmd.NewCommand("diff", "--name-only").AddDynamicArguments(oldCommitID, newCommitID). + var stdoutReader io.ReadCloser + err := gitcmd.NewCommand("diff", "--name-only").AddDynamicArguments(oldCommitID, newCommitID). WithEnv(env). WithDir(repo.Path). - WithStdout(stdoutWriter). - WithPipelineFunc(func(ctx context.Context, cancel context.CancelFunc) error { - // Close the writer end of the pipe to begin processing - _ = stdoutWriter.Close() - defer func() { - // Close the reader on return to terminate the git command if necessary - _ = stdoutReader.Close() - }() + WithStdoutReader(&stdoutReader). + WithPipelineFunc(func(ctx gitcmd.Context) error { // Now scan the output from the command scanner := bufio.NewScanner(stdoutReader) for scanner.Scan() { diff --git a/modules/git/error.go b/modules/git/error.go index d4b5412da9..1b7bdca043 100644 --- a/modules/git/error.go +++ b/modules/git/error.go @@ -4,8 +4,6 @@ package git import ( - "context" - "errors" "fmt" "strings" @@ -143,10 +141,3 @@ func IsErrMoreThanOne(err error) bool { func (err *ErrMoreThanOne) Error() string { return fmt.Sprintf("ErrMoreThanOne Error: %v: %s\n%s", err.Err, err.StdErr, err.StdOut) } - -func IsErrCanceledOrKilled(err error) bool { - // When "cancel()" a git command's context, the returned error of "Run()" could be one of them: - // - context.Canceled - // - *exec.ExitError: "signal: killed" - return err != nil && (errors.Is(err, context.Canceled) || err.Error() == "signal: killed") -} diff --git a/modules/git/git.go b/modules/git/git.go index 6b93ed072f..2412e7ea33 100644 --- a/modules/git/git.go +++ b/modules/git/git.go @@ -12,7 +12,6 @@ import ( "path/filepath" "runtime" "strings" - "time" "code.gitea.io/gitea/modules/git/gitcmd" "code.gitea.io/gitea/modules/log" @@ -140,10 +139,6 @@ func InitSimple() error { log.Warn("git module has been initialized already, duplicate init may work but it's better to fix it") } - if setting.Git.Timeout.Default > 0 { - gitcmd.SetDefaultCommandExecutionTimeout(time.Duration(setting.Git.Timeout.Default) * time.Second) - } - if err := gitcmd.SetExecutablePath(setting.Git.Path); err != nil { return err } diff --git a/modules/git/gitcmd/command.go b/modules/git/gitcmd/command.go index 790c516383..0f3abeb568 100644 --- a/modules/git/gitcmd/command.go +++ b/modules/git/gitcmd/command.go @@ -28,13 +28,6 @@ import ( // In most cases, it shouldn't be used. Use AddXxx function instead type TrustedCmdArgs []internal.CmdArg -// defaultCommandExecutionTimeout default command execution timeout duration -var defaultCommandExecutionTimeout = 360 * time.Second - -func SetDefaultCommandExecutionTimeout(timeout time.Duration) { - defaultCommandExecutionTimeout = timeout -} - // DefaultLocale is the default LC_ALL to run git commands in. const DefaultLocale = "C" @@ -44,13 +37,14 @@ type Command struct { prog string args []string preErrors []error - cmd *exec.Cmd // for debug purpose only configArgs []string opts runOpts + cmd *exec.Cmd + cmdCtx context.Context - cmdCancel context.CancelFunc - cmdFinished context.CancelFunc + cmdCancel process.CancelCauseFunc + cmdFinished process.FinishedFunc cmdStartTime time.Time cmdStdinWriter *io.WriteCloser @@ -209,11 +203,9 @@ func ToTrustedCmdArgs(args []string) TrustedCmdArgs { return ret } -// runOpts represents parameters to run the command. If UseContextTimeout is specified, then Timeout is ignored. type runOpts struct { - Env []string - Timeout time.Duration - UseContextTimeout bool + Env []string + Timeout time.Duration // Dir is the working dir for the git command, however: // FIXME: this could be incorrect in many cases, for example: @@ -236,7 +228,7 @@ type runOpts struct { // Use new functions like WithStdinWriter to avoid such problems. Stdin io.Reader - PipelineFunc func(context.Context, context.CancelFunc) error + PipelineFunc func(Context) error } func commonBaseEnvs() []string { @@ -321,16 +313,11 @@ func (c *Command) WithStdin(stdin io.Reader) *Command { return c } -func (c *Command) WithPipelineFunc(f func(context.Context, context.CancelFunc) error) *Command { +func (c *Command) WithPipelineFunc(f func(Context) error) *Command { c.opts.PipelineFunc = f return c } -func (c *Command) WithUseContextTimeout(useContextTimeout bool) *Command { - c.opts.UseContextTimeout = useContextTimeout - return c -} - // WithParentCallerInfo can be used to set the caller info (usually function name) of the parent function of the caller. // For most cases, "Run" family functions can get its caller info automatically // But if you need to call "Run" family functions in a wrapper function: "FeatureFunc -> GeneralWrapperFunc -> RunXxx", @@ -363,9 +350,7 @@ func (c *Command) Start(ctx context.Context) (retErr error) { defer func() { if retErr != nil { // release the pipes to avoid resource leak - safeClosePtrCloser(c.cmdStdoutReader) - safeClosePtrCloser(c.cmdStderrReader) - safeClosePtrCloser(c.cmdStdinWriter) + c.closeStdioPipes() // if error occurs, we must also finish the task, otherwise, cmdFinished will be called in "Wait" function if c.cmdFinished != nil { c.cmdFinished() @@ -380,12 +365,6 @@ func (c *Command) Start(ctx context.Context) (retErr error) { return err } - // We must not change the provided options - timeout := c.opts.Timeout - if timeout <= 0 { - timeout = defaultCommandExecutionTimeout - } - cmdLogString := c.LogString() if c.callerInfo == "" { c.WithParentCallerInfo() @@ -399,83 +378,85 @@ func (c *Command) Start(ctx context.Context) (retErr error) { span.SetAttributeString(gtprof.TraceAttrFuncCaller, c.callerInfo) span.SetAttributeString(gtprof.TraceAttrGitCommand, cmdLogString) - if c.opts.UseContextTimeout { + if c.opts.Timeout <= 0 { c.cmdCtx, c.cmdCancel, c.cmdFinished = process.GetManager().AddContext(ctx, desc) } else { - c.cmdCtx, c.cmdCancel, c.cmdFinished = process.GetManager().AddContextTimeout(ctx, timeout, desc) + c.cmdCtx, c.cmdCancel, c.cmdFinished = process.GetManager().AddContextTimeout(ctx, c.opts.Timeout, desc) } c.cmdStartTime = time.Now() - cmd := exec.CommandContext(ctx, c.prog, append(c.configArgs, c.args...)...) - c.cmd = cmd // for debug purpose only + c.cmd = exec.CommandContext(ctx, c.prog, append(c.configArgs, c.args...)...) if c.opts.Env == nil { - cmd.Env = os.Environ() + c.cmd.Env = os.Environ() } else { - cmd.Env = c.opts.Env + c.cmd.Env = c.opts.Env } - process.SetSysProcAttribute(cmd) - cmd.Env = append(cmd.Env, CommonGitCmdEnvs()...) - cmd.Dir = c.opts.Dir - cmd.Stdout = c.opts.Stdout - cmd.Stdin = c.opts.Stdin + process.SetSysProcAttribute(c.cmd) + c.cmd.Env = append(c.cmd.Env, CommonGitCmdEnvs()...) + c.cmd.Dir = c.opts.Dir + c.cmd.Stdout = c.opts.Stdout + c.cmd.Stdin = c.opts.Stdin - if _, err := safeAssignPipe(c.cmdStdinWriter, cmd.StdinPipe); err != nil { + if _, err := safeAssignPipe(c.cmdStdinWriter, c.cmd.StdinPipe); err != nil { return err } - if _, err := safeAssignPipe(c.cmdStdoutReader, cmd.StdoutPipe); err != nil { + if _, err := safeAssignPipe(c.cmdStdoutReader, c.cmd.StdoutPipe); err != nil { return err } - if _, err := safeAssignPipe(c.cmdStderrReader, cmd.StderrPipe); err != nil { + if _, err := safeAssignPipe(c.cmdStderrReader, c.cmd.StderrPipe); err != nil { return err } if c.cmdManagedStderr != nil { - if cmd.Stderr != nil { + if c.cmd.Stderr != nil { panic("CombineStderr needs managed (but not caller-provided) stderr pipe") } - cmd.Stderr = c.cmdManagedStderr + c.cmd.Stderr = c.cmdManagedStderr } - return cmd.Start() + return c.cmd.Start() +} + +func (c *Command) closeStdioPipes() { + safeClosePtrCloser(c.cmdStdoutReader) + safeClosePtrCloser(c.cmdStderrReader) + safeClosePtrCloser(c.cmdStdinWriter) } func (c *Command) Wait() error { defer func() { - safeClosePtrCloser(c.cmdStdoutReader) - safeClosePtrCloser(c.cmdStderrReader) - safeClosePtrCloser(c.cmdStdinWriter) + c.closeStdioPipes() c.cmdFinished() }() - cmd, ctx, cancel := c.cmd, c.cmdCtx, c.cmdCancel - if c.opts.PipelineFunc != nil { - err := c.opts.PipelineFunc(ctx, cancel) - if err != nil { - cancel() - errWait := cmd.Wait() - return errors.Join(err, errWait) + errCallback := c.opts.PipelineFunc(&cmdContext{Context: c.cmdCtx, cmd: c}) + // after the pipeline function returns, we can safely cancel the command context and close the stdio pipes + c.cmdCancel(errCallback) + c.closeStdioPipes() + errWait := c.cmd.Wait() + errCause := context.Cause(c.cmdCtx) + // the pipeline function should be able to know whether it succeeds or fails + if errCallback == nil && (errCause == nil || errors.Is(errCause, context.Canceled)) { + return nil } + return errors.Join(errCallback, errCause, errWait) } - errWait := cmd.Wait() + // there might be other goroutines using the context or pipes, so we just wait for the command to finish + errWait := c.cmd.Wait() elapsed := time.Since(c.cmdStartTime) if elapsed > time.Second { - log.Debug("slow git.Command.Run: %s (%s)", c, elapsed) + log.Debug("slow git.Command.Run: %s (%s)", c, elapsed) // TODO: no need to log this for long-running commands } + // Here the logic is different from "PipelineFunc" case, + // because PipelineFunc can return error if it fails, it knows whether it succeeds or fails. + // But in normal case, the caller just runs the git command, the command's exit code is the source of truth. + // If the caller need to know whether the command error is caused by cancellation, it should check the "err" by itself. errCause := context.Cause(c.cmdCtx) - if errors.Is(errCause, context.Canceled) { - // if the ctx is canceled without other error, it must be caused by normal cancellation - return errCause - } - if errWait != nil { - // no matter whether there is other cause error, if "Wait" also has error, - // it's likely the error is caused by Wait error (from git command) - return errWait - } - return errCause + return errors.Join(errCause, errWait) } func (c *Command) StartWithStderr(ctx context.Context) RunStdError { @@ -513,59 +494,6 @@ func (c *Command) Run(ctx context.Context) (err error) { return c.Wait() } -type RunStdError interface { - error - Unwrap() error - Stderr() string -} - -type runStdError struct { - err error // usually the low-level error like `*exec.ExitError` - stderr string // git command's stderr output - errMsg string // the cached error message for Error() method -} - -func (r *runStdError) Error() string { - // FIXME: GIT-CMD-STDERR: it is a bad design, the stderr should not be put in the error message - // But a lot of code only checks `strings.Contains(err.Error(), "git error")` - if r.errMsg == "" { - r.errMsg = fmt.Sprintf("%s - %s", r.err.Error(), strings.TrimSpace(r.stderr)) - } - return r.errMsg -} - -func (r *runStdError) Unwrap() error { - return r.err -} - -func (r *runStdError) Stderr() string { - return r.stderr -} - -func ErrorAsStderr(err error) (string, bool) { - var runErr RunStdError - if errors.As(err, &runErr) { - return runErr.Stderr(), true - } - return "", false -} - -func StderrHasPrefix(err error, prefix string) bool { - stderr, ok := ErrorAsStderr(err) - if !ok { - return false - } - return strings.HasPrefix(stderr, prefix) -} - -func IsErrorExitCode(err error, code int) bool { - var exitError *exec.ExitError - if errors.As(err, &exitError) { - return exitError.ExitCode() == code - } - return false -} - // RunStdString runs the command and returns stdout/stderr as string. and store stderr to returned error (err combined with stderr). func (c *Command) RunStdString(ctx context.Context) (stdout, stderr string, runErr RunStdError) { stdoutBytes, stderrBytes, runErr := c.WithParentCallerInfo().runStdBytes(ctx) diff --git a/modules/git/gitcmd/command_race_test.go b/modules/git/gitcmd/command_race_test.go deleted file mode 100644 index 962470b6db..0000000000 --- a/modules/git/gitcmd/command_race_test.go +++ /dev/null @@ -1,38 +0,0 @@ -// Copyright 2017 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -//go:build race - -package gitcmd - -import ( - "context" - "testing" - "time" -) - -func TestRunWithContextNoTimeout(t *testing.T) { - maxLoops := 10 - - // 'git --version' does not block so it must be finished before the timeout triggered. - for i := 0; i < maxLoops; i++ { - cmd := NewCommand("--version") - if err := cmd.Run(t.Context()); err != nil { - t.Fatal(err) - } - } -} - -func TestRunWithContextTimeout(t *testing.T) { - maxLoops := 10 - - // 'git hash-object --stdin' blocks on stdin so we can have the timeout triggered. - for i := 0; i < maxLoops; i++ { - cmd := NewCommand("hash-object", "--stdin") - if err := cmd.WithTimeout(1 * time.Millisecond).Run(t.Context()); err != nil { - if err != context.DeadlineExceeded { - t.Fatalf("Testing %d/%d: %v", i, maxLoops, err) - } - } - } -} diff --git a/modules/git/gitcmd/command_test.go b/modules/git/gitcmd/command_test.go index c227a0e210..63e82635be 100644 --- a/modules/git/gitcmd/command_test.go +++ b/modules/git/gitcmd/command_test.go @@ -4,9 +4,11 @@ package gitcmd import ( + "context" "fmt" "os" "testing" + "time" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/tempdir" @@ -111,3 +113,15 @@ func TestRunStdError(t *testing.T) { require.ErrorAs(t, fmt.Errorf("wrapped %w", err), &asErr) } + +func TestRunWithContextTimeout(t *testing.T) { + t.Run("NoTimeout", func(t *testing.T) { + // 'git --version' does not block so it must be finished before the timeout triggered. + err := NewCommand("--version").Run(t.Context()) + require.NoError(t, err) + }) + t.Run("WithTimeout", func(t *testing.T) { + err := NewCommand("hash-object", "--stdin").WithTimeout(1 * time.Millisecond).Run(t.Context()) + require.ErrorIs(t, err, context.DeadlineExceeded) + }) +} diff --git a/modules/git/gitcmd/context.go b/modules/git/gitcmd/context.go new file mode 100644 index 0000000000..dbcea0e626 --- /dev/null +++ b/modules/git/gitcmd/context.go @@ -0,0 +1,28 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package gitcmd + +import ( + "context" +) + +type Context interface { + context.Context + + // CancelWithCause is a helper function to cancel the context with a specific error cause + // And it returns the same error for convenience, to break the PipelineFunc easily + CancelWithCause(err error) error + + // In the future, this interface will be extended to support stdio pipe readers/writers +} + +type cmdContext struct { + context.Context + cmd *Command +} + +func (c *cmdContext) CancelWithCause(err error) error { + c.cmd.cmdCancel(err) + return err +} diff --git a/modules/git/gitcmd/error.go b/modules/git/gitcmd/error.go new file mode 100644 index 0000000000..8f5397b96d --- /dev/null +++ b/modules/git/gitcmd/error.go @@ -0,0 +1,78 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package gitcmd + +import ( + "context" + "errors" + "fmt" + "os/exec" + "strings" +) + +type RunStdError interface { + error + Unwrap() error + Stderr() string +} + +type runStdError struct { + err error // usually the low-level error like `*exec.ExitError` + stderr string // git command's stderr output + errMsg string // the cached error message for Error() method +} + +func (r *runStdError) Error() string { + // FIXME: GIT-CMD-STDERR: it is a bad design, the stderr should not be put in the error message + // But a lot of code only checks `strings.Contains(err.Error(), "git error")` + if r.errMsg == "" { + r.errMsg = fmt.Sprintf("%s - %s", r.err.Error(), strings.TrimSpace(r.stderr)) + } + return r.errMsg +} + +func (r *runStdError) Unwrap() error { + return r.err +} + +func (r *runStdError) Stderr() string { + return r.stderr +} + +func ErrorAsStderr(err error) (string, bool) { + var runErr RunStdError + if errors.As(err, &runErr) { + return runErr.Stderr(), true + } + return "", false +} + +func StderrHasPrefix(err error, prefix string) bool { + stderr, ok := ErrorAsStderr(err) + if !ok { + return false + } + return strings.HasPrefix(stderr, prefix) +} + +func IsErrorExitCode(err error, code int) bool { + var exitError *exec.ExitError + if errors.As(err, &exitError) { + return exitError.ExitCode() == code + } + return false +} + +func IsErrorSignalKilled(err error) bool { + var exitError *exec.ExitError + return errors.As(err, &exitError) && exitError.String() == "signal: killed" +} + +func IsErrorCanceledOrKilled(err error) bool { + // When "cancel()" a git command's context, the returned error of "Run()" could be one of them: + // - context.Canceled + // - *exec.ExitError: "signal: killed" + // TODO: in the future, we need to use unified error type from gitcmd.Run to check whether it is manually canceled + return errors.Is(err, context.Canceled) || IsErrorSignalKilled(err) +} diff --git a/modules/git/grep.go b/modules/git/grep.go index edd46f4314..bf348387e7 100644 --- a/modules/git/grep.go +++ b/modules/git/grep.go @@ -77,9 +77,7 @@ func GrepSearch(ctx context.Context, repo *Repository, search string, opts GrepO var stdoutReader io.ReadCloser err := cmd.WithDir(repo.Path). WithStdoutReader(&stdoutReader). - WithPipelineFunc(func(ctx context.Context, cancel context.CancelFunc) error { - defer stdoutReader.Close() - + WithPipelineFunc(func(ctx gitcmd.Context) error { isInBlock := false rd := bufio.NewReaderSize(stdoutReader, util.IfZero(opts.MaxLineLength, 16*1024)) var res *GrepResult @@ -105,8 +103,7 @@ func GrepSearch(ctx context.Context, repo *Repository, search string, opts GrepO } if line == "" { if len(results) >= opts.MaxResultLimit { - cancel() - break + return ctx.CancelWithCause(nil) } isInBlock = false continue diff --git a/modules/git/remote.go b/modules/git/remote.go index 1999ad4b94..ae56c5576a 100644 --- a/modules/git/remote.go +++ b/modules/git/remote.go @@ -74,9 +74,9 @@ func (err *ErrInvalidCloneAddr) Unwrap() error { func IsRemoteNotExistError(err error) bool { // see: https://github.com/go-gitea/gitea/issues/32889#issuecomment-2571848216 // Should not add space in the end, sometimes git will add a `:` - prefix1 := "exit status 128 - fatal: No such remote" // git < 2.30 - prefix2 := "exit status 2 - error: No such remote" // git >= 2.30 - return strings.HasPrefix(err.Error(), prefix1) || strings.HasPrefix(err.Error(), prefix2) + prefix1 := "fatal: No such remote" // git < 2.30, exit status 128 + prefix2 := "error: No such remote" // git >= 2.30. exit status 2 + return gitcmd.StderrHasPrefix(err, prefix1) || gitcmd.StderrHasPrefix(err, prefix2) } // ParseRemoteAddr checks if given remote address is valid, diff --git a/modules/git/repo_stats.go b/modules/git/repo_stats.go index 175f5592c2..2232fbadf4 100644 --- a/modules/git/repo_stats.go +++ b/modules/git/repo_stats.go @@ -5,9 +5,8 @@ package git import ( "bufio" - "context" "fmt" - "os" + "io" "sort" "strconv" "strings" @@ -55,15 +54,6 @@ func (repo *Repository) GetCodeActivityStats(fromTime time.Time, branch string) } stats.CommitCountInAllBranches = c - stdoutReader, stdoutWriter, err := os.Pipe() - if err != nil { - return nil, err - } - defer func() { - _ = stdoutReader.Close() - _ = stdoutWriter.Close() - }() - gitCmd := gitcmd.NewCommand("log", "--numstat", "--no-merges", "--pretty=format:---%n%h%n%aN%n%aE%n", "--date=iso"). AddOptionFormat("--since=%s", since) if len(branch) == 0 { @@ -72,11 +62,11 @@ func (repo *Repository) GetCodeActivityStats(fromTime time.Time, branch string) gitCmd.AddArguments("--first-parent").AddDynamicArguments(branch) } + var stdoutReader io.ReadCloser 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) scanner.Split(bufio.ScanLines) stats.CommitCount = 0 diff --git a/modules/git/submodule.go b/modules/git/submodule.go index 45059eae77..b32e6e4031 100644 --- a/modules/git/submodule.go +++ b/modules/git/submodule.go @@ -7,7 +7,7 @@ import ( "bufio" "context" "fmt" - "os" + "io" "code.gitea.io/gitea/modules/git/gitcmd" "code.gitea.io/gitea/modules/log" @@ -21,23 +21,15 @@ type TemplateSubmoduleCommit struct { // GetTemplateSubmoduleCommits returns a list of submodules paths and their commits from a repository // This function is only for generating new repos based on existing template, the template couldn't be too large. func GetTemplateSubmoduleCommits(ctx context.Context, repoPath string) (submoduleCommits []TemplateSubmoduleCommit, _ error) { - stdoutReader, stdoutWriter, err := os.Pipe() - if err != nil { - return nil, err - } - - err = gitcmd.NewCommand("ls-tree", "-r", "--", "HEAD"). + var stdoutReader io.ReadCloser + err := gitcmd.NewCommand("ls-tree", "-r", "--", "HEAD"). WithDir(repoPath). - 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) for scanner.Scan() { entry, err := parseLsTreeLine(scanner.Bytes()) if err != nil { - cancel() return err } if entry.EntryMode == EntryModeCommit { diff --git a/modules/gitrepo/blame.go b/modules/gitrepo/blame.go index a2cb7ac1b7..192cf2c91f 100644 --- a/modules/gitrepo/blame.go +++ b/modules/gitrepo/blame.go @@ -174,7 +174,7 @@ func CreateBlameReader(ctx context.Context, objectFormat git.ObjectFormat, repo } go func() { // TODO: it doesn't work for directories (the directories shouldn't be "blamed"), and the "err" should be returned by "Read" but not by "Close" - err := RunCmdWithStderr(ctx, repo, cmd.WithUseContextTimeout(true).WithStdout(stdout)) + err := RunCmdWithStderr(ctx, repo, cmd.WithStdout(stdout)) done <- err _ = stdout.Close() }() diff --git a/modules/log/init.go b/modules/log/init.go index 3fb5200ad7..ccaab50de3 100644 --- a/modules/log/init.go +++ b/modules/log/init.go @@ -35,10 +35,10 @@ func init() { } } -func newProcessTypedContext(parent context.Context, desc string) (ctx context.Context, cancel context.CancelFunc) { +func newProcessTypedContext(parent context.Context, desc string) (context.Context, context.CancelFunc) { // the "process manager" also calls "log.Trace()" to output logs, so if we want to create new contexts by the manager, we need to disable the trace temporarily process.TraceLogDisable(true) defer process.TraceLogDisable(false) - ctx, _, cancel = process.GetManager().AddTypedContext(parent, desc, process.SystemProcessType, false) - return ctx, cancel + ctx, _, finished := process.GetManager().AddTypedContext(parent, desc, process.SystemProcessType, false) + return ctx, context.CancelFunc(finished) } diff --git a/modules/nosql/manager.go b/modules/nosql/manager.go index 375c2b5d00..9d3ab49c9a 100644 --- a/modules/nosql/manager.go +++ b/modules/nosql/manager.go @@ -20,7 +20,7 @@ var manager *Manager // Manager is the nosql connection manager type Manager struct { ctx context.Context - finished context.CancelFunc + finished process.FinishedFunc mutex sync.Mutex RedisConnections map[string]*redisClientHolder diff --git a/modules/process/manager.go b/modules/process/manager.go index 661511ce8d..c51d6c76f9 100644 --- a/modules/process/manager.go +++ b/modules/process/manager.go @@ -13,6 +13,7 @@ import ( "time" "code.gitea.io/gitea/modules/gtprof" + "code.gitea.io/gitea/modules/util" ) // TODO: This packages still uses a singleton for the Manager. @@ -27,12 +28,14 @@ var ( DefaultContext = context.Background() ) -// IDType is a pid type -type IDType string +type ( + // IDType is a pid type + IDType string -// FinishedFunc is a function that marks that the process is finished and can be removed from the process table -// - it is simply an alias for context.CancelFunc and is only for documentary purposes -type FinishedFunc = context.CancelFunc + CancelCauseFunc func(cause ...error) + // FinishedFunc is a function that marks that the process is finished and can be removed from the process table + FinishedFunc func() +) var ( traceDisabled atomic.Int64 @@ -84,6 +87,10 @@ func GetManager() *Manager { return manager } +func cancelCauseFunc(cancelCause context.CancelCauseFunc) CancelCauseFunc { + return func(cause ...error) { cancelCause(util.OptionalArg(cause)) } +} + // AddContext creates a new context and adds it as a process. Once the process is finished, finished must be called // to remove the process from the process table. It should not be called until the process is finished but must always be called. // @@ -92,11 +99,10 @@ func GetManager() *Manager { // // Most processes will not need to use the cancel function but there will be cases whereby you want to cancel the process but not immediately remove it from the // process table. -func (pm *Manager) AddContext(parent context.Context, description string) (ctx context.Context, cancel context.CancelFunc, finished FinishedFunc) { - ctx, cancel = context.WithCancel(parent) - - ctx, _, finished = pm.Add(ctx, description, cancel, NormalProcessType, true) - +func (pm *Manager) AddContext(parent context.Context, description string) (context.Context, CancelCauseFunc, FinishedFunc) { + ctx, ctxCancel := context.WithCancelCause(parent) + cancel := cancelCauseFunc(ctxCancel) + ctx, _, finished := pm.Add(ctx, description, cancel, NormalProcessType, true) return ctx, cancel, finished } @@ -108,11 +114,10 @@ func (pm *Manager) AddContext(parent context.Context, description string) (ctx c // // Most processes will not need to use the cancel function but there will be cases whereby you want to cancel the process but not immediately remove it from the // process table. -func (pm *Manager) AddTypedContext(parent context.Context, description, processType string, currentlyRunning bool) (ctx context.Context, cancel context.CancelFunc, finished FinishedFunc) { - ctx, cancel = context.WithCancel(parent) - - ctx, _, finished = pm.Add(ctx, description, cancel, processType, currentlyRunning) - +func (pm *Manager) AddTypedContext(parent context.Context, description, processType string, currentlyRunning bool) (context.Context, CancelCauseFunc, FinishedFunc) { + ctx, ctxCancel := context.WithCancelCause(parent) + cancel := cancelCauseFunc(ctxCancel) + ctx, _, finished := pm.Add(ctx, description, cancel, processType, currentlyRunning) return ctx, cancel, finished } @@ -124,21 +129,23 @@ func (pm *Manager) AddTypedContext(parent context.Context, description, processT // // Most processes will not need to use the cancel function but there will be cases whereby you want to cancel the process but not immediately remove it from the // process table. -func (pm *Manager) AddContextTimeout(parent context.Context, timeout time.Duration, description string) (ctx context.Context, cancel context.CancelFunc, finished FinishedFunc) { +func (pm *Manager) AddContextTimeout(parent context.Context, timeout time.Duration, description string) (context.Context, CancelCauseFunc, FinishedFunc) { if timeout <= 0 { // it's meaningless to use timeout <= 0, and it must be a bug! so we must panic here to tell developers to make the timeout correct panic("the timeout must be greater than zero, otherwise the context will be cancelled immediately") } - - ctx, cancel = context.WithTimeout(parent, timeout) - - ctx, _, finished = pm.Add(ctx, description, cancel, NormalProcessType, true) - + ctx, ctxCancelTimeout := context.WithTimeout(parent, timeout) + ctx, ctxCancelCause := context.WithCancelCause(ctx) + cancel := func(cause ...error) { + ctxCancelCause(util.OptionalArg(cause)) + ctxCancelTimeout() + } + ctx, _, finished := pm.Add(ctx, description, cancel, NormalProcessType, true) return ctx, cancel, finished } // Add create a new process -func (pm *Manager) Add(ctx context.Context, description string, cancel context.CancelFunc, processType string, currentlyRunning bool) (context.Context, IDType, FinishedFunc) { +func (pm *Manager) Add(ctx context.Context, description string, cancel CancelCauseFunc, processType string, currentlyRunning bool) (context.Context, IDType, FinishedFunc) { parentPID := GetParentPID(ctx) pm.mutex.Lock() diff --git a/modules/process/process.go b/modules/process/process.go index 06a28c4a60..d81f5ffa1d 100644 --- a/modules/process/process.go +++ b/modules/process/process.go @@ -4,7 +4,6 @@ package process import ( - "context" "time" ) @@ -21,7 +20,7 @@ type process struct { ParentPID IDType Description string Start time.Time - Cancel context.CancelFunc + Cancel CancelCauseFunc Type string } diff --git a/modules/queue/workerqueue.go b/modules/queue/workerqueue.go index 0f5b105551..d8b0722caf 100644 --- a/modules/queue/workerqueue.go +++ b/modules/queue/workerqueue.go @@ -21,7 +21,7 @@ import ( // It can use different underlying (base) queue types type WorkerPoolQueue[T any] struct { ctxRun context.Context - ctxRunCancel context.CancelFunc + ctxRunCancel process.FinishedFunc shutdownDone chan struct{} shutdownTimeout atomic.Int64 // in case some buggy handlers (workers) would hang forever, "shutdown" should finish in predictable time diff --git a/modules/setting/git.go b/modules/setting/git.go index 318f2c0cac..29fd3daf8a 100644 --- a/modules/setting/git.go +++ b/modules/setting/git.go @@ -33,11 +33,8 @@ var Git = struct { DisablePartialClone bool DiffRenameSimilarityThreshold string Timeout struct { - Default int Migrate int Mirror int - Clone int - Pull int GC int `ini:"GC"` } `ini:"git.timeout"` }{ @@ -56,18 +53,12 @@ var Git = struct { DisablePartialClone: false, DiffRenameSimilarityThreshold: "50%", Timeout: struct { - Default int Migrate int Mirror int - Clone int - Pull int GC int `ini:"GC"` }{ - Default: 360, Migrate: 600, Mirror: 300, - Clone: 300, - Pull: 300, GC: 60, }, } diff --git a/options/locale/locale_en-US.json b/options/locale/locale_en-US.json index f6c2aeb3cb..f336605066 100644 --- a/options/locale/locale_en-US.json +++ b/options/locale/locale_en-US.json @@ -3284,8 +3284,6 @@ "admin.config.git_gc_args": "GC Arguments", "admin.config.git_migrate_timeout": "Migration Timeout", "admin.config.git_mirror_timeout": "Mirror Update Timeout", - "admin.config.git_clone_timeout": "Clone Operation Timeout", - "admin.config.git_pull_timeout": "Pull Operation Timeout", "admin.config.git_gc_timeout": "GC Operation Timeout", "admin.config.log_config": "Log Configuration", "admin.config.logger_name_fmt": "Logger: %s", diff --git a/routers/private/hook_verification.go b/routers/private/hook_verification.go index 9c357f4b41..0867028c4c 100644 --- a/routers/private/hook_verification.go +++ b/routers/private/hook_verification.go @@ -5,9 +5,7 @@ package private import ( "bufio" - "context" "io" - "os" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/git/gitcmd" @@ -18,16 +16,6 @@ import ( // This file contains commit verification functions for refs passed across in hooks func verifyCommits(oldCommitID, newCommitID string, repo *git.Repository, env []string) error { - stdoutReader, stdoutWriter, err := os.Pipe() - if err != nil { - log.Error("Unable to create os.Pipe for %s", repo.Path) - return err - } - defer func() { - _ = stdoutReader.Close() - _ = stdoutWriter.Close() - }() - var command *gitcmd.Command objectFormat, _ := repo.GetObjectFormat() if oldCommitID == objectFormat.EmptyObjectID().String() { @@ -39,18 +27,13 @@ func verifyCommits(oldCommitID, newCommitID string, repo *git.Repository, env [] command = gitcmd.NewCommand("rev-list").AddDynamicArguments(oldCommitID + "..." + newCommitID) } // This is safe as force pushes are already forbidden - err = command.WithEnv(env). + var stdoutReader io.ReadCloser + err := command.WithEnv(env). WithDir(repo.Path). - WithStdout(stdoutWriter). - WithPipelineFunc(func(ctx context.Context, cancel context.CancelFunc) error { - _ = stdoutWriter.Close() + WithStdoutReader(&stdoutReader). + WithPipelineFunc(func(ctx gitcmd.Context) error { err := readAndVerifyCommitsFromShaReader(stdoutReader, repo, env) - if err != nil { - log.Error("readAndVerifyCommitsFromShaReader failed: %v", err) - cancel() - } - _ = stdoutReader.Close() - return err + return ctx.CancelWithCause(err) }). Run(repo.Ctx) if err != nil && !isErrUnverifiedCommit(err) { @@ -72,34 +55,20 @@ func readAndVerifyCommitsFromShaReader(input io.ReadCloser, repo *git.Repository } func readAndVerifyCommit(sha string, repo *git.Repository, env []string) error { - stdoutReader, stdoutWriter, err := os.Pipe() - if err != nil { - log.Error("Unable to create pipe for %s: %v", repo.Path, err) - return err - } - defer func() { - _ = stdoutReader.Close() - _ = stdoutWriter.Close() - }() - commitID := git.MustIDFromString(sha) - + var stdoutReader io.ReadCloser return gitcmd.NewCommand("cat-file", "commit").AddDynamicArguments(sha). WithEnv(env). WithDir(repo.Path). - WithStdout(stdoutWriter). - WithPipelineFunc(func(ctx context.Context, cancel context.CancelFunc) error { - _ = stdoutWriter.Close() + WithStdoutReader(&stdoutReader). + WithPipelineFunc(func(ctx gitcmd.Context) error { commit, err := git.CommitFromReader(repo, commitID, stdoutReader) if err != nil { return err } verification := asymkey_service.ParseCommitWithSignature(ctx, commit) if !verification.Verified { - cancel() - return &errUnverifiedCommit{ - commit.ID.String(), - } + return ctx.CancelWithCause(&errUnverifiedCommit{commit.ID.String()}) } return nil }). diff --git a/routers/web/repo/githttp.go b/routers/web/repo/githttp.go index db1b1d5cb3..e169955832 100644 --- a/routers/web/repo/githttp.go +++ b/routers/web/repo/githttp.go @@ -447,9 +447,9 @@ func serviceRPC(ctx *context.Context, service string) { if err := gitrepo.RunCmdWithStderr(ctx, h.getStorageRepo(), cmd.AddArguments("."). WithEnv(append(os.Environ(), h.environ...)). WithStdin(reqBody). - WithStdout(ctx.Resp). - WithUseContextTimeout(true)); err != nil { - if !git.IsErrCanceledOrKilled(err) { + WithStdout(ctx.Resp), + ); err != nil { + if !gitcmd.IsErrorCanceledOrKilled(err) { log.Error("Fail to serve RPC(%s) in %s: %v", service, h.getStorageRepo().RelativePath(), err) } } diff --git a/services/cron/tasks_basic.go b/services/cron/tasks_basic.go index 48380b7b9a..c620959cc1 100644 --- a/services/cron/tasks_basic.go +++ b/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) diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index dbb1898f21..fb1bd7f4ef 100644 --- a/services/gitdiff/gitdiff.go +++ b/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) } diff --git a/services/pull/merge_prepare.go b/services/pull/merge_prepare.go index d38138c514..2c9727df52 100644 --- a/services/pull/merge_prepare.go +++ b/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() }). diff --git a/services/pull/patch.go b/services/pull/patch.go index ce3afcc331..30b8f964de 100644 --- a/services/pull/patch.go +++ b/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..." diff --git a/services/pull/patch_unmerged.go b/services/pull/patch_unmerged.go index 3cfaabe662..ca0e515616 100644 --- a/services/pull/patch_unmerged.go +++ b/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 { diff --git a/services/pull/pull.go b/services/pull/pull.go index baad89aa55..490d23abca 100644 --- a/services/pull/pull.go +++ b/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 { diff --git a/services/pull/review.go b/services/pull/review.go index 9aeeb4c31d..6e16205e70 100644 --- a/services/pull/review.go +++ b/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) } } diff --git a/services/repository/contributors_graph.go b/services/repository/contributors_graph.go index 6f42c75036..952aed6d9a 100644 --- a/services/repository/contributors_graph.go +++ b/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) diff --git a/services/repository/files/temp_repo.go b/services/repository/files/temp_repo.go index 17acf1f9ce..9e2a1ec91b 100644 --- a/services/repository/files/temp_repo.go +++ b/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) } diff --git a/services/repository/gitgraph/graph.go b/services/repository/gitgraph/graph.go index 268878e166..565f67872e 100644 --- a/services/repository/gitgraph/graph.go +++ b/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() diff --git a/templates/admin/config.tmpl b/templates/admin/config.tmpl index 57631fd9c6..728746713c 100644 --- a/templates/admin/config.tmpl +++ b/templates/admin/config.tmpl @@ -307,10 +307,6 @@
{{.Git.Timeout.Migrate}} {{ctx.Locale.Tr "tool.raw_seconds"}}
{{ctx.Locale.Tr "admin.config.git_mirror_timeout"}}
{{.Git.Timeout.Mirror}} {{ctx.Locale.Tr "tool.raw_seconds"}}
-
{{ctx.Locale.Tr "admin.config.git_clone_timeout"}}
-
{{.Git.Timeout.Clone}} {{ctx.Locale.Tr "tool.raw_seconds"}}
-
{{ctx.Locale.Tr "admin.config.git_pull_timeout"}}
-
{{.Git.Timeout.Pull}} {{ctx.Locale.Tr "tool.raw_seconds"}}
{{ctx.Locale.Tr "admin.config.git_gc_timeout"}}
{{.Git.Timeout.GC}} {{ctx.Locale.Tr "tool.raw_seconds"}}