From 1e422416d0b38ac508fc3330cd30b04b0f721e82 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 01:52:56 +0000 Subject: [PATCH 1/4] family.py: `bump` and `remove` refuse `member-path` when a row's `path:` is not `members/` and take every path they hand git literally, so a hand-edited `path: members/*` no longer makes `remove --member Atlas` exit 0 with one commit that unmounts every member while dropping one row, and `path: .` no longer `git rm -r -f`s the holder's own files before dying on the missing manifest (#299) THE DEFECT. `cmd_bump` and `cmd_remove` read the member's path from the manifest row, `str(row.get("path") or f"{MEMBERS_DIR}/{project}")`, and handed it to git as it stood. Git reads a path as a pathspec, and nothing checked the field or passed `--literal-pathspecs`: - `path: members/*` made `remove --member Atlas` exit 0 with ONE commit that deleted every member's gitlink, `members/Atlas` and `members/Borg` alike, while dropping only Atlas's row. Borg was then in the manifest and not in the tree. - `path: .` made it `git rm -r -f` the holder's own files, then die on a `FileNotFoundError` reading the `family.yaml` it had just removed. - `bump` needed a second accident first: `member-uninitialized` wants a checkout with a `.git` at the literal path, so it took a directory named `members/*` as well as the hand edit. With one it exited 0: it fetched and checked `--to` out inside that clone, then staged and committed the clone as a NEW gitlink at `members/*` beside the rewritten row. The tool writes `path:` itself, as `members/` and nothing else (`_member_row`), and `validate-family.py` has a rule for exactly that, `member-path`. But the validator runs after a commit, in CI and in the doctor, never in front of these two verbs, so they trusted the field it would refuse. THE THREE RULINGS (the issue's decisions, as the coordinator made them). 1. REFUSE, before any git call and any write. One new function, `_member_path(project, row=None)`, is the one definition of the path a member has. `_member_row` (the row `add` writes) calls it with no row, so `add` is unchanged; `bump` and `remove` call it with the row after their any-case lookup (#221, #206), so `project` is the row's own spelling. A row whose `path:` is not exactly `members/` raises `Refusal("member-path", ...)`, the code the validator's finding uses for the same rule. The refusal names the row's value and the expected one, and its remediation says the tool writes that field itself: put the path back in `family.yaml`, or `git checkout -- family.yaml` if the edit is not committed. A row with no `path:` keeps defaulting; the comparison is exact, as the validator's is. `validate-family.py` and `repo_shape.py` are NOT changed: both ship into every holder, so a change there moves every pin. The validator keeps its own spelling of the rule, the second one. 2. LITERAL PATHSPECS, in the place git takes global options, on every git call in these two verbs that takes the member's path: `_nothing_to_record`'s `git diff`, `_commit_one`'s `git commit` (its stage was already literal through `force_stage`), and `remove`'s `git submodule deinit` and `git rm`. Proved on git 2.43.0 with `members/*` in a scratch holder: each of the four acts on the neighbours without the flag and does nothing with it, and `git submodule`, a command of its own, honours the global flag (it reaches it as `GIT_LITERAL_PATHSPECS`), so no per-pathspec `:(literal)` magic was needed. With ruling 1 the path is always `members/`, so this is defence in depth that does not depend on the row being right. 3. SCOPE: `bump` and `remove` only. `add` builds its path from checked values; `siblings` and the validator only read the row. TESTS (`tests/test_family_member_path.py`, in `test_family.py`'s family and the offline environment of `test_family_add_refusals.py`: members from local bare remotes, a `gh` shim that logs and exits 97, an isolated git configuration). - `remove` and `bump` with Atlas's row hand-edited, uncommitted, to `members/*`, `.`, the neighbour's path, the member's own path with a trailing slash, and the member's path in another case: exit 2, the refusal text, `git ls-files -s -z` and `git status --porcelain=v1 -z` byte-identical, HEAD unchanged, the neighbour's gitlink, row and checkout still there, no `gh` call. - The same refusal with `subprocess.run` forbidden, so it is made before any git call and not merely before any write. - `bump` with a literal `members/*` checkout holding a `.git` refuses and leaves that clone's HEAD where it was (skipped on Windows, which refuses `*` in a file name; every other row needs no such directory). - One expectation moves in `tests/test_family_add_refusals.py`: the failed `git commit` an unwound `add` shows is now `git --literal-pathspecs commit ...`, because `_commit_one` is shared and `add`'s paths are literals either way. - The controls, the row the tool writes and a row with no `path:`, still exit 0 in ONE commit of exactly the paths each verb is for, and validate. - Each of the four git calls, with the glob `members/*`, reaches the neighbour without `--literal-pathspecs` (the premise) and does not with it. - Every call that names the path in a real `bump` and a real `remove` carries the flag among git's global options, the set of calls checked exactly. MUTANTS, each run on a scratch copy of the tree. - Refusal dropped, flags kept: the 10 refusal rows, the no-git-call rows, the literal-directory row and the one-definition row fail. `remove` with `members/*` still stops, at `deinit`, exit 2, nothing written; but `path: .` still dies on the `FileNotFoundError` with the whole tree staged for deletion, and `bump` with the literal `members/*` clone still exits 0 with the wrong commit, so the flags are no substitute for the refusal. - Refusal and every flag dropped (what main did): `remove` with `members/*` exits 0 with one commit whose `git show --stat` names `members/IRRS` and `members/IRSS`, 18 deletions, one row dropped. - Refusal dropped and `git rm`'s flag dropped alone: `deinit` stops the run first, so that flag is unreachable end to end. With `deinit` stubbed out to reach it, `git rm` without its flag stages the deletion of BOTH members (the literal `commit` then stops the commit); with it, exit 2 and the index is clean. - Each flag dropped alone (`deinit`, `rm`, `diff`, `commit`), refusal kept: the every-call test fails, and only it. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Sonnet 5.5 --- scripts/family.py | 81 +++++- tests/test_family_add_refusals.py | 3 +- tests/test_family_member_path.py | 400 ++++++++++++++++++++++++++++++ 3 files changed, 475 insertions(+), 9 deletions(-) create mode 100644 tests/test_family_member_path.py diff --git a/scripts/family.py b/scripts/family.py index 88125222..b3c4d974 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -264,13 +264,51 @@ def _member_lines(row: dict) -> list[str]: ] +def _member_path(project: str, row: dict | None = None) -> str: + """The ONE path a member has, `members/` (#299); given the + member's row, that path or the refusal that says the row names another. + + THE TOOL WRITES `path:` ITSELF, as this and nothing else (`_member_row`), + and `bump` and `remove` used to read it back and hand it to git as it + stood. Git reads a path as a pathspec: `path: members/*` made `remove + --member Atlas` exit 0 with one commit that unmounted EVERY member while + dropping only Atlas's row, and `path: .` made it `git rm -r -f` the + holder's own files and die on the missing manifest. A hand edit is outside + the supported path (AGENTS.md: do not hand-edit the `members:` block), and + the rule this asks is the one `validate-family.py` already records as + `member-path`; but the validator runs after a commit, in CI and in the + doctor, never in front of these two verbs, so they ask it themselves, + BEFORE ANY GIT CALL AND ANY WRITE. + + A row with no `path:` keeps defaulting to it, as it always has, and the + comparison is EXACT, as the validator's is: `members/IRRS/`, `members/irrs` + and a neighbour's path are each a different string and each refused. The + validator keeps its own spelling of this rule on purpose. It ships INTO + every holder, as `repo_shape.py` does, so sharing it would move every + holder's pin for a refusal of a hand edit. + """ + expected = f"{MEMBERS_DIR}/{project}" + declared = expected if row is None else str(row.get("path") or expected) + if declared != expected: + raise Refusal( + "member-path", + f"{project}: path is {declared!r} in {MANIFEST}, expected " + f"{expected!r}. Git would read that value as a pathspec, so no " + "git command was run and nothing was written.", + "Remediation: this tool writes that field itself, so a different " + f"value is a hand edit. Put `path: {expected}` back in " + f"{MANIFEST}, or `git checkout -- {MANIFEST}` if the edit is not " + "committed, then run the same command again.") + return expected + + def _member_row(project: str, project_id: str, repository: str, commit: str, digest: str) -> dict: return { "project": project, "id": project_id, "repository": repository, - "path": f"{MEMBERS_DIR}/{project}", + "path": _member_path(project), "pin": {"revision_kind": "commit", "commit": commit, "tree_sha256": digest}, } @@ -425,10 +463,19 @@ def _commit_one(root: Path, message: str, paths: list[str], scaffold and `update-shape.py` share, so it is called here rather than spelled a second time; `bump`'s re-stage before its nothing-to-record check is the same call, for the same reason. + + THE COMMIT READS THEM LITERALLY TOO (#299). The stage above was literal + and `git commit -- ` was not, so a path that is a glob to git + committed whatever else it matched: with `members/*` named, the move of + every OTHER member's gitlink somebody had staged went into this commit + beside the one it was for. `bump` and `remove` hand it a path that is + always `members/` (`_member_path`); the flag is the guard that + does not depend on that being so. """ force_stage(root, paths if add is None else add) message, trailer_args = commit_trailers(message, trailers) - args = ["git", "commit", "-q", "-F", "-", *trailer_args, "--", *paths] + args = ["git", "--literal-pathspecs", "commit", "-q", "-F", "-", + *trailer_args, "--", *paths] check_program(args) env = dict(os.environ) for key, fallback in (("GIT_AUTHOR_NAME", "openRepoShape family"), @@ -1681,10 +1728,15 @@ def _nothing_to_record(root: Path, paths: list[str]) -> bool: its HEAD is. The caller has staged the gitlink first, as `_commit_one` would, so a stray one in the index is not a difference. Exit 1 is git's "there is a difference"; any other failure is a real one and propagates. + + THE PATHS ARE TAKEN LITERALLY (`--literal-pathspecs`, #299), as the + commit this answers for takes them: a glob here would find the + difference in a neighbouring member and answer "something to record" for + a member that has nothing, so the commit would go ahead. """ try: - run(["git", "diff", "--quiet", "--ignore-submodules=dirty", "HEAD", - "--", *paths], cwd=root) + run(["git", "--literal-pathspecs", "diff", "--quiet", + "--ignore-submodules=dirty", "HEAD", "--", *paths], cwd=root) except CommandFailed as exc: if exc.code == 1: return False @@ -1710,7 +1762,9 @@ def cmd_bump(args) -> int: f"--to {args.to!r} is not a 40-hex commit", "Remediation: pass the full 40 characters. A tag can be moved and " "a commit cannot; a pin is a commit or it is nothing.") - path = str(row.get("path") or f"{MEMBERS_DIR}/{project}") + # THE ROW'S `path:` IS CHECKED, NOT TRUSTED (#299): `member` below, the + # fetch and checkout in it, and every pathspec after them are this path. + path = _member_path(project, row) member = family.root / path if not (member / ".git").exists(): raise Refusal( @@ -1799,13 +1853,24 @@ def cmd_remove(args) -> int: "something that is not a member is a no-op worth refusing, " "because the argument is usually a typo for something that is.") project = str(row["project"]) - path = str(row.get("path") or f"{MEMBERS_DIR}/{project}") + # THE ROW'S `path:` IS CHECKED, NOT TRUSTED (#299), before the first git + # call: `path: members/*` unmounted every member under one commit naming + # one, and `path: .` removed the holder's own files. + path = _member_path(project, row) # 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 # a row would be a family that confers something. - run(["git", "submodule", "deinit", "-f", "-q", "--", path], cwd=family.root) - run(["git", "rm", "-r", "-q", "-f", "--", path], cwd=family.root) + # + # BOTH CALLS TAKE THE PATH LITERALLY (`--literal-pathspecs`, #299), the + # global option, which `git submodule` honours too: the path is always + # `members/` here, so this is defence in depth that does not + # depend on the row being right. Without it a glob in the path would + # unmount and remove every member it matched. + run(["git", "--literal-pathspecs", "submodule", "deinit", "-f", "-q", + "--", path], cwd=family.root) + run(["git", "--literal-pathspecs", "rm", "-r", "-q", "-f", "--", path], + cwd=family.root) family.write_members([r for r in family.members if str(r.get("project")) != project]) head = _commit_one( diff --git a/tests/test_family_add_refusals.py b/tests/test_family_add_refusals.py index 0d1b7627..deecce40 100644 --- a/tests/test_family_add_refusals.py +++ b/tests/test_family_add_refusals.py @@ -732,7 +732,8 @@ def test_a_commit_that_fails_unwinds_the_mount_and_the_pin(family, holder, f"mounting {ORG}/{SPARE} at members/{SPARE}\n") assert result.stderr.splitlines() == [ f"REFUSED committing {SPARE} to the family. THE EXACT COMMAND WAS:", - f" git commit -q -F - -- .gitmodules members/{SPARE} family.yaml", + " git --literal-pathspecs commit -q -F - -- .gitmodules " + f"members/{SPARE} family.yaml", f" (in {holder.resolve()})", " exit 1", "--- output ---", diff --git a/tests/test_family_member_path.py b/tests/test_family_member_path.py new file mode 100644 index 00000000..152e0ffb --- /dev/null +++ b/tests/test_family_member_path.py @@ -0,0 +1,400 @@ +# SPDX-License-Identifier: Apache-2.0 +"""`family.py bump` and `remove` act on the one path a member can have, or +refuse (#299). + +Both verbs took the member's path from the manifest row's `path:` and handed +it to git as a PATHSPEC, with no check and no `--literal-pathspecs`. The tool +writes that field itself, as `members/` and nothing else, so a +different value is a hand edit; and a hand edit to `path: members/*` made +`remove --member Atlas` exit 0 with ONE commit that unmounted every member +while dropping only Atlas's row, and `path: .` made it `git rm -r -f` the +holder's own files and die on the missing manifest. `validate-family.py` has a +rule for it (`member-path`), but it runs after a commit, never in front of +these two verbs. So they refuse `member-path` themselves, before any git call +and any write; and every git call that takes the path takes it literally, which +does not depend on the row being right. + +THE ISSUE'S MEMBERS ARE `Atlas` AND `Borg`; this family's are `IRRS` and +`IRSS`, so `IRRS` is the row a test edits and `IRSS` is the neighbour that must +be left as it was. The same family, rules and offline environment as +`test_family.py`, `test_family_add_refusals.py` and +`test_family_add_excludes.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 +(`GIT_CONFIG_GLOBAL`, `GIT_CONFIG_NOSYSTEM`). No test pushes to the fixture's +own remotes. + +A refusal is asked of the holder the way `test_family_add_refusals.py` asks it +of `add`: not "is the sentence printed" but "is everything exactly as it was", +down to the bytes of `git ls-files -s -z` and `git status --porcelain=v1 -z`, +with the hand edit itself still sitting uncommitted in the work tree. +""" + +from __future__ import annotations + +import os +import shutil +import subprocess +import sys +from pathlib import Path + +import pytest + +from conftest import git, run_script +#: 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, MEMBERS, NAME, ORG, commit_in_member, family, family_module, + manifest, validate, +) +from test_family_add_refusals import ( # noqa: F401 + holder, offline, refusal, snapshot, +) +from test_scaffold_ignored_shape_files import excluding + +#: The row a test edits, and the neighbour that must come out untouched. +MEMBER, OTHER = MEMBERS +GLOB = "members/*" + +#: What a hand edit can leave in `path:`. Each is a string `validate-family.py` +#: refuses as `member-path`, and each is one a verb used to act on: a glob +#: over every member, the holder itself, a NEIGHBOUR'S path (no glob needed to +#: unmount the wrong member), the member's own path with a trailing slash +#: (git reads it as the same path) and in another case (a case-insensitive disk +#: reads it as the same path). The comparison is exact, as the validator's is. +SPELLINGS = { + "glob": GLOB, + "dot": ".", + "neighbour": f"members/{OTHER}", + "trailing-slash": f"members/{MEMBER}/", + "case": f"members/{MEMBER.lower()}", +} + +#: A name Windows refuses in a file name: `*` is one of the characters its +#: file systems reserve. Only the test that needs a directory of that name is +#: skipped there; every refusal row needs none and runs everywhere. +NO_STAR_FILES = pytest.mark.skipif( + sys.platform == "win32", + reason="Windows refuses `*` in a file name, so the directory `members/*` " + "this test makes cannot exist there; every other test in this " + "module needs no such directory and runs on every platform") + + +def isolated(offline, tmp_path: Path) -> dict: + """`offline`'s environment plus a git configuration that is the test's + own: no run here reads the global or the system configuration of whoever + runs the suite, and every commit has an identity to be made under.""" + return {**offline.env, **excluding(tmp_path / "operator", b""), + "GIT_CONFIG_NOSYSTEM": "1", + "GIT_AUTHOR_NAME": "t", "GIT_AUTHOR_EMAIL": "t@t.invalid", + "GIT_COMMITTER_NAME": "t", "GIT_COMMITTER_EMAIL": "t@t.invalid"} + + +def raw(root: Path, env: dict, *args: str, + check: bool = True) -> subprocess.CompletedProcess: + """`git` in `root` under `env`, its output as BYTES.""" + proc = subprocess.run(["git", *args], cwd=str(root), capture_output=True, + check=False, env={**os.environ, **env}) + if check: + assert proc.returncode == 0, (f"git {' '.join(args)} in {root}:\n" + f"{proc.stderr!r}") + return proc + + +def hand_edit(root: Path, project: str, path: str | None) -> None: + """`project`'s row in `family.yaml` given `path:` `path` (or none when + `path` is None), UNCOMMITTED: what a person who edits the manifest by hand + leaves behind. The manifest reads it back as typed, so a test is never + about a value the reader changed on the way.""" + manifest_path = root / "family.yaml" + text = manifest_path.read_bytes().decode("utf-8") + line = f" path: members/{project}\n" + assert text.count(line) == 1, "fixture drift: the row's own path line" + manifest_path.write_bytes(text.replace( + line, "" if path is None else f" path: {path}\n").encode("utf-8")) + row = {r["project"]: r for r in manifest(root)["members"]}[project] + assert row.get("path") == path + + +def state(root: Path, env: dict) -> dict: + """Everything a refused verb must leave exactly as it found it.""" + 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(), + "the rest": snapshot(root), + } + + +def commits(root: Path, env: dict) -> int: + return int(raw(root, env, "rev-list", "--count", "HEAD").stdout) + + +def committed(root: Path, env: dict) -> set[str]: + return set(raw(root, env, "show", "--name-only", "--format=", "HEAD" + ).stdout.decode("utf-8").split()) + + +def older(family) -> str: + """A commit of `MEMBER` that is not the one the holder pins, so a bump to + it has something to record.""" + return git("rev-parse", "main~1", cwd=family["remotes"] / f"{MEMBER}.git" + ).stdout.strip() + + +def verb_args(verb: str, root: Path, family) -> list[str]: + extra = ["--to", older(family)] if verb == "bump" else [] + return [verb, "--family-root", str(root), "--member", MEMBER, *extra] + + +def neighbour_intact(root: Path, env: dict) -> None: + """The member NOT named is still mounted, pinned and checked out.""" + entry = raw(root, env, "ls-tree", "HEAD", "--", f"members/{OTHER}" + ).stdout + assert entry.startswith(b"160000 commit "), entry + assert [r["project"] for r in manifest(root)["members"]] == list(MEMBERS) + assert (root / "members" / OTHER / "project.yaml").is_file() + + +# --- the refusal ------------------------------------------------------------ + +@pytest.mark.parametrize("spelling", SPELLINGS.values(), ids=SPELLINGS) +@pytest.mark.parametrize("verb", ["remove", "bump"]) +def test_a_row_whose_path_is_not_the_members_is_refused_and_nothing_moves( + family, holder, offline, tmp_path, verb, spelling): + """THE ISSUE'S PROBES. `remove` with `path: members/*` exited 0 with ONE + commit that deleted every member's gitlink; with `path: .` it removed the + holder's own files and died with a traceback. Now: exit 2, the refusal + the validator's own finding is named for, the row's value and the + expected one in it, the remedy that puts the field back, and the holder + byte for byte as it was, the hand edit still uncommitted.""" + env = isolated(offline, tmp_path) + hand_edit(holder, MEMBER, spelling) + assert raw(holder, env, "status", "--porcelain=v1", "-z").stdout == ( + b" M family.yaml\0"), "premise: the hand edit is the only change" + before = state(holder, env) + + result = run_script(FAMILY, *verb_args(verb, holder, family), env=env) + + assert result.returncode == 2, result.stdout + result.stderr + assert result.stdout == "", "refused before anything was reported" + said, remedy = refusal(result.stderr) + assert said == ( + f"REFUSED member-path: {MEMBER}: path is {spelling!r} in family.yaml, " + f"expected 'members/{MEMBER}'. Git would read that value as a " + "pathspec, so no git command was run and nothing was written.") + assert remedy == ( + "Remediation: this tool writes that field itself, so a different " + f"value is a hand edit. Put `path: members/{MEMBER}` back in " + "family.yaml, or `git checkout -- family.yaml` if the edit is not " + "committed, then run the same command again.") + assert state(holder, env) == before + neighbour_intact(holder, env) + assert not offline.gh_log.exists(), offline.gh_log.read_text() + + +@pytest.mark.parametrize("verb", ["remove", "bump"]) +def test_the_refusal_is_made_before_any_git_call( + family, holder, offline, tmp_path, monkeypatch, capsys, verb): + """"Nothing was written" is what the holder shows; this is the other half + of the ruling, that no git command is RUN either. Run in this process with + every subprocess forbidden: a verb that reached git before the check + fails here on the call, whatever the end state says.""" + module = family_module() + hand_edit(holder, MEMBER, GLOB) + before = (holder / "family.yaml").read_bytes() + args = verb_args(verb, holder, family) + + def forbidden(argv, *rest, **named): + raise AssertionError(f"a program was run before the refusal: {argv}") + + monkeypatch.setattr(subprocess, "run", forbidden) + assert module.main(args) == 2 + err = capsys.readouterr().err + assert err.startswith("REFUSED member-path: ") + assert (holder / "family.yaml").read_bytes() == before + + +@NO_STAR_FILES +def test_bump_refuses_a_row_naming_a_directory_that_is_really_there( + family, holder, offline, tmp_path): + """THE ISSUE'S ONE WAY `bump` GOT PAST ITS OWN `member-uninitialized`: a + checkout, `.git` and all, in a directory literally named `members/*`. It + then fetched and checked that out at `--to`, and staged and committed it + as a gitlink of its own. The refusal comes before all of that: the clone + stays where it was.""" + env = isolated(offline, tmp_path) + git("clone", "-q", str(family["remotes"] / f"{MEMBER}.git"), GLOB, + cwd=holder) + star = holder / "members" / "*" + assert (star / ".git").exists(), "premise: the literal checkout is there" + tip = raw(star, env, "rev-parse", "HEAD").stdout + hand_edit(holder, MEMBER, GLOB) + before = state(holder, env) + + result = run_script(FAMILY, *verb_args("bump", holder, family), env=env) + + assert result.returncode == 2, result.stdout + result.stderr + assert refusal(result.stderr)[0].startswith("REFUSED member-path: ") + assert raw(star, env, "rev-parse", "HEAD").stdout == tip + assert state(holder, env) == before + neighbour_intact(holder, env) + + +# --- the controls ----------------------------------------------------------- + +@pytest.mark.parametrize("edit", ["as-written", "no-path-line"]) +@pytest.mark.parametrize("verb", ["remove", "bump"]) +def test_a_row_with_the_members_own_path_or_none_still_exits_zero_in_one_commit( + family, holder, offline, tmp_path, verb, edit): + """THE CONTROLS: the row the tool writes, and a row with no `path:` at + all, which keeps defaulting to `members/`. Each is still ONE + commit of exactly the paths the verb is for, and the holder still + validates.""" + env = isolated(offline, tmp_path) + if edit == "no-path-line": + hand_edit(holder, MEMBER, None) + was = commits(holder, env) + + result = run_script(FAMILY, *verb_args(verb, holder, family), env=env) + + assert result.returncode == 0, result.stdout + result.stderr + assert commits(holder, env) == was + 1 + if verb == "remove": + assert committed(holder, env) == {".gitmodules", "family.yaml", + f"members/{MEMBER}"} + assert [r["project"] for r in manifest(holder)["members"]] == [OTHER] + else: + assert committed(holder, env) == {"family.yaml", f"members/{MEMBER}"} + row = {r["project"]: r for r in manifest(holder)["members"]}[MEMBER] + assert row["path"] == f"members/{MEMBER}" + assert row["pin"]["commit"] == older(family) + assert validate(holder).returncode == 0 + assert not offline.gh_log.exists() + + +def test_the_one_definition_is_the_path_add_writes_and_the_check_of_a_row(): + """`_member_path` is what `_member_row` (the row `add` writes) calls, and + what `bump` and `remove` ask of a row: the same string, and a refusal for + any other, with no disk and no git.""" + module = family_module() + row = module._member_row(MEMBER, "irrs", f"{ORG}/{MEMBER}", "a" * 40, + "b" * 64) + assert row["path"] == module._member_path(MEMBER) == f"members/{MEMBER}" + for kept in ({}, {"path": None}, {"path": ""}, {"path": row["path"]}): + assert module._member_path(MEMBER, kept) == row["path"], kept + for spelling in (*SPELLINGS.values(), f"members\\{MEMBER}", + f"./members/{MEMBER}", " members/" + MEMBER): + with pytest.raises(module.Refusal) as caught: + module._member_path(MEMBER, {"path": spelling}) + assert caught.value.code == "member-path", spelling + + +# --- literal pathspecs ------------------------------------------------------ + +def reached_the_neighbour(call: str, root: Path, env: dict, + proc: subprocess.CompletedProcess) -> bool: + """Did `call`, given the glob `members/*`, act on `OTHER`, a member it was + never named for? Each answer is read from git, never from an exit code + alone.""" + if call == "deinit": + return not (root / "members" / OTHER / "project.yaml").exists() + if call == "rm": + return raw(root, env, "ls-files", "-s", "--", f"members/{OTHER}" + ).stdout == b"" + if call == "diff": + return proc.returncode == 1 + return proc.returncode == 0 and f"members/{OTHER}" in committed(root, env) + + +GIT_CALLS = { + "deinit": ("submodule", "deinit", "-f", "-q", "--", GLOB), + "rm": ("rm", "-r", "-q", "-f", "--", GLOB), + "diff": ("diff", "--quiet", "--ignore-submodules=dirty", "HEAD", "--", + GLOB), + "commit": ("commit", "-q", "-m", "a wrong commit", "--", GLOB), +} + + +@pytest.mark.parametrize("call", GIT_CALLS) +def test_the_global_flag_keeps_a_glob_from_reaching_a_neighbour_in_each_call( + holder, offline, tmp_path, call): + """EACH GIT CALL `bump` AND `remove` MAKE WITH THE MEMBER'S PATH, run with + the path `members/*`, on THIS git and on every runner's: the plain call + reaches the neighbour (the premise: a test whose glob matched nothing + would pass on any code), and the same call under the GLOBAL + `--literal-pathspecs` does not. `git submodule` is its own command, which + is why `deinit` is here; it honours the global option, which reaches it as + `GIT_LITERAL_PATHSPECS` in the environment.""" + env = isolated(offline, tmp_path) + literal_holder = tmp_path / "literal" + shutil.copytree(holder, literal_holder, symlinks=True) + if call in ("diff", "commit"): + # A neighbour with something to find: its gitlink moved and staged. + for root in (holder, literal_holder): + commit_in_member(root / "members" / OTHER) + raw(root, env, "add", "--", f"members/{OTHER}") + + plain = raw(holder, env, *GIT_CALLS[call], check=False) + assert reached_the_neighbour(call, holder, env, plain), ( + "premise: without the flag, the glob reaches the neighbour; " + + plain.stderr.decode("utf-8", "replace")) + + literal = raw(literal_holder, env, "--literal-pathspecs", + *GIT_CALLS[call], check=False) + assert not reached_the_neighbour(call, literal_holder, env, literal), ( + literal.stderr.decode("utf-8", "replace")) + assert (literal_holder / "members" / OTHER / "project.yaml").is_file() + assert raw(literal_holder, env, "ls-tree", "HEAD", "--", + f"members/{OTHER}").stdout.startswith(b"160000 commit ") + + +def recorded(monkeypatch) -> list[list[str]]: + """Every program this process runs from now on, argv by argv, still run + for real.""" + calls: list[list[str]] = [] + real = subprocess.run + + def record(argv, *rest, **named): + calls.append(list(argv)) + return real(argv, *rest, **named) + + monkeypatch.setattr(subprocess, "run", record) + return calls + + +def global_options(argv: list[str], subcommands: set[str]) -> list[str]: + """What stands between `git` and its subcommand.""" + at = next(i for i, word in enumerate(argv) if i and word in subcommands) + return argv[1:at] + + +@pytest.mark.parametrize("verb, subcommands", [ + ("remove", {"submodule", "rm", "commit"}), + ("bump", {"add", "diff", "commit"})]) +def test_every_git_call_that_takes_the_members_path_is_literal( + family, holder, offline, tmp_path, monkeypatch, verb, subcommands): + """THE FLAG IS THERE, in the place git takes global options, on every call + a real run of the verb makes with the member's path: `remove`'s `deinit`, + `rm` and `commit`; `bump`'s two stages, its `diff` and its `commit`. The + set is exact, so a new call that names the path is a test to extend rather + than a call that goes unchecked.""" + module = family_module() + for key, value in isolated(offline, tmp_path).items(): + monkeypatch.setenv(key, value) + calls = recorded(monkeypatch) + path = f"members/{OTHER if verb == 'remove' else MEMBER}" + args = verb_args(verb, holder, family) + if verb == "remove": + args[args.index(MEMBER)] = OTHER + + assert module.main(args) == 0 + + named = [argv for argv in calls if argv[0] == "git" and path in argv] + assert {argv[next(i for i, word in enumerate(argv) + if i and word in subcommands)] + for argv in named} == subcommands + for argv in named: + assert "--literal-pathspecs" in global_options(argv, subcommands), argv From 58aa4e3ae2328cbdd19717e36587041c85401358 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 01:59:43 +0000 Subject: [PATCH 2/4] family.py: `bump` and `remove` also refuse `member-path` for a row whose `project:` is `.` or `..`, because its `path:` is then equal to `members/` and passed the comparison, so a hand-edited `project: .` with `path: members/.` no longer makes `remove` unmount every member in one commit and `project: ..` no longer stages the holder's own files for deletion (#299) The first commit of #299 refuses a row whose `path:` is not exactly `members/`. A row hand-edited in BOTH fields, `project: .` and `path: members/.`, or `project: ..` and `path: members/..`, is equal to the path the tool computes for it, so it passed, and `remove` acted on a path that is not a member: - `project: .`: `remove --member .` exited 0 with ONE commit that deleted both members' gitlinks, `.gitmodules` and the manifest (18 deletions), because `git rm -r -f -- members/.` is the members directory itself. The literal-pathspecs flag does not help: a path that is a real directory is the same path literally. - `project: ..`: `git rm -r -f -- members/..` removed the holder's own files from the index, then `write_members` died on the `FileNotFoundError` of the `family.yaml` it had just deleted, exit 1, the tree staged for deletion. - `bump` was stopped by accident, not by design: `member-uninitialized` for `members/.`, and a `fatal: reference is not a tree` from the `git checkout` it ran in the holder's own root for `members/..`. `add` cannot write either row: it refuses the mount path as `member-path-taken` (probed: `add --member InkRouter/..` and `/.`), and `cmd_add` does not call `_member_path`, so `add`'s path and its refusal code are as they were. `_member_path(project, row)` now also refuses, under the same code, a row whose project is not a single directory name directly under `members/`: `.`, `..`, the empty name, or one with a `/`. The refusal says why, that the path would be `members/.` (the members directory itself) or `members/..` (the holder's root), not a member, and that no git command was run and nothing was written; its remediation says the tool never writes such a row, so restore `family.yaml` from git or remove the row by hand. The clause is made only when a row is given, so `_member_row`, which is what `add` writes, behaves as it did. `bump` and `remove` reach it through the call the first commit made; nothing in either verb changes. `MEMBER_RE` is NOT changed. It lives in `scripts/family.py` (line 104) and is read once, by `_checked_member`, which only `cmd_add` calls; `bump` and `remove` take `--member` through `checked_value` and `repo_basename` and never meet it, so tightening it would not have reached them. The validator's own `QUALIFIED_RE` (`templates/family-root/scripts/validate-family.py`, copied into every holder) already requires each segment to start with a letter or a digit. TESTS (`tests/test_family_member_path.py`): `remove` and `bump` against `project: .` with `path: members/.` and `project: ..` with `path: members/..` exit 2 with the refusal text, `git ls-files -s -z` and `git status --porcelain=v1 -z` byte-identical, HEAD unchanged, both members' gitlinks, rows and checkouts intact, and no `gh` call; the definition's own test covers `.`, `..`, the empty name and `a/b`, with and without a `path:`, and that no row means no refusal. MUTANT, the new clause dropped on a scratch copy: the four new rows and the definition's test fail, and the probe's wrong outcomes come back. `remove --member .` exits 0 with one commit deleting `members/IRRS` and `members/IRSS`, `.gitmodules` and the manifest's rows (4 files, 18 deletions); `remove --member ..` dies on the `FileNotFoundError` with the holder's files staged for deletion; `bump` falls through to the two accidents above. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Sonnet 5.5 --- scripts/family.py | 27 ++++++++++ tests/test_family_member_path.py | 87 ++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+) diff --git a/scripts/family.py b/scripts/family.py index b3c4d974..d14a8a60 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -286,8 +286,35 @@ def _member_path(project: str, row: dict | None = None) -> str: validator keeps its own spelling of this rule on purpose. It ships INTO every holder, as `repo_shape.py` does, so sharing it would move every holder's pin for a refusal of a hand edit. + + A ROW'S PROJECT HAS TO BE A DIRECTORY NAME TOO. Its `path:` is checked + against `members/`, so a row that says `project: ..` and `path: + members/..` is EQUAL to the expected path and passed the comparison, and + `remove` then ran `git rm -r -f -- members/..` over the holder's root, + while `project: .` and `path: members/.` unmounted every member in one + commit naming one (#299). Each takes two hand edits, the project and the + path, and `add` cannot write either: it refuses the mount path as + `member-path-taken`, so `_member_row` is never reached. A project that + does not make `members/` ONE directory directly under `members/` + has no path of its own to be compared with, so it is refused here, under + the same code, only when a row is given: what `add` writes (`_member_row`, + no row) is as it was. """ expected = f"{MEMBERS_DIR}/{project}" + if row is not None and (project in ("", ".", "..") or "/" in project): + names = {".": f"the {MEMBERS_DIR} directory itself", + "..": "the holder's root"}.get( + project, f"no single directory directly under {MEMBERS_DIR}/") + raise Refusal( + "member-path", + f"{project!r} is not a project name: its path would be " + f"{expected!r}, which is {names}, not a member. Acting on it " + "would unmount more than one member or remove the holder's own " + "files, so no git command was run and nothing was written.", + "Remediation: this tool never writes such a row, so it is a hand " + f"edit. Restore {MANIFEST} from git (`git checkout -- " + f"{MANIFEST}`) or remove the row by hand, then run the same " + "command again.") declared = expected if row is None else str(row.get("path") or expected) if declared != expected: raise Refusal( diff --git a/tests/test_family_member_path.py b/tests/test_family_member_path.py index 152e0ffb..8bb57479 100644 --- a/tests/test_family_member_path.py +++ b/tests/test_family_member_path.py @@ -24,6 +24,13 @@ (`GIT_CONFIG_GLOBAL`, `GIT_CONFIG_NOSYSTEM`). No test pushes to the fixture's own remotes. +THE ROW'S PROJECT HAS TO BE A DIRECTORY NAME TOO. A row hand-edited to +`project: .` and `path: members/.`, or `project: ..` and `path: members/..`, +IS `members/` and passes the comparison above; `remove --member .` +then unmounted every member in one commit naming one, and `--member ..` `git +rm -r -f`'d the holder's root and died on the missing manifest. The same +`member-path` refusal covers them (the second commit of #299). + A refusal is asked of the holder the way `test_family_add_refusals.py` asks it of `add`: not "is the sentence printed" but "is everything exactly as it was", down to the bytes of `git ls-files -s -z` and `git status --porcelain=v1 -z`, @@ -117,6 +124,24 @@ def hand_edit(root: Path, project: str, path: str | None) -> None: assert row.get("path") == path +def hand_edit_project(root: Path, project: str, as_project: str) -> None: + """`project`'s row renamed to `as_project` AND given the path that name + makes, `members/`: the two hand edits that leave a row equal + to the path the tool would compute for it. UNCOMMITTED, as `hand_edit` + leaves it.""" + manifest_path = root / "family.yaml" + text = manifest_path.read_bytes().decode("utf-8") + for old, new in ( + (f" - project: {project}\n", f" - project: {as_project}\n"), + (f" path: members/{project}\n", + f" path: members/{as_project}\n")): + assert text.count(old) == 1, "fixture drift: the row's own lines" + text = text.replace(old, new) + manifest_path.write_bytes(text.encode("utf-8")) + row = {r["project"]: r for r in manifest(root)["members"]}[as_project] + assert row["path"] == f"members/{as_project}" + + def state(root: Path, env: dict) -> dict: """Everything a refused verb must leave exactly as it found it.""" return { @@ -243,6 +268,57 @@ def test_bump_refuses_a_row_naming_a_directory_that_is_really_there( neighbour_intact(holder, env) +DOT_ROWS = { + "dot": (".", "the members directory itself"), + "dotdot": ("..", "the holder's root"), +} + + +@pytest.mark.parametrize("project, names", DOT_ROWS.values(), ids=DOT_ROWS) +@pytest.mark.parametrize("verb", ["remove", "bump"]) +def test_a_row_whose_project_is_not_a_directory_name_is_refused( + family, holder, offline, tmp_path, verb, project, names): + """`project: .` with `path: members/.`, and `project: ..` with `path: + members/..`: the row's path EQUALS `members/`, so only the + project can be refused. `remove` used to exit 0 with one commit deleting + every member's gitlink for the first, and to stage the holder's own files + for deletion and die on a `FileNotFoundError` for the second. Now the + same `member-path` refusal, exit 2, and the holder byte for byte as it + was, both members still mounted, pinned and checked out.""" + env = isolated(offline, tmp_path) + hand_edit_project(holder, MEMBER, project) + assert raw(holder, env, "status", "--porcelain=v1", "-z").stdout == ( + b" M family.yaml\0"), "premise: the hand edit is the only change" + before = state(holder, env) + args = verb_args(verb, holder, family) + args[args.index(MEMBER)] = project + + result = run_script(FAMILY, *args, env=env) + + assert result.returncode == 2, result.stdout + result.stderr + assert result.stdout == "", "refused before anything was reported" + said, remedy = refusal(result.stderr) + assert said == ( + f"REFUSED member-path: {project!r} is not a project name: its path " + f"would be 'members/{project}', which is {names}, not a member. " + "Acting on it would unmount more than one member or remove the " + "holder's own files, so no git command was run and nothing was " + "written.") + assert remedy == ( + "Remediation: this tool never writes such a row, so it is a hand " + "edit. Restore family.yaml from git (`git checkout -- family.yaml`) " + "or remove the row by hand, then run the same command again.") + assert state(holder, env) == before + for mounted in MEMBERS: + entry = raw(holder, env, "ls-tree", "HEAD", "--", f"members/{mounted}" + ).stdout + assert entry.startswith(b"160000 commit "), entry + assert (holder / "members" / mounted / "project.yaml").is_file() + assert [r["project"] for r in manifest(holder)["members"]] == [project, + OTHER] + assert not offline.gh_log.exists(), offline.gh_log.read_text() + + # --- the controls ----------------------------------------------------------- @pytest.mark.parametrize("edit", ["as-written", "no-path-line"]) @@ -290,6 +366,17 @@ def test_the_one_definition_is_the_path_add_writes_and_the_check_of_a_row(): with pytest.raises(module.Refusal) as caught: module._member_path(MEMBER, {"path": spelling}) assert caught.value.code == "member-path", spelling + # A project that is not one directory name has no path to compare: the row + # is refused under the same code, whatever its `path:` says (or does not). + for project in ("", ".", "..", "a/b"): + for named in ({}, {"path": f"members/{project}"}): + with pytest.raises(module.Refusal) as caught: + module._member_path(project, named) + assert caught.value.code == "member-path", (project, named) + # What `add` writes, with no row, is as it was. + assert module._member_path("..") == "members/.." + assert module._member_row("..", "x", f"{ORG}/..", "a" * 40, "b" * 64 + )["path"] == "members/.." # --- literal pathspecs ------------------------------------------------------ From 2f0408598fca57b704268f021ac386a875f7e9a0 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 02:37:26 +0000 Subject: [PATCH 3/4] family.py: `_commit_one` commits with `:(literal)` on each path instead of the global `--literal-pathspecs` so the holder's commit hooks no longer inherit `GIT_LITERAL_PATHSPECS`, the `.`/`..` refusal names a remedy that can work, `cmd_add` takes its path from `_member_path`, and `add` refuses a `.` or `..` segment as `member-malformed` so `add --member /..` on a fresh holder no longer tells the human to stage the deletion of every file (#299) The review of 58aa4e3 found no MUST and two SHOULDs, and the coordinator ruled those, the one nit that matters and an out-of-scope finding in. 1. THE COMMIT'S PATHS ARE `:(literal)`, NOT GLOBAL. `git --literal-pathspecs` works by putting `GIT_LITERAL_PATHSPECS=1` in git's environment, and every child of `git commit` inherits it: the holder's `pre-commit`, `commit-msg` and `post-commit` hooks, its own or the operator's `core.hooksPath`. On git 2.43 a hook that picks the staged files with a glob (`git diff --cached --name-only -- '*.yaml'`) saw `family.yaml` on a plain commit and NOTHING under the flag, so a linter behind it checked nothing on every `add`, `bump` and `remove` commit. `_commit_one` now passes `-- :(literal) ...` on the commit, and no global option. Proved on 2.43.0 on the form `git commit -q -F - -- `: `:(literal)members/*` is refused ("did not match any file(s)") while a neighbour's gitlink is staged, a path `git rm` has taken out of the index and the tree still commits (HEAD has it), and the hook sees the variable unset. `deinit`, `rm` and `diff` run no hooks and keep the global option; `force_stage`'s `add`, which is shared, is untouched. The failed commit an unwound `add` shows is now `git commit -q -F - -- :(literal).gitmodules :(literal)members/IRQS :(literal)family.yaml`, and `test_family_add_refusals.py` pins that. 2. THE `.`/`..` REFUSAL'S REMEDIATION. "Run the same command again" cannot succeed after either remedy (`--member .` then names no row), and "remove the row by hand" leaves the member's gitlink with no row, which the validator and the doctor both pass. It now says: restore `family.yaml` from git (`git checkout -- family.yaml` if the edit is not committed), or put the row's `project:` and `path:` back as the tool wrote them, then run the command again naming that member. 3. ONE DEFINITION. `cmd_add` spelled `members/` itself, so the definition had two spellings in one file. It calls `_member_path(project)` now, with no row: the same string, the same behaviour. 4. `add --member /..` ON A FRESH HOLDER. `MEMBER_RE`'s class admits the dot (for `.github`), so `/..` matched, and its mount path `members/..` is the holder's root. With no `members/` directory, `_checked_mount_path` found the root's own files "in the index" under it and printed the remediation `git rm -r --cached -- members/..`, which stages the deletion of EVERY file the holder has (the reviewer pasted it on a scratch copy: 15 deletions). `_checked_member` now refuses a segment of `.` or `..`, owner or project, as `member-malformed`, add's existing argument refusal, before the member is read or anything is mounted, and its message says a directory reference is not a name and where the project would mount. `MEMBER_RE` (in `scripts/family.py`, read only there) is left as it was: `.github` stays legal. The `_member_path` docstring no longer says `add` refuses these as `member-path-taken`; that held only once `members/` existed. TESTS (`tests/test_family_member_path.py`, 28 -> 40 in the file). - A holder with a `pre-commit` hook that writes `GIT_LITERAL_PATHSPECS` and the staged files a `'*.yaml'` glob finds: through a real `add`, `bump` and `remove` it sees the variable unset and `family.yaml`. - The every-call recorder test now asserts `:(literal)` on each of the commit's paths, and the global option's absence, for the commit, and the global option on the other calls; the glob-reaches-a-neighbour test runs the commit in its magic form. - `add --member` of `/..`, `/.`, `../` and `./` is refused `member-malformed` on a fresh holder (no `members/`) and on one with members: exit 2, nothing on stdout, no clone, no `gh` call, the holder byte for byte as it was; names with a dot inside (`.github`, `a.b`) still pass. - The dot refusal's remediation text is pinned. MUTANTS, each on a scratch copy. - The global flag back on the commit: the hook test fails for all three verbs (the hook logs `literal=1` and its glob finds nothing), both every-call tests fail, and the pinned commit line in `test_family_add_refusals.py` fails; unmutated, all pass. - The `add` dot-segment refusal dropped: the eight `member-malformed` rows fail, and the fresh holder's `add --member InkRouter/..` prints the `git rm -r --cached -- members/..` remediation again. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Sonnet 5.5 --- scripts/family.py | 57 +++++++-- tests/test_family_add_refusals.py | 4 +- tests/test_family_member_path.py | 206 ++++++++++++++++++++++++++---- 3 files changed, 230 insertions(+), 37 deletions(-) diff --git a/scripts/family.py b/scripts/family.py index d14a8a60..a14ecf48 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -293,8 +293,9 @@ def _member_path(project: str, row: dict | None = None) -> str: `remove` then ran `git rm -r -f -- members/..` over the holder's root, while `project: .` and `path: members/.` unmounted every member in one commit naming one (#299). Each takes two hand edits, the project and the - path, and `add` cannot write either: it refuses the mount path as - `member-path-taken`, so `_member_row` is never reached. A project that + path, and `add` cannot write either: `_checked_member` refuses a `.` or + `..` segment as `member-malformed` before the member is read, so + `_member_row` is never reached with one. A project that does not make `members/` ONE directory directly under `members/` has no path of its own to be compared with, so it is refused here, under the same code, only when a row is given: what `add` writes (`_member_row`, @@ -313,8 +314,10 @@ def _member_path(project: str, row: dict | None = None) -> str: "files, so no git command was run and nothing was written.", "Remediation: this tool never writes such a row, so it is a hand " f"edit. Restore {MANIFEST} from git (`git checkout -- " - f"{MANIFEST}`) or remove the row by hand, then run the same " - "command again.") + f"{MANIFEST}` if the edit is not committed), or put the row's " + "`project:` and `path:` back as the tool wrote them " + f"(`project: `, `path: {MEMBERS_DIR}/`), then " + "run the command again naming that member.") declared = expected if row is None else str(row.get("path") or expected) if declared != expected: raise Refusal( @@ -356,6 +359,32 @@ def _checked_member(raw: str) -> tuple[str, str]: "Remediation: name the owner — a family's members may live in " "another organisation, so there is no owner to assume. For " "`remove` and `bump`, the bare `` is what is wanted.") + # A SEGMENT THAT IS `.` OR `..` IS A DIRECTORY REFERENCE, NOT A NAME + # (#299). `MEMBER_RE`'s class admits the dot, for `.github`, so `/..` + # matched, and its mount path `members/..` is the holder's root: on a + # fresh holder `_checked_mount_path` found the root's own files "in the + # index" under it and told the human to `git rm -r --cached -- members/..`, + # which stages the deletion of every file the holder has. The class is + # left as it is; only the two names that are no name are refused, before + # the member is read or anything is mounted. An owner of `.` or `..` + # names no account. + owner, _, project = name.partition("/") + for which, segment in (("owner", owner), ("project", project)): + if segment not in (".", ".."): + continue + if which == "owner": + why = "no organisation or account is called that" + else: + where = ("the members directory itself" if segment == "." + else "the holder's root") + why = f"it would mount at {MEMBERS_DIR}/{segment}, {where}" + raise Refusal( + "member-malformed", + f"--member {raw!r} has `{segment}` as its {which}, which is a " + f"directory reference and not a name: {why}", + "Remediation: name the repository as `/`, each a " + "real name; `.` and `..` are neither, though a name may contain " + "a dot (`.github`).") checked_value("--member", name) return name, repo_basename(name) @@ -496,13 +525,25 @@ def _commit_one(root: Path, message: str, paths: list[str], committed whatever else it matched: with `members/*` named, the move of every OTHER member's gitlink somebody had staged went into this commit beside the one it was for. `bump` and `remove` hand it a path that is - always `members/` (`_member_path`); the flag is the guard that + always `members/` (`_member_path`); the magic is the guard that does not depend on that being so. + + PER-PATHSPEC `:(literal)`, NOT THE GLOBAL `--literal-pathspecs`, ON THIS + ONE CALL. The global option works by putting `GIT_LITERAL_PATHSPECS=1` in + git's environment, and every child of `git commit` inherits it: the + holder's `pre-commit`, `commit-msg` and `post-commit` hooks, its own or the + operator's `core.hooksPath`. A hook that picks the staged files with a + glob (`git diff --cached --name-only -- '*.yaml'`) then matched nothing, + and a linter behind it checked nothing, on every `add`, `bump` and + `remove` commit. The magic is spelled on each path and reaches no hook. It + is right for a path `git rm` took out of the index and the tree, which the + commit still matches in HEAD. `deinit`, `rm` and `diff` run no hooks, so + they keep the global option. """ force_stage(root, paths if add is None else add) message, trailer_args = commit_trailers(message, trailers) - args = ["git", "--literal-pathspecs", "commit", "-q", "-F", "-", - *trailer_args, "--", *paths] + args = ["git", "commit", "-q", "-F", "-", *trailer_args, "--", + *[f":(literal){path}" for path in paths]] check_program(args) env = dict(os.environ) for key, fallback in (("GIT_AUTHOR_NAME", "openRepoShape family"), @@ -1694,7 +1735,7 @@ def cmd_add(args) -> int: """ family = Family.open(args.family_root) repository, project = _checked_member(args.member) - path = f"{MEMBERS_DIR}/{project}" + path = _member_path(project) at = _checked_add_target(family, repository, project, path, args.at) url = _member_url(repository, args.local_remote_dir) diff --git a/tests/test_family_add_refusals.py b/tests/test_family_add_refusals.py index deecce40..c46759b0 100644 --- a/tests/test_family_add_refusals.py +++ b/tests/test_family_add_refusals.py @@ -732,8 +732,8 @@ def test_a_commit_that_fails_unwinds_the_mount_and_the_pin(family, holder, f"mounting {ORG}/{SPARE} at members/{SPARE}\n") assert result.stderr.splitlines() == [ f"REFUSED committing {SPARE} to the family. THE EXACT COMMAND WAS:", - " git --literal-pathspecs commit -q -F - -- .gitmodules " - f"members/{SPARE} family.yaml", + " git commit -q -F - -- :(literal).gitmodules " + f":(literal)members/{SPARE} :(literal)family.yaml", f" (in {holder.resolve()})", " exit 1", "--- output ---", diff --git a/tests/test_family_member_path.py b/tests/test_family_member_path.py index 8bb57479..b6a12470 100644 --- a/tests/test_family_member_path.py +++ b/tests/test_family_member_path.py @@ -11,8 +11,10 @@ holder's own files and die on the missing manifest. `validate-family.py` has a rule for it (`member-path`), but it runs after a commit, never in front of these two verbs. So they refuse `member-path` themselves, before any git call -and any write; and every git call that takes the path takes it literally, which -does not depend on the row being right. +and any write; and every git call that takes the path takes it literally (the +global `--literal-pathspecs` on `deinit`, `rm` and `diff`; per-pathspec +`:(literal)` on the commit, which runs the holder's hooks and must not hand +them `GIT_LITERAL_PATHSPECS`), which does not depend on the row being right. THE ISSUE'S MEMBERS ARE `Atlas` AND `Borg`; this family's are `IRRS` and `IRSS`, so `IRRS` is the row a test edits and `IRSS` is the neighbour that must @@ -52,9 +54,10 @@ #: 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, MEMBERS, NAME, ORG, commit_in_member, family, family_module, - manifest, validate, + FAMILY, MEMBERS, NAME, ORG, SPARE, commit_in_member, family, + family_module, manifest, validate, ) +from test_family_add_excludes import empty_holder # noqa: F401 from test_family_add_refusals import ( # noqa: F401 holder, offline, refusal, snapshot, ) @@ -306,8 +309,11 @@ def test_a_row_whose_project_is_not_a_directory_name_is_refused( "written.") assert remedy == ( "Remediation: this tool never writes such a row, so it is a hand " - "edit. Restore family.yaml from git (`git checkout -- family.yaml`) " - "or remove the row by hand, then run the same command again.") + "edit. Restore family.yaml from git (`git checkout -- family.yaml` " + "if the edit is not committed), or put the row's `project:` and " + "`path:` back as the tool wrote them (`project: `, `path: " + "members/`), then run the command again naming that " + "member.") assert state(holder, env) == before for mounted in MEMBERS: entry = raw(holder, env, "ls-tree", "HEAD", "--", f"members/{mounted}" @@ -319,6 +325,78 @@ def test_a_row_whose_project_is_not_a_directory_name_is_refused( assert not offline.gh_log.exists(), offline.gh_log.read_text() +# --- `add`: a `.` or `..` segment is not a name ----------------------------- + +#: `--member` spellings with a segment that is a directory reference, and the +#: sentence the refusal ends on: the one thing wrong with each. +DIRECTORY_REFERENCES = { + "project-dotdot": (f"{ORG}/..", "..", "project", + "it would mount at members/.., the holder's root"), + "project-dot": (f"{ORG}/.", ".", "project", + "it would mount at members/., the members directory " + "itself"), + "owner-dotdot": (f"../{SPARE}", "..", "owner", + "no organisation or account is called that"), + "owner-dot": (f"./{SPARE}", ".", "owner", + "no organisation or account is called that"), +} + + +@pytest.mark.parametrize("spelling, segment, which, why", + DIRECTORY_REFERENCES.values(), + ids=DIRECTORY_REFERENCES) +@pytest.mark.parametrize("kind", ["fresh-holder", "holder-with-members"]) +def test_add_refuses_a_dot_segment_as_malformed_before_anything_is_read( + family, offline, tmp_path, request, kind, spelling, segment, which, + why): + """`add --member /..` matched `MEMBER_RE` (its class admits the dot, + for `.github`) and its mount path `members/..` is the holder's root. On a + FRESH holder `member-path-taken` then found the root's own files in the + index under it and told the human to run `git rm -r --cached -- + members/..`, which stages the deletion of EVERY file the holder has. A + segment of `.` or `..` is a directory reference, not a name, so it is the + argument refusal `member-malformed`, before the member is read: not one + line on stdout (`checking ... before mounting it` comes first otherwise), + no clone, no `gh` call, and the holder byte for byte as it was.""" + env = isolated(offline, tmp_path) + if kind == "fresh-holder": + root = request.getfixturevalue("empty_holder") + assert not (root / "members").exists(), "premise: a fresh holder" + remotes = tmp_path / "remotes" + else: + root = request.getfixturevalue("holder") + assert (root / "members").is_dir() + remotes = family["remotes"] + before = state(root, env) + + result = run_script(FAMILY, "add", "--family-root", str(root), + "--member", spelling, "--local-remote-dir", + str(remotes), env=env) + + assert result.returncode == 2, result.stdout + result.stderr + assert result.stdout == "", "refused before the member was even read" + said, remedy = refusal(result.stderr) + assert said == ( + f"REFUSED member-malformed: --member {spelling!r} has `{segment}` as " + f"its {which}, which is a directory reference and not a name: {why}") + assert remedy == ( + "Remediation: name the repository as `/`, each a real " + "name; `.` and `..` are neither, though a name may contain a dot " + "(`.github`).") + assert state(root, env) == before + offline.assert_left_nothing() + + +def test_add_still_takes_a_name_that_contains_a_dot(): + """`MEMBER_RE`'s class is not narrowed: `.github` is a name, and only the + two directory references are refused.""" + module = family_module() + assert module._checked_member(f"{ORG}/.github") == (f"{ORG}/.github", + ".github") + assert module._checked_member(f".github/{SPARE}")[1] == SPARE + assert module._checked_member(f"{ORG}/a.b")[1] == "a.b" + + # --- the controls ----------------------------------------------------------- @pytest.mark.parametrize("edit", ["as-written", "no-path-line"]) @@ -405,16 +483,26 @@ def reached_the_neighbour(call: str, root: Path, env: dict, } +def literal_form(call: str) -> list[str]: + """`call` as the tool spells it: the global option in front, except on the + commit, whose path is `:(literal)`-prefixed instead (see `_commit_one`).""" + if call == "commit": + return [word if word != GLOB else f":(literal){GLOB}" + for word in GIT_CALLS[call]] + return ["--literal-pathspecs", *GIT_CALLS[call]] + + @pytest.mark.parametrize("call", GIT_CALLS) -def test_the_global_flag_keeps_a_glob_from_reaching_a_neighbour_in_each_call( +def test_a_literal_spelling_keeps_a_glob_off_the_neighbour_in_each_call( holder, offline, tmp_path, call): """EACH GIT CALL `bump` AND `remove` MAKE WITH THE MEMBER'S PATH, run with the path `members/*`, on THIS git and on every runner's: the plain call reaches the neighbour (the premise: a test whose glob matched nothing - would pass on any code), and the same call under the GLOBAL - `--literal-pathspecs` does not. `git submodule` is its own command, which - is why `deinit` is here; it honours the global option, which reaches it as - `GIT_LITERAL_PATHSPECS` in the environment.""" + would pass on any code), and the same call in the tool's literal spelling + does not: the GLOBAL `--literal-pathspecs` for `deinit`, `rm` and `diff`, + `:(literal)` on the path for the commit. `git submodule` is its own + command, which is why `deinit` is here; it honours the global option, + which reaches it as `GIT_LITERAL_PATHSPECS` in the environment.""" env = isolated(offline, tmp_path) literal_holder = tmp_path / "literal" shutil.copytree(holder, literal_holder, symlinks=True) @@ -429,8 +517,7 @@ def test_the_global_flag_keeps_a_glob_from_reaching_a_neighbour_in_each_call( "premise: without the flag, the glob reaches the neighbour; " + plain.stderr.decode("utf-8", "replace")) - literal = raw(literal_holder, env, "--literal-pathspecs", - *GIT_CALLS[call], check=False) + literal = raw(literal_holder, env, *literal_form(call), check=False) assert not reached_the_neighbour(call, literal_holder, env, literal), ( literal.stderr.decode("utf-8", "replace")) assert (literal_holder / "members" / OTHER / "project.yaml").is_file() @@ -458,16 +545,26 @@ def global_options(argv: list[str], subcommands: set[str]) -> list[str]: return argv[1:at] -@pytest.mark.parametrize("verb, subcommands", [ - ("remove", {"submodule", "rm", "commit"}), - ("bump", {"add", "diff", "commit"})]) +def subcommand(argv: list[str], subcommands: set[str]) -> str: + return next(word for i, word in enumerate(argv) + if i and word in subcommands) + + +@pytest.mark.parametrize("verb, flagged, commit_paths", [ + ("remove", {"submodule", "rm"}, + [".gitmodules", f"members/{OTHER}", "family.yaml"]), + ("bump", {"add", "diff"}, [f"members/{MEMBER}", "family.yaml"])]) def test_every_git_call_that_takes_the_members_path_is_literal( - family, holder, offline, tmp_path, monkeypatch, verb, subcommands): - """THE FLAG IS THERE, in the place git takes global options, on every call - a real run of the verb makes with the member's path: `remove`'s `deinit`, - `rm` and `commit`; `bump`'s two stages, its `diff` and its `commit`. The - set is exact, so a new call that names the path is a test to extend rather - than a call that goes unchecked.""" + family, holder, offline, tmp_path, monkeypatch, verb, flagged, + commit_paths): + """THE LITERAL SPELLING IS THERE on every call a real run of the verb makes + with the member's path. `remove`'s `deinit` and `rm`, and `bump`'s two + stages and its `diff`, carry `--literal-pathspecs` in the place git takes + global options. The COMMIT of both carries `:(literal)` on each of its + paths and NOT the global option: that option is `GIT_LITERAL_PATHSPECS` in + the environment of every hook the commit runs. The set of calls is exact, + so a new call that names the path is a test to extend rather than a call + that goes unchecked.""" module = family_module() for key, value in isolated(offline, tmp_path).items(): monkeypatch.setenv(key, value) @@ -476,12 +573,67 @@ def test_every_git_call_that_takes_the_members_path_is_literal( args = verb_args(verb, holder, family) if verb == "remove": args[args.index(MEMBER)] = OTHER + subcommands = {*flagged, "commit"} assert module.main(args) == 0 - named = [argv for argv in calls if argv[0] == "git" and path in argv] - assert {argv[next(i for i, word in enumerate(argv) - if i and word in subcommands)] - for argv in named} == subcommands + named = [argv for argv in calls if argv[0] == "git" + and (path in argv or f":(literal){path}" in argv)] + assert {subcommand(argv, subcommands) for argv in named} == subcommands for argv in named: - assert "--literal-pathspecs" in global_options(argv, subcommands), argv + options = argv[1:argv.index(subcommand(argv, subcommands))] + if subcommand(argv, subcommands) == "commit": + assert "--literal-pathspecs" not in options, argv + assert argv[argv.index("--") + 1:] == [ + f":(literal){each}" for each in commit_paths], argv + else: + assert "--literal-pathspecs" in options, argv + + +def with_a_glob_hook(root: Path, tmp_path: Path) -> Path: + """A `pre-commit` hook in `root` that writes what a glob-picking hook sees: + the variable git's own `--literal-pathspecs` would put in its environment, + and the staged files `git diff --cached --name-only -- '*.yaml'` finds, one + per line. Returns the log. `sh`, which git runs hooks with on every OS + (git for Windows ships its own); LOCAL `core.hooksPath`, so a global one + cannot point git somewhere this hook is not.""" + log = tmp_path / "hook.log" + hooks = tmp_path / "hooks" + hooks.mkdir() + (hooks / "pre-commit").write_bytes(( + "#!/bin/sh\n" + f"{{ echo \"literal=${{GIT_LITERAL_PATHSPECS-unset}}\"\n" + " git diff --cached --name-only -- '*.yaml'; } >> " + f"'{log.as_posix()}'\n" + "exit 0\n").encode("utf-8")) + (hooks / "pre-commit").chmod(0o755) + git("config", "--local", "core.hooksPath", hooks.as_posix(), cwd=root) + return log + + +@pytest.mark.parametrize("verb", ["add", "bump", "remove"]) +def test_the_commit_hooks_see_no_literal_pathspecs_and_find_the_manifest( + family, holder, offline, tmp_path, verb): + """THE GLOBAL `--literal-pathspecs` ON THE COMMIT LEAKED INTO THE HOLDER'S + HOOKS. It is `GIT_LITERAL_PATHSPECS=1` in git's environment, which every + child of `git commit` inherits, so a `pre-commit` hook that picked the + staged files with a glob matched nothing on every `add`, `bump` and + `remove` commit: a linter behind it checked nothing and said nothing. The + commit takes `:(literal)` on its paths now. Through a real `add`, `bump` + and `remove`, the hook sees no such variable, and its glob finds the + manifest the commit is of.""" + env = isolated(offline, tmp_path) + log = with_a_glob_hook(holder, tmp_path) + if verb == "add": + args = ["add", "--family-root", str(holder), "--member", + f"{ORG}/{SPARE}", "--local-remote-dir", + str(family["remotes"])] + else: + args = verb_args(verb, holder, family) + + result = run_script(FAMILY, *args, env=env) + + assert result.returncode == 0, result.stdout + result.stderr + assert log.is_file(), "premise: the hook ran" + assert log.read_bytes().decode("utf-8").split() == ["literal=unset", + "family.yaml"] From 8f8eb925f1c083ea41b5286485fd645a86bf7cf3 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 04:23:45 +0000 Subject: [PATCH 4/4] family.py: `_commit_one` drops the caller's `GIT_LITERAL_PATHSPECS` from its commit's environment, because that variable turns off the pathspec magic `:(literal)` is, so a caller who exports it no longer fails every `add`, `bump` and `remove` commit with `pathspec ':(literal)family.yaml' did not match` (#299) The re-check of 2f04085 found that `:(literal)` works only while pathspec magic is on. `GIT_LITERAL_PATHSPECS=1` turns magic off, so with it in the caller's environment git read `:(literal)family.yaml` as a file of that name, found none, and exited 1. `_commit_one` copies `os.environ` into the commit's environment, so the caller's value reached git: - `add` exited 2 with the failed commit shown, and unwound the mount; - `bump` and `remove` exited 2 with no rollback: the rewritten pin staged and the member checked out at `--to`, or the unmount and the dropped row staged and no commit; - main and 58aa4e3, whose commit used the global option, exited 0 in that environment, so this was a regression against both. Who has the variable set: a person or script that exports it as a safety default, or any `family.py` run from a hook or alias of a `git --literal-pathspecs` command, which exports it to its children. The fix is one line, `env.pop("GIT_LITERAL_PATHSPECS", None)`, on the environment `_commit_one` already builds for its commit, and only there. The commit's paths are literal by their `:(literal)` magic and need nothing from the environment; the hooks it runs then see the variable unset, as the hook test of the previous commit pins. The other pathspec variables (`GIT_GLOB_PATHSPECS`, `GIT_NOGLOB_PATHSPECS`, `GIT_ICASE_PATHSPECS`) leave magic on and are not touched; the reviewer found each harmless on 2.43. Wording: the `_commit_one` docstring said `deinit`, `rm` and `diff` "run no hooks". `git rm`, like `force_stage`'s `git add`, writes the index and so runs a `post-index-change` hook, which still sees `GIT_LITERAL_PATHSPECS=1`; it now says "no COMMIT hooks" and names that hook. The unused `global_options` helper in `tests/test_family_member_path.py` is removed. TEST (`tests/test_family_member_path.py`, 40 -> 43): with `GIT_LITERAL_PATHSPECS=1` exported to the run, `add`, `bump` and `remove` each exit 0 in ONE commit of exactly the paths each is for, leave nothing staged behind (`git status --porcelain=v1 -z` empty), and the holder validates. MUTANT, the `env.pop` dropped on a scratch copy: the three new rows fail with `error: pathspec ':(literal)family.yaml' did not match any file(s) known to git` on `git commit -q -F - -- :(literal)...`, exit 1; the other 40 pass. With the pop, all 43 pass. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Sonnet 5.5 --- scripts/family.py | 18 +++++++++++-- tests/test_family_member_path.py | 45 +++++++++++++++++++++++++++----- 2 files changed, 55 insertions(+), 8 deletions(-) diff --git a/scripts/family.py b/scripts/family.py index a14ecf48..583037b2 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -537,8 +537,21 @@ def _commit_one(root: Path, message: str, paths: list[str], and a linter behind it checked nothing, on every `add`, `bump` and `remove` commit. The magic is spelled on each path and reaches no hook. It is right for a path `git rm` took out of the index and the tree, which the - commit still matches in HEAD. `deinit`, `rm` and `diff` run no hooks, so - they keep the global option. + commit still matches in HEAD. `deinit`, `rm` and `diff` run no COMMIT + hooks, so they keep the global option (`rm`, like `force_stage`'s `add`, + writes the index, so a `post-index-change` hook still sees the variable + after them; that is theirs and predates this). + + THE CALLER'S OWN `GIT_LITERAL_PATHSPECS` IS DROPPED FROM THIS ONE COMMIT'S + ENVIRONMENT. The variable turns pathspec magic OFF, and `:(literal)` is + magic: with it exported (a safety default, or a `family.py` run from a + hook or alias of a `git --literal-pathspecs` command, which exports it to + its children) git took `:(literal)family.yaml` as a file of that name, + found none, and every `add`, `bump` and `remove` commit failed, `bump` and + `remove` leaving the pin or the unmount staged with no rollback. The + commit's paths are literal by their magic and need no help from the + environment. Only this variable: the other pathspec variables turn magic + ON or change only how a glob matches, and the magic still reads. """ force_stage(root, paths if add is None else add) message, trailer_args = commit_trailers(message, trailers) @@ -546,6 +559,7 @@ def _commit_one(root: Path, message: str, paths: list[str], *[f":(literal){path}" for path in paths]] check_program(args) env = dict(os.environ) + env.pop("GIT_LITERAL_PATHSPECS", None) for key, fallback in (("GIT_AUTHOR_NAME", "openRepoShape family"), ("GIT_COMMITTER_NAME", "openRepoShape family"), ("GIT_AUTHOR_EMAIL", "family@openreposhape.invalid"), diff --git a/tests/test_family_member_path.py b/tests/test_family_member_path.py index b6a12470..c9f72759 100644 --- a/tests/test_family_member_path.py +++ b/tests/test_family_member_path.py @@ -539,12 +539,6 @@ def record(argv, *rest, **named): return calls -def global_options(argv: list[str], subcommands: set[str]) -> list[str]: - """What stands between `git` and its subcommand.""" - at = next(i for i, word in enumerate(argv) if i and word in subcommands) - return argv[1:at] - - def subcommand(argv: list[str], subcommands: set[str]) -> str: return next(word for i, word in enumerate(argv) if i and word in subcommands) @@ -637,3 +631,42 @@ def test_the_commit_hooks_see_no_literal_pathspecs_and_find_the_manifest( assert log.is_file(), "premise: the hook ran" assert log.read_bytes().decode("utf-8").split() == ["literal=unset", "family.yaml"] + + +#: What each verb's ONE commit is of, as git reports it. +COMMITTED = { + "add": {".gitmodules", f"members/{SPARE}", "family.yaml"}, + "bump": {f"members/{MEMBER}", "family.yaml"}, + "remove": {".gitmodules", f"members/{MEMBER}", "family.yaml"}, +} + + +@pytest.mark.parametrize("verb", ["add", "bump", "remove"]) +def test_a_caller_who_exports_literal_pathspecs_still_gets_one_commit( + family, holder, offline, tmp_path, verb): + """`:(literal)` is pathspec MAGIC, and `GIT_LITERAL_PATHSPECS=1` turns + magic off: with it in the caller's environment git read `:(literal) + family.yaml` as a file of that name and found none, so every commit of + `add`, `bump` and `remove` failed (`bump` and `remove` with the pin or the + unmount left staged and no rollback), where main and the first form of + this fix exited 0. A safety default, or a `family.py` run from a hook or an + alias of `git --literal-pathspecs`, puts it there. The commit drops that + one variable from its own environment: with it exported, each verb exits + 0 in ONE commit of exactly the paths it is for, and leaves nothing staged + behind.""" + env = {**isolated(offline, tmp_path), "GIT_LITERAL_PATHSPECS": "1"} + was = commits(holder, env) + if verb == "add": + args = ["add", "--family-root", str(holder), "--member", + f"{ORG}/{SPARE}", "--local-remote-dir", + str(family["remotes"])] + else: + args = verb_args(verb, holder, family) + + result = run_script(FAMILY, *args, env=env) + + assert result.returncode == 0, result.stdout + result.stderr + assert commits(holder, env) == was + 1 + assert committed(holder, env) == COMMITTED[verb] + assert raw(holder, env, "status", "--porcelain=v1", "-z").stdout == b"" + assert validate(holder).returncode == 0