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): .git

strings.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 lint clean
  • make test-changed clean (3375 tests, 0 failures)
  • Verified the German failure mode reproduces on main before the change
Edited by Jay McCure

Merge request reports

Loading
Loading