A git refusal is read once, where git's words already are - #429
Conversation
`Failure` was added so a caller would not have to recover a fact by looking for
words in a message. Three callers were still looking: branch_manager matched
"already exists", repo_manager matched "couldn't find remote ref", and
render.rs lowercased a reason and sniffed three host phrases to decide whether
to print a hint -- a classification living in the renderer, one crate away from
the stderr it was reading.
Three arms now carry those facts -- BranchAlreadyExists, RefMissingOnRemote,
RepositoryNotFound -- and each is read by a named function next to the verb that
produces it. `captured_reading` takes the reader as an argument, so a phrase is
scoped to the verb that says it rather than to any git output that contains it:
"already exists" means the branch to `git branch` and the destination directory
to `git clone`, which the old flat substring could not tell apart. `reason`
stays for rendering and nothing branches on it.
Moving the readings turned up two live defects, which is what the tests are:
- repo_manager missed `Couldn't find remote ref` -- capital C, git up to v2.20.0
(remote.c:1785), and a bare die() rather than die(_()), so the pinned C locale
never reached it. A host on that git turned an ordinary "start a new branch"
launch into a failure, because the ref-missing answer read as one. The reader
is case-insensitive.
- The wrong-owner hint missed every host whose 404 body does not say "repository
not found". Codeberg answers `remote: Not found.`, so git's own
`repository '<url>' not found` (git-remote-http) is the only line there is,
and it is not the substring `repository not found`. Recorded from a live
clone; the reader matches git's line as a whole line ending in `' not found`,
which keeps out `repository '/path' does not exist` and git's other
`'%s' not found` messages.
Rejected: replacing the phrases with an exit code or a porcelain signal. git
exits 128 for all four of these and for refused keys and DNS failures besides,
and `git branch` has no --porcelain. Asking show-ref after a failed `git branch`
would cost a spawn on the launch path and answer a different question -- whether
the branch is there now, which a concurrent dl could have made true in between.
The words are the only signal, so the change is where they are read, not
whether.
Rejected: pinning LC_ALL=C on clone_bare to catch git's own translated
`repository '%s' not found`. It would put the whole clone failure in front of a
non-English reader in English to gain a hint. A lost candidate is the safe
direction.
Not done here: git.rs still holds two English strings of its own ("git is not on
PATH", "git ... timed out after Ns") against the crate's no-English rule. Moving
them means GitRefused carries the verb and the bound instead of a message, and
the fifteen sites that launder `reason` into a String field all have to carry
structure. Separate ticket.
Core's public surface gains `clients::git::Failure` and `GitRefused::how`;
public-api.rest.txt is hand-edited here because cargo-public-api needs a nightly
this environment has not got. CI's regeneration is authoritative.
Refs #399, map #406.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideMoves all git-English interpretation into verb-specific readers in the git client, exposes typed refusal facts to callers, and replaces brittle production substring checks with enum matching while adding coverage for historical, host-specific, and near-miss stderr forms. Sequence diagram for verb-scoped git refusal classificationsequenceDiagram
participant Flow as Flow caller
participant Git as clients::git
participant Process as git process
Flow->>Git: create_branch / fetch_ref / clone_bare
Git->>Process: Run git verb with captured stderr
Process-->>Git: exit status and stderr
Git->>Git: verb-specific reader(stderr)
Git-->>Flow: GitRefused { how: Failure, reason }
Flow->>Flow: Match refused.how()
alt BranchAlreadyExists
Flow-->>Flow: Treat branch creation as successful
else RefMissingOnRemote
Flow-->>Flow: Use default-branch fallback
else RepositoryNotFound
Flow-->>Flow: Offer wrong-owner hint
else Other Failure
Flow-->>Flow: Preserve and report reason
end
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Two-axis wf-review of fix/git-refusal-classification @ e7f5168, fixed point 57955a3...HEAD. Reviewed in fresh context, in a scratch worktree; nothing on the branch was touched.
Independently executed here (not taken from the PR body):
cargo test --workspace --no-fail-fast— green, 1591 tests, 0 failures, including every test #401/#416 lists as flaky.cargo clippy --locked --all-targets -- -D warningsandcargo fmt --check— clean.- Both claimed reds, re-run against unmodified production code. Both hold up (below).
- All four git wordings, live, against git 2.51.1 and against git's own source at the tags cited.
Standards
Judged against the repo's own rules first — devlaunch-runner/src/lib.rs:47-52 ("Nothing here holds a message meant for a person"), domain/metadata.rs:25-28 ("core renders no English"), clients/git.rs's "what they get from here is verbs", and docs/development.md:53 on the clients:: tier — then the Fowler baseline.
Nothing blocking. The move is in the direction the standards point: a Feature Envy (render.rs classifying git's stderr) and a Primitive Obsession (a String carrying a decision) both leave, and git.rs:149's new claim "Nothing branches on it" is now literally true — grepped, see Spec §2.
Widening Failure to pub is the right seam and matches established precedent exactly: clients::gh::GhUnavailable, clients::ssh::NotRun and clients::devpod::ListingUnreadable are all pub and all imported by render.rs. dl is a separate crate, so there is no narrower one. The rejected alternative in the PR body (a bool accessor per case) would reinstate the boolean-blindness the arms exist to remove. public-api.api.txt — the promise file — is correctly untouched.
The bare fn pointer Reading (git.rs:273) is right at this size: three module-private call sites, no reader that varies at runtime, and naming it at the call site is what earns the verb-scoping test. reads_nothing is a null object rather than a Middle Man — it exists so captured and captured_reading share one path.
Findings, severity order — all non-blocking:
- No CHANGELOG entry.
CHANGELOG.mdsays "All notable changes to this project will be documented in this file", and 14 of the last 20 merges touch it. This is not a pure refactor: Codeberg/Forgejo now gets the wrong-owner hint it never got, and pre-2.21 git now takes the default-branch fallback instead of failing. Both are user-visible, and both are what[Unreleased]is for. _ =>on the enum this PR exists to make decidable.flows/repo_manager.rs:1508(new here) and:1423(pre-existing shape). A future arm lands inFetchOutcome::Failed/FetchRepoError::Refusedwith no compiler prompt — the exact silent-misclassification failure mode being removed one layer up. Safe today only becausefetch_ref/fetch_allare wired to readers that cannot emit the other arms, which nothing in the type says.branch_manager.rs:252sidesteps it correctly with==. Spelling the four process-ended arms out would restore exhaustiveness.- Three doc lines claim more than the code does.
git.rs:243— "Each reader below records what was tried instead of the words":branch_already_existsdoes (show-ref) andrepository_not_founddoes (the rejected shorter forms), butref_missing_on_remote(git.rs:308-315) records only git's version history.git.rs:1024— "The three callers are the three verbs a decision hangs off":captured(git.rs:1019) is a fourth caller, covering ~20 verbs.git.rs:18— "every caller matches an arm" is not true of the callers that only carryreason(). git.rs:22is 102 columns where the rest of the module header wraps at ~80; the edit inserted text and left the tail unrewrapped. rustfmt does not touch comments, so this is not fmt-enforced.
On the hand-edited public-api.rest.txt: CI's public-api job is the authority and it passed. Checked anyway, row by row, against GhUnavailable (:167-178), ssh::NotRun (:216-227) and devpod::WorkspaceSource (:125-137) — variants alphabetical under the decl; impl order Clone Eq PartialEq Debug Copy StructuralPartialEq identical; the enum sorted before the module's structs the way WorkspaceSource precedes Workspace; how inserted before reason inside the inherent impl. 1 decl + 7 variants + 10 impl/fn rows = 18. No inconsistent row.
Spec
Spec is #399, "git refusals travel as a type and as English, and three decisions read the English".
- All three named sites converted. Spec: "
flows/branch_manager.rs:252,flows/repo_manager.rs:1507,dl/src/render.rs:1887". Nowbranch_manager.rs:252,repo_manager.rs:1506-1507,render.rs:1892;reads_as_repository_not_founddeleted. ✅ - CONFIRMED — "
reasonstays for rendering; nothing branches on it" holds. Grepped all ofrust/includingdl/andaid/. Every surviving production.reason()is a.to_owned()into areason: Stringfield or aformat!— the ~15 laundering sites the spec explicitly permits, plusrender.rs:2038. Nocontains/starts_with/==on a laundered reason outside tests. The whole point of the ticket lands. ✅ - CONFIRMED — the deferred section really was deferred.
git.rs:210/:217still hold "git is not on PATH" and the timeout string, verbatim; #432 is open with that sizing. Not silently changed. ✅ - CONFIRMED — the highest-value risk is clean. The old renderer-side sniff classified any reason string regardless of which verb produced it; the new reader is scoped to
clone_bare, so a refusal arriving from another verb would silently lose the hint. Traced it:CloneError::GitRefusedhas exactly one construction site,repo_manager.rs:1238, fed solely byself.git.clone_bareat:1233. Same forrepo_manager.rs:1506(onlyfetch_ref) andbranch_manager.rs:252(onlycreate_branch). Every old substring check already saw exactly one verb. Scoping strictly narrows here, never widens — no regression. ✅ - CONFIRMED — verb scoping is right, and the phrases are right.
create_branchis the only classified branch creator;checkout_reset(git.rs:823) uses-B, which cannot say "already exists". Ran the real cases on git 2.51.1:fatal: a branch named 'feat' already exists; the namespace collision isfatal: cannot lock ref 'refs/heads/feat/x': 'refs/heads/feat' exists; cannot create ...— no "already", so the negative atgit/tests.rs:361is genuinely pinned, not tautological. Also scanned git 2.51.1's own binaries for every'%s' not foundformat string:branch '%s' not found,tag '%s' not found,replace ref '%s' not found,accepted promisor remote '%s' not found,remote-tracking branch '%s' not found— none containsrepository ', so the two-part line matcher atgit.rs:355-357cannot swallow any of them.repository '%s' does not existends the wrong way and is excluded. ✅ - CONFIRMED — git's source says exactly what the PR says it says. Fetched at the tags, not trusted from the comments:
remote.c@ v2.20.0:1785 →die("Couldn't find remote ref %s", name);— baredie(), capital C, untranslatable. Exactly the cited line number.remote.c@ v2.21.0:1840 →die(_("couldn't find remote ref %s"), name);branch.c@ v2.34.0:208 →die(_("A branch named '%s' already exists."),branch.c@ v2.35.0:307 →die(_("a branch named '%s' already exists"),remote-curl.c@ v2.51.0:522 →die(_("repository '%s' not found"),
All five verbatim, all at the cited lines. This is unusually well-sourced. ✅
- CONFIRMED — locale reasoning is sound.
c_locale()is used at exactly two sites,git.rs:559(fetch_ref) and:651(create_branch).branch_already_existsmatches the lowercase tail of adie(_())sentence, so it needs the C pin and has it.ref_missing_on_remoteis case-insensitive precisely because pre-2.21 git bypassed gettext — the one place the pin cannot help, and the reader compensates.clone_barepins nothing: three of its four phrases are the remote's bytes and never pass through gettext, and the fourth (repository '%s' not found) is translated, so under a non-C locale that one wording degrades to hint not offered — a candidate withheld, never a wrong one offered. The PR states this trade-off explicitly. ✅ - CRLF is safe. Rust's
str::lines()strips a trailing\r, so\r\nneeds no help fromtrim_end().Cloning into bare repository 'X'...containsrepository 'but ends'..., so the progress line is not a false positive. - FINDING (minor) — the spec's own named proving path has no red. Spec: "Red-first: change the fixture stderr to a reworded-but-equivalent git message and watch
branch_managertake the wrong path."branch_already_exists(git.rs:363-367) isstderr.contains("already exists")— byte-identical to the deletedrefused.reason().contains("already exists"). The branch path is a pure no-op refactor and both new branch tests pass against pre-PR code. The two reds coverrepo_managerandrenderinstead. The refactor is right; the specific proving path the ticket asked for was not proved. - SUSPECTED (low, pre-existing) — "not found" is also what a host says about a repo you cannot see. GitHub and GitLab answer 404 for private repos, so an access failure reads as
RepositoryNotFoundand offers a wrong-owner hint for a repo that exists. Unchanged frommain's sniff — noting it, not charging it to this PR. The new git-line matcher widens this slightly to any host that 404s a 401/403.
The two claimed live defects — both hold up
Red 1 — CONFIRMED by execution. Restored repo_manager's pre-PR substring check in a scratch worktree and re-ran the new test against it:
test flows::repo_manager::tests::a_ref_the_remote_has_not_got_is_its_own_answer_whatever_case_git_uses ... FAILED
thread '...' panicked at devlaunch-core/src/flows/repo_manager.rs:2741:9:
assertion `left == right` failed
left: Failed { reason: "fatal: Couldn't find remote ref refs/heads/nosuch" }
right: RefMissingOnRemote
Matches the PR body verbatim. The cause is confirmed at source: remote.c:1785 @ v2.20.0 is a bare die(), so LC_ALL=C provably could not reach it.
Red 2 — CONFIRMED by execution, and the field recording reproduces. Compiled main's reads_as_repository_not_found verbatim and fed it the Codeberg stderr:
test render::red_two_verification::mains_sniff_misses_codebergs_404 ... FAILED
thread '...' panicked at dl/src/render.rs:3084:9:
assertion failed: reads_as_repository_not_found("Cloning into bare repository '/cache/acme/widgets'...\nremote: Not found.\nfatal: repository 'https://codeberg.org/acme/widgets.git/' not found")
And the live 404 the PR recorded reproduces here today, byte for byte:
$ LC_ALL=C git clone --bare https://codeberg.org/blooop-nonexistent-xyz/nope.git
Cloning into bare repository '/tmp/cb404'...
remote: Not found.
fatal: repository 'https://codeberg.org/blooop-nonexistent-xyz/nope.git/' not found
GitHub's two forms also reproduce (remote: Repository not found. over https, ERROR: Repository not found. over ssh), and the ssh 404 carries git's stock "and the repository exists" advice alongside — which is why the near-miss negative at git/tests.rs:410 is load-bearing rather than decorative.
Could not verify
cargo-public-apineeds nightly, which this environment cannot install, so I could not regeneratepublic-api.rest.txtand diff it byte-for-byte against the hand edit. CI'spublic-apijob passing is the authority; the manual consistency check above is the most I could add.- GitLab's and Bitbucket's 404 wordings were not re-recorded live (no account to hand). They are unchanged from
mainand carry no claimed defect.
Verdict
Approve. No blocking findings on either axis. The spec's central invariant — no production decision reads git's English — is met and grepped; the verb scoping is correct and, uniquely, cannot have regressed because each converted site was already fed by exactly one verb; every git-source citation is verbatim at the line given; and both claimed live defects reproduce against unmodified production code.
Four non-blocking items, none of which needs to hold the merge, in the order I would take them:
- CHANGELOG entry — two user-visible behaviour changes shipping with no
[Unreleased]line, against a convention 14 of the last 20 merges follow. The one I would actually do before merging. _ =>atrepo_manager.rs:1508/:1423on the enum this PR exists to make decidable.- Three doc lines (
git.rs:243,:1024,:18) that claim slightly more than the code does. git.rs:22left unrewrapped at 102 columns.
Per this repo's own rule, #399 stays the map owner's to close.
…G entry The review's first non-blocking item, and the one it said it would do before merging. The refactor itself is invisible to users, but the two defects it exposed are not: on git 2.20 and older a "start a new branch" launch failed instead of falling back to the default branch, and a Codeberg or Forgejo 404 lost the wrong-owner hint. Both are behaviour a user can see change, and the convention 14 of the last 20 merges follow is a line under [Unreleased]. Two bullets rather than one, because the two symptoms have nothing in common from where a reader stands -- only the reader they share underneath, which is what this branch moved. [Unreleased] had no Fixed section, so the entry adds the heading. Markdown only -- nothing under rust/ is touched, and cargo fmt --check is clean.
|
Added the missing CHANGELOG entry (3982989), under a new Why it was owed: the refactor is invisible to users, but the two defects it exposed are not. On git 2.20 and older a "start a new branch" launch failed outright instead of falling back to the default branch, because Two bullets rather than one, because the two symptoms have nothing in common from where a reader stands -- only the reader underneath, which is what this branch moved. Collapse them if you would rather have one. Markdown only -- nothing under |
Conflict was CHANGELOG.md alone: main's aid-agent entry (#413) and this branch's two entries both land under [Unreleased]'s second '### Fixed'. Kept both.
Conflict was CHANGELOG.md alone: main's git-refusal entries (#429) and this branch's entries both append under [Unreleased]'s '### Fixed'. Kept both. repo_manager.rs and public-api.rest.txt auto-merged.
1 participant
Builds #399. Map: #406. Do not merge yet, and #399 is the map owner's to close.
What moved
GitRefusedcarried the same fact twice — a typedhow: Failureand git's English inreason— and three production decisions read the English.Failuregains three arms, each read by a named function inclients/git.rsnext to the verb that produces it:BranchAlreadyExistsbranch_already_existscreate_branchflows/branch_manager.rs:252RefMissingOnRemoteref_missing_on_remotefetch_refflows/repo_manager.rs:1507RepositoryNotFoundrepository_not_foundclone_baredl/src/render.rs:1887(+reads_as_repository_not_founddeleted)captured_readingtakes the reader as an argument, so a phrase is scoped to the verb that says it rather than to any git output containing it. That is not decoration:git clonesaysdestination path 'x' already existsabout a directory, and the old flat substring inbranch_managercould not have told the two apart had the refusal ever reached it. Pinned by a test.reasonstays and is still what gets printed; nothing branches on it now.The red, verbatim
Both are real defects the move exposed. Both were run against unmodified production code.
1.
repo_managermissed git's older capitalisation. Up to v2.20.0 git wroteCouldn't find remote ref %sthrough a baredie()rather thandie(_())(remote.c:1785) — neither lowercase nor translatable, so the pinnedLC_ALL=Cnever reached it; lowercase and translated from v2.21.0 (remote.c:1840). On a host still running that git, an ordinary "start a new branch" launch became a failure, because the ref-missing answer read as one and the default-branch fallback never ran.2. The wrong-owner hint missed every host whose 404 body is not GitHub's. Recorded live on 2026-08-24 with git 2.51.1:
None of the three phrases the renderer sniffed appear. git's own
repository '<url>' not found(fromgit-remote-http) is the only line there is, and it is not the substringrepository not found— the renderer's own doc already admitted losing this wording, without noticing that a host, not just a locale, could lose it.The reader now matches git's line as a whole line ending in
' not found, which keeps outrepository '/path' does not existand git's other'%s' not foundmessages (branch '%s' not found,tag '%s' not found) — both pinned as negatives.What was checked before it was encoded
Every phrase was run against real git (2.51.1 in the devcontainer) and against git's source at the tags where the wording changed, rather than trusted from the existing comments:
git branchon an existing branch →fatal: a branch named 'x' already exists(v2.35.0+);A branch named '%s' already exists.up to v2.34.0. The tail of the sentence carries both, so the reader matches the tail.git branch feat/xwherefeatexists →cannot lock ref 'refs/heads/feat/x': 'refs/heads/feat' exists; cannot create .... Saysexists, is not the branch being there, must not be swallowed. Pinned as a negative.git fetchfor a missing ref →couldn't find remote ref refs/heads/x, exit 128.What could not be classified any other way, and why
All four cases genuinely need the message. git exits 128 for a missing ref, a refused key, a DNS failure, a 404 and a branch that is already there alike, so the status carries nothing, and
git branchhas no--porcelain. Askingshow-refafter a failedgit branchwould cost a second spawn on the launch path and answer a different question — whether the branch is there now, which a concurrentdlcould have made true in between. So the change is where the words are read, not whether. Every reader records what was tried instead.Rejected
LC_ALL=Conclone_bareto catch git's own translatedrepository '%s' not found. It would put the whole clone failure in front of a non-English reader in English, to gain a hint. A candidate not offered beats a wrong one offered; the existing choice stands, with the reasoning moved next to the reader.boolaccessor per case (is_repository_not_found()) to keepFailurecrate-private. That is the tag-plus-predicate shape the arms exist to replace.Not done here
git.rsstill holds two English strings of its own —"git is not on PATH"and"git {named} timed out after {N}s"— against the crate's no-English rule. Moving them meansGitRefusedcarries the verb name and the bound instead of a message, and the ~15 sites that launderreasoninto areason: Stringfield all have to carry structure instead. That is a separate ticket's worth of churn, so it is left, per the ticket's own instruction.Public surface
devlaunch_core::clients::gitgainspub enum Failure(7 arms) andpub fn GitRefused::how(&self) -> Failure; thehowfield is now private, so there is one way to read it.public-api.rest.txtis hand-edited —cargo-public-apineeds a nightly toolchain this environment cannot install. CI's regeneration is authoritative;tests/public_api_snapshots.rspasses.Gate
cargo test --workspace --no-fail-fast,cargo clippy --locked --all-targets -- -D warnings,cargo fmt --checkall green. One run hitaid/tests/interactive.rs(the known pty flake, #401); it passes alone and passed on the--no-fail-fastre-run.Summary by Sourcery
Classify Git refusals at the Git client boundary so flows can make reliable decisions without parsing human-readable error messages.
Bug Fixes:
Enhancements:
Tests: