fix

Author: wxiaoguang

modules/git/gitcmd/command.go

@@ -310,7 +310,11 @@ func (c *Command) WithUseContextTimeout(useContextTimeout bool) *Command {
 // For most cases, "Run" family functions can get its caller info automatically
 // But if you need to call "Run" family functions in a wrapper function: "FeatureFunc -> GeneralWrapperFunc -> RunXxx",
 // 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 != "" {
+		return c
+	}
 	if len(optInfo) > 0 {
 		c.opts.callerInfo = optInfo[0]
 		return c
@@ -428,7 +432,8 @@ type runStdError struct {
 }
 
 func (r *runStdError) Error() string {
-	// the stderr must be in the returned error text, some code only checks `strings.Contains(err.Error(), "git error")`
+	// 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")`
 	if r.errMsg == "" {
 		r.errMsg = ConcatenateError(r.err, r.stderr).Error()
 	}
@@ -458,7 +463,7 @@ func (c *Command) RunStdString(ctx context.Context) (stdout, stderr string, runE
 }
 
 // RunStdBytes runs the command and returns stdout/stderr as bytes. and store stderr to returned error (err combined with stderr).
-func (c *Command) RunStdBytes(ctx context.Context) ( /*stdout*/ []byte /*stderr*/, []byte /*runErr*/, RunStdError) {
+func (c *Command) RunStdBytes(ctx context.Context) (stdout, stderr []byte, runErr RunStdError) {
 	return c.WithParentCallerInfo().runStdBytes(ctx)
 }
 
@@ -467,14 +472,15 @@ func (c *Command) runStdBytes(ctx context.Context) ( /*stdout*/ []byte /*stderr*
 		// 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

modules/git/gitcmd/command_test.go

@@ -41,6 +41,7 @@ func TestRunWithContextStd(t *testing.T) {
 		if assert.Error(t, err) {
 			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.Empty(t, stdout)
 		}
@@ -52,6 +53,7 @@ func TestRunWithContextStd(t *testing.T) {
 		if assert.Error(t, err) {
 			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.Empty(t, stdout)
 		}

services/pull/merge.go

@@ -406,7 +406,7 @@ 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.WithCmd(pushCmd).Run(ctx); err != nil {
+	if err := mergeCtx.PrepareGitCmd(pushCmd).Run(ctx); err != nil {
 		if strings.Contains(mergeCtx.errbuf.String(), "non-fast-forward") {
 			return "", &git.ErrPushOutOfDate{
 				StdOut: mergeCtx.outbuf.String(),
@@ -440,7 +440,7 @@ func commitAndSignNoAuthor(ctx *mergeContext, message string) error {
 		}
 		cmdCommit.AddOptionFormat("-S%s", ctx.signKey.KeyID)
 	}
-	if err := ctx.WithCmd(cmdCommit).Run(ctx); err != nil {
+	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())
 	}
@@ -501,7 +501,7 @@ func (err ErrMergeDivergingFastForwardOnly) Error() string {
 }
 
 func runMergeCommand(ctx *mergeContext, mergeStyle repo_model.MergeStyle, cmd *gitcmd.Command) error {
-	if err := ctx.WithCmd(cmd).Run(ctx); err != nil {
+	if err := ctx.PrepareGitCmd(cmd).Run(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

services/pull/merge_prepare.go

@@ -32,7 +32,7 @@ type mergeContext struct {
 	env       []string
 }
 
-func (ctx *mergeContext) WithCmd(cmd *gitcmd.Command) *gitcmd.Command {
+func (ctx *mergeContext) PrepareGitCmd(cmd *gitcmd.Command) *gitcmd.Command {
 	ctx.outbuf.Reset()
 	ctx.errbuf.Reset()
 	return cmd.WithEnv(ctx.env).
@@ -73,7 +73,7 @@ func createTemporaryRepoForMerge(ctx context.Context, pr *issues_model.PullReque
 	}
 
 	if expectedHeadCommitID != "" {
-		trackingCommitID, _, err := mergeCtx.WithCmd(gitcmd.NewCommand("show-ref", "--hash").AddDynamicArguments(git.BranchPrefix + trackingBranch)).RunStdString(ctx)
+		trackingCommitID, _, err := mergeCtx.PrepareGitCmd(gitcmd.NewCommand("show-ref", "--hash").AddDynamicArguments(git.BranchPrefix + trackingBranch)).RunStdString(ctx)
 		if err != nil {
 			defer cancel()
 			log.Error("failed to get sha of head branch in %-v: show-ref[%s] --hash refs/heads/tracking: %v", mergeCtx.pr, mergeCtx.tmpBasePath, err)
@@ -151,7 +151,7 @@ func prepareTemporaryRepoForMerge(ctx *mergeContext) error {
 	}
 
 	setConfig := func(key, value string) error {
-		if err := ctx.WithCmd(gitcmd.NewCommand("config", "--local").AddDynamicArguments(key, value)).
+		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())
@@ -184,7 +184,7 @@ func prepareTemporaryRepoForMerge(ctx *mergeContext) error {
 	}
 
 	// Read base branch index
-	if err := ctx.WithCmd(gitcmd.NewCommand("read-tree", "HEAD")).
+	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())
@@ -272,15 +272,15 @@ func (err ErrRebaseConflicts) Error() string {
 // if there is a conflict it will return an ErrRebaseConflicts
 func rebaseTrackingOnToBase(ctx *mergeContext, mergeStyle repo_model.MergeStyle) error {
 	// Checkout head branch
-	if err := ctx.WithCmd(gitcmd.NewCommand("checkout", "-b").AddDynamicArguments(stagingBranch, trackingBranch)).
+	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())
 	}
 	ctx.outbuf.Reset()
 	ctx.errbuf.Reset()
 
 	// Rebase before merging
-	if err := ctx.WithCmd(gitcmd.NewCommand("rebase").AddDynamicArguments(baseBranch)).
+	if err := ctx.PrepareGitCmd(gitcmd.NewCommand("rebase").AddDynamicArguments(baseBranch)).
 		Run(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 {

services/pull/merge_rebase.go

@@ -109,7 +109,7 @@ func doMergeStyleRebase(ctx *mergeContext, mergeStyle repo_model.MergeStyle, mes
 	}
 
 	// Checkout base branch again
-	if err := ctx.WithCmd(gitcmd.NewCommand("checkout").AddDynamicArguments(baseBranch)).
+	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())

services/pull/merge_squash.go

@@ -80,7 +80,7 @@ func doMergeStyleSquash(ctx *mergeContext, message string) error {
 		}
 		cmdCommit.AddOptionFormat("-S%s", ctx.signKey.KeyID)
 	}
-	if err := ctx.WithCmd(cmdCommit).Run(ctx); err != nil {
+	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())
 	}

services/pull/temp_repo.go

@@ -37,7 +37,7 @@ type prTmpRepoContext struct {
 	errbuf      *strings.Builder // any use should be preceded by a Reset and preferably after use
 }
 
-func (ctx *prTmpRepoContext) WithCmd(cmd *gitcmd.Command) *gitcmd.Command {
+func (ctx *prTmpRepoContext) PrepareGitCmd(cmd *gitcmd.Command) *gitcmd.Command {
 	ctx.outbuf.Reset()
 	ctx.errbuf.Reset()
 	return cmd.WithDir(ctx.tmpBasePath).
@@ -130,22 +130,22 @@ func createTemporaryRepoForPR(ctx context.Context, pr *issues_model.PullRequest)
 		return nil, nil, fmt.Errorf("Unable to add base repository to temporary repo [%s -> tmpBasePath]: %w", pr.BaseRepo.FullName(), err)
 	}
 
-	if err := prCtx.WithCmd(gitcmd.NewCommand("remote", "add", "-t").AddDynamicArguments(pr.BaseBranch).AddArguments("-m").AddDynamicArguments(pr.BaseBranch).AddDynamicArguments("origin", baseRepoPath)).
+	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())
 		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())
 	}
 
-	if err := prCtx.WithCmd(gitcmd.NewCommand("fetch", "origin").AddArguments(fetchArgs...).
+	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())
 		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())
 	}
 
-	if err := prCtx.WithCmd(gitcmd.NewCommand("symbolic-ref").AddDynamicArguments("HEAD", git.BranchPrefix+baseBranch)).
+	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())
 		cancel()
@@ -158,7 +158,7 @@ func createTemporaryRepoForPR(ctx context.Context, pr *issues_model.PullRequest)
 		return nil, nil, fmt.Errorf("Unable to add head base repository to temporary repo [%s -> tmpBasePath]: %w", pr.HeadRepo.FullName(), err)
 	}
 
-	if err := prCtx.WithCmd(gitcmd.NewCommand("remote", "add").AddDynamicArguments(remoteRepoName, headRepoPath)).
+	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())
 		cancel()
@@ -176,7 +176,7 @@ func createTemporaryRepoForPR(ctx context.Context, pr *issues_model.PullRequest)
 	} else {
 		headBranch = pr.GetGitHeadRefName()
 	}
-	if err := prCtx.WithCmd(gitcmd.NewCommand("fetch").AddArguments(fetchArgs...).AddDynamicArguments(remoteRepoName, headBranch+":"+trackingBranch)).
+	if err := prCtx.PrepareGitCmd(gitcmd.NewCommand("fetch").AddArguments(fetchArgs...).AddDynamicArguments(remoteRepoName, headBranch+":"+trackingBranch)).
 		Run(ctx); err != nil {
 		cancel()
 		if !gitrepo.IsBranchExist(ctx, pr.HeadRepo, pr.HeadBranch) {