refactor(git): replace git stderr matching with exit codes (#39866)

Matching git's stderr text is fragile because it breaks silently when
git rewords a message. Where git offers an exit code or a query with a
defined answer, use that instead, and drop matches that could never
trigger. Follow-up to the discussion in https://github.com/go-gitea/gitea/pull/39857.

- Detect a missing remote by `git remote` exit code 2
- Resolve annotated tags to their tree with cat-file `^{tree}` instead
of spawning `ls-tree` when listing tree entries
- Skip the wiki page lookup when the default branch doesn't exist yet
- Remove unreachable "no merge base" and "unknown revision" checks

modules/git/compare.go

@@ -46,21 +46,13 @@ func GetDivergingCommits(ctx context.Context, repo RepositoryFacade, baseBranch,
 // GetCommitIDsBetweenReverse returns the last commit IDs between two commits in reverse order (from old to new) with limit.
 // If the result exceeds the limit, the old commits IDs will be ignored
 func GetCommitIDsBetweenReverse(ctx context.Context, repo RepositoryFacade, startRef, endRef, notRef string, limit int) ([]string, error) {
-	genCmd := func(reversions ...string) *gitcmd.Command {
-		cmd := gitcmd.NewCommand("rev-list", "--reverse").
-			AddArguments("-n").AddDynamicArguments(strconv.Itoa(limit)).
-			AddDynamicArguments(reversions...)
-		if notRef != "" { // --not should be kept as the last parameter of git command, otherwise the result will be wrong
-			cmd.AddOptionValues("--not", notRef)
-		}
-		return cmd
-	}
-	stdout, _, err := genCmd(startRef + ".." + endRef).WithRepo(repo).RunStdString(ctx)
-	if gitcmd.IsStderr(err, gitcmd.StderrNoMergeBase) {
-		// if the start and end are not related (no merge base), just get all commits pushed by "end ref"
-		// previously it would return the results of git rev-list before last so let's try that...
-		stdout, _, err = genCmd(endRef).WithRepo(repo).RunStdString(ctx)
+	cmd := gitcmd.NewCommand("rev-list", "--reverse").
+		AddArguments("-n").AddDynamicArguments(strconv.Itoa(limit)).
+		AddDynamicArguments(startRef + ".." + endRef)
+	if notRef != "" { // --not should be kept as the last parameter of git command, otherwise the result will be wrong
+		cmd.AddOptionValues("--not", notRef)
 	}
+	stdout, _, err := cmd.WithRepo(repo).RunStdString(ctx)
 	if err != nil {
 		return nil, err
 	}

modules/git/gitcmd/error.go

@@ -95,14 +95,10 @@ const (
 	StderrNoSuchPath         StderrPrefix = "fatal: no such path"
 	StderrBadObject          StderrPrefix = "fatal: bad object "
 
-	StderrNoSuchRemote StderrPrefix = "error: no such remote"
-
 	StderrAuthenticationFailed StderrPrefix = "fatal: Authentication failed for"
 	StderrCouldNotReadUsername StderrPrefix = "fatal: could not read Username"
 
-	StderrUnknownRevisionOrPath StderrRegexp = "^fatal: .*: unknown revision or path not in the working tree"
-	StderrNoMergeBase           StderrRegexp = "^fatal: .*: no merge base"
-	StderrFileNoEnoughLines     StderrRegexp = `^fatal: file .* has only \d+ lines?`
+	StderrFileNoEnoughLines StderrRegexp = `^fatal: file .* has only \d+ lines?`
 )
 
 func matchStderrCheck(stderr string, checkIntf StderrCheck) (match bool) {

modules/git/gitcmd/error_test.go

@@ -14,8 +14,6 @@ func TestIsStderr(t *testing.T) {
 		check  StderrCheck
 		stderr string
 	}{
-		{StderrUnknownRevisionOrPath, "fatal: ambiguous argument 'origin': unknown revision or path not in the working tree...."},
-		{StderrNoMergeBase, "fatal: origin/main..HEAD: no merge base...."},
 		{StderrFileNoEnoughLines, "fatal: file foo/bar has only 1 line"},
 		{StderrAuthenticationFailed, "Cloning into 'repo'...\r\nremote: Invalid username or token.\r\nfatal: Authentication failed for 'https://host/repo.git/'\r\n"},
 	}

modules/git/remote.go

@@ -64,9 +64,9 @@ func (err *ErrInvalidCloneAddr) Unwrap() error {
 	return util.ErrInvalidArgument
 }
 
-// IsRemoteNotExistError checks the prefix of the error message to see whether a remote does not exist.
+// IsRemoteNotExistError checks whether a "git remote" subcommand failed because the remote does not exist
 func IsRemoteNotExistError(err error) bool {
-	return gitcmd.IsStderr(err, gitcmd.StderrNoSuchRemote)
+	return gitcmd.IsErrorExitCode(err, 2)
 }
 
 // ParseRemoteAddr checks if given remote address is valid,

modules/git/repo_commit.go

@@ -317,28 +317,19 @@ func (repo *Repository) CommitsByFileAndRange(ctx context.Context, opts CommitsB
 // CommitsBetween returns a list that contains commits between [after, before). After is the first item in the slice.
 // If "before" and "after" are not related, it returns the all commits for the "after" commit.
 func (repo *Repository) CommitsBetween(ctx context.Context, afterRef, beforeRef RefName, limit int, optSkip ...int) ([]*Commit, error) {
-	gitCmd := func() *gitcmd.Command {
-		cmd := gitcmd.NewCommand("rev-list").WithRepo(repo)
-		if limit >= 0 {
-			cmd.AddOptionValues("--max-count", strconv.Itoa(limit))
-		}
-		if len(optSkip) > 0 {
-			cmd.AddOptionValues("--skip", strconv.Itoa(optSkip[0]))
-		}
-		return cmd
+	cmd := gitcmd.NewCommand("rev-list").WithRepo(repo)
+	if limit >= 0 {
+		cmd.AddOptionValues("--max-count", strconv.Itoa(limit))
+	}
+	if len(optSkip) > 0 {
+		cmd.AddOptionValues("--skip", strconv.Itoa(optSkip[0]))
 	}
-	var stdout []byte
-	var err error
 	if beforeRef == "" {
-		stdout, _, err = gitCmd().AddDynamicArguments(afterRef.String()).RunStdBytes(ctx)
+		cmd.AddDynamicArguments(afterRef.String())
 	} else {
-		stdout, _, err = gitCmd().AddDynamicArguments(beforeRef.String() + ".." + afterRef.String()).RunStdBytes(ctx)
-		if gitcmd.IsStderr(err, gitcmd.StderrNoMergeBase) {
-			// future versions of git >= 2.28 are likely to return an error if before and last have become unrelated.
-			// if the beforeRef and afterRef are not related (no merge base), just get all commits pushed by afterRef
-			stdout, _, err = gitCmd().AddDynamicArguments(afterRef.String()).RunStdBytes(ctx)
-		}
+		cmd.AddDynamicArguments(beforeRef.String() + ".." + afterRef.String())
 	}
+	stdout, _, err := cmd.RunStdBytes(ctx)
 	if err != nil {
 		return nil, err
 	}

modules/git/repo_compare.go

@@ -46,11 +46,6 @@ func (repo *Repository) GetDiffNumChangedFiles(ctx context.Context, base, head s
 		WithRepo(repo).
 		WithStdoutCopy(w).
 		RunWithStderr(ctx); err != nil {
-		if gitcmd.IsStderr(err, gitcmd.StderrNoMergeBase) {
-			// git >= 2.28 now returns an error if base and head have become unrelated.
-			// it doesn't make sense to count the changed files in this case because UI won't display such diff
-			return 0, nil
-		}
 		return 0, err
 	}
 	return w.numLines, nil

modules/git/tree.go

@@ -83,7 +83,7 @@ type Tree struct {
 }
 
 // ListEntries returns all entries of current tree.
-func (t *Tree) ListEntries(ctx context.Context, gitRepo *Repository) (Entries, error) {
+func (t *Tree) ListEntries(_ctx context.Context, gitRepo *Repository) (Entries, error) {
 	if t.entriesParsed {
 		return t.entries, nil
 	}
@@ -109,36 +109,22 @@ func (t *Tree) ListEntries(ctx context.Context, gitRepo *Repository) (Entries, e
 			return nil, err
 		}
 	}
-	if info.Type == "tree" {
-		t.entries, err = catBatchParseTreeEntries(t.ID.Type(), t, rd, info.Size)
+	if info.Type != "tree" { // resolve annotated tags to their tree, a blob yields ErrNotExist
+		if err := DiscardFull(rd, info.Size+1); err != nil {
+			return nil, err
+		}
+		info, rd, err = batch.QueryContent(t.ID.String() + "^{tree}")
 		if err != nil {
 			return nil, err
 		}
-		t.entriesParsed = true
-		return t.entries, nil
 	}
 
-	// Not a tree just use ls-tree instead
-	if err := DiscardFull(rd, info.Size+1); err != nil {
+	t.entries, err = catBatchParseTreeEntries(t.ID.Type(), t, rd, info.Size)
+	if err != nil {
 		return nil, err
 	}
-
-	stdout, _, runErr := gitcmd.NewCommand("ls-tree", "-l").AddDynamicArguments(t.ID.String()).WithRepo(gitRepo).RunStdBytes(ctx)
-	if runErr != nil {
-		if gitcmd.IsStderr(runErr, gitcmd.StderrNotValidObjectName) || gitcmd.IsStderr(runErr, gitcmd.StderrNotTreeObject) {
-			return nil, ErrNotExist{
-				ID: t.ID.String(),
-			}
-		}
-		return nil, runErr
-	}
-
-	t.entries, err = parseTreeEntries(stdout, t)
-	if err == nil {
-		t.entriesParsed = true
-	}
-
-	return t.entries, err
+	t.entriesParsed = true
+	return t.entries, nil
 }
 
 // listEntriesRecursive returns all entries of current tree recursively including all subtrees

modules/git/tree_test.go

@@ -8,6 +8,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestSubTree_Issue29101(t *testing.T) {
@@ -26,6 +27,24 @@ func TestSubTree_Issue29101(t *testing.T) {
 	}
 }
 
+func TestTreeListEntriesResolvesTagAndRejectsBlob(t *testing.T) {
+	repo, err := OpenRepositoryLocal(t.Context(), filepath.Join(testReposDir, "repo1_bare"))
+	require.NoError(t, err)
+	defer repo.Close()
+
+	entries, err := newTree(MustIDFromString("3ad28a9149a2864384548f3d17ed7f38014c9e8a")).ListEntries(t.Context(), repo)
+	require.NoError(t, err)
+	names := make([]string, 0, len(entries))
+	for _, entry := range entries {
+		names = append(names, entry.Name())
+	}
+	assert.Equal(t, []string{"file1.txt", "file2.txt", "foo"}, names)
+	assert.True(t, entries[2].IsDir())
+
+	_, err = newTree(MustIDFromString("e2129701f1a4d54dc44f03c93bca0a2aec7c5449")).ListEntries(t.Context(), repo)
+	assert.True(t, IsErrNotExist(err))
+}
+
 func Test_GetTreePathLatestCommit(t *testing.T) {
 	repo, err := OpenRepositoryLocal(t.Context(), filepath.Join(testReposDir, "repo6_blame"))
 	assert.NoError(t, err)

routers/web/repo/pull.go

@@ -370,8 +370,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 := errors.Is(err, util.ErrNotExist) || gitcmd.IsStderr(err, gitcmd.StderrNotValidObjectName) || gitcmd.IsStderr(err, gitcmd.StderrUnknownRevisionOrPath)
-		if !isKnownErrorForBroken {
+		if !errors.Is(err, util.ErrNotExist) && !gitcmd.IsStderr(err, gitcmd.StderrNotValidObjectName) {
 			log.Error("GetCompareInfo: %v", err)
 		}
 		prInfo.IsPullRequestBroken = true

services/mirror/mirror_pull.go

@@ -43,7 +43,7 @@ func UpdateAddress(ctx context.Context, m *repo_model.Mirror, addr string) error
 	}
 
 	err = git.ManagedRemoteAdd(ctx, repo, remoteName, addr, git.RemoteOptionMirrorFetch)
-	if err != nil && !git.IsRemoteNotExistError(err) {
+	if err != nil {
 		return err
 	}
 
@@ -56,7 +56,7 @@ func UpdateAddress(ctx context.Context, m *repo_model.Mirror, addr string) error
 		}
 
 		err = git.ManagedRemoteAdd(ctx, repo.WikiStorageRepo(), remoteName, wikiRemotePath, git.RemoteOptionMirrorFetch)
-		if err != nil && !git.IsRemoteNotExistError(err) {
+		if err != nil {
 			return err
 		}
 	}

services/wiki/wiki.go

@@ -15,7 +15,6 @@ import (
 	"gitea.dev/models/unit"
 	user_model "gitea.dev/models/user"
 	"gitea.dev/modules/git"
-	"gitea.dev/modules/git/gitcmd"
 	"gitea.dev/modules/globallock"
 	"gitea.dev/modules/graceful"
 	"gitea.dev/modules/log"
@@ -50,17 +49,18 @@ func InitWiki(ctx context.Context, repo *repo_model.Repository) error {
 }
 
 // prepareGitPath try to find a suitable file path with file name by the given raw wiki name.
+// An empty defaultWikiBranch means the branch doesn't exist yet.
 // return: existence, prepared file path with name, error
 func prepareGitPath(ctx context.Context, gitRepo *git.Repository, defaultWikiBranch string, wikiPath WebPath) (bool, string, error) {
 	unescaped := string(wikiPath) + ".md"
 	gitPath := WebPathToGitPath(wikiPath)
+	if defaultWikiBranch == "" {
+		return false, gitPath, nil
+	}
 
 	// Look for both files
 	filesInIndex, err := gitRepo.LsTree(ctx, defaultWikiBranch, unescaped, gitPath)
 	if err != nil {
-		if gitcmd.IsStderr(err, gitcmd.StderrNotValidObjectName) {
-			return false, gitPath, nil // branch doesn't exist
-		}
 		log.Error("Wiki LsTree failed, err: %v", err)
 		return false, gitPath, err
 	}
@@ -136,7 +136,7 @@ func updateWikiPage(ctx context.Context, doer *user_model.User, repo *repo_model
 		}
 	}
 
-	isWikiExist, newWikiPath, err := prepareGitPath(ctx, gitRepo, repo.DefaultWikiBranch, newWikiName)
+	isWikiExist, newWikiPath, err := prepareGitPath(ctx, gitRepo, cloneOpts.Branch, newWikiName)
 	if err != nil {
 		return err
 	}
@@ -152,7 +152,7 @@ func updateWikiPage(ctx context.Context, doer *user_model.User, repo *repo_model
 		isOldWikiExist := true
 		oldWikiPath := newWikiPath
 		if oldWikiName != newWikiName {
-			isOldWikiExist, oldWikiPath, err = prepareGitPath(ctx, gitRepo, repo.DefaultWikiBranch, oldWikiName)
+			isOldWikiExist, oldWikiPath, err = prepareGitPath(ctx, gitRepo, cloneOpts.Branch, oldWikiName)
 			if err != nil {
 				return err
 			}

services/wiki/wiki_test.go

@@ -300,18 +300,7 @@ func TestPrepareWikiFileName(t *testing.T) {
 }
 
 func TestPrepareWikiFileName_FirstPage(t *testing.T) {
-	// Now create a temporaryDirectory
-	tmpDir := t.TempDir()
-
-	err := git.InitRepositoryLocal(t.Context(), tmpDir, true, git.Sha1ObjectFormat.Name(), "")
-	assert.NoError(t, err)
-
-	gitRepo, err := git.OpenRepositoryLocal(t.Context(), tmpDir)
-	require.NoError(t, err)
-
-	defer gitRepo.Close()
-
-	existence, newWikiPath, err := prepareGitPath(t.Context(), gitRepo, "master", "Home")
+	existence, newWikiPath, err := prepareGitPath(t.Context(), nil, "", "Home")
 	assert.False(t, existence)
 	assert.NoError(t, err)
 	assert.Equal(t, "Home.md", newWikiPath)