refactor: use RunWithStderr for git commands (#40295)

If the stderr output is not handled, then it needs to use RunWithStderr
to make sure the git error message is included in the returned error.
Otherwise, when the git command fails, the error log won't show useful
information.

Ref:

* #39795 : no useful information in log messages
*
https://github.com/go-gitea/gitea/pull/40160#pullrequestreview-5752720866
This commit is contained in:
wxiaoguang
2026-11-09 17:59:52 +00:00
committed by GitHub
parent e0b2f3425e
commit 772f365254
26 changed files with 57 additions and 47 deletions

View File

@@ -92,6 +92,8 @@ conditional `UPDATE` or a lock.
- Pass already opened repositories down or use `git.RepositoryFromContextOrOpen`
rather than reopening them or spawning extra `git` processes.
- If git command stderr output is not handled by the caller, then it needs to use RunWithStderr
to make sure the git error message is included in the returned error and displayed in the logs.
## Dependencies

View File

@@ -17,7 +17,7 @@ import (
func TestMergeBaseNoCommonHistory(t *testing.T) {
repoDir := filepath.Join(t.TempDir(), "repo.git")
require.NoError(t, gitcmd.NewCommand("init").AddDynamicArguments(repoDir).Run(t.Context()))
require.NoError(t, gitcmd.NewCommand("init").AddDynamicArguments(repoDir).RunWithStderr(t.Context()))
_, _, runErr := gitcmd.NewCommand("fast-import").WithDir(repoDir).WithStdinBytes([]byte(strings.TrimSpace(`
commit refs/heads/branch1
committer User <user@example.com> 1714310400 +0000

View File

@@ -323,7 +323,7 @@ func GetAffectedFiles(ctx context.Context, repo *Repository, branchName, oldComm
}
return scanner.Err()
}).
Run(ctx)
RunWithStderr(ctx)
if err != nil {
log.Error("Unable to get affected files for commits from %s to %s in %s: %v", oldCommitID, newCommitID, repo.LogString(), err)
}

View File

@@ -18,7 +18,7 @@ import (
// Ref: https://www.kernel.org/pub/software/scm/git/docs/git-gc.html
func FetchRemoteTempCommit(ctx context.Context, repo, remoteRepo RepositoryFacade, commitID string) error {
// since it is for temporary use, we don't need to update any ref or lock anything or maintain anything
return fetchRemoteCommand(remoteRepo, commitID).WithRepo(repo).Run(ctx)
return fetchRemoteCommand(remoteRepo, commitID).WithRepo(repo).RunWithStderr(ctx)
}
// FetchRemoteCommitUpdateRef fetches a commit from a remote repository and points refName at it

View File

@@ -12,5 +12,5 @@ import (
// Fsck verifies the connectivity and validity of the objects in the database
func Fsck(ctx context.Context, repo RepositoryFacade, timeout time.Duration, args gitcmd.TrustedCmdArgs) error {
return gitcmd.NewCommand("fsck").AddArguments(args...).WithTimeout(timeout).WithRepo(repo).Run(ctx)
return gitcmd.NewCommand("fsck").AddArguments(args...).WithTimeout(timeout).WithRepo(repo).RunWithStderr(ctx)
}

View File

@@ -568,6 +568,9 @@ func (c *Command) RunWithStderr(ctx context.Context) RunStdError {
return c.WaitWithStderr()
}
// Run runs the command and leaves stderr handling to the caller, such as through [Command.MakeStderrPipe].
// Without a stderr reader, Git's diagnostics are discarded and the error may only report an exit status.
// Use [Command.RunWithStderr] to include Git's stderr in the returned error.
func (c *Command) Run(ctx context.Context) (err error) {
if err = c.WithParentCallerInfo().Start(ctx); err != nil {
return err

View File

@@ -96,7 +96,7 @@ func TestRunWithContextStd(t *testing.T) {
cmd = NewCommand("grep")
cmd.AddOptionGrepExpr("-x")
assert.NoError(t, cmd.Run(t.Context()))
assert.NoError(t, cmd.RunWithStderr(t.Context()))
assert.Equal(t, []string{"grep", "-e", "-x"}, cmd.args)
}

View File

@@ -15,7 +15,7 @@ import (
func prepareRepoDirRenameConflict(t *testing.T) string {
repoDir := filepath.Join(t.TempDir(), "repo-dir-rename-conflict.git")
require.NoError(t, gitcmd.NewCommand("init", "--bare").AddDynamicArguments(repoDir).Run(t.Context()))
require.NoError(t, gitcmd.NewCommand("init", "--bare").AddDynamicArguments(repoDir).RunWithStderr(t.Context()))
stdin := `blob
mark :1
data 2
@@ -62,7 +62,7 @@ B
from :3
M 100644 :5 z/d
`
require.NoError(t, gitcmd.NewCommand("fast-import").WithDir(repoDir).WithStdinBytes([]byte(stdin)).Run(t.Context()))
require.NoError(t, gitcmd.NewCommand("fast-import").WithDir(repoDir).WithStdinBytes([]byte(stdin)).RunWithStderr(t.Context()))
return repoDir
}

View File

@@ -33,9 +33,9 @@ func TestReceivePushRejectsDuplicateTreeEntries(t *testing.T) {
ctx := t.Context()
sourceDir := filepath.Join(t.TempDir(), "source.git")
require.NoError(t, gitcmd.NewCommand("init", "--bare").AddDynamicArguments(sourceDir).Run(ctx))
require.NoError(t, gitcmd.NewCommand("init", "--bare").AddDynamicArguments(sourceDir).RunWithStderr(ctx))
targetDir := filepath.Join(t.TempDir(), "target.git")
require.NoError(t, gitcmd.NewCommand("init", "--bare").AddDynamicArguments(targetDir).Run(ctx))
require.NoError(t, gitcmd.NewCommand("init", "--bare").AddDynamicArguments(targetDir).RunWithStderr(ctx))
hashBlob := func(content string) string {
stdout, _, err := gitcmd.NewCommand("hash-object", "-w", "--stdin").WithDir(sourceDir).WithStdinBytes([]byte(content)).RunStdString(ctx)
@@ -54,7 +54,7 @@ func TestReceivePushRejectsDuplicateTreeEntries(t *testing.T) {
require.NoError(t, err)
commitID = strings.TrimSpace(commitID)
require.NoError(t, gitcmd.NewCommand("update-ref", "refs/heads/atk").AddDynamicArguments(commitID).WithDir(sourceDir).Run(ctx))
require.NoError(t, gitcmd.NewCommand("update-ref", "refs/heads/atk").AddDynamicArguments(commitID).WithDir(sourceDir).RunWithStderr(ctx))
_, _, err = gitcmd.NewCommand("push").AddDynamicArguments(targetDir, "refs/heads/atk:refs/heads/atk").WithDir(sourceDir).RunStdString(ctx)
require.Error(t, err, "push of a commit with duplicate tree entries must be rejected by the receiving repository's fsck")

View File

@@ -260,9 +260,9 @@ func ParseRefSuffix(ref string) (refName, refSuffix string) {
}
func UpdateRef(ctx context.Context, repo RepositoryFacade, refName, newCommitID string) error {
return gitcmd.NewCommand("update-ref").AddDynamicArguments(refName, newCommitID).WithRepo(repo).Run(ctx)
return gitcmd.NewCommand("update-ref").AddDynamicArguments(refName, newCommitID).WithRepo(repo).RunWithStderr(ctx)
}
func RemoveRef(ctx context.Context, repo RepositoryFacade, refName string) error {
return gitcmd.NewCommand("update-ref", "--no-deref", "-d").AddDynamicArguments(refName).WithRepo(repo).Run(ctx)
return gitcmd.NewCommand("update-ref", "--no-deref", "-d").AddDynamicArguments(refName).WithRepo(repo).RunWithStderr(ctx)
}

View File

@@ -150,7 +150,7 @@ func TestCommitsByFileAndRange(t *testing.T) {
assert.False(t, hasMore)
repoFollowRenameDir := filepath.Join(t.TempDir(), "repo.git")
require.NoError(t, gitcmd.NewCommand("init").AddDynamicArguments(repoFollowRenameDir).Run(t.Context()))
require.NoError(t, gitcmd.NewCommand("init").AddDynamicArguments(repoFollowRenameDir).RunWithStderr(t.Context()))
_, _, runErr := gitcmd.NewCommand("fast-import").WithDir(repoFollowRenameDir).WithStdinBytes([]byte(strings.TrimSpace(`
blob
mark :1

View File

@@ -55,7 +55,7 @@ func (repo *Repository) GetDiff(ctx context.Context, compareArg string, w io.Wri
return gitcmd.NewCommand("diff", "-p").AddDynamicArguments(compareArg).
WithRepo(repo).
WithStdoutCopy(w).
Run(ctx)
RunWithStderr(ctx)
}
// GetDiffBinary generates and returns patch data between given revisions, including binary diffs.
@@ -64,7 +64,7 @@ func (repo *Repository) GetDiffBinary(ctx context.Context, compareArg string, w
AddDynamicArguments(compareArg).
WithRepo(repo).
WithStdoutCopy(w).
Run(ctx)
RunWithStderr(ctx)
}
// GetPatch generates and returns format-patch data between given revisions, able to be used with `git apply`
@@ -72,7 +72,7 @@ func (repo *Repository) GetPatch(ctx context.Context, compareArg string, w io.Wr
return gitcmd.NewCommand("format-patch", "--binary", "--stdout").AddDynamicArguments(compareArg).
WithRepo(repo).
WithStdoutCopy(w).
Run(ctx)
RunWithStderr(ctx)
}
// GetFilesChangedBetween returns a list of all files that have been changed between the given commits

View File

@@ -45,6 +45,9 @@ func TestGetFormatPatch(t *testing.T) {
patch := string(patchb)
assert.Regexp(t, "^From 8d92fc95", patch)
assert.Contains(t, patch, "Subject: [PATCH] Add file2.txt")
err = repo.GetPatch(t.Context(), "gitea-missing-revision", io.Discard)
assert.ErrorContains(t, err, "fatal: ambiguous argument 'gitea-missing-revision'")
}
func TestReadPatch(t *testing.T) {

View File

@@ -37,7 +37,7 @@ func GetTemplateSubmoduleCommits(ctx context.Context, repo RepositoryFacade) (su
}
return scanner.Err()
}).
Run(ctx)
RunWithStderr(ctx)
if err != nil {
return nil, fmt.Errorf("GetTemplateSubmoduleCommits: error running git ls-tree: %v", err)
}

View File

@@ -34,10 +34,12 @@ func verifyCommits(ctx context.Context, oldCommitID, newCommitID string, repo *g
err := command.WithEnv(env).
WithRepo(repo).
WithPipelineFunc(func(gitCtx gitcmd.Context) error {
err := readAndVerifyCommitsFromShaReader(ctx, stdoutReader, repo, env)
if err := readAndVerifyCommitsFromShaReader(ctx, stdoutReader, repo, env); err != nil {
return gitCtx.CancelPipeline(err)
}
return nil
}).
Run(ctx)
RunWithStderr(ctx)
if err != nil && !isErrUnverifiedCommit(err) {
log.Error("Unable to check commits from %s to %s in %s: %v", oldCommitID, newCommitID, repo.LogString(), err)
}
@@ -75,7 +77,7 @@ func readAndVerifyCommit(ctx context.Context, sha string, repo *git.Repository,
}
return nil
}).
Run(ctx)
RunWithStderr(ctx)
}
type errUnverifiedCommit struct {

View File

@@ -41,4 +41,7 @@ func TestVerifyCommits(t *testing.T) {
assert.Error(t, err)
}
}
err = verifyCommits(t.Context(), objectFormat.EmptyObjectID().String(), "gitea-missing-revision", gitRepo, nil)
assert.ErrorContains(t, err, "fatal: ambiguous argument 'gitea-missing-revision'")
}

View File

@@ -1049,7 +1049,7 @@ revert
from :2
D test2.txt
D test10.txt`
require.NoError(t, gitcmd.NewCommand("fast-import").WithRepo(pull.BaseRepo).WithStdinBytes([]byte(stdin)).Run(t.Context()))
require.NoError(t, gitcmd.NewCommand("fast-import").WithRepo(pull.BaseRepo).WithStdinBytes([]byte(stdin)).RunWithStderr(t.Context()))
gitRepo, err := git.OpenRepository(t.Context(), pull.BaseRepo)
assert.NoError(t, err)

View File

@@ -437,7 +437,7 @@ func TestRun(t *testing.T) {
_ = daemon.Run()
}))
require.NoError(t, err)
require.NoError(t, gitcmd.NewCommand("clone", "-q", "--bare").AddDynamicArguments("git://127.0.0.1:"+port+"/repo1_bare", filepath.Join(base, "allowed")).Run(t.Context()))
require.NoError(t, gitcmd.NewCommand("clone", "-q", "--bare").AddDynamicArguments("git://127.0.0.1:"+port+"/repo1_bare", filepath.Join(base, "allowed")).RunWithStderr(t.Context()))
_, stderr, runErr := gitcmd.NewCommand("clone", "-q", "--bare").AddDynamicArguments("git://127.0.0.2:"+port+"/repo1_bare", filepath.Join(base, "denied")).RunStdString(t.Context())
require.Error(t, runErr)
assert.Contains(t, stderr, "target denied by policy")

View File

@@ -33,8 +33,7 @@ type mergeContext struct {
}
// PrepareGitCmd prepares a git command with the correct directory, environment, and output buffers
// This function can only be called with gitcmd.Run()
// Do NOT use it with gitcmd.RunStd*() functions, otherwise it will panic
// Use RunWithStderr because this already configures a stdout buffer.
func (ctx *mergeContext) PrepareGitCmd(cmd *gitcmd.Command) *gitcmd.Command {
ctx.outbuf.Reset()
return cmd.WithEnv(ctx.env).
@@ -211,7 +210,7 @@ func getDiffTree(ctx context.Context, repo git.RepositoryFacade, baseBranch, hea
cmd := gitcmd.NewCommand("diff-tree", "--no-commit-id", "--name-only", "-r", "-r", "-z", "--root")
diffOutReader, diffOutReaderClose := cmd.MakeStdoutPipe()
defer diffOutReaderClose()
err := cmd.AddDynamicArguments(baseBranch, headBranch).
return cmd.AddDynamicArguments(baseBranch, headBranch).
WithRepo(repo).
WithPipelineFunc(func(ctx gitcmd.Context) error {
// Now scan the output from the command
@@ -228,8 +227,7 @@ func getDiffTree(ctx context.Context, repo git.RepositoryFacade, baseBranch, hea
}
return scanner.Err()
}).
Run(ctx)
return err
RunWithStderr(ctx)
}
// ErrRebaseConflicts represents an error if rebase fails with a conflict

View File

@@ -78,7 +78,7 @@ func doMergeRebaseFastForward(ctx *mergeContext) error {
return err
}
addCommitSigningOptions(cmdCommit, ctx.signKey)
if err := cmdCommit.WithRepo(ctx.tmpRepo).Run(ctx); err != nil {
if err := cmdCommit.WithRepo(ctx.tmpRepo).RunWithStderr(ctx); err != nil {
log.Error("Unable to amend commit message: %v", err)
return err
}

View File

@@ -41,7 +41,7 @@ func checkConflictsMergeTree(ctx context.Context, pr *issues_model.PullRequest,
// against the current base commit (baseCommitID) using `git diff-tree`. The command returns exit code 0
// if there is no diff between these trees (empty patch) and exit code 1 if there is a diff.
gitErr := gitcmd.NewCommand("diff-tree", "-r", "--quiet").
AddDynamicArguments(treeHash, baseCommitID).WithRepo(pr.BaseRepo).Run(ctx)
AddDynamicArguments(treeHash, baseCommitID).WithRepo(pr.BaseRepo).RunWithStderr(ctx)
switch {
case gitErr == nil:
log.Debug("PullRequest[%d]: Patch is empty - ignoring", pr.ID)

View File

@@ -38,8 +38,7 @@ type prTmpRepoContext struct {
}
// PrepareGitCmd prepares a git command with the correct directory, environment, and output buffers
// This function can only be called with gitcmd.Run()
// Do NOT use it with gitcmd.RunStd*() functions, otherwise it will panic
// Use RunWithStderr because this already configures a stdout buffer.
func (ctx *prTmpRepoContext) PrepareGitCmd(cmd *gitcmd.Command) *gitcmd.Command {
ctx.outbuf.Reset()
return cmd.WithRepo(ctx.tmpRepo).WithStdoutBuffer(ctx.outbuf)

View File

@@ -195,7 +195,7 @@ func lfsCommitAndPushTest(t *testing.T, dstPath string, sizes ...int) (pushedFil
t.Run("CommitAndPushLFS", func(t *testing.T) {
defer tests.PrintCurrentTest(t)()
prefix := "lfs-data-file-"
err := gitcmd.NewCommand("lfs").AddArguments("install").WithDir(dstPath).Run(t.Context())
var err error = gitcmd.NewCommand("lfs").AddArguments("install").WithDir(dstPath).RunWithStderr(t.Context())
assert.NoError(t, err)
_, _, err = gitcmd.NewCommand("lfs").AddArguments("track").AddDynamicArguments(prefix + "*").
WithDir(dstPath).RunStdString(t.Context())
@@ -427,7 +427,7 @@ func doBranchProtectPRMerge(baseCtx *APITestContext, dstPath string) func(t *tes
// Try to force push without force push permissions, which should fail
t.Run("ForcePushWithoutForcePermissions", func(t *testing.T) {
t.Run("CreateDivergentHistory", func(t *testing.T) {
gitcmd.NewCommand("reset", "--hard", "HEAD~1").WithDir(dstPath).Run(t.Context())
require.NoError(t, gitcmd.NewCommand("reset", "--hard", "HEAD~1").WithDir(dstPath).RunWithStderr(t.Context()))
_, err := generateCommitWithNewData(t.Context(), testFileSizeSmall, dstPath, "user2@example.com", "User Two", "branch-data-file-new")
assert.NoError(t, err)
})
@@ -873,11 +873,11 @@ func doCreateAgitFlowPull(dstPath string, ctx *APITestContext, headBranch string
})
t.Run("Push", func(t *testing.T) {
err := gitcmd.NewCommand("push", "origin", "HEAD:refs/for/master", "-o").
runErr := gitcmd.NewCommand("push", "origin", "HEAD:refs/for/master", "-o").
AddDynamicArguments("topic=" + headBranch).
WithDir(dstPath).
Run(t.Context())
require.NoError(t, err)
RunWithStderr(t.Context())
require.NoError(t, runErr)
unittest.AssertCount(t, &issues_model.PullRequest{}, pullNum+1)
pr1 = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{
@@ -945,11 +945,11 @@ func doCreateAgitFlowPull(dstPath string, ctx *APITestContext, headBranch string
})
t.Run("Push2", func(t *testing.T) {
err := gitcmd.NewCommand("push", "origin", "HEAD:refs/for/master", "-o").
runErr := gitcmd.NewCommand("push", "origin", "HEAD:refs/for/master", "-o").
AddDynamicArguments("topic=" + headBranch).
WithDir(dstPath).
Run(t.Context())
require.NoError(t, err)
RunWithStderr(t.Context())
require.NoError(t, runErr)
unittest.AssertCount(t, &issues_model.PullRequest{}, pullNum+2)
prMsg, err := doAPIGetPullRequest(*ctx, ctx.Username, ctx.Reponame, pr1.Index)(t)

View File

@@ -113,7 +113,7 @@ func TestAgitPullPush(t *testing.T) {
assert.NoError(t, err)
// create PR via agit
err = gitcmd.NewCommand(testCase.pushArgs...).WithDir(dstPath).Run(t.Context())
err = gitcmd.NewCommand(testCase.pushArgs...).WithDir(dstPath).RunWithStderr(t.Context())
assert.NoError(t, err)
// check pull request exist
@@ -132,14 +132,14 @@ func TestAgitPullPush(t *testing.T) {
// push 2
err = gitcmd.NewCommand("push", "origin", "HEAD:refs/for/master/test-agit-push").
WithDir(dstPath).
Run(t.Context())
RunWithStderr(t.Context())
assert.NoError(t, err)
pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: 1, Flow: issues_model.PullRequestFlowAGit, HeadBranch: "user2/test-agit-push"})
assert.Equal(t, 0, pr.CommitsBehind)
assert.Equal(t, 2, pr.CommitsAhead)
// reset to first commit
err = gitcmd.NewCommand("reset", "--hard", "HEAD~1").WithDir(dstPath).Run(t.Context())
err = gitcmd.NewCommand("reset", "--hard", "HEAD~1").WithDir(dstPath).RunWithStderr(t.Context())
assert.NoError(t, err)
// test force push without confirm
@@ -153,7 +153,7 @@ func TestAgitPullPush(t *testing.T) {
// test force push with confirm
err = gitcmd.NewCommand("push", "origin", "HEAD:refs/for/master/test-agit-push", "-o", "force-push").
WithDir(dstPath).
Run(t.Context())
RunWithStderr(t.Context())
assert.NoError(t, err)
_, stderr, _ = gitcmd.NewCommand("push", "origin", "HEAD:refs/for/no-such-branch/test-agit-push").
@@ -193,7 +193,7 @@ func TestAgitPullPush(t *testing.T) {
err = gitcmd.NewCommand("push", "origin", "HEAD:refs/for/master/test-agit-review").
WithDir(dstPath).
Run(t.Context())
RunWithStderr(t.Context())
assert.NoError(t, err)
// Reload PR to get updated commit ID

View File

@@ -402,7 +402,7 @@ func TestCreateAgitPullWithReadPermission(t *testing.T) {
err := gitcmd.NewCommand("push", "origin", "HEAD:refs/for/master", "-o").
AddDynamicArguments("topic=test-topic").
WithDir(dstPath).
Run(t.Context())
RunWithStderr(t.Context())
assert.NoError(t, err)
repo = unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user2", Name: "repo1"})

View File

@@ -407,7 +407,7 @@ func TestCantMergeUnrelated(t *testing.T) {
Name: "repo1",
})
err := gitcmd.NewCommand("read-tree", "--empty").WithRepo(repo1).Run(t.Context())
var err error = gitcmd.NewCommand("read-tree", "--empty").WithRepo(repo1).RunWithStderr(t.Context())
assert.NoError(t, err)
stdout, _, err := gitcmd.NewCommand("hash-object", "-w", "--stdin").