Repository navigation
Pin git's message locale at the last site that classified translated stderr - #211
Conversation
create_local_branch tells "the branch is already there" (swallowed) from a real failure (raised) by looking for "already exists" in git's stderr, and git marks that string for translation. Measured on this host with git's own de catalog: LC_ALL=en_US.UTF-8 LANGUAGE=de turns it into "Schwerwiegend: Branch 'dup' existiert bereits", so the benign arm is missed and an ordinary re-launch of an existing branch raises. Same pin the fetch path already carries, layered on os.environ rather than replacing it so ssh auth and the rest of the environment survive.
The default-branch fetch -- the one that gives a brand-new branch something current to be cut from -- tested only for FetchFailed and let the other outcomes fall off the end. The dispatch immediately around it names all three and rejects an unrecognised one, and the sum type exists precisely so that guarantee holds everywhere its values are read; absorbed silently, a fourth outcome would be indistinguishable from a clean fetch at exactly the moment a new branch is being based on the cache. No behaviour changes for the three outcomes that exist today.
The dispatch the base-branch fix was held up against was itself unexercised -- its rejecting arm was the one uncovered line in the method. Test only; the guard already worked.
Reviewer's GuidePins git’s message locale for branch creation to avoid misclassifying translated stderr, and makes base-branch fetch handling exhaustively name all known outcomes and loudly reject unknown ones, with tests and documentation updated to lock in the guarantees. Sequence diagram for git branch creation with pinned C localesequenceDiagram
participant WorkspaceBranchManager
participant Subprocess
participant Git
WorkspaceBranchManager->>Subprocess: create_local_branch(..., env={**os.environ, LC_ALL=C, LANGUAGE=C})
Subprocess->>Git: git branch <branch>
Git-->>Subprocess: stderr "fatal: branch '<branch>' already exists" (untranslated)
Subprocess-->>WorkspaceBranchManager: CompletedProcess(stderr)
WorkspaceBranchManager->>WorkspaceBranchManager: classify stderr contains "already exists"
WorkspaceBranchManager-->>WorkspaceBranchManager: swallow harmless already_exists case
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@@ Coverage Diff @@
## main #211 +/- ##
==========================================
+ Coverage 92.76% 92.80% +0.03%
==========================================
Files 24 24
Lines 3649 3653 +4
==========================================
+ Hits 3385 3390 +5
+ Misses 264 263 -1
🚀 New features to boost your workflow:
|
The spec review drove the real call path (ensure_branch_exists, both callers guarded by not local_exists under the repo lock) and showed the already-exists arm is reachable only in a race — not on ordinary re-launches as the entry claimed. State the latent hazard, not an everyday failure. Follow-up for the flagged push_branch_to_remote env replacement filed as #212.
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Two-axis review of #211 against ticket #187 at merge-base 9013d20. Preflight: all checks green, non-empty diff (5 files, +110/−1), MERGEABLE.
Standards
Nothing blocking. The change conforms to the repo's documented and de facto conventions.
- Env-pin duplication at n=2 — don't extract the code; do dedupe the rationale (nit).
env={**os.environ, "LC_ALL": "C", "LANGUAGE": "C"}is one line, and Fowler's Duplicated Code doesn't bite at one line across two modules. What is duplicated is the justification: the same ~7-line "LANGUAGE outranks a non-C LC_ALL" comment now appears atbranch_manager.py:79-86andrepo_manager.py:385-390, again in both tests, and again in the CHANGELOG. A named module-level constant (e.g.C_MESSAGE_LOCALE_ENVbesideFetchOutcomeinrepo_manager.py) carrying the reason once would buy the dedupe without inventing a wrapper. Arun_git()helper would be premature; a shared constant is not. - Shotgun Surgery exposure is real but bounded (observation). The invariant "any git call whose stderr is classified textually must pin the message locale" is enforced only by prose. Grep confirms exactly two such sites codebase-wide —
branch_manager.py:92andrepo_manager.py:396— and after this PR both are pinned. A third site is where extraction becomes mandatory. - The
base_outcomedispatch is right, and thepassarms are load-bearing (observation).unhandled_fetch_outcome(outcome: NoReturn)is an exhaustiveness checktyverifies by narrowing toNever; the explicit arms are what make the check fire. Matches the sibling dispatch exactly. - Nit: the two aligned sites still disagree on the None-guard.
repo_manager.py:395usese.stderr or "";branch_manager.py:92is unguarded. Pre-existing, not introduced here. (Folded into follow-up #212.) - Nit:
ensure_branchis now ~90 lines at three nesting levels — but the two dispatches carry different payloads, so leaving it beats forcing symmetry. - Test duplication is the conforming choice — no shared-assertion helper convention exists to hook into. CHANGELOG in the correct section; README correctly untouched.
Spec
Verified independently on this host:
- Live defect reproduced at the classifier.
LC_ALL=en_US.UTF-8 LANGUAGE=de git branch dup→Schwerwiegend: Branch 'dup' existiert bereits;LC_ALL=C.UTF-8 LANGUAGE=destays English. Both the translation claim and the "one glibc rule" justification for pinningLANGUAGEtoo are true, not assumed. - Tests: all three pass; the two touched suites total 92 passed. Mutation spot-check: replacing (rather than extending) os.environ turns the test red on
KeyError: 'SSH_AUTH_SOCK'— the assertion is load-bearing. - No behaviour change for the three existing outcomes;
FetchFailedwarning byte-identical. - "Last site" claim holds: only
branch_manager.py:92andrepo_manager.py:395classify git stderr textually; both now pinned.
Findings:
- BLOCKING (doc accuracy only). The CHANGELOG asserted "every launch of an existing branch raised
Failed to create branch". Not true:create_local_branchis reached only fromensure_branch_exists, both call sites guarded bynot local_existsunder the RepoLock — so the already-exists arm is reachable only in a race. Driving the realensure_branch_existsagainst a German-locale git with the pin stripped produced no raise. The fix and the ticket's diagnosis are right; the release note overstated realised user impact. - Non-blocking.
push_branch_to_remote's env replacement is correctly out of scope per the ticket, but the promised follow-up ticket had not been filed.
Deviations judged: extra requested-ref test — sound; both halves in one PR — sound (ticket: "Fix both together"); CHANGELOG yes / README no — sound; push hazard flagged not fixed — sound in scope.
Verdict
Request changes → resolved during review. The single blocking finding was documentation accuracy; the orchestrator reworded the CHANGELOG entry to the verified reachability (commit c18891d, "latent misclassification at the seam, not an everyday launch failure") and filed the promised follow-up as #212 (which also carries the None-guard alignment nit). With those in, no findings block. GitHub refuses author-account review states, so this is posted as a comment; this written verdict governs. Merging with a merge commit once checks re-green on c18891d.
Closes #187.
Both halves the ticket names, in one PR — they came out of one review pass, neither is large, and the ticket allowed either shape.
1. The translated-stderr hazard, one file over
Creating a branch in the bare cache tells the harmless "it is already there" (swallowed, and the launch carries on) from a real failure (raised) by looking for
already existsin git's stderr. git marks that string for translation, and unlike the fetch path it had no locale pin.Measured, not assumed — against git's own German catalog, on this host:
No
already existsin it, so the benign arm is missed. Driving the real method against a real git in that locale, with the fix reverted:and with the fix in place,
RESULT: swallowed (correct). So this was a live defect on a non-English host, not a theoretical one: every launch of a branch already in the cache failed — the ordinary re-launch, the common case.The fix is the one the fetch path already carries: hand git an explicit env pinning
LC_ALL=CandLANGUAGE=C, layered on the inherited environment rather than replacing it so ssh auth and the rest survive. Both are pinned rather than justLC_ALL, so the guarantee does not rest on the single glibc rule that letsLC_ALL=CoutrankLANGUAGE— the run above confirms that rule is real (C.UTF-8+LANGUAGE=destays English), and also confirms it is the only thing that would have saved a one-variable pin.This was the last site classifying translated git output without the pin.
2. The base-branch fetch's outcome, every arm named
When the requested branch does not exist upstream, a second fetch gets the default branch so the new branch is cut from something current. That fetch's outcome was tested only for the failure arm; the other two fell off the end. The dispatch immediately around it names all three and rejects an unrecognised one, and the sum type exists precisely so that guarantee holds wherever its values are read — absorbed silently, a fourth outcome would be indistinguishable from a clean fetch at exactly the moment a brand-new branch is being based on the cache.
Every arm is now named, with an explicit rejection for anything else. No behaviour changes for the three outcomes that exist today — the two newly-named arms both do what falling through did.
Tests
Three, each mutation-checked (fix reverted → test goes red):
test_already_exists_answer_survives_a_translated_gitLC_ALL=CandLANGUAGE=C, and the surrounding environment (SSH_AUTH_SOCK) survivesos.environinstead of extending it; red again whenLANGUAGEis left unpinnedtest_an_unrecognised_base_fetch_outcome_is_rejected_not_absorbedRefMissingOnRemotearm turns an existing test red, so the named arms are load-bearing rather than decorativetest_an_unrecognised_requested_ref_outcome_is_rejected_not_absorbedThe first mirrors the locale test #185 added, at the seam the ticket named. The third is a declared small extension: the dispatch the fix was held up against was itself unexercised — its rejecting arm was the one uncovered line in the method — so pinning only the new one would have left the pair asymmetric. Test only; that guard already worked.
Not fixed here
Pushing a branch to the remote builds its env as a bare
{"GIT_SSH_COMMAND": ...}, replacing the inherited environment rather than extending it — so when an ssh key is passed, git runs with noPATH, noHOMEand noSSH_AUTH_SOCK. Same shape of mistake as the one fixed above and noticed while fixing it, but a different failure and outside this ticket's scope; left for a ticket of its own rather than smuggled in.Checks
pixi run ciexits 0 — pylint 10.00/10, 1659 passed, 23 deselected. CHANGELOG updated under## [Unreleased]; no README change, as no CLI surface moves.🤖 Generated with Claude Code
Summary by Sourcery
Pin git branch creation to the C locale and tighten handling of base-branch fetch outcomes when ensuring workspace branches.
Bug Fixes:
Enhancements:
Documentation:
Tests: