✨ feat(naming): derive the branch parser from worktree.branch_pattern - #476
Merged
Conversation
`branch_pattern` decided how a branch name was written; a hardcoded
`^([a-z]+)/#(\d+)-([a-z0-9-]+)$` decided how one was read. A repo that set
`branch_pattern = "{type}-{issue}-{desc}"` therefore created `feat-41-foo` and
then failed to recognise the branch it had just created: no issue auto-linking,
no gitmoji, `gwm commit-prefix` erroring, empty hook placeholders on the
remove / bootstrap paths, a rename modal that refused to open, and a `doctor`
orphan check that skipped every branch as user-managed.
`BranchParser` compiles the pattern into the regex that reads it back, mirroring
the exact `expand_placeholders` call the formatter makes. `BRANCH_RE` is gone,
and with it `parse_branch`, which had no production caller left.
Refused rather than compiled into a parser that reads back the wrong thing:
- two placeholders with no literal between them. `{issue}{desc}` writes
`42123-x` from `42` + `123-x` and reads it back as `4212` + `3-x`;
`{desc}{issue}` is ambiguous outright, since `a12` is what both `a` + `12`
and `a1` + `2` produce.
- the same placeholder twice, which the formatter expands to the same value in
both positions and no backreference-free engine can split.
Adjacency is the whole rule. The issue body proposed "the separator must not
belong to the charset of the token on its left", which would reject
`{desc}-{issue}`; measured against the engine that ships, `user-auth-42` reads
back as `user-auth` + `42`, because an issue number cannot contain the `-` and
greedy backtracking therefore has exactly one split to find. Rejecting it would
have refused a pattern that works.
Two deliberate narrowings, both reported rather than silent:
- `{type}` compiles to an alternation of the configured branch types, so a
branch carrying a type the repo does not declare is no longer claimed as
gwm's. The TUI rename already refused those one step later; now the parser
and the modal agree, and the refusal message names the pattern and both
possible reasons instead of asserting "free-form" about a branch that plainly
carries a type.
- a literal in the type's position is text, not a type, so
`feat/#{issue}-{desc}` yields no branch type even when `feat` is the only
configured one. Inferring intent from literal text is guesswork on any repo
where a type name is also an ordinary word.
`branch_pattern_warning` keeps both call sites and reports the residue: a
pattern the compiler refuses, and a segment the pattern never writes. The
second is a new message shape, because "N of the shapes probed read back the
wrong type" both over-quantified a permanent absence and hid the one-line fix.
The probe survives as the backstop for what the compiler does not mirror, which
is measured rather than assumed: a `~`-leading pattern is the case, since the
formatter ends with `shellexpand::tilde` and the reader cannot undo it.
`gwm commit-prefix` gained the guard that goes with all this: a pattern that
carries no `{type}` or `{issue}` parses perfectly and would have printed
` (#):` into a commit message.
Tests: the compiler per token, the plausible-convention table round-tripping,
both ambiguity refusals, the type narrowing, and a guard that reads the token
list out of `expand_placeholders` itself so a placeholder added later cannot
silently fall through as a literal.
refs #417
The `branch_pattern` section (EN + FR) promised the opposite of what now
happens: it said the reader was a fixed regex, listed `{type}-{issue}-{desc}`,
`{type}/{issue}-{desc}` and `wt/{type}/#{issue}-{desc}` under "parses back to
nothing at all", and told readers to keep the `<type>/#<issue>-<desc>` skeleton.
All of those round-trip now.
The table is rewritten from the real check rather than reasoned about, and the
test that pins it was rewritten with it, so the two cannot drift. It gains the
adjacency rule (with the note that a separator drawn from the left
placeholder's own charset is fine, so `{desc}-{issue}` works), the patterns
refused at compile time, the ones that compile but drop a segment, and the
single shape the compiler does not mirror.
The CHANGELOG's #415 entry closed on "deriving the parser from the pattern is
tracked by #417". Both ship in the same release, so it points at the entry
below it instead of at an open issue.
closes #417
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The guard that reads the placeholder list out of `expand_placeholders` anchored on `"\n}\n"` to find the end of the function. The Windows runner checks the source out with CRLF, so the anchor never matched and the test failed on the read (`the function has a body`) rather than on the compiler it exists to check. Green on macOS and Linux, red on Windows. Verified by running the same extraction against a CRLF copy of `src/config.rs`: seven tokens found either way, none without the normalisation. refs #417
Deriving the parser dropped two things the previous release read fine, which
was a regression dressed up as a documented trade-off.
`{type}` compiled into an alternation of the configured branch types. A branch
created while `wip` was in `.gwm.toml` stopped being recognised as gwm's the
moment `wip` was removed: `doctor`'s orphan check skipped it, `gwm
commit-prefix` errored on it, and the TUI rename lost its precise "type is not
configured" message in favour of a generic one. It is `[a-z]+` again. Nothing
needed the narrowing: with adjacent placeholders refused, `[a-z]+` splits every
pattern in the documented table, and the one consumer that genuinely requires a
*configured* type checks the resolved list itself.
A pattern that hardcodes a segment lost it entirely. `feat/#{issue}-{desc}`,
`{type}/#1-{desc}` and `{type}/#{issue}-fixed` were all readable in 1.5.0,
because its `^([a-z]+)/#(\d+)-([a-z0-9-]+)$` happened to have a group exactly
where the literal sits. The parser now recovers those literals as constants, so
gitmoji, auto-linking and `gwm commit-prefix` keep working on those repos.
The recovery is an exact match, not an inference. A branch type is looked up in
the repo's configured list, so `feature/{issue}-{desc}` recovers nothing:
`feature` names a namespace, not a type. An issue number has to be all digits.
A description is whatever `DESC_RE` accepts, and runs last on what the stricter
two left, which is what keeps `feat/#1-fixed` from reading its description as
`1-fixed`. A segment is only claimed when exactly one candidate survives.
`{repo}` and `{home}` are deliberately not sources: the user wrote `feat` in
`feat/#{issue}-{desc}` and meant it, but nobody chose the repo's name for this,
and a repo called `docs` must not type its own branches.
`desc` is the loose oracle, and it self-reports rather than hiding: in
`wt/{type}/#{issue}` the namespace `wt` is the one `DESC_RE`-shaped token, so it
is claimed, and the round-trip probe immediately says the description does not
match what `gwm create` was given. Every constant that costs something surfaces
that way, which is why no separate disclosure channel was needed.
What a frozen segment costs is reported exactly as #415 reported it, unchanged:
`gwm create fix 42 x` under `feat/#{issue}-{desc}` writes a `feat/` branch, so
the type asked for is not the type read back. `gwm doctor` warns. With `feat`
as the only configured type there is no loss and it stays quiet, which is the
case #415's own review pinned.
The acceptance criterion is a parity table computed from the `v1.5.0` tag's
regex rather than reasoned about: the four patterns it read correctly are
asserted to read the same way, and the two it read *wrongly* are asserted to
read correctly now, so matching 1.5.0 there would mean keeping the bug.
refs #417
**A separator both neighbours could swallow is refused** (`src/naming.rs`).
Refusing adjacent placeholders was necessary but not sufficient: a non-empty
literal is not automatically a safe boundary. `{type}-{issue}9{desc}` writes
`feat-42919x` from issue `42` and desc `19x`, and the greedy `\d+` slides right
across the `9` to read issue `4291` and desc `x`. Measured, and worse than the
mis-split alone: because it is deterministic, every probe agreed with it, so
`gwm doctor` reported the pattern as perfectly valid while auto-linking pointed
at the wrong issue. That is exactly the silent failure this issue exists to
remove, reintroduced one layer down.
The condition is two-sided, which is what keeps it narrower than the rule
#417's body proposed: the separator's first character has to be swallowable by
the placeholder on its left AND producible by the one on its right, so the
displaced text can hand back a replacement separator. `-` after `{desc}`
satisfies the first half and never the second, since `\d+` cannot contain it,
so `{desc}-{issue}` keeps its single valid split and stays legal. Every pattern
in the documented table still compiles.
**`gwm commit-prefix` no longer echoes the pattern raw** (`src/cli.rs`). Both
errors #417 added quote `worktree.branch_pattern`, which is repo-supplied, and
this command does not go through the TOFU trust gate. Quoting it raw handed an
unvetted `.gwm.toml` the same terminal escape channel `branch_pattern_warning`
already neutralises. `sanitise_for_terminal` becomes `pub(crate)` rather than
growing a second copy, since every site that quotes a config value needs it.
**The rename modal refuses a pattern that leaves a segment empty**
(`src/tui/app.rs`). Before #417 a branch written by `{type}/{desc}` did not
parse, so the modal declined to open. Deriving the parser made it parse with
`issue == ""`, which opened a form the user cannot submit: the submit path runs
`BranchSpec::new_with_types`, which rejects an empty issue, and typing one in
would not help either since the pattern has nowhere to write it. It now refuses
up front and names the missing placeholder, the same shape as `commit-prefix`.
A pattern that *freezes* a segment is unaffected: the constant fills it, so
`feat/#{issue}-{desc}` still opens and still renames.
refs #417
…ample Codex review, second pass on PR #476. Three of the four findings land on the same rule, which is the signal to stop deriving it one example at a time and enumerate the space instead. The rule now asks the only question that matters — can the boundary between two placeholders land in more than one place — and answers it from the charsets: - adjacency is refused only when the alphabets overlap, so `{type}{issue}` compiles (`[a-z]+` stops at the first digit, `\d+` at the first letter) instead of falling back to the inert parser and taking auto-linking, `commit-prefix`, the hooks and the TUI rename away from a config that round-trips; - a separator is examined whole rather than by its first character, so `{type}-{issue}9-{desc}` compiles: shifting the split needs the left side to eat a *repeating* prefix of the separator and the right side to hand the rest back, and `\d+` can never supply the `-`; - literals either side of a placeholder no longer fuse in the text the constant recovery reads, so `1{type}2-{desc}` stops freezing issue `12` — a number no branch it writes ever contains. `{repo}` / `{home}` break the run too. `gwm pr` keeps its `chore` fallback: since #417 a pattern with no `{type}` parses and reports an empty type rather than failing, which the `.map` treated as a real one. The rule itself is now pinned by `the_ambiguity_rule_accepts_exactly_the_patterns_that_round_trip`, which generates every pattern over the three placeholders and eight separators, decides round-tripping with a regex the test builds itself, and requires the compiler to accept exactly those. It fails on a silent mis-split and on an over-strict refusal alike — both classes of finding this pass reported. It caught two more gaps while being written. refs #417
Codex review, third pass on PR #476. Knowing the same set of tokens as `expand_placeholders` is not enough — the compiler has to find them the same way. The formatter is a chain of `str::replace`, so it sees `{type}` at offset 1 of `{{type}` and writes `{feat`. The compiler scanned for `{` instead, took `{{type}` for one unknown token, and compiled a regex demanding that text back verbatim, so no branch such a pattern wrote was ever read back: auto-linking, the hooks, `[pr_template.by_type]` and the TUI rename all went quiet on a pattern that formats perfectly well. The scan now searches for each known token and takes whichever occurs earliest, which is what `str::replace` does. Everything the formatter leaves alone — an unknown `{foo}`, an unbalanced brace — stays literal for the same reason rather than by a separate rule. refs #417
…self-substituting expansion Codex review, fourth pass on PR #476. Two independent surfaces. The rename form let a *frozen* segment be edited. `{type}-1-{desc}` hardcodes the issue number, and #417 recovers it, so the form opens with `1` in the issue field — but `branch_name` has no `{issue}` to write a new value into, while `path_pattern` still has one. Changing it renamed nothing and moved the worktree directory instead. `submit_edit_worktree` now refuses, naming the placeholder and the value it is pinned to. The refusal is scoped to a frozen segment the user actually changed, not to the pattern: `feat/#{issue}-{desc}` freezes the type and its rename worked in 1.5.0, so editing the issue or the description under it has to keep working — there is a test for each side. `expand_placeholders` substitutes in a fixed order and each `str::replace` runs over what the previous ones produced, so a `{repo}` / `{home}` expansion that itself contains a later token is substituted again: a repo directory named `{type}` makes `{repo}/#{issue}-{desc}` write `feat/#42-x` while the compiler demanded `{type}/#42-x` back, and every read-back feature was off with nothing saying why. Replaying the whole chain for an input nobody has is not worth it, so the pattern is refused and the message names both the token and what its expansion carries. refs #417
Closed
3 tasks
…ing it mirrors it Codex review, fifth pass on PR #476. Third finding in three passes on the same claim — that `compile` tokenises the way `expand_placeholders` substitutes — each time on a shape the previous fix did not cover: a brace before a placeholder, an expansion carrying another token, and now a token formed across an expansion boundary (`{{repo}}` in a repo called `type` writes `feat/#42-x`, since the formatter substitutes what its own expansions produce). Patching the third instance would have invited a fourth. The claim is now verified instead: `compile` writes one probe branch through the real formatter and refuses the pattern when the parser it just built cannot read that back. The special-case scan of `{repo}` / `{home}` expansions added last pass is gone — this subsumes it, and covers whatever else drifts. Two deliberate exclusions, both stated in the code: a `~` prefix, whose divergence is real but known, documented, and reported by `branch_pattern_warning`'s probe with every affected feature named; and a pattern the formatter itself cannot expand, which fails at `gwm create` with its own error. The rename form's other half — a frozen segment the user does *not* edit, whose value the worktree path still carries — is #478. It predates this PR (verified against the v1.5.0 tag: identical `submit_edit_worktree`, and `BRANCH_RE` reads `feat/#42-x` as type `feat`), and fixing it means deriving a second parser from `path_pattern` and deciding what a rename means when the two patterns disagree. refs #417
Owner
Author
Review pass 5: one finding fixed, one deferred to #478Fixed ( Patching the third instance would have invited a fourth, so the claim is verified rather than argued: Deferred to #478. The other rename finding, a segment frozen in It predates this PR, verified against the tag rather than assumed:
What #417 changes is reach, not behaviour. Fixing it properly means deriving a second parser from This PR does close the sharper half: a frozen segment the user edits is refused outright (pass 4), because changing it renamed nothing and moved the directory instead. |
Codex review, sixth pass on PR #476. Two findings on constant recovery, plus a message that contradicted itself. The recovery asked each oracle for a *globally unique* candidate, which is not what the previous release did. `feat/#{issue}-fix` freezes a type and a description that are both configured branch types, so neither was unique and both were dropped — 1.5.0 read the pair correctly. And a description's charset spans the `-` its first character cannot be, so `-fixed--desc` and `-fixed-` are single values that a dash-joined tokeniser split and truncated; 1.5.0 read both. Recovery is now positional first: a literal is only a candidate for a segment if it sits where that segment goes — before `{issue}` for a type, after it for a description — and the marker each placeholder leaves in the literal text is what says where that is. Then the oracles run, and a segment is recovered only when one reading of the whole pattern survives, so `feat/fix-{issue}-{desc}` still recovers neither of the two types it names while `feat/#1-fix`, where reading `fix` as the type leaves no digits for the issue, recovers all three. The obligation is now enumerated instead of sampled, which is what the hand-picked baseline missed twice: 1.5.0 read a branch iff it matched one hardcoded regex, so a test runs *that regex* over every pattern in the family it accepts — 16 of them — and requires the same triple back. Both findings were inside that family. Two consequences, both intended. `wt/{type}/#{issue}` no longer reads `wt` as the description of every branch: it sits before the type, where no segment goes. And `1{type}2-{desc}` reads its `2` as the issue number, from the position an issue number occupies, exactly as `{type}/#1-{desc}` reads its `1`; the pass-2 finding was about `12`, a value assembled across a placeholder and present in no branch the pattern writes, and that stays impossible. `gwm commit-prefix` said to add "`{type}` or `{issue}`" when the pattern carried neither, but a prefix needs both, so following the advice left the command failing. refs #417
…ised spec Codex review, seventh pass on PR #476 — and a direct consequence of the previous one. A frozen description does not have to be canonical: `DESC_RE` accepts `fixed-`, and since the recovery started reading a literal whole rather than trimming its trailing dash, `{type}/#{issue}-fixed-` freezes exactly that. But the guard compared the constant against the `BranchSpec`, whose description has been through `kebab` — which strips the dash. So every submit looked like a change to a frozen segment and the rename form could not be submitted at all, even when the user only touched the issue number. The guard is about what the user typed, so it now compares what the user typed. Moved above `BranchSpec::new_with_types` to make that structural rather than a matter of remembering. refs #417
…ne with Codex review, eighth pass on PR #476. In a workspace, two repos whose directories share a basename are disambiguated for display — the second becomes `api-2` (#304) — and `App::repo_name` holds that name. Every formatter call in the rename flow already expands `{repo}` with it, but the parser #417 added was compiled from the workdir basename instead. Under a `{repo}` pattern the two disagreed, so the form refused to open on a worktree the repo owns, reporting a branch it had written itself as free-form. Which name is right for `{repo}` is a separate and older question — `spec.branch_name(..., &self.repo_name)` predates this PR. What matters here is that one name is used, since parser and formatter agreeing is the whole of #417. refs #417
Owner
Author
Review pass 8: one fixed, three dismissed with evidenceThis is the review loop's iteration cap, so it is the last fixing round on this PR. Fixed ( Dismissed: removing Dismissed: the frozen-segment / Dismissed here: the |
…irectory
The rename form rebuilt the triple from the branch alone, so a segment
`branch_pattern` freezes was read as the frozen literal even when the
worktree's own directory still carried the value `gwm create` was given.
`branch_pattern` and `path_pattern` need not carry the same segments, and
when they do not, neither name holds the whole triple. Under
`branch_pattern = "feat/#{issue}-{desc}"` with the default
`path_pattern`, `gwm create fix 42 x` writes the branch `feat/#42-x` and
the directory `fix-42-x`: the branch has no `{type}` to put `fix` into,
so the directory is the only place that value still exists. Renaming the
description therefore moved the directory to `feat-42-…` and dropped the
`fix`, silently and without being asked.
`naming::worktree_spec` now reads both. The branch wins for every
segment it writes from a placeholder — it is the worktree's identity, and
a directory renamed by hand or created under an older `path_pattern` must
not rewrite it. The directory is consulted only for the rest. A missing
directory name, one `path_pattern` does not match, and a `path_pattern`
that will not compile all leave the branch's reading untouched, since a
broken path pattern has its own diagnostic and must not take the rename
away.
Reading a segment from the directory does not make it editable: the
submit guard still refuses to change what `branch_pattern` cannot write,
now comparing against the value the form was *opened with* rather than
against the pattern's literal, since the two now differ.
Also drops a paragraph this branch had duplicated in the English
`gwm-toml.md`.
closes #478
The family was enumerated with `DESC_RE`-valid literals, but 1.5.0's
description group was `[a-z0-9-]+` — looser, and it accepts a value
opening with `-`. So `{type}/#{issue}---fix` was outside the family this
test walked while being squarely inside the one the old regex read, and
the test claimed more parity than it had.
Widened to 1.5.0's charset: 24 patterns instead of 16. The extra ones
diverge, and the divergence is now asserted rather than absent. It is a
bug 1.5.0 had rather than a contract it kept: `--fix` fails `DESC_RE`, so
`BranchSpec::validate` rejects the very description that parser handed
back — the rename form could not submit it and `gwm create` could never
have produced it. A leading `-` is also what #416 banned from a name,
since `gwm remove` and `git branch -d` read it as a flag. The leading
dashes are dropped; the description is not.
refs #417
6 tasks
House style for anything published under the maintainer name: no em dashes in docs, issues or PR comments. These five lines were added by this branch, so they are the ones to fix; the surrounding text predates it and is left alone. refs #417
The first pass counted lines rather than occurrences, so it missed two in the config docs and left the CHANGELOG untouched. Both are published under the maintainer name, so the same house style applies. Only lines this branch adds are rewritten; the surrounding text predates it and is left alone. refs #417
Found validating the branch by hand, which is exactly what review could not see. The create form and the rename modal both built their `Branch :` and `Dir :` rows from a hardcoded `<type>/#<issue>-<desc>` and `<type>-<issue>-<desc>`, ignoring `branch_pattern` / `path_pattern` entirely. That is the same defect #417 exists to remove, one layer up: the pattern was honoured when writing a name and nowhere else. The rename case was the loud one. With `feat/#{issue}-{desc}`, moving the type selector to `docs` previewed the branch `docs/#42-login` while submitting would have written `feat/#42-login`, since the pattern has no `{type}` to write into. The modal promised a rename the repo cannot perform. Both now expand the real patterns, through the form's fields as they stand: no validation, no failure, an empty issue simply expands to nothing. The refusal message loses the value it quoted. `LoaderWidget` renders one unwrapped line, so a message sized by user input clips at an arbitrary point, and the first version was cut at "has no {type} to write,", leaving half a reason. It is now a fixed 36 characters whatever the branch holds, and the value it would have quoted is on screen in the modal's `From :` row. A render test asserts the whole sentence reaches the buffer, so the clip cannot come back. refs #417
`literal_constants` decided ambiguity per *reading* instead of per
segment, so one ambiguous segment took the unanimous ones down with it
and `commit-prefix`, the templates and the TUI saw a branch with no type
where the pattern states one. Two shapes hit it:
- `feat/feat/#{issue}-{desc}`, whose two readings both say `feat`, which
is one answer and not a coin toss;
- `feat/#{issue}-fix/done`, whose two readings disagree about the
description while agreeing about the type.
State the rule where it belongs: a segment is recovered iff every
maximal reading names it with the same value. Genuine disagreement, as
in `feat/fix/#{issue}-{desc}`, is still refused.
refs #417
The entry described the positional half of literal recovery and stopped short of what decides the rest, which the fix just before this commit moved from per reading to per segment. refs #417
The rename form refused to edit any segment `branch_pattern` does not
write, on the grounds that the submit would move the directory and leave
the branch alone. Under a config that puts the type in the path on
purpose, that reading is backwards: `feat/#{issue}-{desc}` will say
`feat` whatever the form holds, so the directory is the only place this
worktree's type exists, and a worktree created as `fix` could never
become `docs`.
What made the refusal look right was a preview that lied, showing
`docs/#42-…` for a submit that writes `feat/#42-…`. With the preview
expanding the real patterns, the modal states plainly that the branch is
unchanged and the directory moves, so there is nothing left to protect
the user from. `rename_worktree` already handles that shape as a
path-only edit and skips every ref mutation, local and remote alike.
Ask `path_pattern` too, and refuse only when neither pattern writes the
segment. There the submit would rebuild the same branch and the same
directory, so saying no still beats a silent no-op.
refs #417
The rename guard asked the parser whether a segment could be read back,
when the question is the formatter's: will a new value be written
anywhere? `BranchSpec::worktree_path` expands `[worktree].base` with the
triple as well, so a `base` of `.../{type}` sorts worktrees into per-type
directories and changing the type moves the worktree between them. That
is a real rename, and the guard refused it because it only looked at
`branch_pattern` and `path_pattern`.
Ask the question the formatter answers: `expand_placeholders` writes a
token wherever it appears, so the test is whether any of the three
patterns it expands carries that token. Refuse only when none does, and
the submit really would rebuild the same branch at the same path.
refs #417
This was referenced Jul 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
worktree.branch_patterndecided how gwm writes a branch name; a hardcoded^([a-z]+)/#(\d+)-([a-z0-9-]+)$decided how it reads one. A repo that setbranch_pattern = "{type}-{issue}-{desc}"createdfeat-41-fooand then failed to recognise the branch it had just created.BranchParsercompiles the pattern into the regex that reads it back.BRANCH_REis gone, and with itparse_branch, which had no production caller left.Closes #417
Type of change
Changes
naming::BranchParsercompilesbranch_patterninto a parser, mirroring the exactexpand_placeholderscallBranchSpec::branch_namemakes.{type}/{issue}/{desc}become named groups,{repo}/{home}become escaped literals, everything else stays literal (which is correct for{repo_path}/{repo_parent}, since the branch path passesNoneforrepo_path).github::read_link(auto-linking),worktree::list(one compile per listing, not per branch),doctor(orphan check),lifecycle::HookContext::for_worktree(remove / bootstrap placeholders),cmd_pr([pr_template.by_type]+ body placeholders), the TUI rename, andgwm commit-prefix.branch_pattern_warningkeeps both call sites and reports the residue: a pattern the compiler refuses, and a segment the pattern never writes.Two corrections to the issue as filed
1.
{desc}-{issue}is not ambiguous. The issue proposed rejecting any pattern whose separator belongs to the left placeholder's charset, citinguser-auth-42splitting asuser-auth-4+2. Measured against the engine that ships, it does not: greedy repetition backtracks, the literal-has to match after the group, and since{issue}is\d+it cannot contain the separator, so exactly one split is valid. Implementing the rule as written would have refused a pattern that works. Full measurement in the correction comment.The rule that ships is narrower: adjacency. Two placeholders with no literal between them are refused (
{issue}{desc}reads42123-xback as4212+3-x;{desc}{issue}is ambiguous outright, sincea12is what botha+12anda1+2produce), plus the same placeholder twice.2. The probe is not replaced by the compiler. Adjacency-rejection is necessary, not sufficient, so
branch_pattern_warningstill probes the derived parser as a backstop. That earns its keep, measured rather than assumed: a~-leading pattern does not round-trip, because the formatter ends withshellexpand::tildeand the reader has no way to undo it.No regression: the 1.5.0 parity table is the acceptance criterion
The set of branches 1.5.0 could read is exactly the set matching its
^([a-z]+)/#(\d+)-([a-z0-9-]+)$, read verbatim off thev1.5.0tag. Computed rather than reasoned about, that is six patterns:{type}/#{issue}-{desc}feat / 42 / my-descfeat/#{issue}-{desc}feat / 42 / my-desc{type}/#1-{desc}feat / 1 / my-desc{type}/#{issue}-fixedfeat / 42 / fixed{type}/#{issue}-prefix-{desc}feat / 42 / prefix-my-descfeat / 42 / my-desc(1.5.0 was wrong, #415 said so){type}/#{issue}-{desc}-{repo}feat / 42 / my-desc-gwm-clifeat / 42 / my-desc(same)every_branch_1_5_0_could_read_is_still_read_the_same_waypins the first four;the_two_patterns_1_5_0_read_wrongly_are_read_correctly_nowpins that matching 1.5.0 on the last two would mean keeping the bug.Frozen segments are recovered, not dropped
feat/#{issue}-{desc}hardcodes the type; 1.5.0 read it back because its regex happened to have a group where the literal sits. The parser recovers such literals as constants, so gitmoji, auto-linking andgwm commit-prefixkeep working.The recovery is an exact match, not an inference:
feature/{issue}-{desc}recovers nothing, becausefeaturenames a namespace and is not in the list.DESC_REaccepts, run last on what the stricter two left. That is what keepsfeat/#1-fixedfrom reading its description as1-fixed.A segment is claimed only when exactly one candidate survives.
{repo}and{home}are deliberately not sources: the user wrotefeatin the pattern and meant it, but nobody chose the repo's name for this, and a repo calleddocsmust not type its own branches.descis the loose oracle and it self-reports rather than hiding. Inwt/{type}/#{issue}the namespacewtis the oneDESC_RE-shaped token, so it is claimed, and the round-trip probe immediately says the description does not match whatgwm createwas given. Every constant that costs something surfaces that way, so no separate disclosure channel was needed.What a frozen segment costs is reported exactly as #415 reported it:
gwm create fix 42 xunderfeat/#{issue}-{desc}writes afeat/branch, so the type asked for is not the one read back, andgwm doctorwarns. Withfeatas the only configured type there is no loss and it stays quiet, which is the case #415's own review pinned.{type}stays[a-z]+The issue proposed compiling
{type}into an alternation of the configured branch types. That would stop recognising a branch created before a type was retired from.gwm.toml, takingdoctor's orphan check andgwm commit-prefixaway from a name the previous release read fine, and collapsing the TUI rename's precise "type is not configured" message into a generic one.Nothing needs it: once adjacent placeholders are refused,
[a-z]+splits every pattern in the documented table, and the one consumer that genuinely requires a configured type, the TUI rename, checks the resolved list itself.gwm commit-prefixguardA pattern that carries no
{type}or{issue}and freezes neither ({type}/{desc},{issue}-{desc}) parses perfectly and yields an empty segment, soresolve_prefixwould have printed(#):straight into a commit message. That is now an error naming the placeholder to add, distinct from the #416 free-form message. Verified end to end against three real repos:feat/#{issue}-{desc}renders:sparkles: feat(#42):,{type}/#1-{desc}renders:bug: fix(#1):, and{type}/{desc}errors.Tests
cargo testpasses locally (underGWM_NO_GLOBAL_CONFIG=1and a CI-like minimalPATH)cargo fmt --checkpassescargo clippy --all-targets -- -D warningspassestests/Driven from a CLI-level red test (
commit_prefix_reads_a_branch_written_with_a_custom_pattern) rather than from the compiler outward, because the acceptance criterion is "the features keep working", not "the regex is right".Added: per-token compilation, the plausible-convention table round-tripping, both ambiguity refusals, the type narrowing, and a guard that reads the token list out of
expand_placeholdersitself so a placeholder added later cannot silently fall through as a literal and reintroduce the divergence this PR deletes.Rewritten: the #415 block. Ten tests asserted that specific patterns were broken; they are not any more. Each keeps its original reasoning as history and asserts the new truth. Two needed a surviving vehicle rather than an inversion:
control_characters_in_the_pattern_never_reach_the_terminalis a security test (P1 from [Fix]: warn when branch_pattern is customised (parser ignores it) #415's review) and must not weaken to "no longer warns". [Feature]: derive the branch parser from branch_pattern (drop BRANCH_RE) #417 added a third echoing path and moved the other two, so all three are exercised now: the compile error, the missing-placeholder verdict, and the probe'se.g.example.the_consumer_mapping_matches_the_call_sitespins "PR/MR detection is unaffected" and "remove/bootstrap, not lifecycle". Re-anchored on the pattern that still reaches that branch.Notes for reviewers
descoracle is the loose one.wt/{type}/#{issue}readswtas the description of every branch it writes. It is reported by the probe rather than silent, and the fix is to write{desc}, but it is the one place recovery guesses.worktree::listnow reads.gwm.tomlonce per call (best-effort,unwrap_or_default) to compile the parser.read_link's public signature is unchanged and derives per call;read_link_withis the hoisted form for loops.branch_patternwill have existing branches created under the old one. Those stop parsing, which is correct but worth stating:doctorreclassifies them as user-managed, andcommit-prefixerrors on them.Linked issues / docs