fix(git): detect "no git repository" by exit status, not message text
Why this is a draft targeting another branch
Stacked on !3623 (merged) — it edits the PreRun head-repo closure that only exists
there, so it cannot target main until !3623 (merged) merges. Opening it now so the
follow-up is visible and linked rather than living in someone's working tree.
Should retarget to main automatically once !3623 (merged) lands; ready to un-draft at
that point.
Follow-up to the discussion on !3623 (merged) (threads 3669144335 and 3669169582) — please don't resolve those as fixed by !3623 (merged) itself.
Problem
!3623 (merged) needs to tell "there is no git repository here" apart from a genuine git
failure. The only signal available was git's stderr text, and git localizes it.
Under LC_ALL=de_DE.UTF-8, outside a repo:
Schwerwiegend: Kein Git-Repository (oder irgendeines der Elternverzeichnisse): .gitstrings.Contains(msg, "not a git repository") does not match that, so the
fallback never fires and mr create outside a git repo keeps failing for
anyone not running in English. Reproduces on a stock macOS git install.
Note this is not a regression in !3623 (merged) — those users are exactly where they were before it. But it does mean the feature only half-ships without this.
Approach
git.ErrNotAGitRepository, tagged onto the error in Remotes() when
GitDir() reports we are outside a repo:
if _, gitDirErr := GitDir(); gitDirErr != nil {
return nil, fmt.Errorf(\"%w: %w\", ErrNotAGitRepository, err)
}GitDir() keys off exit status rather than text, so detection is
locale-independent by construction — no LC_ALL pinning required. The double
%w keeps git's own message for display while making errors.Is work.
This follows the pattern already in the package: ErrNotOnAnyBranch and
CurrentBranch() convert a raw *run.CmdError into a domain sentinel at the
internal/git boundary so callers never inspect command internals.
Also promotes the inline errors.New(\"no git remotes found\") to
cmdutils.ErrNoGitRemotes — same reasoning, and it was already returned
unwrapped, so errors.Is works with no other change.
mr create then reads:
switch {
case err == nil:
return resolved()
case errors.Is(err, git.ErrNotAGitRepository), errors.Is(err, cmdutils.ErrNoGitRemotes):
return f.BaseRepo()
default:
return nil, err
}No new imports; no new package dependencies (cmdutils already imports git,
not the reverse).
Note on the changed test stubs
TestMRCreate_NoGitRepo_FlagCompleteSucceeds and
TestMRCreate_PushOutsideGitRepo_PropagatesError had RemotesStub return a
plain errors.New(\"fatal: not a git repository...\"). They now return the
sentinel-wrapped form, because the stub stands in for git.Remotes() and that
is what git.Remotes() returns. Worth knowing that this means those tests no
longer say anything about real git output — TestRemotes_ReturnsSentinelOutsideGitRepo
covers that end, driving the real binary under LC_ALL=de_DE.UTF-8.
That test asserts errors.Is, plus \"git: exit status 128\" to prove the
underlying error is preserved rather than replaced. It deliberately does not
assert on the German text, which depends on git translations being installed.
Test plan
-
go test ./internal/git/... ./internal/cmdutils/... ./internal/commands/mr/create/...passes -
make lintclean -
make test-changedclean (3375 tests, 0 failures) - Verified the German failure mode reproduces on
mainbefore the change