From 03ddadd645eca7991acd7a537b5256dc8e3480d2 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 04:16:22 +0000 Subject: [PATCH 1/4] family.py: `add` refuses `member-path-registered` when a `.gitmodules` section under another name already gives the mount path, and `validate-family.py` finds `gitmodules-path-twice`, so `add` no longer commits two sections for one path that a recursive clone resolves to the later one, and a holder that already has such a ghost section fails its own gate (#300) `add` asked the disk, the index, git's dry run and the NAMES in `.gitmodules` (#242's `member-name-taken`), but never whether a section under ANOTHER name already gave the mount path. git writes the member's own section beside such a "ghost" without a word, so `add` exited 0 and committed two sections for one path, `validate-family.py` said `family ok`, and a recursive clone followed the section LAST in the file: with the ghost second it died fetching the ghost's url. A ghost is what a half-removed member, a hand edit or a merge that kept both sides leaves behind. `_checked_mount_path` now refuses it, in its own block after `member-name-taken`'s, before the member is read or anything is mounted. `member-path-registered` names each such section and its url (or that it has none). The remediation is the removal by hand, `git config -f .gitmodules --remove-section submodule.` (quoted for a shell) and then `git add .gitmodules`, or `family.py remove` if it was a member. The section is never dropped or rewritten as part of the add: it is somebody's registration. It is read through the reader the name check already uses, `_submodule_paths`, extended in place to return each section's url beside its path, so it reads the copy git reads: the working tree's, else the index's, else HEAD's. The path is compared exactly. A section whose name IS the mount path (what `remove` leaves when a ghost followed the member) is not a ghost, and git rewrites it in place. `validate-family.py` gains `gitmodules-path-twice`: two or more sections of the holder's `.gitmodules` with one path. It reads the working-tree file with `git config -z --file .gitmodules --list`, as `family.py` does, with no parser of its own, and it runs in every mode, `--pins` and `--no-members` too, like #206's duplicate id, because it needs nothing but that file. No `.gitmodules` is no finding; one git cannot read is the refusal `gitmodules-unreadable`, never a pass. The validator is a copied file, so a holder takes it through `update-shape.py`, which reports it `upstream-changed` for a holder cut before this commit. `--help` is unchanged, so `docs/cli.md` is too. Not changed here: `remove` after a ghost. With the ghost second, `remove` commits a `.gitmodules` that still holds `[submodule "members/

"]`, and leaves that section's registration in `.git/config`; with the ghost first, one that still holds the ghost. git's `rm` and `deinit` take out the one section git resolves the path to, the later. `remove` is #299's function this round, so that is a follow-up, as is a finding for a section whose path no row names. tests/test_family_add_refusals.py, appended: a section named `ghost` for `members/IRQS`, committed and in the work tree, only in the index's `.gitmodules`, or only in HEAD's, each built in git's index and objects: `add` exits 2 with `member-path-registered` before the member is read, nothing cloned or mounted, the holder snapshot byte-identical. The control, the plain add and an add past a section for `members/IRQSX` or a section named `members/IRQS` for its own path, exits 0 with one section for the path. Two ghosts, one named with a space and one with no url, are both named, and the printed commands, run as printed, clear the way for the same add. The validator finds a second section for `members/IRRS` in all three modes and passes one section per path; an unreadable `.gitmodules` is a refusal. Mutants: without the refusal the double section comes back and the three rows and the two-ghost test fail; with the reader on the working tree alone the index and HEAD rows fail, and so do #242's off-disk name-taken rows; without the validator check its two tests fail. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Opus 5.5 --- scripts/family.py | 65 +++++- .../family-root/scripts/validate-family.py | 73 +++++- tests/test_family_add_refusals.py | 213 ++++++++++++++++++ 3 files changed, 340 insertions(+), 11 deletions(-) diff --git a/scripts/family.py b/scripts/family.py index 88125222..72e32eca 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -67,6 +67,7 @@ import os import re import runpy +import shlex import shutil import stat import subprocess @@ -966,11 +967,18 @@ def _index_entries(root: Path, path: str) -> list[tuple[str, str]]: return entries -def _submodule_paths(root: Path) -> dict[str, str]: - """`{name: path}` of every submodule the holder's `.gitmodules` names, - read from where `git submodule add` reads it: the file in the working - tree, or the index's copy where there is none, or HEAD's where the index - has none either.""" +def _submodule_paths(root: Path) -> dict[str, tuple[str, str | None]]: + """`{name: (path, url)}` of every submodule the holder's `.gitmodules` + names, read from where `git submodule add` reads it: the file in the + working tree, or the index's copy where there is none, or HEAD's where + the index has none either. `url` is None for a section that gives no + url; a section that gives no path is no submodule and is not listed. + + THE ONE READER OF THE FILE, for both questions `_checked_mount_path` + asks of it: the path a NAME is given (`member-name-taken`, #242), and + the names a PATH is given, each with its url (`member-path-registered`, + #300). A second reader could read a second copy of the file. + """ if os.path.lexists(root / GITMODULES): source = ["--file", GITMODULES] else: @@ -985,14 +993,15 @@ def _submodule_paths(root: Path) -> dict[str, str]: return {} listed = git_out(["config", "-z", *source, "--list"], cwd=root, binary=True) - found = {} + found: dict[str, 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": - found[section[len("submodule."):]] = value - return found + if section.startswith("submodule.") and variable in ("path", "url"): + found.setdefault(section[len("submodule."):], {})[variable] = value + return {name: (keys["path"], keys.get("url")) + for name, keys in found.items() if "path" in keys} def _checked_mount_path(root: Path, project: str, path: str) -> None: @@ -1023,6 +1032,18 @@ def _checked_mount_path(root: Path, project: str, path: str) -> None: the member under a new one (`members/IRRS1`) instead. Git before 2.51 rewrites the other submodule's section to point at the member. Both are refused here, on every git. + - THE PATH UNDER ANOTHER NAME (#300), the mirror of the name: a section + that already gives the mount path to a submodule named something + else — a half-removed member, a hand edit, a merge that kept both + sides. This one git does not make at all, with `--force` or without + it: it writes the member's own section beside that one, so `add` + committed two sections for one path, `validate-family.py` passed + them, and a recursive clone followed the LATER of the two, the other + one's url as soon as an edit put it last. It is refused, naming the + section and its url, and left exactly as it is: it is somebody's + registration, and dropping or rewriting it as part of the add would + decide whose without asking. The path is compared exactly; git reads + `.gitmodules` that way too. """ if os.path.lexists(root / path): raise Refusal( @@ -1060,7 +1081,8 @@ def _checked_mount_path(root: Path, project: str, path: str) -> None: "names a submodule, take that out (`git rm`); anything else it " "names, a held `index.lock` say, is fixed where git says. Then " "re-run.") from exc - other = _submodule_paths(root).get(path) + sections = _submodule_paths(root) + other = sections.get(path, (None, None))[0] if other is not None and other != path: raise Refusal( "member-name-taken", @@ -1072,6 +1094,29 @@ def _checked_mount_path(root: Path, project: str, path: str) -> None: f"its section to point at {project} (git before 2.51), or mount " f"{project} under another name this tool does not follow (git " "2.51 and later, with the `--force` this tool mounts with).") + ghosts = [(name, url) for name, (at, url) in sections.items() + if at == path and name != path] + if ghosts: + named = " and ".join( + f"`{name}` ({'no url' if url is None else 'url ' + url})" + for name, url in ghosts) + removal = " and ".join( + f"`git config -f {GITMODULES} --remove-section " + f"{shlex.quote('submodule.' + name)}`" for name, _ in ghosts) + raise Refusal( + "member-path-registered", + f"{GITMODULES} already gives the path {path} to the submodule" + + ("s" if len(ghosts) > 1 else "") + f" named {named}, and that " + f"is the path {project} is mounted at", + "Remediation: git would write the member's own section beside " + "it, and a recursive clone of the holder follows whichever " + "section for that path comes last in the file. A section " + "somebody wrote is not this tool's to drop or rewrite, so take it " + f"out by hand — {removal}, then `git add {GITMODULES}` — or, if " + "it was a member, with `family.py remove`; then re-run. With no " + f"{GITMODULES} in the working tree, put back the copy git read " + f"first: `git checkout -- {GITMODULES}` (the index's), or `git " + f"checkout HEAD -- {GITMODULES}`.") def _checked_add_target(family: Family, repository: str, project: str, diff --git a/templates/family-root/scripts/validate-family.py b/templates/family-root/scripts/validate-family.py index 0991ba1b..2bfcacf1 100755 --- a/templates/family-root/scripts/validate-family.py +++ b/templates/family-root/scripts/validate-family.py @@ -37,6 +37,10 @@ and, across the rows, that no two record ONE `id`: that is one project pinned twice, however each row spelled its repository (#206). This one reads `family.yaml` alone, so `--no-members` asks it too. + - that no two sections of `.gitmodules` give ONE path (#300): a recursive + clone fetches that path from only one of them, so the other is a url + the next edit can make the one. It reads that file alone, so + `--no-members` asks it too. - the COPIED shape files, each against its `sha256` row in `contracts/shape-pin.yaml`. Editing a copy in place is drift and the exit is to carry it upstream, never to update the digest. @@ -52,6 +56,7 @@ from __future__ import annotations import argparse +import os import re import shlex import sys @@ -67,6 +72,7 @@ DATE_RE = re.compile(r"^\d{4}-\d{2}-\d{2}$") QUALIFIED_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9_.-]*/[A-Za-z0-9][A-Za-z0-9_.-]*$") MANIFEST = "family.yaml" +GITMODULES = ".gitmodules" DEFAULT_MEMBERS_DIR = "members" LOCKSTEP = ( @@ -570,13 +576,78 @@ def _check_member_ids_unique(root: Path, members: list, f"name {first} there instead to keep {project}.") +def _check_gitmodules_paths(root: Path, report: Report) -> None: + """No two sections of `.gitmodules` give ONE path (#300). + + `family.py add` committed a second section for a mount path that a + section under another name already gave — a half-removed member, a hand + edit, a merge that kept both sides — and this gate passed the result. A + recursive clone fetches the path from only one of them, the last in the + file, so the other is a url the next edit to the file can make the one + fetched. `add` refuses to write one now; this finds a holder that + already has one. + + READ AS GIT READS IT, with `git config -z --file .gitmodules --list` as + `family.py` reads it, and no parser of this file's own: the working-tree + file, which in CI is HEAD's. A holder with no members has no + `.gitmodules`, and that is no finding; one git cannot read is a refusal, + because nothing could be cloned from it either. A section whose path no + row names is another question, and not asked here. + """ + if not os.path.lexists(root / GITMODULES): + return + try: + listed = git_out(["config", "-z", "--file", GITMODULES, "--list"], + cwd=root, binary=True) + except Refusal as exc: + raise Refusal( + "gitmodules-unreadable", + f"{GITMODULES} could not be read as git reads it: {exc.detail}", + "Remediation: git cannot read it either, so none of this " + "holder's submodules can be cloned from it as it stands. Mend " + f"the file by hand (`git config -f {GITMODULES} --list` says " + "where it breaks), then run this again.") from exc + sections: dict[str, 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 in ("path", "url"): + sections.setdefault(section[len("submodule."):], + {})[variable] = value + paths: dict[str, list[str]] = {} + for name, keys in sections.items(): + if "path" in keys: + paths.setdefault(keys["path"], []).append(name) + for path, names in paths.items(): + if len(names) < 2: + continue + listing = ", ".join( + f"`{name}` (" + (f"url {sections[name]['url']}" + if "url" in sections[name] else "no url") + ")" + for name in names) + report.finding( + "gitmodules-path-twice", + f"{GITMODULES} gives the path {path} to {len(names)} " + f"submodules, {listing}. A recursive clone fetches that path " + "from only one of them, the last in the file, so the order of " + "the sections decides which repository lands there. Keep the one " + "whose url is the member's (`family.py add` names it after its " + f"path, `{path}`) and take each other out by hand, `git config " + f"-f {GITMODULES} --remove-section submodule.`, then commit " + f"{GITMODULES}.") + + def _check_members(root: Path, members: list, no_members: bool, report: Report) -> None: """Every row in `members:`, or the `--no-members` skip line. - Split from `main` for #137. + Split from `main` for #137. The two checks that read a file alone — + `family.yaml`'s ids (#206) and `.gitmodules`'s paths (#300) — come + first, so `--no-members` asks them too. """ _check_member_ids_unique(root, members, report) + _check_gitmodules_paths(root, report) if no_members: print(f" -- {len(members)} member(s) SKIPPED (--no-members): " "their gitlinks, digests and project ids are not checked " diff --git a/tests/test_family_add_refusals.py b/tests/test_family_add_refusals.py index 0d1b7627..df56a26c 100644 --- a/tests/test_family_add_refusals.py +++ b/tests/test_family_add_refusals.py @@ -1280,3 +1280,216 @@ def test_the_first_add_to_a_holder_with_no_commit_makes_its_root_commit( assert row["pin"]["commit"] == tip assert git("status", "--porcelain", cwd=root).stdout == "" offline.assert_left_nothing() + + +# --- a section under ANOTHER name at the mount path (#300) ------------------ +# +# The mirror of `member-name-taken` (#242), which refuses a section whose NAME +# is the mount path for another path: here a section under another name +# already gives the mount PATH. git writes the member's own section beside it +# without a word, so `add` exited 0 and committed two sections for one path, +# `validate-family.py` said `family ok`, and a recursive clone followed the +# one LAST in the file — the other's url, as soon as an edit put it there. +# Each holder below is built in git's index and objects, never through a +# checkout of the file. + +#: A section under another name whose path is the spare's mount path, with a +#: url nothing can fetch: what a half-removed member, a hand edit or a merge +#: that kept both sides leaves behind. +GHOST = (f'[submodule "ghost"]\n\tpath = members/{SPARE}\n' + "\turl = /nowhere/ghost.git\n").encode("utf-8") + +GHOST_REFUSED = ( + "REFUSED member-path-registered: .gitmodules already gives the path " + f"members/{SPARE} to the submodule named `ghost` (url " + f"/nowhere/ghost.git), and that is the path {SPARE} is mounted at") + + +def ghost_remediation(removal: str) -> str: + """What `member-path-registered` tells the human to do, around the + `git config` command(s), `removal`, that take each section out.""" + return ( + "Remediation: git would write the member's own section beside it, " + "and a recursive clone of the holder follows whichever section for " + "that path comes last in the file. A section somebody wrote is not " + f"this tool's to drop or rewrite, so take it out by hand — {removal}, " + "then `git add .gitmodules` — or, if it was a member, with " + "`family.py remove`; then re-run. With no .gitmodules in the working " + "tree, put back the copy git read first: `git checkout -- " + ".gitmodules` (the index's), or `git checkout HEAD -- .gitmodules`.") + + +def gitmodules_with(root: Path, extra: bytes, kept_in: str) -> bytes: + """HEAD's `.gitmodules` with `extra` appended, kept ONLY where `kept_in` + says, and built in git's index and objects rather than through a + checkout: "the work tree" commits it and writes the same bytes to disk; + "the index" stages it over HEAD's and leaves no file on disk; "HEAD" + commits it, then takes it off the disk AND out of the index, where `git + submodule add` still reads it. Returns the bytes.""" + data = subprocess.run(["git", "show", "HEAD:.gitmodules"], + cwd=str(root), capture_output=True, + check=True).stdout + extra + blob = subprocess.run(["git", "hash-object", "-w", "--stdin"], + cwd=str(root), input=data, capture_output=True, + check=True).stdout.decode("ascii").strip() + git("update-index", "--cacheinfo", f"100644,{blob},.gitmodules", + cwd=root) + if kept_in != "the index": + git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-qm", + "A section of somebody else's", cwd=root) + if kept_in == "the work tree": + (root / ".gitmodules").write_bytes(data) + else: + (root / ".gitmodules").unlink() + if kept_in == "HEAD": + git("rm", "-q", "--cached", "--", ".gitmodules", cwd=root) + return data + + +def gitmodules_paths(root: Path) -> list[str]: + """Every `path` HEAD's `.gitmodules` gives, in file order, as git lists + them.""" + listed = git("config", "--blob", "HEAD:.gitmodules", "--list", + cwd=root).stdout.splitlines() + return [line.partition("=")[2] for line in listed + if line.partition("=")[0].rpartition(".")[2] == "path"] + + +@pytest.mark.parametrize("kept_in", ["the work tree", "the index", "HEAD"]) +def test_a_section_under_another_name_at_the_mount_path_is_refused( + family, holder, offline, kept_in): + """THE ISSUE'S PROBE, from each copy of the file `git submodule add` + reads: the working tree's, else the index's, else HEAD's. A section named + `ghost` gives `members/IRQS` to a url nothing can fetch, and `add + --member InkRouter/IRQS` is refused before the member is read — not one + line on stdout, nothing cloned, nothing mounted — naming the section and + its url, and the holder is byte for byte as it was, the ghost included: + it is somebody's registration, not this tool's to drop.""" + gitmodules_with(holder, GHOST, kept_in) + before = snapshot(holder) + result = offline.add(holder, SPARE, remotes=family["remotes"]) + assert result.returncode == 2, result.stdout + result.stderr + assert result.stdout == "" + assert refusal(result.stderr) == (GHOST_REFUSED, ghost_remediation( + "`git config -f .gitmodules --remove-section submodule.ghost`")) + assert snapshot(holder) == before + assert not (holder / "members" / SPARE).exists() + assert not (holder / ".git" / "modules" / "members" / SPARE).exists() + offline.assert_left_nothing() + + +@pytest.mark.parametrize("other", [ + None, (f"members/{SPARE}X", "ghost"), + (f"members/{SPARE}", f"members/{SPARE}")], + ids=["no other section", "another path", "the member's own name"]) +def test_a_section_that_is_not_another_name_at_the_mount_path_is_added_past( + family, holder, offline, other): + """THE CONTROL. The plain add still exits 0, and so does one past a + section for ANOTHER path that only begins like the mount path (the path + is compared exactly), and one past a section that already carries the + member's own name for its own path — what `remove` leaves behind when a + ghost came after the member's section (#300) — which git rewrites in + place. Each commits ONE section for the mount path, with the member's + url, and the holder passes its own gate.""" + if other is not None: + path, name = other + gitmodules_with(holder, (f'[submodule "{name}"]\n\tpath = {path}\n' + "\turl = /nowhere/ghost.git\n").encode(), + "the work tree") + # The member's url, spelled the way `add` spelled IRRS's beside it. + url = git("config", "-f", ".gitmodules", "submodule.members/IRRS.url", + cwd=holder).stdout.strip()[:-len("IRRS.git")] + f"{SPARE}.git" + result = offline.add(holder, SPARE, remotes=family["remotes"]) + assert result.returncode == 0, result.stdout + result.stderr + assert result.stderr == "" + assert gitmodules_paths(holder).count(f"members/{SPARE}") == 1 + assert git("config", "--blob", "HEAD:.gitmodules", + f"submodule.members/{SPARE}.url", + cwd=holder).stdout.strip() == url + assert git("status", "--porcelain", cwd=holder).stdout == "" + assert validate(holder).returncode == 0 + offline.assert_left_nothing() + + +def test_every_section_at_the_mount_path_is_named_and_taken_out_as_printed( + family, holder, offline): + """TWO SECTIONS under other names at one mount path, one named with a + space and one with no url: the refusal names both, each with its url or + the want of one, and its `git config` commands, typed exactly as + printed, take both out — after which the same `add` runs, and the holder + gives the mount path to one section, the member's.""" + import shlex + gitmodules_with(holder, GHOST + (f'[submodule "old {SPARE}"]\n' + f"\tpath = members/{SPARE}\n").encode(), + "the work tree") + before = snapshot(holder) + refused = offline.add(holder, SPARE, remotes=family["remotes"]) + assert refused.returncode == 2, refused.stdout + refused.stderr + assert refused.stdout == "" + said, remediation = refusal(refused.stderr) + assert said == ( + "REFUSED member-path-registered: .gitmodules already gives the path " + f"members/{SPARE} to the submodules named `ghost` (url " + f"/nowhere/ghost.git) and `old {SPARE}` (no url), and that is the " + f"path {SPARE} is mounted at") + assert remediation == ghost_remediation( + "`git config -f .gitmodules --remove-section submodule.ghost` and " + "`git config -f .gitmodules --remove-section " + f"'submodule.old {SPARE}'`") + assert snapshot(holder) == before + commands = re.findall(r"`(git config [^`]*)`", remediation) + assert len(commands) == 2, remediation + for command in commands: + git(*shlex.split(command)[1:], cwd=holder) + git("add", "--", ".gitmodules", cwd=holder) + added = offline.add(holder, SPARE, remotes=family["remotes"]) + assert added.returncode == 0, added.stdout + added.stderr + assert gitmodules_paths(holder) == [ + "members/IRRS", "members/IRSS", f"members/{SPARE}"] + assert git("status", "--porcelain", cwd=holder).stdout == "" + assert validate(holder).returncode == 0 + offline.assert_left_nothing() + + +def test_the_validator_finds_two_sections_for_one_path(family, holder): + """`gitmodules-path-twice`, so a holder that already has a ghost fails + its own gate instead of passing it. One section per path is clean; one + more for `members/IRRS`, under another name and LAST in the file — the + one a recursive clone follows — is the only finding, under `--pins` and + `--no-members` as well, because it reads `.gitmodules` alone.""" + assert validate(holder).returncode == 0 + url = git("config", "-f", ".gitmodules", "submodule.members/IRRS.url", + cwd=holder).stdout.strip() + gitmodules_with(holder, b'[submodule "ghost"]\n\tpath = members/IRRS\n' + b"\turl = /nowhere/ghost.git\n", "the work tree") + for extra in ((), ("--pins",), ("--no-members",)): + result = validate(holder, *extra) + assert result.returncode == 1, result.stderr + result.stdout + assert [line for line in result.stderr.splitlines() + if line.startswith("FINDING")] == [ + "FINDING gitmodules-path-twice: .gitmodules gives the path " + f"members/IRRS to 2 submodules, `members/IRRS` (url {url}), " + "`ghost` (url /nowhere/ghost.git). A recursive clone fetches " + "that path from only one of them, the last in the file, so the " + "order of the sections decides which repository lands there. " + "Keep the one whose url is the member's (`family.py add` names " + "it after its path, `members/IRRS`) and take each other out by " + "hand, `git config -f .gitmodules --remove-section " + "submodule.`, then commit .gitmodules."], extra + + +def test_a_gitmodules_git_cannot_read_is_a_refusal_not_a_pass(holder): + """The validator reads `.gitmodules` as git reads it, so a file git + cannot read is a question it cannot answer: a refusal, exit 2, never an + implicit pass.""" + (holder / ".gitmodules").write_bytes(b'[submodule "broken"\n') + result = validate(holder) + assert result.returncode == 2, result.stderr + result.stdout + assert result.stderr.startswith( + "REFUSED gitmodules-unreadable: .gitmodules could not be read as git " + "reads it: `git config -z --file .gitmodules --list` in ") + assert result.stderr.splitlines()[-1] == ( + "Remediation: git cannot read it either, so none of this holder's " + "submodules can be cloned from it as it stands. Mend the file by " + "hand (`git config -f .gitmodules --list` says where it breaks), " + "then run this again.") From e0cc35128d94700b400882feb4505696d4f532b7 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 04:59:21 +0000 Subject: [PATCH 2/4] family.py: `member-path-registered` prints a removal by key, `git config --unset-all`, that works for every section, a name holding `]` or a hand-capitalised header included, says it is a POSIX spelling and names the restore only when the working tree has no `.gitmodules`, and `validate-family.py` refuses `gitmodules-unreadable` when it cannot run git instead of crashing with a finding's exit code (#300) The review of 03ddadd (MERGEABLE WITH NITS, no MUST) found one test gap and five nits. Each is fixed here, in a commit on top of 03ddadd. A ghost nobody had staged was untested. Every "work tree" row committed the ghost, so the index held it too, and a reader that took the index before the working tree passed every test while it let an unstaged, hand-edited ghost through: exit 0, two sections committed. A fourth row now writes the ghost to the working-tree file alone. The printed removal could not run for every name. `git config --remove-section` cannot address a section whose name holds `]`, nor one whose header a hand spelled `[Submodule "..."]`: git 2.43 says `no such section` for both. The refusal now takes each section out by its keys, `git config -f .gitmodules --unset-all 'submodule..path'` and the same for its `url` where it has one. A key reaches every section `git config --list` names, and git drops a header with its last key. The commands are said to be POSIX shell spellings (they are quoted with `shlex.quote`), and the sentence on putting back a missing `.gitmodules` is printed only when the working tree has none. The validator's finding names the same spelling. `validate-family.py --no-members` ran no git before #300, and with no git on PATH the new read crashed with a traceback and exit 1, a finding's code. An `OSError` from that read is now `gitmodules-unreadable`, exit 2, naming the reason. Tests, in the #300 block of tests/test_family_add_refusals.py: - the work-tree-only row; - a section named `Members/IRQS` for `members/IRQS` is refused as another name; - a section for `members/IRQS/` is added past, because git compares a section's path exactly; - a name with `]` and a capitalised header each get commands that, run as printed, clear the way for the same add; - two sections with the same url are still `gitmodules-path-twice`; - the holder's own validator, run in this process with its git call failing as a missing program does, refuses instead of crashing. `import shlex` moves to the module's imports. Mutants, over `-k "section or gitmodules or name_taken or no_git"` in this file and tests/test_family_add_excludes.py, on 03ddadd and on this commit: - the index read first, names compared case-insensitively, a trailing slash stripped, and only differing urls flagged each passed all 12 selected tests on 03ddadd, and each fails here (3, 1, 1 and 1 tests); - the `OSError` clause dropped fails the new validator test; - the writer's three mutants still fail. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Opus 5.5 --- scripts/family.py | 23 +- .../family-root/scripts/validate-family.py | 15 +- tests/test_family_add_refusals.py | 232 ++++++++++++++---- 3 files changed, 208 insertions(+), 62 deletions(-) diff --git a/scripts/family.py b/scripts/family.py index 72e32eca..655547ff 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -1100,9 +1100,16 @@ def _checked_mount_path(root: Path, project: str, path: str) -> None: named = " and ".join( f"`{name}` ({'no url' if url is None else 'url ' + url})" for name, url in ghosts) + # BY KEY, NOT BY SECTION: `--remove-section` cannot address a + # section whose name holds `]`, nor one whose header a hand spelled + # `[Submodule "..."]` (git 2.43: `no such section`), while a key + # reaches every section `git config --list` names. Its path is what + # makes it a submodule; git drops the header with the last key. removal = " and ".join( - f"`git config -f {GITMODULES} --remove-section " - f"{shlex.quote('submodule.' + name)}`" for name, _ in ghosts) + f"`git config -f {GITMODULES} --unset-all " + f"{shlex.quote('submodule.' + name + '.' + key)}`" + for name, url in ghosts + for key in (("path",) if url is None else ("path", "url"))) raise Refusal( "member-path-registered", f"{GITMODULES} already gives the path {path} to the submodule" @@ -1112,11 +1119,13 @@ def _checked_mount_path(root: Path, project: str, path: str) -> None: "it, and a recursive clone of the holder follows whichever " "section for that path comes last in the file. A section " "somebody wrote is not this tool's to drop or rewrite, so take it " - f"out by hand — {removal}, then `git add {GITMODULES}` — or, if " - "it was a member, with `family.py remove`; then re-run. With no " - f"{GITMODULES} in the working tree, put back the copy git read " - f"first: `git checkout -- {GITMODULES}` (the index's), or `git " - f"checkout HEAD -- {GITMODULES}`.") + f"out by hand, in a POSIX shell — {removal}, then `git add " + f"{GITMODULES}` — or, if it was a member, with `family.py " + "remove`; then re-run." + + ("" if os.path.lexists(root / GITMODULES) else + f" With no {GITMODULES} in the working tree, put back the " + f"copy git read first: `git checkout -- {GITMODULES}` (the " + f"index's), or `git checkout HEAD -- {GITMODULES}`.")) def _checked_add_target(family: Family, repository: str, project: str, diff --git a/templates/family-root/scripts/validate-family.py b/templates/family-root/scripts/validate-family.py index 2bfcacf1..68898cfc 100755 --- a/templates/family-root/scripts/validate-family.py +++ b/templates/family-root/scripts/validate-family.py @@ -607,6 +607,17 @@ def _check_gitmodules_paths(root: Path, report: Report) -> None: "holder's submodules can be cloned from it as it stands. Mend " f"the file by hand (`git config -f {GITMODULES} --list` says " "where it breaks), then run this again.") from exc + except OSError as exc: + # NO GIT TO RUN is a question this run cannot answer, so a refusal + # (exit 2), never the traceback that exits 1, a finding's code: + # `--no-members` ran no git at all before this check. + raise Refusal( + "gitmodules-unreadable", + f"{GITMODULES} could not be read as git reads it: git could not " + f"be run ({exc.strerror or exc})", + "Remediation: put git on PATH and run this again; this check " + "reads the file the way git does, and nothing could fetch this " + "holder's submodules without git either.") from exc sections: dict[str, dict[str, str]] = {} for record in listed.split(b"\0"): key, _, value = record.decode("utf-8", @@ -634,8 +645,8 @@ def _check_gitmodules_paths(root: Path, report: Report) -> None: "the sections decides which repository lands there. Keep the one " "whose url is the member's (`family.py add` names it after its " f"path, `{path}`) and take each other out by hand, `git config " - f"-f {GITMODULES} --remove-section submodule.`, then commit " - f"{GITMODULES}.") + f"-f {GITMODULES} --unset-all submodule..path` and the " + f"same for its `url`, then commit {GITMODULES}.") def _check_members(root: Path, members: list, no_members: bool, diff --git a/tests/test_family_add_refusals.py b/tests/test_family_add_refusals.py index df56a26c..f0a984d8 100644 --- a/tests/test_family_add_refusals.py +++ b/tests/test_family_add_refusals.py @@ -33,6 +33,7 @@ import importlib.util import os import re +import shlex import shutil import stat import subprocess @@ -1304,31 +1305,46 @@ def test_the_first_add_to_a_holder_with_no_commit_makes_its_root_commit( f"members/{SPARE} to the submodule named `ghost` (url " f"/nowhere/ghost.git), and that is the path {SPARE} is mounted at") +#: How the refusal spells taking GHOST out: by key, which reaches every +#: section, its path and then its url. +GHOST_REMOVAL = ( + "`git config -f .gitmodules --unset-all submodule.ghost.path` and " + "`git config -f .gitmodules --unset-all submodule.ghost.url`") -def ghost_remediation(removal: str) -> str: + +def ghost_remediation(removal: str, off_disk: bool = False) -> str: """What `member-path-registered` tells the human to do, around the - `git config` command(s), `removal`, that take each section out.""" + `git config` command(s), `removal`, that take each section out — and, + only where the working tree has no `.gitmodules` (`off_disk`), how to + put back the copy git read.""" return ( "Remediation: git would write the member's own section beside it, " "and a recursive clone of the holder follows whichever section for " "that path comes last in the file. A section somebody wrote is not " - f"this tool's to drop or rewrite, so take it out by hand — {removal}, " - "then `git add .gitmodules` — or, if it was a member, with " - "`family.py remove`; then re-run. With no .gitmodules in the working " - "tree, put back the copy git read first: `git checkout -- " - ".gitmodules` (the index's), or `git checkout HEAD -- .gitmodules`.") + "this tool's to drop or rewrite, so take it out by hand, in a POSIX " + f"shell — {removal}, then `git add .gitmodules` — or, if it was a " + "member, with `family.py remove`; then re-run." + + (" With no .gitmodules in the working tree, put back the copy git " + "read first: `git checkout -- .gitmodules` (the index's), or `git " + "checkout HEAD -- .gitmodules`." if off_disk else "")) def gitmodules_with(root: Path, extra: bytes, kept_in: str) -> bytes: """HEAD's `.gitmodules` with `extra` appended, kept ONLY where `kept_in` says, and built in git's index and objects rather than through a - checkout: "the work tree" commits it and writes the same bytes to disk; - "the index" stages it over HEAD's and leaves no file on disk; "HEAD" - commits it, then takes it off the disk AND out of the index, where `git - submodule add` still reads it. Returns the bytes.""" + checkout: "the work tree only" writes it to disk and neither stages nor + commits it — a hand edit nobody has staged, the commonest ghost, which + only a reader that asks the working tree FIRST sees; "the work tree" + commits it and writes the same bytes to disk; "the index" stages it over + HEAD's and leaves no file on disk; "HEAD" commits it, then takes it off + the disk AND out of the index, where `git submodule add` still reads it. + Returns the bytes.""" data = subprocess.run(["git", "show", "HEAD:.gitmodules"], cwd=str(root), capture_output=True, check=True).stdout + extra + if kept_in == "the work tree only": + (root / ".gitmodules").write_bytes(data) + return data blob = subprocess.run(["git", "hash-object", "-w", "--stdin"], cwd=str(root), input=data, capture_output=True, check=True).stdout.decode("ascii").strip() @@ -1355,38 +1371,85 @@ def gitmodules_paths(root: Path) -> list[str]: if line.partition("=")[0].rpartition(".")[2] == "path"] -@pytest.mark.parametrize("kept_in", ["the work tree", "the index", "HEAD"]) +def take_out_as_printed(root: Path, remediation: str) -> list[str]: + """Run each `git config` command `remediation` prints, exactly as + printed (split as a POSIX shell splits it, run with no shell), then + stage `.gitmodules`, as the remediation says. Returns the commands.""" + commands = re.findall(r"`(git config [^`]*)`", remediation) + for command in commands: + git(*shlex.split(command)[1:], cwd=root) + git("add", "--", ".gitmodules", cwd=root) + return commands + + +def refused_unchanged(result: subprocess.CompletedProcess, root: Path, + before: dict) -> tuple[str, str]: + """Refused before the member was read — nothing on stdout, nothing + cloned or mounted — with the holder byte for byte as `before` recorded + it. Returns `(the REFUSED line, the remediation)`.""" + assert result.returncode == 2, result.stdout + result.stderr + assert result.stdout == "" + assert snapshot(root) == before + assert not (root / "members" / SPARE).exists() + assert not (root / ".git" / "modules" / "members" / SPARE).exists() + return refusal(result.stderr) + + +@pytest.mark.parametrize("kept_in", ["the work tree only", "the work tree", + "the index", "HEAD"]) def test_a_section_under_another_name_at_the_mount_path_is_refused( family, holder, offline, kept_in): """THE ISSUE'S PROBE, from each copy of the file `git submodule add` - reads: the working tree's, else the index's, else HEAD's. A section named - `ghost` gives `members/IRQS` to a url nothing can fetch, and `add - --member InkRouter/IRQS` is refused before the member is read — not one - line on stdout, nothing cloned, nothing mounted — naming the section and - its url, and the holder is byte for byte as it was, the ghost included: - it is somebody's registration, not this tool's to drop.""" + reads: the working tree's — an unstaged hand edit as well as a committed + ghost — else the index's, else HEAD's. A section named `ghost` gives + `members/IRQS` to a url nothing can fetch, and `add --member + InkRouter/IRQS` is refused before the member is read — not one line on + stdout, nothing cloned, nothing mounted — naming the section and its + url, and the holder is byte for byte as it was, the ghost included: it + is somebody's registration, not this tool's to drop. Only where the + working tree has no `.gitmodules` does the remediation say how to put + one back.""" gitmodules_with(holder, GHOST, kept_in) before = snapshot(holder) result = offline.add(holder, SPARE, remotes=family["remotes"]) - assert result.returncode == 2, result.stdout + result.stderr - assert result.stdout == "" - assert refusal(result.stderr) == (GHOST_REFUSED, ghost_remediation( - "`git config -f .gitmodules --remove-section submodule.ghost`")) - assert snapshot(holder) == before - assert not (holder / "members" / SPARE).exists() - assert not (holder / ".git" / "modules" / "members" / SPARE).exists() + assert refused_unchanged(result, holder, before) == ( + GHOST_REFUSED, ghost_remediation( + GHOST_REMOVAL, off_disk=kept_in in ("the index", "HEAD"))) + offline.assert_left_nothing() + + +def test_a_section_named_the_mount_path_in_another_case_is_another_name( + family, holder, offline): + """git's submodule names are case-sensitive, so `Members/IRQS` is + ANOTHER name than the `members/IRQS` git gives the mount, and its + section for the mount path is a ghost like any other: refused, named in + its own spelling.""" + gitmodules_with(holder, (f'[submodule "Members/{SPARE}"]\n' + f"\tpath = members/{SPARE}\n" + "\turl = /nowhere/ghost.git\n").encode(), + "the work tree") + before = snapshot(holder) + result = offline.add(holder, SPARE, remotes=family["remotes"]) + said, _ = refused_unchanged(result, holder, before) + assert said == ( + "REFUSED member-path-registered: .gitmodules already gives the path " + f"members/{SPARE} to the submodule named `Members/{SPARE}` (url " + f"/nowhere/ghost.git), and that is the path {SPARE} is mounted at") offline.assert_left_nothing() @pytest.mark.parametrize("other", [ - None, (f"members/{SPARE}X", "ghost"), + None, (f"members/{SPARE}X", "ghost"), (f"members/{SPARE}/", "ghost"), (f"members/{SPARE}", f"members/{SPARE}")], - ids=["no other section", "another path", "the member's own name"]) + ids=["no other section", "another path", "a trailing slash", + "the member's own name"]) def test_a_section_that_is_not_another_name_at_the_mount_path_is_added_past( family, holder, offline, other): """THE CONTROL. The plain add still exits 0, and so does one past a - section for ANOTHER path that only begins like the mount path (the path - is compared exactly), and one past a section that already carries the + section for ANOTHER path: one that only begins like the mount path, or + one that ends in a slash — git compares a section's path exactly, so a + recursive clone never fetches the mount path from it, and neither does + this refusal. So does one past a section that already carries the member's own name for its own path — what `remove` leaves behind when a ghost came after the member's section (#300) — which git rewrites in place. Each commits ONE section for the mount path, with the member's @@ -1418,30 +1481,21 @@ def test_every_section_at_the_mount_path_is_named_and_taken_out_as_printed( the want of one, and its `git config` commands, typed exactly as printed, take both out — after which the same `add` runs, and the holder gives the mount path to one section, the member's.""" - import shlex gitmodules_with(holder, GHOST + (f'[submodule "old {SPARE}"]\n' f"\tpath = members/{SPARE}\n").encode(), "the work tree") before = snapshot(holder) refused = offline.add(holder, SPARE, remotes=family["remotes"]) - assert refused.returncode == 2, refused.stdout + refused.stderr - assert refused.stdout == "" - said, remediation = refusal(refused.stderr) + said, remediation = refused_unchanged(refused, holder, before) assert said == ( "REFUSED member-path-registered: .gitmodules already gives the path " f"members/{SPARE} to the submodules named `ghost` (url " f"/nowhere/ghost.git) and `old {SPARE}` (no url), and that is the " f"path {SPARE} is mounted at") assert remediation == ghost_remediation( - "`git config -f .gitmodules --remove-section submodule.ghost` and " - "`git config -f .gitmodules --remove-section " - f"'submodule.old {SPARE}'`") - assert snapshot(holder) == before - commands = re.findall(r"`(git config [^`]*)`", remediation) - assert len(commands) == 2, remediation - for command in commands: - git(*shlex.split(command)[1:], cwd=holder) - git("add", "--", ".gitmodules", cwd=holder) + f"{GHOST_REMOVAL} and `git config -f .gitmodules --unset-all " + f"'submodule.old {SPARE}.path'`") + assert len(take_out_as_printed(holder, remediation)) == 3, remediation added = offline.add(holder, SPARE, remotes=family["remotes"]) assert added.returncode == 0, added.stdout + added.stderr assert gitmodules_paths(holder) == [ @@ -1451,17 +1505,60 @@ def test_every_section_at_the_mount_path_is_named_and_taken_out_as_printed( offline.assert_left_nothing() -def test_the_validator_finds_two_sections_for_one_path(family, holder): +@pytest.mark.parametrize("header, name", [ + (f'[submodule "old]{SPARE}"]', f"old]{SPARE}"), + ('[Submodule "ghost"]', "ghost")], + ids=["a name with ]", "a header a hand capitalised"]) +def test_a_section_git_cannot_name_by_section_is_taken_out_as_printed( + family, holder, offline, header, name): + """THE PRINTED COMMANDS WORK FOR EVERY SECTION. `git config + --remove-section` cannot address a section whose name holds `]`, nor one + whose header a hand spelled `[Submodule "..."]` (git 2.43 says `no such + section` for both), so the refusal takes a section out by its keys, + which reach every section `git config --list` names. Run exactly as + printed, they clear the way for the same `add`.""" + gitmodules_with(holder, (f"{header}\n\tpath = members/{SPARE}\n" + "\turl = /nowhere/ghost.git\n").encode(), + "the work tree only") + before = snapshot(holder) + refused = offline.add(holder, SPARE, remotes=family["remotes"]) + said, remediation = refused_unchanged(refused, holder, before) + assert said == ( + "REFUSED member-path-registered: .gitmodules already gives the path " + f"members/{SPARE} to the submodule named `{name}` (url " + f"/nowhere/ghost.git), and that is the path {SPARE} is mounted at") + quoted = [shlex.quote(f"submodule.{name}.{key}") + for key in ("path", "url")] + assert remediation == ghost_remediation( + f"`git config -f .gitmodules --unset-all {quoted[0]}` and " + f"`git config -f .gitmodules --unset-all {quoted[1]}`") + assert len(take_out_as_printed(holder, remediation)) == 2, remediation + added = offline.add(holder, SPARE, remotes=family["remotes"]) + assert added.returncode == 0, added.stdout + added.stderr + assert gitmodules_paths(holder) == [ + "members/IRRS", "members/IRSS", f"members/{SPARE}"] + assert validate(holder).returncode == 0 + offline.assert_left_nothing() + + +@pytest.mark.parametrize("same_url", [False, True], + ids=["another url", "the same url"]) +def test_the_validator_finds_two_sections_for_one_path(family, holder, + same_url): """`gitmodules-path-twice`, so a holder that already has a ghost fails its own gate instead of passing it. One section per path is clean; one more for `members/IRRS`, under another name and LAST in the file — the one a recursive clone follows — is the only finding, under `--pins` and - `--no-members` as well, because it reads `.gitmodules` alone.""" + `--no-members` as well, because it reads `.gitmodules` alone. Two + sections for one path are a finding whatever their urls: the same url + twice is still one path given twice, and the next edit can change + either.""" assert validate(holder).returncode == 0 url = git("config", "-f", ".gitmodules", "submodule.members/IRRS.url", cwd=holder).stdout.strip() - gitmodules_with(holder, b'[submodule "ghost"]\n\tpath = members/IRRS\n' - b"\turl = /nowhere/ghost.git\n", "the work tree") + other = url if same_url else "/nowhere/ghost.git" + gitmodules_with(holder, (f'[submodule "ghost"]\n\tpath = members/IRRS\n' + f"\turl = {other}\n").encode(), "the work tree") for extra in ((), ("--pins",), ("--no-members",)): result = validate(holder, *extra) assert result.returncode == 1, result.stderr + result.stdout @@ -1469,13 +1566,13 @@ def test_the_validator_finds_two_sections_for_one_path(family, holder): if line.startswith("FINDING")] == [ "FINDING gitmodules-path-twice: .gitmodules gives the path " f"members/IRRS to 2 submodules, `members/IRRS` (url {url}), " - "`ghost` (url /nowhere/ghost.git). A recursive clone fetches " - "that path from only one of them, the last in the file, so the " - "order of the sections decides which repository lands there. " - "Keep the one whose url is the member's (`family.py add` names " - "it after its path, `members/IRRS`) and take each other out by " - "hand, `git config -f .gitmodules --remove-section " - "submodule.`, then commit .gitmodules."], extra + f"`ghost` (url {other}). A recursive clone fetches that path " + "from only one of them, the last in the file, so the order of " + "the sections decides which repository lands there. Keep the one " + "whose url is the member's (`family.py add` names it after its " + "path, `members/IRRS`) and take each other out by hand, `git " + "config -f .gitmodules --unset-all submodule..path` and " + "the same for its `url`, then commit .gitmodules."], extra def test_a_gitmodules_git_cannot_read_is_a_refusal_not_a_pass(holder): @@ -1493,3 +1590,32 @@ def test_a_gitmodules_git_cannot_read_is_a_refusal_not_a_pass(holder): "submodules can be cloned from it as it stands. Mend the file by " "hand (`git config -f .gitmodules --list` says where it breaks), " "then run this again.") + + +def test_a_validator_with_no_git_to_run_refuses_instead_of_crashing( + holder, monkeypatch, capsys): + """`--no-members` ran no git before `.gitmodules` was read, so a + machine without git now meets one here. That is a question this run + cannot answer — a refusal, exit 2 — never the traceback that exits 1, a + finding's code. The holder's own validator is run IN THIS PROCESS with + its one git call failing as a missing program does, so no PATH is + changed for anything else.""" + monkeypatch.setattr(sys, "path", list(sys.path)) + spec = importlib.util.spec_from_file_location( + "holder_validate_family", holder / "scripts" / "validate-family.py") + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + + def no_git(args, cwd, binary=False): + raise FileNotFoundError(2, "No such file or directory", "git") + + monkeypatch.setattr(module, "git_out", no_git) + code = module.main(["--root", str(holder), "--no-members"]) + out, err = capsys.readouterr() + assert code == 2, out + err + assert err.splitlines() == [ + "REFUSED gitmodules-unreadable: .gitmodules could not be read as git " + "reads it: git could not be run (No such file or directory)", + "Remediation: put git on PATH and run this again; this check reads " + "the file the way git does, and nothing could fetch this holder's " + "submodules without git either."] From 9bb6f837ab018858bfac76cc36faa1ae27716cf0 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 05:18:21 +0000 Subject: [PATCH 3/4] family.py: `member-path-registered` prints one `git config --unset-all` per key the ghost section holds, not its path and url alone, so the whole section goes, header and all, and a member later given that name no longer inherits a stale `branch`, `shallow` or `update` (#300) The re-check of e0cc351 (MERGEABLE WITH NITS, no MUST, no SHOULD) found one real nit. Unsetting a ghost's `path` and `url` took the section out only when those were its only keys. Any other key (`branch`, `shallow`, `update`) stayed behind with its header. Such a section is inert while it has no path, but a later `add` of a member whose mount git gives the same name writes its path and url into that section and inherits the rest. In the reviewer's probe, `add --member testorg/Zed` committed `branch = dev` and `shallow = true` into Zed's section, the validator said ok, and `git submodule update --remote members/Zed` then failed. `_submodule_paths`, still the one reader of the file, now keeps every key of each section beside its path and url, once each, in the order git lists them. The refusal prints one `--unset-all 'submodule..'` per key, so git drops the header with the last one. That still reaches a name holding `]` and a hand-capitalised header, and `--unset-all` takes a key given twice. A comment line inside the section keeps its header but no key. The validator, which reads only paths and urls, now says to unset every key the section holds. tests/test_family_add_refusals.py: a ghost named `members/IRQZ` carries `branch`, `shallow` and `update` beside its path and url. Its five printed commands, run as printed, leave nothing under `submodule.members/IRQZ.`, and an add of a member called IRQZ then commits a section holding only `path` and `url`. With only path and url unset (what e0cc351 printed) that test fails: on its text and, with the text assertion dropped, on the leftover keys and then on the keys IRQZ's section inherits. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Opus 5.5 --- scripts/family.py | 38 +++++++------ .../family-root/scripts/validate-family.py | 5 +- tests/test_family_add_refusals.py | 56 ++++++++++++++++++- 3 files changed, 78 insertions(+), 21 deletions(-) diff --git a/scripts/family.py b/scripts/family.py index 655547ff..ad6dc148 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -967,17 +967,21 @@ def _index_entries(root: Path, path: str) -> list[tuple[str, str]]: return entries -def _submodule_paths(root: Path) -> dict[str, tuple[str, str | None]]: - """`{name: (path, url)}` of every submodule the holder's `.gitmodules` - names, read from where `git submodule add` reads it: the file in the - working tree, or the index's copy where there is none, or HEAD's where - the index has none either. `url` is None for a section that gives no - url; a section that gives no path is no submodule and is not listed. +def _submodule_paths( + root: Path) -> dict[str, tuple[str, str | None, tuple[str, ...]]]: + """`{name: (path, url, keys)}` of every submodule the holder's + `.gitmodules` names, read from where `git submodule add` reads it: the + file in the working tree, or the index's copy where there is none, or + HEAD's where the index has none either. `url` is None for a section that + gives no url; a section that gives no path is no submodule and is not + listed. `keys` is every key the section holds, path and url included, + once each, in the order git lists them. THE ONE READER OF THE FILE, for both questions `_checked_mount_path` asks of it: the path a NAME is given (`member-name-taken`, #242), and - the names a PATH is given, each with its url (`member-path-registered`, - #300). A second reader could read a second copy of the file. + the names a PATH is given, each with its url and its keys + (`member-path-registered`, #300). A second reader could read a second + copy of the file. """ if os.path.lexists(root / GITMODULES): source = ["--file", GITMODULES] @@ -998,9 +1002,9 @@ def _submodule_paths(root: Path) -> dict[str, tuple[str, str | None]]: key, _, value = record.decode("utf-8", "surrogateescape").partition("\n") section, _, variable = key.rpartition(".") - if section.startswith("submodule.") and variable in ("path", "url"): + if section.startswith("submodule."): found.setdefault(section[len("submodule."):], {})[variable] = value - return {name: (keys["path"], keys.get("url")) + return {name: (keys["path"], keys.get("url"), tuple(keys)) for name, keys in found.items() if "path" in keys} @@ -1094,22 +1098,24 @@ def _checked_mount_path(root: Path, project: str, path: str) -> None: f"its section to point at {project} (git before 2.51), or mount " f"{project} under another name this tool does not follow (git " "2.51 and later, with the `--force` this tool mounts with).") - ghosts = [(name, url) for name, (at, url) in sections.items() + ghosts = [(name, url, keys) for name, (at, url, keys) in sections.items() if at == path and name != path] if ghosts: named = " and ".join( f"`{name}` ({'no url' if url is None else 'url ' + url})" - for name, url in ghosts) + for name, url, _ in ghosts) # BY KEY, NOT BY SECTION: `--remove-section` cannot address a # section whose name holds `]`, nor one whose header a hand spelled # `[Submodule "..."]` (git 2.43: `no such section`), while a key - # reaches every section `git config --list` names. Its path is what - # makes it a submodule; git drops the header with the last key. + # reaches every section `git config --list` names. EVERY KEY it + # holds, not its path and url alone: a `branch`, `shallow` or + # `update` left behind is inherited by the submodule a later `add` + # gives that name, which git writes into the same section. git + # drops the header with the last key. removal = " and ".join( f"`git config -f {GITMODULES} --unset-all " f"{shlex.quote('submodule.' + name + '.' + key)}`" - for name, url in ghosts - for key in (("path",) if url is None else ("path", "url"))) + for name, _, keys in ghosts for key in keys) raise Refusal( "member-path-registered", f"{GITMODULES} already gives the path {path} to the submodule" diff --git a/templates/family-root/scripts/validate-family.py b/templates/family-root/scripts/validate-family.py index 68898cfc..3046054a 100755 --- a/templates/family-root/scripts/validate-family.py +++ b/templates/family-root/scripts/validate-family.py @@ -645,8 +645,9 @@ def _check_gitmodules_paths(root: Path, report: Report) -> None: "the sections decides which repository lands there. Keep the one " "whose url is the member's (`family.py add` names it after its " f"path, `{path}`) and take each other out by hand, `git config " - f"-f {GITMODULES} --unset-all submodule..path` and the " - f"same for its `url`, then commit {GITMODULES}.") + f"-f {GITMODULES} --unset-all submodule..` for every " + "key that section holds, so a submodule later given its name " + f"inherits none of them, then commit {GITMODULES}.") def _check_members(root: Path, members: list, no_members: bool, diff --git a/tests/test_family_add_refusals.py b/tests/test_family_add_refusals.py index f0a984d8..9cf0d72d 100644 --- a/tests/test_family_add_refusals.py +++ b/tests/test_family_add_refusals.py @@ -1306,7 +1306,8 @@ def test_the_first_add_to_a_holder_with_no_commit_makes_its_root_commit( f"/nowhere/ghost.git), and that is the path {SPARE} is mounted at") #: How the refusal spells taking GHOST out: by key, which reaches every -#: section, its path and then its url. +#: section, one command for each key it holds, in the order git lists +#: them. GHOST_REMOVAL = ( "`git config -f .gitmodules --unset-all submodule.ghost.path` and " "`git config -f .gitmodules --unset-all submodule.ghost.url`") @@ -1571,8 +1572,9 @@ def test_the_validator_finds_two_sections_for_one_path(family, holder, "the sections decides which repository lands there. Keep the one " "whose url is the member's (`family.py add` names it after its " "path, `members/IRRS`) and take each other out by hand, `git " - "config -f .gitmodules --unset-all submodule..path` and " - "the same for its `url`, then commit .gitmodules."], extra + "config -f .gitmodules --unset-all submodule..` for " + "every key that section holds, so a submodule later given its " + "name inherits none of them, then commit .gitmodules."], extra def test_a_gitmodules_git_cannot_read_is_a_refusal_not_a_pass(holder): @@ -1619,3 +1621,51 @@ def no_git(args, cwd, binary=False): "Remediation: put git on PATH and run this again; this check reads " "the file the way git does, and nothing could fetch this holder's " "submodules without git either."] + + +def test_every_key_of_a_ghost_goes_so_a_later_member_of_its_name_inherits_none( + family, holder, offline, tmp_path): + """EVERY KEY, NOT ITS PATH AND URL ALONE. A ghost named `members/IRQZ` + for the spare's mount path also carries `branch`, `shallow` and + `update`. The refusal prints one `git config --unset-all` per key it + holds, and run as printed they take the whole section out, header and + all. Had any key stayed, the add of a member that IS called IRQZ would + have had git write its path and url into that same section and inherit + the rest — a `branch = dev` that `git submodule update --remote` then + fails on, under a gate that says ok. Here IRQZ's section holds its path + and its url and nothing else.""" + remotes = tmp_path / "remotes" + remotes.mkdir() + for name in (SPARE, "IRQZ"): + shutil.copytree(family["remotes"] / f"{SPARE}.git", + remotes / f"{name}.git", symlinks=True) + gitmodules_with(holder, ( + '[submodule "members/IRQZ"]\n' + f"\tpath = members/{SPARE}\n\turl = /nowhere/ghost.git\n" + "\tbranch = dev\n\tshallow = true\n\tupdate = none\n").encode(), + "the work tree") + before = snapshot(holder) + refused = offline.add(holder, SPARE, remotes=remotes) + said, remediation = refused_unchanged(refused, holder, before) + assert said == ( + "REFUSED member-path-registered: .gitmodules already gives the path " + f"members/{SPARE} to the submodule named `members/IRQZ` (url " + f"/nowhere/ghost.git), and that is the path {SPARE} is mounted at") + assert remediation == ghost_remediation(" and ".join( + "`git config -f .gitmodules --unset-all " + f"submodule.members/IRQZ.{key}`" + for key in ("path", "url", "branch", "shallow", "update"))) + assert len(take_out_as_printed(holder, remediation)) == 5, remediation + assert git("config", "-f", ".gitmodules", "--get-regexp", + r"^submodule\.members/IRQZ\.", cwd=holder, + check=False).stdout == "" + assert "members/IRQZ" not in (holder / ".gitmodules").read_text( + encoding="utf-8") + added = offline.add(holder, "IRQZ", remotes=remotes) + assert added.returncode == 0, added.stdout + added.stderr + listed = git("config", "--blob", "HEAD:.gitmodules", "--get-regexp", + r"^submodule\.members/IRQZ\.", cwd=holder).stdout + assert [line.split(" ", 1)[0] for line in listed.splitlines()] == [ + "submodule.members/IRQZ.path", "submodule.members/IRQZ.url"] + assert git("status", "--porcelain", cwd=holder).stdout == "" + offline.assert_left_nothing() From c23e2b0b8ba8e02a68b5acd4aa1d8978655c39f1 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 05:54:02 +0000 Subject: [PATCH 4/4] tests/test_family_add_refusals.py: the same-url validator row has git write its ghost section, so the member's url, a path full of backslashes on Windows, is escaped as git escapes it and the row no longer writes a `.gitmodules` git cannot read, which failed tests-windows at 9bb6f83 (#300) `test_the_validator_finds_two_sections_for_one_path[the same url]` read the member's url back with `git config` and wrote it raw into a new `[submodule "ghost"]` section. On Windows that url is a path like `D:\a\_temp\t\...\IRRS.git`, and in a git config file a backslash starts an escape. `\a` is not one git knows, so git refused the whole file (`fatal: bad config line 9 in file .gitmodules`), and the validator rightly said `gitmodules-unreadable`, exit 2, where the row expected its finding. ubuntu and macOS passed, because their paths hold no backslash. The product is right; the test wrote an invalid file. The row now builds the section with a new helper, `git_section`, which has `git config -f ` write it and returns those bytes. `gitmodules_with` appends them to HEAD's `.gitmodules`, so the section is still the last in the file, as the row's docstring says. Every other write in the #300 block is a literal (`/nowhere/ghost.git`, `members/...` paths, names without a backslash); the control row reads the member's url only to compare it. Reproduced on Linux with the url `C:\a\b.git`. Written raw, as 9bb6f83 wrote it, the validator exits 2: `gitmodules-unreadable`, `bad config line 9 in file .gitmodules`. Written by git (`C:\\a\\b.git` in the file), it exits 1 with `gitmodules-path-twice` naming `ghost` (url C:\a\b.git). Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Opus 5.5 --- tests/test_family_add_refusals.py | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/tests/test_family_add_refusals.py b/tests/test_family_add_refusals.py index 9cf0d72d..71376f4b 100644 --- a/tests/test_family_add_refusals.py +++ b/tests/test_family_add_refusals.py @@ -1363,6 +1363,21 @@ def gitmodules_with(root: Path, extra: bytes, kept_in: str) -> bytes: return data +def git_section(scratch: Path, name: str, **values: str) -> bytes: + """`[submodule ""]` holding `values`, in the bytes GIT writes for + them, for `gitmodules_with` to append. A value read from git must be + escaped as git escapes it: a Windows path's backslashes written raw + (`D:\\a\\...`) are escapes git does not know, and the file is then one it + cannot read at all (`bad config line`). `scratch` is a file of the + test's own, written afresh.""" + if scratch.exists(): + scratch.unlink() + for key, value in values.items(): + git("config", "-f", str(scratch), f"submodule.{name}.{key}", value, + cwd=scratch.parent) + return scratch.read_bytes() + + def gitmodules_paths(root: Path) -> list[str]: """Every `path` HEAD's `.gitmodules` gives, in file order, as git lists them.""" @@ -1545,7 +1560,7 @@ def test_a_section_git_cannot_name_by_section_is_taken_out_as_printed( @pytest.mark.parametrize("same_url", [False, True], ids=["another url", "the same url"]) def test_the_validator_finds_two_sections_for_one_path(family, holder, - same_url): + tmp_path, same_url): """`gitmodules-path-twice`, so a holder that already has a ghost fails its own gate instead of passing it. One section per path is clean; one more for `members/IRRS`, under another name and LAST in the file — the @@ -1558,8 +1573,11 @@ def test_the_validator_finds_two_sections_for_one_path(family, holder, url = git("config", "-f", ".gitmodules", "submodule.members/IRRS.url", cwd=holder).stdout.strip() other = url if same_url else "/nowhere/ghost.git" - gitmodules_with(holder, (f'[submodule "ghost"]\n\tpath = members/IRRS\n' - f"\turl = {other}\n").encode(), "the work tree") + # Written by git, so the member's url — a path with backslashes on + # Windows — is escaped as git escapes it. + gitmodules_with(holder, git_section(tmp_path / "ghost-section", "ghost", + path="members/IRRS", url=other), + "the work tree") for extra in ((), ("--pins",), ("--no-members",)): result = validate(holder, *extra) assert result.returncode == 1, result.stderr + result.stdout