diff --git a/.golangci.yml b/.golangci.yml index 41a0185df7..62c1d005fa 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -50,6 +50,8 @@ linters: desc: do not use the go-chi cache package, use gitea's cache system - pkg: github.com/pkg/errors desc: use builtin errors package instead + - pkg: github.com/go-ap/errors + desc: use builtin errors package instead nolintlint: allow-unused: false require-explanation: true diff --git a/cmd/hook.go b/cmd/hook.go index b65d5fbbd2..3a03b65a37 100644 --- a/cmd/hook.go +++ b/cmd/hook.go @@ -318,7 +318,7 @@ func runHookPostReceive(ctx context.Context, c *cli.Command) error { setup(ctx, c.Bool("debug")) // First of all run update-server-info no matter what - if _, _, err := gitcmd.NewCommand("update-server-info").RunStdString(ctx); err != nil { + if err := gitcmd.NewCommand("update-server-info").RunWithStderr(ctx); err != nil { return fmt.Errorf("failed to call 'git update-server-info': %w", err) } diff --git a/modules/git/attribute/batch.go b/modules/git/attribute/batch.go index 27befdfa25..6038815d82 100644 --- a/modules/git/attribute/batch.go +++ b/modules/git/attribute/batch.go @@ -76,13 +76,11 @@ func NewBatchChecker(repo *git.Repository, treeish string, attributes []string) _ = stdinReader.Close() _ = lw.Close() }() - stdErr := new(bytes.Buffer) err := cmd.WithEnv(envs). WithDir(repo.Path). WithStdin(stdinReader). WithStdout(lw). - WithStderr(stdErr). - Run(ctx) + RunWithStderr(ctx) if err != nil && !git.IsErrCanceledOrKilled(err) { log.Error("Attribute checker for commit %s exits with error: %v", treeish, err) diff --git a/modules/git/attribute/checker.go b/modules/git/attribute/checker.go index 49c0eb90ef..4b86d7d9ba 100644 --- a/modules/git/attribute/checker.go +++ b/modules/git/attribute/checker.go @@ -69,14 +69,11 @@ func CheckAttributes(ctx context.Context, gitRepo *git.Repository, treeish strin defer cancel() stdOut := new(bytes.Buffer) - stdErr := new(bytes.Buffer) - if err := cmd.WithEnv(append(os.Environ(), envs...)). WithDir(gitRepo.Path). WithStdout(stdOut). - WithStderr(stdErr). - Run(ctx); err != nil { - return nil, fmt.Errorf("failed to run check-attr: %w\n%s\n%s", err, stdOut.String(), stdErr.String()) + RunWithStderr(ctx); err != nil { + return nil, fmt.Errorf("failed to run check-attr: %w", err) } fields := bytes.Split(stdOut.Bytes(), []byte{'\000'}) diff --git a/modules/git/catfile_batch_reader.go b/modules/git/catfile_batch_reader.go index 2f65f522c5..aede3cf422 100644 --- a/modules/git/catfile_batch_reader.go +++ b/modules/git/catfile_batch_reader.go @@ -7,6 +7,7 @@ import ( "bufio" "bytes" "context" + "errors" "io" "math" "strconv" @@ -40,15 +41,13 @@ func newCatFileBatch(ctx context.Context, repoPath string, cmdCatFile *gitcmd.Co var batchStdinWriter io.WriteCloser var batchStdoutReader io.ReadCloser - stderr := strings.Builder{} cmdCatFile = cmdCatFile. WithDir(repoPath). WithStdinWriter(&batchStdinWriter). WithStdoutReader(&batchStdoutReader). - WithStderr(&stderr). WithUseContextTimeout(true) - err := cmdCatFile.Start(ctx) + err := cmdCatFile.StartWithStderr(ctx) if err != nil { log.Error("Unable to start git command %v: %v", cmdCatFile.LogString(), err) // ideally here it should return the error, but it would require refactoring all callers @@ -63,9 +62,9 @@ func newCatFileBatch(ctx context.Context, repoPath string, cmdCatFile *gitcmd.Co } go func() { - err := cmdCatFile.Wait() - if err != nil { - log.Error("cat-file --batch command failed in repo %s: %v - stderr: %s", repoPath, err, stderr.String()) + err := cmdCatFile.WaitWithStderr() + if err != nil && !errors.Is(err, context.Canceled) { + log.Error("cat-file --batch command failed in repo %s, error: %v", repoPath, err) } ctxCancel(err) }() diff --git a/modules/git/commit.go b/modules/git/commit.go index 1917a72bbf..e66a33ef98 100644 --- a/modules/git/commit.go +++ b/modules/git/commit.go @@ -120,7 +120,7 @@ func CommitChanges(ctx context.Context, repoPath string, opts CommitChangesOptio _, _, err := cmd.WithDir(repoPath).RunStdString(ctx) // No stderr but exit status 1 means nothing to commit. - if err != nil && err.Error() == "exit status 1" { + if gitcmd.IsErrorExitCode(err, 1) { return nil } return err @@ -315,7 +315,7 @@ func GetFullCommitID(ctx context.Context, repoPath, shortID string) (string, err WithDir(repoPath). RunStdString(ctx) if err != nil { - if strings.Contains(err.Error(), "exit status 128") { + if gitcmd.IsErrorExitCode(err, 128) { return "", ErrNotExist{shortID, ""} } return "", err diff --git a/modules/git/diff.go b/modules/git/diff.go index 309d8f4615..6fa5e6f2bc 100644 --- a/modules/git/diff.go +++ b/modules/git/diff.go @@ -5,7 +5,6 @@ package git import ( "bufio" - "bytes" "context" "fmt" "io" @@ -80,14 +79,9 @@ func GetRepoRawDiffForFile(repo *Repository, startCommit, endCommit string, diff return fmt.Errorf("invalid diffType: %s", diffType) } - stderr := new(bytes.Buffer) - if err = cmd.WithDir(repo.Path). + return cmd.WithDir(repo.Path). WithStdout(writer). - WithStderr(stderr). - Run(repo.Ctx); err != nil { - return fmt.Errorf("Run: %w - %s", err, stderr) - } - return nil + RunWithStderr(repo.Ctx) } // ParseDiffHunkString parse the diff hunk content and return diff --git a/modules/git/gitcmd/command.go b/modules/git/gitcmd/command.go index f394ab7103..790c516383 100644 --- a/modules/git/gitcmd/command.go +++ b/modules/git/gitcmd/command.go @@ -40,6 +40,7 @@ const DefaultLocale = "C" // Command represents a command with its subcommands or arguments. type Command struct { + callerInfo string prog string args []string preErrors []error @@ -52,9 +53,10 @@ type Command struct { cmdFinished context.CancelFunc cmdStartTime time.Time - cmdStdinWriter *io.WriteCloser - cmdStdoutReader *io.ReadCloser - cmdStderrReader *io.ReadCloser + cmdStdinWriter *io.WriteCloser + cmdStdoutReader *io.ReadCloser + cmdStderrReader *io.ReadCloser + cmdManagedStderr *bytes.Buffer } func logArgSanitize(arg string) string { @@ -221,7 +223,7 @@ type runOpts struct { // The correct approach is to use `--git-dir" global argument Dir string - Stdout, Stderr io.Writer + Stdout io.Writer // Stdin is used for passing input to the command // The caller must make sure the Stdin writer is closed properly to finish the Run function. @@ -235,8 +237,6 @@ type runOpts struct { Stdin io.Reader PipelineFunc func(context.Context, context.CancelFunc) error - - callerInfo string } func commonBaseEnvs() []string { @@ -310,12 +310,6 @@ func (c *Command) WithStderrReader(r *io.ReadCloser) *Command { return c } -// WithStderr is deprecated, use WithStderrReader instead -func (c *Command) WithStderr(stderr io.Writer) *Command { - c.opts.Stderr = stderr - return c -} - func (c *Command) WithStdinWriter(w *io.WriteCloser) *Command { c.cmdStdinWriter = w return c @@ -343,11 +337,11 @@ func (c *Command) WithUseContextTimeout(useContextTimeout bool) *Command { // then you can to call this function in GeneralWrapperFunc to set the caller info of FeatureFunc. // The caller info can only be set once. func (c *Command) WithParentCallerInfo(optInfo ...string) *Command { - if c.opts.callerInfo != "" { + if c.callerInfo != "" { return c } if len(optInfo) > 0 { - c.opts.callerInfo = optInfo[0] + c.callerInfo = optInfo[0] return c } skip := 1 /*parent "wrap/run" functions*/ + 1 /*this function*/ @@ -356,7 +350,7 @@ func (c *Command) WithParentCallerInfo(optInfo ...string) *Command { if pos := strings.LastIndex(callerInfo, "/"); pos >= 0 { callerInfo = callerInfo[pos+1:] } - c.opts.callerInfo = callerInfo + c.callerInfo = callerInfo return c } @@ -372,7 +366,7 @@ func (c *Command) Start(ctx context.Context) (retErr error) { safeClosePtrCloser(c.cmdStdoutReader) safeClosePtrCloser(c.cmdStderrReader) safeClosePtrCloser(c.cmdStdinWriter) - // if no error, cmdFinished will be called in "Wait" function + // if error occurs, we must also finish the task, otherwise, cmdFinished will be called in "Wait" function if c.cmdFinished != nil { c.cmdFinished() } @@ -393,16 +387,16 @@ func (c *Command) Start(ctx context.Context) (retErr error) { } cmdLogString := c.LogString() - if c.opts.callerInfo == "" { + if c.callerInfo == "" { c.WithParentCallerInfo() } // these logs are for debugging purposes only, so no guarantee of correctness or stability - desc := fmt.Sprintf("git.Run(by:%s, repo:%s): %s", c.opts.callerInfo, logArgSanitize(c.opts.Dir), cmdLogString) + desc := fmt.Sprintf("git.Run(by:%s, repo:%s): %s", c.callerInfo, logArgSanitize(c.opts.Dir), cmdLogString) log.Debug("git.Command: %s", desc) _, span := gtprof.GetTracer().Start(ctx, gtprof.TraceSpanGitRun) defer span.End() - span.SetAttributeString(gtprof.TraceAttrFuncCaller, c.opts.callerInfo) + span.SetAttributeString(gtprof.TraceAttrFuncCaller, c.callerInfo) span.SetAttributeString(gtprof.TraceAttrGitCommand, cmdLogString) if c.opts.UseContextTimeout { @@ -425,7 +419,6 @@ func (c *Command) Start(ctx context.Context) (retErr error) { cmd.Env = append(cmd.Env, CommonGitCmdEnvs()...) cmd.Dir = c.opts.Dir cmd.Stdout = c.opts.Stdout - cmd.Stderr = c.opts.Stderr cmd.Stdin = c.opts.Stdin if _, err := safeAssignPipe(c.cmdStdinWriter, cmd.StdinPipe); err != nil { @@ -437,19 +430,32 @@ func (c *Command) Start(ctx context.Context) (retErr error) { if _, err := safeAssignPipe(c.cmdStderrReader, cmd.StderrPipe); err != nil { return err } + + if c.cmdManagedStderr != nil { + if cmd.Stderr != nil { + panic("CombineStderr needs managed (but not caller-provided) stderr pipe") + } + cmd.Stderr = c.cmdManagedStderr + } return cmd.Start() } func (c *Command) Wait() error { - defer c.cmdFinished() + defer func() { + safeClosePtrCloser(c.cmdStdoutReader) + safeClosePtrCloser(c.cmdStderrReader) + safeClosePtrCloser(c.cmdStdinWriter) + 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() - _ = cmd.Wait() - return err + errWait := cmd.Wait() + return errors.Join(err, errWait) } } @@ -472,6 +478,34 @@ func (c *Command) Wait() error { return errCause } +func (c *Command) StartWithStderr(ctx context.Context) RunStdError { + c.cmdManagedStderr = &bytes.Buffer{} + err := c.Start(ctx) + if err != nil { + return &runStdError{err: err} + } + return nil +} + +func (c *Command) WaitWithStderr() RunStdError { + if c.cmdManagedStderr == nil { + panic("CombineStderr needs managed (but not caller-provided) stderr pipe") + } + errWait := c.Wait() + if errWait == nil { + // if no exec error but only stderr output, the stderr output is still saved in "c.cmdManagedStderr" and can be read later + return nil + } + return &runStdError{err: errWait, stderr: util.UnsafeBytesToString(c.cmdManagedStderr.Bytes())} +} + +func (c *Command) RunWithStderr(ctx context.Context) RunStdError { + if err := c.StartWithStderr(ctx); err != nil { + return &runStdError{err: err} + } + return c.WaitWithStderr() +} + func (c *Command) Run(ctx context.Context) (err error) { if err = c.Start(ctx); err != nil { return err @@ -493,9 +527,9 @@ type runStdError struct { 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 lof of code only checks `strings.Contains(err.Error(), "git error")` + // But a lot of code only checks `strings.Contains(err.Error(), "git error")` if r.errMsg == "" { - r.errMsg = ConcatenateError(r.err, r.stderr).Error() + r.errMsg = fmt.Sprintf("%s - %s", r.err.Error(), strings.TrimSpace(r.stderr)) } return r.errMsg } @@ -543,24 +577,16 @@ func (c *Command) RunStdBytes(ctx context.Context) (stdout, stderr []byte, runEr return c.WithParentCallerInfo().runStdBytes(ctx) } -func (c *Command) runStdBytes(ctx context.Context) ( /*stdout*/ []byte /*stderr*/, []byte /*runErr*/, RunStdError) { - if c.opts.Stdout != nil || c.opts.Stderr != nil { +func (c *Command) runStdBytes(ctx context.Context) ([]byte, []byte, RunStdError) { + if c.opts.Stdout != nil || c.cmdStdoutReader != nil || c.cmdStderrReader != nil { // we must panic here, otherwise there would be bugs if developers set Stdin/Stderr by mistake, and it would be very difficult to debug panic("stdout and stderr field must be nil when using RunStdBytes") } stdoutBuf := &bytes.Buffer{} - stderrBuf := &bytes.Buffer{} err := c.WithParentCallerInfo(). WithStdout(stdoutBuf). - WithStderr(stderrBuf). - Run(ctx) - if err != nil { - // FIXME: GIT-CMD-STDERR: it is a bad design, the stderr should not be put in the error message - // But a lot of code depends on it, so we have to keep this behavior - return nil, stderrBuf.Bytes(), &runStdError{err: err, stderr: util.UnsafeBytesToString(stderrBuf.Bytes())} - } - // even if there is no err, there could still be some stderr output - return stdoutBuf.Bytes(), stderrBuf.Bytes(), nil + RunWithStderr(ctx) + return stdoutBuf.Bytes(), c.cmdManagedStderr.Bytes(), err } func (c *Command) DebugKill() { diff --git a/modules/git/gitcmd/command_test.go b/modules/git/gitcmd/command_test.go index 4a21154cf2..c227a0e210 100644 --- a/modules/git/gitcmd/command_test.go +++ b/modules/git/gitcmd/command_test.go @@ -45,7 +45,7 @@ func TestRunWithContextStd(t *testing.T) { assert.Equal(t, stderr, err.Stderr()) assert.Equal(t, "fatal: Not a valid object name no-such\n", err.Stderr()) // FIXME: GIT-CMD-STDERR: it is a bad design, the stderr should not be put in the error message - assert.Equal(t, "exit status 128 - fatal: Not a valid object name no-such\n", err.Error()) + assert.Equal(t, "exit status 128 - fatal: Not a valid object name no-such", err.Error()) assert.Empty(t, stdout) } } @@ -57,7 +57,7 @@ func TestRunWithContextStd(t *testing.T) { assert.Equal(t, string(stderr), err.Stderr()) assert.Equal(t, "fatal: Not a valid object name no-such\n", err.Stderr()) // FIXME: GIT-CMD-STDERR: it is a bad design, the stderr should not be put in the error message - assert.Equal(t, "exit status 128 - fatal: Not a valid object name no-such\n", err.Error()) + assert.Equal(t, "exit status 128 - fatal: Not a valid object name no-such", err.Error()) assert.Empty(t, stdout) } } diff --git a/modules/git/gitcmd/utils.go b/modules/git/gitcmd/utils.go index 5c7546d7be..0df3fc2c4b 100644 --- a/modules/git/gitcmd/utils.go +++ b/modules/git/gitcmd/utils.go @@ -4,22 +4,9 @@ package gitcmd import ( - "fmt" "io" - - "code.gitea.io/gitea/modules/util" ) -// ConcatenateError concatenates an error with stderr string -// FIXME: use RunStdError instead -func ConcatenateError(err error, stderr string) error { - if len(stderr) == 0 { - return err - } - errMsg := fmt.Sprintf("%s - %s", err.Error(), stderr) - return util.ErrorWrap(&runStdError{err: err, stderr: stderr, errMsg: errMsg}, "%s", errMsg) -} - func safeClosePtrCloser[T *io.ReadCloser | *io.WriteCloser](c T) { switch v := any(c).(type) { case *io.ReadCloser: diff --git a/modules/git/grep.go b/modules/git/grep.go index ed69a788a4..edd46f4314 100644 --- a/modules/git/grep.go +++ b/modules/git/grep.go @@ -5,11 +5,10 @@ package git import ( "bufio" - "bytes" "context" "errors" "fmt" - "os" + "io" "slices" "strconv" "strings" @@ -42,15 +41,6 @@ type GrepOptions struct { } func GrepSearch(ctx context.Context, repo *Repository, search string, opts GrepOptions) ([]*GrepResult, error) { - stdoutReader, stdoutWriter, err := os.Pipe() - if err != nil { - return nil, fmt.Errorf("unable to create os pipe to grep: %w", err) - } - defer func() { - _ = stdoutReader.Close() - _ = stdoutWriter.Close() - }() - /* The output is like this ( "^@" means \x00): @@ -83,12 +73,11 @@ func GrepSearch(ctx context.Context, repo *Repository, search string, opts GrepO cmd.AddDynamicArguments(util.IfZero(opts.RefName, "HEAD")) cmd.AddDashesAndList(opts.PathspecList...) opts.MaxResultLimit = util.IfZero(opts.MaxResultLimit, 50) - stderr := bytes.Buffer{} - err = cmd.WithDir(repo.Path). - WithStdout(stdoutWriter). - WithStderr(&stderr). + + var stdoutReader io.ReadCloser + err := cmd.WithDir(repo.Path). + WithStdoutReader(&stdoutReader). WithPipelineFunc(func(ctx context.Context, cancel context.CancelFunc) error { - _ = stdoutWriter.Close() defer stdoutReader.Close() isInBlock := false @@ -133,17 +122,17 @@ func GrepSearch(ctx context.Context, repo *Repository, search string, opts GrepO } return nil }). - Run(ctx) + RunWithStderr(ctx) // git grep exits by cancel (killed), usually it is caused by the limit of results - if gitcmd.IsErrorExitCode(err, -1) && stderr.Len() == 0 { + if gitcmd.IsErrorExitCode(err, -1) && err.Stderr() == "" { return results, nil } // git grep exits with 1 if no results are found - if gitcmd.IsErrorExitCode(err, 1) && stderr.Len() == 0 { + if gitcmd.IsErrorExitCode(err, 1) && err.Stderr() == "" { return nil, nil } if err != nil && !errors.Is(err, context.Canceled) { - return nil, fmt.Errorf("unable to run git grep: %w, stderr: %s", err, stderr.String()) + return nil, fmt.Errorf("unable to run git grep: %w", err) } return results, nil } diff --git a/modules/git/log_name_status.go b/modules/git/log_name_status.go index 72e513000b..129f3c6c05 100644 --- a/modules/git/log_name_status.go +++ b/modules/git/log_name_status.go @@ -64,20 +64,12 @@ func LogNameStatusRepo(ctx context.Context, repository, head, treepath string, p cmd.AddDashesAndList(files...) go func() { - stderr := strings.Builder{} err := cmd.WithDir(repository). WithStdout(stdoutWriter). - WithStderr(&stderr). - Run(ctx) - if err != nil { - _ = stdoutWriter.CloseWithError(gitcmd.ConcatenateError(err, (&stderr).String())) - return - } - - _ = stdoutWriter.Close() + RunWithStderr(ctx) + _ = stdoutWriter.CloseWithError(err) }() - // For simplicities sake we'll us a buffered reader to read from the cat-file --batch bufReader := bufio.NewReaderSize(stdoutReader, 32*1024) return bufReader, cancel diff --git a/modules/git/pipeline/catfile.go b/modules/git/pipeline/catfile.go index a4d1ff64cf..ccd6884a6a 100644 --- a/modules/git/pipeline/catfile.go +++ b/modules/git/pipeline/catfile.go @@ -5,7 +5,6 @@ package pipeline import ( "bufio" - "bytes" "context" "fmt" "io" @@ -14,7 +13,6 @@ import ( "sync" "code.gitea.io/gitea/modules/git/gitcmd" - "code.gitea.io/gitea/modules/log" ) // CatFileBatchCheck runs cat-file with --batch-check @@ -23,15 +21,12 @@ func CatFileBatchCheck(ctx context.Context, shasToCheckReader *io.PipeReader, ca defer shasToCheckReader.Close() defer catFileCheckWriter.Close() - stderr := new(bytes.Buffer) - var errbuf strings.Builder cmd := gitcmd.NewCommand("cat-file", "--batch-check") if err := cmd.WithDir(tmpBasePath). WithStdin(shasToCheckReader). WithStdout(catFileCheckWriter). - WithStderr(stderr). - Run(ctx); err != nil { - _ = catFileCheckWriter.CloseWithError(fmt.Errorf("git cat-file --batch-check [%s]: %w - %s", tmpBasePath, err, errbuf.String())) + RunWithStderr(ctx); err != nil { + _ = catFileCheckWriter.CloseWithError(fmt.Errorf("git cat-file --batch-check [%s]: %w", tmpBasePath, err)) } } @@ -40,16 +35,11 @@ func CatFileBatchCheckAllObjects(ctx context.Context, catFileCheckWriter *io.Pip defer wg.Done() defer catFileCheckWriter.Close() - stderr := new(bytes.Buffer) - var errbuf strings.Builder cmd := gitcmd.NewCommand("cat-file", "--batch-check", "--batch-all-objects") if err := cmd.WithDir(tmpBasePath). WithStdout(catFileCheckWriter). - WithStderr(stderr). - Run(ctx); err != nil { - log.Error("git cat-file --batch-check --batch-all-object [%s]: %v - %s", tmpBasePath, err, errbuf.String()) - err = fmt.Errorf("git cat-file --batch-check --batch-all-object [%s]: %w - %s", tmpBasePath, err, errbuf.String()) - _ = catFileCheckWriter.CloseWithError(err) + RunWithStderr(ctx); err != nil { + _ = catFileCheckWriter.CloseWithError(fmt.Errorf("git cat-file --batch-check --batch-all-object [%s]: %w", tmpBasePath, err)) errChan <- err } } @@ -60,15 +50,12 @@ func CatFileBatch(ctx context.Context, shasToBatchReader *io.PipeReader, catFile defer shasToBatchReader.Close() defer catFileBatchWriter.Close() - stderr := new(bytes.Buffer) - var errbuf strings.Builder if err := gitcmd.NewCommand("cat-file", "--batch"). WithDir(tmpBasePath). WithStdin(shasToBatchReader). WithStdout(catFileBatchWriter). - WithStderr(stderr). - Run(ctx); err != nil { - _ = shasToBatchReader.CloseWithError(fmt.Errorf("git rev-list [%s]: %w - %s", tmpBasePath, err, errbuf.String())) + RunWithStderr(ctx); err != nil { + _ = shasToBatchReader.CloseWithError(fmt.Errorf("git rev-list [%s]: %w", tmpBasePath, err)) } } diff --git a/modules/git/pipeline/lfs_nogogit.go b/modules/git/pipeline/lfs_nogogit.go index 417af27037..41282d29a9 100644 --- a/modules/git/pipeline/lfs_nogogit.go +++ b/modules/git/pipeline/lfs_nogogit.go @@ -33,17 +33,11 @@ func FindLFSFile(repo *git.Repository, objectID git.ObjectID) ([]*LFSResult, err }() go func() { - stderr := strings.Builder{} err := gitcmd.NewCommand("rev-list", "--all"). WithDir(repo.Path). WithStdout(revListWriter). - WithStderr(&stderr). - Run(repo.Ctx) - if err != nil { - _ = revListWriter.CloseWithError(gitcmd.ConcatenateError(err, (&stderr).String())) - } else { - _ = revListWriter.Close() - } + RunWithStderr(repo.Ctx) + _ = revListWriter.CloseWithError(err) }() // Next feed the commits in order into cat-file --batch, followed by their trees and sub trees as necessary. diff --git a/modules/git/pipeline/namerev.go b/modules/git/pipeline/namerev.go index 782b5f0531..39f7f31900 100644 --- a/modules/git/pipeline/namerev.go +++ b/modules/git/pipeline/namerev.go @@ -4,11 +4,9 @@ package pipeline import ( - "bytes" "context" "fmt" "io" - "strings" "sync" "code.gitea.io/gitea/modules/git/gitcmd" @@ -20,14 +18,11 @@ func NameRevStdin(ctx context.Context, shasToNameReader *io.PipeReader, nameRevS defer shasToNameReader.Close() defer nameRevStdinWriter.Close() - stderr := new(bytes.Buffer) - var errbuf strings.Builder if err := gitcmd.NewCommand("name-rev", "--stdin", "--name-only", "--always"). WithDir(tmpBasePath). WithStdin(shasToNameReader). WithStdout(nameRevStdinWriter). - WithStderr(stderr). - Run(ctx); err != nil { - _ = shasToNameReader.CloseWithError(fmt.Errorf("git name-rev [%s]: %w - %s", tmpBasePath, err, errbuf.String())) + RunWithStderr(ctx); err != nil { + _ = shasToNameReader.CloseWithError(fmt.Errorf("git name-rev [%s]: %w", tmpBasePath, err)) } } diff --git a/modules/git/pipeline/revlist.go b/modules/git/pipeline/revlist.go index 755b165a65..ead562c9e9 100644 --- a/modules/git/pipeline/revlist.go +++ b/modules/git/pipeline/revlist.go @@ -5,7 +5,6 @@ package pipeline import ( "bufio" - "bytes" "context" "fmt" "io" @@ -13,7 +12,6 @@ import ( "sync" "code.gitea.io/gitea/modules/git/gitcmd" - "code.gitea.io/gitea/modules/log" ) // RevListAllObjects runs rev-list --objects --all and writes to a pipewriter @@ -21,16 +19,11 @@ func RevListAllObjects(ctx context.Context, revListWriter *io.PipeWriter, wg *sy defer wg.Done() defer revListWriter.Close() - stderr := new(bytes.Buffer) - var errbuf strings.Builder cmd := gitcmd.NewCommand("rev-list", "--objects", "--all") if err := cmd.WithDir(basePath). WithStdout(revListWriter). - WithStderr(stderr). - Run(ctx); err != nil { - log.Error("git rev-list --objects --all [%s]: %v - %s", basePath, err, errbuf.String()) - err = fmt.Errorf("git rev-list --objects --all [%s]: %w - %s", basePath, err, errbuf.String()) - _ = revListWriter.CloseWithError(err) + RunWithStderr(ctx); err != nil { + _ = revListWriter.CloseWithError(fmt.Errorf("git rev-list --objects --all [%s]: %w", basePath, err)) errChan <- err } } @@ -39,18 +32,15 @@ func RevListAllObjects(ctx context.Context, revListWriter *io.PipeWriter, wg *sy func RevListObjects(ctx context.Context, revListWriter *io.PipeWriter, wg *sync.WaitGroup, tmpBasePath, headSHA, baseSHA string, errChan chan<- error) { defer wg.Done() defer revListWriter.Close() - stderr := new(bytes.Buffer) - var errbuf strings.Builder + cmd := gitcmd.NewCommand("rev-list", "--objects").AddDynamicArguments(headSHA) if baseSHA != "" { cmd = cmd.AddArguments("--not").AddDynamicArguments(baseSHA) } if err := cmd.WithDir(tmpBasePath). WithStdout(revListWriter). - WithStderr(stderr). - Run(ctx); err != nil { - log.Error("git rev-list [%s]: %v - %s", tmpBasePath, err, errbuf.String()) - errChan <- fmt.Errorf("git rev-list [%s]: %w - %s", tmpBasePath, err, errbuf.String()) + RunWithStderr(ctx); err != nil { + errChan <- fmt.Errorf("git rev-list [%s]: %w", tmpBasePath, err) } } diff --git a/modules/git/repo.go b/modules/git/repo.go index bea599e22e..1e31eb1b80 100644 --- a/modules/git/repo.go +++ b/modules/git/repo.go @@ -8,7 +8,6 @@ import ( "bytes" "context" "fmt" - "io" "net/url" "os" "path" @@ -83,22 +82,19 @@ func InitRepository(ctx context.Context, repoPath string, bare bool, objectForma // IsEmpty Check if repository is empty. func (repo *Repository) IsEmpty() (bool, error) { - var errbuf, output strings.Builder - if err := gitcmd.NewCommand(). + stdout, _, err := gitcmd.NewCommand(). AddOptionFormat("--git-dir=%s", repo.Path). AddArguments("rev-list", "-n", "1", "--all"). WithDir(repo.Path). - WithStdout(&output). - WithStderr(&errbuf). - Run(repo.Ctx); err != nil { - if (err.Error() == "exit status 1" && strings.TrimSpace(errbuf.String()) == "") || err.Error() == "exit status 129" { + RunStdString(repo.Ctx) + if err != nil { + if (gitcmd.IsErrorExitCode(err, 1) && err.Stderr() == "") || gitcmd.IsErrorExitCode(err, 129) { // git 2.11 exits with 129 if the repo is empty return true, nil } - return true, fmt.Errorf("check empty: %w - %s", err, errbuf.String()) + return true, fmt.Errorf("check empty: %w", err) } - - return strings.TrimSpace(output.String()) == "", nil + return strings.TrimSpace(stdout) == "", nil } // CloneRepoOptions options when clone a repository @@ -171,16 +167,10 @@ func Clone(ctx context.Context, from, to string, opts CloneRepoOptions) error { } } - stderr := new(bytes.Buffer) - if err := cmd. + return cmd. WithTimeout(opts.Timeout). WithEnv(envs). - WithStdout(io.Discard). - WithStderr(stderr). - Run(ctx); err != nil { - return gitcmd.ConcatenateError(err, stderr.String()) - } - return nil + RunWithStderr(ctx) } // PushOptions options when push to remote diff --git a/modules/git/repo_archive.go b/modules/git/repo_archive.go index 8a9eec9e6a..2bbe3f0bb3 100644 --- a/modules/git/repo_archive.go +++ b/modules/git/repo_archive.go @@ -62,13 +62,7 @@ func (repo *Repository) CreateArchive(ctx context.Context, format ArchiveType, t cmd.AddOptionFormat("--format=%s", format.String()) cmd.AddDynamicArguments(commitID) - var stderr strings.Builder - err := cmd.WithDir(repo.Path). + return cmd.WithDir(repo.Path). WithStdout(target). - WithStderr(&stderr). - Run(ctx) - if err != nil { - return gitcmd.ConcatenateError(err, stderr.String()) - } - return nil + RunWithStderr(ctx) } diff --git a/modules/git/repo_branch_nogogit.go b/modules/git/repo_branch_nogogit.go index 7eb8c48466..f8a27455d5 100644 --- a/modules/git/repo_branch_nogogit.go +++ b/modules/git/repo_branch_nogogit.go @@ -101,23 +101,13 @@ func WalkShowRef(ctx context.Context, repoPath string, extraArgs gitcmd.TrustedC }() go func() { - stderrBuilder := &strings.Builder{} args := gitcmd.TrustedCmdArgs{"for-each-ref", "--format=%(objectname) %(refname)"} args = append(args, extraArgs...) err := gitcmd.NewCommand(args...). WithDir(repoPath). WithStdout(stdoutWriter). - WithStderr(stderrBuilder). - Run(ctx) - if err != nil { - if stderrBuilder.Len() == 0 { - _ = stdoutWriter.Close() - return - } - _ = stdoutWriter.CloseWithError(gitcmd.ConcatenateError(err, stderrBuilder.String())) - } else { - _ = stdoutWriter.Close() - } + RunWithStderr(ctx) + _ = stdoutWriter.CloseWithError(err) }() i := 0 diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go index 4a441429f4..bf3f7238d2 100644 --- a/modules/git/repo_commit.go +++ b/modules/git/repo_commit.go @@ -232,7 +232,6 @@ func (repo *Repository) CommitsByFileAndRange(opts CommitsByFileAndRangeOptions) _ = stdoutWriter.Close() }() go func() { - stderr := strings.Builder{} gitCmd := gitcmd.NewCommand("rev-list"). AddOptionFormat("--max-count=%d", setting.Git.CommitsRangeSize). AddOptionFormat("--skip=%d", (opts.Page-1)*setting.Git.CommitsRangeSize) @@ -251,13 +250,8 @@ func (repo *Repository) CommitsByFileAndRange(opts CommitsByFileAndRangeOptions) gitCmd.AddDashesAndList(opts.File) err := gitCmd.WithDir(repo.Path). WithStdout(stdoutWriter). - WithStderr(&stderr). - Run(repo.Ctx) - if err != nil { - _ = stdoutWriter.CloseWithError(gitcmd.ConcatenateError(err, (&stderr).String())) - } else { - _ = stdoutWriter.Close() - } + RunWithStderr(repo.Ctx) + _ = stdoutWriter.CloseWithError(err) }() objectFormat, err := repo.GetObjectFormat() diff --git a/modules/git/repo_compare.go b/modules/git/repo_compare.go index 8d26f1e9e7..f78d569b83 100644 --- a/modules/git/repo_compare.go +++ b/modules/git/repo_compare.go @@ -34,7 +34,6 @@ func (l *lineCountWriter) Write(p []byte) (n int, err error) { func (repo *Repository) GetDiffNumChangedFiles(base, head string, directComparison bool) (int, error) { // Now there is git diff --shortstat but this appears to be slower than simply iterating with --nameonly w := &lineCountWriter{} - stderr := new(bytes.Buffer) separator := "..." if directComparison { @@ -47,24 +46,21 @@ func (repo *Repository) GetDiffNumChangedFiles(base, head string, directComparis AddArguments("--"). WithDir(repo.Path). WithStdout(w). - WithStderr(stderr). - Run(repo.Ctx); err != nil { - if strings.Contains(stderr.String(), "no merge base") { + RunWithStderr(repo.Ctx); err != nil { + if strings.Contains(err.Stderr(), "no merge base") { // git >= 2.28 now returns an error if base and head have become unrelated. // previously it would return the results of git diff -z --name-only base head so let's try that... w = &lineCountWriter{} - stderr.Reset() if err = gitcmd.NewCommand("diff", "-z", "--name-only"). AddDynamicArguments(base, head). AddArguments("--"). WithDir(repo.Path). WithStdout(w). - WithStderr(stderr). - Run(repo.Ctx); err == nil { + RunWithStderr(repo.Ctx); err == nil { return w.numLines, nil } } - return 0, fmt.Errorf("%w: Stderr: %s", err, stderr) + return 0, err } return w.numLines, nil } @@ -73,11 +69,9 @@ var patchCommits = regexp.MustCompile(`^From\s(\w+)\s`) // GetDiff generates and returns patch data between given revisions, optimized for human readability func (repo *Repository) GetDiff(compareArg string, w io.Writer) error { - stderr := new(bytes.Buffer) return gitcmd.NewCommand("diff", "-p").AddDynamicArguments(compareArg). WithDir(repo.Path). WithStdout(w). - WithStderr(stderr). Run(repo.Ctx) } @@ -92,11 +86,9 @@ func (repo *Repository) GetDiffBinary(compareArg string, w io.Writer) error { // GetPatch generates and returns format-patch data between given revisions, able to be used with `git apply` func (repo *Repository) GetPatch(compareArg string, w io.Writer) error { - stderr := new(bytes.Buffer) return gitcmd.NewCommand("format-patch", "--binary", "--stdout").AddDynamicArguments(compareArg). WithDir(repo.Path). WithStdout(w). - WithStderr(stderr). Run(repo.Ctx) } diff --git a/modules/git/repo_index.go b/modules/git/repo_index.go index 4068f86bb2..4e62198570 100644 --- a/modules/git/repo_index.go +++ b/modules/git/repo_index.go @@ -101,21 +101,17 @@ func (repo *Repository) RemoveFilesFromIndex(filenames ...string) error { return err } cmd := gitcmd.NewCommand("update-index", "--remove", "-z", "--index-info") - stdout := new(bytes.Buffer) - stderr := new(bytes.Buffer) - buffer := new(bytes.Buffer) + input := new(bytes.Buffer) for _, file := range filenames { if file != "" { // using format: mode SP type SP sha1 TAB path - buffer.WriteString("0 blob " + objectFormat.EmptyObjectID().String() + "\t" + file + "\000") + input.WriteString("0 blob " + objectFormat.EmptyObjectID().String() + "\t" + file + "\000") } } return cmd. WithDir(repo.Path). - WithStdin(bytes.NewReader(buffer.Bytes())). - WithStdout(stdout). - WithStderr(stderr). - Run(repo.Ctx) + WithStdin(bytes.NewReader(input.Bytes())). + RunWithStderr(repo.Ctx) } type IndexObjectInfo struct { @@ -127,19 +123,15 @@ type IndexObjectInfo struct { // AddObjectsToIndex adds the provided object hashes to the index at the provided filenames func (repo *Repository) AddObjectsToIndex(objects ...IndexObjectInfo) error { cmd := gitcmd.NewCommand("update-index", "--add", "--replace", "-z", "--index-info") - stdout := new(bytes.Buffer) - stderr := new(bytes.Buffer) - buffer := new(bytes.Buffer) + input := new(bytes.Buffer) for _, object := range objects { // using format: mode SP type SP sha1 TAB path - buffer.WriteString(object.Mode + " blob " + object.Object.String() + "\t" + object.Filename + "\000") + input.WriteString(object.Mode + " blob " + object.Object.String() + "\t" + object.Filename + "\000") } return cmd. WithDir(repo.Path). - WithStdin(bytes.NewReader(buffer.Bytes())). - WithStdout(stdout). - WithStderr(stderr). - Run(repo.Ctx) + WithStdin(bytes.NewReader(input.Bytes())). + RunWithStderr(repo.Ctx) } // AddObjectToIndex adds the provided object hash to the index at the provided filename diff --git a/modules/git/repo_object.go b/modules/git/repo_object.go index 2a39a3c4d8..fbaaa707e7 100644 --- a/modules/git/repo_object.go +++ b/modules/git/repo_object.go @@ -5,7 +5,6 @@ package git import ( - "bytes" "io" "strings" @@ -74,16 +73,12 @@ func (repo *Repository) hashObject(reader io.Reader, save bool) (string, error) } else { cmd = gitcmd.NewCommand("hash-object", "--stdin") } - stdout := new(bytes.Buffer) - stderr := new(bytes.Buffer) - err := cmd. + stdout, _, err := cmd. WithDir(repo.Path). WithStdin(reader). - WithStdout(stdout). - WithStderr(stderr). - Run(repo.Ctx) + RunStdString(repo.Ctx) if err != nil { return "", err } - return strings.TrimSpace(stdout.String()), nil + return strings.TrimSpace(stdout), nil } diff --git a/modules/git/repo_ref_nogogit.go b/modules/git/repo_ref_nogogit.go index 09bb0df7b8..a5737fb859 100644 --- a/modules/git/repo_ref_nogogit.go +++ b/modules/git/repo_ref_nogogit.go @@ -22,17 +22,11 @@ func (repo *Repository) GetRefsFiltered(pattern string) ([]*Reference, error) { }() go func() { - stderrBuilder := &strings.Builder{} err := gitcmd.NewCommand("for-each-ref"). WithDir(repo.Path). WithStdout(stdoutWriter). - WithStderr(stderrBuilder). Run(repo.Ctx) - if err != nil { - _ = stdoutWriter.CloseWithError(gitcmd.ConcatenateError(err, stderrBuilder.String())) - } else { - _ = stdoutWriter.Close() - } + _ = stdoutWriter.CloseWithError(err) }() refs := make([]*Reference, 0) diff --git a/modules/git/repo_stats.go b/modules/git/repo_stats.go index cfb35288fe..175f5592c2 100644 --- a/modules/git/repo_stats.go +++ b/modules/git/repo_stats.go @@ -72,11 +72,9 @@ func (repo *Repository) GetCodeActivityStats(fromTime time.Time, branch string) gitCmd.AddArguments("--first-parent").AddDynamicArguments(branch) } - stderr := new(strings.Builder) err = gitCmd. WithDir(repo.Path). WithStdout(stdoutWriter). - WithStderr(stderr). WithPipelineFunc(func(ctx context.Context, cancel context.CancelFunc) error { _ = stdoutWriter.Close() scanner := bufio.NewScanner(stdoutReader) @@ -146,9 +144,9 @@ func (repo *Repository) GetCodeActivityStats(fromTime time.Time, branch string) _ = stdoutReader.Close() return nil }). - Run(repo.Ctx) + RunWithStderr(repo.Ctx) if err != nil { - return nil, fmt.Errorf("Failed to get GetCodeActivityStats for repository.\nError: %w\nStderr: %s", err, stderr) + return nil, fmt.Errorf("GetCodeActivityStats: %w", err) } return stats, nil diff --git a/modules/git/repo_tag.go b/modules/git/repo_tag.go index 4ad0c6e5ab..dacf9b40ec 100644 --- a/modules/git/repo_tag.go +++ b/modules/git/repo_tag.go @@ -118,7 +118,6 @@ func (repo *Repository) GetTagInfos(page, pageSize int) ([]*Tag, int, error) { stdoutReader, stdoutWriter := io.Pipe() defer stdoutReader.Close() defer stdoutWriter.Close() - stderr := strings.Builder{} go func() { err := gitcmd.NewCommand("for-each-ref"). @@ -126,13 +125,8 @@ func (repo *Repository) GetTagInfos(page, pageSize int) ([]*Tag, int, error) { AddArguments("--sort", "-*creatordate", "refs/tags"). WithDir(repo.Path). WithStdout(stdoutWriter). - WithStderr(&stderr). - Run(repo.Ctx) - if err != nil { - _ = stdoutWriter.CloseWithError(gitcmd.ConcatenateError(err, stderr.String())) - } else { - _ = stdoutWriter.Close() - } + RunWithStderr(repo.Ctx) + _ = stdoutWriter.CloseWithError(err) }() var tags []*Tag diff --git a/modules/git/repo_tree.go b/modules/git/repo_tree.go index 964342ba00..9b119e8426 100644 --- a/modules/git/repo_tree.go +++ b/modules/git/repo_tree.go @@ -58,16 +58,12 @@ func (repo *Repository) CommitTree(author, committer *Signature, tree *Tree, opt cmd.AddArguments("--no-gpg-sign") } - stdout := new(bytes.Buffer) - stderr := new(bytes.Buffer) - err := cmd.WithEnv(env). + stdout, _, err := cmd.WithEnv(env). WithDir(repo.Path). WithStdin(messageBytes). - WithStdout(stdout). - WithStderr(stderr). - Run(repo.Ctx) + RunStdString(repo.Ctx) if err != nil { - return nil, gitcmd.ConcatenateError(err, stderr.String()) + return nil, err } - return NewIDFromString(strings.TrimSpace(stdout.String())) + return NewIDFromString(strings.TrimSpace(stdout)) } diff --git a/modules/gitrepo/archive.go b/modules/gitrepo/archive.go index 086dc86344..138639ec47 100644 --- a/modules/gitrepo/archive.go +++ b/modules/gitrepo/archive.go @@ -36,12 +36,7 @@ func CreateArchive(ctx context.Context, repo Repository, format string, target i paths[i] = path.Clean(paths[i]) } cmd.AddDynamicArguments(paths...) - - var stderr strings.Builder - if err := RunCmd(ctx, repo, cmd.WithStdout(target).WithStderr(&stderr)); err != nil { - return gitcmd.ConcatenateError(err, stderr.String()) - } - return nil + return RunCmdWithStderr(ctx, repo, cmd.WithStdout(target)) } // CreateBundle create bundle content to the target path diff --git a/modules/gitrepo/blame.go b/modules/gitrepo/blame.go index bd64c748d4..a2cb7ac1b7 100644 --- a/modules/gitrepo/blame.go +++ b/modules/gitrepo/blame.go @@ -12,16 +12,16 @@ import ( "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/git/gitcmd" - "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" ) func LineBlame(ctx context.Context, repo Repository, revision, file string, line uint) (string, error) { - return RunCmdString(ctx, repo, + stdout, _, err := RunCmdString(ctx, repo, gitcmd.NewCommand("blame"). AddOptionFormat("-L %d,%d", line, line). AddOptionValues("-p", revision). AddDashesAndList(file)) + return stdout, err } // BlamePart represents block of blame - continuous lines with one sha @@ -173,17 +173,10 @@ func CreateBlameReader(ctx context.Context, objectFormat git.ObjectFormat, repo return nil, err } go func() { - stderr := bytes.Buffer{} // 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 := RunCmd(ctx, repo, cmd.WithUseContextTimeout(true). - WithStdout(stdout). - WithStderr(&stderr), - ) + err := RunCmdWithStderr(ctx, repo, cmd.WithUseContextTimeout(true).WithStdout(stdout)) done <- err _ = stdout.Close() - if err != nil { - log.Error("Error running git blame (dir: %v): %v, stderr: %v", repoPath, err, stderr.String()) - } }() bufferedReader := bufio.NewReader(reader) diff --git a/modules/gitrepo/branch.go b/modules/gitrepo/branch.go index e05d75caf8..4c40d1fba3 100644 --- a/modules/gitrepo/branch.go +++ b/modules/gitrepo/branch.go @@ -36,14 +36,14 @@ func GetBranchCommitID(ctx context.Context, repo Repository, branch string) (str // SetDefaultBranch sets default branch of repository. func SetDefaultBranch(ctx context.Context, repo Repository, name string) error { - _, err := RunCmdString(ctx, repo, gitcmd.NewCommand("symbolic-ref", "HEAD"). + _, _, err := RunCmdString(ctx, repo, gitcmd.NewCommand("symbolic-ref", "HEAD"). AddDynamicArguments(git.BranchPrefix+name)) return err } // GetDefaultBranch gets default branch of repository. func GetDefaultBranch(ctx context.Context, repo Repository) (string, error) { - stdout, err := RunCmdString(ctx, repo, gitcmd.NewCommand("symbolic-ref", "HEAD")) + stdout, _, err := RunCmdString(ctx, repo, gitcmd.NewCommand("symbolic-ref", "HEAD")) if err != nil { return "", err } @@ -56,7 +56,7 @@ func GetDefaultBranch(ctx context.Context, repo Repository) (string, error) { // IsReferenceExist returns true if given reference exists in the repository. func IsReferenceExist(ctx context.Context, repo Repository, name string) bool { - _, err := RunCmdString(ctx, repo, gitcmd.NewCommand("show-ref", "--verify").AddDashesAndList(name)) + _, _, err := RunCmdString(ctx, repo, gitcmd.NewCommand("show-ref", "--verify").AddDashesAndList(name)) return err == nil } @@ -76,7 +76,7 @@ func DeleteBranch(ctx context.Context, repo Repository, name string, force bool) } cmd.AddDashesAndList(name) - _, err := RunCmdString(ctx, repo, cmd) + _, _, err := RunCmdString(ctx, repo, cmd) return err } @@ -85,12 +85,12 @@ func CreateBranch(ctx context.Context, repo Repository, branch, oldbranchOrCommi cmd := gitcmd.NewCommand("branch") cmd.AddDashesAndList(branch, oldbranchOrCommit) - _, err := RunCmdString(ctx, repo, cmd) + _, _, err := RunCmdString(ctx, repo, cmd) return err } // RenameBranch rename a branch func RenameBranch(ctx context.Context, repo Repository, from, to string) error { - _, err := RunCmdString(ctx, repo, gitcmd.NewCommand("branch", "-m").AddDynamicArguments(from, to)) + _, _, err := RunCmdString(ctx, repo, gitcmd.NewCommand("branch", "-m").AddDynamicArguments(from, to)) return err } diff --git a/modules/gitrepo/command.go b/modules/gitrepo/command.go index d4cb6093fc..fd21b9a725 100644 --- a/modules/gitrepo/command.go +++ b/modules/gitrepo/command.go @@ -13,11 +13,14 @@ func RunCmd(ctx context.Context, repo Repository, cmd *gitcmd.Command) error { return cmd.WithDir(repoPath(repo)).WithParentCallerInfo().Run(ctx) } -func RunCmdString(ctx context.Context, repo Repository, cmd *gitcmd.Command) (string, error) { - res, _, err := cmd.WithDir(repoPath(repo)).WithParentCallerInfo().RunStdString(ctx) - return res, err +func RunCmdString(ctx context.Context, repo Repository, cmd *gitcmd.Command) (string, string, gitcmd.RunStdError) { + return cmd.WithDir(repoPath(repo)).WithParentCallerInfo().RunStdString(ctx) } -func RunCmdBytes(ctx context.Context, repo Repository, cmd *gitcmd.Command) ([]byte, []byte, error) { +func RunCmdBytes(ctx context.Context, repo Repository, cmd *gitcmd.Command) ([]byte, []byte, gitcmd.RunStdError) { return cmd.WithDir(repoPath(repo)).WithParentCallerInfo().RunStdBytes(ctx) } + +func RunCmdWithStderr(ctx context.Context, repo Repository, cmd *gitcmd.Command) gitcmd.RunStdError { + return cmd.WithDir(repoPath(repo)).WithParentCallerInfo().RunWithStderr(ctx) +} diff --git a/modules/gitrepo/commit.go b/modules/gitrepo/commit.go index da0f3b85a2..0ab17862fe 100644 --- a/modules/gitrepo/commit.go +++ b/modules/gitrepo/commit.go @@ -88,7 +88,7 @@ func AllCommitsCount(ctx context.Context, repo Repository, hidePRRefs bool, file cmd.AddDashesAndList(files...) } - stdout, err := RunCmdString(ctx, repo, cmd) + stdout, _, err := RunCmdString(ctx, repo, cmd) if err != nil { return 0, err } @@ -102,7 +102,7 @@ func GetFullCommitID(ctx context.Context, repo Repository, shortID string) (stri // GetLatestCommitTime returns time for latest commit in repository (across all branches) func GetLatestCommitTime(ctx context.Context, repo Repository) (time.Time, error) { - stdout, err := RunCmdString(ctx, repo, + stdout, _, err := RunCmdString(ctx, repo, gitcmd.NewCommand("for-each-ref", "--sort=-committerdate", git.BranchPrefix, "--count", "1", "--format=%(committerdate)")) if err != nil { return time.Time{}, err diff --git a/modules/gitrepo/commit_file.go b/modules/gitrepo/commit_file.go index cd4bb340d0..0cd4f99130 100644 --- a/modules/gitrepo/commit_file.go +++ b/modules/gitrepo/commit_file.go @@ -5,7 +5,6 @@ package gitrepo import ( "bufio" - "bytes" "context" "io" @@ -76,16 +75,14 @@ func GetCommitFileStatus(ctx context.Context, repo Repository, commitID string) close(done) }() - stderr := new(bytes.Buffer) err := gitcmd.NewCommand("log", "--name-status", "-m", "--pretty=format:", "--first-parent", "--no-renames", "-z", "-1"). AddDynamicArguments(commitID). WithDir(repoPath(repo)). WithStdout(w). - WithStderr(stderr). - Run(ctx) - w.Close() // Close writer to exit parsing goroutine + RunWithStderr(ctx) + _ = w.Close() // Close writer to exit parsing goroutine if err != nil { - return nil, gitcmd.ConcatenateError(err, stderr.String()) + return nil, err } <-done diff --git a/modules/gitrepo/compare.go b/modules/gitrepo/compare.go index b8e4c30d6c..06cf880d99 100644 --- a/modules/gitrepo/compare.go +++ b/modules/gitrepo/compare.go @@ -22,7 +22,7 @@ type DivergeObject struct { func GetDivergingCommits(ctx context.Context, repo Repository, baseBranch, targetBranch string) (*DivergeObject, error) { cmd := gitcmd.NewCommand("rev-list", "--count", "--left-right"). AddDynamicArguments(baseBranch + "..." + targetBranch).AddArguments("--") - stdout, err1 := RunCmdString(ctx, repo, cmd) + stdout, _, err1 := RunCmdString(ctx, repo, cmd) if err1 != nil { return nil, err1 } diff --git a/modules/gitrepo/config.go b/modules/gitrepo/config.go index bc1746fc3f..9be3ef94ae 100644 --- a/modules/gitrepo/config.go +++ b/modules/gitrepo/config.go @@ -12,7 +12,7 @@ import ( ) func GitConfigGet(ctx context.Context, repo Repository, key string) (string, error) { - result, err := RunCmdString(ctx, repo, gitcmd.NewCommand("config", "--get"). + result, _, err := RunCmdString(ctx, repo, gitcmd.NewCommand("config", "--get"). AddDynamicArguments(key)) if err != nil { return "", err @@ -27,7 +27,7 @@ func getRepoConfigLockKey(repoStoragePath string) string { // GitConfigAdd add a git configuration key to a specific value for the given repository. func GitConfigAdd(ctx context.Context, repo Repository, key, value string) error { return globallock.LockAndDo(ctx, getRepoConfigLockKey(repo.RelativePath()), func(ctx context.Context) error { - _, err := RunCmdString(ctx, repo, gitcmd.NewCommand("config", "--add"). + _, _, err := RunCmdString(ctx, repo, gitcmd.NewCommand("config", "--add"). AddDynamicArguments(key, value)) return err }) @@ -38,7 +38,7 @@ func GitConfigAdd(ctx context.Context, repo Repository, key, value string) error // If the key exists, it will be updated to the new value. func GitConfigSet(ctx context.Context, repo Repository, key, value string) error { return globallock.LockAndDo(ctx, getRepoConfigLockKey(repo.RelativePath()), func(ctx context.Context) error { - _, err := RunCmdString(ctx, repo, gitcmd.NewCommand("config"). + _, _, err := RunCmdString(ctx, repo, gitcmd.NewCommand("config"). AddDynamicArguments(key, value)) return err }) diff --git a/modules/gitrepo/diff.go b/modules/gitrepo/diff.go index ad7f24762f..e1be47904a 100644 --- a/modules/gitrepo/diff.go +++ b/modules/gitrepo/diff.go @@ -4,7 +4,6 @@ package gitrepo import ( - "bytes" "context" "fmt" "io" @@ -22,7 +21,7 @@ func GetDiffShortStatByCmdArgs(ctx context.Context, repo Repository, trustedArgs // we get: // " 9902 files changed, 2034198 insertions(+), 298800 deletions(-)\n" cmd := gitcmd.NewCommand("diff", "--shortstat").AddArguments(trustedArgs...).AddDynamicArguments(dynamicArgs...) - stdout, err := RunCmdString(ctx, repo, cmd) + stdout, _, err := RunCmdString(ctx, repo, cmd) if err != nil { return 0, 0, 0, err } @@ -65,12 +64,8 @@ func parseDiffStat(stdout string) (numFiles, totalAdditions, totalDeletions int, // GetReverseRawDiff dumps the reverse diff results of repository in given commit ID to io.Writer. func GetReverseRawDiff(ctx context.Context, repo Repository, commitID string, writer io.Writer) error { - stderr := new(bytes.Buffer) - if err := RunCmd(ctx, repo, gitcmd.NewCommand("show", "--pretty=format:revert %H%n", "-R"). + return RunCmdWithStderr(ctx, repo, gitcmd.NewCommand("show", "--pretty=format:revert %H%n", "-R"). AddDynamicArguments(commitID). - WithStdout(writer). - WithStderr(stderr)); err != nil { - return fmt.Errorf("GetReverseRawDiff: %w - %s", err, stderr) - } - return nil + WithStdout(writer), + ) } diff --git a/modules/gitrepo/merge.go b/modules/gitrepo/merge.go index b0fae88d13..8d58e21c8d 100644 --- a/modules/gitrepo/merge.go +++ b/modules/gitrepo/merge.go @@ -13,7 +13,7 @@ import ( // MergeBase checks and returns merge base of two commits. func MergeBase(ctx context.Context, repo Repository, baseCommitID, headCommitID string) (string, error) { - mergeBase, err := RunCmdString(ctx, repo, gitcmd.NewCommand("merge-base"). + mergeBase, _, err := RunCmdString(ctx, repo, gitcmd.NewCommand("merge-base"). AddDashesAndList(baseCommitID, headCommitID)) if err != nil { return "", fmt.Errorf("get merge-base of %s and %s failed: %w", baseCommitID, headCommitID, err) diff --git a/modules/gitrepo/remote.go b/modules/gitrepo/remote.go index ce43988461..3cbc34eedb 100644 --- a/modules/gitrepo/remote.go +++ b/modules/gitrepo/remote.go @@ -6,8 +6,6 @@ package gitrepo import ( "context" "errors" - "io" - "time" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/git/gitcmd" @@ -36,7 +34,7 @@ func GitRemoteAdd(ctx context.Context, repo Repository, remoteName, remoteURL st return errors.New("unknown remote option: " + string(options[0])) } } - _, err := RunCmdString(ctx, repo, cmd.AddDynamicArguments(remoteName, remoteURL)) + _, _, err := RunCmdString(ctx, repo, cmd.AddDynamicArguments(remoteName, remoteURL)) return err }) } @@ -44,7 +42,7 @@ func GitRemoteAdd(ctx context.Context, repo Repository, remoteName, remoteURL st func GitRemoteRemove(ctx context.Context, repo Repository, remoteName string) error { return globallock.LockAndDo(ctx, getRepoConfigLockKey(repo.RelativePath()), func(ctx context.Context) error { cmd := gitcmd.NewCommand("remote", "rm").AddDynamicArguments(remoteName) - _, err := RunCmdString(ctx, repo, cmd) + _, _, err := RunCmdString(ctx, repo, cmd) return err }) } @@ -60,21 +58,3 @@ func GitRemoteGetURL(ctx context.Context, repo Repository, remoteName string) (* } return giturl.ParseGitURL(addr) } - -// GitRemotePrune prunes the remote branches that no longer exist in the remote repository. -func GitRemotePrune(ctx context.Context, repo Repository, remoteName string, timeout time.Duration, stdout, stderr io.Writer) error { - return RunCmd(ctx, repo, gitcmd.NewCommand("remote", "prune"). - AddDynamicArguments(remoteName). - WithTimeout(timeout). - WithStdout(stdout). - WithStderr(stderr)) -} - -// GitRemoteUpdatePrune updates the remote branches and prunes the ones that no longer exist in the remote repository. -func GitRemoteUpdatePrune(ctx context.Context, repo Repository, remoteName string, timeout time.Duration, stdout, stderr io.Writer) error { - return RunCmd(ctx, repo, gitcmd.NewCommand("remote", "update", "--prune"). - AddDynamicArguments(remoteName). - WithTimeout(timeout). - WithStdout(stdout). - WithStderr(stderr)) -} diff --git a/modules/indexer/code/bleve/bleve.go b/modules/indexer/code/bleve/bleve.go index 7027927eb7..010ee39660 100644 --- a/modules/indexer/code/bleve/bleve.go +++ b/modules/indexer/code/bleve/bleve.go @@ -163,7 +163,7 @@ func (b *Indexer) addUpdate(ctx context.Context, catFileBatch git.CatFileBatch, var err error if !update.Sized { var stdout string - stdout, err = gitrepo.RunCmdString(ctx, repo, gitcmd.NewCommand("cat-file", "-s").AddDynamicArguments(update.BlobSha)) + stdout, _, err = gitrepo.RunCmdString(ctx, repo, gitcmd.NewCommand("cat-file", "-s").AddDynamicArguments(update.BlobSha)) if err != nil { return err } diff --git a/modules/indexer/code/elasticsearch/elasticsearch.go b/modules/indexer/code/elasticsearch/elasticsearch.go index 8c3a2cf508..99f974b646 100644 --- a/modules/indexer/code/elasticsearch/elasticsearch.go +++ b/modules/indexer/code/elasticsearch/elasticsearch.go @@ -148,7 +148,7 @@ func (b *Indexer) addUpdate(ctx context.Context, catFileBatch git.CatFileBatch, var err error if !update.Sized { var stdout string - stdout, err = gitrepo.RunCmdString(ctx, repo, gitcmd.NewCommand("cat-file", "-s").AddDynamicArguments(update.BlobSha)) + stdout, _, err = gitrepo.RunCmdString(ctx, repo, gitcmd.NewCommand("cat-file", "-s").AddDynamicArguments(update.BlobSha)) if err != nil { return nil, err } diff --git a/modules/indexer/code/git.go b/modules/indexer/code/git.go index ca9c6a2974..a17b10551d 100644 --- a/modules/indexer/code/git.go +++ b/modules/indexer/code/git.go @@ -18,7 +18,7 @@ import ( ) func getDefaultBranchSha(ctx context.Context, repo *repo_model.Repository) (string, error) { - stdout, err := gitrepo.RunCmdString(ctx, repo, gitcmd.NewCommand("show-ref", "-s").AddDynamicArguments(git.BranchPrefix+repo.DefaultBranch)) + stdout, _, err := gitrepo.RunCmdString(ctx, repo, gitcmd.NewCommand("show-ref", "-s").AddDynamicArguments(git.BranchPrefix+repo.DefaultBranch)) if err != nil { return "", err } @@ -35,7 +35,7 @@ func getRepoChanges(ctx context.Context, repo *repo_model.Repository, revision s needGenesis := len(status.CommitSha) == 0 if !needGenesis { hasAncestorCmd := gitcmd.NewCommand("merge-base").AddDynamicArguments(status.CommitSha, revision) - stdout, _ := gitrepo.RunCmdString(ctx, repo, hasAncestorCmd) + stdout, _, _ := gitrepo.RunCmdString(ctx, repo, hasAncestorCmd) // FIXME: error is not handled needGenesis = len(stdout) == 0 } @@ -101,7 +101,7 @@ func genesisChanges(ctx context.Context, repo *repo_model.Repository, revision s // nonGenesisChanges get changes since the previous indexer update func nonGenesisChanges(ctx context.Context, repo *repo_model.Repository, revision string) (*internal.RepoChanges, error) { diffCmd := gitcmd.NewCommand("diff", "--name-status").AddDynamicArguments(repo.CodeIndexerStatus.CommitSha, revision) - stdout, runErr := gitrepo.RunCmdString(ctx, repo, diffCmd) + stdout, _, runErr := gitrepo.RunCmdString(ctx, repo, diffCmd) if runErr != nil { // previous commit sha may have been removed by a force push, so // try rebuilding from scratch diff --git a/routers/private/hook_pre_receive.go b/routers/private/hook_pre_receive.go index 88e8b466f1..39955116c4 100644 --- a/routers/private/hook_pre_receive.go +++ b/routers/private/hook_pre_receive.go @@ -191,7 +191,7 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID string, r // 2. Disallow force pushes to protected branches if oldCommitID != objectFormat.EmptyObjectID().String() { - output, err := gitrepo.RunCmdString(ctx, + output, _, err := gitrepo.RunCmdString(ctx, repo, gitcmd.NewCommand("rev-list", "--max-count=1"). AddDynamicArguments(oldCommitID, "^"+newCommitID). diff --git a/routers/web/repo/githttp.go b/routers/web/repo/githttp.go index 325fe85f12..db1b1d5cb3 100644 --- a/routers/web/repo/githttp.go +++ b/routers/web/repo/githttp.go @@ -5,7 +5,6 @@ package repo import ( - "bytes" "compress/gzip" "fmt" "net/http" @@ -445,15 +444,13 @@ func serviceRPC(ctx *context.Context, service string) { h.environ = append(h.environ, "GIT_PROTOCOL="+protocol) } - var stderr bytes.Buffer - if err := gitrepo.RunCmd(ctx, h.getStorageRepo(), cmd.AddArguments("."). + if err := gitrepo.RunCmdWithStderr(ctx, h.getStorageRepo(), cmd.AddArguments("."). WithEnv(append(os.Environ(), h.environ...)). - WithStderr(&stderr). WithStdin(reqBody). WithStdout(ctx.Resp). WithUseContextTimeout(true)); err != nil { if !git.IsErrCanceledOrKilled(err) { - log.Error("Fail to serve RPC(%s) in %s: %v - %s", service, h.getStorageRepo().RelativePath(), err, stderr.String()) + log.Error("Fail to serve RPC(%s) in %s: %v", service, h.getStorageRepo().RelativePath(), err) } } } diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 627fe93b77..cff501ad71 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -239,7 +239,7 @@ func GetMergedBaseCommitID(ctx *context.Context, issue *issues_model.Issue) stri } if commitSHA != "" { // Get immediate parent of the first commit in the patch, grab history back - parentCommit, err = gitrepo.RunCmdString(ctx, ctx.Repo.Repository, + parentCommit, _, err = gitrepo.RunCmdString(ctx, ctx.Repo.Repository, gitcmd.NewCommand("rev-list", "-1", "--skip=1").AddDynamicArguments(commitSHA)) if err == nil { parentCommit = strings.TrimSpace(parentCommit) diff --git a/services/agit/agit.go b/services/agit/agit.go index 15fc2e8fb5..fa2ddd9baf 100644 --- a/services/agit/agit.go +++ b/services/agit/agit.go @@ -229,7 +229,7 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git. } if !forcePush.Value() { - output, err := gitrepo.RunCmdString(ctx, repo, + output, _, err := gitrepo.RunCmdString(ctx, repo, gitcmd.NewCommand("rev-list", "--max-count=1"). AddDynamicArguments(oldCommitID, "^"+opts.NewCommitIDs[i]), ) diff --git a/services/doctor/heads.go b/services/doctor/heads.go index bdadfa674c..4d34b18e18 100644 --- a/services/doctor/heads.go +++ b/services/doctor/heads.go @@ -20,10 +20,10 @@ func synchronizeRepoHeads(ctx context.Context, logger log.Logger, autofix bool) numReposUpdated := 0 err := iterateRepositories(ctx, func(repo *repo_model.Repository) error { numRepos++ - _, defaultBranchErr := gitrepo.RunCmdString(ctx, repo, + _, _, defaultBranchErr := gitrepo.RunCmdString(ctx, repo, gitcmd.NewCommand("rev-parse").AddDashesAndList(repo.DefaultBranch)) - head, headErr := gitrepo.RunCmdString(ctx, repo, + head, _, headErr := gitrepo.RunCmdString(ctx, repo, gitcmd.NewCommand("symbolic-ref", "--short", "HEAD")) // what we expect: default branch is valid, and HEAD points to it diff --git a/services/doctor/mergebase.go b/services/doctor/mergebase.go index 852e37f415..a76ed8afb7 100644 --- a/services/doctor/mergebase.go +++ b/services/doctor/mergebase.go @@ -43,17 +43,17 @@ func checkPRMergeBase(ctx context.Context, logger log.Logger, autofix bool) erro if !pr.HasMerged { var err error - pr.MergeBase, err = gitrepo.RunCmdString(ctx, repo, gitcmd.NewCommand("merge-base").AddDashesAndList(pr.BaseBranch, pr.GetGitHeadRefName())) + pr.MergeBase, _, err = gitrepo.RunCmdString(ctx, repo, gitcmd.NewCommand("merge-base").AddDashesAndList(pr.BaseBranch, pr.GetGitHeadRefName())) if err != nil { var err2 error - pr.MergeBase, err2 = gitrepo.RunCmdString(ctx, repo, gitcmd.NewCommand("rev-parse").AddDynamicArguments(git.BranchPrefix+pr.BaseBranch)) + pr.MergeBase, _, err2 = gitrepo.RunCmdString(ctx, repo, gitcmd.NewCommand("rev-parse").AddDynamicArguments(git.BranchPrefix+pr.BaseBranch)) if err2 != nil { logger.Warn("Unable to get merge base for PR ID %d, #%d onto %s in %s/%s. Error: %v & %v", pr.ID, pr.Index, pr.BaseBranch, pr.BaseRepo.OwnerName, pr.BaseRepo.Name, err, err2) return nil } } } else { - parentsString, err := gitrepo.RunCmdString(ctx, repo, gitcmd.NewCommand("rev-list", "--parents", "-n", "1").AddDynamicArguments(pr.MergedCommitID)) + parentsString, _, err := gitrepo.RunCmdString(ctx, repo, gitcmd.NewCommand("rev-list", "--parents", "-n", "1").AddDynamicArguments(pr.MergedCommitID)) if err != nil { logger.Warn("Unable to get parents for merged PR ID %d, #%d onto %s in %s/%s. Error: %v", pr.ID, pr.Index, pr.BaseBranch, pr.BaseRepo.OwnerName, pr.BaseRepo.Name, err) return nil @@ -66,7 +66,7 @@ func checkPRMergeBase(ctx context.Context, logger log.Logger, autofix bool) erro refs := append([]string{}, parents[1:]...) refs = append(refs, pr.GetGitHeadRefName()) cmd := gitcmd.NewCommand("merge-base").AddDashesAndList(refs...) - pr.MergeBase, err = gitrepo.RunCmdString(ctx, repo, cmd) + pr.MergeBase, _, err = gitrepo.RunCmdString(ctx, repo, cmd) if err != nil { logger.Warn("Unable to get merge base for merged PR ID %d, #%d onto %s in %s/%s. Error: %v", pr.ID, pr.Index, pr.BaseBranch, pr.BaseRepo.OwnerName, pr.BaseRepo.Name, err) return nil diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index f00c90d737..dbb1898f21 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -1271,13 +1271,11 @@ func getDiffBasic(ctx context.Context, gitRepo *git.Repository, opts *DiffOption }() go func() { - stderr := &bytes.Buffer{} if err := cmdDiff.WithTimeout(time.Duration(setting.Git.Timeout.Default) * time.Second). WithDir(repoPath). WithStdout(writer). - WithStderr(stderr). - Run(cmdCtx); err != nil && !git.IsErrCanceledOrKilled(err) { - log.Error("error during GetDiff(git diff dir: %s): %v, stderr: %s", repoPath, err, stderr.String()) + RunWithStderr(cmdCtx); err != nil && !git.IsErrCanceledOrKilled(err) { + log.Error("error during GetDiff(git diff dir: %s): %v", repoPath, err) } _ = writer.Close() diff --git a/services/migrations/gitea_uploader.go b/services/migrations/gitea_uploader.go index f32c095d76..eafcb48895 100644 --- a/services/migrations/gitea_uploader.go +++ b/services/migrations/gitea_uploader.go @@ -661,7 +661,7 @@ func (g *GiteaLocalUploader) updateGitForPullRequest(ctx context.Context, pr *ba fetchArg = git.BranchPrefix + fetchArg } - _, err = gitrepo.RunCmdString(ctx, g.repo, gitcmd.NewCommand("fetch", "--no-tags").AddDashesAndList(remote, fetchArg)) + _, _, err = gitrepo.RunCmdString(ctx, g.repo, gitcmd.NewCommand("fetch", "--no-tags").AddDashesAndList(remote, fetchArg)) if err != nil { log.Error("Fetch branch from %s failed: %v", pr.Head.CloneURL, err) return head, nil @@ -696,7 +696,7 @@ func (g *GiteaLocalUploader) updateGitForPullRequest(ctx context.Context, pr *ba // The SHA is empty log.Warn("Empty reference, no pull head for PR #%d in %s/%s", pr.Number, g.repoOwner, g.repoName) } else { - _, err = gitrepo.RunCmdString(ctx, g.repo, gitcmd.NewCommand("rev-list", "--quiet", "-1").AddDynamicArguments(pr.Head.SHA)) + _, _, err = gitrepo.RunCmdString(ctx, g.repo, gitcmd.NewCommand("rev-list", "--quiet", "-1").AddDynamicArguments(pr.Head.SHA)) if err != nil { // Git update-ref remove bad references with a relative path log.Warn("Deprecated local head %s for PR #%d in %s/%s, removing %s", pr.Head.SHA, pr.Number, g.repoOwner, g.repoName, pr.GetGitHeadRefName()) diff --git a/services/mirror/mirror_pull.go b/services/mirror/mirror_pull.go index f9c40049db..14a226f453 100644 --- a/services/mirror/mirror_pull.go +++ b/services/mirror/mirror_pull.go @@ -193,38 +193,19 @@ func parseRemoteUpdateOutput(output, remoteName string) []*mirrorSyncResult { return results } -func pruneBrokenReferences(ctx context.Context, - m *repo_model.Mirror, - timeout time.Duration, - stdoutBuilder, stderrBuilder *strings.Builder, - isWiki bool, -) error { - wiki := "" - var storageRepo gitrepo.Repository = m.Repo - if isWiki { - wiki = "Wiki " - storageRepo = m.Repo.WikiStorageRepo() - } - - stderrBuilder.Reset() - stdoutBuilder.Reset() - - pruneErr := gitrepo.GitRemotePrune(ctx, storageRepo, m.GetRemoteName(), timeout, stdoutBuilder, stderrBuilder) +func pruneBrokenReferences(ctx context.Context, m *repo_model.Mirror, gitRepo gitrepo.Repository, timeout time.Duration) error { + cmd := gitcmd.NewCommand("remote", "prune").AddDynamicArguments(m.GetRemoteName()).WithTimeout(timeout) + stdout, _, pruneErr := gitrepo.RunCmdString(ctx, gitRepo, cmd) if pruneErr != nil { - stdout := stdoutBuilder.String() - stderr := stderrBuilder.String() - - // sanitize the output, since it may contain the remote address, which may - // contain a password - stderrMessage := util.SanitizeCredentialURLs(stderr) + // sanitize the output, since it may contain the remote address, which may contain a password + stderrMessage := util.SanitizeCredentialURLs(pruneErr.Stderr()) stdoutMessage := util.SanitizeCredentialURLs(stdout) - log.Error("Failed to prune mirror repository %s%-v references:\nStdout: %s\nStderr: %s\nErr: %v", wiki, m.Repo, stdoutMessage, stderrMessage, pruneErr) - desc := fmt.Sprintf("Failed to prune mirror repository %s'%s' references: %s", wiki, storageRepo.RelativePath(), stderrMessage) + log.Error("Failed to prune mirror repository %s references:\nStdout: %s\nStderr: %s\nErr: %v", gitRepo.RelativePath(), stdoutMessage, stderrMessage, pruneErr) + desc := fmt.Sprintf("Failed to prune mirror repository %s references: %s", gitRepo.RelativePath(), stderrMessage) if err := system_model.CreateRepositoryNotice(desc); err != nil { log.Error("CreateRepositoryNotice: %v", err) } - // this if will only be reached on a successful prune so try to get the mirror again } return pruneErr } @@ -249,59 +230,46 @@ func checkRecoverableSyncError(stderrMessage string) bool { // runSync returns true if sync finished without error. func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bool) { - timeout := time.Duration(setting.Git.Timeout.Mirror) * time.Second - log.Trace("SyncMirrors [repo: %-v]: running git remote update...", m.Repo) - // use fetch but not remote update because git fetch support --tags but remote update doesn't - cmd := gitcmd.NewCommand("fetch") - if m.EnablePrune { - cmd.AddArguments("--prune") - } - cmd.AddArguments("--tags").AddDynamicArguments(m.GetRemoteName()) - remoteURL, remoteErr := gitrepo.GitRemoteGetURL(ctx, m.Repo, m.GetRemoteName()) if remoteErr != nil { log.Error("SyncMirrors [repo: %-v]: GetRemoteURL Error %v", m.Repo, remoteErr) return nil, false } - envs := proxy.EnvWithProxy(remoteURL.URL) + timeout := time.Duration(setting.Git.Timeout.Mirror) * time.Second - stdoutBuilder := strings.Builder{} - stderrBuilder := strings.Builder{} - if err := gitrepo.RunCmd(ctx, m.Repo, cmd.WithTimeout(timeout). - WithEnv(envs). - WithStdout(&stdoutBuilder). - WithStderr(&stderrBuilder)); err != nil { - stdout := stdoutBuilder.String() - stderr := stderrBuilder.String() + // use fetch but not remote update because git fetch support --tags but remote update doesn't + cmdFetch := func() *gitcmd.Command { + cmd := gitcmd.NewCommand("fetch", "--tags") + if m.EnablePrune { + cmd.AddArguments("--prune") + } + return cmd.AddDynamicArguments(m.GetRemoteName()).WithTimeout(timeout).WithEnv(envs) + } + var err error + var fetchOutput string // it is from fetch's stderr + fetchStdout, fetchStderr, err := gitrepo.RunCmdString(ctx, m.Repo, cmdFetch()) + if err != nil { // sanitize the output, since it may contain the remote address, which may contain a password - stderrMessage := util.SanitizeCredentialURLs(stderr) - stdoutMessage := util.SanitizeCredentialURLs(stdout) + stderrMessage := util.SanitizeCredentialURLs(fetchStderr) + stdoutMessage := util.SanitizeCredentialURLs(fetchStdout) // Now check if the error is a resolve reference due to broken reference - if checkRecoverableSyncError(stderr) { + if checkRecoverableSyncError(fetchStderr) { log.Warn("SyncMirrors [repo: %-v]: failed to update mirror repository due to broken references:\nStdout: %s\nStderr: %s\nErr: %v\nAttempting Prune", m.Repo, stdoutMessage, stderrMessage, err) err = nil - // Attempt prune - pruneErr := pruneBrokenReferences(ctx, m, timeout, &stdoutBuilder, &stderrBuilder, false) + pruneErr := pruneBrokenReferences(ctx, m, m.Repo, timeout) if pruneErr == nil { // Successful prune - reattempt mirror - stderrBuilder.Reset() - stdoutBuilder.Reset() - if err = gitrepo.RunCmd(ctx, m.Repo, cmd.WithTimeout(timeout). - WithStdout(&stdoutBuilder). - WithStderr(&stderrBuilder)); err != nil { - stdout := stdoutBuilder.String() - stderr := stderrBuilder.String() - - // sanitize the output, since it may contain the remote address, which may - // contain a password - stderrMessage = util.SanitizeCredentialURLs(stderr) - stdoutMessage = util.SanitizeCredentialURLs(stdout) + fetchStdout, fetchStderr, err = gitrepo.RunCmdString(ctx, m.Repo, cmdFetch()) + if err != nil { + // sanitize the output, since it may contain the remote address, which may contain a password + stderrMessage = util.SanitizeCredentialURLs(fetchStderr) + stdoutMessage = util.SanitizeCredentialURLs(fetchStdout) } } } @@ -310,13 +278,13 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo if err != nil { log.Error("SyncMirrors [repo: %-v]: failed to update mirror repository:\nStdout: %s\nStderr: %s\nErr: %v", m.Repo, stdoutMessage, stderrMessage, err) desc := fmt.Sprintf("Failed to update mirror repository '%s': %s", m.Repo.RelativePath(), stderrMessage) - if err = system_model.CreateRepositoryNotice(desc); err != nil { + if err := system_model.CreateRepositoryNotice(desc); err != nil { log.Error("CreateRepositoryNotice: %v", err) } return nil, false } } - output := stderrBuilder.String() + fetchOutput = fetchStderr // the result of "git fetch" is in stderr if err := gitrepo.WriteCommitGraph(ctx, m.Repo); err != nil { log.Error("SyncMirrors [repo: %-v]: %v", m.Repo, err) @@ -353,16 +321,16 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo log.Error("SyncMirrors [repo: %-v]: failed to update size for mirror repository: %v", m.Repo.FullName(), err) } + cmdRemoteUpdatePrune := func() *gitcmd.Command { + return gitcmd.NewCommand("remote", "update", "--prune"). + AddDynamicArguments(m.GetRemoteName()).WithTimeout(timeout).WithEnv(envs) + } + if repo_service.HasWiki(ctx, m.Repo) { log.Trace("SyncMirrors [repo: %-v Wiki]: running git remote update...", m.Repo) - stderrBuilder.Reset() - stdoutBuilder.Reset() - - if err := gitrepo.GitRemoteUpdatePrune(ctx, m.Repo.WikiStorageRepo(), m.GetRemoteName(), - timeout, &stdoutBuilder, &stderrBuilder); err != nil { - stdout := stdoutBuilder.String() - stderr := stderrBuilder.String() - + // the result of "git remote update" is in stderr + stdout, stderr, err := gitrepo.RunCmdString(ctx, m.Repo.WikiStorageRepo(), cmdRemoteUpdatePrune()) + if err != nil { // sanitize the output, since it may contain the remote address, which may contain a password stderrMessage := util.SanitizeCredentialURLs(stderr) stdoutMessage := util.SanitizeCredentialURLs(stdout) @@ -373,16 +341,11 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo err = nil // Attempt prune - pruneErr := pruneBrokenReferences(ctx, m, timeout, &stdoutBuilder, &stderrBuilder, true) + pruneErr := pruneBrokenReferences(ctx, m, m.Repo.WikiStorageRepo(), timeout) if pruneErr == nil { // Successful prune - reattempt mirror - stderrBuilder.Reset() - stdoutBuilder.Reset() - - if err = gitrepo.GitRemoteUpdatePrune(ctx, m.Repo.WikiStorageRepo(), m.GetRemoteName(), - timeout, &stdoutBuilder, &stderrBuilder); err != nil { - stdout := stdoutBuilder.String() - stderr := stderrBuilder.String() + stdout, stderr, err = gitrepo.RunCmdString(ctx, m.Repo.WikiStorageRepo(), cmdRemoteUpdatePrune()) + if err != nil { stderrMessage = util.SanitizeCredentialURLs(stderr) stdoutMessage = util.SanitizeCredentialURLs(stdout) } @@ -393,7 +356,7 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo if err != nil { log.Error("SyncMirrors [repo: %-v Wiki]: failed to update mirror repository wiki:\nStdout: %s\nStderr: %s\nErr: %v", m.Repo, stdoutMessage, stderrMessage, err) desc := fmt.Sprintf("Failed to update mirror repository wiki '%s': %s", m.Repo.WikiStorageRepo().RelativePath(), stderrMessage) - if err = system_model.CreateRepositoryNotice(desc); err != nil { + if err := system_model.CreateRepositoryNotice(desc); err != nil { log.Error("CreateRepositoryNotice: %v", err) } return nil, false @@ -418,7 +381,7 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo } m.UpdatedUnix = timeutil.TimeStampNow() - return parseRemoteUpdateOutput(output, m.GetRemoteName()), true + return parseRemoteUpdateOutput(fetchOutput, m.GetRemoteName()), true } func getRepoPullMirrorLockKey(repoID int64) string { diff --git a/services/pull/check.go b/services/pull/check.go index f1d72cc7c6..44ec17f510 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -287,10 +287,9 @@ func getMergeCommit(ctx context.Context, pr *issues_model.PullRequest) (*git.Com prHeadRef := pr.GetGitHeadRefName() // Check if the pull request is merged into BaseBranch - if _, err := gitrepo.RunCmdString(ctx, pr.BaseRepo, - gitcmd.NewCommand("merge-base", "--is-ancestor"). - AddDynamicArguments(prHeadRef, pr.BaseBranch)); err != nil { - if strings.Contains(err.Error(), "exit status 1") { + cmd := gitcmd.NewCommand("merge-base", "--is-ancestor").AddDynamicArguments(prHeadRef, pr.BaseBranch) + if err := gitrepo.RunCmdWithStderr(ctx, pr.BaseRepo, cmd); err != nil { + if gitcmd.IsErrorExitCode(err, 1) { // prHeadRef is not an ancestor of the base branch return nil, nil } @@ -315,7 +314,7 @@ func getMergeCommit(ctx context.Context, pr *issues_model.PullRequest) (*git.Com objectFormat := git.ObjectFormatFromName(pr.BaseRepo.ObjectFormatName) // Get the commit from BaseBranch where the pull request got merged - mergeCommit, err := gitrepo.RunCmdString(ctx, pr.BaseRepo, + mergeCommit, _, err := gitrepo.RunCmdString(ctx, pr.BaseRepo, gitcmd.NewCommand("rev-list", "--ancestry-path", "--merges", "--reverse"). AddDynamicArguments(prHeadCommitID+".."+pr.BaseBranch)) if err != nil { diff --git a/services/pull/merge.go b/services/pull/merge.go index 88e30c6832..6dab23aa21 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -412,27 +412,25 @@ func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *use // Push back to upstream. // This cause an api call to "/api/internal/hook/post-receive/...", // If it's merge, all db transaction and operations should be there but not here to prevent deadlock. - if err := mergeCtx.PrepareGitCmd(pushCmd).Run(ctx); err != nil { - if strings.Contains(mergeCtx.errbuf.String(), "non-fast-forward") { + if err := mergeCtx.PrepareGitCmd(pushCmd).RunWithStderr(ctx); err != nil { + if strings.Contains(err.Stderr(), "non-fast-forward") { return "", &git.ErrPushOutOfDate{ StdOut: mergeCtx.outbuf.String(), - StdErr: mergeCtx.errbuf.String(), + StdErr: err.Stderr(), Err: err, } - } else if strings.Contains(mergeCtx.errbuf.String(), "! [remote rejected]") { + } else if strings.Contains(err.Stderr(), "! [remote rejected]") { err := &git.ErrPushRejected{ StdOut: mergeCtx.outbuf.String(), - StdErr: mergeCtx.errbuf.String(), + StdErr: err.Stderr(), Err: err, } err.GenerateMessage() return "", err } - return "", fmt.Errorf("git push: %s", mergeCtx.errbuf.String()) + return "", fmt.Errorf("git push: %s", err.Stderr()) } mergeCtx.outbuf.Reset() - mergeCtx.errbuf.Reset() - return mergeCommitID, nil } @@ -446,9 +444,8 @@ func commitAndSignNoAuthor(ctx *mergeContext, message string) error { } cmdCommit.AddOptionFormat("-S%s", ctx.signKey.KeyID) } - if err := ctx.PrepareGitCmd(cmdCommit).Run(ctx); err != nil { - log.Error("git commit %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String()) - return fmt.Errorf("git commit %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String()) + if err := ctx.PrepareGitCmd(cmdCommit).RunWithStderr(ctx); err != nil { + return fmt.Errorf("git commit %v: %w\n%s", ctx.pr, err, ctx.outbuf.String()) } return nil } @@ -507,39 +504,37 @@ func (err ErrMergeDivergingFastForwardOnly) Error() string { } func runMergeCommand(ctx *mergeContext, mergeStyle repo_model.MergeStyle, cmd *gitcmd.Command) error { - if err := ctx.PrepareGitCmd(cmd).Run(ctx); err != nil { + if err := ctx.PrepareGitCmd(cmd).RunWithStderr(ctx); err != nil { // Merge will leave a MERGE_HEAD file in the .git folder if there is a conflict if _, statErr := os.Stat(filepath.Join(ctx.tmpBasePath, ".git", "MERGE_HEAD")); statErr == nil { // We have a merge conflict error - log.Debug("MergeConflict %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String()) + log.Debug("MergeConflict %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), err.Stderr()) return ErrMergeConflicts{ Style: mergeStyle, StdOut: ctx.outbuf.String(), - StdErr: ctx.errbuf.String(), + StdErr: err.Stderr(), Err: err, } - } else if strings.Contains(ctx.errbuf.String(), "refusing to merge unrelated histories") { - log.Debug("MergeUnrelatedHistories %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String()) + } else if strings.Contains(err.Stderr(), "refusing to merge unrelated histories") { + log.Debug("MergeUnrelatedHistories %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), err.Stderr()) return ErrMergeUnrelatedHistories{ Style: mergeStyle, StdOut: ctx.outbuf.String(), - StdErr: ctx.errbuf.String(), + StdErr: err.Stderr(), Err: err, } - } else if mergeStyle == repo_model.MergeStyleFastForwardOnly && strings.Contains(ctx.errbuf.String(), "Not possible to fast-forward, aborting") { - log.Debug("MergeDivergingFastForwardOnly %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String()) + } else if mergeStyle == repo_model.MergeStyleFastForwardOnly && strings.Contains(err.Stderr(), "Not possible to fast-forward, aborting") { + log.Debug("MergeDivergingFastForwardOnly %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), err.Stderr()) return ErrMergeDivergingFastForwardOnly{ StdOut: ctx.outbuf.String(), - StdErr: ctx.errbuf.String(), + StdErr: err.Stderr(), Err: err, } } - log.Error("git merge %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String()) - return fmt.Errorf("git merge %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String()) + log.Error("git merge %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), err.Stderr()) + return fmt.Errorf("git merge %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), err.Stderr()) } ctx.outbuf.Reset() - ctx.errbuf.Reset() - return nil } diff --git a/services/pull/merge_prepare.go b/services/pull/merge_prepare.go index d2aa967b5e..d38138c514 100644 --- a/services/pull/merge_prepare.go +++ b/services/pull/merge_prepare.go @@ -38,12 +38,10 @@ type mergeContext struct { // Do NOT use it with gitcmd.RunStd*() functions, otherwise it will panic func (ctx *mergeContext) PrepareGitCmd(cmd *gitcmd.Command) *gitcmd.Command { ctx.outbuf.Reset() - ctx.errbuf.Reset() return cmd.WithEnv(ctx.env). WithDir(ctx.tmpBasePath). WithParentCallerInfo(). - WithStdout(ctx.outbuf). - WithStderr(ctx.errbuf) + WithStdout(ctx.outbuf) } // ErrSHADoesNotMatch represents a "SHADoesNotMatch" kind of error. @@ -97,7 +95,6 @@ func createTemporaryRepoForMerge(ctx context.Context, pr *issues_model.PullReque } mergeCtx.outbuf.Reset() - mergeCtx.errbuf.Reset() if err := prepareTemporaryRepoForMerge(mergeCtx); err != nil { defer cancel() return nil, nil, err @@ -167,13 +164,11 @@ func prepareTemporaryRepoForMerge(ctx *mergeContext) error { setConfig := func(key, value string) error { if err := ctx.PrepareGitCmd(gitcmd.NewCommand("config", "--local").AddDynamicArguments(key, value)). - Run(ctx); err != nil { - log.Error("git config [%s -> %q]: %v\n%s\n%s", key, value, err, ctx.outbuf.String(), ctx.errbuf.String()) - return fmt.Errorf("git config [%s -> %q]: %w\n%s\n%s", key, value, err, ctx.outbuf.String(), ctx.errbuf.String()) + RunWithStderr(ctx); err != nil { + log.Error("git config [%s -> %q]: %v\n%s\n%s", key, value, err, ctx.outbuf.String(), err.Stderr()) + return fmt.Errorf("git config [%s -> %q]: %w\n%s\n%s", key, value, err, ctx.outbuf.String(), err.Stderr()) } ctx.outbuf.Reset() - ctx.errbuf.Reset() - return nil } @@ -200,13 +195,11 @@ func prepareTemporaryRepoForMerge(ctx *mergeContext) error { // Read base branch index if err := ctx.PrepareGitCmd(gitcmd.NewCommand("read-tree", "HEAD")). - Run(ctx); err != nil { - log.Error("git read-tree HEAD: %v\n%s\n%s", err, ctx.outbuf.String(), ctx.errbuf.String()) - return fmt.Errorf("Unable to read base branch in to the index: %w\n%s\n%s", err, ctx.outbuf.String(), ctx.errbuf.String()) + RunWithStderr(ctx); err != nil { + log.Error("git read-tree HEAD: %v\n%s\n%s", err, ctx.outbuf.String(), err.Stderr()) + return fmt.Errorf("Unable to read base branch in to the index: %w\n%s\n%s", err, ctx.outbuf.String(), err.Stderr()) } ctx.outbuf.Reset() - ctx.errbuf.Reset() - return nil } @@ -288,15 +281,14 @@ func (err ErrRebaseConflicts) Error() string { func rebaseTrackingOnToBase(ctx *mergeContext, mergeStyle repo_model.MergeStyle) error { // Checkout head branch if err := ctx.PrepareGitCmd(gitcmd.NewCommand("checkout", "-b").AddDynamicArguments(stagingBranch, trackingBranch)). - Run(ctx); err != nil { - return fmt.Errorf("unable to git checkout tracking as staging in temp repo for %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String()) + RunWithStderr(ctx); err != nil { + return fmt.Errorf("unable to git checkout tracking as staging in temp repo for %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), err.Stderr()) } ctx.outbuf.Reset() - ctx.errbuf.Reset() // Rebase before merging if err := ctx.PrepareGitCmd(gitcmd.NewCommand("rebase").AddDynamicArguments(baseBranch)). - Run(ctx); err != nil { + RunWithStderr(ctx); err != nil { // Rebase will leave a REBASE_HEAD file in .git if there is a conflict if _, statErr := os.Stat(filepath.Join(ctx.tmpBasePath, ".git", "REBASE_HEAD")); statErr == nil { var commitSha string @@ -310,7 +302,7 @@ func rebaseTrackingOnToBase(ctx *mergeContext, mergeStyle repo_model.MergeStyle) commitShaBytes, readErr := os.ReadFile(failingCommitPath) if readErr != nil { // Abandon this attempt to handle the error - return fmt.Errorf("unable to git rebase staging on to base in temp repo for %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String()) + return fmt.Errorf("unable to git rebase staging on to base in temp repo for %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), err.Stderr()) } commitSha = strings.TrimSpace(string(commitShaBytes)) ok = true @@ -319,20 +311,19 @@ func rebaseTrackingOnToBase(ctx *mergeContext, mergeStyle repo_model.MergeStyle) } if !ok { log.Error("Unable to determine failing commit sha for failing rebase in temp repo for %-v. Cannot cast as ErrRebaseConflicts.", ctx.pr) - return fmt.Errorf("unable to git rebase staging on to base in temp repo for %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String()) + return fmt.Errorf("unable to git rebase staging on to base in temp repo for %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), err.Stderr()) } - log.Debug("Conflict when rebasing staging on to base in %-v at %s: %v\n%s\n%s", ctx.pr, commitSha, err, ctx.outbuf.String(), ctx.errbuf.String()) + log.Debug("Conflict when rebasing staging on to base in %-v at %s: %v\n%s\n%s", ctx.pr, commitSha, err, ctx.outbuf.String(), err.Stderr()) return ErrRebaseConflicts{ CommitSHA: commitSha, Style: mergeStyle, StdOut: ctx.outbuf.String(), - StdErr: ctx.errbuf.String(), + StdErr: err.Stderr(), Err: err, } } - return fmt.Errorf("unable to git rebase staging on to base in temp repo for %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String()) + return fmt.Errorf("unable to git rebase staging on to base in temp repo for %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), err.Stderr()) } ctx.outbuf.Reset() - ctx.errbuf.Reset() return nil } diff --git a/services/pull/merge_rebase.go b/services/pull/merge_rebase.go index 0fa4fd00f6..a3b01848fd 100644 --- a/services/pull/merge_rebase.go +++ b/services/pull/merge_rebase.go @@ -110,13 +110,11 @@ func doMergeStyleRebase(ctx *mergeContext, mergeStyle repo_model.MergeStyle, mes // Checkout base branch again if err := ctx.PrepareGitCmd(gitcmd.NewCommand("checkout").AddDynamicArguments(baseBranch)). - Run(ctx); err != nil { - log.Error("git checkout base prior to merge post staging rebase %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String()) - return fmt.Errorf("git checkout base prior to merge post staging rebase %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String()) + RunWithStderr(ctx); err != nil { + log.Error("git checkout base prior to merge post staging rebase %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), err.Stderr()) + return fmt.Errorf("git checkout base prior to merge post staging rebase %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), err.Stderr()) } ctx.outbuf.Reset() - ctx.errbuf.Reset() - if mergeStyle == repo_model.MergeStyleRebase { return doMergeRebaseFastForward(ctx) } diff --git a/services/pull/merge_squash.go b/services/pull/merge_squash.go index b5f2a4deff..d92660d60b 100644 --- a/services/pull/merge_squash.go +++ b/services/pull/merge_squash.go @@ -81,11 +81,10 @@ func doMergeStyleSquash(ctx *mergeContext, message string) error { } cmdCommit.AddOptionFormat("-S%s", ctx.signKey.KeyID) } - if err := ctx.PrepareGitCmd(cmdCommit).Run(ctx); err != nil { - log.Error("git commit %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String()) - return fmt.Errorf("git commit [%s:%s -> %s:%s]: %w\n%s\n%s", ctx.pr.HeadRepo.FullName(), ctx.pr.HeadBranch, ctx.pr.BaseRepo.FullName(), ctx.pr.BaseBranch, err, ctx.outbuf.String(), ctx.errbuf.String()) + if err := ctx.PrepareGitCmd(cmdCommit).RunWithStderr(ctx); err != nil { + log.Error("git commit %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), err.Stderr()) + return fmt.Errorf("git commit [%s:%s -> %s:%s]: %w\n%s\n%s", ctx.pr.HeadRepo.FullName(), ctx.pr.HeadBranch, ctx.pr.BaseRepo.FullName(), ctx.pr.BaseBranch, err, ctx.outbuf.String(), err.Stderr()) } ctx.outbuf.Reset() - ctx.errbuf.Reset() return nil } diff --git a/services/pull/patch.go b/services/pull/patch.go index d82fe3e225..ce3afcc331 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -249,7 +249,7 @@ func AttemptThreeWayMerge(ctx context.Context, gitPath string, gitRepo *git.Repo defer cancel() // First we use read-tree to do a simple three-way merge - if _, _, err := gitcmd.NewCommand("read-tree", "-m").AddDynamicArguments(base, ours, theirs).WithDir(gitPath).RunStdString(ctx); err != nil { + if err := gitcmd.NewCommand("read-tree", "-m").AddDynamicArguments(base, ours, theirs).WithDir(gitPath).RunWithStderr(ctx); err != nil { log.Error("Unable to run read-tree -m! Error: %v", err) return false, nil, fmt.Errorf("unable to run read-tree -m! Error: %w", err) } @@ -413,25 +413,15 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo * // in memory - which is very wasteful. // - alternatively we can do the equivalent of: // `git apply --check ... | grep ...` - // meaning we don't store all of the conflicts unnecessarily. - stderrReader, stderrWriter, err := os.Pipe() - if err != nil { - log.Error("Unable to open stderr pipe: %v", err) - return false, fmt.Errorf("unable to open stderr pipe: %w", err) - } - defer func() { - _ = stderrReader.Close() - _ = stderrWriter.Close() - }() + // meaning we don't store all the conflicts unnecessarily. + var stderrReader io.ReadCloser // 8. Run the check command conflict = false err = cmdApply. WithDir(tmpBasePath). - WithStderr(stderrWriter). + WithStderrReader(&stderrReader). WithPipelineFunc(func(ctx context.Context, cancel context.CancelFunc) error { - // Close the writer end of the pipe to begin processing - _ = stderrWriter.Close() defer func() { // Close the reader on return to terminate the git command if necessary _ = stderrReader.Close() @@ -491,7 +481,7 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo * }). Run(gitRepo.Ctx) - // 9. Check if the found conflictedfiles is non-zero, "err" could be non-nil, so we should ignore it if we found conflicts. + // 9. Check if the found conflicted files is non-zero, "err" could be non-nil, so we should ignore it if we found conflicts. // Note: `"err" could be non-nil` is due that if enable 3-way merge, it doesn't return any error on found conflicts. if len(pr.ConflictedFiles) > 0 { if conflict { diff --git a/services/pull/patch_unmerged.go b/services/pull/patch_unmerged.go index 0491680313..3cfaabe662 100644 --- a/services/pull/patch_unmerged.go +++ b/services/pull/patch_unmerged.go @@ -71,11 +71,9 @@ func readUnmergedLsFileLines(ctx context.Context, tmpBasePath string, outputChan _ = lsFilesReader.Close() }() - stderr := &strings.Builder{} err = gitcmd.NewCommand("ls-files", "-u", "-z"). WithDir(tmpBasePath). WithStdout(lsFilesWriter). - WithStderr(stderr). WithPipelineFunc(func(_ context.Context, _ context.CancelFunc) error { _ = lsFilesWriter.Close() defer func() { @@ -113,9 +111,9 @@ func readUnmergedLsFileLines(ctx context.Context, tmpBasePath string, outputChan outputChan <- toemit } }). - Run(ctx) + RunWithStderr(ctx) if err != nil { - outputChan <- &lsFileLine{err: fmt.Errorf("git ls-files -u -z: %w", gitcmd.ConcatenateError(err, stderr.String()))} + outputChan <- &lsFileLine{err: fmt.Errorf("git ls-files -u -z: %w", err)} } } diff --git a/services/pull/pull.go b/services/pull/pull.go index ff5c562001..baad89aa55 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -4,7 +4,6 @@ package pull import ( - "bytes" "context" "errors" "fmt" @@ -532,10 +531,8 @@ func checkIfPRContentChanged(ctx context.Context, pr *issues_model.PullRequest, return false, mergeBase, fmt.Errorf("unable to open pipe for to run diff: %w", err) } - stderr := new(bytes.Buffer) if err := cmd.WithDir(prCtx.tmpBasePath). WithStdout(stdoutWriter). - WithStderr(stderr). WithPipelineFunc(func(ctx context.Context, cancel context.CancelFunc) error { _ = stdoutWriter.Close() defer func() { @@ -543,11 +540,10 @@ func checkIfPRContentChanged(ctx context.Context, pr *issues_model.PullRequest, }() return util.IsEmptyReader(stdoutReader) }). - Run(ctx); err != nil { - if err == util.ErrNotEmpty { + RunWithStderr(ctx); err != nil { + if errors.Is(err, util.ErrNotEmpty) { return true, mergeBase, nil } - err = gitcmd.ConcatenateError(err, stderr.String()) log.Error("Unable to run diff on %s %s %s in tempRepo for PR[%d]%s/%s...%s/%s: Error: %v", newCommitID, oldCommitID, mergeBase, diff --git a/services/pull/temp_repo.go b/services/pull/temp_repo.go index 113d1cb49e..f5c24a1bd2 100644 --- a/services/pull/temp_repo.go +++ b/services/pull/temp_repo.go @@ -32,8 +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, - errbuf *strings.Builder // any use should be preceded by a Reset and preferably after use + 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 } // PrepareGitCmd prepares a git command with the correct directory, environment, and output buffers @@ -41,10 +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() - ctx.errbuf.Reset() - return cmd.WithDir(ctx.tmpBasePath). - WithStdout(ctx.outbuf). - WithStderr(ctx.errbuf) + return cmd.WithDir(ctx.tmpBasePath).WithStdout(ctx.outbuf) } // createTemporaryRepoForPR creates a temporary repo with "base" for pr.BaseBranch and "tracking" for pr.HeadBranch @@ -87,7 +83,6 @@ func createTemporaryRepoForPR(ctx context.Context, pr *issues_model.PullRequest) tmpBasePath: tmpBasePath, pr: pr, outbuf: &strings.Builder{}, - errbuf: &strings.Builder{}, } baseRepoPath := pr.BaseRepo.RepoPath() @@ -133,25 +128,25 @@ func createTemporaryRepoForPR(ctx context.Context, pr *issues_model.PullRequest) } if err := prCtx.PrepareGitCmd(gitcmd.NewCommand("remote", "add", "-t").AddDynamicArguments(pr.BaseBranch).AddArguments("-m").AddDynamicArguments(pr.BaseBranch).AddDynamicArguments("origin", baseRepoPath)). - Run(ctx); err != nil { - log.Error("%-v Unable to add base repository as origin [%s -> %s]: %v\n%s\n%s", pr, pr.BaseRepo.FullName(), tmpBasePath, err, prCtx.outbuf.String(), prCtx.errbuf.String()) + RunWithStderr(ctx); err != nil { + log.Error("%-v Unable to add base repository as origin [%s -> %s]: %v\n%s\n%s", pr, pr.BaseRepo.FullName(), tmpBasePath, err, prCtx.outbuf.String(), err.Stderr()) cancel() - return nil, nil, fmt.Errorf("Unable to add base repository as origin [%s -> tmpBasePath]: %w\n%s\n%s", pr.BaseRepo.FullName(), err, prCtx.outbuf.String(), prCtx.errbuf.String()) + return nil, nil, fmt.Errorf("Unable to add base repository as origin [%s -> tmpBasePath]: %w\n%s\n%s", pr.BaseRepo.FullName(), err, prCtx.outbuf.String(), err.Stderr()) } if err := prCtx.PrepareGitCmd(gitcmd.NewCommand("fetch", "origin").AddArguments(fetchArgs...). AddDashesAndList(git.BranchPrefix+pr.BaseBranch+":"+git.BranchPrefix+baseBranch, git.BranchPrefix+pr.BaseBranch+":"+git.BranchPrefix+"original_"+baseBranch)). - Run(ctx); err != nil { - log.Error("%-v Unable to fetch origin base branch [%s:%s -> base, original_base in %s]: %v:\n%s\n%s", pr, pr.BaseRepo.FullName(), pr.BaseBranch, tmpBasePath, err, prCtx.outbuf.String(), prCtx.errbuf.String()) + RunWithStderr(ctx); err != nil { + log.Error("%-v Unable to fetch origin base branch [%s:%s -> base, original_base in %s]: %v:\n%s\n%s", pr, pr.BaseRepo.FullName(), pr.BaseBranch, tmpBasePath, err, prCtx.outbuf.String(), err.Stderr()) cancel() - return nil, nil, fmt.Errorf("Unable to fetch origin base branch [%s:%s -> base, original_base in tmpBasePath]: %w\n%s\n%s", pr.BaseRepo.FullName(), pr.BaseBranch, err, prCtx.outbuf.String(), prCtx.errbuf.String()) + return nil, nil, fmt.Errorf("Unable to fetch origin base branch [%s:%s -> base, original_base in tmpBasePath]: %w\n%s\n%s", pr.BaseRepo.FullName(), pr.BaseBranch, err, prCtx.outbuf.String(), err.Stderr()) } if err := prCtx.PrepareGitCmd(gitcmd.NewCommand("symbolic-ref").AddDynamicArguments("HEAD", git.BranchPrefix+baseBranch)). - Run(ctx); err != nil { - log.Error("%-v Unable to set HEAD as base branch in [%s]: %v\n%s\n%s", pr, tmpBasePath, err, prCtx.outbuf.String(), prCtx.errbuf.String()) + RunWithStderr(ctx); err != nil { + log.Error("%-v Unable to set HEAD as base branch in [%s]: %v\n%s\n%s", pr, tmpBasePath, err, prCtx.outbuf.String(), err.Stderr()) cancel() - return nil, nil, fmt.Errorf("Unable to set HEAD as base branch in tmpBasePath: %w\n%s\n%s", err, prCtx.outbuf.String(), prCtx.errbuf.String()) + return nil, nil, fmt.Errorf("Unable to set HEAD as base branch in tmpBasePath: %w\n%s\n%s", err, prCtx.outbuf.String(), err.Stderr()) } if err := addCacheRepo(tmpBasePath, headRepoPath); err != nil { @@ -161,10 +156,10 @@ func createTemporaryRepoForPR(ctx context.Context, pr *issues_model.PullRequest) } if err := prCtx.PrepareGitCmd(gitcmd.NewCommand("remote", "add").AddDynamicArguments(remoteRepoName, headRepoPath)). - Run(ctx); err != nil { - log.Error("%-v Unable to add head repository as head_repo [%s -> %s]: %v\n%s\n%s", pr, pr.HeadRepo.FullName(), tmpBasePath, err, prCtx.outbuf.String(), prCtx.errbuf.String()) + RunWithStderr(ctx); err != nil { + log.Error("%-v Unable to add head repository as head_repo [%s -> %s]: %v\n%s\n%s", pr, pr.HeadRepo.FullName(), tmpBasePath, err, prCtx.outbuf.String(), err.Stderr()) cancel() - return nil, nil, fmt.Errorf("Unable to add head repository as head_repo [%s -> tmpBasePath]: %w\n%s\n%s", pr.HeadRepo.FullName(), err, prCtx.outbuf.String(), prCtx.errbuf.String()) + return nil, nil, fmt.Errorf("Unable to add head repository as head_repo [%s -> tmpBasePath]: %w\n%s\n%s", pr.HeadRepo.FullName(), err, prCtx.outbuf.String(), err.Stderr()) } trackingBranch := "tracking" @@ -179,18 +174,16 @@ func createTemporaryRepoForPR(ctx context.Context, pr *issues_model.PullRequest) headBranch = pr.GetGitHeadRefName() } if err := prCtx.PrepareGitCmd(gitcmd.NewCommand("fetch").AddArguments(fetchArgs...).AddDynamicArguments(remoteRepoName, headBranch+":"+trackingBranch)). - Run(ctx); err != nil { + RunWithStderr(ctx); err != nil { cancel() if exist, _ := git_model.IsBranchExist(ctx, pr.HeadRepo.ID, pr.HeadBranch); !exist { return nil, nil, git_model.ErrBranchNotExist{ BranchName: pr.HeadBranch, } } - log.Error("%-v Unable to fetch head_repo head branch [%s:%s -> tracking in %s]: %v:\n%s\n%s", pr, pr.HeadRepo.FullName(), pr.HeadBranch, tmpBasePath, err, prCtx.outbuf.String(), prCtx.errbuf.String()) - return nil, nil, fmt.Errorf("Unable to fetch head_repo head branch [%s:%s -> tracking in tmpBasePath]: %w\n%s\n%s", pr.HeadRepo.FullName(), headBranch, err, prCtx.outbuf.String(), prCtx.errbuf.String()) + log.Error("%-v Unable to fetch head_repo head branch [%s:%s -> tracking in %s]: %v:\n%s\n%s", pr, pr.HeadRepo.FullName(), pr.HeadBranch, tmpBasePath, err, prCtx.outbuf.String(), err.Stderr()) + return nil, nil, fmt.Errorf("Unable to fetch head_repo head branch [%s:%s -> tracking in tmpBasePath]: %w\n%s\n%s", pr.HeadRepo.FullName(), headBranch, err, prCtx.outbuf.String(), err.Stderr()) } prCtx.outbuf.Reset() - prCtx.errbuf.Reset() - return prCtx, cancel, nil } diff --git a/services/pull/update_rebase.go b/services/pull/update_rebase.go index 6a70c03467..c267b6978a 100644 --- a/services/pull/update_rebase.go +++ b/services/pull/update_rebase.go @@ -71,7 +71,6 @@ func updateHeadByRebaseOnToBase(ctx context.Context, pr *issues_model.PullReques // TODO: this cause an api call to "/api/internal/hook/post-receive/...", // that prevents us from doint the whole merge in one db transaction mergeCtx.outbuf.Reset() - mergeCtx.errbuf.Reset() if err := pushCmd. WithEnv(repo_module.FullPushingEnvironment( @@ -84,27 +83,24 @@ func updateHeadByRebaseOnToBase(ctx context.Context, pr *issues_model.PullReques )). WithDir(mergeCtx.tmpBasePath). WithStdout(mergeCtx.outbuf). - WithStderr(mergeCtx.errbuf). - Run(ctx); err != nil { - if strings.Contains(mergeCtx.errbuf.String(), "non-fast-forward") { + RunWithStderr(ctx); err != nil { + if strings.Contains(err.Stderr(), "non-fast-forward") { return &git.ErrPushOutOfDate{ StdOut: mergeCtx.outbuf.String(), - StdErr: mergeCtx.errbuf.String(), + StdErr: err.Stderr(), Err: err, } - } else if strings.Contains(mergeCtx.errbuf.String(), "! [remote rejected]") { + } else if strings.Contains(err.Stderr(), "! [remote rejected]") { err := &git.ErrPushRejected{ StdOut: mergeCtx.outbuf.String(), - StdErr: mergeCtx.errbuf.String(), + StdErr: err.Stderr(), Err: err, } err.GenerateMessage() return err } - return fmt.Errorf("git push: %s", mergeCtx.errbuf.String()) + return fmt.Errorf("git push: %s", err.Stderr()) } mergeCtx.outbuf.Reset() - mergeCtx.errbuf.Reset() - return nil } diff --git a/services/release/release.go b/services/release/release.go index a0d3736b44..a482501164 100644 --- a/services/release/release.go +++ b/services/release/release.go @@ -371,7 +371,7 @@ func DeleteReleaseByID(ctx context.Context, repo *repo_model.Repository, rel *re } } - if stdout, err := gitrepo.RunCmdString(ctx, repo, + if stdout, _, err := gitrepo.RunCmdString(ctx, repo, gitcmd.NewCommand("tag", "-d").AddDashesAndList(rel.TagName), ); err != nil && !strings.Contains(err.Error(), "not found") { log.Error("DeleteReleaseByID (git tag -d): %d in %v Failed:\nStdout: %s\nError: %v", rel.ID, repo, stdout, err) diff --git a/services/repository/check.go b/services/repository/check.go index 57d627c63d..e521af94e1 100644 --- a/services/repository/check.go +++ b/services/repository/check.go @@ -88,7 +88,7 @@ func GitGcRepo(ctx context.Context, repo *repo_model.Repository, timeout time.Du command := gitcmd.NewCommand("gc").AddArguments(args...) var stdout string var err error - stdout, err = gitrepo.RunCmdString(ctx, repo, command) + stdout, _, err = gitrepo.RunCmdString(ctx, repo, command) if err != nil { log.Error("Repository garbage collection failed for %-v. Stdout: %s\nError: %v", repo, stdout, err) desc := fmt.Sprintf("Repository garbage collection failed for %s. Stdout: %s\nError: %v", repo.RelativePath(), stdout, err) diff --git a/services/repository/contributors_graph.go b/services/repository/contributors_graph.go index 2c5c7c604f..6f42c75036 100644 --- a/services/repository/contributors_graph.go +++ b/services/repository/contributors_graph.go @@ -131,10 +131,8 @@ func getExtendedCommitStats(repo *git.Repository, revision string /*, limit int gitCmd.AddDynamicArguments(baseCommit.ID.String()) var extendedCommitStats []*ExtendedCommitStats - stderr := new(strings.Builder) err = gitCmd.WithDir(repo.Path). WithStdout(stdoutWriter). - WithStderr(stderr). WithPipelineFunc(func(ctx context.Context, cancel context.CancelFunc) error { _ = stdoutWriter.Close() scanner := bufio.NewScanner(stdoutReader) @@ -191,9 +189,9 @@ func getExtendedCommitStats(repo *git.Repository, revision string /*, limit int _ = stdoutReader.Close() return nil }). - Run(repo.Ctx) + RunWithStderr(repo.Ctx) if err != nil { - return nil, fmt.Errorf("Failed to get ContributorsCommitStats for repository.\nError: %w\nStderr: %s", err, stderr) + return nil, fmt.Errorf("ContributorsCommitStats: %w", err) } return extendedCommitStats, nil diff --git a/services/repository/create.go b/services/repository/create.go index 7439fc8f08..bfac83419d 100644 --- a/services/repository/create.go +++ b/services/repository/create.go @@ -315,7 +315,7 @@ func CreateRepositoryDirectly(ctx context.Context, doer, owner *user_model.User, licenses = append(licenses, opts.License) var stdout string - stdout, err = gitrepo.RunCmdString(ctx, repo, gitcmd.NewCommand("rev-parse", "HEAD")) + stdout, _, err = gitrepo.RunCmdString(ctx, repo, gitcmd.NewCommand("rev-parse", "HEAD")) if err != nil { log.Error("CreateRepository(git rev-parse HEAD) in %v: Stdout: %s\nError: %v", repo, stdout, err) return nil, fmt.Errorf("CreateRepository(git rev-parse HEAD): %w", err) @@ -476,7 +476,7 @@ func updateGitRepoAfterCreate(ctx context.Context, repo *repo_model.Repository) return fmt.Errorf("checkDaemonExportOK: %w", err) } - if stdout, err := gitrepo.RunCmdString(ctx, repo, + if stdout, _, err := gitrepo.RunCmdString(ctx, repo, gitcmd.NewCommand("update-server-info")); err != nil { log.Error("CreateRepository(git update-server-info) in %v: Stdout: %s\nError: %v", repo, stdout, err) return fmt.Errorf("CreateRepository(git update-server-info): %w", err) diff --git a/services/repository/files/patch.go b/services/repository/files/patch.go index ee567ecd37..1f6fd033ed 100644 --- a/services/repository/files/patch.go +++ b/services/repository/files/patch.go @@ -164,20 +164,15 @@ func ApplyDiffPatch(ctx context.Context, repo *repo_model.Repository, doer *user } } - stdout := &strings.Builder{} - stderr := &strings.Builder{} - cmdApply := gitcmd.NewCommand("apply", "--index", "--recount", "--cached", "--ignore-whitespace", "--whitespace=fix", "--binary") if git.DefaultFeatures().CheckVersionAtLeast("2.32") { cmdApply.AddArguments("-3") } if err := cmdApply.WithDir(t.basePath). - WithStdout(stdout). - WithStderr(stderr). WithStdin(strings.NewReader(opts.Content)). - Run(ctx); err != nil { - return nil, fmt.Errorf("Error: Stdout: %s\nStderr: %s\nErr: %w", stdout.String(), stderr.String(), err) + RunWithStderr(ctx); err != nil { + return nil, fmt.Errorf("git apply error: %w", err) } // Now write the tree diff --git a/services/repository/files/temp_repo.go b/services/repository/files/temp_repo.go index b605236c03..17acf1f9ce 100644 --- a/services/repository/files/temp_repo.go +++ b/services/repository/files/temp_repo.go @@ -98,7 +98,7 @@ func (t *TemporaryUploadRepository) Init(ctx context.Context, objectFormatName s // SetDefaultIndex sets the git index to our HEAD func (t *TemporaryUploadRepository) SetDefaultIndex(ctx context.Context) error { - if _, _, err := gitcmd.NewCommand("read-tree", "HEAD").WithDir(t.basePath).RunStdString(ctx); err != nil { + if err := gitcmd.NewCommand("read-tree", "HEAD").WithDir(t.basePath).RunWithStderr(ctx); err != nil { return fmt.Errorf("SetDefaultIndex: %w", err) } return nil @@ -106,7 +106,7 @@ func (t *TemporaryUploadRepository) SetDefaultIndex(ctx context.Context) error { // RefreshIndex looks at the current index and checks to see if merges or updates are needed by checking stat() information. func (t *TemporaryUploadRepository) RefreshIndex(ctx context.Context) error { - if _, _, err := gitcmd.NewCommand("update-index", "--refresh").WithDir(t.basePath).RunStdString(ctx); err != nil { + if err := gitcmd.NewCommand("update-index", "--refresh").WithDir(t.basePath).RunWithStderr(ctx); err != nil { return fmt.Errorf("RefreshIndex: %w", err) } return nil @@ -115,16 +115,11 @@ func (t *TemporaryUploadRepository) RefreshIndex(ctx context.Context) error { // LsFiles checks if the given filename arguments are in the index func (t *TemporaryUploadRepository) LsFiles(ctx context.Context, filenames ...string) ([]string, error) { stdOut := new(bytes.Buffer) - stdErr := new(bytes.Buffer) - if err := gitcmd.NewCommand("ls-files", "-z").AddDashesAndList(filenames...). WithDir(t.basePath). WithStdout(stdOut). - WithStderr(stdErr). - Run(ctx); err != nil { - log.Error("Unable to run git ls-files for temporary repo: %s (%s) Error: %v\nstdout: %s\nstderr: %s", t.repo.FullName(), t.basePath, err, stdOut.String(), stdErr.String()) - err = fmt.Errorf("Unable to run git ls-files for temporary repo of: %s Error: %w\nstdout: %s\nstderr: %s", t.repo.FullName(), err, stdOut.String(), stdErr.String()) - return nil, err + 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) } fileList := make([]string, 0, len(filenames)) @@ -149,8 +144,6 @@ func (t *TemporaryUploadRepository) RemoveFilesFromIndex(ctx context.Context, fi if err != nil { return fmt.Errorf("unable to get object format for temporary repo: %q, error: %w", t.repo.FullName(), err) } - stdOut := new(bytes.Buffer) - stdErr := new(bytes.Buffer) stdIn := new(bytes.Buffer) for _, file := range filenames { if file != "" { @@ -162,11 +155,9 @@ func (t *TemporaryUploadRepository) RemoveFilesFromIndex(ctx context.Context, fi if err := gitcmd.NewCommand("update-index", "--remove", "-z", "--index-info"). WithDir(t.basePath). - WithStdout(stdOut). - WithStderr(stdErr). WithStdin(stdIn). - Run(ctx); err != nil { - return fmt.Errorf("unable to update-index for temporary repo: %q, error: %w\nstdout: %s\nstderr: %s", t.repo.FullName(), err, stdOut.String(), stdErr.String()) + RunWithStderr(ctx); err != nil { + return fmt.Errorf("unable to update-index for temporary repo: %q, error: %w", t.repo.FullName(), err) } return nil } @@ -174,16 +165,12 @@ func (t *TemporaryUploadRepository) RemoveFilesFromIndex(ctx context.Context, fi // HashObjectAndWrite writes the provided content to the object db and returns its hash func (t *TemporaryUploadRepository) HashObjectAndWrite(ctx context.Context, content io.Reader) (string, error) { stdOut := new(bytes.Buffer) - stdErr := new(bytes.Buffer) - if err := gitcmd.NewCommand("hash-object", "-w", "--stdin"). WithDir(t.basePath). WithStdout(stdOut). - WithStderr(stdErr). WithStdin(content). - Run(ctx); err != nil { - log.Error("Unable to hash-object to temporary repo: %s (%s) Error: %v\nstdout: %s\nstderr: %s", t.repo.FullName(), t.basePath, err, stdOut.String(), stdErr.String()) - return "", fmt.Errorf("Unable to hash-object to temporary repo: %s Error: %w\nstdout: %s\nstderr: %s", t.repo.FullName(), err, stdOut.String(), stdErr.String()) + RunWithStderr(ctx); err != nil { + return "", fmt.Errorf("unable to hash-object to temporary repo: %s, error: %w", t.repo.FullName(), err) } return strings.TrimSpace(stdOut.String()), nil @@ -191,17 +178,15 @@ func (t *TemporaryUploadRepository) HashObjectAndWrite(ctx context.Context, cont // AddObjectToIndex adds the provided object hash to the index with the provided mode and path func (t *TemporaryUploadRepository) AddObjectToIndex(ctx context.Context, mode, objectHash, objectPath string) error { - if _, _, err := gitcmd.NewCommand("update-index", "--add", "--replace", "--cacheinfo"). - AddDynamicArguments(mode, objectHash, objectPath).WithDir(t.basePath).RunStdString(ctx); err != nil { - stderr := err.Error() - if matched, _ := regexp.MatchString(".*Invalid path '.*", stderr); matched { + if err := gitcmd.NewCommand("update-index", "--add", "--replace", "--cacheinfo"). + AddDynamicArguments(mode, objectHash, objectPath).WithDir(t.basePath).RunWithStderr(ctx); err != nil { + if matched, _ := regexp.MatchString(".*Invalid path '.*", err.Stderr()); matched { return ErrFilePathInvalid{ Message: objectPath, Path: objectPath, } } - log.Error("Unable to add object to index: %s %s %s in temporary repo %s(%s) Error: %v", mode, objectHash, objectPath, t.repo.FullName(), t.basePath, err) - return fmt.Errorf("Unable to add object to index at %s in temporary repo %s Error: %w", objectPath, t.repo.FullName(), err) + return fmt.Errorf("unable to add object to index at %s in temporary repo %s, error: %w", objectPath, t.repo.FullName(), err) } return nil } @@ -342,18 +327,13 @@ func (t *TemporaryUploadRepository) CommitTree(ctx context.Context, opts *Commit ) stdout := new(bytes.Buffer) - stderr := new(bytes.Buffer) if err := cmdCommitTree. WithEnv(env). WithDir(t.basePath). WithStdout(stdout). - WithStderr(stderr). WithStdin(messageBytes). - Run(ctx); err != nil { - log.Error("Unable to commit-tree in temporary repo: %s (%s) Error: %v\nStdout: %s\nStderr: %s", - t.repo.FullName(), t.basePath, err, stdout, stderr) - return "", fmt.Errorf("Unable to commit-tree in temporary repo: %s Error: %w\nStdout: %s\nStderr: %s", - t.repo.FullName(), err, stdout, stderr) + RunWithStderr(ctx); err != nil { + return "", fmt.Errorf("unable to commit-tree in temporary repo: %s Error: %w", t.repo.FullName(), err) } return strings.TrimSpace(stdout.String()), nil } @@ -390,13 +370,11 @@ func (t *TemporaryUploadRepository) DiffIndex(ctx context.Context) (*gitdiff.Dif _ = stdoutReader.Close() _ = stdoutWriter.Close() }() - stderr := new(bytes.Buffer) var diff *gitdiff.Diff err = gitcmd.NewCommand("diff-index", "--src-prefix=\\a/", "--dst-prefix=\\b/", "--cached", "-p", "HEAD"). WithTimeout(30 * time.Second). WithDir(t.basePath). WithStdout(stdoutWriter). - WithStderr(stderr). WithPipelineFunc(func(ctx context.Context, cancel context.CancelFunc) error { _ = stdoutWriter.Close() defer cancel() @@ -409,9 +387,8 @@ func (t *TemporaryUploadRepository) DiffIndex(ctx context.Context) (*gitdiff.Dif } return nil }). - Run(ctx) + RunWithStderr(ctx) if err != nil && !git.IsErrCanceledOrKilled(err) { - log.Error("Unable to diff-index in temporary repo %s (%s). Error: %v\nStderr: %s", t.repo.FullName(), t.basePath, err, stderr) 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 f89d9a095a..268878e166 100644 --- a/services/repository/gitgraph/graph.go +++ b/services/repository/gitgraph/graph.go @@ -8,7 +8,6 @@ import ( "bytes" "context" "os" - "strings" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/git/gitcmd" @@ -45,7 +44,6 @@ func GetCommitGraph(r *git.Repository, page, maxAllowedColors int, hidePRRefs bo } graph := NewGraph() - stderr := new(strings.Builder) stdoutReader, stdoutWriter, err := os.Pipe() if err != nil { return nil, err @@ -57,7 +55,6 @@ func GetCommitGraph(r *git.Repository, page, maxAllowedColors int, hidePRRefs bo if err := graphCmd. WithDir(r.Path). WithStdout(stdoutWriter). - WithStderr(stderr). WithPipelineFunc(func(ctx context.Context, cancel context.CancelFunc) error { _ = stdoutWriter.Close() defer stdoutReader.Close() @@ -110,7 +107,7 @@ func GetCommitGraph(r *git.Repository, page, maxAllowedColors int, hidePRRefs bo } return scanner.Err() }). - Run(r.Ctx); err != nil { + RunWithStderr(r.Ctx); err != nil { return graph, err } return graph, nil diff --git a/services/repository/migrate.go b/services/repository/migrate.go index 8f515326ad..bc46c5e09b 100644 --- a/services/repository/migrate.go +++ b/services/repository/migrate.go @@ -225,7 +225,7 @@ func MigrateRepositoryGitData(ctx context.Context, u *user_model.User, // this is necessary for sync local tags from remote configName := fmt.Sprintf("remote.%s.fetch", mirrorModel.GetRemoteName()) - if stdout, err := gitrepo.RunCmdString(ctx, repo, + if stdout, _, err := gitrepo.RunCmdString(ctx, repo, gitcmd.NewCommand("config"). AddOptionValues("--add", configName, `+refs/tags/*:refs/tags/*`)); err != nil { log.Error("MigrateRepositoryGitData(git config --add +refs/tags/*:refs/tags/*) in %v: Stdout: %s\nError: %v", repo, stdout, err) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 53e68432cc..cc66c23d4a 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -1020,20 +1020,16 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApprovalForAgitFlow(t *testing. }) assert.NoError(t, err) - stderrBuf := &bytes.Buffer{} - - err = gitcmd.NewCommand("push", "origin", "HEAD:refs/for/master", "-o"). + _, stderr, err := gitcmd.NewCommand("push", "origin", "HEAD:refs/for/master", "-o"). AddDynamicArguments(`topic=test/head2`). AddArguments("-o"). AddDynamicArguments(`title="create a test pull request with agit"`). AddArguments("-o"). AddDynamicArguments(`description="This PR is a test pull request which created with agit"`). WithDir(dstPath). - WithStderr(stderrBuf). - Run(t.Context()) + RunStdString(t.Context()) assert.NoError(t, err) - - assert.Contains(t, stderrBuf.String(), setting.AppURL+"user2/repo1/pulls/6") + assert.Contains(t, stderr, setting.AppURL+"user2/repo1/pulls/6") baseRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user2", Name: "repo1"}) pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{