diff --git a/AGENTS.md b/AGENTS.md index ded190cb..07f60556 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 2f79cf68..607b7a8b 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -710,21 +710,44 @@ 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). ANOTHER NAME COMES FIRST, because a +#: 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 +#: 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: 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: """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", @@ -743,7 +766,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: @@ -755,7 +779,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}"] # --------------------------------------------------------------------------- @@ -1862,7 +1887,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 @@ -1888,7 +1915,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 05125658..e0150475 100644 --- a/templates/family-root/README.md +++ b/templates/family-root/README.md @@ -118,7 +118,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..433a6627 100755 --- a/templates/family-root/scripts/validate-family.py +++ b/templates/family-root/scripts/validate-family.py @@ -120,6 +120,23 @@ #: 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). Another name comes +#: 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. +#: 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: 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: """`git -C switch -c family/-`: the branch @@ -129,7 +146,9 @@ 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); 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}") @@ -401,7 +420,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 +803,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 +1141,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..e6490ddc --- /dev/null +++ b/tests/test_family_switch_branch_exists.py @@ -0,0 +1,505 @@ +# 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: +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 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 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 +`--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 ast +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 + 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 + 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, 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 +#: 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 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"] + + +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 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 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: + refused_as_left(holder, switch, branch) + 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 + + +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 +@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 + 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 + + 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 + + +# --- 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. 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) + 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 +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 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: 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)} + 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 + 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. 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 --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) + 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, + 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..2b611edf 100644 --- a/tests/test_validate_family_branch_step.py +++ b/tests/test_validate_family_branch_step.py @@ -47,6 +47,16 @@ #: 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: 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): """The template `validate-family.py`, loaded as a module. It puts its @@ -143,7 +153,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: