From 3f477476c9f21932d78b2eba899d9e71db3684e9 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Mon, 12 Oct 2026 23:44:23 +0000 Subject: [PATCH 1/6] family.py: `init` refuses to land inside a checkout of openRepoShape unless `--into` or `--work-dir` names the place, and its NEXT STEPS name this file from where `init` ran, so AGENTS.md's own command no longer puts the holder inside the standard and the next command it prints no longer fails `can't open file` (#488) AGENTS.md spelled the command `python3 scripts/family.py init --org --family `, a path that runs from the standard's root, and `--into` defaults to where the person stands. Followed exactly it printed `landing /Orbit/Orbit` with no word, and `make siblings` would then clone every member into the standard's checkout too. The NEXT STEPS block printed `python3 family.py add ...`, built from `Path(__file__).name`, which from the root fails `can't open file`, exit 2. - `_landing`: with neither `--into` nor `--work-dir`, a landing inside a checkout of openRepoShape (a directory holding `scripts/family.py` beside `templates/family-root/`, found in the landing's ancestry, so another clone or an install counts too) is refused, `family-landing-in-standard`, before anything is created and under `--dry-run` alike. The remedy names `--into ..` from the root, and from deeper inside the relative path that reaches the checkout's parent. An explicit `--into` or `--work-dir` is not second-guessed. `family-holder-already-here` is still asked first. - `_family_script`: NEXT STEPS and the `family-holder-already-here` remedy name this file relative to the directory `init` ran in, `scripts/family.py` from the root as every other place spells it, absolute where no relative path exists. - AGENTS.md's init command passes `--into ..`, as README's already does, and step 1 says why (line-neutral: step 1 is one line shorter). `--into --help` says it; docs/cli.md regenerated. - tests/test_family_init_landing.py: the old command from the root is refused (a dry run, so nothing can land in the checkout); a real run in a fake checkout creates nothing; the remedy from a subdirectory; an explicit `--into .` or `--work-dir .` passes; outside a checkout the default is unchanged; AGENTS.md's line as written lands beside the checkout; and the printed NEXT, from the root and from elsewhere, names a path that runs. Lane: openreposhape-5 (openRepoShape-5) Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 20 +-- docs/cli.md | 8 +- scripts/family.py | 115 ++++++++++++++--- tests/test_family_init_landing.py | 198 ++++++++++++++++++++++++++++++ 4 files changed, 314 insertions(+), 27 deletions(-) create mode 100644 tests/test_family_init_landing.py diff --git a/AGENTS.md b/AGENTS.md index 31fee71f..2fee4df4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -359,22 +359,22 @@ nothing. Use one when the parts DEPLOY SEPARATELY; one project is what parts that ship together already are. ```sh -python3 scripts/family.py init --org --family [--reuse-empty-repo] +python3 scripts/family.py init --org --family --into .. [--reuse-empty-repo] python3 scripts/family.py add --family-root --member / python3 scripts/family.py bump --family-root --member --to python3 scripts/family.py siblings --family-root ``` -1. **`init` creates a repository, so the human must say so first.** There is - no prompt here — `family.py` is not `setup.sh` — which makes it your job - to get an explicit yes before you run it, and to run `--dry-run` first and - show them the plan. If `/` already exists and is EMPTY, add - `--reuse-empty-repo`; if it has commits, stop and ask. +1. **`init` creates a repository, so the human must say so first.** No + prompt asks — `family.py` is not `setup.sh` — so get an explicit yes, and + run `--dry-run` first and show them the plan. If `/` exists and + is EMPTY, add `--reuse-empty-repo`; if it has commits, stop and ask. **It lands at `//`** — `--into` is the PARENT - directory and defaults to where you are standing, the folder `/` - is created in it, and the holder goes inside that. Standing in the family - folder already, it does not nest a second one. `--work-dir ` is the - override (`/`, no folder) and the two are refused together. + directory, the folder `/` is made in it (not again when you stand + in it already) and the holder goes inside that. `--into` defaults to where + you stand, refused inside a checkout of this standard, hence `--into ..` + above (`family-landing-in-standard`). `--work-dir ` is the override + (`/`, no folder) and the two are refused together. Say the landing path back to the human: the doubled `/` is deliberate and reads like a mistake until somebody explains it. 2. **A family pins ASSEMBLY ROOTS, never legs.** `--member /`, diff --git a/docs/cli.md b/docs/cli.md index 79cdbec8..8d315839 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -687,9 +687,11 @@ options: create a bare repository here instead of calling `gh repo create` (the TEST path; no network) --into DIR the PARENT directory the family folder is made in - (default: the directory you are standing in). The - holder lands at //, beside where - `make siblings` clones the members. + (default: the directory you are standing in, which is + refused inside a checkout of openRepoShape: from its + root, pass `--into ..`). The holder lands at + //, beside where `make siblings` + clones the members. --work-dir WORK_DIR THE OVERRIDE: land the holder at / and make no family folder at all. Mutually exclusive with --into. diff --git a/scripts/family.py b/scripts/family.py index c177d2bf..33eb7eed 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -715,6 +715,85 @@ def _local_remote_is_empty(bare: Path) -> bool | None: "siblings.py") +def _family_script() -> str: + """This file as the person who ran it names it from where they stand + (#488): the path its NEXT lines run it by. + + `init`'s NEXT STEPS printed `Path(__file__).name`, `family.py`, which + runs only from `scripts/`. AGENTS.md runs `init` from the standard's + root, where that line failed `can't open file`, exit 2. So the path is + RELATIVE TO THE DIRECTORY THE COMMAND WAS RUN FROM: from the root it is + `scripts/family.py`, as `setup-project.py`, `validate-family.py`, + AGENTS.md and README spell it, and from anywhere else it is the path + that reaches this file from there. It is absolute where no relative path + exists, another drive on Windows, or where that directory is gone. + """ + here = Path(__file__).resolve() + try: + return os.path.relpath(here, Path.cwd()) + except (ValueError, OSError): + return str(here) + + +def _standard_checkout(start: Path) -> Path | None: + """The checkout of openRepoShape that `start` is, or lies inside, or + None (#488). + + A checkout is a directory holding `scripts/family.py` beside + `templates/family-root/`, which is what this file needs of its own + (`SIBLINGS_SCRIPT`, `FAMILY_TEMPLATE`), so it is found whatever the + directory is called and whichever copy of the standard is asked: this + one, another clone, an install. `os.path`'s tests, which answer False + for a directory this user cannot search rather than raising. + """ + for directory in (start, *start.parents): + if os.path.isfile(directory / "scripts" / "family.py") and \ + os.path.isdir(directory / "templates" / "family-root"): + return directory + return None + + +def _refuse_landing_in_standard(into: Path, holder: Path, + family: str) -> None: + """`family-landing-in-standard`: with neither `--into` nor `--work-dir`, + `init` refuses to land inside a checkout of openRepoShape (#488). + + AGENTS.md spelled the command `python3 scripts/family.py init ...`, a + path that runs from the standard's root, and `--into` defaults to where + the person stands, so the command followed exactly printed `landing + //` with no word, and `make siblings` would + then have cloned every member into the standard's tree too. README's + example passes `--into ..`, and so does AGENTS.md's now; this is the + refusal for the run that does not. An explicit `--into` or `--work-dir` + is somebody's choice of a place and is not second-guessed. + + THE REMEDY NAMES `--into ..` FROM THE ROOT, and from deeper inside the + checkout the relative path that reaches its parent from there, so the + printed flag lands the family beside the checkout wherever the person + stands. Asked before anything is created, under `--dry-run` alike. + """ + checkout = _standard_checkout(into) + if checkout is None: + return + beside = checkout.parent + try: + parent = os.path.relpath(beside, into) + except ValueError: # another drive on Windows + parent = str(beside) + raise Refusal( + "family-landing-in-standard", + f"with neither --into nor --work-dir the holder lands where you are " + f"standing, at {holder}, which is inside {checkout}, a checkout of " + "openRepoShape itself (it holds scripts/family.py and " + "templates/family-root/). The family would be part of the " + "standard's tree, and `make siblings` would clone every member into " + "it too. Nothing was created, and `--dry-run` refuses the same.", + f"Remediation: pass `--into {parent}` to land it beside this " + f"checkout, at {beside / family / family}, or `--into ` for any " + "other parent directory; `--work-dir ` is the override " + "(`/`, no family folder). There is no --force.") + + def _landing(args, family: str) -> tuple[Path | None, Path]: """`(the family folder, the holder)` — WHERE `init` PUTS THINGS. @@ -735,22 +814,28 @@ def _landing(args, family: str) -> tuple[Path | None, Path]: `--work-dir` keeps today's semantics untouched (`/`, no folder made) because every test and every rehearsal in this suite passes it, and a flag that quietly changed meaning is worse than a second flag. + + THE DEFAULT IS REFUSED INSIDE A CHECKOUT OF THE STANDARD (#488, + `_refuse_landing_in_standard`), after the holder-already-here question: + a holder that exists there is the more specific answer. """ if args.work_dir is not None: return None, args.work_dir.expanduser().resolve() / family into = (args.into or Path.cwd()).expanduser().resolve() - if into.name == family: - if (into / family / MANIFEST).is_file(): - raise Refusal( - "family-holder-already-here", - f"{into} is already the {family} family folder and its holder " - f"is at {into / family}, so there is nothing to init", - "Remediation: `family.py add --family-root " - f"{into / family} --member /` grows it, and " - "`make siblings` in the holder puts each member's working " - "clone beside it. There is no --force.") - return into, into / family - return into / family, into / family / family + if into.name == family and (into / family / MANIFEST).is_file(): + raise Refusal( + "family-holder-already-here", + f"{into} is already the {family} family folder and its holder " + f"is at {into / family}, so there is nothing to init", + f"Remediation: `{PYTHON} {_family_script()} add --family-root " + f"{into / family} --member /` grows it, and " + "`make siblings` in the holder puts each member's working " + "clone beside it. There is no --force.") + folder, holder = ((into, into / family) if into.name == family else + (into / family, into / family / family)) + if args.into is None: + _refuse_landing_in_standard(into, holder, family) + return folder, holder def _checked_family_identity(args) -> tuple[str, str, str]: @@ -1049,7 +1134,7 @@ def _print_next_steps(args, work: Path, folder: Path | None) -> None: print(f""" NEXT STEPS - {PYTHON} {Path(__file__).name} add --family-root {work} \\ + {PYTHON} {_family_script()} add --family-root {work} \\ --member {args.org}/ cd {work} && make siblings @@ -4339,7 +4424,9 @@ def build_parser() -> argparse.ArgumentParser: "`gh repo create` (the TEST path; no network)") init.add_argument("--into", type=Path, default=None, metavar="DIR", help="the PARENT directory the family folder is made " - "in (default: the directory you are standing in). " + "in (default: the directory you are standing in, " + "which is refused inside a checkout of " + "openRepoShape: from its root, pass `--into ..`). " "The holder lands at //, " "beside where `make siblings` clones the members.") init.add_argument("--work-dir", type=Path, default=None, diff --git a/tests/test_family_init_landing.py b/tests/test_family_init_landing.py new file mode 100644 index 00000000..73c2c6be --- /dev/null +++ b/tests/test_family_init_landing.py @@ -0,0 +1,198 @@ +# SPDX-License-Identifier: Apache-2.0 +"""`family.py init` run as AGENTS.md spells it: where it lands, and the next +command it prints (#488). + +AGENTS.md spelled the command `python3 scripts/family.py init --org +--family `, a path that runs from the standard's root, and `--into` +defaults to where the person stands. Followed exactly, it printed `landing +/Orbit/Orbit` and would have made the family inside the checkout of +openRepoShape, where `make siblings` then clones every member too. README's +example passed `--into ..`; AGENTS.md's did not. And the NEXT STEPS block +printed `python3 family.py add ...`, built from `Path(__file__).name`, which +from the root fails `can't open file`, exit 2. + +So: with neither `--into` nor `--work-dir`, `init` refuses to land inside a +checkout of the standard (`family-landing-in-standard`) and names `--into ..`; +an explicit `--into` or `--work-dir` is somebody's choice and is not +second-guessed; AGENTS.md's command passes `--into ..`; and NEXT names this +file by the path that reaches it from where the person stood. + +A RUN THAT COULD LAND INSIDE THIS CHECKOUT IS ONLY EVER A `--dry-run`, which +creates nothing whatever the answer. The real runs stand in a FAKE checkout +under the test's own temporary directory (the two paths the detection reads, +and nothing else), so a regression writes there and never into the +repository under test. NO NETWORK: every remote is `--local-remote-dir`. +""" + +from __future__ import annotations + +import os +import re +import subprocess +import sys + +import pytest + +from conftest import REPO, run_script + +sys.path.insert(0, str(REPO / "scripts")) +from repo_shape import PYTHON # noqa: E402 + +FAMILY = REPO / "scripts" / "family.py" +ORG = "testorg" +NAME = "Orbit" +CODE = "family-landing-in-standard" + + +def init(cwd, tmp_path, *extra: str) -> subprocess.CompletedProcess: + """`init` as AGENTS.md spelled it before #488, with `extra` appended: + run from `cwd`, the remote a bare repository under `tmp_path`.""" + return run_script(FAMILY, "init", "--org", ORG, "--family", NAME, + "--created-by", "Test Human", + "--created-on", "2026-10-12", + "--local-remote-dir", str(tmp_path / "remotes"), + *extra, cwd=cwd) + + +def fake_checkout(path): + """A directory a checkout of openRepoShape is recognised by: a + `scripts/family.py` beside a `templates/family-root/`, and nothing else. + The tool under test is still this repository's own `family.py`.""" + (path / "scripts").mkdir(parents=True) + (path / "scripts" / "family.py").write_bytes(b"") + (path / "templates" / "family-root").mkdir(parents=True) + return path + + +def landing(stdout: str) -> str: + """The holder path the plan's `landing` line names.""" + found = re.search(r"^landing\s+(.+?)\s{3}", stdout, re.M) + assert found, stdout + return found.group(1) + + +def test_the_old_agents_md_command_from_the_root_is_refused(tmp_path): + """THE ISSUE'S ROW: from the standard's root, with no `--into`, the plan + used to say `landing /Orbit/Orbit` and exit 0. Now it is + refused before the plan, naming the checkout, the place it would have + landed and the flag that lands it beside the checkout.""" + result = init(REPO, tmp_path, "--dry-run") + assert result.returncode == 2, result.stdout + result.stderr + assert result.stdout == "" + holder = REPO.resolve() / NAME / NAME + # Searched for, not read off the first line: a checkout with uncommitted + # changes is warned about on stderr first, by `_shape_snapshot`. + assert (f"REFUSED {CODE}: with neither --into nor --work-dir the holder " + f"lands where you are standing, at {holder}, which is inside " + f"{REPO.resolve()}, a checkout of openRepoShape itself" + in result.stderr), result.stderr + assert ("Remediation: pass `--into ..` to land it beside this checkout, " + f"at {REPO.resolve().parent / NAME / NAME}, or `--into `" + in result.stderr), result.stderr + assert "Nothing was created, and `--dry-run` refuses the same." \ + in result.stderr + assert not (REPO / NAME).exists() + assert not (tmp_path / "remotes").exists() + + +def test_the_real_run_is_refused_before_anything_is_created(tmp_path): + """The same refusal without `--dry-run`, from a checkout's root: no + family folder, no holder and no remote.""" + checkout = fake_checkout(tmp_path / "std") + result = init(checkout, tmp_path) + assert result.returncode == 2, result.stdout + result.stderr + assert f"REFUSED {CODE}:" in result.stderr + assert "`--into ..`" in result.stderr + assert sorted(path.name for path in checkout.iterdir()) == [ + "scripts", "templates"] + assert not (tmp_path / "remotes").exists() + + +def test_deeper_inside_the_checkout_the_remedy_reaches_its_parent(tmp_path): + """`--into ..` from a subdirectory would land inside the checkout + still, so the remedy names the relative path that reaches the + checkout's parent from where the person stands.""" + checkout = fake_checkout(tmp_path / "std") + deeper = checkout / "docs" / "notes" + deeper.mkdir(parents=True) + result = init(deeper, tmp_path, "--dry-run") + assert result.returncode == 2, result.stdout + result.stderr + up = os.path.join("..", "..", "..") + assert f"Remediation: pass `--into {up}` to land it beside this " \ + "checkout" in result.stderr, result.stderr + assert f"at {tmp_path.resolve() / NAME / NAME}," in result.stderr + + +@pytest.mark.parametrize("flag", ["--into", "--work-dir"]) +def test_an_explicit_place_inside_the_checkout_is_not_second_guessed( + tmp_path, flag): + """`--into .` or `--work-dir .` names the place on purpose: the plan is + printed, as before, and the dry run exits 0.""" + checkout = fake_checkout(tmp_path / "std") + result = init(checkout, tmp_path, flag, ".", "--dry-run") + assert result.returncode == 0, result.stdout + result.stderr + assert CODE not in result.stderr + wanted = (checkout.resolve() / NAME / NAME if flag == "--into" else + checkout.resolve() / NAME) + assert landing(result.stdout) == str(wanted) + + +def test_outside_any_checkout_the_default_still_lands_where_you_stand( + tmp_path): + """The default is unchanged everywhere else: `/Orbit/Orbit`.""" + here = tmp_path / "projects" + here.mkdir() + result = init(here, tmp_path, "--dry-run") + assert result.returncode == 0, result.stdout + result.stderr + assert landing(result.stdout) == str(here.resolve() / NAME / NAME) + + +def test_agents_md_init_command_lands_beside_the_checkout(tmp_path): + """AGENTS.md's own line, as it is written now, run from the root: it + passes `--into ..`, so the family lands BESIDE the checkout. A dry run, + because the real one would write outside the test's directory.""" + agents = (REPO / "AGENTS.md").read_text(encoding="utf-8") + lines = [line for line in agents.splitlines() + if line.startswith("python3 scripts/family.py init ")] + assert len(lines) == 1, lines + words = lines[0].split() + assert words[words.index("--into") + 1] == "..", lines[0] + result = init(REPO, tmp_path, "--into", "..", "--dry-run") + assert result.returncode == 0, result.stdout + result.stderr + assert landing(result.stdout) == str(REPO.resolve().parent / NAME / NAME) + assert not (REPO.parent / NAME).exists() + + +def next_add_line(stdout: str) -> str: + """The NEXT STEPS `add` command, its continuation lines joined.""" + block = stdout.split("NEXT STEPS", 1)[1].replace("\\\n", " ") + lines = [line.strip() for line in block.splitlines() + if " add --family-root " in line] + assert len(lines) == 1, stdout + return " ".join(lines[0].split()) + + +@pytest.mark.parametrize("where", ["the standard's root", "elsewhere"]) +def test_the_printed_next_runs_from_where_init_was_run(tmp_path, where): + """THE SECOND HALF OF THE ISSUE: NEXT used to print `python3 family.py + add`, which no directory but `scripts/` can run. It names this file by + the path that reaches it from the directory `init` ran in: + `scripts/family.py` from the root, as every other place spells it, and + the relative path from anywhere else. That path, run from there, is + this repository's `family.py`.""" + cwd = REPO if where == "the standard's root" else tmp_path / "elsewhere" + cwd.mkdir(exist_ok=True) + result = init(cwd, tmp_path, "--into", str(tmp_path / "fam")) + assert result.returncode == 0, result.stdout + result.stderr + script = os.path.relpath(FAMILY.resolve(), cwd.resolve()) + if where == "the standard's root": + assert script == os.path.join("scripts", "family.py") + line = next_add_line(result.stdout) + assert line.startswith(f"{PYTHON} {script} add --family-root "), line + assert "family.py add" not in line.replace(script, ""), line + assert os.path.samefile(cwd / script, FAMILY) + helped = subprocess.run([sys.executable, script, "add", "--help"], + cwd=str(cwd), capture_output=True, text=True, + check=False) + assert helped.returncode == 0, helped.stderr + assert helped.stdout.startswith("usage: family.py add"), helped.stdout From 3e0777078232ec8d4334a9cc5ea3360b854c8758 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Mon, 12 Oct 2026 23:58:51 +0000 Subject: [PATCH 2/6] family.py and siblings.py: every command printed for pasting spells its paths through one quoter, `repo_shape.shell_quoted`, and the `member-git-dir-cached` remedy names the store on its own line before its quoted `rm -rf`, so under a folder with a space the remedy no longer deletes the wrong directory and `add`'s push line no longer fails `cannot change to` (#486) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `scripts/family.py` and a holder's `scripts/siblings.py` printed every path in their pasteable commands bare. Under a holder at `.../my work/Sonata/Sonata` the `member-git-dir-cached` remedy `rm -rf ` split into `rm -rf .../my`, an absolute directory that is NOT the store, and a relative `work/...`; `init`'s NEXT STEPS and `add`'s NEXT split `--family-root` and `git -C` (`fatal: cannot change to '.../my'`, exit 128); and the `mv` lines for a misnamed family folder pasted as four arguments. - `scripts/repo_shape.py` gains `shell_quoted` and `shell_line`, in a new section at its end: the doctor's `quote_arg` rule (#101/#102), the same two alphabets and the same two escapes, `sh`'s `'\''` and PowerShell's doubled apostrophe, chosen by `os.name` or a `platform` argument, leaving an ordinary word bare. It does not flatten to ASCII, as the doctor's report does, because a command that deletes or moves a directory must name that directory; so PowerShell's typographic single quotes are doubled too. It lives here because `siblings.py` is a holder's copy and this is the one library a holder carries beside it, as #456 put `spelled` here. The pinned `repo_shape.py` and `siblings.py` move in every project and holder; #456 already moved `repo_shape.py` in this release. - `family.py`: `init`'s NEXT STEPS print the `add` on ONE line (it pasted only into a POSIX shell, by its `\` continuation) and the `cd` with every path quoted, as does the `family-holder-already-here` remedy; `add`'s NEXT quotes `git -C `; `member-path-taken` quotes each spelling it names; and `member-git-dir-cached` and `remove`'s note about the same store print the directory alone on its own line, then `rm -rf` with its one argument quoted for the POSIX shell `rm -rf` is a command of, which the sentence names (Git Bash on Windows; PowerShell refuses `-rf` and deletes nothing). - `siblings.py`: the parent-folder warning's `mv` and `mkdir -p` lines go through `shell_line` for a POSIX shell, and the warning says which shell. - The doctor and `update-shape.py` keep their own quoters: the doctor's flattens to ASCII by its report's contract, and `update-shape.py`'s POSIX-only `shlex` lines are lane 3's open #374. Not an import change. - tests/test_family_printed_quoting.py: the quoter round-trips through `shlex.split` and through a PowerShell reader for spaces, apostrophes, `é`, a typographic apostrophe, shell syntax and the empty string, and agrees with the doctor's `quote_arg` on every ASCII value; and under a holder in `my work` and in `Dana's work`, `init`'s NEXT, `add`'s NEXT, the cached-store remedy and note, and the `mv` lines each split into the argv they mean, the `rm -rf` run as split deletes the store while the decoy `/my` survives, and the `mv` lines run as split move the holder. Three existing tests' expected lines follow the new wording. Lane: openreposhape-5 (openRepoShape-5) Co-Authored-By: Claude Opus 5.5 --- scripts/family.py | 79 +++-- scripts/repo_shape.py | 71 +++++ templates/family-root/scripts/siblings.py | 19 +- tests/test_family.py | 13 +- tests/test_family_add_excludes.py | 9 +- tests/test_family_add_refusals.py | 8 +- tests/test_family_printed_quoting.py | 334 ++++++++++++++++++++++ 7 files changed, 499 insertions(+), 34 deletions(-) create mode 100644 tests/test_family_printed_quoting.py diff --git a/scripts/family.py b/scripts/family.py index 33eb7eed..94d0dcd2 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -148,6 +148,9 @@ from shape_materialize import ( # noqa: E402 FAMILY_TEMPLATED, PLACEHOLDER_RE, render, ) +# How every command this prints for a person to paste spells its paths +# (#486), the quoting the holder's `scripts/siblings.py` uses too. +from repo_shape import shell_quoted # noqa: E402 NAMING_POLICY = SHAPE_ROOT / "contracts" / "repository-naming.yaml" MANIFEST = "family.yaml" @@ -735,6 +738,17 @@ def _family_script() -> str: return str(here) +def _add_command(holder: Path, org: str) -> str: + """The `add` that grows the holder at `holder`, as `init` prints it: + ONE line, so it pastes into PowerShell too, with this file's path, the + holder's and the organisation each spelled by `shell_quoted` for the + reader's shell (#486). A holder under `my work/` used to print + `--family-root .../my work/Orbit`, two arguments to argparse. + `` is left for the person to fill in.""" + return (f"{PYTHON} {shell_quoted(_family_script())} add --family-root " + f"{shell_quoted(holder)} --member {shell_quoted(org)}/") + + def _standard_checkout(start: Path) -> Path | None: """The checkout of openRepoShape that `start` is, or lies inside, or None (#488). @@ -827,10 +841,9 @@ def _landing(args, family: str) -> tuple[Path | None, Path]: "family-holder-already-here", f"{into} is already the {family} family folder and its holder " f"is at {into / family}, so there is nothing to init", - f"Remediation: `{PYTHON} {_family_script()} add --family-root " - f"{into / family} --member /` grows it, and " - "`make siblings` in the holder puts each member's working " - "clone beside it. There is no --force.") + f"Remediation: `{_add_command(into / family, args.org)}` grows " + "it, and `make siblings` in the holder puts each member's " + "working clone beside it. There is no --force.") folder, holder = ((into, into / family) if into.name == family else (into / family, into / family / family)) if args.into is None: @@ -1134,9 +1147,8 @@ def _print_next_steps(args, work: Path, folder: Path | None) -> None: print(f""" NEXT STEPS - {PYTHON} {_family_script()} add --family-root {work} \\ - --member {args.org}/ - cd {work} && make siblings + {_add_command(work, args.org)} + cd {shell_quoted(work)} && make siblings `add` mounts the member at {MEMBERS_DIR}/, records its pin, and writes ONE commit; land it as a pull request. `make siblings` then puts each @@ -1791,7 +1803,10 @@ def _checked_mount_path(root: Path, project: str, path: str) -> None: # gave `MEMBERS/Atlas`, and kept its file too), or both, while # `.gitmodules` and the manifest say `path`. variant = name[:len(path)] - spelled = " ".join(sorted({entry[0][:len(path)] for entry in held})) + spellings = sorted({entry[0][:len(path)] for entry in held}) + # QUOTED (#486), each one: it names an index entry somebody else + # made, and that is not held to a member's alphabet. + spelled = " ".join(shell_quoted(name) for name in spellings) raise Refusal( "member-path-taken", f"the family root's index holds `{name}` ({kind}){more}" @@ -1803,7 +1818,7 @@ def _checked_mount_path(root: Path, project: str, path: str) -> None: "Remediation: this is a half-finished add or a hand edit. `git rm " f"-r --cached -- {spelled}` takes it out of the index and touches " "no file; " - + ("then " if spelled == path else + + ("then " if spellings == [path] else "where HEAD holds it too, commit that removal first, since the " f"add's own commit names {path} alone: otherwise HEAD keeps " "the other spelling beside the member, or, with " @@ -1875,6 +1890,31 @@ def _checked_mount_path(root: Path, project: str, path: str) -> None: f"index's), or `git checkout HEAD -- {GITMODULES}`.")) +def _deletion(store: Path) -> list[str]: + """The lines that let a person delete an object store `git rm` left + behind: the directory alone on its own line, to be checked before + anything is pasted, then the one command that deletes it and nothing + else (#486). + + THE ONE DESTRUCTIVE COMMAND THIS FILE PRINTS, and it printed its path + bare: under a holder at `.../my work/Sonata/Sonata` a shell split `rm + -rf .../my work/Sonata/Sonata/.git/modules/members/Beacon` into `rm -rf + .../my`, an absolute directory that is NOT the store (a `Code` beside + the `Code Projects` the holder is in), and a second argument relative to + wherever the person stood. So its one argument is quoted, by + `repo_shape.shell_quoted`, for the POSIX shell `rm -rf` is a command of + (Git Bash is one on Windows; PowerShell refuses `-rf` and deletes + nothing), and the sentence says which shell. + + NAMED IN POSIX, as git spells `.git/modules/` and as the person + who retypes it does. + """ + shown = store.as_posix() + return ["It is this directory:", f" {shown}", + "and in a POSIX shell (Git Bash on Windows) this deletes it and " + "nothing else:", f" rm -rf {shell_quoted(shown, 'posix')}"] + + def _checked_add_target(family: Family, repository: str, project: str, path: str, at: str | None) -> str | None: """Every refusal `add` makes about the HOLDER and its own arguments, @@ -1925,9 +1965,10 @@ def _checked_add_target(family: Family, repository: str, project: str, "write over it", "Remediation: if nothing unpushed lives in it — and nothing does " "unless somebody committed inside the mount — delete it and " - f"re-run:\n rm -rf {cached.as_posix()}\nIt is left behind by " - "`git rm` on purpose, which is why this tool will not remove it " - "for you.") + "re-run. " + + "\n".join(_deletion(cached)) + + "\nIt is left behind by `git rm` on purpose, which is why this " + "tool will not remove it for you.") # The pin is written into a `members:` block `write_members` has to be # able to find; a manifest it would refuse is refused NOW, while there is # nothing to undo. @@ -2740,8 +2781,8 @@ def cmd_add(args) -> int: print(f" {project:<16} {commit[:12]} tree {digest[:12]}… " f"(project {project_id})") print(f" committed {head[:12]}: .gitmodules, {path}, {MANIFEST}") - print(f"\nNEXT git -C {family.root} push -u origin && open a " - "pull request") + print(f"\nNEXT git -C {shell_quoted(family.root)} push -u origin " + " && open a pull request") return 0 @@ -4212,10 +4253,12 @@ def _unmount_and_drop(family: Family, row: dict, path: str) -> int: # `.git/modules/`, and so does the human who deletes it. cached = family.root / ".git" / "modules" / path if cached.exists(): - print(f" NOTE {cached.as_posix()} still holds that member's object " - "store. `git rm` leaves it on purpose, in case somebody " - "committed inside the mount; delete it (`rm -rf`) before adding " - "this member back.") + # The directory alone on a line, then the command, quoted (#486), + # exactly as `add`'s `member-git-dir-cached` names the same store. + print(" NOTE that member's object store is left behind. `git rm` " + "leaves it on purpose, in case somebody committed inside the " + "mount; delete it before adding this member back. " + + "\n ".join(_deletion(cached))) return 0 diff --git a/scripts/repo_shape.py b/scripts/repo_shape.py index 2bcae52f..25a17653 100644 --- a/scripts/repo_shape.py +++ b/scripts/repo_shape.py @@ -30,6 +30,9 @@ - `spelled` and `escape_unencodable_stdout` — how both roots' `bootstrap.py` print a person's text: what a terminal must not be handed raw is escaped, and a stdout that cannot encode it never raises (#391). + - `shell_quoted` and `shell_line` — how `scripts/family.py` and a holder's + `scripts/siblings.py` spell a path in a command a person pastes, so that + it reaches the command as ONE argument in `sh` or PowerShell (#486). This file is COPIED into a scaffolded assembly root by `scaffold-project.py` and its sha256 is recorded in that project's `contracts/shape-pin.yaml`, so an @@ -2614,3 +2617,71 @@ def spelled(value: object) -> str: or unicodedata.category(char) in ESCAPED_CATEGORIES) else char for char in str(value)) + + +# --------------------------------------------------------------------------- +# How a command a person pastes spells a path (#486) +# --------------------------------------------------------------------------- +# +# `scripts/family.py` and a family holder's `scripts/siblings.py` print +# commands for a person to paste, and printed every path in them bare. A +# family folder whose path holds a space is ordinary ("My Projects" on macOS, +# a Windows profile directory), and under a holder at `.../my work/Sonata/ +# Sonata` the `rm -rf` of a cached object store split into `rm -rf .../my`, +# an absolute directory that is NOT the store, and a second argument relative +# to wherever the person stood. `siblings.py` is a copy every holder carries, +# and this file is the one library a holder carries beside it, so the quoting +# both use is written here, once. + +#: A word `sh` reads as itself, needing no quotes: letters, digits and the +#: punctuation a path, a flag, a commit or a branch name is spelled with. `~` +#: is absent, because a leading one is expanded, and so is every character +#: `sh` reads as syntax. `shape-doctor.py`'s `UNQUOTED_POSIX`, the same set. +SHELL_BARE_POSIX = re.compile(r"[A-Za-z0-9_./:@%+=,-]+") +#: PowerShell's, and NOT the same set: it gains the backslash a Windows path +#: is spelled with and loses `,`, PowerShell's list separator, and `@`, which +#: leading is splatting. `shape-doctor.py`'s `UNQUOTED_NT`, the same set. +SHELL_BARE_NT = re.compile(r"[A-Za-z0-9_./:%+=\\-]+") +#: What PowerShell takes for a single quote: the apostrophe and the four +#: typographic single quotes, which a folder name on macOS or Windows can +#: hold. Inside a verbatim string each is written by doubling it. +POWERSHELL_SINGLE_QUOTES = frozenset("'‘’‚‛") + + +def shell_quoted(value: object, platform: str | None = None) -> str: + """`value` as ONE word of a command a person pastes into the shell of + `platform` (`os.name` when None): bare where that shell reads every + character of it as itself, else whole, in single quotes (#486). + + THE DOCTOR'S RULE, `shape-doctor.py`'s `quote_arg`, which has quoted + every path its NEXT lines print since #101/#102: `sh` and PowerShell are + two dialects, and each reads the other's escape as a different value + with no error to say so. `sh` closes the quotation, escapes the + apostrophe and reopens it (`'O'\\''Brien'`), and `shlex.split` hands + that back as the value that went in; PowerShell doubles it inside a + verbatim string (`'O''Brien'`), where `\\` is an ordinary character. + What needs no quoting is left alone, so an ordinary path prints as it + always did. `platform` lets a test ask for either dialect on any host. + + ONE DIFFERENCE FROM THE DOCTOR, ON PURPOSE. Its report is pure ASCII, so + `quote_arg` flattens a value to ASCII before it quotes it, and a path + holding `é` is printed as some other path. A command that deletes or + moves a directory has to name THAT directory, so nothing here is + flattened. The typographic single quotes are the price: PowerShell reads + each of them as an apostrophe, so each is doubled too. On a value made + of ASCII the two functions give the same answer, which + `tests/test_family_printed_quoting.py` holds them to. + """ + text = str(value) + windows = (os.name if platform is None else platform) == "nt" + if (SHELL_BARE_NT if windows else SHELL_BARE_POSIX).fullmatch(text): + return text + if windows: + return "'" + "".join(char * 2 if char in POWERSHELL_SINGLE_QUOTES + else char for char in text) + "'" + return "'" + text.replace("'", "'\\''") + "'" + + +def shell_line(words, platform: str | None = None) -> str: + """`words` as one command line, each spelled by `shell_quoted` (#486).""" + return " ".join(shell_quoted(word, platform) for word in words) diff --git a/templates/family-root/scripts/siblings.py b/templates/family-root/scripts/siblings.py index 86398cf9..6d30756e 100755 --- a/templates/family-root/scripts/siblings.py +++ b/templates/family-root/scripts/siblings.py @@ -119,7 +119,7 @@ # arithmetic `resolve_relative` hands git's relative-url rule to. from repo_shape import ( # noqa: E402 PYTHON, Refusal, find_repo_root, join_remote, load_yaml, - remote_local_path, repo_basename, same_repository, + remote_local_path, repo_basename, same_repository, shell_line, ) # THE CREDENTIAL AND THE MEMBER ROWS ARE READ BY `bootstrap.py` ALREADY, and a # second definition of either is how the two start disagreeing about which @@ -758,7 +758,9 @@ def parent_folder_warning(root: Path, family: str) -> str | None: "shell, an editor", " and any LINKED WORKTREE (its `.git` file carries an absolute " "path), so it", - " is yours to run, when nothing is standing in it:", + " is yours to run, when nothing is standing in it, in a POSIX " + "shell (Git Bash", + " on Windows):", "", ] wanted = folder.parent / family @@ -767,12 +769,15 @@ def parent_folder_warning(root: Path, family: str) -> str | None: # `/` IS the holder itself and the move needs a # staging name — `mv A A/A` cannot be spelled in one command. staged = folder.parent / f"{family}.holder" - lines += [f" mv {root} {staged}", - f" mkdir -p {wanted}", - f" mv {staged} {wanted / family}"] + steps = [["mv", root, staged], ["mkdir", "-p", wanted], + ["mv", staged, wanted / family]] else: - lines += [f" mkdir -p {wanted}", - f" mv {root} {wanted / family}"] + steps = [["mkdir", "-p", wanted], ["mv", root, wanted / family]] + # EVERY PATH QUOTED (#486), by `repo_shape.shell_line`, the quoting + # `family.py` prints its commands with: under `my work/` a bare `mv` + # moved nothing and pasted as four arguments. For the POSIX shell `mv` + # and `mkdir -p` are commands of, as the sentence above says. + lines += [f" {shell_line(step, 'posix')}" for step in steps] lines += ["", " Then re-run `make siblings` from the moved holder; the " "members are", diff --git a/tests/test_family.py b/tests/test_family.py index 43ef3a41..d1f5b9fa 100644 --- a/tests/test_family.py +++ b/tests/test_family.py @@ -1820,11 +1820,14 @@ def test_siblings_warns_about_the_parent_folder_and_moves_nothing(family, assert "NOTHING WAS MOVED" in result.stderr assert "LINKED WORKTREE" in result.stderr # The exact commands, and the staging name the one-step `mv A A/A` cannot - # be spelled without. - assert f"mv {holder} {tmp_path / 'InkRouter.holder'}" in result.stderr - assert f"mkdir -p {tmp_path / 'InkRouter'}" in result.stderr - assert f"mv {tmp_path / 'InkRouter.holder'} " \ - f"{tmp_path / 'InkRouter' / 'InkRouter'}" in result.stderr + # be spelled without. Each is a POSIX shell's and quoted for one (#486): + # `shlex.join` is the standard library's spelling of that, which leaves + # these paths bare everywhere but in Windows' backslashes. + staged, wanted = tmp_path / "InkRouter.holder", tmp_path / "InkRouter" + assert shlex.join(["mv", str(holder), str(staged)]) in result.stderr + assert shlex.join(["mkdir", "-p", str(wanted)]) in result.stderr + assert shlex.join(["mv", str(staged), str(wanted / "InkRouter")]) \ + in result.stderr assert holder.is_dir(), "it moves nothing, and that is the whole point" assert (holder / "family.yaml").is_file(), ( "it moves nothing, and that is the whole point") diff --git a/tests/test_family_add_excludes.py b/tests/test_family_add_excludes.py index 4a29f666..d6737599 100644 --- a/tests/test_family_add_excludes.py +++ b/tests/test_family_add_excludes.py @@ -36,6 +36,7 @@ from __future__ import annotations import os +import shlex import subprocess import sys from pathlib import Path @@ -299,8 +300,12 @@ def test_a_cached_store_is_refused_before_the_mount_would_reactivate_it( f"object store of a previously removed {SPARE}, and `git submodule " "add` will not write over it", "Remediation: if nothing unpushed lives in it — and nothing does " - "unless somebody committed inside the mount — delete it and re-run:", - f" rm -rf {store.as_posix()}", + "unless somebody committed inside the mount — delete it and re-run. " + "It is this directory:", + f" {store.as_posix()}", + "and in a POSIX shell (Git Bash on Windows) this deletes it and " + "nothing else:", + f" rm -rf {shlex.quote(store.as_posix())}", "It is left behind by `git rm` on purpose, which is why this tool " "will not remove it for you.", ] diff --git a/tests/test_family_add_refusals.py b/tests/test_family_add_refusals.py index 41da0e6d..b0ef041f 100644 --- a/tests/test_family_add_refusals.py +++ b/tests/test_family_add_refusals.py @@ -563,8 +563,12 @@ def test_a_cached_store_in_a_linked_worktree_is_refused_where_git_keeps_it( f"object store of a previously removed {SPARE}, and `git submodule " "add` will not write over it", "Remediation: if nothing unpushed lives in it — and nothing does " - "unless somebody committed inside the mount — delete it and re-run:", - f" rm -rf {store.as_posix()}", + "unless somebody committed inside the mount — delete it and re-run. " + "It is this directory:", + f" {store.as_posix()}", + "and in a POSIX shell (Git Bash on Windows) this deletes it and " + "nothing else:", + f" rm -rf {shlex.quote(store.as_posix())}", "It is left behind by `git rm` on purpose, which is why this tool " "will not remove it for you.", ] diff --git a/tests/test_family_printed_quoting.py b/tests/test_family_printed_quoting.py new file mode 100644 index 00000000..829df939 --- /dev/null +++ b/tests/test_family_printed_quoting.py @@ -0,0 +1,334 @@ +# SPDX-License-Identifier: Apache-2.0 +"""Every command `scripts/family.py` and a holder's `scripts/siblings.py` +print for a person to paste spells its paths through ONE quoter, +`repo_shape.shell_quoted` (#486). + +They printed every path bare. A family folder whose path holds a space is +ordinary ("My Projects" on macOS, a Windows profile directory), and under a +holder at `.../my work/Sonata/Sonata`: + + * `add`'s `member-git-dir-cached` remedy, `rm -rf `, + split into `rm -rf .../my`, an absolute directory that is NOT the store, + and a second argument relative to wherever the person stood; + * `remove`'s note about the same store named it the same way; + * `init`'s NEXT STEPS and `add`'s NEXT split `--family-root` and `git -C` + in two (`fatal: cannot change to '.../my'`, exit 128); + * `siblings.py`'s `mv` lines for a misnamed family folder pasted as four + arguments and moved nothing. + +THE PROOF IS THE SHELL'S OWN READING. Each printed command, under a holder +whose path holds a space (and, in the second run, an apostrophe), is split +the way the reader's shell splits it, and must come back as the argv it was +meant to be: `shlex.split` for a POSIX shell, and `powershell_words` below +for PowerShell, the dialect `shell_quoted` speaks on Windows. A command for +a POSIX shell only (`rm -rf`, `mv`, `mkdir -p`) is quoted for one on every +host and split with `shlex` on every host. Where a command is safe to run in +a test, it is RUN as split: the `rm -rf` deletes the store and the decoy +directory a bare split would have named survives; the `mv` lines move the +holder where the warning said. + +THE SAME FAMILY AS `test_family.py`: members scaffolded into local bare +repositories, no network, and a holder COPIED into the awkward folder. +""" + +from __future__ import annotations + +import importlib.util +import os +import shlex +import shutil +import subprocess +import sys +from pathlib import Path + +import pytest + +from conftest import REPO, git, run_script +#: The family fixture and its names, imported from the module that owns them. +from test_family import ( # noqa: F401 + FAMILY, NAME, ORG, SPARE, family, manifest, validate, +) + +sys.path.insert(0, str(REPO / "scripts")) +from repo_shape import PYTHON, shell_line, shell_quoted # noqa: E402 + +#: The two folders a holder is copied into: a space, and a space beside an +#: apostrophe, which is where `sh` and PowerShell escape differently. +FOLDERS = ["my work", "Dana's work"] + +#: The characters PowerShell reads as a single quote. +PS_QUOTES = "'\u2018\u2019\u201a\u201b" + + +def powershell_words(line: str) -> list[str]: + """PowerShell's reading of a command line, as far as these commands + need it: whitespace-separated words, where a word may be or hold a + verbatim `'...'` string, inside which any of PowerShell's five single + quotes doubled is one of it and nothing else is special. That is the + only quoting `shell_quoted` emits for PowerShell; a test that split on + whitespace would tear a quoted path with a space in it in half.""" + words: list[str] = [] + index, length = 0, len(line) + while index < length: + if line[index].isspace(): + index += 1 + continue + word: list[str] = [] + while index < length and not line[index].isspace(): + if line[index] in PS_QUOTES: + index += 1 + while index < length: + if line[index] not in PS_QUOTES: + word.append(line[index]) + index += 1 + elif index + 1 < length and line[index + 1] in PS_QUOTES: + word.append(line[index + 1]) + index += 2 + else: + index += 1 + break + continue + word.append(line[index]) + index += 1 + words.append("".join(word)) + return words + + +def host_words(line: str) -> list[str]: + """`line` as the shell `shell_quoted` speaks on THIS host reads it.""" + return powershell_words(line) if os.name == "nt" else shlex.split(line) + + +# --- the quoter --------------------------------------------------------------- + +#: What must survive UNQUOTED, on both platforms: a line whose whole job is +#: to be read should not carry quotes no shell needs. +ORDINARY = ["/srv/work/Atlas", "scripts/family.py", "--family-root", + "members/IRRS", "family/add-irrs", + "3d46ab5b9c1dcafe0123456789abcdef01234567", "origin", "-C"] + +#: Values each shell must be handed whole: a space, an apostrophe, both, a +#: Windows path with both, a name no ASCII flattening may touch, the +#: typographic apostrophe a macOS folder name carries, shell syntax, and the +#: empty string. +AWKWARD = [ + "/srv/my projects/Atlas", + "/srv/O'Brien/Atlas", + "/srv/Code Projects/Sonata/Sonata/.git/modules/members/Beacon", + r"C:\Users\Jane O'Neill\proj", + "/srv/José/Atlas", + "/srv/Dana\u2019s work/Atlas", + "$HOME/a;b`c`|d&e", + "~/x", "a,b", "@a", "", +] + + +@pytest.mark.parametrize("platform", ["posix", "nt"]) +@pytest.mark.parametrize("value", ORDINARY) +def test_an_ordinary_word_is_left_alone(platform, value): + assert shell_quoted(value, platform) == value + + +@pytest.mark.parametrize("value", AWKWARD) +def test_the_posix_form_is_one_word_to_sh(value): + quoted = shell_quoted(value, "posix") + assert shlex.split(quoted) == [value], quoted + + +@pytest.mark.parametrize("value", AWKWARD) +def test_the_powershell_form_is_one_word_to_powershell(value): + quoted = shell_quoted(value, "nt") + assert powershell_words(quoted) == [value], quoted + + +def test_the_two_dialects_escape_an_apostrophe_differently(): + """Each reads the other's escape as a different value: `sh` reads + `'O''Brien'` as `OBrien`. So the dialect is the platform's.""" + assert shell_quoted("O'Brien", "posix") == "'O'\\''Brien'" + assert shell_quoted("O'Brien", "nt") == "'O''Brien'" + assert shlex.split(shell_quoted("O'Brien", "nt")) == ["OBrien"] + here = "nt" if os.name == "nt" else "posix" + assert shell_quoted("O'Brien") == shell_quoted("O'Brien", here) + + +def test_nothing_is_flattened_and_a_typographic_apostrophe_is_doubled(): + """A command that deletes a directory must name THAT directory: `é` + stays `é`, where the doctor's ASCII report would flatten it. PowerShell + reads `’` as an apostrophe, so its form doubles it.""" + assert shell_quoted("/srv/José", "posix") == "'/srv/José'" + assert shell_quoted("Dana\u2019s", "nt") == "'Dana\u2019\u2019s'" + assert shell_quoted("Dana\u2019s", "posix") == "'Dana\u2019s'" + + +def test_a_line_is_its_words_each_quoted(): + words = ["git", "-C", "/srv/my work/Sonata", "push", "-u", "origin", + "family/add-irrs"] + assert shlex.split(shell_line(words, "posix")) == words + assert powershell_words(shell_line(words, "nt")) == words + + +@pytest.fixture(scope="module") +def doctor(): + spec = importlib.util.spec_from_file_location( + "doctor_for_quoting", REPO / "shape-doctor.py") + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +@pytest.mark.parametrize("platform", ["posix", "nt"]) +@pytest.mark.parametrize("value", ORDINARY + [ + value for value in AWKWARD if value.isascii()] + [r"C:\work,old\Atlas"]) +def test_it_is_the_doctors_rule_on_any_ascii_value(doctor, platform, value): + """ONE RULE IN TWO PLACES, held together here. `shape-doctor.py`'s + `quote_arg` flattens its value to ASCII first, because its report is + pure ASCII, and is otherwise this function: the same two alphabets and + the same two escapes. On an ASCII value the two must agree.""" + assert shell_quoted(value, platform) == doctor.quote_arg(value, platform) + + +# --- the commands, under a folder with a space in it -------------------------- + +def copied_holder(family, base: Path) -> Path: + """A private copy of the family's holder at `/InkRouter`.""" + base.mkdir(parents=True, exist_ok=True) + holder = base / NAME + shutil.copytree(family["root"], holder, symlinks=True) + return holder.resolve() + + +@pytest.mark.parametrize("folder", FOLDERS) +def test_init_next_steps_paste_whole(tmp_path, folder): + """`add --family-root ` and `cd `, each one line, each + path one word.""" + into = tmp_path / folder + into.mkdir() + result = run_script(FAMILY, "init", "--org", ORG, "--family", "Orbit", + "--created-by", "Test Human", + "--created-on", "2026-10-12", + "--local-remote-dir", str(tmp_path / "remotes"), + "--into", str(into), cwd=tmp_path) + assert result.returncode == 0, result.stdout + result.stderr + holder = into.resolve() / "Orbit" / "Orbit" + block = result.stdout.split("NEXT STEPS", 1)[1].splitlines() + add = next(line for line in block if " add --family-root " in line) + script = os.path.relpath(FAMILY.resolve(), tmp_path.resolve()) + assert host_words(add) == [PYTHON, script, "add", "--family-root", + str(holder), "--member", f"{ORG}/"] + cd = next(line for line in block if line.strip().startswith("cd ")) + assert host_words(cd) == ["cd", str(holder), "&&", "make", "siblings"] + + +@pytest.mark.parametrize("folder", FOLDERS) +def test_add_next_names_the_holder_whole(family, tmp_path, folder): + """`git -C push ...`: the holder is one word, and that word is + the holder, as git itself says when it is handed it.""" + holder = copied_holder(family, tmp_path / folder) + result = run_script(FAMILY, "add", "--family-root", str(holder), + "--member", f"{ORG}/{SPARE}", + "--local-remote-dir", str(family["remotes"])) + assert result.returncode == 0, result.stdout + result.stderr + line = next(line for line in result.stdout.splitlines() + if line.startswith("NEXT ")) + words = host_words(line[len("NEXT "):]) + assert words[:4] == ["git", "-C", str(holder), "push"], line + toplevel = subprocess.run(["git", "-C", words[2], "rev-parse", + "--show-toplevel"], capture_output=True, + text=True, check=True).stdout.strip() + assert os.path.samefile(toplevel, holder) + + +def deletion(lines: list[str], store: Path) -> list[str]: + """The directory line and the command line after it, checked: the + directory alone, as git spells it, then `rm -rf` and that directory as + ONE word to a POSIX shell. Returns the command's argv.""" + shown = store.as_posix() + at = lines.index(f" {shown}") + assert "POSIX shell" in lines[at + 1], lines + command = lines[at + 2].strip() + argv = shlex.split(command) + assert argv == ["rm", "-rf", shown], command + return argv + + +@pytest.mark.parametrize("folder", FOLDERS) +def test_the_cached_store_is_named_and_deleted_whole(family, tmp_path, + folder): + """THE P1 ROW. `remove` leaves the member's object store behind and its + note names it; `add` of the same member is refused over it and its + remedy names it. Each prints the directory alone on its own line, then + the `rm -rf` that deletes it, its one argument quoted. Run as split, the + command deletes the store and nothing else: the decoy a bare split would + have deleted (`/my`, for `/my work/...`) is still there, and + the add then goes through.""" + decoy = tmp_path / folder.split(" ")[0] + decoy.mkdir() + (decoy / "keep.txt").write_text("not the store\n", encoding="utf-8") + holder = copied_holder(family, tmp_path / folder) + store = holder / ".git" / "modules" / "members" / "IRSS" + + removed = run_script(FAMILY, "remove", "--family-root", str(holder), + "--member", "IRSS") + assert removed.returncode == 0, removed.stdout + removed.stderr + assert store.is_dir() + note = [line[2:] for line in removed.stdout.splitlines()] + deletion(note, store) + + again = run_script(FAMILY, "add", "--family-root", str(holder), + "--member", f"{ORG}/IRSS", + "--local-remote-dir", str(family["remotes"])) + assert again.returncode == 2, again.stdout + again.stderr + assert "REFUSED member-git-dir-cached:" in again.stderr + argv = deletion(again.stderr.splitlines(), store) + + if os.name == "nt": + # `rm` is no program of Windows' own; the path is what the command + # was split into, and that is what is deleted. + shutil.rmtree(argv[2], onerror=lambda f, p, _e: ( + os.chmod(p, 0o700), f(p))) + else: + subprocess.run(argv, check=True) + assert not store.exists() + assert (decoy / "keep.txt").read_text(encoding="utf-8") == \ + "not the store\n" + assert (holder / "family.yaml").is_file() + back = run_script(FAMILY, "add", "--family-root", str(holder), + "--member", f"{ORG}/IRSS", + "--local-remote-dir", str(family["remotes"])) + assert back.returncode == 0, back.stdout + back.stderr + assert validate(holder).returncode == 0 + + +@pytest.mark.parametrize("folder", FOLDERS) +def test_the_siblings_mv_lines_paste_whole_and_move_the_holder(family, + tmp_path, + folder): + """`siblings.py`'s parent-folder warning: the holder sits in a folder + not named after the family, and the warning prints the `mv` and `mkdir + -p` that put it right and moves nothing. Each line is one command to a + POSIX shell with every path one word, and run in order, as split, they + move the holder to `/InkRouter/InkRouter`.""" + holder = copied_holder(family, tmp_path / folder) + result = run_script(FAMILY, "siblings", "--family-root", str(holder), + "--dry-run") + assert result.returncode == 0, result.stdout + result.stderr + assert "NOTHING WAS MOVED" in result.stderr + assert "in a POSIX shell (Git Bash" in result.stderr + commands = [shlex.split(line) for line in result.stderr.splitlines() + if line.startswith(" " * 12) and line.strip()[:3] in + ("mv ", "mkd")] + staged = holder.parent.parent / f"{NAME}.holder" + wanted = holder.parent.parent / NAME + assert commands == [["mv", str(holder), str(staged)], + ["mkdir", "-p", str(wanted)], + ["mv", str(staged), str(wanted / NAME)]] + assert holder.is_dir(), "the warning moves nothing" + if os.name == "nt": + return # `mv` and `mkdir -p` are a POSIX shell's, as the line says + for argv in commands: + subprocess.run(argv, check=True) + assert not holder.exists() + moved = wanted / NAME + assert (moved / "family.yaml").is_file() + assert git("rev-parse", "--show-toplevel", cwd=moved).stdout.strip() \ + == str(moved) From 5b59233f43c9b5a0a3b09891c3a21b3d6d79199e Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Tue, 13 Oct 2026 00:20:46 +0000 Subject: [PATCH 3/6] family.py: `add`, `bump` and `remove` refuse to commit onto the holder's tracking branch or a HEAD on no branch, naming the `git switch -c` that makes one, and end with the push of the branch they committed on, so AGENTS.md's "land it as a pull request" can be followed and the printed NEXT no longer pushes a `` that does not exist (#487) AGENTS.md says each of the three writes ONE commit, to land as a pull request, and no step made the branch that pull request needs. After `init` the holder stands on `main`, and all three committed straight onto it; `add` printed `NEXT git -C push -u origin && open a pull request`, a placeholder no repository has (`src refspec ... does not match any`), even on a branch; `bump` and `remove` printed no NEXT. `bump-leg.py` refuses exactly this for a leg (`bump-leg-on-tracking-branch`). - The real run of each refuses the holder's tracking branch (`family-on-tracking-branch`) and a HEAD on no branch (`family-holder-detached`, as `bump-leg.py` refuses `bump-leg-root-detached`), after the refusals about the arguments and the holder and before the member is read, fetched or cloned or anything is written. The remedy is the quoted `git -C switch -c `: `family/add-`, `family/bump--` or `family/remove-`, `member` for a name that is no plain branch word. - `--dry-run` runs on any branch and prints a `branch` line: where the commit lands, or that the real run refuses there, by its code, with the `switch` on a line of its own. The module's EXIT CODES paragraph names it as the one refusal a dry run does not make. - Each act that commits ends with ONE pasteable line, `NEXT git -C push -u origin `, every word quoted by `shell_line` (#486), then `then open a pull request against ; never push to it directly`. `bump` and `remove` gain it. - `init`'s NEXT STEPS open with the `switch` before the `add`. - `add`, `bump` and `remove` `--help` say the rule; docs/cli.md regenerated. - AGENTS.md's family block gains the `switch` step before the `add`, and step 3 says the tracking branch is refused (`family-on-tracking-branch`) and the branch is pushed as its NEXT line says; README's InkRouter block gains the same step and its paragraph says it. Both line-neutral (AGENTS.md steps 2 and 5 and README's `make bootstrap` paragraph are tightened). docs/handbook.html mirrors the block and the sentence; its footer names #487 and the new README blob. - Fixtures that committed on `main` commit on a branch: `tests/test_family.py` grows the family on `family/members` and pushes it to the remote's `main`, as the merged pull request would; `test_shape_doctor.py`'s holder and `test_family_add_excludes.py`'s `empty_holder` switch to a branch; `test_family_add_refusals.py`'s NEXT expectation names the branch, its unborn holder is born on a branch of its own, its empty-family holder switches, and its corrupt-HEAD test writes the branch HEAD is on and, for HEAD naming a tag, answers the branch check in-process, since the test is about the rollback's reading of HEAD; the siblings layout row names the holder's branch. - tests/test_family_tracking_branch.py: for each act, refused on `main` with the holder byte for byte as it was (and, for `add`, nothing cloned), the printed `switch` run as split makes the branch, the act goes through on it, and its printed NEXT run as split pushes it, which the remote then holds at the act's commit; a detached HEAD refused; dry runs on `main` and on a branch; suggested names pass `git check-ref-format --branch`; `init`'s NEXT STEPS open with the `switch`. Lane: openreposhape-5 (openRepoShape-5) Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 22 +-- README.md | 26 +-- docs/cli.md | 15 ++ docs/handbook.html | 10 +- scripts/family.py | 207 ++++++++++++++++++--- tests/test_family.py | 12 +- tests/test_family_add_excludes.py | 2 + tests/test_family_add_refusals.py | 32 +++- tests/test_family_tracking_branch.py | 268 +++++++++++++++++++++++++++ tests/test_shape_doctor.py | 2 + 10 files changed, 537 insertions(+), 59 deletions(-) create mode 100644 tests/test_family_tracking_branch.py diff --git a/AGENTS.md b/AGENTS.md index 2fee4df4..f8c29f4e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -360,6 +360,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 python3 scripts/family.py add --family-root --member / python3 scripts/family.py bump --family-root --member --to python3 scripts/family.py siblings --family-root @@ -378,14 +379,14 @@ python3 scripts/family.py siblings --family-root Say the landing path back to the human: the doubled `/` is deliberate and reads like a mistake until somebody explains it. 2. **A family pins ASSEMBLY ROOTS, never legs.** `--member /`, - never `-spec`. The tool refuses a leg, and the refusal is - correct: a leg has no `project.yaml` and belongs to its own root. + never `-spec`: a leg has no `project.yaml`, and is rightly refused. 3. **Each of `add`, `bump` and `remove` writes ONE commit** moving the - gitlink and the pin together. Land it as a pull request; never suggest a - push to the default branch. Do not hand-edit `family.yaml`'s `members:` + gitlink and the pin together, on a branch you make first (see above): the + tracking branch is refused (`family-on-tracking-branch`). Push the branch + as its `NEXT` line says and land it as a pull request; never suggest a + push to the default branch. Never hand-edit `family.yaml`'s `members:` block — the tool rewrites it wholesale. `bump` takes the same repeatable - `--trailer ": "` as `update-shape.py apply`, for the lines its - commit must end with. + `--trailer ": "` as `update-shape.py apply`. 4. **The member must be scaffolded or adopted FIRST.** A family cannot make a project out of a repository; it can only pin one that already is. 5. **`make siblings` in the holder — or `family.py siblings --family-root @@ -398,11 +399,10 @@ python3 scripts/family.py siblings --family-root parent folder is not named after the family it warns and prints an `mv`, and running that `mv` yourself relocates a checkout out from under a shell, an editor and every agent lane, and breaks any linked worktree - (its `.git` file carries an absolute path). It also never touches an - existing clone beyond `git fetch`: no checkout, no reset, no pull, - because somebody is working in there. A directory that is a clone of a - different repository is reported and skipped; the exit is the human's - hand, not an overwrite. + (its `.git` file carries an absolute path). An existing clone gets only + `git fetch` — no checkout, no reset, no pull — because somebody works in + there; one of a different repository is reported and skipped, and the + exit is the human's hand, not an overwrite. 6. `update-shape.py` updates a family root exactly as it updates a project, and the same four refusals apply. It mirrors into `family.yaml`. An existing holder receives `scripts/siblings.py` that way: `check` reports diff --git a/README.md b/README.md index 5465d135..ae6ebea4 100644 --- a/README.md +++ b/README.md @@ -901,27 +901,27 @@ InkRouter/ the family FOLDER (see the layout above) ```sh python3 scripts/family.py init --org InkRouter --family InkRouter --into .. --reuse-empty-repo +git -C ../InkRouter/InkRouter switch -c family/add-irrs python3 scripts/family.py add --family-root ../InkRouter/InkRouter --member InkRouter/IRRS python3 scripts/family.py bump --family-root ../InkRouter/InkRouter --member IRRS --to <40 hex> python3 scripts/family.py remove --family-root ../InkRouter/InkRouter --member IRRS python3 scripts/family.py siblings --family-root ../InkRouter/InkRouter ``` -`add`, `bump` and `remove` each write ONE commit, with explicit pathspecs, -moving the gitlink and `members[].pin` together — the same lockstep rule an -assembly root applies to its legs, and `scripts/validate-family.py` refuses -when they disagree. Each row also records the member's own `project.yaml` -`id`, so the validator can check the tree mounted at `members/` is -the project the row claims rather than merely a repository at the right -commit. **A family pins assembly roots, never legs**: adding `-spec` -is refused, because a leg has no `project.yaml` and belongs to its own root. +`add`, `bump` and `remove` each write ONE commit, with explicit pathspecs, on a +branch made first (the tracking branch is refused) and print its push: the +gitlink and `members[].pin` move together, the lockstep rule an assembly root +applies to its legs, which `scripts/validate-family.py` enforces. Each row +records the member's `project.yaml` `id`, so the validator can check +`members/` holds the project the row claims, not merely a repository +at the right commit. **A family pins assembly roots, never legs**: +`-spec` is refused; a leg has no `project.yaml`. `make bootstrap` in the holder fetches every member and its legs — resolving -the same GitHub App or `SHAPE_LEGS_TOKEN` credential the workflow does, -because a member is a private assembly root that mounts two private legs of -its own — and then runs each member's own `make bootstrap`. `make validate` -runs the family validator and then each member's `make validate`. A member -that could not be fetched is reported and skipped, never a failure. +the GitHub App or `SHAPE_LEGS_TOKEN` credential the workflow does, as a member +is a private assembly root mounting two private legs — then runs each member's +own `make bootstrap`; `make validate` runs the family validator, then each +member's. A member it cannot fetch is reported and skipped, never a failure. The holder carries the same COPY pin an assembly root does, so `update-shape.py` re-syncs it the same way; it mirrors into `family.yaml` diff --git a/docs/cli.md b/docs/cli.md index 8d315839..5279e170 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -708,6 +708,11 @@ usage: family.py add [-h] --family-root FAMILY_ROOT --member ORG/PROJECT [--at COMMIT] [--local-remote-dir LOCAL_REMOTE_DIR] [--trailer "KEY: VALUE"] [--dry-run] +Mount and pin a member. Its ONE commit lands on the branch the holder stands +on: the real run refuses the holder's tracking branch and a HEAD on no branch, +naming the `git switch -c` that makes one, and ends with the `git push` of +that branch. --dry-run runs on any branch. + options: -h, --help show this help message and exit --family-root FAMILY_ROOT @@ -737,6 +742,11 @@ options: usage: family.py bump [-h] --family-root FAMILY_ROOT --member PROJECT --to COMMIT [--trailer "KEY: VALUE"] [--dry-run] +Move a member's gitlink and pin together. Its ONE commit lands on the branch +the holder stands on: the real run refuses the holder's tracking branch and a +HEAD on no branch, naming the `git switch -c` that makes one, and ends with +the `git push` of that branch. --dry-run runs on any branch. + options: -h, --help show this help message and exit --family-root FAMILY_ROOT @@ -761,6 +771,11 @@ options: usage: family.py remove [-h] --family-root FAMILY_ROOT --member PROJECT [--dry-run] +Unmount a member and drop its row. Its ONE commit lands on the branch the +holder stands on: the real run refuses the holder's tracking branch and a HEAD +on no branch, naming the `git switch -c` that makes one, and ends with the +`git push` of that branch. --dry-run runs on any branch. + options: -h, --help show this help message and exit --family-root FAMILY_ROOT diff --git a/docs/handbook.html b/docs/handbook.html index 1baf4391..b72d7fb4 100644 --- a/docs/handbook.html +++ b/docs/handbook.html @@ -1245,12 +1245,14 @@

