From 743bbaa9c2a77894302d819a48c33c36cee31946 Mon Sep 17 00:00:00 2001 From: Sandro Date: Sat, 6 Jun 2026 13:06:08 +0200 Subject: [PATCH] fix: refactor git error handling and make archive streaming handle non-existing commit id (#38007) Co-authored-by: wxiaoguang --- modules/git/gitcmd/error.go | 62 +++++++++++++++--------- modules/git/remote.go | 6 +-- modules/git/repo_commit.go | 6 +-- modules/git/repo_commit_nogogit.go | 14 +++--- modules/git/tree_nogogit.go | 3 +- routers/api/v1/repo/pull.go | 2 +- routers/web/repo/pull.go | 4 +- services/repository/archiver/archiver.go | 6 +-- services/wiki/wiki.go | 2 +- tests/integration/wiki_test.go | 2 +- 10 files changed, 57 insertions(+), 50 deletions(-) diff --git a/modules/git/gitcmd/error.go b/modules/git/gitcmd/error.go index 436f4e18ae..8d5ec5f5e6 100644 --- a/modules/git/gitcmd/error.go +++ b/modules/git/gitcmd/error.go @@ -9,6 +9,8 @@ import ( "fmt" "os/exec" "strings" + + "gitea.dev/modules/util" ) type RunStdError interface { @@ -41,32 +43,14 @@ func (r *runStdError) Stderr() string { } func ErrorAsStderr(err error) (string, bool) { - var runErr RunStdError - if errors.As(err, &runErr) { + if runErr, ok := errors.AsType[RunStdError](err); ok { return runErr.Stderr(), true } return "", false } -func StderrHasPrefix(err error, prefix string) bool { - stderr, ok := ErrorAsStderr(err) - if !ok { - return false - } - return strings.HasPrefix(stderr, prefix) -} - -func StderrContains(err error, sub string) bool { - stderr, ok := ErrorAsStderr(err) - if !ok { - return false - } - return strings.Contains(stderr, sub) -} - func IsErrorExitCode(err error, code int) bool { - var exitError *exec.ExitError - if errors.As(err, &exitError) { + if exitError, ok := errors.AsType[*exec.ExitError](err); ok { return exitError.ExitCode() == code } return false @@ -85,11 +69,41 @@ func IsErrorCanceledOrKilled(err error) bool { return errors.Is(err, context.Canceled) || IsErrorSignalKilled(err) } -func IsStdErrorNotValidObjectName(err error) bool { +type StderrPrefix string + +type StderrSubStr string + +const ( + StderrNotValidObjectName StderrPrefix = "fatal: not a valid object name" + StderrNotTreeObject StderrPrefix = "fatal: not a tree object" + StderrPathSpec StderrPrefix = "fatal: pathspec" + StderrBadRevision StderrPrefix = "fatal: bad revision" + + StderrNoSuchRemote1 StderrPrefix = "fatal: no such remote" // git < 2.30, exit status 128 + StderrNoSuchRemote2 StderrPrefix = "error: no such remote" // git >= 2.30. exit status 2 + + // fatal: ambiguous argument 'origin': unknown revision or path not in the working tree. + StderrUnknownRevisionOrPath StderrSubStr = "unknown revision or path not in the working tree" +) + +func IsStderr[T StderrPrefix | StderrSubStr](err error, check T) bool { stderr, ok := ErrorAsStderr(err) - // Git is lowercasing the "fatal: Not a valid object name" error message - // ref: https://lore.kernel.org/git/pull.2052.git.1771836302101.gitgitgadget@gmail.com - return ok && strings.Contains(strings.ToLower(stderr), "fatal: not a valid object name") + if !ok { + return false + } + checkLen := len(check) + if len(stderr) < checkLen { + return false + } + switch any(check).(type) { + case StderrPrefix: + // Git is lowercasing the "fatal: Not a valid object name" error message + // ref: https://lore.kernel.org/git/pull.2052.git.1771836302101.gitgitgadget@gmail.com + return util.AsciiEqualFold(stderr[:checkLen], string(check)) + case StderrSubStr: + return strings.Contains(stderr, string(check)) + } + return false } type pipelineError struct { diff --git a/modules/git/remote.go b/modules/git/remote.go index b4fe6cfd10..e94ca374e4 100644 --- a/modules/git/remote.go +++ b/modules/git/remote.go @@ -66,11 +66,7 @@ func (err *ErrInvalidCloneAddr) Unwrap() error { // IsRemoteNotExistError checks the prefix of the error message to see whether a remote does not exist. func IsRemoteNotExistError(err error) bool { - // see: https://github.com/go-gitea/gitea/issues/32889#issuecomment-2571848216 - // Should not add space in the end, sometimes git will add a `:` - prefix1 := "fatal: No such remote" // git < 2.30, exit status 128 - prefix2 := "error: No such remote" // git >= 2.30. exit status 2 - return gitcmd.StderrHasPrefix(err, prefix1) || gitcmd.StderrHasPrefix(err, prefix2) + return gitcmd.IsStderr(err, gitcmd.StderrNoSuchRemote1) || gitcmd.IsStderr(err, gitcmd.StderrNoSuchRemote2) } // ParseRemoteAddr checks if given remote address is valid, diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go index b955f2709b..20827062df 100644 --- a/modules/git/repo_commit.go +++ b/modules/git/repo_commit.go @@ -24,9 +24,9 @@ func (repo *Repository) GetTagCommitID(name string) (string, error) { return repo.GetRefCommitID(TagPrefix + name) } -// GetCommit returns commit object of by ID string. -func (repo *Repository) GetCommit(commitID string) (*Commit, error) { - id, err := repo.ConvertToGitID(commitID) +// GetCommit returns a commit object of by the git ref. +func (repo *Repository) GetCommit(ref string) (*Commit, error) { + id, err := repo.ConvertToGitID(ref) if err != nil { return nil, err } diff --git a/modules/git/repo_commit_nogogit.go b/modules/git/repo_commit_nogogit.go index c90650532b..d8b842beee 100644 --- a/modules/git/repo_commit_nogogit.go +++ b/modules/git/repo_commit_nogogit.go @@ -109,16 +109,16 @@ func (repo *Repository) getCommitWithBatch(batch CatFileBatch, id ObjectID) (*Co } } -// ConvertToGitID returns a GitHash object from a potential ID string -func (repo *Repository) ConvertToGitID(commitID string) (ObjectID, error) { +// ConvertToGitID returns a git object ID from the git ref, it doesn't guarantee the returned ID really exists +func (repo *Repository) ConvertToGitID(ref string) (ObjectID, error) { objectFormat, err := repo.GetObjectFormat() if err != nil { return nil, err } - if len(commitID) == objectFormat.FullLength() && objectFormat.IsValid(commitID) { - ID, err := NewIDFromString(commitID) + if len(ref) == objectFormat.FullLength() && objectFormat.IsValid(ref) { + id, err := NewIDFromString(ref) if err == nil { - return ID, nil + return id, nil } } @@ -127,10 +127,10 @@ func (repo *Repository) ConvertToGitID(commitID string) (ObjectID, error) { return nil, err } defer cancel() - info, err := batch.QueryInfo(commitID) + info, err := batch.QueryInfo(ref) if err != nil { if IsErrNotExist(err) { - return nil, ErrNotExist{commitID, ""} + return nil, ErrNotExist{ref, ""} } return nil, err } diff --git a/modules/git/tree_nogogit.go b/modules/git/tree_nogogit.go index 1d78ecdee2..ebb4c7bb12 100644 --- a/modules/git/tree_nogogit.go +++ b/modules/git/tree_nogogit.go @@ -7,7 +7,6 @@ package git import ( "io" - "strings" "gitea.dev/modules/git/gitcmd" ) @@ -65,7 +64,7 @@ func (t *Tree) ListEntries() (Entries, error) { stdout, _, runErr := gitcmd.NewCommand("ls-tree", "-l").AddDynamicArguments(t.ID.String()).WithDir(t.repo.Path).RunStdBytes(t.repo.Ctx) if runErr != nil { - if gitcmd.IsStdErrorNotValidObjectName(runErr) || strings.Contains(runErr.Error(), "fatal: not a tree object") { + if gitcmd.IsStderr(runErr, gitcmd.StderrNotValidObjectName) || gitcmd.IsStderr(runErr, gitcmd.StderrNotTreeObject) { return nil, ErrNotExist{ ID: t.ID.String(), } diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 123c0c671a..6f6ba77fae 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -1435,7 +1435,7 @@ func GetPullRequestCommits(ctx *context.APIContext) { compareInfo, err = git_service.GetCompareInfo(ctx, pr.BaseRepo, pr.BaseRepo, baseGitRepo, git.RefNameFromBranch(pr.BaseBranch), git.RefName(pr.GetGitHeadRefName()), false, false) } - if gitcmd.StderrHasPrefix(err, "fatal: bad revision") { + if gitcmd.IsStderr(err, gitcmd.StderrBadRevision) { ctx.APIError(http.StatusNotFound, "invalid base branch or revision") return } else if err != nil { diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 6862a3efc0..eb6d5408a5 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -376,9 +376,7 @@ func (prInfo *pullRequestViewInfo) prepareViewFillCompareInfo(ctx *context.Conte pull := prInfo.issue.PullRequest prInfo.CompareInfo, err = git_service.GetCompareInfo(ctx, ctx.Repo.Repository, ctx.Repo.Repository, ctx.Repo.GitRepo, baseRef, git.RefName(pull.GetGitHeadRefName()), false, false) if err != nil { - isKnownErrorForBroken := gitcmd.IsStdErrorNotValidObjectName(err) || - // fatal: ambiguous argument 'origin': unknown revision or path not in the working tree. - gitcmd.StderrContains(err, "unknown revision or path not in the working tree") + isKnownErrorForBroken := gitcmd.IsStderr(err, gitcmd.StderrNotValidObjectName) || gitcmd.IsStderr(err, gitcmd.StderrUnknownRevisionOrPath) if !isKnownErrorForBroken { log.Error("GetCompareInfo: %v", err) } diff --git a/services/repository/archiver/archiver.go b/services/repository/archiver/archiver.go index 93e83b51de..2b78118556 100644 --- a/services/repository/archiver/archiver.go +++ b/services/repository/archiver/archiver.go @@ -56,13 +56,13 @@ func NewRequest(repo *repo_model.Repository, gitRepo *git.Repository, archiveRef } // Get corresponding commit. - commitID, err := gitRepo.ConvertToGitID(archiveRefShortName) + commit, err := gitRepo.GetCommit(archiveRefShortName) if err != nil { return nil, util.NewNotExistErrorf("unrecognized repository reference: %s", archiveRefShortName) } r := &ArchiveRequest{Repo: repo, archiveRefShortName: archiveRefShortName, Type: archiveType, Paths: paths} - r.CommitID = commitID.String() + r.CommitID = commit.ID.String() return r, nil } @@ -330,7 +330,7 @@ func ServeRepoArchive(ctx *gitea_context.Base, archiveReq *ArchiveRequest) error // because errors may happen in git command and such cases aren't in our control. httplib.ServeSetHeaders(ctx.Resp, httplib.ServeHeaderOptions{Filename: downloadName}) if err := archiveReq.Stream(ctx, ctx.Resp); err != nil && !ctx.Written() { - if gitcmd.StderrHasPrefix(err, "fatal: pathspec") { + if gitcmd.IsStderr(err, gitcmd.StderrPathSpec) || gitcmd.IsStderr(err, gitcmd.StderrNotTreeObject) { return util.NewInvalidArgumentErrorf("path doesn't exist or is invalid") } return fmt.Errorf("archive repo %s: failed to stream: %w", archiveReq.Repo.FullName(), err) diff --git a/services/wiki/wiki.go b/services/wiki/wiki.go index 9fcc5750ec..abe846ae06 100644 --- a/services/wiki/wiki.go +++ b/services/wiki/wiki.go @@ -59,7 +59,7 @@ func prepareGitPath(gitRepo *git.Repository, defaultWikiBranch string, wikiPath // Look for both files filesInIndex, err := gitRepo.LsTree(defaultWikiBranch, unescaped, gitPath) if err != nil { - if gitcmd.IsStdErrorNotValidObjectName(err) { + if gitcmd.IsStderr(err, gitcmd.StderrNotValidObjectName) { return false, gitPath, nil // branch doesn't exist } log.Error("Wiki LsTree failed, err: %v", err) diff --git a/tests/integration/wiki_test.go b/tests/integration/wiki_test.go index 0c1b00e507..11eef67c68 100644 --- a/tests/integration/wiki_test.go +++ b/tests/integration/wiki_test.go @@ -72,7 +72,7 @@ EOF // reader can't push _, _, runErr = gitcmd.NewCommand("push", "origin", "refs/heads/master").WithDir(dstLocalPath).RunStdString(t.Context()) - assert.True(t, gitcmd.StderrContains(runErr, "remote: Repository not found\n")) + assert.Contains(t, runErr.Stderr(), "remote: Repository not found\n") req := NewRequest(t, "GET", "/user2/repo1/wiki/raw/Home.md") resp := MakeRequest(t, req, http.StatusOK) assert.Contains(t, resp.Body.String(), "This is the home page!")