From 37faf29838c3ca873267703cfbfa1c7ed472a876 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sun, 18 Oct 2026 15:53:43 +0000 Subject: [PATCH 1/2] family.py add: a commit that records the member's gitlink anywhere but `members/` is taken back and refused, `member-gitlink-misplaced`, naming the path git chose, and a `members:` line spelled `null`, `Null`, `NULL` or `~` takes the first member as `members:` does (#920) The commit, after the fact (#459's U049, #363's option 2, which PR #574 left out). After `_commit_one`, `_checked_gitlink_committed` reads the commit's whole tree and its parent's with `git ls-tree -r` and no pathspec. With no gitlink at exactly `members/` it names each gitlink the commit holds that its parent does not, the path git chose, or says there is none, and refuses `member-gitlink-misplaced`. `MountRollback.take_back` first moves the branch back with `git update-ref`, only where the commit is one commit on where the command found the branch (a first commit where the branch had none, which is deleted again), and only while the branch still names it, so a commit anything else made is never lost. `undo` then unwinds the mount as for every refusal after it. Where the commit is not taken back, nothing is undone and the remedy says the commit and the mount are the human's to take out. Measured before, on 50178305, exit 0 with the branch on a commit whose `.gitmodules` and `family.yaml` name `members/IRQS`: #363's own state with #574's refusal switched off in process (git committed `Members/IRQS`), a `pre-commit` hook that takes the gitlink out (no gitlink at all), and one that records it at `parked/IRQS`. The `members:` line (#459's U188, RV870's NIT-3). `Family.rewritten` found only `members:` and `members: []`, so `members: null`, which the holder's reader and its `validate-family.py` read as no members, was refused `family-members-unrecognised`, "has 0 `members:` line(s)". `_opens_members` also takes `members: ` where the holder's reader reads the one word as null, so `null`, `Null`, `NULL` and `~` are found and the first member is written there. The line is taken as strictly as `members:` is: a comment after the word is still a hand edit the tool refuses rather than drop, and an indented `members: null` is a key of another mapping. Tests: tests/test_family_add_gitlink_and_null_members.py. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Opus 5.5 --- scripts/family.py | 148 ++++++- ...est_family_add_gitlink_and_null_members.py | 372 ++++++++++++++++++ 2 files changed, 517 insertions(+), 3 deletions(-) create mode 100644 tests/test_family_add_gitlink_and_null_members.py diff --git a/scripts/family.py b/scripts/family.py index e8ed8bd6..f07e1408 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -414,13 +414,17 @@ def rewritten(self, rows: list[dict]) -> str: refuse a `family.yaml` holding a change HEAD does not (`_checked_manifest_committed`), and this refusal is only reached once the shape it names is committed. + + THE `members:` LINE IS ANY THE HOLDER'S READER READS AS ONE WITH NO + ROWS (`_opens_members`, #920), so `members: null` is found as + `members:` is. """ text = self.path.read_text(encoding="utf-8") lines = text.splitlines() out: list[str] = [] seen = 0 for line in lines: - if line.rstrip() in ("members:", "members: []"): + if _opens_members(line): seen += 1 out.append("members: []" if not rows else "members:") for row in rows: @@ -440,6 +444,40 @@ def rewritten(self, rows: list[dict]) -> str: return "\n".join(out) + "\n" +def _opens_members(line: str) -> bool: + """Whether `line` is the `members:` line `Family.rewritten` rewrites + from: `members:`, `members: []`, or `members: ` where the word is + one the holder's own reader reads as null (#920). + + THE READER IS ASKED OF THE WORD, NOT A LIST OF SPELLINGS, as `_member_id` + asks it. The finder took only `members:` and `members: []`, while the + reader reads the words `null`, `Null` and `NULL`, and `~`, as the null a + bare `members:` is, and `Family.open` and the holder's own + `validate-family.py` both read that null as no members. So a holder + whose `family.yaml` said `members: null` passed its own gate, and `add` + refused it as having no `members:` line at all + (`family-members-unrecognised`) and wrote nothing. + + ONE WORD, AS THE TWO SPELLINGS ARE: the line is taken as strictly as + `members:` always was, at the margin, with one space before the word + and nothing after it but blanks. A comment after it is still a hand edit + this tool does not rewrite, which would drop it + (`test_a_manifest_this_tool_will_not_rewrite_is_refused_before_the_mount` + holds `members: # edited by hand` to that), and an indented + ` members: null` is a key of another mapping. + """ + text = line.rstrip() + if text in ("members:", "members: []"): + return True + word = text[len("members: "):] + if not text.startswith("members: ") or word.split() != [word]: + return False + try: + return parse_yaml(f"members: {word}\n") == {"members": None} + except YamlError: + return False + + #: What `add`, `bump` and `remove` say under `family-members-unreadable`. MEMBERS_UNREADABLE = ( "Remediation: `members:` is the list of rows `family.py add` writes, one " @@ -6492,7 +6530,9 @@ class MountRollback: NEVER OVER A COMMIT THAT LANDED. HEAD is recorded too, and if it has moved the one commit happened: undoing the tree under it would leave a working copy that contradicts its own history, so nothing is undone and - the sentence says so. + the sentence says so. The one commit `add` takes back itself, one whose + gitlink git recorded somewhere other than the mount path (#920), is + taken back by `take_back` first, so `undo` finds HEAD where it was. "EXACTLY AS IT WAS" ONLY FOR WHAT WAS COMPARED (#302). Each step puts back what it recorded, by its exact name. git can stage under a name no @@ -6657,6 +6697,43 @@ def _index_moved(self) -> str: parts.append(f"{said}: {shown}{more}") return "; ".join(parts) + def take_back(self, made: str) -> tuple[bool, str]: + """Take `made`, the commit this command made, back off the branch. + Returns `(whether it was, the sentence that says so, or why not)` + (#920). + + ONLY THIS COMMAND'S ONE COMMIT. The branch moves back to the commit + HEAD named before the mount, or is left with no commit, as it was, + only where `made` is one commit on that one (a root commit where + there was none), and only while the branch still names `made`: `git + update-ref` is given `made` as the value it must find there, so a + commit anything else put on the branch is never lost. Nothing else + moves. The index and the work tree keep what the commit took, which + is what `undo` was recorded to put back. + """ + before = [self.head] if self.head is not None else [] + try: + parents = git_out(["rev-list", "--parents", "--no-walk", made], + cwd=self.root).split()[1:] + if parents != before: + on = (f"one commit on {self.head[:12]}, where this command " + "found the branch" if self.head is not None else + "a first commit, on a branch that had none") + return False, (f" It was NOT taken back: {made[:12]} is not " + f"{on}, and this tool takes back no commit it " + "did not make.") + moved = ["update-ref", "-m", + f"family.py add: {made[:12]} taken back"] + git_out([*moved, "HEAD", self.head, made] if self.head is not None + else [*moved, "-d", "HEAD", made], cwd=self.root) + except Refusal as exc: + return False, f" It was NOT taken back: {_reason(exc)}." + if self.head is None: + return True, (" It has been taken back: the branch has no commit " + "again.") + return True, (" It has been taken back: the branch names " + f"{self.head[:12]} again.") + def undo(self) -> str: """Put the holder back. Returns the sentence a refusal ends with: what was put back, or — if any of it could not be — what is left, @@ -6760,6 +6837,67 @@ def _rewrite_members(family: Family, rows: list[dict], what: str) -> None: "room, then run the same command again.") from exc +#: What `member-gitlink-misplaced` tells the human to do (#920), and what it +#: says more where the commit was not taken back. +GITLINK_MISPLACED_REMEDIATION = ( + "Remediation: `add` asks git for the gitlink at `members/` and " + "nowhere else, and every cause known to record it elsewhere, another " + "spelling of `members/` in the index, in HEAD or on disk, is refused " + "before the mount, so this cause is not one of them. Find what moved it, " + "such as a hook or a setting like `core.ignorecase`, and mend that") +GITLINK_NOT_TAKEN_BACK = ( + ". The commit and the mount are still there, for the reason above: `git " + "log` shows what the branch holds, and taking them out is yours to do " + "before you run the same command again.") + + +def _gitlinks_at(root: Path, commit: str | None) -> dict[str, str]: + """`{path: the commit it records}` of every gitlink `commit` holds, by + `_tree_entries` with no pathspec; none for no commit.""" + if commit is None: + return {} + return {name: oid for name, kind, oid in _tree_entries(root, commit, True) + if kind == INDEX_ENTRY_KINDS[0o160000]} + + +def _checked_gitlink_committed(root: Path, project: str, path: str, + head: str, rollback: MountRollback) -> None: + """Refuse `member-gitlink-misplaced` unless `head`, the commit `add` has + just made, records a gitlink at exactly `path`, after taking that commit + back (`MountRollback.take_back`) for the caller's `undo` (#920). + + THE COMMIT ITSELF IS ASKED, AFTER THE FACT. `.gitmodules` and + `family.yaml` name `members/`, but git records the gitlink where + it chooses: with `core.ignorecase` set and HEAD holding + `Members/README.md` that the index no longer held, `add` committed it as + `Members/` with exit 0, and a fresh clone left the member empty + (#363). #574 refuses each such case variant before the mount, and this + is option 2 of #363, which PR #574 left out: whatever the cause, a + commit that records no gitlink at the path is never left on the branch. + It reads the commit's whole tree, and its parent's, with no pathspec + (`git ls-tree -r`), so no pathspec setting can narrow either, and names + each gitlink the commit holds that its parent does not: the path git + chose. + """ + made = _gitlinks_at(root, head) + if path in made: + return + before = _gitlinks_at(root, rollback.head) + chosen = [name for name, oid in made.items() if before.get(name) != oid] + where = (f"{project}'s gitlink at " + + " and ".join(f"`{shown_path(name)}`" for name in chosen) + + f", not at `{path}`" if chosen else + f"no new gitlink, at `{path}` or anywhere else") + taken, said = rollback.take_back(head) + raise Refusal( + "member-gitlink-misplaced", + f"the commit {head[:12]} records {where}, while {GITMODULES} and " + f"{MANIFEST} name `{path}`, so a fresh clone would leave the member " + "empty." + said, GITLINK_MISPLACED_REMEDIATION + + (", then run the same command again." if taken else + GITLINK_NOT_TAKEN_BACK)) + + def _mount_and_pin(family: Family, repository: str, project: str, path: str, url: str, protocol: list[str], at: str | None, rollback: MountRollback, @@ -6830,6 +6968,8 @@ def _mount_and_pin(family: Family, repository: str, project: str, path: str, "project: its own assembly root, its own legs, its own gate. " "Membership confers nothing.\n", [GITMODULES, path, MANIFEST], trailers=trailers) + rollback.step = f"reading where the commit records {project}'s gitlink" + _checked_gitlink_committed(family.root, project, path, head, rollback) return commit, digest, project_id, head @@ -6986,7 +7126,9 @@ def cmd_add(args) -> int: (`_probe_member`, of a throwaway clone), then the member's id against the rows (`_checked_member_id`, #330), and only then the mount, the pin and the commit (`_mount_and_pin`), inside a `MountRollback` that puts the holder - back if anything still fails and says what it put back. + back if anything still fails and says what it put back, the commit + included where it records the gitlink anywhere but the mount path + (`_checked_gitlink_committed`, #920). WHEN TWO APPLY, THE EARLIER ONE IS SAID. The holder's refusals need nothing cloned, so they come first: a section under another name that diff --git a/tests/test_family_add_gitlink_and_null_members.py b/tests/test_family_add_gitlink_and_null_members.py new file mode 100644 index 00000000..4a649957 --- /dev/null +++ b/tests/test_family_add_gitlink_and_null_members.py @@ -0,0 +1,372 @@ +# SPDX-License-Identifier: Apache-2.0 +"""`family.py add` checks its own commit for the gitlink at exactly +`members/`, and finds a `members:` line spelled as any null the +holder's reader reads (#920). + +THE COMMIT, AFTER THE FACT (#459's U049, #363's option 2, which PR #574 left +out). #574 refuses each known cause of a misplaced gitlink, another spelling +of `members/` in the index, in HEAD or on disk, before the mount, and nothing +asked the commit afterwards: a cause nobody foresaw would commit the gitlink +somewhere else with exit 0, and a fresh clone would leave the member empty. +Each test here makes such a commit: #363's own state, with #574's refusal +switched off in process so that git itself chooses the path, or a +`pre-commit` hook that moves the gitlink or takes it out. `add` must take its +commit back, unwind the mount as every refusal after the mount does, name +the path git chose, and never take back a commit it did not make. + +THE `members:` LINE (#459's U188, RV870's NIT-3). `add`'s finder took only +`members:` and `members: []`, so a holder whose `family.yaml` said `members: +null`, which its own gate reads as no members, was refused as having no +`members:` line at all. One test per spelling of null the reader reads, a +control for `members: []`, and the rule that only a line at the margin is +the manifest's own `members:`. + +THE SAME FAMILY AS `test_family.py`, and the same offline rules as +`test_family_add_refusals.py`, whose fixtures and helpers these are. +""" + +from __future__ import annotations + +import re +import sys +from pathlib import Path + +import pytest + +from conftest import git +#: The family fixture and its names, imported from the module that owns +#: them; importing a fixture gives this module its own instance of it. +from test_family import ( # noqa: F401 + ORG, SPARE, family, holder, manifest, +) +#: The offline environment, the in-process module, and the holder's +#: snapshot, from the module that tests every other refusal after the mount. +from test_family_add_refusals import ( # noqa: F401 + UNDONE, add_in_process, assert_unchanged, family_module, offline, plant, + refusal, snapshot, unborn_holder, +) +#: A `members:` line in place of the fixture's block, committed, from the +#: module that reads every other spelling of it (#864). +from test_family_members_not_a_list import members_are + +#: `add`'s mount path for the spare member. +MOUNT = f"members/{SPARE}" + +#: What `member-gitlink-misplaced` tells the human to do, before its last +#: words, which say whether the commit was taken back. +MISPLACED_REMEDIATION = ( + "Remediation: `add` asks git for the gitlink at `members/` and " + "nowhere else, and every cause known to record it elsewhere, another " + "spelling of `members/` in the index, in HEAD or on disk, is refused " + "before the mount, so this cause is not one of them. Find what moved it, " + "such as a hook or a setting like `core.ignorecase`, and mend that") + +#: Its last words where the commit was taken back. +TAKEN_BACK = ", then run the same command again." + +#: Its last words where it was not. +NOT_TAKEN_BACK = ( + ". The commit and the mount are still there, for the reason above: `git " + "log` shows what the branch holds, and taking them out is yours to do " + "before you run the same command again.") + +#: What the unwind puts back after a commit is taken back in a holder that +#: has members, so a `.gitmodules`, in `undo`'s order: the pin was written, +#: so `family.yaml` is restored as well as everything the mount made. +UNWOUND = UNDONE.format( + restored=f"the gitlink at {MOUNT} unstaged, {MOUNT} removed, " + f".git/modules/{MOUNT} removed, .gitmodules restored byte for " + "byte, family.yaml restored byte for byte, " + f"`submodule.{MOUNT}` dropped from the holder's git config") + +#: What the refused commit's sha is read back out of. +MADE_RE = re.compile(r"REFUSED member-gitlink-misplaced: the commit " + r"([0-9a-f]{12}) records ") + +#: Two `pre-commit` hooks, each a cause `add` cannot know before the mount, +#: with what the refusal says the commit records: one takes the member's +#: gitlink out of the commit, and one records it at `parked/`. +#: git runs the hook on the index it commits, and reads that index again +#: after it, so what the hook stages is what the commit holds. +HOOKS = { + "takes it out": ( + f"git rm -q --cached -- {MOUNT}\n", + f"no new gitlink, at `{MOUNT}` or anywhere else"), + "moves it": ( + f"sha=$(git rev-parse :{MOUNT}) || exit 1\n" + f"git rm -q --cached -- {MOUNT}\n" + f'git update-index --add --cacheinfo "160000,$sha,parked/{SPARE}"\n', + f"{SPARE}'s gitlink at `parked/{SPARE}`, not at `{MOUNT}`"), +} + +#: A hook is a `#!/bin/sh` file git runs, which git for Windows runs only +#: through its own shell; the POSIX runs prove the rule. +POSIX_HOOKS = pytest.mark.skipif(sys.platform == "win32", + reason="a #!/bin/sh hook") + + +def head(root: Path) -> str: + return git("rev-parse", "HEAD", cwd=root).stdout.strip() + + +def misplaced(made: str, where: str) -> str: + """The start of the REFUSED line for the commit `made`, which records + `where`, up to the sentence saying whether it was taken back.""" + return (f"REFUSED member-gitlink-misplaced: the commit {made} records " + f"{where}, while .gitmodules and family.yaml name `{MOUNT}`, so " + "a fresh clone would leave the member empty.") + + +def made_of(stderr: str) -> str: + """The twelve characters of the commit the refusal names.""" + found = MADE_RE.match(stderr) + assert found is not None, stderr + return found.group(1) + + +def gitlinks(root: Path, commit: str) -> list[str]: + """The path of every gitlink `commit` records, as `git ls-tree -r` + lists them.""" + return [line.partition("\t")[2] for line in git( + "ls-tree", "-r", commit, cwd=root).stdout.splitlines() + if line.startswith("160000 ")] + + +def hooked(root: Path, tmp_path: Path, name: str, body: str) -> None: + """The hook `name` running `body`, in a hooks folder of the test's own + that the holder's own config names (`core.hooksPath`), so neither the + operator's global setting nor `.git/hooks` decides where git looks.""" + hooks = tmp_path / "hooks" + hooks.mkdir(exist_ok=True) + (hooks / name).write_text("#!/bin/sh\n" + body, encoding="utf-8") + (hooks / name).chmod(0o755) + git("config", "--local", "core.hooksPath", hooks.as_posix(), cwd=root) + + +# --- the commit, after the fact (U049) --------------------------------------- + +def test_363s_own_state_is_taken_back_naming_the_path_git_chose( + family, holder, offline, family_module, monkeypatch, capsys): + """#363, AS IT WAS MEASURED. HEAD holds `Members/README.md`, an + uncommitted `git rm` has taken it out of the index, and `core.ignorecase` + is set: git commits the new gitlink in the spelling HEAD gives the + folder, `Members/IRQS`, while `.gitmodules` and `family.yaml` say + `members/IRQS`. #574 refuses that state before the mount + (`_members_spellings`), so its refusal is switched off here, in process, + to make git choose a path nothing refused. The commit is taken back, the + mount unwound, and the holder is the one the snapshot recorded, the + human's staged `git rm` included. The path the refusal names is the + one the commit it took back records.""" + plant(holder, "100644", "Members/README.md") + git("commit", "-q", "-m", "A folder in another spelling", cwd=holder) + git("rm", "-r", "-q", "--cached", "--", "Members", cwd=holder) + git("config", "--local", "core.ignorecase", "true", cwd=holder) + monkeypatch.setattr(family_module, "_members_spellings", + lambda root, project, on_disk, rows: []) + was = head(holder) + before = snapshot(holder) + assert before["status"] == "D Members/README.md\n" + + code = add_in_process(family_module, monkeypatch, offline, holder, SPARE, + family["remotes"]) + out, err = capsys.readouterr() + assert code == 2, out + err + assert out == (f"checking {ORG}/{SPARE} before mounting it\n" + f"mounting {ORG}/{SPARE} at {MOUNT}\n") + made = made_of(err) + assert refusal(err) == ( + misplaced(made, f"{SPARE}'s gitlink at `Members/{SPARE}`, not at " + f"`{MOUNT}`") + + f" It has been taken back: the branch names {was[:12]} again. " + + UNWOUND, MISPLACED_REMEDIATION + TAKEN_BACK) + assert git("rev-parse", f"{made}^", cwd=holder).stdout.strip() == was + assert sorted(gitlinks(holder, made)) == sorted( + [f"Members/{SPARE}", *gitlinks(holder, was)]) + assert snapshot(holder) == before + assert not (holder / MOUNT).exists() + offline.assert_left_nothing() + + +@POSIX_HOOKS +@pytest.mark.parametrize("hook", list(HOOKS)) +def test_a_commit_a_hook_moved_the_gitlink_in_is_taken_back( + family, holder, offline, tmp_path, hook): + """A CAUSE NOBODY FORESAW, through the real command: a `pre-commit` hook + takes the gitlink out of the commit, or records it at `parked/IRQS`. + `add` exited 0 for both, leaving the branch on a commit whose + `.gitmodules` and `family.yaml` name a gitlink it does not hold there. + Now the commit is taken back and the holder is exactly as it was.""" + body, where = HOOKS[hook] + hooked(holder, tmp_path, "pre-commit", body) + was = head(holder) + before = snapshot(holder) + + result = offline.add(holder, SPARE, remotes=family["remotes"]) + assert result.returncode == 2, result.stdout + result.stderr + assert refusal(result.stderr) == ( + misplaced(made_of(result.stderr), where) + + f" It has been taken back: the branch names {was[:12]} again. " + + UNWOUND, MISPLACED_REMEDIATION + TAKEN_BACK) + assert_unchanged(holder, before, SPARE) + offline.assert_left_nothing() + + +@POSIX_HOOKS +def test_a_commit_the_branch_has_moved_on_from_is_never_taken_back( + family, holder, offline, tmp_path): + """ONLY THIS COMMAND'S ONE COMMIT IS TAKEN BACK. A `post-commit` hook + puts a commit of its own on the branch after `add`'s, so HEAD names a + commit that is not one commit on where `add` found the branch. Taking + it back would lose the hook's commit too, so nothing is taken back and + nothing undone, the refusal says why, and the remedy leaves the commit + and the mount to the human.""" + hooked(holder, tmp_path, "pre-commit", HOOKS["takes it out"][0]) + hooked(holder, tmp_path, "post-commit", + 'git update-ref HEAD "$(git commit-tree -p HEAD ' + '-m "a commit a hook made" "HEAD^{tree}")"\n') + was = head(holder) + + result = offline.add(holder, SPARE, remotes=family["remotes"]) + assert result.returncode == 2, result.stdout + result.stderr + now = head(holder) + assert git("log", "-1", "--format=%s", cwd=holder).stdout == ( + "a commit a hook made\n") + assert git("rev-parse", "HEAD~2", cwd=holder).stdout.strip() == was + assert refusal(result.stderr) == ( + misplaced(now[:12], HOOKS["takes it out"][1]) + + f" It was NOT taken back: {now[:12]} is not one commit on " + f"{was[:12]}, where this command found the branch, and this tool " + "takes back no commit it did not make. The commit had already " + "landed, so nothing was undone: `git log -1` in the holder shows it.", + MISPLACED_REMEDIATION + NOT_TAKEN_BACK) + assert (holder / MOUNT / "project.yaml").is_file() + offline.assert_left_nothing() + + +def test_take_back_moves_no_branch_that_names_another_commit(holder, + family_module): + """THE VALUE `git update-ref` MUST FIND. `take_back` is handed a commit + that is one commit on where the rollback found the branch, but the + branch names another one by then. git is given the commit to take back + as the value the branch must hold, so it refuses, the branch is left on + the other commit, and the sentence quotes git.""" + was = head(holder) + rollback = family_module.MountRollback(holder, MOUNT) + tree = git("rev-parse", "HEAD^{tree}", cwd=holder).stdout.strip() + made = git("commit-tree", "-p", was, "-m", "this command's", tree, + cwd=holder).stdout.strip() + other = git("commit-tree", "-p", was, "-m", "anybody else's", tree, + cwd=holder).stdout.strip() + git("update-ref", "HEAD", other, cwd=holder) + + taken, said = rollback.take_back(made) + assert taken is False + assert said.startswith( + " It was NOT taken back: `git update-ref -m family.py add: " + f"{made[:12]} taken back HEAD {was} {made}` in "), said + assert head(holder) == other + + +@POSIX_HOOKS +def test_a_first_commit_is_taken_back_to_a_branch_with_none(family, offline, + tmp_path): + """A HOLDER WITH NO COMMIT YET gets `add`'s root commit (#203). Where + that commit holds no gitlink at the mount path, the branch is deleted + again, which is what it was, and the holder is unwound to the + untracked `family.yaml` the human left.""" + root = unborn_holder(tmp_path) + hooked(root, tmp_path, "pre-commit", HOOKS["takes it out"][0]) + before = snapshot(root) + assert before["HEAD"] == "" + + result = offline.add(root, SPARE, remotes=family["remotes"]) + assert result.returncode == 2, result.stdout + result.stderr + assert refusal(result.stderr) == ( + misplaced(made_of(result.stderr), HOOKS["takes it out"][1]) + + " It has been taken back: the branch has no commit again. " + + UNDONE.format( + restored=f"the gitlink at {MOUNT} unstaged, {MOUNT} removed, " + f".git/modules/{MOUNT} removed, .gitmodules removed, as " + "there was none before, family.yaml restored byte for " + f"byte, `submodule.{MOUNT}` dropped from the holder's " + "git config"), MISPLACED_REMEDIATION + TAKEN_BACK) + assert snapshot(root) == before + assert git("symbolic-ref", "HEAD", cwd=root).stdout == ( + "refs/heads/family/unborn\n") + offline.assert_left_nothing() + + +# --- the `members:` line (U188) ---------------------------------------------- + +#: `(the value after members:, what the holder's reader reads it as)`: +#: each spelling of null the reader reads (`repo_shape._scalar`), and then +#: `[]`, the control, found before #920 too. A comment after the value is +#: still a hand edit `add` refuses +#: (`test_a_manifest_this_tool_will_not_rewrite_is_refused_before_the_mount`). +SPELLINGS = [ + pytest.param("null", None, id="null"), + pytest.param("Null", None, id="Null"), + pytest.param("NULL", None, id="NULL"), + pytest.param("~", None, id="tilde"), + pytest.param("[]", [], id="control-empty-list"), +] + + +@pytest.mark.parametrize("value,read", SPELLINGS) +def test_the_first_member_is_written_under_any_members_line_read_as_none( + family, holder, offline, value, read): + """RV870's NIT-3. The holder's reader, and so its own gate, reads each + of these as no members, and `add` refused every one but `[]` as having + no `members:` line at all (`family-members-unrecognised`), writing + nothing. Now the line is found and rewritten as `members:` with the one + row under it, and nothing above it moves. The fixture's two mounts stay + as they are, unnamed by any row, as `test_family_members_not_a_list.py` + leaves them for its own controls.""" + members_are(holder, value) + assert manifest(holder)["members"] == read + kept = (holder / "family.yaml").read_text(encoding="utf-8").partition( + "\nmembers:")[0] + was = head(holder) + + result = offline.add(holder, SPARE, remotes=family["remotes"]) + assert result.returncode == 0, result.stdout + result.stderr + assert result.stderr == "" + text = (holder / "family.yaml").read_text(encoding="utf-8") + assert text.startswith(f"{kept}\nmembers:\n - project: {SPARE}\n") + assert [row["project"] for row in manifest(holder)["members"]] == [SPARE] + assert git("rev-parse", "HEAD~1", cwd=holder).stdout.strip() == was + assert git("show", "--name-only", "--format=", "HEAD", + cwd=holder).stdout.split() == [".gitmodules", "family.yaml", + MOUNT] + assert git("status", "--porcelain", cwd=holder).stdout == "" + offline.assert_left_nothing() + + +def test_an_indented_members_null_is_never_the_members_line(tmp_path, + family_module): + """AT THE MARGIN ONLY. The reader reads ` members: null` alone as it + reads `members: null`, but in the file it is a key of another mapping. + Rewritten with no rows, a manifest that holds one above its own + `members: []` comes back byte for byte, the indented line kept.""" + text = ("kind: family-manifest\nnotes:\n members: null\n" + "members: []\n") + (tmp_path / "family.yaml").write_text(text, encoding="utf-8") + held = family_module.Family(tmp_path, {"members": []}) + assert held.rewritten([]) == text + + +@pytest.mark.parametrize("line", ["members: ~ # no member yet", + "members: null"]) +def test_a_null_with_more_than_one_word_is_still_a_hand_edit( + tmp_path, family_module, line): + """ONE WORD, AS `members:` AND `members: []` ARE TAKEN. The reader reads + a comment after the null, or a second space before it, the same way, + but the line is rewritten whole and a comment on it would be dropped: + it is refused as the hand edit it always was, and nothing is + rewritten.""" + (tmp_path / "family.yaml").write_text( + f"kind: family-manifest\n{line}\n", encoding="utf-8") + held = family_module.Family(tmp_path, {"members": None}) + with pytest.raises(family_module.Refusal) as caught: + held.rewritten([]) + assert caught.value.code == "family-members-unrecognised" From 39f846466701322c4e19fc752aed7f5b131006dc Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sun, 18 Oct 2026 18:32:04 +0000 Subject: [PATCH 2/2] family.py add: a second `members:` line above the block is refused with a true count, not cut off; a commit a `post-commit` hook took off is said as it is; and another member's gitlink is never named as this one's (#920) Round 1 of lane 5's review of #957 (issuecomment-6253361722). SHOULD 1. `Family.rewritten` stopped at the first line `_opens_members` takes and never counted a second, so a `members: []` above `name:`, with the real block still at the end (the reader keeps the last key, and the holder's gate passes the file), was cut off there with exit 0, every key below it gone; with this branch `members: null` did the same. Every such line is counted first now, and anything but one is refused, `family-members-unrecognised`, "has 2 `members:` line(s)", before the mount. A `# members: null` comment is not counted. NIT 1. A `post-commit` hook that runs `git reset --soft HEAD~1` leaves the branch where `add` found it. That is said as it is ("the branch names again, where this command found it: the commit this command made is not on it"), `undo` puts the holder back, and the remedy says to run the command again, where it said "It was NOT taken back" and "The commit and the mount are still there" over a clean holder. NIT 2. The path git chose is a gitlink at a path the branch's earlier commit did not have, or one that records the member's commit, so another member's gitlink moved by the same cause is not named as this one's. The docstring names that earlier commit as the one `rollback.head` records, not the commit's parent. NIT 3. `HOOKS` gains a file and a folder at `members/`, another commit at a new path, another member's gitlink moved to another commit, and the member's commit at another member's path. NIT 4. The `rollback.step` line before the check is gone: nothing after it raises the `CommandFailed` that prints a step. NIT 5. A unit test refuses `take_back` of a first commit while the branch names another first commit (R4), and one rewrites past a word the reader refuses (R5). Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Opus 5.5 --- scripts/family.py | 74 +++++--- ...est_family_add_gitlink_and_null_members.py | 176 ++++++++++++++++-- 2 files changed, 214 insertions(+), 36 deletions(-) diff --git a/scripts/family.py b/scripts/family.py index f07e1408..3778670f 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -415,22 +415,21 @@ def rewritten(self, rows: list[dict]) -> str: (`_checked_manifest_committed`), and this refusal is only reached once the shape it names is committed. - THE `members:` LINE IS ANY THE HOLDER'S READER READS AS ONE WITH NO - ROWS (`_opens_members`, #920), so `members: null` is found as - `members:` is. + THE `members:` LINE is `members:`, `members: []`, or `members:` and + one word the holder's reader reads as null (`_opens_members`, #920), + so `members: null` is found as `members:` is. + + EVERY SUCH LINE IS COUNTED FIRST, and anything but one is refused + (#920, lane 5's review of #957, SHOULD 1). The rewrite stopped at the + first and never counted a second, so a `members: []` above `name:`, + with the real block still at the end, which the reader takes as the + last of two keys and the holder's gate passes, was cut off there with + exit 0: every key below it went, and the gate then failed the + commit. With the null words found, `members: null` did the same. """ text = self.path.read_text(encoding="utf-8") lines = text.splitlines() - out: list[str] = [] - seen = 0 - for line in lines: - if _opens_members(line): - seen += 1 - out.append("members: []" if not rows else "members:") - for row in rows: - out.extend(_member_lines(row)) - break - out.append(line) + seen = sum(1 for line in lines if _opens_members(line)) if seen != 1: raise Refusal( "family-members-unrecognised", @@ -441,6 +440,14 @@ def rewritten(self, rows: list[dict]) -> str: "file and everything below it belongs to this tool; fix it by " "hand this once and commit that (`git commit -- " f"{MANIFEST}`), then run the same command again.") + out: list[str] = [] + for line in lines: + if _opens_members(line): + out.append("members: []" if not rows else "members:") + for row in rows: + out.extend(_member_lines(row)) + break + out.append(line) return "\n".join(out) + "\n" @@ -6837,14 +6844,16 @@ def _rewrite_members(family: Family, rows: list[dict], what: str) -> None: "room, then run the same command again.") from exc -#: What `member-gitlink-misplaced` tells the human to do (#920), and what it -#: says more where the commit was not taken back. +#: What `member-gitlink-misplaced` tells the human to do (#920), then how it +#: ends: where the branch is back where `add` found it, so `undo` has put the +#: holder back, and where it is not. GITLINK_MISPLACED_REMEDIATION = ( "Remediation: `add` asks git for the gitlink at `members/` and " "nowhere else, and every cause known to record it elsewhere, another " "spelling of `members/` in the index, in HEAD or on disk, is refused " "before the mount, so this cause is not one of them. Find what moved it, " "such as a hook or a setting like `core.ignorecase`, and mend that") +GITLINK_TAKEN_BACK = ", then run the same command again." GITLINK_NOT_TAKEN_BACK = ( ". The commit and the mount are still there, for the reason above: `git " "log` shows what the branch holds, and taking them out is yours to do " @@ -6861,10 +6870,12 @@ def _gitlinks_at(root: Path, commit: str | None) -> dict[str, str]: def _checked_gitlink_committed(root: Path, project: str, path: str, - head: str, rollback: MountRollback) -> None: + commit: str, head: str, + rollback: MountRollback) -> None: """Refuse `member-gitlink-misplaced` unless `head`, the commit `add` has just made, records a gitlink at exactly `path`, after taking that commit back (`MountRollback.take_back`) for the caller's `undo` (#920). + `commit` is the member's, the one the pin records. THE COMMIT ITSELF IS ASKED, AFTER THE FACT. `.gitmodules` and `family.yaml` name `members/`, but git records the gitlink where @@ -6874,16 +6885,30 @@ def _checked_gitlink_committed(root: Path, project: str, path: str, (#363). #574 refuses each such case variant before the mount, and this is option 2 of #363, which PR #574 left out: whatever the cause, a commit that records no gitlink at the path is never left on the branch. - It reads the commit's whole tree, and its parent's, with no pathspec - (`git ls-tree -r`), so no pathspec setting can narrow either, and names - each gitlink the commit holds that its parent does not: the path git - chose. + It reads the whole tree of the commit, and of the one the branch named + before the mount (`rollback.head`), with no pathspec (`git ls-tree -r`), + so no pathspec setting can narrow either. The path git chose is each + gitlink the commit holds at a path the earlier one does not, or that + records `commit`, so another member's gitlink, moved by the same cause, + is never named as this one's (lane 5's review of #957, NIT 2). + + A BRANCH BACK WHERE `add` FOUND IT holds no commit of this command's: a + `post-commit` hook took it off. That is said as it is, and `undo` puts + the holder back as for any refusal before the commit (NIT 1). """ + if head == rollback.head: + raise Refusal( + "member-gitlink-misplaced", + f"the branch names {head[:12]} again, where this command found " + "it: the commit this command made is not on it, so no gitlink at " + f"`{path}` is either, while {GITMODULES} and {MANIFEST} name " + f"`{path}`.", GITLINK_MISPLACED_REMEDIATION + GITLINK_TAKEN_BACK) made = _gitlinks_at(root, head) if path in made: return before = _gitlinks_at(root, rollback.head) - chosen = [name for name, oid in made.items() if before.get(name) != oid] + chosen = [name for name, oid in made.items() + if name not in before or oid == commit] where = (f"{project}'s gitlink at " + " and ".join(f"`{shown_path(name)}`" for name in chosen) + f", not at `{path}`" if chosen else @@ -6894,8 +6919,7 @@ def _checked_gitlink_committed(root: Path, project: str, path: str, f"the commit {head[:12]} records {where}, while {GITMODULES} and " f"{MANIFEST} name `{path}`, so a fresh clone would leave the member " "empty." + said, GITLINK_MISPLACED_REMEDIATION - + (", then run the same command again." if taken else - GITLINK_NOT_TAKEN_BACK)) + + (GITLINK_TAKEN_BACK if taken else GITLINK_NOT_TAKEN_BACK)) def _mount_and_pin(family: Family, repository: str, project: str, path: str, @@ -6968,8 +6992,8 @@ def _mount_and_pin(family: Family, repository: str, project: str, path: str, "project: its own assembly root, its own legs, its own gate. " "Membership confers nothing.\n", [GITMODULES, path, MANIFEST], trailers=trailers) - rollback.step = f"reading where the commit records {project}'s gitlink" - _checked_gitlink_committed(family.root, project, path, head, rollback) + _checked_gitlink_committed(family.root, project, path, commit, head, + rollback) return commit, digest, project_id, head diff --git a/tests/test_family_add_gitlink_and_null_members.py b/tests/test_family_add_gitlink_and_null_members.py index 4a649957..297130b1 100644 --- a/tests/test_family_add_gitlink_and_null_members.py +++ b/tests/test_family_add_gitlink_and_null_members.py @@ -37,7 +37,7 @@ #: The family fixture and its names, imported from the module that owns #: them; importing a fixture gives this module its own instance of it. from test_family import ( # noqa: F401 - ORG, SPARE, family, holder, manifest, + NAME, ORG, SPARE, family, holder, manifest, ) #: The offline environment, the in-process module, and the holder's #: snapshot, from the module that tests every other refusal after the mount. @@ -83,20 +83,50 @@ MADE_RE = re.compile(r"REFUSED member-gitlink-misplaced: the commit " r"([0-9a-f]{12}) records ") -#: Two `pre-commit` hooks, each a cause `add` cannot know before the mount, -#: with what the refusal says the commit records: one takes the member's -#: gitlink out of the commit, and one records it at `parked/`. -#: git runs the hook on the index it commits, and reads that index again -#: after it, so what the hook stages is what the commit holds. +#: The commit of the member's gitlink, as a `pre-commit` hook reads it, then +#: the gitlink taken out of the commit. +OUT = (f"sha=$(git rev-parse :{MOUNT}) || exit 1\n" + f"git rm -q --cached -- {MOUNT}\n") + +#: A blob of the hook's own, for a file at or under the mount path. +BLOB = "blob=$(echo a file | git hash-object -w --stdin) || exit 1\n" + +#: `pre-commit` hooks, each a cause `add` cannot know before the mount, with +#: what the refusal says the commit records. git runs the hook on the index +#: it commits, and reads that index again after it, so what the hook stages +#: is what the commit holds. The commit holds: no gitlink; the member's at +#: `parked/`; another commit there, at a path the holder did not +#: have; a file at the mount path; a folder there; another +#: member's gitlink moved to another commit, which is not this member's +#: (lane 5's review of #957, NIT 2); and the member's commit at another +#: member's path, which is. HOOKS = { "takes it out": ( - f"git rm -q --cached -- {MOUNT}\n", - f"no new gitlink, at `{MOUNT}` or anywhere else"), + OUT, f"no new gitlink, at `{MOUNT}` or anywhere else"), "moves it": ( - f"sha=$(git rev-parse :{MOUNT}) || exit 1\n" - f"git rm -q --cached -- {MOUNT}\n" - f'git update-index --add --cacheinfo "160000,$sha,parked/{SPARE}"\n', + OUT + "git update-index --add --cacheinfo " + f'"160000,$sha,parked/{SPARE}"\n', + f"{SPARE}'s gitlink at `parked/{SPARE}`, not at `{MOUNT}`"), + "parks another commit": ( + OUT + "git update-index --add --cacheinfo " + f'"160000,$(git rev-parse HEAD),parked/{SPARE}"\n', f"{SPARE}'s gitlink at `parked/{SPARE}`, not at `{MOUNT}`"), + "a file there": ( + OUT + BLOB + + f'git update-index --add --cacheinfo "100644,$blob,{MOUNT}"\n', + f"no new gitlink, at `{MOUNT}` or anywhere else"), + "a folder there": ( + OUT + BLOB + + f'git update-index --add --cacheinfo "100644,$blob,{MOUNT}/README"' + "\n", + f"no new gitlink, at `{MOUNT}` or anywhere else"), + "another member moved": ( + OUT + 'git update-index --cacheinfo "160000,$(git rev-parse HEAD),' + 'members/IRRS"\n', + f"no new gitlink, at `{MOUNT}` or anywhere else"), + "onto another member": ( + OUT + 'git update-index --cacheinfo "160000,$sha,members/IRRS"\n', + f"{SPARE}'s gitlink at `members/IRRS`, not at `{MOUNT}`"), } #: A hook is a `#!/bin/sh` file git runs, which git for Windows runs only @@ -243,6 +273,32 @@ def test_a_commit_the_branch_has_moved_on_from_is_never_taken_back( offline.assert_left_nothing() +@POSIX_HOOKS +def test_a_commit_a_hook_took_off_the_branch_is_said_as_it_is( + family, holder, offline, tmp_path): + """A `post-commit` HOOK THAT TAKES THE COMMIT OFF ITSELF (lane 5's review + of #957, NIT 1): `git reset --soft HEAD~1` leaves the branch where `add` + found it, with the mount still staged. The refusal says the branch holds + no commit of the command's, the mount is unwound as before any commit, + and the remedy says to run the command again, which is what is true: + the holder is as it was. It used to say "It was NOT taken back" and + "The commit and the mount are still there" over a clean holder.""" + hooked(holder, tmp_path, "post-commit", "git reset -q --soft HEAD~1\n") + was = head(holder) + before = snapshot(holder) + + result = offline.add(holder, SPARE, remotes=family["remotes"]) + assert result.returncode == 2, result.stdout + result.stderr + assert refusal(result.stderr) == ( + f"REFUSED member-gitlink-misplaced: the branch names {was[:12]} " + "again, where this command found it: the commit this command made " + f"is not on it, so no gitlink at `{MOUNT}` is either, while " + f".gitmodules and family.yaml name `{MOUNT}`. " + UNWOUND, + MISPLACED_REMEDIATION + TAKEN_BACK) + assert_unchanged(holder, before, SPARE) + offline.assert_left_nothing() + + def test_take_back_moves_no_branch_that_names_another_commit(holder, family_module): """THE VALUE `git update-ref` MUST FIND. `take_back` is handed a commit @@ -267,6 +323,29 @@ def test_take_back_moves_no_branch_that_names_another_commit(holder, assert head(holder) == other +def test_take_back_deletes_no_first_commit_it_did_not_make(tmp_path, + family_module): + """THE SAME VALUE ON A BRANCH THAT HAD NO COMMIT (lane 5's review of + #957, NIT 5, R4). The branch is deleted only while it names the commit + handed to `take_back`. Where another first commit is on it by then, git + refuses, and the branch keeps that commit.""" + root = unborn_holder(tmp_path) + rollback = family_module.MountRollback(root, MOUNT) + tree = git("write-tree", cwd=root).stdout.strip() + made = git("commit-tree", "-m", "this command's", tree, + cwd=root).stdout.strip() + other = git("commit-tree", "-m", "anybody else's", tree, + cwd=root).stdout.strip() + git("update-ref", "HEAD", other, cwd=root) + + taken, said = rollback.take_back(made) + assert taken is False + assert said.startswith( + " It was NOT taken back: `git update-ref -m family.py add: " + f"{made[:12]} taken back -d HEAD {made}` in "), said + assert head(root) == other + + @POSIX_HOOKS def test_a_first_commit_is_taken_back_to_a_branch_with_none(family, offline, tmp_path): @@ -370,3 +449,78 @@ def test_a_null_with_more_than_one_word_is_still_a_hand_edit( with pytest.raises(family_module.Refusal) as caught: held.rewritten([]) assert caught.value.code == "family-members-unrecognised" + + + +def test_a_word_the_reader_refuses_is_no_members_line(tmp_path, + family_module): + """`_opens_members` asks the reader of the word, and a word it refuses + (`@x`) opens no block. Through the command `Family.open` refuses such a + file first, so this is asked of the rewrite alone (lane 5's review of + #957, NIT 5, R5): the one real `members: []` is found, and nothing + raises.""" + text = "kind: family-manifest\nmembers: @x\nmembers: []\n" + (tmp_path / "family.yaml").write_text(text, encoding="utf-8") + held = family_module.Family(tmp_path, {"members": []}) + assert held.rewritten([]) == text + + +#: What `family-members-unrecognised` tells the human to do. +UNRECOGNISED_REMEDIATION = ( + "Remediation: the manifest was hand-edited into a shape this tool does " + "not recognise. `members:` is the LAST block in the file and everything " + "below it belongs to this tool; fix it by hand this once and commit that " + "(`git commit -- family.yaml`), then run the same command again.") + + +def a_line_above_the_name(root: Path, line: str) -> None: + """`line` at the margin above `family.yaml`'s `name:`, with the real + `members:` block still at the end, committed, as a person commits a + hand edit before a verb (#684).""" + path = root / "family.yaml" + text = path.read_text(encoding="utf-8") + assert text.count("\nname: ") == 1 + path.write_bytes(text.replace("\nname: ", f"\n{line}\nname: ") + .encode("utf-8")) + git("commit", "-q", "-m", "a line above name:", "--", "family.yaml", + cwd=root) + + +@pytest.mark.parametrize("line", ["members: null", "members: []"]) +def test_a_second_members_line_above_the_block_is_refused(family, holder, + offline, line): + """LANE 5'S REVIEW OF #957, SHOULD 1. The reader takes the last of two + `members:` keys, so `Family.open` and the holder's gate both accept this + file. The rewrite stopped at the first such line and cut the manifest + off there, every key below it gone, with exit 0, for `members: []` on + main and for `members: null` with this branch. Every line is counted + first now, so both are refused before the mount with a count that is + true, and nothing moves.""" + a_line_above_the_name(holder, line) + 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) == ( + "REFUSED family-members-unrecognised: " + f"{holder.resolve() / 'family.yaml'} has 2 `members:` line(s) where " + "exactly one was expected, so this tool will not rewrite it", + UNRECOGNISED_REMEDIATION) + assert_unchanged(holder, before, SPARE) + offline.assert_left_nothing() + + +def test_a_members_line_in_a_comment_is_not_counted(family, holder, offline): + """The control: a `# members: null` above `name:` is a comment, so the + block is the one line counted, the member is added, and every key is + kept.""" + a_line_above_the_name(holder, "# members: null") + + result = offline.add(holder, SPARE, remotes=family["remotes"]) + assert result.returncode == 0, result.stdout + result.stderr + assert manifest(holder)["name"] == NAME + assert SPARE in [row["project"] for row in manifest(holder)["members"]] + assert "\n# members: null\nname: " in (holder / "family.yaml").read_text( + encoding="utf-8") + offline.assert_left_nothing()