The InkRouter example

…six more, added as they arrive IRRS/ IRSS/ … the same members as WORKING clones, on main 
python3 scripts/family.py init --org InkRouter --family InkRouter --into .. --reuse-empty-repo
+git -C ../InkRouter/InkRouter switch -c family/add-irrs
 python3 scripts/family.py add  --family-root ../InkRouter/InkRouter --member InkRouter/IRRS
 python3 scripts/family.py bump --family-root ../InkRouter/InkRouter --member IRRS --to <40 hex>
 python3 scripts/family.py remove --family-root ../InkRouter/InkRouter --member IRRS
 python3 scripts/family.py siblings --family-root ../InkRouter/InkRouter

add, bump and remove each write ONE commit, with - explicit pathspecs, moving the gitlink and members[].pin together — the + explicit pathspecs, on a branch made first (the tracking branch is refused) and print its + push, moving the gitlink and members[].pin together — the same lockstep rule an assembly root applies to its legs, and scripts/validate-family.py refuses when they disagree. Each row also records the member’s own project.yaml id, so the validator can check the @@ -1799,9 +1801,11 @@

For AI assistants

name, added (#289); bump-leg.py refusing a workflow file, the workflows directory or the leg’s pin that this user cannot open, by name, added (#383); what setup.sh --help does from a download, which clones nothing, and which commands - list every flag, added (#341) — a page cannot name the commit that contains it, and + list every flag, added (#341); family.py add, bump and + remove committing on a branch made first, never the tracking branch, and + printing the push of it, added (#487) — a page cannot name the commit that contains it, and the blob below is the fact a test checks. - README.md blob 5465d135ca7263010ee354424041f90c4553d2e7. Every command, flag, exit code + README.md blob ae6ebea4c39bffeaeaa8fc624741e68ec0438653. Every command, flag, exit code and file name on this page is quoted from that README and from AGENTS.md.

diff --git a/scripts/family.py b/scripts/family.py index 94d0dcd2..9e10f09c 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -55,7 +55,11 @@ EXIT CODES: 0 done, or a `--dry-run` printed what it would do · 1 nothing to do · 2 a refusal, and a refusal made before the first write is the same refusal under `--dry-run`. Every subcommand takes `--dry-run`: `init`, `add`, -`bump`, `remove` and `siblings` (#326). +`bump`, `remove` and `siblings` (#326). ONE EXCEPTION, THE BRANCH (#487): a +real `add`, `bump` or `remove` refuses to commit onto the holder's tracking +branch, or onto no branch at all, as `bump-leg.py` does for a leg; a dry run +commits nothing, so it runs on any branch and says where the real run's commit +would land, or that it would refuse there and the command that makes a branch. `siblings` exits with the template script's own code, unchanged. A `bump` whose commit would record nothing — the member is already pinned at `--to`, in the gitlink and in `family.yaml` alike — is `nothing`: it says `nothing to do`, as @@ -150,7 +154,7 @@ ) # How every command this prints for a person to paste spells its paths # (#486), the quoting the holder's `scripts/siblings.py` uses too. -from repo_shape import shell_quoted # noqa: E402 +from repo_shape import shell_line, shell_quoted # noqa: E402 NAMING_POLICY = SHAPE_ROOT / "contracts" / "repository-naming.yaml" MANIFEST = "family.yaml" @@ -566,6 +570,117 @@ def _read_member_project(path: Path, project: str, repository: str, return project_id +# --------------------------------------------------------------------------- +# The branch an act commits on (#487) +# --------------------------------------------------------------------------- +# +# AGENTS.md says each of `add`, `bump` and `remove` writes ONE commit, to land +# as a pull request, never as a push to the default branch. Nothing made the +# branch that pull request needs: after `init` the holder stands on its +# tracking branch, and all three committed straight onto it, while `add` +# printed `git -C push -u origin `, a placeholder no +# repository has (`src refspec ... does not match any`), and `bump` and +# `remove` printed no NEXT at all. `bump-leg.py` refuses exactly this for a +# leg (`bump-leg-on-tracking-branch`). So the real run of each refuses the +# tracking branch and a HEAD on no branch, before it reads the member or +# writes anything, with the `git switch -c` that makes a branch; and each +# act that commits ends with ONE pasteable line that pushes the branch it +# committed on. + + +def _holder_branch(root: Path) -> str | None: + """The branch the holder's HEAD is on, or None: detached, or a HEAD + that names something other than a branch.""" + try: + ref = _git_bytes(["symbolic-ref", "-q", "HEAD"], root).decode( + "utf-8", "surrogateescape").strip() + except CommandFailed: + return None + return ref[len("refs/heads/"):] if ref.startswith("refs/heads/") else None + + +#: A branch-name component the suggestion may use as it stands: dot-separated +#: words of lowercase letters, digits, `_` and `-`, which no git rule refuses +#: but a trailing `.lock`, which is checked beside it. +BRANCH_WORD_RE = re.compile(r"[a-z0-9][a-z0-9_-]*(?:\.[a-z0-9_-]+)*") + + +def _suggested_branch(verb: str, name: str, commit: str | None = None) -> str: + """The branch a refusal suggests for one act (#487): `family/- + `, with the commit's first twelve for `bump`, as `bump-leg.py` + suggests `bump/-`. The member is its row's `id:` (`bump`, + `remove`), or its name lowercased (`add`, which has not read the member + yet); one that is not a plain branch word is said as `member`.""" + word = name.lower() + if not BRANCH_WORD_RE.fullmatch(word) or word.endswith(".lock"): + word = "member" + return f"family/{verb}-{word}" + (f"-{commit[:12]}" if commit else "") + + +def _switch_command(root: Path, branch: str) -> str: + """`git -C switch -c `, every word spelled by + `shell_quoted` for the reader's shell (#486).""" + return shell_line(["git", "-C", root, "switch", "-c", branch]) + + +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.""" + 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.") + if branch is None: + raise Refusal( + "family-holder-detached", + f"{family.root} is not on a branch, so `{verb}` would write a " + "commit no branch holds, with nothing to push and open a pull " + "request from. No git command that writes was run and nothing " + "was written.", remedy) + raise Refusal( + "family-on-tracking-branch", + f"{family.root} is on {branch!r}, which {MANIFEST} declares as the " + f"holder's tracking branch, and `{verb}` will not write a commit " + "whose only next step is a push to the default branch. No git " + "command that writes was run and nothing was written.", remedy) + + +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.""" + branch = _holder_branch(family.root) + tracking = family.tracking() + if branch is not None and branch != tracking: + return [f"branch {branch}: the commit lands there, for a pull " + f"request against {tracking}"] + code, where = (("family-holder-detached", "the holder is on no branch") + if branch is None else + ("family-on-tracking-branch", + 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)}"] + + +def _print_next(family: "Family", branch: str) -> None: + """The NEXT an act that committed ends with (#487): ONE pasteable line + that pushes the branch the commit is on, every word quoted for the + reader's shell (#486), then the pull request it is for.""" + print("\nNEXT " + shell_line(["git", "-C", family.root, "push", "-u", + "origin", branch])) + print(f" then open a pull request against {family.tracking()}; " + "never push to it directly") + + def _commit_one(root: Path, message: str, paths: list[str], add: list[str] | None = None, trailers: list[str] = ()) -> str: @@ -1142,18 +1257,23 @@ def _finish_holder_remote(args, repository: str, topic: str, def _print_next_steps(args, work: Path, folder: Path | None) -> None: """The NEXT STEPS block `init` ends with. - Split from `cmd_init` for #136. + 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. """ + switch = shell_line(["git", "-C", work, "switch", "-c"]) print(f""" NEXT STEPS + {switch} family/add- {_add_command(work, args.org)} cd {shell_quoted(work)} && make siblings `add` mounts the member at {MEMBERS_DIR}/, records its pin, and -writes ONE commit; land it as a pull request. `make siblings` then puts each -member's WORKING clone beside the holder, in the family folder, on its -tracking branch. +writes ONE commit on that branch: it refuses {args.tracking_branch}, the +holder's tracking branch. Push the branch as its NEXT line says and land it +as a pull request. `make siblings` then puts each member's WORKING clone +beside the holder, in the family folder, on its tracking branch. TWO COPIES OF EVERY MEMBER, on purpose: pinned and DETACHED inside the holder under {MEMBERS_DIR}/ (what `make bootstrap` and `make validate` read), and a @@ -2605,7 +2725,7 @@ def _resolves(root: Path, spec: str) -> bool: def _add_dry_run(family: Family, repository: str, project: str, path: str, url: str, protocol: list[str], commit: str, at: str | None, - trailers: list[str]) -> int: + trailers: list[str], suggested: str) -> int: """`add --dry-run` (#326), entered once every refusal `add` makes before its first write has been asked — the holder's (`_checked_add_target`), the member's (`_probe_member`) and whether the `id:` the probe read is @@ -2708,6 +2828,8 @@ def _add_dry_run(family: Family, repository: str, project: str, path: str, print(f"row as {MANIFEST} would carry it:") for line in _member_lines(row): print(line) + for line in _branch_said(family, "add", suggested): + print(line) _print_trailers(trailers) print(f"\n--dry-run: nothing was changed in the holder. {repository} was " "read from throwaway clones in temporary directories outside the " @@ -2722,8 +2844,10 @@ def cmd_add(args) -> int: would write and change nothing in the holder (#326, `_add_dry_run`). IN THIS ORDER, AND ONLY THIS ORDER: the holder's refusals - (`_checked_add_target`), then the member's (`_probe_member`, of a - throwaway clone), then the member's id against the rows + (`_checked_add_target`), then the branch the commit lands on, which the + real run refuses on the tracking branch or none (`_checked_branch`, + #487) and a dry run only reports, then the member's (`_probe_member`, of + a throwaway clone), then the member's id against the rows (`_checked_member_id`, #330), and only then the mount, the pin and the commit (`_mount_and_pin`), inside a `MountRollback` that puts the holder back if anything still fails and says what it put back. @@ -2739,6 +2863,9 @@ def cmd_add(args) -> int: repository, project = _checked_member(args.member) path = _member_path(project) at = _checked_add_target(family, repository, project, path, args.at) + suggested = _suggested_branch("add", project) + branch = None if args.dry_run else _checked_branch(family, "add", + suggested) url = _member_url(repository, args.local_remote_dir) protocol = FILE_PROTOCOL if args.local_remote_dir is not None else [] @@ -2759,7 +2886,7 @@ def cmd_add(args) -> int: # before `MountRollback` is taken and anything is mounted (#326). if args.dry_run: return _add_dry_run(family, repository, project, path, url, - protocol, commit, at, args.trailer) + protocol, commit, at, args.trailer, suggested) rollback = MountRollback(family.root, path) print(f"mounting {repository} at {path}", flush=True) @@ -2781,8 +2908,7 @@ def cmd_add(args) -> int: print(f" {project:<16} {commit[:12]} tree {digest[:12]}… " f"(project {project_id})") print(f" committed {head[:12]}: .gitmodules, {path}, {MANIFEST}") - print(f"\nNEXT git -C {shell_quoted(family.root)} push -u origin " - " && open a pull request") + _print_next(family, branch) return 0 @@ -3769,7 +3895,7 @@ def _print_trailers(trailers: list[str]) -> None: def _bump_dry_run(family: Family, row: dict, path: str, commit: str, - trailers: list[str], fetched: bool) -> int: + trailers: list[str], fetched: bool, suggested: str) -> int: """`bump --dry-run`, entered after the member is fetched (or the fetch failed, `fetched` False, and said so) and before it is checked out (#326): the answers a real `bump` gives, and no write. @@ -3832,6 +3958,8 @@ def _bump_dry_run(family: Family, row: dict, path: str, commit: str, print(f"row as {MANIFEST} would carry it:") for line in _member_lines(new): print(line) + for line in _branch_said(family, "bump", suggested): + print(line) _print_trailers(trailers) how = (f"`git fetch` ran in {path}, as a real bump's does, which is how " "--to was resolved and the digest above computed" if fetched else @@ -4000,6 +4128,13 @@ def cmd_bump(args) -> int: "Remediation: `git submodule update --init " + path + "`, or " "`make bootstrap`, which resolves a credential for a private " "member first.") + # THE BRANCH BEFORE THE FETCH (#487): the real run's commit needs one + # that is not the tracking branch, and a refusal is cheaper before the + # member's remote is asked anything. The dry run reports it instead. + suggested = _suggested_branch("bump", str(row.get("id") or project), + commit) + branch = None if args.dry_run else _checked_branch(family, "bump", + suggested) fetched = True try: run(["git", "fetch", "-q", "origin"], cwd=member) @@ -4009,11 +4144,14 @@ def cmd_bump(args) -> int: "must already be in its object store", file=sys.stderr) if args.dry_run: return _bump_dry_run(family, row, path, commit, args.trailer, - fetched) + fetched, suggested) _checked_bump_target(family, row, path, commit) rollback = PinRollback(family.root, path, "bump", (MANIFEST,)) - return _put_back_on_failure(rollback, lambda: _bump_and_pin( + code = _put_back_on_failure(rollback, lambda: _bump_and_pin( family, row, path, commit, args.trailer, rollback)) + if code == 0: + _print_next(family, branch) + return code # --------------------------------------------------------------------------- @@ -4262,7 +4400,8 @@ def _unmount_and_drop(family: Family, row: dict, path: str) -> int: return 0 -def _remove_dry_run(family: Family, row: dict, path: str) -> int: +def _remove_dry_run(family: Family, row: dict, path: str, + suggested: str) -> int: """`remove --dry-run` (#326): what a real `remove` would take out, and no git command that writes. @@ -4332,6 +4471,8 @@ def _remove_dry_run(family: Family, row: dict, path: str) -> int: "deleted with everything in it: `git submodule deinit -f` discards " "uncommitted work in the member, and `git rm` the directory" if os.path.lexists(root / path) else "not on disk")) + for line in _branch_said(family, "remove", suggested): + print(line) print("\n--dry-run: nothing was changed: the index, .gitmodules, " f"{MANIFEST}, the work tree and the holder's git config are as they " "were, and no git command that writes was run.") @@ -4359,9 +4500,12 @@ def cmd_remove(args) -> int: # the same refusal, as it makes every one above. _checked_one_section(family.root, project, path) # Every refusal above is the same under `--dry-run`, which leaves here, - # before the unmount, the real run's first write (#326). + # before the unmount, the real run's first write (#326), and reports the + # branch the real run refuses (#487) rather than refusing it. + suggested = _suggested_branch("remove", str(row.get("id") or project)) if args.dry_run: - return _remove_dry_run(family, row, path) + return _remove_dry_run(family, row, path, suggested) + branch = _checked_branch(family, "remove", suggested) # WHAT CAN BE ASKED BEFORE THE UNMOUNT IS (#365); what fails after it is # put back (`PinRollback`). A `members:` block this tool will not rewrite # was refused after `deinit` and `git rm`, with the member unmounted and @@ -4373,8 +4517,11 @@ def cmd_remove(args) -> int: _checked_index_unlocked(family.root, path) rollback = PinRollback(family.root, path, "remove", (GITMODULES, MANIFEST), unmount=True) - return _put_back_on_failure( + code = _put_back_on_failure( rollback, lambda: _unmount_and_drop(family, row, path)) + if code == 0: + _print_next(family, branch) + return code # --------------------------------------------------------------------------- @@ -4423,6 +4570,15 @@ def cmd_siblings(args) -> int: # --------------------------------------------------------------------------- +#: What `add`, `bump` and `remove` each say in `--help` about the branch +#: their commit lands on (#487). +BRANCH_RULE = ("Its ONE commit lands on the branch the holder stands on: " + "the real run refuses the holder's tracking branch and a HEAD " + "on no branch, naming the `git switch -c` that makes one, and " + "ends with the `git push` of that branch. --dry-run runs on " + "any branch.") + + def build_parser() -> argparse.ArgumentParser: parser = argparse.ArgumentParser( prog="family.py", description=__doc__.splitlines()[0], @@ -4480,7 +4636,9 @@ def build_parser() -> argparse.ArgumentParser: init.add_argument("--dry-run", action="store_true") init.set_defaults(func=cmd_init) - add = subparsers.add_parser("add", help="mount and pin a member") + add = subparsers.add_parser( + "add", help="mount and pin a member", + description="Mount and pin a member. " + BRANCH_RULE) add.add_argument("--family-root", required=True) add.add_argument("--member", required=True, metavar="ORG/PROJECT", help="the member's ASSEMBLY ROOT, `/` — " @@ -4505,7 +4663,10 @@ def build_parser() -> argparse.ArgumentParser: "nothing") add.set_defaults(func=cmd_add) - bump = subparsers.add_parser("bump", help="move a member's pin") + bump = subparsers.add_parser( + "bump", help="move a member's pin", + description="Move a member's gitlink and pin together. " + + BRANCH_RULE) bump.add_argument("--family-root", required=True) bump.add_argument("--member", required=True, metavar="PROJECT") bump.add_argument("--to", required=True, metavar="COMMIT", @@ -4524,7 +4685,9 @@ def build_parser() -> argparse.ArgumentParser: "if there is nothing to do") bump.set_defaults(func=cmd_bump) - remove = subparsers.add_parser("remove", help="unmount a member") + remove = subparsers.add_parser( + "remove", help="unmount a member", + description="Unmount a member and drop its row. " + BRANCH_RULE) remove.add_argument("--family-root", required=True) remove.add_argument("--member", required=True, metavar="PROJECT") remove.add_argument("--dry-run", action="store_true", diff --git a/tests/test_family.py b/tests/test_family.py index d1f5b9fa..5d93e278 100644 --- a/tests/test_family.py +++ b/tests/test_family.py @@ -60,6 +60,13 @@ #: removing a member to make room would test `remove` by accident. SPARE = "IRQS" +#: The branch the fixture grows the family on (#487). `add`, `bump` and +#: `remove` refuse to commit onto the holder's tracking branch, so the members +#: are added on a branch, as a person adds them for a pull request, and the +#: remote's `main` is then moved to it, as merging that pull request leaves +#: it. Every copy of the holder a test makes stands on this branch. +WORK_BRANCH = "family/members" + #: `git` reads one-off configuration out of the environment, which is how a #: test gives a plain `git submodule update` permission to clone a local path #: without the tool under test knowing anything about it. @@ -100,12 +107,13 @@ def family(tmp_path_factory) -> dict: "--work-dir", str(base / "fam")) assert created.returncode == 0, created.stderr + created.stdout root = base / "fam" / NAME + git("switch", "-q", "-c", WORK_BRANCH, cwd=root) for project in MEMBERS: added = run_script(FAMILY, "add", "--family-root", str(root), "--member", f"{ORG}/{project}", "--local-remote-dir", str(base / "remotes")) assert added.returncode == 0, added.stderr + added.stdout - git("push", "-q", "origin", "main", cwd=root) + git("push", "-q", "origin", f"{WORK_BRANCH}:main", cwd=root) return {"base": base, "root": root, "remotes": base / "remotes", "work": base / "work", "init": created} @@ -1723,7 +1731,7 @@ def test_siblings_clones_each_member_beside_the_holder_on_its_branch(estate): assert "siblings ok: 2 member(s)" in proc.stdout assert layout(proc.stdout) == [ "what name state branch", - f"holder {NAME} the holder main", + f"holder {NAME} the holder {WORK_BRANCH}", "sibling IRRS cloned main", "sibling IRSS cloned main", "pinned members/IRRS pinned in the holder main", diff --git a/tests/test_family_add_excludes.py b/tests/test_family_add_excludes.py index d6737599..80044c29 100644 --- a/tests/test_family_add_excludes.py +++ b/tests/test_family_add_excludes.py @@ -178,6 +178,8 @@ def empty_holder(tmp_path) -> Path: assert created.returncode == 0, created.stderr + created.stdout root = tmp_path / "work" / "Contoso" assert not (root / "members").exists() + # On a branch, as `add` requires: it refuses the tracking branch (#487). + git("switch", "-q", "-c", "family/add", cwd=root) return root diff --git a/tests/test_family_add_refusals.py b/tests/test_family_add_refusals.py index b0ef041f..35615529 100644 --- a/tests/test_family_add_refusals.py +++ b/tests/test_family_add_refusals.py @@ -49,7 +49,7 @@ #: rather than rebuilt: a holder made any other way would test a different #: holder. Importing the fixture gives this module its own instance of it. from test_family import ( # noqa: F401 - FAMILY, NAME, ORG, SPARE, family, manifest, validate, + FAMILY, NAME, ORG, SPARE, WORK_BRANCH, family, manifest, validate, ) #: Every refusal that comes after the mount ends with this sentence when the @@ -65,7 +65,11 @@ "`{root}-spec` or `{root}-code` — is never right: the legs belong to " "their own assembly root{named}.") -NEXT = "NEXT git -C {root} push -u origin && open a pull request" +#: What a good `add` ends with (#487): the push of the branch it committed on, +#: which is the fixture's own, and the pull request it is for. +NEXT = ("NEXT git -C {root} push -u origin {branch}", + " then open a pull request against main; never push to it " + "directly") #: What an `--at` naming a tree or a blob, or a tag of one, is told to do. AT_NAMES_NO_COMMIT_REMEDIATION = ( @@ -253,9 +257,11 @@ def add_in_process(module, monkeypatch, offline: Offline, root: Path, def unborn_holder(tmp_path: Path) -> Path: """A holder with NO COMMIT YET: `git init` and a hand-written `family.yaml`, not even staged. `family.py init` always commits; a - holder made by hand does not.""" + holder made by hand does not. Its unborn HEAD is on a branch of its own, + not the tracking branch `main` that `add` refuses to commit onto + (#487), whatever the default branch of the `git init` running here.""" root = tmp_path / "Unborn" - git("init", "-q", str(root), cwd=tmp_path) + git("init", "-q", "-b", "family/unborn", str(root), cwd=tmp_path) (root / "family.yaml").write_bytes( b"schema_version: 1\nkind: family-manifest\nname: Unborn\n" b"members: []\n") @@ -834,6 +840,8 @@ def test_the_first_member_of_an_empty_family_unwinds_to_no_gitmodules( root = tmp_path / "work" / "Contoso" assert not (root / ".gitmodules").exists() assert not (root / "members").exists() + # On a branch, as `add` requires: it refuses the tracking branch (#487). + git("switch", "-q", "-c", f"family/add-{SPARE.lower()}", cwd=root) monkeypatch.setattr(family_module, "write_lf", full_disk) monkeypatch.setattr(tempfile, "tempdir", str(offline.temp)) monkeypatch.setenv("PATH", offline.env["PATH"]) @@ -1182,8 +1190,8 @@ def test_a_head_that_names_no_readable_commit_is_unwound_like_any_other( """HEAD IS READ ONE WAY, BEFORE AND AFTER. `MountRollback` records it with `rev-parse -q --verify HEAD^{commit}` and `undo` reads it the same way. A bare `rev-parse HEAD` is not the same answer where HEAD is a symbolic ref - to an ANNOTATED TAG (the tag's own sha, not the commit's) or where - `refs/heads/main` names a MISSING OBJECT (that object's sha, where the + to an ANNOTATED TAG (the tag's own sha, not the commit's) or where the + branch HEAD is on names a MISSING OBJECT (that object's sha, where the other says there is none): `undo` took the difference for the one commit having landed, said "nothing was undone" and left the half-added member staged, in a holder it had unwound before. The two states are corrupt, so @@ -1193,8 +1201,15 @@ def test_a_head_that_names_no_readable_commit_is_unwound_like_any_other( git("-c", "user.name=t", "-c", "user.email=t@t", "tag", "-a", "-m", "an annotated tag", "v1", cwd=holder) git("symbolic-ref", "HEAD", "refs/tags/v1", cwd=holder) + # A HEAD that names a tag is on no branch, which the real `add` + # refuses before the mount (#487, `family-holder-detached`). What + # this test asks is how the rollback READS such a HEAD, so the + # branch is answered here as a holder on a branch answers it. + monkeypatch.setattr(family_module, "_checked_branch", + lambda *_args: WORK_BRANCH) else: - ref = holder / ".git" / "refs" / "heads" / "main" + # The branch HEAD is on: the fixture's own (#487), not `main`. + ref = holder / ".git" / "refs" / "heads" / WORK_BRANCH ref.parent.mkdir(parents=True, exist_ok=True) ref.write_bytes(b"deadbeef" * 5 + b"\n") if git("rev-parse", "HEAD", cwd=holder, check=False).stdout.strip() \ @@ -1248,7 +1263,8 @@ def test_a_good_add_is_one_commit_on_its_branch_as_before(family, holder, f" {SPARE:<16} {tip[:12]} tree {digest[:12]}… (project irqs)", f" committed {head[:12]}: .gitmodules, members/{SPARE}, family.yaml", "", - NEXT.format(root=holder.resolve()), + *[line.format(root=holder.resolve(), branch=WORK_BRANCH) + for line in NEXT], ] assert git("rev-parse", "HEAD~1", cwd=holder).stdout.strip() == was assert git("show", "--name-only", "--format=", "HEAD", diff --git a/tests/test_family_tracking_branch.py b/tests/test_family_tracking_branch.py new file mode 100644 index 00000000..5a5bef86 --- /dev/null +++ b/tests/test_family_tracking_branch.py @@ -0,0 +1,268 @@ +# SPDX-License-Identifier: Apache-2.0 +"""`add`, `bump` and `remove` commit on a BRANCH, and say which (#487). + +AGENTS.md says each of the three writes ONE commit, to land as a pull +request and never as a push to the default branch, and no step made the +branch that pull request needs. After `init` the holder stands on its +tracking branch, and all three committed straight onto it; `add` printed +`NEXT git -C push -u origin && open a pull request`, a +placeholder no repository has (`error: src refspec family/add-atlas does not +match any`, exit 1), even on a branch; and `bump` and `remove` printed no +NEXT at all. `bump-leg.py` refuses exactly this for a leg +(`bump-leg-on-tracking-branch`). + +So, as `bump-leg.py` does: the REAL run of each refuses the holder's tracking +branch (`family-on-tracking-branch`) and a HEAD on no branch +(`family-holder-detached`), before the member is read or anything is +written, with the `git -C switch -c ` that makes one, +quoted; `--dry-run` runs anywhere and says what the real run would do; and +each act that commits ends with ONE pasteable line pushing the branch it +committed on, then the pull request it is for. + +THE PROOF IS THE PRINTED LINES, RUN. The remedy's `switch` is split the way +the reader's shell splits it (`shlex`, or PowerShell's reading on Windows, +the dialect `repo_shape.shell_quoted` speaks there) and run; the act is run +again and goes through; its NEXT is split and run, and the holder's remote +then has the branch at the commit the act wrote. + +THE SAME FAMILY AS `test_family.py`, in a private copy put back on its +tracking branch, which is where `init` leaves a holder. The remote a push +reaches is this module's own instance of the fixture's, and every branch a +test pushes there is deleted again. +""" + +from __future__ import annotations + +import shutil +import subprocess + +import pytest + +from conftest import git, run_script +#: The family fixture, its names and the work branch it grows the holder on, +#: imported from the module that owns them. +from test_family import ( # noqa: F401 + FAMILY, NAME, ORG, SPARE, WORK_BRANCH, family, manifest, validate, +) +#: The shell's reading of a printed line on this host, owned by #486's tests. +from test_family_printed_quoting import host_words + +TRACKING = "main" + + +@pytest.fixture +def on_main(family, tmp_path): + """A private copy of the holder, back on its tracking branch with every + member the fixture added: what a person has after `init`, or after + pulling `main` once the last pull request landed.""" + holder = tmp_path / NAME + shutil.copytree(family["root"], holder, symlinks=True) + git("switch", "-q", "-C", TRACKING, cwd=holder) + assert git("status", "--porcelain", cwd=holder).stdout == "" + return holder.resolve() + + +def older_commit(family, project: str = "IRRS") -> str: + """A commit of the member that is not the one the family pins: the + fixture pins each member's tip, and its parent is the scaffold's own.""" + return git("rev-parse", "main~1", + cwd=family["remotes"] / f"{project}.git").stdout.strip() + + +def act(family, holder, verb: str, *extra: str): + """`verb` against `holder`, as a person types it.""" + if verb == "add": + args = ["--member", f"{ORG}/{SPARE}", + "--local-remote-dir", str(family["remotes"])] + elif verb == "bump": + args = ["--member", "IRRS", "--to", older_commit(family)] + else: + args = ["--member", "IRSS"] + return run_script(FAMILY, verb, "--family-root", str(holder), *args, + *extra) + + +#: The branch each act's refusal suggests: `family/-`, the +#: member by its row's id, and the commit's first twelve for `bump`. +def suggested(family, verb: str) -> str: + if verb == "add": + return f"family/add-{SPARE.lower()}" + if verb == "bump": + return f"family/bump-irrs-{older_commit(family)[:12]}" + return "family/remove-irss" + + +def snapshot(holder) -> dict: + return {"head": git("rev-parse", "HEAD", cwd=holder).stdout, + "branch": git("symbolic-ref", "-q", "HEAD", cwd=holder, + check=False).stdout, + "index": git("ls-files", "-s", cwd=holder).stdout, + "status": git("status", "--porcelain", cwd=holder).stdout, + "manifest": (holder / "family.yaml").read_bytes(), + "modules": sorted(p.name for p in + (holder / ".git" / "modules" / "members") + .iterdir())} + + +def remedy_switch(stderr: str) -> list[str]: + """The remedy's command line, as the reader's shell reads it.""" + lines = stderr.splitlines() + at = next(i for i, line in enumerate(lines) + if line.startswith("Remediation: ") + and line.endswith("run the same command again:")) + return host_words(lines[at + 1]) + + +def next_push(stdout: str) -> tuple[list[str], str]: + """The NEXT line, as the reader's shell reads it, and the line under + it.""" + lines = stdout.splitlines() + at = next(i for i, line in enumerate(lines) if line.startswith("NEXT ")) + return host_words(lines[at][len("NEXT "):]), lines[at + 1] + + +@pytest.mark.parametrize("verb", ["add", "bump", "remove"]) +def test_each_act_refuses_the_tracking_branch_then_lands_on_a_branch( + family, on_main, verb): + """THE ISSUE, END TO END, for each of the three acts: refused on `main` + with the holder exactly as it was; the printed `switch` makes the + branch; the act goes through on it; its printed NEXT pushes that + branch, which the remote then holds at the act's commit.""" + holder = on_main + before = snapshot(holder) + refused = act(family, holder, verb) + assert refused.returncode == 2, refused.stdout + refused.stderr + # Not one line on stdout: for `add`, not even the line that says the + # member is being read, so nothing was cloned either. + assert refused.stdout == "" + assert (f"REFUSED family-on-tracking-branch: {holder} is on " + f"'{TRACKING}', which family.yaml declares as the holder's " + f"tracking branch, and `{verb}` will not write a commit whose " + "only next step is a push to the default branch. No git command " + "that writes was run and nothing was written." + in refused.stderr), refused.stderr + assert snapshot(holder) == before + + branch = suggested(family, verb) + switch = remedy_switch(refused.stderr) + assert switch == ["git", "-C", str(holder), "switch", "-c", branch] + subprocess.run(switch, check=True, capture_output=True) + + done = act(family, holder, verb) + assert done.returncode == 0, done.stdout + done.stderr + head = git("rev-parse", "HEAD", cwd=holder).stdout.strip() + assert git("rev-parse", branch, cwd=holder).stdout.strip() == head + assert git("rev-parse", TRACKING, cwd=holder).stdout == before["head"], ( + "the tracking branch did not move") + push, then = next_push(done.stdout) + assert push == ["git", "-C", str(holder), "push", "-u", "origin", + branch] + assert then == (f" then open a pull request against {TRACKING}; " + "never push to it directly") + assert done.stdout.rstrip().endswith(then), ( + "NEXT is the last thing the act says") + pushed = subprocess.run(push, capture_output=True, text=True, + check=False) + try: + assert pushed.returncode == 0, pushed.stderr + remote = family["remotes"] / f"{NAME}.git" + assert git("rev-parse", f"refs/heads/{branch}", + cwd=remote).stdout.strip() == head + finally: + git("push", "-q", "origin", "--delete", branch, cwd=holder, + check=False) + assert validate(holder).returncode == 0 + + +def test_a_holder_on_no_branch_is_refused_too(family, on_main): + """A detached HEAD would take a commit no branch holds and leave + nothing to push, so it is refused with the same remedy, as + `bump-leg.py` refuses `bump-leg-root-detached`.""" + holder = on_main + git("checkout", "-q", "--detach", cwd=holder) + before = snapshot(holder) + refused = act(family, holder, "add") + assert refused.returncode == 2, refused.stdout + refused.stderr + assert refused.stdout == "" + assert (f"REFUSED family-holder-detached: {holder} is not on a branch" + in refused.stderr), refused.stderr + assert remedy_switch(refused.stderr) == [ + "git", "-C", str(holder), "switch", "-c", suggested(family, "add")] + assert snapshot(holder) == before + + +@pytest.mark.parametrize("verb", ["add", "bump", "remove"]) +def test_a_dry_run_runs_on_the_tracking_branch_and_says_so(family, on_main, + verb): + """`--dry-run` commits nothing, so it runs on any branch: on the + tracking branch it exits 0 and says the real run refuses there, by its + code, with the `switch` that makes a branch on a line of its own.""" + holder = on_main + before = snapshot(holder) + result = act(family, holder, 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]) == [ + "git", "-C", str(holder), "switch", "-c", suggested(family, verb)] + assert "NEXT" not in result.stdout + assert snapshot(holder) == before + + +@pytest.mark.parametrize("verb", ["add", "bump", "remove"]) +def test_a_dry_run_on_a_branch_names_where_the_commit_lands(family, on_main, + verb): + holder = on_main + git("switch", "-q", "-c", "family/wip", cwd=holder) + result = act(family, holder, verb, "--dry-run") + assert result.returncode == 0, result.stdout + result.stderr + assert (f"branch family/wip: the commit lands there, for a pull " + f"request against {TRACKING}") in result.stdout.splitlines() + + +def test_the_suggested_branch_is_a_name_git_takes(): + """A member name that is no plain branch word is suggested as + `member`: the remedy must paste and work.""" + import importlib.util + spec = importlib.util.spec_from_file_location("family_branch", FAMILY) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + assert module._suggested_branch("add", "Atlas") == "family/add-atlas" + assert module._suggested_branch("add", "Ze.d") == "family/add-ze.d" + assert module._suggested_branch("bump", "irrs", "a" * 40) == \ + "family/bump-irrs-" + "a" * 12 + for odd in ("x.lock", "a..b", "a.", "-a", ""): + assert module._suggested_branch("remove", odd) == \ + "family/remove-member" + for name in ("atlas", "ze.d", "a_b-c"): + branch = module._suggested_branch("add", name) + assert subprocess.run(["git", "check-ref-format", "--branch", + branch], capture_output=True, + check=False).returncode == 0, branch + + +def test_init_next_steps_make_the_branch_before_the_add(tmp_path): + """`init` leaves the holder on its tracking branch, where `add` now + refuses, so its NEXT STEPS open with the `switch` that makes a branch, + before the `add`: the first printed command is not one that refuses.""" + into = tmp_path / "projects" + into.mkdir() + result = run_script(FAMILY, "init", "--org", ORG, "--family", "Orbit", + "--created-by", "Test Human", + "--created-on", "2026-10-12", + "--local-remote-dir", str(tmp_path / "remotes"), + "--into", str(into), cwd=tmp_path) + assert result.returncode == 0, result.stdout + result.stderr + holder = into.resolve() / "Orbit" / "Orbit" + block = [line.strip() for line in + result.stdout.split("NEXT STEPS", 1)[1].splitlines() + if line.strip()] + assert host_words(block[0]) == ["git", "-C", str(holder), "switch", + "-c", "family/add-"] + assert " add --family-root " in block[1] + assert git("symbolic-ref", "--short", "HEAD", cwd=holder).stdout.strip() \ + == TRACKING + assert "it refuses main, the holder's tracking branch" in " ".join( + result.stdout.split()) diff --git a/tests/test_shape_doctor.py b/tests/test_shape_doctor.py index fc1c74fc..7f504b3f 100644 --- a/tests/test_shape_doctor.py +++ b/tests/test_shape_doctor.py @@ -419,6 +419,8 @@ def holder(standard, tmp_path_factory) -> dict: "--work-dir", str(base / "fam")) assert created.returncode == 0, created.stderr + created.stdout root = base / "fam" / FAMILY_NAME + # On a branch, as `add` requires: it refuses the tracking branch (#487). + git("switch", "-q", "-c", "family/members", cwd=root) added = run_script(standard / "scripts" / "family.py", "add", "--family-root", str(root), "--member", f"{FAMILY_NAME}/{FAMILY_MEMBER}", From 4bfa6b515097b1010ef8d63c36fc7a6aae3c7f5d Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Tue, 13 Oct 2026 01:54:35 +0000 Subject: [PATCH 4/6] family.py, siblings.py, setup-project.py: a holder that kept an older `repo_shape.py` still runs `make siblings`, `park` and `resume`; a word that begins with `=` is quoted; `init` prints `cd` and `make siblings` on two lines; setup-project's `add` names the branch first and quotes the holder (#486 review) - siblings.py quotes its POSIX `mv` and `mkdir -p` lines with `shlex.join` and no longer imports `shell_line`. A holder that kept its own `repo_shape.py` through `update-shape.py apply --accept-local` took the new siblings.py without the name and died with an ImportError at `make siblings`, `park` and `resume`, while `check` and the validator read the holder as fine. A test runs the holder's siblings.py `--help`, the misnamed-folder warning and `--make park` beside a `repo_shape.py` that lacks every name #486 added. - `repo_shape.shell_quoted` quotes a word that begins with `=`, in both dialects: zsh, macOS's login shell, expands a bare `=word` to a command's path. The doctor's `quote_arg` keeps the gap; the agreement test excludes that one case. - The agreement test with the doctor's quoter now covers every printable ASCII character alone and before, inside and after a word, and asserts the two alphabets are the same patterns. A new test runs the quoted words through the host's own sh, bash and zsh. - `init`'s NEXT STEPS print `cd ` and `make siblings` on two lines: Windows PowerShell 5.1 rejects `&&` between two statements. - setup-project.py's family hand-over prints `git -C switch -c family/add-` before the `add`, which refuses the tracking branch (#487), and quotes the holder with its own copy of the rule, held to `shell_quoted` by a test. Lane: openreposhape-5 (openRepoShape-5) Co-Authored-By: Claude Opus 5.5 --- scripts/family.py | 7 +- scripts/repo_shape.py | 12 +- setup-project.py | 36 ++++- templates/family-root/scripts/siblings.py | 17 ++- tests/test_family_printed_quoting.py | 178 ++++++++++++++++++++-- 5 files changed, 230 insertions(+), 20 deletions(-) diff --git a/scripts/family.py b/scripts/family.py index db1451ce..a597cb25 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -1263,6 +1263,10 @@ 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. + + ONE COMMAND A LINE, `cd` apart from `make siblings`: Windows PowerShell + 5.1, the shell `setup-project.py` documents, rejects `&&` between two + statements, and these lines are quoted for the shell of the host (#486). """ switch = shell_line(["git", "-C", work, "switch", "-c"]) print(f""" @@ -1270,7 +1274,8 @@ def _print_next_steps(args, work: Path, folder: Path | None) -> None: {switch} family/add- {_add_command(work, args.org)} - cd {shell_quoted(work)} && make siblings + cd {shell_quoted(work)} + make siblings `add` mounts the member at {MEMBERS_DIR}/, records its pin, and writes ONE commit on that branch: it refuses {args.tracking_branch}, the diff --git a/scripts/repo_shape.py b/scripts/repo_shape.py index 69c0ec0e..c226fc20 100644 --- a/scripts/repo_shape.py +++ b/scripts/repo_shape.py @@ -2817,11 +2817,19 @@ def shell_quoted(value: object, platform: str | None = None) -> str: flattened. The typographic single quotes are the price: PowerShell reads each of them as an apostrophe, so each is doubled too. On a value made of ASCII the two functions give the same answer, which - `tests/test_family_printed_quoting.py` holds them to. + `tests/test_family_printed_quoting.py` holds them to, but one: + + A WORD THAT BEGINS WITH `=` IS QUOTED, in both dialects. zsh, the login + shell macOS ships, expands a bare `=word` to the path of the command + `word` (its EQUALS option, on by default), so `=ls` reaches the command + as `/bin/ls` and `=x` as an error. `=` anywhere else is itself to every + shell, and quoting a leading one costs PowerShell nothing. The doctor's + `quote_arg` still leaves it bare: a known gap, named where #486 lands. """ text = str(value) windows = (os.name if platform is None else platform) == "nt" - if (SHELL_BARE_NT if windows else SHELL_BARE_POSIX).fullmatch(text): + bare = SHELL_BARE_NT if windows else SHELL_BARE_POSIX + if not text.startswith("=") and bare.fullmatch(text): return text if windows: return "'" + "".join(char * 2 if char in POWERSHELL_SINGLE_QUOTES diff --git a/setup-project.py b/setup-project.py index be7d44cd..a060b5eb 100755 --- a/setup-project.py +++ b/setup-project.py @@ -1733,6 +1733,29 @@ def clone_and_bootstrap(opts: Options, org: str, landing: Landing) -> dict: # 8. hand over # --------------------------------------------------------------------------- +#: A word this host's shell reads as itself, needing no quotes: +#: `scripts/repo_shape.py`'s `SHELL_BARE_NT` on Windows, else its +#: `SHELL_BARE_POSIX`. This file runs before any checkout of the standard +#: exists, so it carries its own copy of the rule, and +#: `tests/test_family_printed_quoting.py` holds the two to one answer. +SHELL_BARE = re.compile(r"[A-Za-z0-9_./:%+=\\-]+" if os.name == "nt" + else r"[A-Za-z0-9_./:@%+=,-]+") + + +def shell_word(value) -> str: + """`value` as ONE word of a command pasted into this host's shell: bare + where every character is itself to it, else in single quotes, doubled + inside for PowerShell, closed-escaped-reopened for `sh` (#486). A + leading `=` is quoted too, which zsh would expand.""" + text = str(value) + if not text.startswith("=") and SHELL_BARE.fullmatch(text): + return text + if os.name == "nt": + return "'" + re.sub("(['\u2018\u2019\u201a\u201b])", r"\1\1", + text) + "'" + return "'" + text.replace("'", "'\\''") + "'" + + def family_next_command(project: str, org: str, landing: Landing) -> None: """The LAST line of the next commands: who records the membership. @@ -1747,9 +1770,18 @@ def family_next_command(project: str, org: str, landing: Landing) -> None: with the standard's own relative path: `family.py` needs the standard beside it, and it works before a holder is on disk at all (RULING of 2026-09-09, ruling 1b). + + A BRANCH FIRST, AND EVERY PATH ONE WORD (#487, #486). `add` refuses to + commit onto the holder's tracking branch, so the `git switch -c` that + makes the branch it suggests comes before it; and a holder under a + folder with a space in it must reach `--family-root` whole. """ - say(" %s scripts/family.py add --family-root %s --member %s/%s" - % (PYTHON_CMD, landing.holder, org, project)) + holder = shell_word(landing.holder) + say(" git -C %s switch -c family/add-%s" % (holder, project.lower())) + say(" # a branch first: `add` refuses the") + say(" # holder's tracking branch") + say(" %s scripts/family.py add --family-root %s --member %s" + % (PYTHON_CMD, holder, shell_word("%s/%s" % (org, project)))) say(" # from a checkout of openRepoShape. The") say(" # HOLDER records the membership; this") say(" # project's manifest says nothing " diff --git a/templates/family-root/scripts/siblings.py b/templates/family-root/scripts/siblings.py index 6d30756e..7038a8ad 100755 --- a/templates/family-root/scripts/siblings.py +++ b/templates/family-root/scripts/siblings.py @@ -101,6 +101,7 @@ import argparse import os import re +import shlex import shutil import subprocess import sys @@ -119,7 +120,7 @@ # arithmetic `resolve_relative` hands git's relative-url rule to. from repo_shape import ( # noqa: E402 PYTHON, Refusal, find_repo_root, join_remote, load_yaml, - remote_local_path, repo_basename, same_repository, shell_line, + remote_local_path, repo_basename, same_repository, ) # THE CREDENTIAL AND THE MEMBER ROWS ARE READ BY `bootstrap.py` ALREADY, and a # second definition of either is how the two start disagreeing about which @@ -773,11 +774,15 @@ def parent_folder_warning(root: Path, family: str) -> str | None: ["mv", staged, wanted / family]] else: steps = [["mkdir", "-p", wanted], ["mv", root, wanted / family]] - # EVERY PATH QUOTED (#486), by `repo_shape.shell_line`, the quoting - # `family.py` prints its commands with: under `my work/` a bare `mv` - # moved nothing and pasted as four arguments. For the POSIX shell `mv` - # and `mkdir -p` are commands of, as the sentence above says. - lines += [f" {shell_line(step, 'posix')}" for step in steps] + # EVERY PATH QUOTED (#486): under `my work/` a bare `mv` moved nothing + # and pasted as four arguments. Quoted for the POSIX shell `mv` and + # `mkdir -p` are commands of, as the sentence above says, by the standard + # library's `shlex.join` and NOT `repo_shape.shell_line`: a holder that + # kept an older `repo_shape.py` (`update-shape.py apply --accept-local`) + # must still be able to run this file, and an import of a name its copy + # lacks would stop `make siblings`, `park` and `resume` dead. + lines += [f" {shlex.join(str(word) for word in step)}" + for step in steps] lines += ["", " Then re-run `make siblings` from the moved holder; the " "members are", diff --git a/tests/test_family_printed_quoting.py b/tests/test_family_printed_quoting.py index 829df939..621c5514 100644 --- a/tests/test_family_printed_quoting.py +++ b/tests/test_family_printed_quoting.py @@ -50,7 +50,9 @@ ) sys.path.insert(0, str(REPO / "scripts")) -from repo_shape import PYTHON, shell_line, shell_quoted # noqa: E402 +from repo_shape import ( # noqa: E402 + PYTHON, SHELL_BARE_NT, SHELL_BARE_POSIX, shell_line, shell_quoted, +) #: The two folders a holder is copied into: a space, and a space beside an #: apostrophe, which is where `sh` and PowerShell escape differently. @@ -105,7 +107,8 @@ def host_words(line: str) -> list[str]: #: to be read should not carry quotes no shell needs. ORDINARY = ["/srv/work/Atlas", "scripts/family.py", "--family-root", "members/IRRS", "family/add-irrs", - "3d46ab5b9c1dcafe0123456789abcdef01234567", "origin", "-C"] + "3d46ab5b9c1dcafe0123456789abcdef01234567", "origin", "-C", + "KEY=value"] #: Values each shell must be handed whole: a space, an apostrophe, both, a #: Windows path with both, a name no ASCII flattening may touch, the @@ -167,6 +170,36 @@ def test_a_line_is_its_words_each_quoted(): assert powershell_words(shell_line(words, "nt")) == words +#: Words that begin with `=`, which zsh (macOS's login shell) expands to a +#: command's path when bare: `=ls` reaches the command as `/bin/ls`. +LEADING_EQUALS = ["=ls", "=", "==x", "=/srv/work"] + + +@pytest.mark.parametrize("platform", ["posix", "nt"]) +@pytest.mark.parametrize("value", LEADING_EQUALS) +def test_a_word_that_begins_with_equals_is_quoted(platform, value): + quoted = shell_quoted(value, platform) + assert quoted == f"'{value}'" + assert shlex.split(quoted) == [value] + assert powershell_words(quoted) == [value] + + +@pytest.mark.skipif(os.name == "nt", reason="a POSIX shell's own reading") +@pytest.mark.parametrize("shell", ["sh", "bash", "zsh"]) +def test_a_real_shell_hands_each_word_back_whole(shell): + """The shells themselves, where the host has them: each value, quoted, + comes back from `printf` byte for byte, and zsh's `=` is among them.""" + if shutil.which(shell) is None: + pytest.skip(f"no {shell} on this host") + values = [*LEADING_EQUALS, "KEY=value", *AWKWARD, + *(chr(code) for code in range(0x21, 0x7f))] + line = "printf '%s\\0' " + shell_line(values, "posix") + argv = [shell, "-f", "-c", line] if shell == "zsh" else \ + [shell, "-c", line] + printed = subprocess.run(argv, capture_output=True, check=True).stdout + assert printed.decode("utf-8").split("\0")[:-1] == values + + @pytest.fixture(scope="module") def doctor(): spec = importlib.util.spec_from_file_location( @@ -176,15 +209,40 @@ def doctor(): return module +#: Every printable ASCII character, alone, before a word, inside one and +#: after one: a character the two alphabets disagree about cannot hide from +#: this the way it could from a hand-picked sample. +EVERY_ASCII = sorted({form for code in range(0x20, 0x7f) + for form in (chr(code), f"{chr(code)}a", + f"a{chr(code)}b", f"a{chr(code)}")}) + + @pytest.mark.parametrize("platform", ["posix", "nt"]) -@pytest.mark.parametrize("value", ORDINARY + [ - value for value in AWKWARD if value.isascii()] + [r"C:\work,old\Atlas"]) -def test_it_is_the_doctors_rule_on_any_ascii_value(doctor, platform, value): +def test_it_is_the_doctors_rule_on_any_ascii_value(doctor, platform): """ONE RULE IN TWO PLACES, held together here. `shape-doctor.py`'s `quote_arg` flattens its value to ASCII first, because its report is pure ASCII, and is otherwise this function: the same two alphabets and - the same two escapes. On an ASCII value the two must agree.""" - assert shell_quoted(value, platform) == doctor.quote_arg(value, platform) + the same two escapes. On an ASCII value the two must agree, but for a + leading `=`, which this function quotes and the doctor does not yet.""" + values = [*ORDINARY, *(value for value in AWKWARD if value.isascii()), + r"C:\work,old\Atlas", *EVERY_ASCII] + differ = {value: (shell_quoted(value, platform), + doctor.quote_arg(value, platform)) + for value in values if not value.startswith("=") + and shell_quoted(value, platform) + != doctor.quote_arg(value, platform)} + assert differ == {} + for value in EVERY_ASCII: + quoted = shell_quoted(value, platform) + reader = powershell_words if platform == "nt" else shlex.split + assert reader(quoted) == [value], quoted + + +def test_the_two_alphabets_are_the_doctors(doctor): + """The same two character sets, spelled as the same two patterns: the + doctor anchors its with `^...$`, and these are asked with `fullmatch`.""" + assert f"^{SHELL_BARE_POSIX.pattern}$" == doctor.UNQUOTED_POSIX.pattern + assert f"^{SHELL_BARE_NT.pattern}$" == doctor.UNQUOTED_NT.pattern # --- the commands, under a folder with a space in it -------------------------- @@ -215,8 +273,12 @@ def test_init_next_steps_paste_whole(tmp_path, folder): script = os.path.relpath(FAMILY.resolve(), tmp_path.resolve()) assert host_words(add) == [PYTHON, script, "add", "--family-root", str(holder), "--member", f"{ORG}/"] - cd = next(line for line in block if line.strip().startswith("cd ")) - assert host_words(cd) == ["cd", str(holder), "&&", "make", "siblings"] + at = next(index for index, line in enumerate(block) + if line.strip().startswith("cd ")) + # Two lines, no `&&`: Windows PowerShell 5.1 rejects it between two + # statements, and these lines are quoted for the host's shell. + assert host_words(block[at]) == ["cd", str(holder)] + assert block[at + 1].strip() == "make siblings" @pytest.mark.parametrize("folder", FOLDERS) @@ -332,3 +394,101 @@ def test_the_siblings_mv_lines_paste_whole_and_move_the_holder(family, assert (moved / "family.yaml").is_file() assert git("rev-parse", "--show-toplevel", cwd=moved).stdout.strip() \ == str(moved) + + +# --- a holder that kept an older `repo_shape.py` ------------------------------ + +#: What #486 added to `repo_shape.py`. A holder that kept its own copy through +#: `update-shape.py apply --accept-local scripts/repo_shape.py` (a `both` row) +#: takes the new `siblings.py` without them. +ADDED_BY_486 = ("SHELL_BARE_POSIX", "SHELL_BARE_NT", "POWERSHELL_SINGLE_QUOTES", + "shell_quoted", "shell_line") + + +def test_siblings_runs_beside_a_repo_shape_without_the_quoter(family, + tmp_path): + """`siblings.py` imported `shell_line`, so a holder whose `repo_shape.py` + predates it died with an ImportError at `make siblings`, `park` and + `resume`, and `update-shape.py check` and the validator both read the + holder as fine. It quotes its POSIX lines with `shlex.join` instead: + `--help`, the misnamed-folder warning (the lines that were quoted with + `shell_line`) and `--make park` all run beside a `repo_shape.py` that + lacks every name #486 added.""" + holder = copied_holder(family, tmp_path / "Dana's work") + library = holder / "scripts" / "repo_shape.py" + text = library.read_text(encoding="utf-8") + assert "def shell_line(" in text, "the holder's copy is not current" + library.write_text(f"{text}\ndel {', '.join(ADDED_BY_486)}\n", + encoding="utf-8") + git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-qam", + "keep an older repo_shape.py", cwd=holder) + lacks = subprocess.run( + [sys.executable, "-c", "import repo_shape, sys; sys.exit(any(" + f"hasattr(repo_shape, name) for name in {ADDED_BY_486!r}))"], + cwd=holder / "scripts", capture_output=True, text=True, check=False) + assert lacks.returncode == 0, lacks.stdout + lacks.stderr + siblings = holder / "scripts" / "siblings.py" + + helped = run_script(siblings, "--help", cwd=holder) + assert helped.returncode == 0, helped.stdout + helped.stderr + assert helped.stdout.startswith("usage: siblings.py"), helped.stdout + + warned = run_script(siblings, "--dry-run", cwd=holder) + assert warned.returncode == 0, warned.stdout + warned.stderr + assert "NOTHING WAS MOVED" in warned.stderr, warned.stderr + commands = [shlex.split(line) for line in warned.stderr.splitlines() + if line.startswith(" " * 12) and line.strip()[:3] in + ("mv ", "mkd")] + staged = holder.parent.parent / f"{NAME}.holder" + wanted = holder.parent.parent / NAME + assert commands == [["mv", str(holder), str(staged)], + ["mkdir", "-p", str(wanted)], + ["mv", str(staged), str(wanted / NAME)]] + + # No member is cloned beside this copy, so `park` runs nowhere and says + # so, exit 1: the report is the proof it ran at all. + parked = run_script(siblings, "--make", "park", "--make-arg", + "ARGS=--dry-run", cwd=holder) + said = parked.stdout + parked.stderr + assert "Traceback" not in said, said + assert "SKIPPED no clone" in said, said + assert "`make park` did not run everywhere" in said, said + + +# --- setup-project.py's hand-over --------------------------------------------- + +@pytest.fixture(scope="module") +def setup_tool(): + spec = importlib.util.spec_from_file_location( + "setup_for_quoting", REPO / "setup-project.py") + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def test_setup_quotes_by_the_same_rule(setup_tool): + """`setup-project.py` runs before any checkout exists, so it carries its + own copy of the rule, for the host it runs on. One answer, every value.""" + for value in [*ORDINARY, *AWKWARD, *LEADING_EQUALS, *EVERY_ASCII]: + assert setup_tool.shell_word(value) == shell_quoted(value), value + + +@pytest.mark.parametrize("folder", FOLDERS) +def test_setup_names_the_branch_then_the_add_each_path_whole(setup_tool, + tmp_path, + folder, capsys): + """The last of `setup-project.py`'s next commands, for a member of a + family: the `git switch -c` that `add` needs first (it refuses the + holder's tracking branch, #487), then the `add`, the holder one word in + each (#486).""" + holder = tmp_path / folder / "Orbit" / "Orbit" + landing = setup_tool.Landing(tmp_path / folder / "Orbit", "Orbit", + holder) + setup_tool.family_next_command("Sample", "demoorg", landing) + commands = [line.strip() for line in capsys.readouterr().out.splitlines() + if line.strip() and not line.strip().startswith("#")] + assert host_words(commands[0]) == ["git", "-C", str(holder), "switch", + "-c", "family/add-sample"] + assert host_words(commands[1]) == [ + setup_tool.PYTHON_CMD, "scripts/family.py", "add", "--family-root", + str(holder), "--member", "demoorg/Sample"] From c47102d0c38bcfdc8c038a2f7c62646bdb44a059 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Tue, 13 Oct 2026 01:54:46 +0000 Subject: [PATCH 5/6] family.py and the holder's docs: the holder's AGENTS-shape.md and README show the branch step before `add`, `--help` says the act prints the push, and a test holds the tracking branch to the one family.yaml declares (#487 review) - templates/family-root/AGENTS-shape.md and README.md open their add/bump/remove block with `git -C switch -c family/add-` and say each act refuses the holder's tracking branch (`family-on-tracking-branch`) and ends by printing the branch's push. - `add`, `bump` and `remove` `--help`: "ends by printing the `git push` of that branch", not "ends with", which read as the tool pushing. docs/cli.md regenerated. - A test inits a family with `--tracking-branch trunk`: `add` is refused on `trunk`, and on a branch named `main` the dry run says the commit lands there for a pull request against `trunk`, the real run commits, and its NEXT pushes `main`. Hard-coding "main" as the tracking branch now fails it. - A test holds AGENTS.md, README.md and the holder's two docs to showing the `switch -c` in the same block as, and before, the first `add`. Lane: openreposhape-5 (openRepoShape-5) Co-Authored-By: Claude Opus 5.5 --- docs/cli.md | 12 +++--- scripts/family.py | 4 +- templates/family-root/AGENTS-shape.md | 8 +++- templates/family-root/README.md | 5 ++- tests/test_family_tracking_branch.py | 58 ++++++++++++++++++++++++++- 5 files changed, 75 insertions(+), 12 deletions(-) diff --git a/docs/cli.md b/docs/cli.md index 5279e170..16579651 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -710,8 +710,8 @@ usage: family.py add [-h] --family-root FAMILY_ROOT --member ORG/PROJECT Mount and pin a member. Its ONE commit lands on the branch the holder stands on: the real run refuses the holder's tracking branch and a HEAD on no branch, -naming the `git switch -c` that makes one, and ends with the `git push` of -that branch. --dry-run runs on any branch. +naming the `git switch -c` that makes one, and ends by printing the `git push` +of that branch. --dry-run runs on any branch. options: -h, --help show this help message and exit @@ -744,8 +744,8 @@ usage: family.py bump [-h] --family-root FAMILY_ROOT --member PROJECT Move a member's gitlink and pin together. Its ONE commit lands on the branch the holder stands on: the real run refuses the holder's tracking branch and a -HEAD on no branch, naming the `git switch -c` that makes one, and ends with -the `git push` of that branch. --dry-run runs on any branch. +HEAD on no branch, naming the `git switch -c` that makes one, and ends by +printing the `git push` of that branch. --dry-run runs on any branch. options: -h, --help show this help message and exit @@ -773,8 +773,8 @@ usage: family.py remove [-h] --family-root FAMILY_ROOT --member PROJECT Unmount a member and drop its row. Its ONE commit lands on the branch the holder stands on: the real run refuses the holder's tracking branch and a HEAD -on no branch, naming the `git switch -c` that makes one, and ends with the -`git push` of that branch. --dry-run runs on any branch. +on no branch, naming the `git switch -c` that makes one, and ends by printing +the `git push` of that branch. --dry-run runs on any branch. options: -h, --help show this help message and exit diff --git a/scripts/family.py b/scripts/family.py index a597cb25..3d520793 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -4653,8 +4653,8 @@ def cmd_siblings(args) -> int: BRANCH_RULE = ("Its ONE commit lands on the branch the holder stands on: " "the real run refuses the holder's tracking branch and a HEAD " "on no branch, naming the `git switch -c` that makes one, and " - "ends with the `git push` of that branch. --dry-run runs on " - "any branch.") + "ends by printing the `git push` of that branch. --dry-run " + "runs on any branch.") def build_parser() -> argparse.ArgumentParser: diff --git a/templates/family-root/AGENTS-shape.md b/templates/family-root/AGENTS-shape.md index 8380b801..12533af8 100644 --- a/templates/family-root/AGENTS-shape.md +++ b/templates/family-root/AGENTS-shape.md @@ -24,6 +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 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: " @@ -35,8 +36,11 @@ row's **`members[].pin`** in `family.yaml` together. `make pins` (`scripts/validate-family.py`) refuses when the two disagree, and it is right to: a gitlink moved alone is a pin that names a commit the tree is not at. Do not hand-edit the `members:` block — the tool rewrites it wholesale. Land the -commit as a pull request; the member must already be scaffolded or adopted, -because a family cannot make a project out of a repository. +commit as a pull request: each refuses this holder's tracking branch +(`family-on-tracking-branch`) and names the `git switch -c` that makes a +branch, as the block's first line does, and ends by printing that branch's +`git push`. The member must already be scaffolded or adopted, because a family +cannot make a project out of a repository. `bump` takes a repeatable `--trailer ": "`, appended to the commit it writes in the order given. It composes that message itself, so whatever diff --git a/templates/family-root/README.md b/templates/family-root/README.md index a0563ec8..2edb4247 100644 --- a/templates/family-root/README.md +++ b/templates/family-root/README.md @@ -108,13 +108,16 @@ 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- 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 ``` Each writes ONE commit, with explicit pathspecs, moving the gitlink and the -pin together. Land it as a pull request. +pin together, on the branch the holder stands on: it refuses the tracking +branch, so make a branch first, as the block's first line does. Push it as the +command's last line says and land it as a pull request. ## Private members in CI diff --git a/tests/test_family_tracking_branch.py b/tests/test_family_tracking_branch.py index 5a5bef86..054c011e 100644 --- a/tests/test_family_tracking_branch.py +++ b/tests/test_family_tracking_branch.py @@ -38,7 +38,7 @@ import pytest -from conftest import git, run_script +from conftest import REPO, git, run_script #: The family fixture, its names and the work branch it grows the holder on, #: imported from the module that owns them. from test_family import ( # noqa: F401 @@ -266,3 +266,59 @@ def test_init_next_steps_make_the_branch_before_the_add(tmp_path): == TRACKING assert "it refuses main, the holder's tracking branch" in " ".join( result.stdout.split()) + + +def test_the_tracking_branch_is_the_one_family_yaml_declares(family, + tmp_path): + """NOT `main` BY NAME. A family made with `--tracking-branch trunk` + refuses `add` on `trunk`, and a branch that happens to be called `main` + is an ordinary work branch there: the dry run says the commit lands on + it for a pull request against `trunk`, the real run commits on it, and + its NEXT pushes `main` for that pull request.""" + remotes = tmp_path / "remotes" + shutil.copytree(family["remotes"], remotes, symlinks=True) + made = run_script(FAMILY, "init", "--org", ORG, "--family", "Orbit", + "--created-by", "Test Human", + "--created-on", "2026-10-13", + "--tracking-branch", "trunk", + "--local-remote-dir", str(remotes), + "--work-dir", str(tmp_path / "fam")) + assert made.returncode == 0, made.stdout + made.stderr + holder = (tmp_path / "fam" / "Orbit").resolve() + assert git("symbolic-ref", "--short", "HEAD", + cwd=holder).stdout.strip() == "trunk" + add = ["add", "--family-root", str(holder), "--member", f"{ORG}/{SPARE}", + "--local-remote-dir", str(remotes)] + + refused = run_script(FAMILY, *add) + assert refused.returncode == 2, refused.stdout + refused.stderr + assert (f"REFUSED family-on-tracking-branch: {holder} is on 'trunk', " + "which family.yaml declares as the holder's tracking branch" + in refused.stderr), refused.stderr + + git("switch", "-q", "-c", "main", cwd=holder) + dry = run_script(FAMILY, *add, "--dry-run") + assert dry.returncode == 0, dry.stdout + dry.stderr + assert ("branch main: the commit lands there, for a pull request " + "against trunk") in dry.stdout.splitlines(), dry.stdout + + done = run_script(FAMILY, *add) + assert done.returncode == 0, done.stdout + done.stderr + push, then = next_push(done.stdout) + assert push == ["git", "-C", str(holder), "push", "-u", "origin", "main"] + assert then == (" then open a pull request against trunk; never " + "push to it directly") + + +@pytest.mark.parametrize("doc", [ + "AGENTS.md", "README.md", "templates/family-root/AGENTS-shape.md", + "templates/family-root/README.md"]) +def test_each_doc_makes_the_branch_before_the_add(doc): + """The standard's own docs and the two a holder carries show the + `git -C switch -c family/add-...` in the same code block as, + and before, the first `add`: followed as written, the first command a + person runs is not one that refuses.""" + text = (REPO / doc).read_text(encoding="utf-8") + add = text.index("python3 scripts/family.py add ") + block = text[text.rindex("```", 0, add):add] + assert " switch -c family/add-" in block, block From 11ae0a87b26589a2f3028e9216ecb2c236196494 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Tue, 13 Oct 2026 02:33:52 +0000 Subject: [PATCH 6/6] family.py, family-root README.md, test_family_printed_quoting.py: the holder README sends a reader to the `NEXT` line for the push, not the act's last line, `refs/heads/` is spelt once as `HEADS_PREFIX`, and the printed-quoting test asserts actual-first throughout (#486 #487 #488 review) The holder's README said "Push it as the command's last line says". The act's last line is "then open a pull request against ; never push to it directly"; the push is the `NEXT` line before it. It now says "as its `NEXT` line says", AGENTS.md's wording. No other holder doc or AGENTS.md and README.md carry the claim. `family.py` held the literal "refs/heads/" four times, in `_holder_branch` and `_member_head` (SonarCloud python:S1192). It is now the module constant `HEADS_PREFIX`; behaviour is unchanged. `test_the_two_alphabets_are_the_doctors` put the expected value first, and the other 32 assertions in `test_family_printed_quoting.py` put the actual value first (python:S3415, anchored at line 138 but naming lines 244 and 245 as the odd ones). Both now read `actual == expected`. Lane: openreposhape-5 (openRepoShape-5) Co-Authored-By: Claude Opus 5.5 --- scripts/family.py | 8 ++++++-- templates/family-root/README.md | 4 ++-- tests/test_family_printed_quoting.py | 4 ++-- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/scripts/family.py b/scripts/family.py index 3d520793..dace16a0 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -164,6 +164,10 @@ #: -- named once so the two commits cannot spell the file differently. GITMODULES = ".gitmodules" FILE_PROTOCOL = ["-c", "protocol.file.allow=always"] +#: What `git symbolic-ref HEAD` prints before a branch's name: the one +#: spelling `_holder_branch` and `_member_head` strip to read a HEAD back as +#: the branch it is on, named once so the two cannot differ. +HEADS_PREFIX = "refs/heads/" #: `/`, the only spelling `--member` accepts. A bare name would #: have to guess an owner, and a family whose members are in another @@ -596,7 +600,7 @@ def _holder_branch(root: Path) -> str | None: "utf-8", "surrogateescape").strip() except CommandFailed: return None - return ref[len("refs/heads/"):] if ref.startswith("refs/heads/") else None + return ref[len(HEADS_PREFIX):] if ref.startswith(HEADS_PREFIX) else None #: A branch-name component the suggestion may use as it stands: dot-separated @@ -3060,7 +3064,7 @@ def _member_head(member: Path) -> tuple[str | None, str | None]: ref = git_out(["symbolic-ref", "-q", "HEAD"], cwd=member) except Refusal: ref = "" - return at, (ref[len("refs/heads/"):] if ref.startswith("refs/heads/") + return at, (ref[len(HEADS_PREFIX):] if ref.startswith(HEADS_PREFIX) else None) diff --git a/templates/family-root/README.md b/templates/family-root/README.md index 2edb4247..d7d246fd 100644 --- a/templates/family-root/README.md +++ b/templates/family-root/README.md @@ -116,8 +116,8 @@ python3 scripts/family.py remove --family-root --member Each writes ONE commit, with explicit pathspecs, moving the gitlink and the pin together, on the branch the holder stands on: it refuses the tracking -branch, so make a branch first, as the block's first line does. Push it as the -command's last line says and land it as a pull request. +branch, so make a branch first, as the block's first line does. Push it as its +`NEXT` line says and land it as a pull request. ## Private members in CI diff --git a/tests/test_family_printed_quoting.py b/tests/test_family_printed_quoting.py index 621c5514..c1af5b64 100644 --- a/tests/test_family_printed_quoting.py +++ b/tests/test_family_printed_quoting.py @@ -241,8 +241,8 @@ def test_it_is_the_doctors_rule_on_any_ascii_value(doctor, platform): def test_the_two_alphabets_are_the_doctors(doctor): """The same two character sets, spelled as the same two patterns: the doctor anchors its with `^...$`, and these are asked with `fullmatch`.""" - assert f"^{SHELL_BARE_POSIX.pattern}$" == doctor.UNQUOTED_POSIX.pattern - assert f"^{SHELL_BARE_NT.pattern}$" == doctor.UNQUOTED_NT.pattern + assert doctor.UNQUOTED_POSIX.pattern == f"^{SHELL_BARE_POSIX.pattern}$" + assert doctor.UNQUOTED_NT.pattern == f"^{SHELL_BARE_NT.pattern}$" # --- the commands, under a folder with a space in it --------------------------