From 6212c311172b0d5311151b5d19bf885350963fa1 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Thu, 15 Oct 2026 22:46:50 +0000 Subject: [PATCH 1/3] family.py and validate-family.py: where an earlier act left the branch, the `git switch -c family/-` remedy says the same command without `-c` switches to it, so `already exists` is no dead end, and the docs say `family/add-` (#607) L2-46. `family.py add`, `bump` and `remove` refuse the holder's tracking branch with `git -C switch -c family/-` (#487), and the holder's `validate-family.py` names the same line before each `add` and `remove` it prints (#539, landed as 54041cc3). Where an earlier act left that branch the line fails, `fatal: a branch named 'family/add-atlas' already exists`, exit 128, and neither tool said what to do. Both now say it in one sentence, the same words in both (`BRANCH_LEFT`): "If `git switch -c` says that branch already exists, an earlier act left it: the same command without `-c` switches to it and keeps every commit on it, or give the branch another name." `family.py` prints it under the `switch -c` of its refusal and of the dry run's `branch` line; the validator ends the prose of `member-gitlink-absent`, `member-duplicate-id` and `member-gitlink-unnamed` with it. No printed command resets or deletes the branch: `git switch` keeps its commits and refuses to overwrite a changed file. In words, not as a second command or a one-line `||`: `family.py`'s lines are quoted for PowerShell on Windows, where 5.1 has no `||`, and README lists the kinds of `family.py` line quoted that way; the validator's report is read in CI, where the checkout holds none of the person's branches, so neither tool can print the one line that works by asking first. L2-45. AGENTS.md, the holder's README and AGENTS-shape.md, and `init`'s NEXT STEPS spelled the branch `family/add-` beside `--member /`. All now say ``, the one placeholder the validator already says for both (#539); `add` takes any branch but the tracking one, and its own refusal names the branch lowercased. AGENTS.md stays at 492 lines. `validate-family.py` and `AGENTS-shape.md` are pinned copies: each holder takes them as `upstream-changed` rows on its next update. New tests in tests/test_family_switch_branch_exists.py type each printed line in both states, with no such branch and with one an earlier act left holding a commit of its own, for the three acts and the three validator remedies; three existing tests take the new text. Lane: openreposhape-5 (openRepoShape-5) Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 2 +- scripts/family.py | 36 ++- templates/family-root/AGENTS-shape.md | 2 +- templates/family-root/README.md | 2 +- .../family-root/scripts/validate-family.py | 22 +- tests/test_family_switch_branch_exists.py | 289 ++++++++++++++++++ tests/test_family_tracking_branch.py | 2 +- tests/test_validate_family_branch_step.py | 11 +- tests/test_validate_family_unnamed_mounts.py | 3 +- 9 files changed, 351 insertions(+), 18 deletions(-) create mode 100644 tests/test_family_switch_branch_exists.py diff --git a/AGENTS.md b/AGENTS.md index 23e390bc..270522b8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -371,7 +371,7 @@ that ship together already are. ```sh python3 scripts/family.py init --org --family --into .. [--reuse-empty-repo] -git -C switch -c family/add- # a branch first: see 3 +git -C switch -c family/add- # a branch first: see 3 python3 scripts/family.py add --family-root --member / python3 scripts/family.py bump --family-root --member --to python3 scripts/family.py siblings --family-root diff --git a/scripts/family.py b/scripts/family.py index a3eb0e08..4cdf6d6d 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -705,21 +705,37 @@ def _switch_command(root: Path, branch: str) -> str: return shell_line(["git", "-C", root, "switch", "-c", branch]) +#: What every remedy that names `_switch_command` says next (#607): where an +#: earlier act left that branch, `switch -c` fails `fatal: a branch named +#: '' already exists` (exit 128), and the person had to know `git +#: switch `. That keeps the branch's commits and refuses to overwrite +#: a changed file; nothing here names `switch -C`, which resets the branch. +#: IN WORDS, NOT A SECOND COMMAND: no one line works in both states in every +#: shell this file quotes for (PowerShell 5.1 has no `||`), and README lists +#: the kinds of line printed for PowerShell. The holder's +#: `validate-family.py` says the same words. +BRANCH_LEFT = ("If `git switch -c` says that branch already exists, an " + "earlier act left it: the same command without `-c` switches " + "to it and keeps every commit on it, or give the branch " + "another name.") + + def _checked_branch(family: "Family", verb: str, suggested: str) -> str: """The branch `verb`'s ONE commit lands on: the one the holder stands on, refused when that is the holder's tracking branch or there is none (#487). Asked by the real run, after the refusals about the arguments and the holder and before the member is read or anything is written; - `--dry-run` asks `_branch_said` instead.""" + `--dry-run` asks `_branch_said` instead. The remedy's `switch -c` is + followed by `BRANCH_LEFT`, for a holder that has the branch (#607).""" branch = _holder_branch(family.root) if branch is not None and branch != family.tracking(): return branch remedy = ("Remediation: these organisations are pull-request only, so " "make a branch for the change and run the same command " f"again:\n {_switch_command(family.root, suggested)}\n" - "then push that branch and open a pull request, as the NEXT " - "line the command ends with says. `--dry-run` runs on any " - "branch.") + f"{BRANCH_LEFT}\nThen push that branch and open a pull " + "request, as the NEXT line the command ends with says. " + "`--dry-run` runs on any branch.") if branch is None: raise Refusal( "family-holder-detached", @@ -738,7 +754,8 @@ def _checked_branch(family: "Family", verb: str, suggested: str) -> str: def _branch_said(family: "Family", verb: str, suggested: str) -> list[str]: """The `branch` line a dry run prints (#487): where the real run's commit lands, or that the real run refuses here, by its code, and the - command that makes a branch, on a line of its own.""" + command that makes a branch, on a line of its own, then `BRANCH_LEFT` + as the real run's remedy says it (#607).""" branch = _holder_branch(family.root) tracking = family.tracking() if branch is not None and branch != tracking: @@ -750,7 +767,8 @@ def _branch_said(family: "Family", verb: str, suggested: str) -> list[str]: f"{branch} is the holder's tracking branch")) return [f"branch {where}, where a real {verb} refuses ({code}); " "make a branch first:", - f" {_switch_command(family.root, suggested)}"] + f" {_switch_command(family.root, suggested)}", + f" {BRANCH_LEFT}"] def _print_next(family: "Family", branch: str) -> None: @@ -1361,7 +1379,9 @@ def _print_next_steps(args, work: Path, folder: Path | None) -> None: Split from `cmd_init` for #136. A BRANCH FIRST (#487): the holder stands on its tracking branch, where `add` refuses to commit, so the block - opens with the `git switch -c` that makes the branch `add` suggests. + opens with the `git switch -c` that makes a branch for it. Its member + is ``, the one placeholder the `--member` under it says + (#607); `add`'s own refusal names the branch lowercased. ONE COMMAND A LINE, `cd` apart from `make siblings`: Windows PowerShell 5.1, the shell `setup-project.py` documents, rejects `&&` between two @@ -1387,7 +1407,7 @@ def _print_next_steps(args, work: Path, folder: Path | None) -> None: print(f""" NEXT STEPS - {switch} family/add- + {switch} family/add- {_add_command(work, args.org)} cd {shell_quoted(work)} make siblings diff --git a/templates/family-root/AGENTS-shape.md b/templates/family-root/AGENTS-shape.md index f99a90d6..a9e740e4 100644 --- a/templates/family-root/AGENTS-shape.md +++ b/templates/family-root/AGENTS-shape.md @@ -24,7 +24,7 @@ Run openRepoShape's `scripts/family.py` FROM a checkout of the standard, against this holder: ```sh -git -C switch -c family/add- # a branch first: see below +git -C switch -c family/add- # a branch first: see below python3 scripts/family.py add --family-root --member / python3 scripts/family.py bump --family-root --member --to <40 hex> \ --trailer "Lane: " --trailer "Co-Authored-By: " diff --git a/templates/family-root/README.md b/templates/family-root/README.md index 9242b494..05b8f77c 100644 --- a/templates/family-root/README.md +++ b/templates/family-root/README.md @@ -112,7 +112,7 @@ project the row claims and not merely a repository at the right commit. From a checkout of `{{SHAPE_REPOSITORY}}`: ```sh -git -C switch -c family/add- +git -C switch -c family/add- python3 scripts/family.py add --family-root --member {{ORG}}/ python3 scripts/family.py bump --family-root --member --to <40 hex> python3 scripts/family.py remove --family-root --member diff --git a/templates/family-root/scripts/validate-family.py b/templates/family-root/scripts/validate-family.py index 450bac3d..97e892c8 100755 --- a/templates/family-root/scripts/validate-family.py +++ b/templates/family-root/scripts/validate-family.py @@ -120,6 +120,18 @@ #: and in `--member` alike (#539). PROJECT_PLACEHOLDER = "" +#: What a remedy that names `_switch_line` says at the end of its prose +#: (#607): where an earlier act left that branch, `switch -c` fails `fatal: a +#: branch named '' already exists` (exit 128). The same command +#: without `-c` switches to it and keeps its commits; nothing here names +#: `switch -C`, which resets it. In words, not a second command: this report +#: is read in CI too, where the checkout has none of the person's branches to +#: ask about. `family.py`'s `BRANCH_LEFT`, word for word. +BRANCH_LEFT = ("If `git switch -c` says that branch already exists, an " + "earlier act left it: the same command without `-c` switches " + "to it and keeps every commit on it, or give the branch " + "another name.") + def _switch_line(root: Path, verb: str, name: str | None) -> str: """`git -C switch -c family/-`: the branch @@ -129,7 +141,8 @@ def _switch_line(root: Path, verb: str, name: str | None) -> str: `name` is the member's row `id:` for `remove` and its project for `add`, lowercased; None says `PROJECT_PLACEHOLDER`, the one the `--member` beside it says. POSIX-quoted, as every other line this file - prints is.""" + prints is. Every remedy that names it ends its prose with `BRANCH_LEFT` + (#607).""" if name is None: return (f"git -C {shlex.quote(str(root))} switch -c " f"family/{verb}-{PROJECT_PLACEHOLDER}") @@ -401,7 +414,7 @@ def _check_member_gitlink(root: Path, path: str, commit: str, project: str, "command below mounts and pins it in one commit, on a branch " "other than the holder's tracking branch, which it refuses, so " "the first makes one; a row with no submodule is a claim about a " - "tree that is not here.\n" + f"tree that is not here. {BRANCH_LEFT}\n" f" {_switch_line(root, 'add', project)}\n" f" {PYTHON} scripts/family.py add --family-root " f"{shlex.quote(str(root))} --member /{shlex.quote(project)}") @@ -784,7 +797,7 @@ def _check_member_ids_unique(root: Path, members: list, f"`{PYTHON} scripts/family.py remove " f"--family-root {shlex.quote(str(root))} --member " f"{shlex.quote(project)}` takes the later row, {project}, out; " - f"name {first} there instead to keep {project}.") + f"name {first} there instead to keep {project}. {BRANCH_LEFT}") def _gitmodules_sections(root: Path) -> dict[str, dict[str, str]]: @@ -1122,7 +1135,8 @@ def _check_gitlinks_named(root: Path, gitlinks: dict[str, str], f"{GITMODULES} section out too, then mount it with the third, " "from a checkout of openRepoShape: family.py add writes its row " "and its gitlink in one commit, and names the object store git " - "rm leaves under .git/modules if one is still there.\n" + "rm leaves under .git/modules if one is still there. " + f"{BRANCH_LEFT}\n" f" {_switch_line(root, 'add', project)}\n" f" git rm -- {shlex.quote(path)}\n" f" {PYTHON} scripts/family.py add --family-root " diff --git a/tests/test_family_switch_branch_exists.py b/tests/test_family_switch_branch_exists.py new file mode 100644 index 00000000..311d9a7f --- /dev/null +++ b/tests/test_family_switch_branch_exists.py @@ -0,0 +1,289 @@ +# SPDX-License-Identifier: Apache-2.0 +"""The `git switch -c family/-` remedy where an earlier act left +that branch, and one placeholder for the member in the branch (#607). + +THE BRANCH THAT IS THERE ALREADY. `family.py add`, `bump` and `remove` +refuse the holder's tracking branch with `git -C switch -c +family/-` (#487), and the holder's `validate-family.py` names +the same line before each `add` and `remove` it prints (#539). Where an +earlier act left that branch the line fails, `fatal: a branch named +'family/add-irqs' already exists`, exit 128, and neither tool said what to +do next. Both now end the remedy with ONE sentence, the same words in both: +the same command without `-c` switches to the branch and keeps every commit +on it, or give the branch another name. Neither prints a command that +resets or deletes it. + +THE PROOF IS THE PRINTED LINE, TYPED, IN BOTH STATES. With no such branch, +the `switch -c` makes it. With one an earlier act left, holding a commit of +its own, the `switch -c` fails as git says and changes nothing, the same +command without `-c` switches to it, the act goes through there, and the +earlier commit is the parent of the act's. + +ONE PLACEHOLDER (L2-45). AGENTS.md, the holder's README and AGENTS-shape.md +and `init`'s NEXT STEPS spelled the branch `family/add-` beside +`--member /`, two spellings of one value; the holder's +validator already says `` for both (#539). All of them now say +``, and the branch filled in as written, in the project's own case, +is one `add` commits on. + +THE SAME FAMILY AS `tests/test_family.py`, in `on_main`'s private copy of +the holder on its tracking branch. Nothing here pushes. +""" + +from __future__ import annotations + +import re +import shlex +import subprocess +from pathlib import Path +from types import SimpleNamespace + +import pytest + +from conftest import REPO, git, run_script +from repo_shape import PYTHON +from test_family import ( # noqa: F401 + ORG, SPARE, family, family_module, manifest, mount_twice, validate, +) +from test_family_printed_quoting import host_words +from test_family_tracking_branch import ( # noqa: F401 + TRACKING, act, next_push, on_main, remedy_switch, snapshot, suggested, +) +# `BRANCH_LEFT` is the sentence both tools end the remedy with, pinned as +# text: a person, an agent and a holder's CI read it, so it is not rebuilt +# from either tool's own constant here. +from test_validate_family_branch_step import BRANCH_LEFT, gate_module +from test_validate_family_unnamed_mounts import spare_commits, stage_gitlink + +#: Each state a remedy is typed in: no branch of that name, and one an +#: earlier act left. +STATES = pytest.mark.parametrize("left", [False, True], + ids=["no-branch", "branch-left"]) + + +def leave_branch(holder: Path, branch: str) -> str: + """What an earlier act leaves: `branch`, made from the holder's HEAD and + holding one commit of its own, while the holder stays where it stands. + Written with `commit-tree` and `update-ref`, so nothing is checked out. + Returns that commit.""" + earlier = git("commit-tree", "-p", "HEAD", "-m", "An earlier act's work", + "HEAD^{tree}", cwd=holder).stdout.strip() + git("update-ref", f"refs/heads/{branch}", earlier, cwd=holder) + return earlier + + +def typed(argv: list[str]) -> subprocess.CompletedProcess: + """`argv` run as a person runs the line it was read from.""" + return subprocess.run(argv, capture_output=True, text=True, check=False) + + +def without_c(argv: list[str]) -> list[str]: + """The same command without `-c`, as the sentence says.""" + assert argv.count("-c") == 1, argv + return [word for word in argv if word != "-c"] + + +def on_branch(holder: Path) -> str: + return git("symbolic-ref", "--short", "HEAD", cwd=holder).stdout.strip() + + +def head(holder: Path) -> str: + return git("rev-parse", "HEAD", cwd=holder).stdout.strip() + + +def type_switch(holder: Path, switch: list[str], branch: str, + earlier: str | None) -> None: + """The remedy's `switch -c`, typed as printed. With no branch of that + name (`earlier` None) it makes it. With one an earlier act left, it + fails as #607 says and changes nothing, and the same command without + `-c` switches to that branch, at its own commit.""" + before = head(holder) + made = typed(switch) + if earlier is None: + assert made.returncode == 0, made.stderr + else: + assert made.returncode == 128, made.stderr + assert re.search(rf"fatal: a branch named '{re.escape(branch)}' " + "already exists", made.stderr, re.I), made.stderr + assert head(holder) == before + assert on_branch(holder) == TRACKING + back = typed(without_c(switch)) + assert back.returncode == 0, back.stderr + assert head(holder) == earlier + assert on_branch(holder) == branch + + +# --- family.py: the refusal, typed as printed, in both states --------------- + +@STATES +@pytest.mark.parametrize("verb", ["add", "bump", "remove"]) +def test_the_refusal_typed_as_printed_puts_the_act_on_its_branch( + family, on_main, verb, left): + """THE ISSUE, for each act. Refused on `main`, the remedy's `switch -c` + is followed by the sentence, and names no command that resets or + deletes a branch. Typed as printed in either state, it puts the holder + on the branch, and the act then commits there: where an earlier act left + the branch, on top of the commit it holds.""" + holder = on_main + branch = suggested(family, verb) + earlier = leave_branch(holder, branch) if left else None + before = snapshot(holder) + refused = act(family, holder, verb) + assert refused.returncode == 2, refused.stdout + refused.stderr + assert snapshot(holder) == before + lines = refused.stderr.splitlines() + switch = remedy_switch(refused.stderr) + at = next(i for i, line in enumerate(lines) + if line.startswith(" ") and host_words(line) == switch) + assert lines[at + 1] == BRANCH_LEFT, lines + assert lines[at + 2].startswith("Then push that branch"), lines + for discards in ("switch -C", "branch -D", "reset", "--discard-changes"): + assert discards not in refused.stderr, refused.stderr + + type_switch(holder, switch, branch, earlier) + done = act(family, holder, verb) + assert done.returncode == 0, done.stdout + done.stderr + assert on_branch(holder) == branch + if earlier is not None: + assert git("rev-parse", "HEAD~1", cwd=holder).stdout.strip() == \ + earlier + push, _ = next_push(done.stdout) + assert push[-1] == branch, push + assert validate(holder).returncode == 0 + + +@pytest.mark.parametrize("verb", ["add", "bump", "remove"]) +def test_a_dry_run_on_the_tracking_branch_says_it_too(family, on_main, verb): + """`--dry-run` names the real run's `switch -c` on a line of its own, + and the sentence under it, as the real run's remedy does.""" + result = act(family, on_main, verb, "--dry-run") + assert result.returncode == 0, result.stdout + result.stderr + lines = result.stdout.splitlines() + at = lines.index(f"branch {TRACKING} is the holder's tracking " + f"branch, where a real {verb} refuses " + "(family-on-tracking-branch); make a branch first:") + assert host_words(lines[at + 1])[-2:] == ["-c", suggested(family, verb)] + assert lines[at + 2] == " " + BRANCH_LEFT + + +# --- validate-family.py: each remedy that names the branch ------------------ + +@STATES +def test_a_missing_gitlinks_remedy_typed_as_printed(on_main, left): + """`member-gitlink-absent`: the remedy's prose ends with the sentence, + and its first command, typed as printed, puts the holder on the branch + the `add` under it needs, in either state.""" + holder = on_main + git("rm", "-r", "-q", "-f", "--", "members/IRRS", cwd=holder) + git("commit", "-qm", "Drop the gitlink but keep the row", cwd=holder) + earlier = leave_branch(holder, "family/add-irrs") if left else None + result = validate(holder) + assert result.returncode == 2, result.stdout + result.stderr + lines = result.stderr.splitlines() + assert lines[0].startswith("REFUSED member-gitlink-absent: "), lines + assert lines[1].endswith( + "a row with no submodule is a claim about a tree that is not here. " + + BRANCH_LEFT), lines[1] + switch = shlex.split(lines[2]) + assert switch[-2:] == ["-c", "family/add-irrs"], lines[2] + type_switch(holder, switch, "family/add-irrs", earlier) + + +@STATES +def test_a_duplicate_ids_remedy_typed_as_printed(family, on_main, left): + """`member-duplicate-id`: the finding ends with the sentence. Its + `switch -c`, typed as printed in either state, puts the holder on the + branch, and the `remove` beside it, run as printed from a checkout of + openRepoShape, then takes the later row out there.""" + holder = on_main + mount_twice(holder, family["remotes"]) + earlier = leave_branch(holder, "family/remove-irrs") if left else None + found = validate(holder) + assert found.returncode == 1, found.stderr + found.stdout + line = found.stderr.split("FINDING member-duplicate-id: ", + 1)[1].splitlines()[0] + assert line.endswith(". " + BRANCH_LEFT), line + switch, remove = [command for command in re.findall(r"`([^`]*)`", line) + if command.startswith(("git -C", PYTHON))] + type_switch(holder, shlex.split(switch), "family/remove-irrs", earlier) + removed = run_script(REPO / "scripts" / "family.py", + *shlex.split(remove)[2:], cwd=REPO) + assert removed.returncode == 0, removed.stderr + removed.stdout + if earlier is not None: + assert git("rev-parse", "HEAD~1", cwd=holder).stdout.strip() == \ + earlier + assert "IRRS2" not in [row["project"] + for row in manifest(holder)["members"]] + + +@STATES +def test_an_unnamed_gitlinks_remedy_typed_as_printed(family, on_main, left): + """`member-gitlink-unnamed` for a hand mount: the finding's prose ends + with the sentence, and its first command, typed as printed in either + state, puts the holder on the branch `add` needs.""" + holder = on_main + _, tip = spare_commits(family) + stage_gitlink(holder, f"members/{SPARE}", tip) + branch = f"family/add-{SPARE.lower()}" + earlier = leave_branch(holder, branch) if left else None + result = validate(holder, "--no-members") + assert result.returncode == 1, result.stderr + result.stdout + finding = result.stderr.split("FINDING member-gitlink-unnamed: ", 1)[1] + assert finding.splitlines()[0].endswith( + "if one is still there. " + BRANCH_LEFT), finding + switch = shlex.split(finding.splitlines()[1]) + assert switch[-2:] == ["-c", branch], switch + type_switch(holder, switch, branch, earlier) + + +def test_the_two_tools_say_the_same_words(monkeypatch): + """ONE ANSWER: `family.py` and the holder's validator, which a holder + carries as a copy and which cannot import `family.py`, spell the + sentence the same, word for word.""" + assert family_module().BRANCH_LEFT == BRANCH_LEFT + assert gate_module(monkeypatch).BRANCH_LEFT == BRANCH_LEFT + + +# --- one placeholder for the member (L2-45) ---------------------------------- + +#: The `switch -c` and the first `add` of each doc's block, each read for the +#: placeholder it puts where the member goes. +BRANCH_WORD = re.compile(r" switch -c family/add-(\S+)") +MEMBER_WORD = re.compile(r" add +--family-root \S+ --member \S*/(\S+)") + + +@pytest.mark.parametrize("doc", [ + "AGENTS.md", "templates/family-root/AGENTS-shape.md", + "templates/family-root/README.md"]) +def test_each_doc_spells_the_branch_as_its_member(doc): + """The branch and `--member` beside it stand for one project, so they + say it one way, ``, the placeholder the holder's validator + says for both (#539).""" + text = (REPO / doc).read_text(encoding="utf-8") + assert BRANCH_WORD.search(text).group(1) == "", doc + assert MEMBER_WORD.search(text).group(1) == "", doc + + +def test_init_next_steps_spell_the_branch_as_its_member(capsys, tmp_path): + """`init`'s NEXT STEPS open with the same pair, spelled the same way.""" + work = tmp_path / "Orbit" / "Orbit" + family_module()._print_next_steps( + SimpleNamespace(org=ORG, tracking_branch=TRACKING), work, None) + printed = capsys.readouterr().out + assert BRANCH_WORD.search(printed).group(1) == "", printed + assert MEMBER_WORD.search(printed).group(1) == "", printed + + +def test_the_branch_filled_in_as_written_is_one_add_commits_on(family, + on_main): + """`` filled in as the project is spelled makes a branch the + act takes: any branch but the tracking one is (#487). `add`'s own + refusal names it lowercased; the case is not what the act asks about.""" + holder = on_main + branch = f"family/add-{SPARE}" + git("switch", "-q", "-c", branch, cwd=holder) + done = act(family, holder, "add") + assert done.returncode == 0, done.stdout + done.stderr + push, _ = next_push(done.stdout) + assert push[-1] == branch, push + assert SPARE in [row["project"] for row in manifest(holder)["members"]] diff --git a/tests/test_family_tracking_branch.py b/tests/test_family_tracking_branch.py index 054c011e..93467d12 100644 --- a/tests/test_family_tracking_branch.py +++ b/tests/test_family_tracking_branch.py @@ -260,7 +260,7 @@ def test_init_next_steps_make_the_branch_before_the_add(tmp_path): result.stdout.split("NEXT STEPS", 1)[1].splitlines() if line.strip()] assert host_words(block[0]) == ["git", "-C", str(holder), "switch", - "-c", "family/add-"] + "-c", "family/add-"] assert " add --family-root " in block[1] assert git("symbolic-ref", "--short", "HEAD", cwd=holder).stdout.strip() \ == TRACKING diff --git a/tests/test_validate_family_branch_step.py b/tests/test_validate_family_branch_step.py index d55915fb..5bb60319 100644 --- a/tests/test_validate_family_branch_step.py +++ b/tests/test_validate_family_branch_step.py @@ -47,6 +47,14 @@ #: How the holder's THE LOCKSTEP RULE starts. RULE = "THE LOCKSTEP RULE: a member's gitlink and its `members[].pin.commit`" +#: The sentence each remedy that names a `switch -c` ends its prose with +#: where that branch exists already (#607), pinned as text, as `family.py` +#: and the holder's validator both say it. +BRANCH_LEFT = ("If `git switch -c` says that branch already exists, an " + "earlier act left it: the same command without `-c` switches " + "to it and keeps every commit on it, or give the branch " + "another name.") + def gate_module(monkeypatch): """The template `validate-family.py`, loaded as a module. It puts its @@ -143,7 +151,8 @@ def test_a_missing_gitlink_remedy_makes_the_branch_and_names_the_holder( "Remediation: from a checkout of openRepoShape, the second command " "below mounts and pins it in one commit, on a branch other than the " "holder's tracking branch, which it refuses, so the first makes one; " - "a row with no submodule is a claim about a tree that is not here.") + "a row with no submodule is a claim about a tree that is not here. " + + BRANCH_LEFT) switch, add = (line.strip() for line in lines[2:]) assert lines[2:] == [" " + switch, " " + add], lines assert shlex.split(switch) == switch_argv(holder, "add", "IRRS") diff --git a/tests/test_validate_family_unnamed_mounts.py b/tests/test_validate_family_unnamed_mounts.py index bcce579c..0f50dff1 100644 --- a/tests/test_validate_family_unnamed_mounts.py +++ b/tests/test_validate_family_unnamed_mounts.py @@ -68,6 +68,7 @@ FAMILY, ORG, SPARE, family, family_module, holder, manifest, validate, ) from test_family_add_refusals import git_section, gitmodules_with +from test_validate_family_branch_step import BRANCH_LEFT from test_validate_family_member_path_glob import hand_edit MODES = pytest.mark.parametrize( @@ -91,7 +92,7 @@ "its .gitmodules section out too, then mount it with the third, from a " "checkout of openRepoShape: family.py add writes its row and its gitlink " "in one commit, and names the object store git rm leaves under " - ".git/modules if one is still there.") + ".git/modules if one is still there. " + BRANCH_LEFT) def switch_for(holder, member: str) -> str: From 66b10431f65f8b2fbd831377b73d95c45cfc743e Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Fri, 16 Oct 2026 00:16:05 +0000 Subject: [PATCH 2/3] family.py and validate-family.py: where an earlier act left the branch, the `switch -c` remedy puts another name first and going back second, for a branch that holds work not merged yet, so neither a branch behind a moved main nor a detached commit is the first advice (#607) Fix round 1 on #637, from the review of 6212c311. S1. The sentence sent a person back to the old branch first. That is right only where the branch holds work not merged yet. Probed in a real local holder: with no commits on the old branch and `main` moved on since (a later member squash-merged), going back and running `add` exits 0, but the branch then conflicts with `main` (`CONFLICT (add/add)` in .gitmodules and `CONFLICT (content)` in family.yaml, `git merge-tree --write-tree` exit 1); with a commit made on a detached HEAD, `git switch ` leaves that commit behind (`git branch --contains` names no branch). Another name starts where the holder stands: `add` exits 0, the merge-tree exits 0, and the detached commit stays on the new branch. `BRANCH_LEFT` now reads: "If `git switch -c` says that branch already exists, an earlier act left it: give the branch another name, which starts it where the holder stands, or, only if that branch holds work not merged yet, the same command with `switch` in place of `switch -c` goes back to it and keeps every commit on it." It is word for word the same in scripts/family.py, in the pinned templates/family-root/scripts/validate-family.py and in the pinned constant in tests/test_validate_family_branch_step.py, and the comments that described the old order are rewritten. The sentence still prints no second command and no `||`. New tests in tests/test_family_switch_branch_exists.py, each run as printed in a real local holder: - `test_another_name_works_where_main_has_moved_since`: an earlier act's branch is left behind a `main` that a later member moved. The printed line fails with `already exists`, the same line with another name works, `add` exits 0, and `git merge-tree --write-tree main ` exits 0. - `test_another_name_keeps_a_commit_made_on_a_detached_head`: after the printed line with another name, `git branch --contains ` names the new branch. S2. #610 adds a fourth `_switch_line` to the validator (in `_lockstep`) with no sentence. `test_every_remedy_that_names_a_switch_line_ends_with_the_sentence` reads the template's source with `ast`, finds every call of `_switch_line` however its string is passed on (a `Refusal`, a `report.finding`, or the `return` of a helper), and requires the prose of the string it stands in, up to the first line break, to end with `BRANCH_LEFT`. `test_the_guard_sees_a_remedy_left_without_the_sentence` shows it fails on the source with the sentence taken out of any one remedy, and on a fourth remedy built by a helper as `_lockstep` builds its. Whichever of #637 and #634 lands second must end `_lockstep`'s prose with `BRANCH_LEFT`, and CI fails until it does. N6. The init NEXT STEPS test read the lines with a regex whose `--family-root \S+` breaks on a root with a space in it; it now reads the words with `host_words`, as the NEXT STEPS test beside it does. A correction to the first commit's body, which says neither tool can print the one line that works by asking first: that holds for the validator, whose report is read in CI where the checkout holds none of the person's branches. `family.py` could ask, and says the validator's words so the two tools keep one rule. Lane: openreposhape-5 (openRepoShape-5) Co-Authored-By: Claude Sonnet 5.5 --- scripts/family.py | 19 +- .../family-root/scripts/validate-family.py | 24 +- tests/test_family_switch_branch_exists.py | 269 ++++++++++++++++-- tests/test_validate_family_branch_step.py | 8 +- 4 files changed, 274 insertions(+), 46 deletions(-) diff --git a/scripts/family.py b/scripts/family.py index 4cdf6d6d..9c3c6d05 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -707,17 +707,24 @@ def _switch_command(root: Path, branch: str) -> str: #: What every remedy that names `_switch_command` says next (#607): where an #: earlier act left that branch, `switch -c` fails `fatal: a branch named -#: '' already exists` (exit 128), and the person had to know `git -#: switch `. That keeps the branch's commits and refuses to overwrite -#: a changed file; nothing here names `switch -C`, which resets the branch. +#: '' already exists` (exit 128). ANOTHER NAME COMES FIRST, because a +#: new branch starts where the holder stands now, which is right in every +#: state. Going back to the old branch is right only where it holds work not +#: merged yet: one left behind a main that has moved on makes the act collide +#: with what main took in since, or find its member already there, and a +#: commit made on a detached HEAD is left behind by any `git switch `. +#: `git switch` keeps the branch's commits and refuses to overwrite a changed +#: file; nothing here names `switch -C`, which resets the branch. #: IN WORDS, NOT A SECOND COMMAND: no one line works in both states in every #: shell this file quotes for (PowerShell 5.1 has no `||`), and README lists #: the kinds of line printed for PowerShell. The holder's #: `validate-family.py` says the same words. BRANCH_LEFT = ("If `git switch -c` says that branch already exists, an " - "earlier act left it: the same command without `-c` switches " - "to it and keeps every commit on it, or give the branch " - "another name.") + "earlier act left it: give the branch another name, which " + "starts it where the holder stands, or, only if that branch " + "holds work not merged yet, the same command with `switch` " + "in place of `switch -c` goes back to it and keeps every " + "commit on it.") def _checked_branch(family: "Family", verb: str, suggested: str) -> str: diff --git a/templates/family-root/scripts/validate-family.py b/templates/family-root/scripts/validate-family.py index 97e892c8..44ef5052 100755 --- a/templates/family-root/scripts/validate-family.py +++ b/templates/family-root/scripts/validate-family.py @@ -122,15 +122,20 @@ #: What a remedy that names `_switch_line` says at the end of its prose #: (#607): where an earlier act left that branch, `switch -c` fails `fatal: a -#: branch named '' already exists` (exit 128). The same command -#: without `-c` switches to it and keeps its commits; nothing here names -#: `switch -C`, which resets it. In words, not a second command: this report -#: is read in CI too, where the checkout has none of the person's branches to -#: ask about. `family.py`'s `BRANCH_LEFT`, word for word. +#: branch named '' already exists` (exit 128). Another name comes +#: first: it starts the branch where the holder stands now, which is right in +#: every state. The same command with `switch` for `switch -c` goes back to +#: the old branch, which is right only where it holds work not merged yet; +#: it keeps its commits, and nothing here names `switch -C`, which resets it. +#: In words, not a second command: this report is read in CI too, where the +#: checkout has none of the person's branches to ask about. `family.py`'s +#: `BRANCH_LEFT`, word for word. BRANCH_LEFT = ("If `git switch -c` says that branch already exists, an " - "earlier act left it: the same command without `-c` switches " - "to it and keeps every commit on it, or give the branch " - "another name.") + "earlier act left it: give the branch another name, which " + "starts it where the holder stands, or, only if that branch " + "holds work not merged yet, the same command with `switch` " + "in place of `switch -c` goes back to it and keeps every " + "commit on it.") def _switch_line(root: Path, verb: str, name: str | None) -> str: @@ -142,7 +147,8 @@ def _switch_line(root: Path, verb: str, name: str | None) -> str: `add`, lowercased; None says `PROJECT_PLACEHOLDER`, the one the `--member` beside it says. POSIX-quoted, as every other line this file prints is. Every remedy that names it ends its prose with `BRANCH_LEFT` - (#607).""" + (#607); openRepoShape's own tests read this file's source and fail one + that does not.""" if name is None: return (f"git -C {shlex.quote(str(root))} switch -c " f"family/{verb}-{PROJECT_PLACEHOLDER}") diff --git a/tests/test_family_switch_branch_exists.py b/tests/test_family_switch_branch_exists.py index 311d9a7f..eaa8fc35 100644 --- a/tests/test_family_switch_branch_exists.py +++ b/tests/test_family_switch_branch_exists.py @@ -9,15 +9,25 @@ earlier act left that branch the line fails, `fatal: a branch named 'family/add-irqs' already exists`, exit 128, and neither tool said what to do next. Both now end the remedy with ONE sentence, the same words in both: -the same command without `-c` switches to the branch and keeps every commit -on it, or give the branch another name. Neither prints a command that -resets or deletes it. +give the branch another name, which starts it where the holder stands, or, +only if that branch holds work not merged yet, the same command with +`switch` in place of `switch -c` goes back to it and keeps every commit on +it. Neither prints a command that resets or deletes it. -THE PROOF IS THE PRINTED LINE, TYPED, IN BOTH STATES. With no such branch, +THE PROOF IS THE PRINTED LINE, TYPED, IN EVERY STATE. With no such branch, the `switch -c` makes it. With one an earlier act left, holding a commit of its own, the `switch -c` fails as git says and changes nothing, the same -command without `-c` switches to it, the act goes through there, and the -earlier commit is the parent of the act's. +command with `switch` for `switch -c` goes back to it, the act goes through +there, and the earlier commit is the parent of the act's. Another name is +the way out where going back is wrong, and the reason it comes first: where +`main` has moved on since the earlier act (a later member was squash-merged), +the new branch starts at that `main` and `add` goes through and merges with +no conflict; where the holder stands on a detached HEAD that carries a +commit, the new branch keeps it. + +THE GUARD. Every remedy of the holder's validator that names `_switch_line` +ends its prose with the sentence, read from the template's source, so a +fourth remedy added later (#610's `_lockstep`) that forgets it fails here. ONE PLACEHOLDER (L2-45). AGENTS.md, the holder's README and AGENTS-shape.md and `init`'s NEXT STEPS spelled the branch `family/add-` beside @@ -32,6 +42,7 @@ from __future__ import annotations +import ast import re import shlex import subprocess @@ -43,7 +54,8 @@ from conftest import REPO, git, run_script from repo_shape import PYTHON from test_family import ( # noqa: F401 - ORG, SPARE, family, family_module, manifest, mount_twice, validate, + FAMILY, ORG, SPARE, family, family_module, manifest, mount_twice, + scaffold_member, validate, ) from test_family_printed_quoting import host_words from test_family_tracking_branch import ( # noqa: F401 @@ -52,7 +64,7 @@ # `BRANCH_LEFT` is the sentence both tools end the remedy with, pinned as # text: a person, an agent and a holder's CI read it, so it is not rebuilt # from either tool's own constant here. -from test_validate_family_branch_step import BRANCH_LEFT, gate_module +from test_validate_family_branch_step import BRANCH_LEFT, TEMPLATE, gate_module from test_validate_family_unnamed_mounts import spare_commits, stage_gitlink #: Each state a remedy is typed in: no branch of that name, and one an @@ -78,7 +90,8 @@ def typed(argv: list[str]) -> subprocess.CompletedProcess: def without_c(argv: list[str]) -> list[str]: - """The same command without `-c`, as the sentence says.""" + """The same command with `switch` in place of `switch -c`, as the + sentence says.""" assert argv.count("-c") == 1, argv return [word for word in argv if word != "-c"] @@ -91,21 +104,40 @@ def head(holder: Path) -> str: return git("rev-parse", "HEAD", cwd=holder).stdout.strip() +def remedy_lines(stderr: str) -> tuple[list[str], list[str]]: + """The remedy's `switch -c` as the reader's shell reads it, and the + lines under it.""" + lines = stderr.splitlines() + switch = remedy_switch(stderr) + at = next(i for i, line in enumerate(lines) + if line.startswith(" ") and host_words(line) == switch) + return switch, lines[at + 1:] + + +def refused_as_left(holder: Path, switch: list[str], branch: str) -> None: + """The remedy's `switch -c`, typed as printed where `branch` is there: + git's `fatal: a branch named ... already exists`, exit 128, and nothing + changed.""" + before = head(holder) + made = typed(switch) + assert made.returncode == 128, made.stderr + assert re.search(rf"fatal: a branch named '{re.escape(branch)}' " + "already exists", made.stderr, re.I), made.stderr + assert head(holder) == before + + def type_switch(holder: Path, switch: list[str], branch: str, earlier: str | None) -> None: """The remedy's `switch -c`, typed as printed. With no branch of that name (`earlier` None) it makes it. With one an earlier act left, it - fails as #607 says and changes nothing, and the same command without - `-c` switches to that branch, at its own commit.""" - before = head(holder) - made = typed(switch) + fails as #607 says and changes nothing, and the same command with + `switch` in place of `switch -c` goes back to that branch, at its own + commit.""" if earlier is None: + made = typed(switch) assert made.returncode == 0, made.stderr else: - assert made.returncode == 128, made.stderr - assert re.search(rf"fatal: a branch named '{re.escape(branch)}' " - "already exists", made.stderr, re.I), made.stderr - assert head(holder) == before + refused_as_left(holder, switch, branch) assert on_branch(holder) == TRACKING back = typed(without_c(switch)) assert back.returncode == 0, back.stderr @@ -113,6 +145,21 @@ def type_switch(holder: Path, switch: list[str], branch: str, assert on_branch(holder) == branch +def type_other_name(holder: Path, switch: list[str], branch: str) -> str: + """The remedy's `switch -c` where `branch` is there, typed as printed + (refused), then with another name: the new branch starts where the + holder stands. Returns its name.""" + assert switch[-1] == branch, switch + refused_as_left(holder, switch, branch) + other = f"{branch}-2" + before = head(holder) + made = typed([*switch[:-1], other]) + assert made.returncode == 0, made.stderr + assert head(holder) == before + assert on_branch(holder) == other + return other + + # --- family.py: the refusal, typed as printed, in both states --------------- @STATES @@ -131,12 +178,9 @@ def test_the_refusal_typed_as_printed_puts_the_act_on_its_branch( refused = act(family, holder, verb) assert refused.returncode == 2, refused.stdout + refused.stderr assert snapshot(holder) == before - lines = refused.stderr.splitlines() - switch = remedy_switch(refused.stderr) - at = next(i for i, line in enumerate(lines) - if line.startswith(" ") and host_words(line) == switch) - assert lines[at + 1] == BRANCH_LEFT, lines - assert lines[at + 2].startswith("Then push that branch"), lines + switch, under = remedy_lines(refused.stderr) + assert under[0] == BRANCH_LEFT, under + assert under[1].startswith("Then push that branch"), under for discards in ("switch -C", "branch -D", "reset", "--discard-changes"): assert discards not in refused.stderr, refused.stderr @@ -166,6 +210,91 @@ def test_a_dry_run_on_the_tracking_branch_says_it_too(family, on_main, verb): assert lines[at + 2] == " " + BRANCH_LEFT +# --- family.py: another name, where going back to the branch is wrong -------- + +#: The member a later pull request adds, and squash-merges, while an earlier +#: act's branch sits: a project that is not one the fixture mounts or keeps +#: spare. +LATER = "IRXS" + + +def move_main_on(holder: Path, remotes_base: Path) -> None: + """What `main` does while an earlier act's branch sits: another member is + added on a branch of its own and its pull request squash-merged, so `main` + holds one new commit with that branch's tree on the main the earlier + branch was made from. The holder is back on `main` and clean.""" + scaffold_member(remotes_base, LATER) + git("switch", "-q", "-c", f"family/add-{LATER.lower()}", cwd=holder) + added = run_script(FAMILY, "add", "--family-root", str(holder), + "--member", f"{ORG}/{LATER}", "--local-remote-dir", + str(remotes_base / "remotes")) + assert added.returncode == 0, added.stderr + added.stdout + squash = git("commit-tree", "-p", TRACKING, "-m", f"Add {LATER} (#1)", + "HEAD^{tree}", cwd=holder).stdout.strip() + git("update-ref", f"refs/heads/{TRACKING}", squash, cwd=holder) + git("switch", "-q", TRACKING, cwd=holder) + assert git("status", "--porcelain", cwd=holder).stdout == "" + + +def test_another_name_works_where_main_has_moved_since(family, on_main, + tmp_path): + """STATE (d), the review's: an earlier act left `family/add-irqs` at the + `main` of its day, holding no commit, and a later member has been + squash-merged since. The refusal's `switch -c`, typed as printed, fails; + the same line with another name starts the branch at the `main` the + holder stands on, `add` goes through there, and what it commits merges + into `main` with no conflict. (Going back to the old branch is the + other way, and it conflicts: the sentence puts it second, for a branch + that holds work not merged yet.)""" + holder = on_main + branch = suggested(family, "add") + git("branch", branch, cwd=holder) + later = tmp_path / "later" + later.mkdir() + move_main_on(holder, later) + assert git("rev-parse", branch, cwd=holder).stdout == \ + git("rev-parse", f"{TRACKING}~1", cwd=holder).stdout + refused = act(family, holder, "add") + assert refused.returncode == 2, refused.stdout + refused.stderr + switch, under = remedy_lines(refused.stderr) + assert under[0] == BRANCH_LEFT, under + other = type_other_name(holder, switch, branch) + done = act(family, holder, "add") + assert done.returncode == 0, done.stdout + done.stderr + merged = git("merge-tree", "--write-tree", TRACKING, other, cwd=holder, + check=False) + assert merged.returncode == 0, merged.stdout + projects = [row["project"] for row in manifest(holder)["members"]] + assert LATER in projects + assert SPARE in projects + assert validate(holder).returncode == 0 + + +def test_another_name_keeps_a_commit_made_on_a_detached_head(family, on_main): + """`family-holder-detached` with a commit made on the detached HEAD, and + the earlier act's branch there already. The refusal's `switch -c`, typed + as printed, fails; the same line with another name keeps that commit: + `git branch --contains` names the new branch and no other. (Going back + to a branch with `switch` leaves a commit no branch holds behind, which + is why the sentence says that way only for a branch with work on it.)""" + holder = on_main + branch = suggested(family, "add") + git("branch", branch, cwd=holder) + git("switch", "-q", "--detach", cwd=holder) + git("commit", "-q", "--allow-empty", "-m", "Work done on a detached HEAD", + cwd=holder) + work = head(holder) + refused = act(family, holder, "add") + assert refused.returncode == 2, refused.stdout + refused.stderr + assert "REFUSED family-holder-detached: " in refused.stderr + switch, under = remedy_lines(refused.stderr) + assert under[0] == BRANCH_LEFT, under + other = type_other_name(holder, switch, branch) + held = git("branch", "--contains", work, "--format=%(refname:short)", + cwd=holder).stdout.split() + assert held == [other] + + # --- validate-family.py: each remedy that names the branch ------------------ @STATES @@ -236,6 +365,85 @@ def test_an_unnamed_gitlinks_remedy_typed_as_printed(family, on_main, left): type_switch(holder, switch, branch, earlier) +def as_text(node: ast.expr) -> str: + """A string expression as the text it builds, each value in it a + `{expression}` placeholder: an f-string, a `+` of strings, a literal.""" + if isinstance(node, ast.Constant) and isinstance(node.value, str): + return node.value + if isinstance(node, ast.JoinedStr): + return "".join(as_text(part) for part in node.values) + if isinstance(node, ast.FormattedValue): + return "{" + ast.unparse(node.value) + "}" + if isinstance(node, ast.BinOp) and isinstance(node.op, ast.Add): + return as_text(node.left) + as_text(node.right) + return "{" + ast.unparse(node) + "}" + + +def switch_remedies(source: str) -> list[tuple[int, str]]: + """Each place `source` names `_switch_line`: its line and the PROSE of + the string it stands in, as text up to the first line break (the + command lines come under it). The string is the whole expression the + call is written in, however it is passed on: the f-string of a + `Refusal` or a `report.finding`, or the `return` of a helper.""" + tree = ast.parse(source) + parents = {child: node for node in ast.walk(tree) + for child in ast.iter_child_nodes(node)} + found = [] + for call in ast.walk(tree): + if not (isinstance(call, ast.Call) + and isinstance(call.func, ast.Name) + and call.func.id == "_switch_line"): + continue + string = call + while isinstance(parents.get(string), + (ast.FormattedValue, ast.JoinedStr, ast.BinOp)): + string = parents[string] + found.append((string.lineno, as_text(string).split("\n", 1)[0])) + return found + + +def unguarded(source: str) -> list[int]: + """The line of each remedy naming `_switch_line` whose prose does not + end with `BRANCH_LEFT`.""" + return [line for line, prose in switch_remedies(source) + if not prose.endswith("{BRANCH_LEFT}")] + + +def test_every_remedy_that_names_a_switch_line_ends_with_the_sentence(): + """THE GUARD (S2). The validator's three remedies that print a `git + switch -c` (`member-gitlink-absent`, `member-duplicate-id` and + `member-gitlink-unnamed`) end their prose with `BRANCH_LEFT`, and so + does any remedy added later: the one that builds it cannot forget, as + #610's `_lockstep` does, without this failing.""" + source = TEMPLATE.read_text(encoding="utf-8") + assert len(switch_remedies(source)) >= 3, switch_remedies(source) + assert unguarded(source) == [] + + +def fourth_remedy(end: str) -> str: + """A helper that returns a remedy naming `_switch_line`, as #610's + `_lockstep` does, its prose ending with `end`.""" + return ("\n\ndef _lockstep_remedy(root):\n return (\n" + " f\"(`{_switch_line(root, 'bump', None)}` makes one); \"\n" + " f\"moving one alone is what this refuses." + + end + "\")\n") + + +def test_the_guard_sees_a_remedy_left_without_the_sentence(): + """The guard is not vacuous: the validator's source with the sentence + taken out of any one of its remedies fails it, and so does a fourth + remedy, built by a helper as `_lockstep` builds its, until the sentence + ends it.""" + source = TEMPLATE.read_text(encoding="utf-8") + placed = [m.start() for m in re.finditer(r"\{BRANCH_LEFT\}", source)] + assert len(placed) >= 3, placed + for at in placed: + dropped = source[:at] + source[at + len("{BRANCH_LEFT}"):] + assert len(unguarded(dropped)) == 1, source[at - 60:at] + assert len(unguarded(source + fourth_remedy(""))) == 1 + assert unguarded(source + fourth_remedy(" {BRANCH_LEFT}")) == [] + + def test_the_two_tools_say_the_same_words(monkeypatch): """ONE ANSWER: `family.py` and the holder's validator, which a holder carries as a copy and which cannot import `family.py`, spell the @@ -247,9 +455,11 @@ def test_the_two_tools_say_the_same_words(monkeypatch): # --- one placeholder for the member (L2-45) ---------------------------------- #: The `switch -c` and the first `add` of each doc's block, each read for the -#: placeholder it puts where the member goes. +#: placeholder it puts where the member goes. The docs say `` where a +#: root goes, so a word with no space in it; what `init` prints says a real +#: root, which may hold one, so it is read with `host_words` instead. BRANCH_WORD = re.compile(r" switch -c family/add-(\S+)") -MEMBER_WORD = re.compile(r" add +--family-root \S+ --member \S*/(\S+)") +MEMBER_WORD = re.compile(r" add +--family-root --member \S*/(\S+)") @pytest.mark.parametrize("doc", [ @@ -269,9 +479,12 @@ def test_init_next_steps_spell_the_branch_as_its_member(capsys, tmp_path): work = tmp_path / "Orbit" / "Orbit" family_module()._print_next_steps( SimpleNamespace(org=ORG, tracking_branch=TRACKING), work, None) - printed = capsys.readouterr().out - assert BRANCH_WORD.search(printed).group(1) == "", printed - assert MEMBER_WORD.search(printed).group(1) == "", printed + block = [line.strip() for line in + capsys.readouterr().out.split("NEXT STEPS", 1)[1].splitlines() + if line.strip()] + switch, add = host_words(block[0]), host_words(block[1]) + assert switch[-1] == "family/add-", block + assert add[add.index("--member") + 1] == f"{ORG}/", block def test_the_branch_filled_in_as_written_is_one_add_commits_on(family, diff --git a/tests/test_validate_family_branch_step.py b/tests/test_validate_family_branch_step.py index 5bb60319..2b611edf 100644 --- a/tests/test_validate_family_branch_step.py +++ b/tests/test_validate_family_branch_step.py @@ -51,9 +51,11 @@ #: where that branch exists already (#607), pinned as text, as `family.py` #: and the holder's validator both say it. BRANCH_LEFT = ("If `git switch -c` says that branch already exists, an " - "earlier act left it: the same command without `-c` switches " - "to it and keeps every commit on it, or give the branch " - "another name.") + "earlier act left it: give the branch another name, which " + "starts it where the holder stands, or, only if that branch " + "holds work not merged yet, the same command with `switch` " + "in place of `switch -c` goes back to it and keeps every " + "commit on it.") def gate_module(monkeypatch): From a2776959b8bc0e57cd6e6712665626577558c417 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Fri, 16 Oct 2026 00:51:37 +0000 Subject: [PATCH 3/3] family.py, validate-family.py and test_family_switch_branch_exists.py: the comments say another name "works in every state", not that it is right in every one, and the guard's and the state-(d) test's docstrings say only what each does, so none of them claims more than it shows (#607) A second fix round on #637, from the re-verify rv607b (APPROVE WITH NITS: 0 blockers, 0 should, 5 nits). Comments and docstrings only; no behaviour changes, and `BRANCH_LEFT` is word for word the same in the three places. 1. Nit 1: the comment above `BRANCH_LEFT` in scripts/family.py and the one in the pinned templates/family-root/scripts/validate-family.py said a new branch starts where the holder stands now, "which is right in every state". In the state where the old branch holds work not merged yet, another name leaves that work behind. Both now say "which works in every state", and the two comments still read the same where they are the same. 2. Nit 3: the docstring of `switch_remedies` in tests/test_family_switch_branch_exists.py said the guard finds the call "however it is passed on", which a switch line moved into a variable would defeat. It now says "wherever the call stands inside the string expression that is passed on". 3. Nit 5, the docstring half: the docstring of `test_another_name_works_where_main_has_moved_since` said going back to the old branch conflicts, which the test does not assert. It now says the test shows the other name works, and that the conflict on the old branch is shown by rv607's probe and decision 2's table in the PR body. No assertion is added. 4. Nit 4 is in the PR body draft, outside this commit: the fix-round line for N4 now quotes decision 1 as written, and a "Fix round 2 (rv607b)" note says what this round did. Nit 2, the guard's control counts, is left as written: each case fails loudly with a clear message. Lane: openreposhape-5 (openRepoShape-5) Co-Authored-By: Claude Sonnet 5.5 --- scripts/family.py | 2 +- templates/family-root/scripts/validate-family.py | 2 +- tests/test_family_switch_branch_exists.py | 13 ++++++++----- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/scripts/family.py b/scripts/family.py index 9c3c6d05..f5b87d6b 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -708,7 +708,7 @@ def _switch_command(root: Path, branch: str) -> str: #: What every remedy that names `_switch_command` says next (#607): where an #: earlier act left that branch, `switch -c` fails `fatal: a branch named #: '' already exists` (exit 128). ANOTHER NAME COMES FIRST, because a -#: new branch starts where the holder stands now, which is right in every +#: new branch starts where the holder stands now, which works in every #: state. Going back to the old branch is right only where it holds work not #: merged yet: one left behind a main that has moved on makes the act collide #: with what main took in since, or find its member already there, and a diff --git a/templates/family-root/scripts/validate-family.py b/templates/family-root/scripts/validate-family.py index 44ef5052..433a6627 100755 --- a/templates/family-root/scripts/validate-family.py +++ b/templates/family-root/scripts/validate-family.py @@ -123,7 +123,7 @@ #: What a remedy that names `_switch_line` says at the end of its prose #: (#607): where an earlier act left that branch, `switch -c` fails `fatal: a #: branch named '' already exists` (exit 128). Another name comes -#: first: it starts the branch where the holder stands now, which is right in +#: first: it starts the branch where the holder stands now, which works in #: every state. The same command with `switch` for `switch -c` goes back to #: the old branch, which is right only where it holds work not merged yet; #: it keeps its commits, and nothing here names `switch -C`, which resets it. diff --git a/tests/test_family_switch_branch_exists.py b/tests/test_family_switch_branch_exists.py index eaa8fc35..e6490ddc 100644 --- a/tests/test_family_switch_branch_exists.py +++ b/tests/test_family_switch_branch_exists.py @@ -243,9 +243,11 @@ def test_another_name_works_where_main_has_moved_since(family, on_main, squash-merged since. The refusal's `switch -c`, typed as printed, fails; the same line with another name starts the branch at the `main` the holder stands on, `add` goes through there, and what it commits merges - into `main` with no conflict. (Going back to the old branch is the - other way, and it conflicts: the sentence puts it second, for a branch - that holds work not merged yet.)""" + into `main` with no conflict. That is what this test shows: the other + name works. Going back to the old branch is the other way, and it + conflicts here, but this test does not assert that; rv607's probe and + decision 2's table in the PR body show it. The sentence puts that way + second, for a branch that holds work not merged yet.""" holder = on_main branch = suggested(family, "add") git("branch", branch, cwd=holder) @@ -383,8 +385,9 @@ def switch_remedies(source: str) -> list[tuple[int, str]]: """Each place `source` names `_switch_line`: its line and the PROSE of the string it stands in, as text up to the first line break (the command lines come under it). The string is the whole expression the - call is written in, however it is passed on: the f-string of a - `Refusal` or a `report.finding`, or the `return` of a helper.""" + call is written in: wherever the call stands inside the string + expression that is passed on, as the f-string of a `Refusal` or a + `report.finding`, or as the `return` of a helper.""" tree = ast.parse(source) parents = {child: node for node in ast.walk(tree) for child in ast.iter_child_nodes(node)}