From 875d8027892f3b29c74bd98fe160f5b018ea809e Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 11:53:26 +0000 Subject: [PATCH 1/3] family.py: `bump --dry-run` fetches the member, resolves `--to` and prints the `old -> new` row and its trailers without checking the member out or rewriting `family.yaml`, and `remove --dry-run` prints the row, gitlink, `.gitmodules` section and directory it would remove and changes nothing, so the module's `EXIT CODES` line is true of both (#326) `bump`, `add` and `remove` took no `--dry-run`, though the module's exit-code line said "1 nothing (a dry run prints and exits 0)", which was true of `init` and `siblings` only, and `bump-leg.py` has one. Seeing what a bump would pin meant running it: fetch, `git checkout` in the member, rewrite `family.yaml`, stage. Even #210's `nothing to do` cost the fetch and the checkout. The coordinator's rulings, as implemented: - `bump --dry-run` leaves after the fetch and before the checkout, the real run's first write, so every refusal made before that point is the same refusal, exit 2: the row, `--to`'s form, #315's `member-path` and `member-uninitialized`. It fetches, as the real run does. A fetch writes into the member's object store, which nothing the holder records, and the help and the docstring say so. `--to` is resolved as `git checkout` takes it (`^{commit}`), and a commit the member lacks is exit 2. The questions the real run asks after its checkout are also asked, of the member's objects: `project.yaml` through the same `_read_member_project` (`_member_id_at`, in a temporary directory), and `family.rewritten`. The digest is `tree_digest`, which reads objects, so it is the real run's digest, never a guess. It prints `commit -> ` and `tree`, `bump-leg.py`'s layout, then the gitlink `old -> new` and the whole row as `family.yaml` would carry it. The row comes from `_bumped_rows`, which the real run now calls too, so the printed row is the written row by construction. Exit 1 follows the real run's #210 rule, asked without the write: HEAD's gitlink is `--to`, and the rewritten manifest hashes (`git hash-object --path`, no `-w`) to HEAD's blob. A digest repair or a gitlink that moved alone is therefore a move under `--dry-run` too. A `project.yaml` that is a symlink is left to the real run, which reads through it, and the dry run says so. - `remove --dry-run` leaves after `member-path` and before `git submodule deinit`. `ls-files --error-unmatch` over the same literal pathspec fails where `deinit` fails, and otherwise lists what `git rm -r` unstages. It asks `family-members-unrecognised`, and prints the row, the gitlink, the `.gitmodules` section `git rm` drops (the one it reads last when two give the path, with the one it leaves named) and the directory. Its readers are its own, not `add`'s `_submodule_paths` and `_index_entries`, which #317 and #318 are changing. - `--trailer` with `--dry-run` prints the trailers after the row, worded as `bump-leg.py` words them, and never for `nothing to do`. - The `EXIT CODES` paragraph says which subcommands have a dry run, and a new paragraph says what each asks. `bump`'s and `remove`'s `--help` gained `--dry-run`, and `docs/cli.md` is regenerated. `add` gets none here. Its dry run follows the three changes to `cmd_add` in flight (this lane's #317 and #318, lane 3's #330), and neither `cmd_add` nor any function only it calls is touched. AGENTS.md is unchanged; a line in its family section is a follow-up. tests/test_family_dry_run.py, over `test_family.py`'s local-bare-remote family, checks "changes nothing" as bytes: the index (`ls-files -s -z`), `status --porcelain=v1 -z`, `family.yaml`, `.gitmodules`, `.git/config`, the member's HEAD and the work tree. Then the real run writes exactly what the dry run printed. Covered: a bump previewed and then made; `--to` the pin (exit 1, with and without trailers); both #210 repairs; trailers printed and then committed; six refusals compared stderr for stderr with the real run; a commit the member lacks; `member-not-a-project` at `--to`; a linked manifest; a remove previewed and then made; two sections for one path; the unrecognised manifest; and, in process, no git command that writes. Mutants, each run against the test the ruling names: the dry-run return moved after the checkout fails `member HEAD changed`; `write_members` before the return fails `family.yaml changed`; `remove`'s return moved after `git rm` fails `index changed`. Part of #326 Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Opus 5.5 --- docs/cli.md | 11 +- scripts/family.py | 305 +++++++++++++++++- tests/test_family_dry_run.py | 582 +++++++++++++++++++++++++++++++++++ 3 files changed, 890 insertions(+), 8 deletions(-) create mode 100644 tests/test_family_dry_run.py diff --git a/docs/cli.md b/docs/cli.md index b646ba8d..13c13df7 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -649,7 +649,7 @@ options: ``` usage: family.py bump [-h] --family-root FAMILY_ROOT --member PROJECT - --to COMMIT [--trailer "KEY: VALUE"] + --to COMMIT [--trailer "KEY: VALUE"] [--dry-run] options: -h, --help show this help message and exit @@ -660,6 +660,11 @@ options: --trailer "KEY: VALUE" append this `: ` line to the commit this writes; repeatable, kept in the order given + --dry-run print the row this would write, and its trailers, and + change nothing in the holder: it FETCHES the member, + as a real bump does, to resolve --to and digest it, + and checks nothing out, writes, stages and commits + nothing; exit 1 if there is nothing to do ``` #### `scripts/family.py remove` @@ -668,11 +673,15 @@ options: ``` usage: family.py remove [-h] --family-root FAMILY_ROOT --member PROJECT + [--dry-run] options: -h, --help show this help message and exit --family-root FAMILY_ROOT --member PROJECT + --dry-run print the row, the gitlink, the .gitmodules section + and the directory this would remove, and change + nothing ``` #### `scripts/family.py siblings` diff --git a/scripts/family.py b/scripts/family.py index 583037b2..f0ebc630 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -52,11 +52,25 @@ implementation of it, `templates/family-root/scripts/siblings.py`, and this subcommand runs that file rather than a second copy of its logic. -EXIT CODES: 0 done · 1 nothing (a dry run prints and exits 0) · 2 a refusal. +EXIT CODES: 0 done, or a `--dry-run` printed what it would do · 1 nothing to +do · 2 a refusal, and under `--dry-run` the same refusal. `init`, `bump`, +`remove` and `siblings` take `--dry-run`; `add` takes none yet (#326). `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 -`bump-leg.py` does for a leg, and writes no commit. +`bump-leg.py` does for a leg, and writes no commit; its dry run says the same +and exits 1 too. + +A DRY RUN ASKS WHAT THE REAL RUN ASKS, AND STOPS BEFORE ITS FIRST WRITE +(#326). `bump --dry-run` makes every refusal `bump` makes and FETCHES the +member, as a real bump does: a fetch writes into the member's object store, +which nothing in the holder records, and it is how `--to` is resolved and +digested. It then reads the member's `project.yaml` and tree from those +objects and prints the row it would write and the trailers its commit would +end with; it never checks the member out, rewrites `family.yaml`, stages or +commits. `remove --dry-run` makes every refusal `remove` makes and prints the +row, the gitlink, the `.gitmodules` section and the directory it would +remove, and runs no git command that writes. """ from __future__ import annotations @@ -1826,7 +1840,183 @@ def _nothing_to_record(root: Path, paths: list[str]) -> bool: return True +def _nothing_to_do(row: dict, commit: str) -> str: + """The sentence a bump that records nothing ends with, real or dry run: + the one `bump-leg.py` answers the same re-run with, repository first.""" + return (f"\nnothing to do: {row['repository']} is already pinned at " + f"{commit[:12]} and every fact agrees.") + + +def _bumped_rows(family: Family, row: dict, project_id: str, commit: str, + digest: str) -> tuple[dict, list[dict]]: + """`(the member's new row, every row as `bump` writes them)`. + + ONE FUNCTION FOR THE REAL RUN AND ITS DRY RUN (#326), so the row `bump + --dry-run` prints is the row a real `bump` writes by construction, not + by two spellings that happen to agree today. + """ + project = str(row["project"]) + new = _member_row(project, project_id, str(row["repository"]), commit, + digest) + return new, [new if str(r.get("project")) == project else r + for r in family.members] + + +def _member_id_at(member: Path, commit: str, project: str, + repository: str) -> str | None: + """The member's `project.yaml` id at `commit`, read from its OBJECTS + rather than from a checkout, or None where only a checkout can read it. + + WHAT `bump` READS AFTER ITS CHECKOUT, ASKED WITHOUT ONE (#326), so `bump + --dry-run` makes the same refusals (`member-not-a-project`, + `member-manifest-unreadable`, `member-bad-id` and `load_yaml`'s own) and + prints the id the real run would write. The one blob is copied into a + directory of its own and read by `_read_member_project`, the function + the checkout is read with, so every sentence is the real run's but the + file's name: ` @ :project.yaml`, where a temporary + path would name a file deleted before anybody reads the line. A + `project.yaml` that is absent, a directory or a submodule is no file, + which is what a checkout's `is_file()` says about it too. + + A SYMLINK IS None. A checkout reads THROUGH a link and the objects hold + only the link's text, so its id is the real run's to read; the dry run + says so rather than refusing a member the real run would pin. + """ + listed = git_out(["ls-tree", "-z", commit, "--", "project.yaml"], + cwd=member, binary=True) + mode, kind, oid = (listed.split(b"\t", 1)[0].split() + [b""] * 3)[:3] + if mode == b"120000": + return None + with tempfile.TemporaryDirectory(prefix="openreposhape-member-") as tmp: + if kind == b"blob": + (Path(tmp) / "project.yaml").write_bytes( + git_out(["cat-file", "blob", oid.decode("ascii")], + cwd=member, binary=True)) + return _read_member_project( + Path(tmp), project, repository, + shown=f"{repository} @ {commit[:12]}:project.yaml") + + +def _head_oid(root: Path, path: str) -> str | None: + """The object HEAD records at `path` — for a gitlink, the commit — or + None where HEAD records nothing there. A tree path, not a pathspec, so + no glob in it is ever expanded.""" + try: + return git_out(["rev-parse", "-q", "--verify", f"HEAD:{path}"], + cwd=root).lower() + except Refusal: + return None + + +def _manifest_blob(root: Path, text: str) -> str: + """The blob id `family.yaml` would have with `text` in it, as `git add` + would store it — `git hash-object --path`, which applies the holder's + own attributes and filters — and WITHOUT `-w`, so nothing is written. + Bytes on stdin, never text, for the reason `_git_input` gives.""" + command = ["git", "hash-object", "--stdin", f"--path={MANIFEST}"] + check_program(command) + proc = subprocess.run(command, cwd=str(root), + input=text.encode("utf-8"), capture_output=True, + check=False) + if proc.returncode != 0: + raise CommandFailed(command, root, proc.returncode, + (proc.stderr + proc.stdout).decode("utf-8", + "replace")) + return proc.stdout.decode("ascii").strip().lower() + + +def _print_trailers(trailers: list[str]) -> None: + """The `--trailer` lines the commit would end with, verbatim and in the + order given, which is the order `commit_trailers` appends them in. + + `bump --dry-run` prints them after the row (#326), worded as + `bump-leg.py --dry-run` prints its own, so a lane reads one form from + either tool before the lines land rather than after. Never printed for + `nothing to do`: a run that writes no commit has nothing to end. + """ + if trailers: + print(f"trailers {len(trailers)} to append, in order:") + for line in trailers: + print(f" {line}") + else: + print("trailers none") + + +def _bump_dry_run(family: Family, row: dict, path: str, commit: str, + trailers: list[str]) -> int: + """`bump --dry-run`, entered after the member is fetched and before it + is checked out (#326): every answer a real `bump` gives, and no write. + + THE REAL RUN'S QUESTIONS, IN ITS ORDER, ASKED OF THE MEMBER'S OBJECTS. + `--to` is resolved as the checkout would take it (`^{commit}`, which is + what `git checkout` accepts), and one the member does not have after the + fetch is exit 2, where the real run's checkout fails. The manifest is + read by `_member_id_at` and the digest by `tree_digest`, which reads + objects and never a checkout, so the digest printed is the one the real + run computes, never a guess. The rows are `_bumped_rows`', and + `family.rewritten` asks `family-members-unrecognised` without writing. + + `nothing to do` IS THE REAL RUN'S OWN RULE (#210), asked without the + write: the commit would record nothing when HEAD's gitlink is already + the commit `--to` resolves to AND the rewritten manifest is the blob + HEAD already has. Not `--to` against the row alone: a row naming `--to` + over a gitlink that moved alone, or a digest that does not recompute, is + a bump with something to commit, under `--dry-run` as well. + """ + project, repository = str(row["project"]), str(row.get("repository")) + member = family.root / path + try: + resolved = run(["git", "rev-parse", "--verify", + f"{commit}^{{commit}}"], cwd=member).lower() + except CommandFailed as exc: + print(exc.loudly(f"resolving {commit[:12]} in {path}, where a real " + "bump checks it out"), file=sys.stderr) + return 2 + project_id = _member_id_at(member, resolved, project, repository) + linked = project_id is None + if linked: + project_id = str(row.get("id") or "") + digest = tree_digest(member, commit) + new, rows = _bumped_rows(family, row, project_id, commit, digest) + text = family.rewritten(rows) + was = str((row.get("pin") or {}).get("commit") or "") + recorded = _head_oid(family.root, path) + + print(f"member {project} {repository} at {path}") + print(f"commit {was[:12]} -> {commit[:12]}") + print(f"tree {digest[:12]}… ({digest})") + if linked: + print("note project.yaml is a symbolic link at " + f"{resolved[:12]}, which only a checkout reads through: the " + "`id:` below is the row's own, and the real run reads it " + "through the link") + if recorded == resolved and \ + _head_oid(family.root, MANIFEST) == _manifest_blob(family.root, + text): + print(_nothing_to_do(row, commit)) + return 1 + print(f"gitlink {path} {(recorded or 'none')[:12]} -> " + f"{resolved[:12]}") + print(f"row as {MANIFEST} would carry it:") + for line in _member_lines(new): + print(line) + _print_trailers(trailers) + print(f"\n--dry-run: nothing was changed in the holder. `git fetch` ran " + f"in {path}, as a real bump's does, which is how --to was resolved " + f"and the digest above computed; the member's checkout, {MANIFEST}, " + "the index and the holder's commits are as they were.") + return 0 + + def cmd_bump(args) -> int: + """Move one member's gitlink and pin in ONE commit, or with `--dry-run` + print the row it would write and change nothing in the holder (#326). + + The refusals before the fetch are the same under `--dry-run`. The dry run + leaves after the fetch and before the checkout, which is the real run's + first write, and asks what the real run asks after it of the member's + objects instead (`_bump_dry_run`). + """ family = Family.open(args.family_root) project = checked_value("--member", repo_basename(args.member)) row = family.member(project) @@ -1861,6 +2051,8 @@ def cmd_bump(args) -> int: except CommandFailed: print(f" note: could not fetch {path} from its origin; the commit " "must already be in its object store", file=sys.stderr) + if args.dry_run: + return _bump_dry_run(family, row, path, commit, args.trailer) try: run(["git", "checkout", "-q", commit], cwd=member) except CommandFailed as exc: @@ -1872,9 +2064,7 @@ def cmd_bump(args) -> int: project_id = _read_member_project(member, project, str(row.get("repository"))) digest = tree_digest(member, commit) - rows = [(_member_row(project, project_id, str(row["repository"]), commit, - digest) if str(r.get("project")) == project else r) - for r in family.members] + _, rows = _bumped_rows(family, row, project_id, commit, digest) original = family.path.read_bytes() family.write_members(rows) # THE GITLINK IS RE-STAGED BEFORE GIT IS ASKED, as `_commit_one` does it @@ -1900,8 +2090,7 @@ def cmd_bump(args) -> int: family.path.write_bytes(original) print(f" {project:<16} {was[:12]} -> {commit[:12]} tree " f"{digest[:12]}…") - print(f"\nnothing to do: {row['repository']} is already pinned at " - f"{commit[:12]} and every fact agrees.") + print(_nothing_to_do(row, commit)) return 1 head = _commit_one( @@ -1923,6 +2112,93 @@ def cmd_bump(args) -> int: # --------------------------------------------------------------------------- +def _gitmodules_names(root: Path, path: str) -> list[str]: + """The submodule names `.gitmodules` in the work tree gives `path`, in + the order git reads them; `git rm` of that gitlink drops the LAST one's + section (#326). + + WHAT `git rm` ITSELF READS. It edits `.gitmodules` only where the work + tree has one, and looks the gitlink's path up in it; given two sections + for one path — a half-removed member, a hand edit — the one git reads + LAST is the one it drops, and the others stay. A name keeps the FIRST + `path` it is given, as git keeps it. So `remove --dry-run` names the + section a real `remove` drops, and any it leaves behind, without asking + git to write anything. Listed here rather than through `add`'s own + reader of the file, which answers a different question. + """ + if not (root / GITMODULES).is_file(): + return [] + listed = git_out(["config", "-z", "--file", GITMODULES, "--list"], + cwd=root, binary=True) + first: dict[str, str] = {} + for record in listed.split(b"\0"): + key, _, value = record.decode("utf-8", + "surrogateescape").partition("\n") + section, _, variable = key.rpartition(".") + if section.startswith("submodule.") and variable == "path": + first.setdefault(section[len("submodule."):], value) + return [name for name, given in first.items() if given == path] + + +def _remove_dry_run(family: Family, row: dict, path: str) -> int: + """`remove --dry-run` (#326): what a real `remove` would take out, and + no git command that writes. + + THE REAL RUN'S FAILURES, IN ITS ORDER. Its first git call, `git + submodule deinit -- `, fails on a path the index holds nothing at; + `ls-files --error-unmatch` over the same LITERAL pathspec fails the same + way, exit 2, and lists what `git rm -r` would unstage when it does not. + `family.rewritten` then asks `family-members-unrecognised`, which the + real run asks after its unmount, without writing. + + PRINTED: the row, each index entry under the path (the gitlink), the + `.gitmodules` section `git rm` drops for it (`_gitmodules_names`), and + the directory `deinit -f` empties and `git rm` deletes, uncommitted work + in the member included. The member's object store is not removed, by + either run, so it is not on the list. + """ + project = str(row["project"]) + root = family.root + listed = git_out(["--literal-pathspecs", "ls-files", "-s", "-z", + "--error-unmatch", "--", path], cwd=root, binary=True) + entries = [] + for record in listed.split(b"\0"): + if record: + head, _, name = record.partition(b"\t") + mode, oid = head.decode("ascii").split()[:2] + entries.append((mode, oid, name.decode("utf-8", "replace"))) + family.rewritten([r for r in family.members + if str(r.get("project")) != project]) + + pinned = str((row.get("pin") or {}).get("commit") or "") + print(f"row {project} {row.get('repository')}, pinned at " + f"{pinned[:12]}, dropped from {MANIFEST}") + for mode, oid, name in entries: + if mode == "160000": + print(f"gitlink {name} {oid[:12]}") + else: + print(f"index {name} {oid[:12]} (mode {mode})") + for name in [name for mode, _, name in entries if mode == "160000"]: + names = _gitmodules_names(root, name) + if not names: + print(f"gitmodules no section gives path {name}, so none is " + "removed") + continue + print(f'gitmodules [submodule "{names[-1]}"] path {name}') + if names[:-1]: + print(" left in place, though it gives the same " + "path: " + ", ".join(f'[submodule "{other}"]' + for other in names[:-1])) + print(f"directory {path} " + ( + "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")) + 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.") + return 0 + + def cmd_remove(args) -> int: family = Family.open(args.family_root) project = checked_value("--member", repo_basename(args.member)) @@ -1939,6 +2215,10 @@ def cmd_remove(args) -> int: # call: `path: members/*` unmounted every member under one commit naming # one, and `path: .` removed the holder's own files. path = _member_path(project, row) + # Every refusal above is the same under `--dry-run`, which leaves here, + # before the unmount, the real run's first write (#326). + if args.dry_run: + return _remove_dry_run(family, row, path) # THE MEMBER REPOSITORY IS NOT TOUCHED. Removing a member unmounts it from # this holder and nothing else: the project keeps its name, its history, # its legs and its gate. A family that could delete a project by dropping @@ -2084,11 +2364,22 @@ def build_parser() -> argparse.ArgumentParser: default=[], type=trailer_line, help="append this `: ` line to the commit " "this writes; repeatable, kept in the order given") + bump.add_argument("--dry-run", action="store_true", + help="print the row this would write, and its " + "trailers, and change nothing in the holder: it " + "FETCHES the member, as a real bump does, to " + "resolve --to and digest it, and checks nothing " + "out, writes, stages and commits nothing; exit 1 " + "if there is nothing to do") bump.set_defaults(func=cmd_bump) remove = subparsers.add_parser("remove", help="unmount a member") remove.add_argument("--family-root", required=True) remove.add_argument("--member", required=True, metavar="PROJECT") + remove.add_argument("--dry-run", action="store_true", + help="print the row, the gitlink, the .gitmodules " + "section and the directory this would remove, " + "and change nothing") remove.set_defaults(func=cmd_remove) siblings = subparsers.add_parser( diff --git a/tests/test_family_dry_run.py b/tests/test_family_dry_run.py new file mode 100644 index 00000000..4b780816 --- /dev/null +++ b/tests/test_family_dry_run.py @@ -0,0 +1,582 @@ +# SPDX-License-Identifier: Apache-2.0 +"""`family.py bump --dry-run` and `remove --dry-run` print what the real run +would do and change nothing in the holder (#326). + +`bump`, `add` and `remove` took no `--dry-run`, though the file's exit-code +paragraph said a dry run prints and exits 0 and `bump-leg.py` has one. The +only way to see what a bump would pin was to run it: fetch the member, check +it out at `--to`, rewrite `family.yaml`, stage. Even the `nothing to do` +answer #210 added cost the fetch and the checkout. + +THE RULINGS THIS FILE HOLDS THE TWO VERBS TO. `bump --dry-run` makes every +refusal `bump` makes, the same refusal and exit 2; it fetches the member, as a +real bump does, and resolves `--to` and digests it there; it prints the row as +`old -> new` and the full row it would write, and the trailers; it exits 0, or +1 for `nothing to do`; and it never checks the member out, rewrites +`family.yaml`, stages or commits. `remove --dry-run` makes every refusal +`remove` makes and prints the row, the gitlink, the `.gitmodules` section by +name and the directory, and changes nothing. `add` takes none yet: the +changes to it in flight land first. + +"CHANGES NOTHING" IS ASKED AS BYTES, the way `test_family_member_path.py` and +`test_family_add_refusals.py` ask it of a refusal: `git ls-files -s -z`, `git +status --porcelain=v1 -z`, `family.yaml`, `.gitmodules`, the holder's +`.git/config`, the member's HEAD and every byte of the work tree. Then the +REAL run goes, and writes exactly what the dry run printed: a preview that +says one thing and a run that does another is worse than none. + +The same family, rules and offline environment as `test_family.py`: members +are scaffolded into local bare repositories, nothing is created on GitHub, a +`gh` on PATH logs and exits 97, and the operator's git configuration is a file +of the test's own. No test pushes to the fixture's own remotes. +""" + +from __future__ import annotations + +import os +import sys +from pathlib import Path + +import pytest + +from conftest import run_script +#: The trailer lines a lane's run lands, from the file that owns them. +from test_commit_trailers import TRAILERS, trailers_of +#: The family fixture, its names and the offline environment, imported from +#: the modules that own them rather than rebuilt: a holder made any other way +#: would test a different holder. +from test_family import ( # noqa: F401 + FAMILY, ORG, family, family_module, manifest, nothing_to_do_line, + validate, +) +from test_family_add_refusals import ( # noqa: F401 + holder, offline, refusal, sections, +) +from test_family_member_path import ( + GLOB, MEMBER, OTHER, hand_edit, isolated, older, raw, recorded, +) + +#: The header `bump --dry-run` prints above the row it would write. +ROW_HEADER = "row as family.yaml would carry it:" + +#: The order a test compares a holder's state in. The FIRST key that differs +#: is the one a failure names, so each verb leads with the fact its own first +#: write would change: the member's checkout for `bump`, the index for +#: `remove`. +BUMP_ORDER = ("member HEAD", "family.yaml", "index", "status", ".gitmodules", + ".git/config", "work tree", "HEAD") +REMOVE_ORDER = ("index", ".gitmodules", "family.yaml", "work tree", + ".git/config", "status", "member HEAD", "HEAD") + + +def work_tree(root: Path) -> dict: + """Every directory and file under `root` but its own `.git/`, each file + as its bytes (a symlink as its target): the holder's work tree, the + members' checkouts included.""" + found: dict = {} + for dirpath, dirnames, filenames in os.walk(root): + here = Path(dirpath) + if here == root: + dirnames[:] = [name for name in dirnames if name != ".git"] + for name in dirnames: + found[(here / name).relative_to(root).as_posix() + "/"] = None + for name in filenames: + target = here / name + found[target.relative_to(root).as_posix()] = ( + os.readlink(target).encode("utf-8") if target.is_symlink() + else target.read_bytes()) + return found + + +def state(root: Path, env: dict, member: str = MEMBER) -> dict: + """Everything a dry run must leave exactly as it found it, as bytes.""" + mount = root / "members" / member + return { + "HEAD": raw(root, env, "rev-parse", "HEAD").stdout, + "index": raw(root, env, "ls-files", "-s", "-z").stdout, + "status": raw(root, env, "status", "--porcelain=v1", "-z").stdout, + "family.yaml": (root / "family.yaml").read_bytes(), + ".gitmodules": (root / ".gitmodules").read_bytes(), + ".git/config": (root / ".git" / "config").read_bytes(), + "member HEAD": (raw(mount, env, "rev-parse", "HEAD").stdout + if mount.is_dir() else None), + "work tree": work_tree(root), + } + + +def assert_as_it_was(root: Path, env: dict, before: dict, order) -> None: + """`root` is in the state `before` recorded, compared key by key in + `order`, so a failure names the first fact that moved.""" + now = state(root, env) + for key in order: + assert now[key] == before[key], f"{key} changed" + + +def pinned_at(root: Path, env: dict, member: str = MEMBER) -> str: + """The commit HEAD's gitlink records for `member`.""" + return raw(root, env, "rev-parse", f"HEAD:members/{member}" + ).stdout.decode("ascii").strip() + + +def bump(root: Path, env: dict, to: str, *extra: str): + return run_script(FAMILY, "bump", "--family-root", str(root), "--member", + MEMBER, "--to", to, *extra, env=env) + + +def remove(root: Path, env: dict, *extra: str): + return run_script(FAMILY, "remove", "--family-root", str(root), + "--member", MEMBER, *extra, env=env) + + +def printed_row(stdout: str) -> list[str]: + """The row `bump --dry-run` printed under `ROW_HEADER`, line by line, as + `family.yaml` would carry it.""" + lines = stdout.splitlines() + row = [] + for line in lines[lines.index(ROW_HEADER) + 1:]: + if not line.startswith((" - ", " ")): + break + row.append(line) + return row + + +def commit_in_mount(root: Path, env: dict, change) -> str: + """A commit in `MEMBER`'s mount that `change(mount)` makes, and the mount + put back at the commit the holder pins: the new commit is in the member's + objects and nowhere else, and the holder is clean. Returns its sha.""" + mount = root / "members" / MEMBER + pinned = raw(mount, env, "rev-parse", "HEAD").stdout.decode().strip() + change(mount) + raw(mount, env, "add", "-A") + raw(mount, env, "commit", "-q", "-m", "A commit only this mount has") + made = raw(mount, env, "rev-parse", "HEAD").stdout.decode().strip() + raw(mount, env, "checkout", "-q", pinned) + assert raw(root, env, "status", "--porcelain=v1", "-z").stdout == b"", ( + "premise: the holder is clean") + return made + + +# --- bump -------------------------------------------------------------------- + +def test_bump_dry_run_prints_the_move_and_a_real_bump_writes_exactly_that( + family, holder, offline, tmp_path): + """THE RULING'S FIRST TEST. A real move, previewed: the `old -> new` line, + the gitlink, the full row, and not one byte of the holder or the member's + checkout moved; no `gh` call, and the scratch copy the manifest is read + through is gone. Then the real bump writes exactly the row the dry run + printed, digest and all, and the holder validates.""" + env = isolated(offline, tmp_path) + target, pinned = older(family), pinned_at(holder, env) + assert target != pinned, "premise: a bump with something to move" + before = state(holder, env) + + dry = bump(holder, env, target, "--dry-run") + + # FIRST, WHATEVER IT PRINTED: nothing moved. + assert_as_it_was(holder, env, before, BUMP_ORDER) + offline.assert_left_nothing() + assert dry.returncode == 0, dry.stdout + dry.stderr + assert dry.stderr == "" + lines = dry.stdout.splitlines() + assert lines[:2] == [ + f"member {MEMBER} {ORG}/{MEMBER} at members/{MEMBER}", + f"commit {pinned[:12]} -> {target[:12]}"] + assert f"gitlink members/{MEMBER} {pinned[:12]} -> {target[:12]}" \ + in lines + row = printed_row(dry.stdout) + assert row[0] == f" - project: {MEMBER}" + assert f' commit: "{target}"' in row + assert lines[-1].startswith("--dry-run: nothing was changed in the " + "holder. `git fetch` ran in members/") + + real = bump(holder, env, target) + + assert real.returncode == 0, real.stdout + real.stderr + text = (holder / "family.yaml").read_bytes().decode("utf-8") + assert "\n".join(row) + "\n" in text, "the row printed is the row written" + written = {r["project"]: r for r in manifest(holder)["members"]}[MEMBER] + digest = written["pin"]["tree_sha256"] + assert written["pin"]["commit"] == target + assert f' tree_sha256: "{digest}"' in row + assert f"({digest})" in lines[2], "the tree line carries the same digest" + assert pinned_at(holder, env) == target + assert validate(holder).returncode == 0 + + +@pytest.mark.parametrize("extra", [ + (), ("--trailer", TRAILERS[0], "--trailer", TRAILERS[1])], + ids=["plain", "with-trailers"]) +def test_bump_dry_run_to_the_pinned_commit_is_nothing_to_do( + holder, offline, tmp_path, extra): + """`--to` the pinned commit: exit 1, the real run's own `nothing to do` + sentence, no row and no trailers (a run that writes no commit has none + to end), and nothing changed.""" + env = isolated(offline, tmp_path) + pinned = pinned_at(holder, env) + before = state(holder, env) + + result = bump(holder, env, pinned, "--dry-run", *extra) + + assert result.returncode == 1, result.stdout + result.stderr + assert result.stderr == "" + assert nothing_to_do_line(MEMBER, pinned) in result.stdout + assert ROW_HEADER not in result.stdout + assert "trailers" not in result.stdout + assert "--dry-run:" not in result.stdout + assert_as_it_was(holder, env, before, BUMP_ORDER) + + +def hand_edit_digest(root: Path, env: dict) -> None: + """`MEMBER`'s `tree_sha256` hand-edited and committed: the row names the + pinned commit and its digest does not recompute.""" + row = {r["project"]: r for r in manifest(root)["members"]}[MEMBER] + manifest_path = root / "family.yaml" + manifest_path.write_bytes(manifest_path.read_bytes().replace( + row["pin"]["tree_sha256"].encode("ascii"), b"b" * 64)) + raw(root, env, "add", "--", "family.yaml") + raw(root, env, "commit", "-q", "-m", "Hand-edit a digest") + + +def move_the_gitlink_alone(root: Path, env: dict) -> None: + """`MEMBER`'s gitlink moved and committed, its row left naming the + pinned commit.""" + mount = root / "members" / MEMBER + raw(mount, env, "commit", "-q", "--allow-empty", "-m", "Moved alone") + raw(root, env, "add", "--", f"members/{MEMBER}") + raw(root, env, "commit", "-q", "-m", "Move the gitlink and not the pin") + + +@pytest.mark.parametrize("break_it", [hand_edit_digest, + move_the_gitlink_alone], + ids=["digest", "gitlink"]) +def test_bump_dry_run_of_a_repair_to_the_pinned_commit_is_a_move( + holder, offline, tmp_path, break_it): + """THE REAL RUN'S `nothing to do` RULE (#210), NOT `--to` AGAINST THE ROW. + `validate-family.py` names `bump --to ` as the repair + for a digest that does not recompute and for a gitlink that moved alone, + and in both the row already names `--to`. The real bump commits the + repair; its dry run says it would, exit 0 and the row, and the real run + then agrees.""" + env = isolated(offline, tmp_path) + pinned = {r["project"]: r for r in manifest(holder)["members"] + }[MEMBER]["pin"]["commit"] + break_it(holder, env) + assert validate(holder).returncode == 1, "premise: the holder is red" + before = state(holder, env) + + dry = bump(holder, env, pinned, "--dry-run") + + assert dry.returncode == 0, dry.stdout + dry.stderr + assert "nothing to do" not in dry.stdout + row = printed_row(dry.stdout) + assert_as_it_was(holder, env, before, BUMP_ORDER) + + real = bump(holder, env, pinned) + + assert real.returncode == 0, real.stdout + real.stderr + assert "\n".join(row) + "\n" in (holder / "family.yaml").read_text( + encoding="utf-8") + assert validate(holder).returncode == 0 + + +def test_bump_dry_run_prints_the_trailers_the_real_commit_ends_with( + family, holder, offline, tmp_path): + """`--trailer` with `--dry-run` prints the lines, in order, after the row + and before the closing line, worded as `bump-leg.py --dry-run` words + them; with none it says so. The real run then ends its commit with + exactly those lines.""" + env = isolated(offline, tmp_path) + target = older(family) + trailers = ("--trailer", TRAILERS[0], "--trailer", TRAILERS[1]) + + dry = bump(holder, env, target, "--dry-run", *trailers) + plain = bump(holder, env, target, "--dry-run") + + assert dry.returncode == plain.returncode == 0, dry.stderr + plain.stderr + lines = dry.stdout.splitlines() + at = lines.index("trailers 2 to append, in order:") + assert lines[at + 1:at + 3] == [f" {TRAILERS[0]}", f" {TRAILERS[1]}"] + assert lines.index(ROW_HEADER) < at + assert lines[-1].startswith("--dry-run: ") + assert "trailers none" in plain.stdout.splitlines() + + real = bump(holder, env, target, *trailers) + + assert real.returncode == 0, real.stdout + real.stderr + assert trailers_of(holder) == list(TRAILERS) + + +def deinit_member(root: Path, env: dict) -> None: + """`MEMBER`'s mount de-initialized: no checkout to resolve `--to` in.""" + raw(root, env, "submodule", "deinit", "-q", "-f", "--", + f"members/{MEMBER}") + + +def hand_edit_path(root: Path, env: dict) -> None: + """#315's `member-path`: `MEMBER`'s `path:` hand-edited to a glob.""" + hand_edit(root, MEMBER, GLOB) + + +#: Each refusal `bump` or `remove` makes before its first write, and how a +#: test brings the holder to it: `(verb, --member, --to, set-up, code)`. +#: `None` for `bump`'s `--to` is the member's older commit, a real move. +REFUSALS = { + "bump-unknown": ("bump", "Nope", "0" * 40, None, "member-unknown"), + "bump-not-a-commit": ("bump", MEMBER, "v1.0.0", None, + "member-to-not-a-commit"), + "bump-member-path": ("bump", MEMBER, None, hand_edit_path, + "member-path"), + "bump-uninitialized": ("bump", MEMBER, None, deinit_member, + "member-uninitialized"), + "remove-unknown": ("remove", "Nope", None, None, "member-unknown"), + "remove-member-path": ("remove", MEMBER, None, hand_edit_path, + "member-path"), +} + + +@pytest.mark.parametrize("verb, member, to, set_up, code", + REFUSALS.values(), ids=REFUSALS) +def test_a_refusal_is_the_same_refusal_under_dry_run( + family, holder, offline, tmp_path, verb, member, to, set_up, code): + """THE RULING: a refusal under `--dry-run` is the same refusal, exit 2. + The real run and the dry run, over the same holder, print the same two + lines on stderr and nothing on stdout, and leave the holder as it was — + #315's `member-path` over a hand-edited `path:` among them, the edit + still sitting uncommitted in the work tree.""" + env = isolated(offline, tmp_path) + if set_up is not None: + set_up(holder, env) + args = [verb, "--family-root", str(holder), "--member", member] + if verb == "bump": + args += ["--to", to or older(family)] + before = state(holder, env) + + real = run_script(FAMILY, *args, env=env) + dry = run_script(FAMILY, *args, "--dry-run", env=env) + + assert dry.returncode == real.returncode == 2, dry.stdout + dry.stderr + assert dry.stdout == real.stdout == "" + assert dry.stderr == real.stderr + assert refusal(dry.stderr)[0].startswith(f"REFUSED {code}: ") + assert_as_it_was(holder, env, before, + BUMP_ORDER if verb == "bump" else REMOVE_ORDER) + + +def test_bump_dry_run_of_a_commit_the_member_does_not_have_exits_2( + holder, offline, tmp_path): + """A `--to` the member's objects do not hold after the fetch is exit 2, + as the real run's checkout of it is, and neither prints a row.""" + env = isolated(offline, tmp_path) + missing = "deadbeef" * 5 + before = state(holder, env) + + dry = bump(holder, env, missing, "--dry-run") + real = bump(holder, env, missing) + + assert dry.returncode == real.returncode == 2, dry.stdout + dry.stderr + assert dry.stdout == real.stdout == "" + assert dry.stderr.startswith( + f"REFUSED resolving {missing[:12]} in members/{MEMBER}, where a real " + "bump checks it out.") + assert_as_it_was(holder, env, before, BUMP_ORDER) + + +def test_bump_dry_run_reads_the_members_manifest_at_to_and_refuses_as_bump( + holder, offline, tmp_path): + """WHAT THE REAL RUN ASKS AFTER ITS CHECKOUT, ASKED OF THE OBJECTS. A + `--to` whose tree has no `project.yaml` is `member-not-a-project` from + both runs, word for word; the dry run asks it without moving the + member's checkout, which the real run leaves at `--to`.""" + env = isolated(offline, tmp_path) + target = commit_in_mount( + holder, env, lambda mount: (mount / "project.yaml").unlink()) + before = state(holder, env) + + dry = bump(holder, env, target, "--dry-run") + + assert dry.returncode == 2, dry.stdout + dry.stderr + assert dry.stdout == "" + assert refusal(dry.stderr)[0].startswith("REFUSED member-not-a-project: ") + assert_as_it_was(holder, env, before, BUMP_ORDER) + real = bump(holder, env, target) + assert real.returncode == 2 + assert real.stderr == dry.stderr + + +@pytest.mark.skipif(sys.platform == "win32", + reason="a symbolic link in a checkout needs privileges " + "on Windows that a runner does not grant") +def test_bump_dry_run_leaves_a_linked_manifest_to_the_real_run( + holder, offline, tmp_path): + """A `project.yaml` that is a SYMLINK at `--to` is only its link text in + the objects, and a checkout reads through it. The dry run does not + refuse it — the real run pins it — and says the `id:` it prints is the + row's own; the real run then writes the row it printed.""" + env = isolated(offline, tmp_path) + + def link(mount: Path) -> None: + (mount / "project.yaml").rename(mount / "manifest.yaml") + (mount / "project.yaml").symlink_to("manifest.yaml") + + target = commit_in_mount(holder, env, link) + + dry = bump(holder, env, target, "--dry-run") + + assert dry.returncode == 0, dry.stdout + dry.stderr + assert any(line.startswith("note project.yaml is a symbolic link") + for line in dry.stdout.splitlines()) + row = printed_row(dry.stdout) + assert " id: irrs" in row + real = bump(holder, env, target) + assert real.returncode == 0, real.stdout + real.stderr + assert "\n".join(row) + "\n" in (holder / "family.yaml").read_text( + encoding="utf-8") + + +# --- remove ------------------------------------------------------------------ + +def test_remove_dry_run_prints_what_a_real_remove_takes_and_changes_nothing( + holder, offline, tmp_path): + """THE RULING'S REMOVE TEST. The row, the gitlink, the `.gitmodules` + section by name and the directory, printed; the index, `.gitmodules`, + `family.yaml`, the work tree and `.git/config` byte for byte as they + were. Then the real remove takes out exactly those four and leaves the + neighbour's row, gitlink, section and directory alone.""" + env = isolated(offline, tmp_path) + gitlink = pinned_at(holder, env) + before = state(holder, env) + + dry = remove(holder, env, "--dry-run") + + # FIRST, WHATEVER IT PRINTED: nothing moved. + assert_as_it_was(holder, env, before, REMOVE_ORDER) + assert dry.returncode == 0, dry.stdout + dry.stderr + assert dry.stderr == "" + lines = dry.stdout.splitlines() + assert lines[:3] == [ + f"row {MEMBER} {ORG}/{MEMBER}, pinned at {gitlink[:12]}, " + "dropped from family.yaml", + f"gitlink members/{MEMBER} {gitlink[:12]}", + f'gitmodules [submodule "members/{MEMBER}"] path members/{MEMBER}', + ] + assert lines[3].startswith(f"directory members/{MEMBER} deleted with " + "everything in it") + assert lines[-1].startswith("--dry-run: nothing was changed") + + real = remove(holder, env) + + assert real.returncode == 0, real.stdout + real.stderr + assert [r["project"] for r in manifest(holder)["members"]] == [OTHER] + assert raw(holder, env, "ls-files", "-s", "--", f"members/{MEMBER}" + ).stdout == b"" + assert raw(holder, env, "ls-files", "-s", "--", f"members/{OTHER}" + ).stdout.startswith(b"160000 ") + assert sections((holder / ".gitmodules").read_text(encoding="utf-8")) \ + == [f"members/{OTHER}"] + assert not (holder / "members" / MEMBER).exists() + assert (holder / "members" / OTHER / "project.yaml").is_file() + assert validate(holder).returncode == 0 + + +def test_remove_dry_run_names_the_section_git_drops_and_the_one_it_leaves( + holder, offline, tmp_path): + """TWO SECTIONS GIVE ONE PATH — a half-removed member, a hand edit — + and `git rm` drops only the one it reads last. The dry run names that + one, and the one left behind, and the real remove drops exactly the + section it named: the dry run says what git will do, not what a reader + of the file might expect.""" + env = isolated(offline, tmp_path) + with (holder / ".gitmodules").open("ab") as gitmodules: + gitmodules.write(f'[submodule "ghost"]\n\tpath = members/{MEMBER}\n' + "\turl = ../elsewhere\n".encode("utf-8")) + raw(holder, env, "add", "--", ".gitmodules") + raw(holder, env, "commit", "-q", "-m", "A second section for one path") + before = state(holder, env) + + dry = remove(holder, env, "--dry-run") + + assert dry.returncode == 0, dry.stdout + dry.stderr + lines = dry.stdout.splitlines() + assert f'gitmodules [submodule "ghost"] path members/{MEMBER}' in lines + assert (" left in place, though it gives the same path: " + f'[submodule "members/{MEMBER}"]') in lines + assert_as_it_was(holder, env, before, REMOVE_ORDER) + + real = remove(holder, env) + + assert real.returncode == 0, real.stdout + real.stderr + assert sections((holder / ".gitmodules").read_text(encoding="utf-8")) \ + == [f"members/{MEMBER}", f"members/{OTHER}"] + + +# --- both -------------------------------------------------------------------- + +@pytest.mark.parametrize("verb", ["bump", "remove"]) +def test_a_dry_run_asks_the_manifest_refusal_the_real_run_asks_after_it_writes( + family, holder, offline, tmp_path, verb): + """`family-members-unrecognised` is asked by the real `bump` after its + checkout and by the real `remove` after its unmount. The dry run asks it + too, exit 2, before anything is written. The hand edit is the one + `test_family_add_refusals.py` makes: a comment on the `members:` line, + which YAML reads past and the line rewrite does not, left uncommitted.""" + env = isolated(offline, tmp_path) + manifest_path = holder / "family.yaml" + text = manifest_path.read_bytes().decode("utf-8") + assert text.count("\nmembers:\n") == 1, "fixture drift" + manifest_path.write_bytes(text.replace( + "\nmembers:\n", "\nmembers: # edited by hand\n").encode("utf-8")) + before = state(holder, env) + extra = ["--to", older(family)] if verb == "bump" else [] + + dry = run_script(FAMILY, verb, "--family-root", str(holder), "--member", + MEMBER, *extra, "--dry-run", env=env) + + assert dry.returncode == 2, dry.stdout + dry.stderr + assert "REFUSED family-members-unrecognised: " in dry.stderr + assert_as_it_was(holder, env, before, + BUMP_ORDER if verb == "bump" else REMOVE_ORDER) + + +#: Git commands that write: to the index, a work tree, a ref or the history. +#: A dry run runs none of them. +WRITERS = {"add", "checkout", "commit", "merge", "mv", "pull", "push", + "reset", "restore", "rm", "stash", "submodule", "switch", + "update-index", "update-ref", "write-tree"} + + +def subcommand_of(argv: list[str]) -> str: + """The git subcommand in `argv`, past git's own options.""" + words = iter(argv[1:]) + for word in words: + if word in ("-c", "-C"): + next(words) + elif not word.startswith("-"): + return word + raise AssertionError(f"no subcommand in {argv}") + + +@pytest.mark.parametrize("verb", ["bump", "remove"]) +def test_a_dry_run_runs_no_git_command_that_writes( + family, holder, offline, tmp_path, monkeypatch, verb): + """"Changes nothing" is what the holder shows; this is the other half, + that nothing which writes is RUN. Every program the dry run starts is + recorded, in this process: no checkout, no stage, no commit, no `git + submodule`, and a `git config` only to list. `bump`'s does fetch, as the + help and the docstring say it does, and `remove`'s does not.""" + module = family_module() + args = [verb, "--family-root", str(holder), "--member", MEMBER, + *(["--to", older(family)] if verb == "bump" else []), "--dry-run"] + for key, value in isolated(offline, tmp_path).items(): + monkeypatch.setenv(key, value) + calls = recorded(monkeypatch) + + assert module.main(args) == 0 + + gits = [argv for argv in calls if Path(argv[0]).name == "git"] + assert gits, "premise: the calls were recorded" + ran = {subcommand_of(argv) for argv in gits} + assert not ran & WRITERS, sorted(ran & WRITERS) + for argv in gits: + if subcommand_of(argv) == "config": + assert "--list" in argv, argv + assert ("fetch" in ran) is (verb == "bump") From f54e20c42f084083b0cb5da79e3ca48d44dfe782 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 12:31:56 +0000 Subject: [PATCH 2/3] family.py: `remove --dry-run` prints a path the index does not hold as the real run prints a failed git command, with no leg remediation, and names exactly the `.gitmodules` sections and the registration a real `git rm` and `deinit` drop, by git's own reading of the work-tree file; fix round on the review of 875d802 (#326) The review of 875d802 (MERGEABLE WITH NITS) found two SHOULDs and eight nits. S1. Over a row whose gitlink the index no longer holds, the real `remove` fails at `git submodule deinit` and prints the command and git's line, exit 2. The dry run failed in `git_out`, whose `Refusal` carries `repo_shape`'s default remediation: run `validate-pins.py`, advance `contracts/-pin.yaml`. A holder has neither. The dry runs now ask git through `_git_bytes`, whose failure is a `CommandFailed`. `remove --dry-run` prints the failed listing as `_bump_dry_run` prints its `rev-parse`: the exact command and git's own `did not match any file(s) known to git`, exit 2. The manifest read, the `.gitmodules` and config listings and `hash-object` go the same way. S2. `_gitmodules_names` claimed a name keeps the FIRST `path` it is given. That is git's rule for a `.gitmodules` read from a tree. The work-tree file, which `git rm` reads, is read with overwrite on (git 2.43, `gitmodules_cb`), and three things follow: - a name keeps its LAST path; - a name moving away takes the old path out of git's path map, whichever name held it; - a suspicious name (`check_submodule_name`, e.g. `../ghost`) is ignored. The model now follows all three over `git config --list`'s reading of the file. It names the one name whose every section `git rm` drops, or none, and the other names whose sections still say the path. Probed, then tested against a real remove: ghost given ELSEWHERE and then the member's path drops both ghost sections; the reverse drops nothing; `../ghost` is ignored and the member's own section is dropped. N1: the docstrings say "every refusal made before its first write", and name what is not asked: a member's local changes the checkout would refuse, and an unmerged `.gitmodules`. N2: after a failed fetch the closing line says `--to` was resolved from what the member's object store already held. N5: the tests compare the holder's own `.git/objects`, and the in-process test treats `hash-object -w` and any non-listing `config` as writes, with a `nothing to do` case that asks `hash-object`. N8: `remove --dry-run` prints the `.git/config` registration `deinit` drops, looked up by the same name, only where the holder has one. N3, N4 and N6 are follow-ups in the PR body. New tests: a row the index does not hold (dry and real both exit 2 on git's line, nothing moves, no leg remediation); four `.gitmodules` hand edits, each compared with a real remove; a failed fetch; the registration in the remove preview; a `nothing to do` case in the in-process test. Mutants: MJ (no `--error-unmatch`) fails the index test; MI (`hash-object -w`) fails the in-process `nothing to do` case; a name keeping its first path fails two of the four hand edits; a suspicious name read like any other fails the third. Part of #326 Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Opus 5.5 --- scripts/family.py | 228 ++++++++++++++++++++++++----------- tests/test_family_dry_run.py | 201 +++++++++++++++++++++++++----- 2 files changed, 330 insertions(+), 99 deletions(-) diff --git a/scripts/family.py b/scripts/family.py index f0ebc630..e1c2ce70 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -53,8 +53,9 @@ subcommand runs that file rather than a second copy of its logic. EXIT CODES: 0 done, or a `--dry-run` printed what it would do · 1 nothing to -do · 2 a refusal, and under `--dry-run` the same refusal. `init`, `bump`, -`remove` and `siblings` take `--dry-run`; `add` takes none yet (#326). +do · 2 a refusal, and a refusal made before the first write is the same +refusal under `--dry-run`. `init`, `bump`, `remove` and `siblings` take +`--dry-run`; `add` takes none yet (#326). `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 @@ -62,15 +63,23 @@ and exits 1 too. A DRY RUN ASKS WHAT THE REAL RUN ASKS, AND STOPS BEFORE ITS FIRST WRITE -(#326). `bump --dry-run` makes every refusal `bump` makes and FETCHES the -member, as a real bump does: a fetch writes into the member's object store, -which nothing in the holder records, and it is how `--to` is resolved and -digested. It then reads the member's `project.yaml` and tree from those -objects and prints the row it would write and the trailers its commit would -end with; it never checks the member out, rewrites `family.yaml`, stages or -commits. `remove --dry-run` makes every refusal `remove` makes and prints the -row, the gitlink, the `.gitmodules` section and the directory it would -remove, and runs no git command that writes. +(#326). `bump --dry-run` makes every refusal `bump` makes before its first +write and FETCHES the member, as a real bump does: a fetch writes into the +member's object store, which nothing in the holder records, and it is how +`--to` is resolved and digested. It then reads the member's `project.yaml` +and tree from those objects, so the refusals the real run makes after its +checkout are asked too, and prints the row it would write and the trailers +its commit would end with; it never checks the member out, rewrites +`family.yaml`, stages or commits. `remove --dry-run` makes every refusal +`remove` makes before its first write and prints the row, the gitlink, the +`.gitmodules` section, the holder's registration of the member and the +directory it would remove, and runs no git command that writes. + +WHAT ONLY THE WRITE ITSELF CAN ANSWER IS NOT ASKED. A member whose local +changes `git checkout` would refuse to overwrite gets exit 0 and a row from +`bump --dry-run`, and exit 2 from the real `bump`'s checkout; an unmerged +`.gitmodules` is refused by the real `remove`'s `git rm` and not by its dry +run. Each is git's own refusal, made with the holder unchanged. """ from __future__ import annotations @@ -1882,16 +1891,15 @@ def _member_id_at(member: Path, commit: str, project: str, only the link's text, so its id is the real run's to read; the dry run says so rather than refusing a member the real run would pin. """ - listed = git_out(["ls-tree", "-z", commit, "--", "project.yaml"], - cwd=member, binary=True) + listed = _git_bytes(["ls-tree", "-z", commit, "--", "project.yaml"], + member) mode, kind, oid = (listed.split(b"\t", 1)[0].split() + [b""] * 3)[:3] if mode == b"120000": return None with tempfile.TemporaryDirectory(prefix="openreposhape-member-") as tmp: if kind == b"blob": (Path(tmp) / "project.yaml").write_bytes( - git_out(["cat-file", "blob", oid.decode("ascii")], - cwd=member, binary=True)) + _git_bytes(["cat-file", "blob", oid.decode("ascii")], member)) return _read_member_project( Path(tmp), project, repository, shown=f"{repository} @ {commit[:12]}:project.yaml") @@ -1908,21 +1916,35 @@ def _head_oid(root: Path, path: str) -> str | None: return None -def _manifest_blob(root: Path, text: str) -> str: - """The blob id `family.yaml` would have with `text` in it, as `git add` - would store it — `git hash-object --path`, which applies the holder's - own attributes and filters — and WITHOUT `-w`, so nothing is written. - Bytes on stdin, never text, for the reason `_git_input` gives.""" - command = ["git", "hash-object", "--stdin", f"--path={MANIFEST}"] +def _git_bytes(args: list[str], cwd: Path, data: bytes | None = None) -> bytes: + """`git ` in `cwd`, its output as BYTES, `data` (if any) on stdin + as bytes for the reason `_git_input` gives; a failure is a + `CommandFailed`. + + WHAT THE DRY RUNS ASK GIT WITH (#326). `git_out`'s failure is a + `Refusal` that carries `repo_shape`'s default remediation, which is a + LEG's (run `validate-pins.py`, advance `contracts/-pin.yaml`), and + a holder has neither. A `CommandFailed` is printed as every failed git + command in this file is: the exact command, where, and git's own words, + exit 2, which is how the real run reports the same failure. + """ + command = ["git", *args] check_program(command) - proc = subprocess.run(command, cwd=str(root), - input=text.encode("utf-8"), capture_output=True, - check=False) + proc = subprocess.run(command, cwd=str(cwd), input=data, + capture_output=True, check=False) if proc.returncode != 0: - raise CommandFailed(command, root, proc.returncode, + raise CommandFailed(command, cwd, proc.returncode, (proc.stderr + proc.stdout).decode("utf-8", "replace")) - return proc.stdout.decode("ascii").strip().lower() + return proc.stdout + + +def _manifest_blob(root: Path, text: str) -> str: + """The blob id `family.yaml` would have with `text` in it, as `git add` + would store it — `git hash-object --path`, which applies the holder's + own attributes and filters — and WITHOUT `-w`, so nothing is written.""" + return _git_bytes(["hash-object", "--stdin", f"--path={MANIFEST}"], root, + text.encode("utf-8")).decode("ascii").strip().lower() def _print_trailers(trailers: list[str]) -> None: @@ -1943,9 +1965,10 @@ def _print_trailers(trailers: list[str]) -> None: def _bump_dry_run(family: Family, row: dict, path: str, commit: str, - trailers: list[str]) -> int: - """`bump --dry-run`, entered after the member is fetched and before it - is checked out (#326): every answer a real `bump` gives, and no write. + trailers: list[str], fetched: bool) -> 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. THE REAL RUN'S QUESTIONS, IN ITS ORDER, ASKED OF THE MEMBER'S OBJECTS. `--to` is resolved as the checkout would take it (`^{commit}`, which is @@ -1962,6 +1985,11 @@ def _bump_dry_run(family: Family, row: dict, path: str, commit: str, HEAD already has. Not `--to` against the row alone: a row naming `--to` over a gitlink that moved alone, or a digest that does not recompute, is a bump with something to commit, under `--dry-run` as well. + + THE CHECKOUT'S OWN QUESTION IS NOT ASKED. Local changes in the member + that `git checkout` would refuse to overwrite are found by the real run, + exit 2, while the dry run prints the row and exits 0: only the checkout + knows which of them the move touches. """ project, repository = str(row["project"]), str(row.get("repository")) member = family.root / path @@ -2001,10 +2029,14 @@ def _bump_dry_run(family: Family, row: dict, path: str, commit: str, for line in _member_lines(new): print(line) _print_trailers(trailers) - print(f"\n--dry-run: nothing was changed in the holder. `git fetch` ran " - f"in {path}, as a real bump's does, which is how --to was resolved " - f"and the digest above computed; the member's checkout, {MANIFEST}, " - "the index and the holder's commits are as they were.") + 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 + f"The fetch of {path} failed, as the note says, so --to was " + "resolved and the digest above computed from what its object " + "store already held, as a real bump would") + print(f"\n--dry-run: nothing was changed in the holder. {how}; the " + f"member's checkout, {MANIFEST}, the index and the holder's commits " + "are as they were.") return 0 @@ -2046,13 +2078,16 @@ def cmd_bump(args) -> int: "Remediation: `git submodule update --init " + path + "`, or " "`make bootstrap`, which resolves a credential for a private " "member first.") + fetched = True try: run(["git", "fetch", "-q", "origin"], cwd=member) except CommandFailed: + fetched = False print(f" note: could not fetch {path} from its origin; the commit " "must already be in its object store", file=sys.stderr) if args.dry_run: - return _bump_dry_run(family, row, path, commit, args.trailer) + return _bump_dry_run(family, row, path, commit, args.trailer, + fetched) try: run(["git", "checkout", "-q", commit], cwd=member) except CommandFailed as exc: @@ -2112,32 +2147,75 @@ def cmd_bump(args) -> int: # --------------------------------------------------------------------------- -def _gitmodules_names(root: Path, path: str) -> list[str]: - """The submodule names `.gitmodules` in the work tree gives `path`, in - the order git reads them; `git rm` of that gitlink drops the LAST one's - section (#326). - - WHAT `git rm` ITSELF READS. It edits `.gitmodules` only where the work - tree has one, and looks the gitlink's path up in it; given two sections - for one path — a half-removed member, a hand edit — the one git reads - LAST is the one it drops, and the others stay. A name keeps the FIRST - `path` it is given, as git keeps it. So `remove --dry-run` names the - section a real `remove` drops, and any it leaves behind, without asking - git to write anything. Listed here rather than through `add`'s own - reader of the file, which answers a different question. +def _suspicious_name(name: str) -> bool: + """Whether git IGNORES a submodule of this name: an empty one, or one + with a `..` component between `/` or `\\` on any platform — git's own + `check_submodule_name`, which warns "ignoring suspicious submodule name" + and reads nothing in that section.""" + return not name or ".." in re.split(r"[/\\]", name) + + +def _gitmodules_names(root: Path, path: str) -> tuple[str | None, list[str]]: + """`(the name whose sections `git rm` drops for the gitlink at `path`, + or None, every other name with a `path = ` line)` (#326). + + GIT'S OWN LOOKUP, MODELLED OVER GIT'S OWN READING OF THE FILE. `git rm` + edits `.gitmodules` only where the work tree has one. It asks which + submodule has the gitlink's path (`submodule_from_path`), and drops EVERY + section of that name. Its reading of the WORK-TREE file is the one with + overwrite on (`parse_config`, as `gitmodules_cb` calls it), and three + rules follow from it, each one a hand edit `remove --dry-run` must not + misreport: + + - a name keeps the LAST `path` it is given; + - a name moving to another path takes the OLD path out of git's path map, + whichever name held it there, so a path a ghost name passed through can + end up with no submodule at all, and then `git rm` drops nothing + ("Could not find section in .gitmodules"); + - a suspicious name (`_suspicious_name`) and a path that reads as an + option (a leading `-`) are ignored. + + The file is read by `git config --list`, never by a parser of this + tool's, as `add`'s `_submodule_paths` reads it; the rules are git's, as + git 2.43 applies them and as the tests check them against a real + `remove` on every runner. """ if not (root / GITMODULES).is_file(): - return [] - listed = git_out(["config", "-z", "--file", GITMODULES, "--list"], - cwd=root, binary=True) - first: dict[str, str] = {} + return None, [] + listed = _git_bytes(["config", "-z", "--file", GITMODULES, "--list"], + root) + paths: dict[str, str] = {} + owner: dict[str, str] = {} + giving: list[str] = [] for record in listed.split(b"\0"): key, _, value = record.decode("utf-8", "surrogateescape").partition("\n") section, _, variable = key.rpartition(".") - if section.startswith("submodule.") and variable == "path": - first.setdefault(section[len("submodule."):], value) - return [name for name, given in first.items() if given == path] + if not section.startswith("submodule.") or variable != "path": + continue + name = section[len("submodule."):] + if value == path and name not in giving: + giving.append(name) + if _suspicious_name(name) or value.startswith("-"): + continue + if name in paths: + owner.pop(paths[name], None) + paths[name] = value + owner[value] = name + dropped = owner.get(path) + return dropped, [name for name in giving if name != dropped] + + +def _registered(root: Path, name: str) -> bool: + """Whether the holder's own git config holds a `submodule.` + section: the registration `git submodule deinit` drops, looked up by the + same name `git rm` drops in `.gitmodules`. By section — the part of a + key before its LAST dot — as `MountRollback` reads one, so a dotted + neighbour's keys are not taken for this name's.""" + listed = _git_bytes(["config", "--local", "-z", "--list"], root) + return any(record.decode("utf-8", "surrogateescape").partition("\n")[0] + .rpartition(".")[0] == f"submodule.{name}" + for record in listed.split(b"\0") if record) def _remove_dry_run(family: Family, row: dict, path: str) -> int: @@ -2145,22 +2223,32 @@ def _remove_dry_run(family: Family, row: dict, path: str) -> int: no git command that writes. THE REAL RUN'S FAILURES, IN ITS ORDER. Its first git call, `git - submodule deinit -- `, fails on a path the index holds nothing at; - `ls-files --error-unmatch` over the same LITERAL pathspec fails the same - way, exit 2, and lists what `git rm -r` would unstage when it does not. + submodule deinit -- `, fails on a path the index holds nothing at, + and the real run prints that failure loudly, the exact command and git's + own `did not match any file(s) known to git`, exit 2. `ls-files + --error-unmatch` over the same LITERAL pathspec fails where it fails, + with the same line from git, and is printed the same way, exit 2; it + lists what `git rm -r` would unstage when it does not fail. `family.rewritten` then asks `family-members-unrecognised`, which the real run asks after its unmount, without writing. PRINTED: the row, each index entry under the path (the gitlink), the - `.gitmodules` section `git rm` drops for it (`_gitmodules_names`), and + `.gitmodules` section `git rm` drops for it and any it leaves that gives + the same path (`_gitmodules_names`), the registration `deinit` drops + from the holder's git config under that same name (`_registered`), and the directory `deinit -f` empties and `git rm` deletes, uncommitted work in the member included. The member's object store is not removed, by either run, so it is not on the list. """ project = str(row["project"]) root = family.root - listed = git_out(["--literal-pathspecs", "ls-files", "-s", "-z", - "--error-unmatch", "--", path], cwd=root, binary=True) + try: + listed = _git_bytes(["--literal-pathspecs", "ls-files", "-s", "-z", + "--error-unmatch", "--", path], root) + except CommandFailed as exc: + print(exc.loudly(f"listing {path} in the index, where a real remove " + "unmounts it"), file=sys.stderr) + return 2 entries = [] for record in listed.split(b"\0"): if record: @@ -2179,16 +2267,20 @@ def _remove_dry_run(family: Family, row: dict, path: str) -> int: else: print(f"index {name} {oid[:12]} (mode {mode})") for name in [name for mode, _, name in entries if mode == "160000"]: - names = _gitmodules_names(root, name) - if not names: - print(f"gitmodules no section gives path {name}, so none is " - "removed") - continue - print(f'gitmodules [submodule "{names[-1]}"] path {name}') - if names[:-1]: + dropped, kept = _gitmodules_names(root, name) + if dropped is None: + print(f"gitmodules none: git reads no section of {GITMODULES} " + f"in the work tree as giving {name}, so none is removed") + else: + print(f'gitmodules [submodule "{dropped}"] path {name}, every ' + "section of that name") + if kept: print(" left in place, though it gives the same " "path: " + ", ".join(f'[submodule "{other}"]' - for other in names[:-1])) + for other in kept)) + if dropped is not None and _registered(root, dropped): + print(f'registration [submodule "{dropped}"] in the holder\'s ' + ".git/config, dropped by `git submodule deinit`") print(f"directory {path} " + ( "deleted with everything in it: `git submodule deinit -f` discards " "uncommitted work in the member, and `git rm` the directory" diff --git a/tests/test_family_dry_run.py b/tests/test_family_dry_run.py index 4b780816..06568325 100644 --- a/tests/test_family_dry_run.py +++ b/tests/test_family_dry_run.py @@ -39,7 +39,7 @@ import pytest -from conftest import run_script +from conftest import git, run_script #: The trailer lines a lane's run lands, from the file that owns them. from test_commit_trailers import TRAILERS, trailers_of #: The family fixture, its names and the offline environment, imported from @@ -64,9 +64,9 @@ #: write would change: the member's checkout for `bump`, the index for #: `remove`. BUMP_ORDER = ("member HEAD", "family.yaml", "index", "status", ".gitmodules", - ".git/config", "work tree", "HEAD") + ".git/config", "work tree", "objects", "HEAD") REMOVE_ORDER = ("index", ".gitmodules", "family.yaml", "work tree", - ".git/config", "status", "member HEAD", "HEAD") + ".git/config", "status", "member HEAD", "objects", "HEAD") def work_tree(root: Path) -> dict: @@ -88,10 +88,21 @@ def work_tree(root: Path) -> dict: return found +def objects(root: Path) -> list[str]: + """Every file in the holder's OWN object store, `.git/objects`. A blob + written there is a write the index and the work tree do not show; the + member's store, which `bump`'s fetch writes into, is under + `.git/modules/` and not in this list.""" + store = root / ".git" / "objects" + return sorted(path.relative_to(store).as_posix() + for path in store.rglob("*") if path.is_file()) + + def state(root: Path, env: dict, member: str = MEMBER) -> dict: """Everything a dry run must leave exactly as it found it, as bytes.""" mount = root / "members" / member return { + "objects": objects(root), "HEAD": raw(root, env, "rev-parse", "HEAD").stdout, "index": raw(root, env, "ls-files", "-s", "-z").stdout, "status": raw(root, env, "status", "--porcelain=v1", "-z").stdout, @@ -381,6 +392,31 @@ def test_bump_dry_run_of_a_commit_the_member_does_not_have_exits_2( assert_as_it_was(holder, env, before, BUMP_ORDER) +def test_bump_dry_run_after_a_failed_fetch_says_what_it_resolved_from( + family, holder, offline, tmp_path): + """A fetch that fails is a note, as in the real run, and `--to` is then + resolved from what the member's object store already holds; the closing + line says THAT, not that the fetch is how `--to` was resolved.""" + env = isolated(offline, tmp_path) + raw(holder / "members" / MEMBER, env, "remote", "set-url", "origin", + str(tmp_path / "gone.git")) + before = state(holder, env) + + dry = bump(holder, env, older(family), "--dry-run") + + assert_as_it_was(holder, env, before, BUMP_ORDER) + assert dry.returncode == 0, dry.stdout + dry.stderr + assert dry.stderr.startswith(f" note: could not fetch members/{MEMBER} " + "from its origin") + assert dry.stdout.splitlines()[-1] == ( + f"--dry-run: nothing was changed in the holder. The fetch of " + f"members/{MEMBER} failed, as the note says, so --to was resolved and " + "the digest above computed from what its object store already held, " + "as a real bump would; the member's checkout, family.yaml, the index " + "and the holder's commits are as they were.") + assert "`git fetch` ran" not in dry.stdout + + def test_bump_dry_run_reads_the_members_manifest_at_to_and_refuses_as_bump( holder, offline, tmp_path): """WHAT THE REAL RUN ASKS AFTER ITS CHECKOUT, ASKED OF THE OBJECTS. A @@ -453,15 +489,20 @@ def test_remove_dry_run_prints_what_a_real_remove_takes_and_changes_nothing( assert dry.returncode == 0, dry.stdout + dry.stderr assert dry.stderr == "" lines = dry.stdout.splitlines() - assert lines[:3] == [ + assert lines[:4] == [ f"row {MEMBER} {ORG}/{MEMBER}, pinned at {gitlink[:12]}, " "dropped from family.yaml", f"gitlink members/{MEMBER} {gitlink[:12]}", - f'gitmodules [submodule "members/{MEMBER}"] path members/{MEMBER}', + f'gitmodules [submodule "members/{MEMBER}"] path members/{MEMBER}, ' + "every section of that name", + f'registration [submodule "members/{MEMBER}"] in the holder\'s ' + ".git/config, dropped by `git submodule deinit`", ] - assert lines[3].startswith(f"directory members/{MEMBER} deleted with " + assert lines[4].startswith(f"directory members/{MEMBER} deleted with " "everything in it") assert lines[-1].startswith("--dry-run: nothing was changed") + assert registered_keys(holder, env, f"members/{MEMBER}"), ( + "premise: the registration the dry run named is there") real = remove(holder, env) @@ -475,38 +516,118 @@ def test_remove_dry_run_prints_what_a_real_remove_takes_and_changes_nothing( == [f"members/{OTHER}"] assert not (holder / "members" / MEMBER).exists() assert (holder / "members" / OTHER / "project.yaml").is_file() + assert registered_keys(holder, env, f"members/{MEMBER}") == [] + assert registered_keys(holder, env, f"members/{OTHER}") assert validate(holder).returncode == 0 -def test_remove_dry_run_names_the_section_git_drops_and_the_one_it_leaves( +def test_remove_dry_run_of_a_member_the_index_does_not_hold_fails_as_remove( holder, offline, tmp_path): - """TWO SECTIONS GIVE ONE PATH — a half-removed member, a hand edit — - and `git rm` drops only the one it reads last. The dry run names that - one, and the one left behind, and the real remove drops exactly the - section it named: the dry run says what git will do, not what a reader - of the file might expect.""" + """A row whose gitlink is no longer in the index: the real `remove`'s + first git call, `git submodule deinit`, fails on the path, loudly, exit + 2. The dry run fails where it fails, on git's same line, printed the + same way, exit 2, and never with a remediation for a LEG + (`validate-pins.py`, `contracts/-pin.yaml`) that a holder does not + have. Nothing moves, from either.""" + env = isolated(offline, tmp_path) + raw(holder, env, "rm", "-q", "--cached", "--", f"members/{MEMBER}") + raw(holder, env, "commit", "-q", "-m", "Unstage the gitlink by hand") + before = state(holder, env) + + dry = remove(holder, env, "--dry-run") + + assert_as_it_was(holder, env, before, REMOVE_ORDER) + real = remove(holder, env) + assert_as_it_was(holder, env, before, REMOVE_ORDER) + assert dry.returncode == real.returncode == 2, dry.stdout + dry.stderr + assert dry.stdout == real.stdout == "" + assert dry.stderr.startswith( + f"REFUSED listing members/{MEMBER} in the index, where a real remove " + "unmounts it. THE EXACT COMMAND WAS:") + assert real.stderr.startswith("REFUSED a git or gh command failed. THE " + "EXACT COMMAND WAS:") + for result in (dry, real): + assert (f"error: pathspec 'members/{MEMBER}' did not match any " + "file(s) known to git") in result.stderr + assert "validate-pins" not in result.stderr + assert "-pin.yaml" not in result.stderr + + +def registered_keys(root: Path, env: dict, name: str) -> list[str]: + """The `submodule..*` lines of the holder's own git config, by + section: the part of a key before its last dot.""" + listed = raw(root, env, "config", "--local", "--list").stdout.decode( + "utf-8", "replace") + return [line for line in listed.splitlines() + if line.partition("=")[0].rpartition(".")[0] + == f"submodule.{name}"] + + +#: Hand edits that give `members/` to a section of another name, +#: appended to `.gitmodules` and committed: `(text, the name git rm then drops +#: every section of, or None for none, the other names whose sections say that +#: path and stay)`. Each is what git 2.43 does, probed: the work-tree file is +#: read with overwrite, so a name keeps its LAST path, a name moving away +#: takes the old path out of git's map, and a suspicious name is ignored. +GHOSTS = { + "two-names": ( + f'[submodule "ghost"]\n\tpath = members/{MEMBER}\n', + "ghost", [f"members/{MEMBER}"]), + "last-path-wins": ( + '[submodule "ghost"]\n\tpath = members/ELSEWHERE\n' + f'[submodule "ghost"]\n\tpath = members/{MEMBER}\n', + "ghost", [f"members/{MEMBER}"]), + "path-left-behind": ( + f'[submodule "ghost"]\n\tpath = members/{MEMBER}\n' + '[submodule "ghost"]\n\tpath = members/ELSEWHERE\n', + None, [f"members/{MEMBER}", "ghost"]), + "suspicious-name": ( + f'[submodule "../ghost"]\n\tpath = members/{MEMBER}\n', + f"members/{MEMBER}", ["../ghost"]), +} + + +@pytest.mark.parametrize("text, dropped, kept", GHOSTS.values(), ids=GHOSTS) +def test_remove_dry_run_names_the_sections_a_real_remove_drops( + holder, offline, tmp_path, text, dropped, kept): + """A SECOND SECTION FOR THE MEMBER'S PATH — a half-removed member, a hand + edit — and the dry run names the name whose sections `git rm` drops (or + that it drops none), the names it leaves that still say the path, and + the registration `deinit` drops under the same name. Then a REAL + `remove` takes out exactly the sections of the name the dry run named, + and the registration it named, and nothing else: the dry run says what + git will do, not what a reader of the file might expect.""" env = isolated(offline, tmp_path) with (holder / ".gitmodules").open("ab") as gitmodules: - gitmodules.write(f'[submodule "ghost"]\n\tpath = members/{MEMBER}\n' - "\turl = ../elsewhere\n".encode("utf-8")) + gitmodules.write(text.encode("utf-8")) raw(holder, env, "add", "--", ".gitmodules") raw(holder, env, "commit", "-q", "-m", "A second section for one path") + listed = sections((holder / ".gitmodules").read_text(encoding="utf-8")) before = state(holder, env) dry = remove(holder, env, "--dry-run") + assert_as_it_was(holder, env, before, REMOVE_ORDER) assert dry.returncode == 0, dry.stdout + dry.stderr lines = dry.stdout.splitlines() - assert f'gitmodules [submodule "ghost"] path members/{MEMBER}' in lines + path = f"members/{MEMBER}" + assert (f'gitmodules [submodule "{dropped}"] path {path}, every ' + "section of that name" if dropped else + "gitmodules none: git reads no section of .gitmodules in the " + f"work tree as giving {path}, so none is removed") in lines assert (" left in place, though it gives the same path: " - f'[submodule "members/{MEMBER}"]') in lines - assert_as_it_was(holder, env, before, REMOVE_ORDER) + + ", ".join(f'[submodule "{name}"]' for name in kept)) in lines + named = [line for line in lines if line.startswith("registration ")] + assert named == ([f'registration [submodule "{path}"] in the holder\'s ' + ".git/config, dropped by `git submodule deinit`"] + if dropped == path else []) real = remove(holder, env) assert real.returncode == 0, real.stdout + real.stderr assert sections((holder / ".gitmodules").read_text(encoding="utf-8")) \ - == [f"members/{MEMBER}", f"members/{OTHER}"] + == [name for name in listed if name != dropped] + assert bool(registered_keys(holder, env, path)) is (not named) # --- both -------------------------------------------------------------------- @@ -537,13 +658,26 @@ def test_a_dry_run_asks_the_manifest_refusal_the_real_run_asks_after_it_writes( BUMP_ORDER if verb == "bump" else REMOVE_ORDER) -#: Git commands that write: to the index, a work tree, a ref or the history. -#: A dry run runs none of them. +#: Git commands that write: to the index, a work tree, a ref, the object +#: store or the history. A dry run runs none of them. `hash-object` and +#: `config` are here only when they would write (`-w`, or anything but a +#: listing); `bump --dry-run` asks `hash-object` for a blob id, with no `-w`. WRITERS = {"add", "checkout", "commit", "merge", "mv", "pull", "push", "reset", "restore", "rm", "stash", "submodule", "switch", "update-index", "update-ref", "write-tree"} +def writes(argv: list[str]) -> bool: + """Whether the git command `argv` writes, by `WRITERS` and the two + commands that write only when told to.""" + sub = subcommand_of(argv) + if sub == "hash-object": + return "-w" in argv + if sub == "config": + return "--list" not in argv + return sub in WRITERS + + def subcommand_of(argv: list[str]) -> str: """The git subcommand in `argv`, past git's own options.""" words = iter(argv[1:]) @@ -555,28 +689,33 @@ def subcommand_of(argv: list[str]) -> str: raise AssertionError(f"no subcommand in {argv}") -@pytest.mark.parametrize("verb", ["bump", "remove"]) +@pytest.mark.parametrize("verb, to, code", [ + ("bump", "older", 0), ("bump", "pinned", 1), ("remove", None, 0)], + ids=["bump-move", "bump-nothing", "remove"]) def test_a_dry_run_runs_no_git_command_that_writes( - family, holder, offline, tmp_path, monkeypatch, verb): + family, holder, offline, tmp_path, monkeypatch, verb, to, code): """"Changes nothing" is what the holder shows; this is the other half, that nothing which writes is RUN. Every program the dry run starts is recorded, in this process: no checkout, no stage, no commit, no `git - submodule`, and a `git config` only to list. `bump`'s does fetch, as the - help and the docstring say it does, and `remove`'s does not.""" + submodule`, a `git config` only to list and a `git hash-object` with no + `-w`, which `nothing to do` is the run that asks. `bump`'s does fetch, + as the help and the docstring say it does, and `remove`'s does not.""" module = family_module() - args = [verb, "--family-root", str(holder), "--member", MEMBER, - *(["--to", older(family)] if verb == "bump" else []), "--dry-run"] + args = [verb, "--family-root", str(holder), "--member", MEMBER] + if to is not None: + args += ["--to", older(family) if to == "older" else + git("rev-parse", f"HEAD:members/{MEMBER}", + cwd=holder).stdout.strip()] for key, value in isolated(offline, tmp_path).items(): monkeypatch.setenv(key, value) calls = recorded(monkeypatch) - assert module.main(args) == 0 + assert module.main([*args, "--dry-run"]) == code gits = [argv for argv in calls if Path(argv[0]).name == "git"] assert gits, "premise: the calls were recorded" + assert [argv for argv in gits if writes(argv)] == [] ran = {subcommand_of(argv) for argv in gits} - assert not ran & WRITERS, sorted(ran & WRITERS) - for argv in gits: - if subcommand_of(argv) == "config": - assert "--list" in argv, argv assert ("fetch" in ran) is (verb == "bump") + assert ("hash-object" in ran) is (to == "pinned"), ( + "premise: `nothing to do` is asked of `hash-object`, and only then") From e80c97cfb88baa1aa91bcda2de211eeae67be6a9 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 13:04:16 +0000 Subject: [PATCH 3/3] tests/test_family_dry_run.py: the dry-run state check lists object files only and runs every git with automatic maintenance off, so git's background maintenance lock no longer fails five tests on the macOS runner (#326) PR #345's first macOS run failed five tests in this file, all `objects changed`: the index-does-not-hold test and the four `.gitmodules` hand edits. In each, the BEFORE snapshot held one more file, `.git/objects/maintenance.lock`. The set-up's `git commit` started git's background auto-maintenance (the runner carries a newer git). The job held the lock when `state()` was taken and released it before the dry run's comparison. A race in the test, not the tool: ubuntu passed. Two changes, either of which closes it: - Every git this module starts runs with `maintenance.auto=false`, `gc.auto=0` and `gc.autoDetach=false`. They are given through git's `GIT_CONFIG_COUNT` protocol, never a global file, layered on `test_family_member_path.isolated` by a wrapper of the same name, so no background job packs or locks objects mid-test. - `objects()` lists OBJECT FILES only: loose objects (two hex digits, then the rest of a sha-1 or sha-256 id) and `pack/pack-*.pack` and `.idx`. A `*.lock`, `info/` and git's other transient files can no longer fail it. A blob the dry run wrote would still be a new object file, so the comparison keeps its meaning. Mutant MI (`hash-object -w`) is killed by the in-process recorded-call check, which this does not touch. Part of #326 Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Opus 5.5 --- tests/test_family_dry_run.py | 48 ++++++++++++++++++++++++++++++------ 1 file changed, 41 insertions(+), 7 deletions(-) diff --git a/tests/test_family_dry_run.py b/tests/test_family_dry_run.py index 06568325..98660d59 100644 --- a/tests/test_family_dry_run.py +++ b/tests/test_family_dry_run.py @@ -34,6 +34,7 @@ from __future__ import annotations import os +import re import sys from pathlib import Path @@ -53,8 +54,30 @@ holder, offline, refusal, sections, ) from test_family_member_path import ( - GLOB, MEMBER, OTHER, hand_edit, isolated, older, raw, recorded, + GLOB, MEMBER, OTHER, hand_edit, older, raw, recorded, ) +from test_family_member_path import isolated as member_path_env + +#: GIT'S AUTOMATIC MAINTENANCE, OFF for every git this module starts, through +#: git's `GIT_CONFIG_COUNT` protocol and never a global file. A `git commit` +#: may start `git maintenance run --auto` in the background (a newer git on +#: the macOS runner does), and it took `.git/objects/maintenance.lock` while +#: a test snapshotted the holder and released it before the comparison: five +#: `objects changed` failures on PR #345's first macOS run, none of them the +#: tool's. With it off, no background job packs or locks objects mid-test. +NO_MAINTENANCE = {"GIT_CONFIG_COUNT": "3", + "GIT_CONFIG_KEY_0": "maintenance.auto", + "GIT_CONFIG_VALUE_0": "false", + "GIT_CONFIG_KEY_1": "gc.auto", "GIT_CONFIG_VALUE_1": "0", + "GIT_CONFIG_KEY_2": "gc.autoDetach", + "GIT_CONFIG_VALUE_2": "false"} + + +def isolated(offline, tmp_path: Path) -> dict: + """`test_family_member_path.isolated`'s offline, identity-carrying + environment, with git's automatic maintenance off (`NO_MAINTENANCE`).""" + return {**member_path_env(offline, tmp_path), **NO_MAINTENANCE} + #: The header `bump --dry-run` prints above the row it would write. ROW_HEADER = "row as family.yaml would carry it:" @@ -88,14 +111,25 @@ def work_tree(root: Path) -> dict: return found +#: An OBJECT FILE under `.git/objects`: a loose object (two hex digits, then +#: the rest of a sha-1 or sha-256 id) or a pack and its index. Only these are +#: listed. A `*.lock`, `info/` and git's other transient files are not +#: objects, and a lock a background job held at the first snapshot and not +#: at the second failed five tests on PR #345's first macOS run. +OBJECT_FILE = re.compile(r"[0-9a-f]{2}/[0-9a-f]{38}(?:[0-9a-f]{24})?" + r"|pack/pack-[0-9a-f]+\.(?:pack|idx)") + + def objects(root: Path) -> list[str]: - """Every file in the holder's OWN object store, `.git/objects`. A blob - written there is a write the index and the work tree do not show; the - member's store, which `bump`'s fetch writes into, is under - `.git/modules/` and not in this list.""" + """Every OBJECT FILE (`OBJECT_FILE`) in the holder's OWN object store, + `.git/objects`. A blob written there is a write the index and the work + tree do not show; the member's store, which `bump`'s fetch writes into, + is under `.git/modules/` and not in this list.""" store = root / ".git" / "objects" - return sorted(path.relative_to(store).as_posix() - for path in store.rglob("*") if path.is_file()) + return sorted(name for name in (path.relative_to(store).as_posix() + for path in store.rglob("*") + if path.is_file()) + if OBJECT_FILE.fullmatch(name)) def state(root: Path, env: dict, member: str = MEMBER) -> dict: