diff --git a/scripts/family.py b/scripts/family.py index 3bc79e61..7629761b 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -67,9 +67,9 @@ write and FETCHES the member, as a real bump does: a fetch writes into the member's object store, which nothing in the holder records, and it is how `--to` is resolved and digested. It then reads the member's `project.yaml` -and tree from those objects, so the refusals the real run makes after its -checkout are asked too, and prints the row it would write and the trailers -its commit would end with; it never checks the member out, rewrites +and tree from those objects, as the real run does before its checkout +(#365), so the two refuse alike, and prints the row it would write and the +trailers its commit would end with; it never checks the member out, rewrites `family.yaml`, stages or commits. `remove --dry-run` makes every refusal `remove` makes before its first write and prints the row, the gitlink, the `.gitmodules` section, the holder's registration of the member and the @@ -89,11 +89,26 @@ `.gitmodules` is refused by the real `remove`'s `git rm` and not by its dry run; a `.gitmodules` the index or HEAD holds and the work tree does not is refused by the real `add`'s `git submodule add`, and its dry run exits 0 with -a note that says so. Each is git's own refusal, made with the holder -unchanged. A `project.yaml` that is a symbolic link is read through the link +a note that says so; and a held `index.lock` is refused by the real `remove` +before it unmounts anything (#365) and not by its dry run, because a lock is +a fact about the moment, not about the holder. Each is git's own refusal or +one made before the first write, and a real run puts back what it wrote +first. A `project.yaml` that is a symbolic link is read through the link by a real `add`'s mount and by nothing before it, so `add --dry-run` leaves its `id:` to the real run and says so; what fails only once the mount is made is unwound by the real run and never reached by its dry run. + +A REFUSED `bump` OR `remove` PUTS THE HOLDER BACK (#365), as a refused `add` +does (#203). `PinRollback` records what each one writes before the first +write and puts it back on any failure after it, and the refusal ends with +the sentence that names what was put back, or what is left and the commands +that put it back, in order. The exit is still 2, and a failed git command is +still quoted as it ran. `remove`'s checkout comes back only from an object +store git already has, never from a remote: where git has none, the sentence +says the checkout is not back. `remove` also refuses, under `--dry-run` as +well, a `.gitmodules` that gives the member's path to more than one +submodule (#360): `git rm` takes out the section of the one git finds for +that path, and its commit used to keep the others. """ from __future__ import annotations @@ -1209,11 +1224,21 @@ def _submodule_paths( 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` + `add`'s 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 and its keys - (`member-path-registered`, #300). A second reader could read a second - copy of the file. + (`member-path-registered`, #300). A second reader for those could read a + second copy of the file. + + IT IS NOT THE FILE'S ONLY READER. `remove` reads the WORK TREE's copy, + which is the one `git rm` edits, twice more, for questions this one does + not answer: `_gitmodules_names` models git's own path lookup (a name + keeps its LAST `path`, and a name that moves on takes the old path out of + git's map), which is the name `git rm` drops and the name whose object + store `git submodule update` uses; and `_sections_giving` names every + section that gives a path in ANY of its `path` lines (#360). All three + read it through `git config --list`, never through a parser of this + tool's. """ if os.path.lexists(root / GITMODULES): source = ["--file", GITMODULES] @@ -1382,8 +1407,7 @@ def _checked_mount_path(root: Path, project: str, path: str) -> None: "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, in a POSIX shell — {removal}, then `git add " - f"{GITMODULES}` — or, if it was a member, with `family.py " - "remove`; then re-run." + f"{GITMODULES}` — and 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 " @@ -1965,6 +1989,27 @@ def undo(self) -> str: return " Nothing was added, and nothing in the holder had changed." +def _rewrite_members(family: Family, rows: list[dict], what: str) -> None: + """`family.write_members`, the rows rewritten `what` (`with Atlas's + pin`), with a failed write made a refusal, `family-manifest-unwritable`, + and never a traceback. + + ONE SPELLING FOR `add`, `bump` AND `remove` (#365), the rule + `update-shape.py` keeps for the same reason: each caller puts the holder + back on a refusal, and an `OSError` reaching `main` would print a + traceback over the one sentence that says the holder was put back. + """ + try: + family.write_members(rows) + except OSError as exc: + raise Refusal( + "family-manifest-unwritable", + f"{MANIFEST} could not be rewritten {what}: " + f"{exc.strerror or exc}", + f"Remediation: make {MANIFEST} writable, and check the disk has " + "room, then run the same command again.") from exc + + def _mount_and_pin(family: Family, repository: str, project: str, path: str, url: str, protocol: list[str], at: str | None, rollback: MountRollback, @@ -2023,19 +2068,7 @@ def _mount_and_pin(family: Family, repository: str, project: str, path: str, digest = tree_digest(member, commit) row = _member_row(project, project_id, repository, commit, digest) rows = sorted(family.members + [row], key=lambda r: str(r["project"])) - # A FAILED WRITE IS A REFUSAL, NEVER A TRACEBACK — the rule - # `update-shape.py` keeps for the same reason: the caller unwinds on a - # refusal, and an `OSError` reaching `main` would print a traceback over - # the one sentence that says the holder was put back. - try: - family.write_members(rows) - except OSError as exc: - raise Refusal( - "family-manifest-unwritable", - f"{MANIFEST} could not be rewritten with {project}'s pin: " - f"{exc.strerror or exc}", - f"Remediation: make {MANIFEST} writable, and check the disk has " - "room, then run the same command again.") from exc + _rewrite_members(family, rows, f"with {project}'s pin") rollback.step = f"committing {project} to the family" head = _commit_one( @@ -2251,6 +2284,836 @@ def cmd_add(args) -> int: return 0 +# --------------------------------------------------------------------------- +# bump and remove: putting the holder back (#365) +# --------------------------------------------------------------------------- + + +#: What every git command that puts a checkout back runs with, first on its +#: line (`_offline_git`): no transport at all, a local path's included. +NO_REMOTE = ["-c", "protocol.allow=never"] + +#: What `_offline_git` puts in the environment of every git command it runs, +#: over the caller's: no terminal prompt, and an EMPTY `GIT_ALLOW_PROTOCOL`, +#: which allows no transport at all and, unlike `NO_REMOTE`, is not +#: overridden by a `protocol..allow` set by name or by the caller's own +#: `GIT_ALLOW_PROTOCOL` (review 365-7cca975, NIT-A). +OFFLINE_ENV = {"GIT_TERMINAL_PROMPT": "0", "GIT_ALLOW_PROTOCOL": ""} + + +def _offline_git(args: list[str], cwd: Path) -> str: + """`git -c protocol.allow=never ` in `cwd`, with `OFFLINE_ENV` in + its environment (`GIT_TERMINAL_PROMPT=0` and an empty + `GIT_ALLOW_PROTOCOL`): a git command of the rollback's restore (#365). + Its output, as `run` returns it; a failure is a `CommandFailed`, as + `run`'s is. + + THE BACKSTOP UNDER THE STORE CHECK (`_unrestorable`). The rollback runs + `git submodule update` only where git has the store and the commit, so + nothing needs a remote; this is for the check being wrong. `-c` reaches + every git the command starts, through `GIT_CONFIG_PARAMETERS`, the clone + `git submodule update` would make included, and `protocol.allow=never` + refuses each transport git would otherwise allow, https, ssh and a + local path alike; with no terminal prompt, a credential git would ask + for is a failure, not a wait. `--no-fetch` stops the fetch into a store + that lacks the commit, and the store is checked before `update` runs at + all. + + WHY `GIT_ALLOW_PROTOCOL` IS EMPTY HERE. `protocol.allow` yields to a + `protocol..allow` set by name and to the caller's + `GIT_ALLOW_PROTOCOL`, so a person whose environment or configuration + allows the file protocol, or any other, had `-c protocol.allow=never` + defeated and a wrong store check cloned from the member's url (measured + with a local bare repository, review 365-7cca975, NIT-A). The variable + OVERRIDES both: git then behaves as if `protocol.allow` were `never` and + each protocol it lists, none here, `always`, whatever the configuration + says. The restore reads local object stores and needs no transport, so + nothing is lost; the `-c` and `--no-fetch` guards stay as the layers + under it. + """ + command = ["git", *NO_REMOTE, *args] + check_program(command) + proc = subprocess.run(command, cwd=str(cwd), capture_output=True, + text=True, encoding="utf-8", errors="replace", + check=False, env={**os.environ, **OFFLINE_ENV}) + if proc.returncode != 0: + raise CommandFailed(command, cwd, proc.returncode, + (proc.stderr or "") + (proc.stdout or "")) + return (proc.stdout or "").strip() + + +def _store_holds(store: Path, commit: str, work: Path) -> bool: + """Whether the object store `store` holds `commit`. + + `--work-tree`, a directory this never reads: a submodule's store names + its checkout in `core.worktree`, and git asked with `--git-dir` alone + changes into that directory first, and dies where `git rm` deleted it + (git 2.43, probed for #365). Under the backstop, so that a store that + fetches what it lacks on demand fetches nothing here either. + """ + try: + _offline_git(["--git-dir", str(store), "--work-tree", str(work), + "cat-file", "-e", f"{commit}^{{commit}}"], work) + except CommandFailed: + return False + return True + + +def _store_git_uses(root: Path, path: str) -> tuple[str, Path] | None: + """`(as git spells it, on disk)` of the object store `git submodule + update`, run in `root`, keeps for the submodule at `path`, there or not; + None where no section of the work tree's `.gitmodules` gives git the + path. + + THE STORE OF THE NAME, NOT OF THE PATH (#365). git keeps a submodule's + store at `modules/` under the git directory (`_module_dir`, given + the name), `` being the submodule `.gitmodules` gives the path to: + git's own path lookup, which `git rm` and `git submodule update` share + and `_gitmodules_names` models. The two are the same for every member + `add` mounts and differ once a section is renamed by hand. + """ + name = _gitmodules_names(root, path)[0] + return None if name is None else _module_dir(root, name) + + +def _unrestorable(root: Path, path: str, commit: str | None) -> str | None: + """Why `git submodule update --init --no-fetch -- `, run in + `root`, could NOT check `commit` out of an object store git already + has; None when it could (#365). + + THE STORE GIT WILL USE (`_store_git_uses`), NOT ONE THE ROLLBACK + RECORDED, because `update` CLONES a store where there is none. The store + a checkout's own `.git` names is not always that one: a section renamed + by hand sent `update` to another name's store, absent, and the rollback + cloned the member from its url while the store it had sat unused; and + `deinit` moves an embedded `.git` into the name's own, which a check of + the recorded one called gone. A store that does not hold `commit` is not + used either: `update` would fetch into it. + """ + found = _store_git_uses(root, path) + if found is None: + return f"no section of {GITMODULES} gives git {path}" + shown, store = found + if not store.is_dir(): + return (f"git would check it out of {shown}, which is not there, and " + "clone it there") + if commit is not None and not _store_holds(store, commit, root): + return (f"git would check it out of {shown}, which does not hold " + f"{commit[:12]}, and fetch it") + return None + + +def _member_head(member: Path) -> tuple[str | None, str | None]: + """`(the commit a checkout stands at, the branch it is on)`, each None + where there is none: no checkout, no commit yet, a detached HEAD. + + ONLY WHERE THE CHECKOUT HAS A `.git` OF ITS OWN. A directory `git + submodule deinit` has emptied is a plain directory inside the holder, and + git asked there answers with the HOLDER's HEAD (git 2.43, probed for + #365): the rollback would have taken the holder's commit for the + member's. + """ + if not os.path.lexists(member / ".git"): + return None, None + try: + at = git_out(["rev-parse", "-q", "--verify", "HEAD^{commit}"], + cwd=member).lower() + except Refusal: + at = None + try: + ref = git_out(["symbolic-ref", "-q", "HEAD"], cwd=member) + except Refusal: + ref = "" + return at, (ref[len("refs/heads/"):] if ref.startswith("refs/heads/") + else None) + + +def _head_target(member: Path, at: str, branch: str | None) -> list[str]: + """What `git checkout -q` is given to put a checkout back at `at`: the + branch it was on, while that branch still names `at`, with `--` after + it so a file of the branch's name is never what is checked out; else + the commit itself.""" + if branch is not None: + try: + named = git_out(["rev-parse", "-q", "--verify", + f"refs/heads/{branch}^{{commit}}"], cwd=member) + except Refusal: + named = "" + if named.lower() == at: + return [branch, "--"] + return [at] + + +def _head_back(member: Path, at: str | None, branch: str | None) -> bool: + """Put a checkout back at `at`, on `branch` if it was on one; False when + it already stands there. + + THE BRANCH TOO, NOT THE COMMIT ALONE. `git submodule add` leaves a new + member ON its branch, and `bump`'s checkout of `--to`, like the checkout + `git submodule update` makes, DETACHES it: putting the commit back alone + would leave the right commit and the branch no longer checked out. + Under `_offline_git`'s backstop, as every command that puts a checkout + back runs: a checkout reads only the store, and fetches nothing even + from a store that fetches what it lacks on demand. + """ + if at is None or _member_head(member) == (at, branch): + return False + _offline_git(["checkout", "-q", *_head_target(member, at, branch)], + member) + return True + + +def _head_by_hand(path: str, at: str, branch: str | None) -> str: + """`_head_back` as the human types it in the holder: the branch the + checkout was on, else the commit. FROM THE RECORD, with no git call: it + is named when a step failed, often with `path` not on disk at all.""" + target = f"{shlex.quote(branch)} --" if branch is not None else at + return f"git -C {shlex.quote(path)} checkout -q {target}" + + +def _entries_by_hand(name: str, entries: list[bytes]) -> str: + """The command that puts `name`'s index entries back as `entries` holds + them, typed in the holder: `--cacheinfo` for the one record a path + usually has (with `--add`, which a path `git rm` took out needs), `git rm + --cached` for a path the index did not hold, and `--index-info` for the + records of a conflicted one.""" + quoted = shlex.quote(name) + if not entries: + return f"git rm -q --cached -- {quoted}" + fields = [entry.partition(b"\t")[0].decode("ascii").split() + for entry in entries] + if len(fields) == 1 and fields[0][2] == "0": + mode, oid, _ = fields[0] + return f"git update-index --add --cacheinfo {mode},{oid},{quoted}" + records = "".join(f"{mode} {oid} {stage}\\t{name}\\n" + for mode, oid, stage in fields) + return (f"printf {shlex.quote(records.replace('%', '%%'))} | " + "git update-index --index-info") + + +class PinRollback: + """Everything `bump` and `remove` change in the holder once they write, + and how to put the holder back (#365): `MountRollback`'s rule for `add` + (#203), for the two verbs that were written before it. + + WHAT EACH ONE WRITES. `bump` checks the member out at `--to`, rewrites + `family.yaml`, stages the gitlink, then stages the manifest and commits. + `remove` runs `git submodule deinit -f`, which empties the member's + checkout and drops its registration from the holder's git config; then + `git rm`, which unstages the gitlink, deletes the directory and takes + the member's section out of `.gitmodules`; then it rewrites + `family.yaml`, stages both files and commits. A holder `pre-commit` hook + that exits 1, or a lock taken by somebody else, failed either verb after + those steps, and exit 2 left every one of them in place, with nothing + said about any of it. + + RECORDED BEFORE THE FIRST WRITE, and put back by `undo` on ANY failure + after it: the index entries of the gitlink and of each file, picked out + of the WHOLE index (`_index_records`), so no pathspec setting in the + environment narrows what is read and no call here names the member's + path (#299 holds every such call to a literal spelling); each file's + bytes; and the commit and branch the member's checkout stands at. + `remove` also records every `submodule.*` key in the holder's git + config, the member's object store, which of the member's own submodules + (its legs) are checked out, and whether `git status --ignored` lists + anything in it. + + `remove`'s CHECKOUT COMES BACK FROM THE STORE GIT KEEPS FOR IT, AND + NEVER FROM A REMOTE: `git submodule update --init --no-fetch -- + members/

`, once the gitlink, `.gitmodules` and the registration are + back, reconnects that store and checks the gitlink's commit out of it. + `update` takes the store of the NAME `.gitmodules` gives the path, and + CLONES one where there is none, so it runs only where that store is + there and holds the commit (`_unrestorable`), never on the strength of + the store the checkout's own `.git` named when it was recorded. Where it + is not, `update` is not run: the directory is made again, empty, as a + member never checked out has it, and the sentence says the checkout + comes back only from its remote; where the store its checkout had is + still there under a name git does not look for (a section renamed by + hand) and the sentence has named it, it says the checkout can come back + from that store with no network, or from its remote, and prints no + command that does either. So too for a member whose + checkout git could not read before the first write, its store gone: it + is recorded as one no store is known to hold, and its `remove` goes + ahead as it always has. Every git command that puts a checkout back runs + under `_offline_git`'s backstop, and `update` with `--no-fetch`, so a + store check that is wrong still reaches no remote. The member's branch + is put back after it, and each leg that was checked out, by the same + rule, from the stores inside the member's. What `deinit -f` discards — + changes nobody committed, untracked and ignored files — is in no store, + so when `git status --ignored` listed anything in the member the + sentence says that much is gone instead of calling the holder as it was. + + THE REGISTRATION COMES BACK AS THE SAME KEYS, NOT THE SAME BYTES. + `git config --add` writes a section it puts back LAST in `.git/config`, + so the sentence says the git config holds the same keys and values and + does not call it as it was. Its recorded bytes are not written back: + `deinit` rewrites the file, a `pre-commit` hook or another git command + can write it while the run goes on, and `update --init` registers a + name it reads, so nothing shows that the bytes recorded before the first + write are all the file should hold afterwards. + + NEVER OVER A COMMIT THAT LANDED, and "AS IT WAS" ONLY FOR WHAT WAS + COMPARED, as `MountRollback` says them. Its steps that do the same thing + here are borrowed below rather than spelled a second time. + + WHAT IS LEFT IS NAMED WITH THE COMMANDS THAT PUT IT BACK. A step that + fails — commonly a lock somebody took after the run began, which refuses + every index write and nothing else — is named with git's own reason, a + step that needs it is not tried (the checkout needs the gitlink, + `.gitmodules` and the registration back), and the sentence gives, in + the order to type them, the commands that do each one by hand. + """ + + _head = MountRollback._head + _bytes = MountRollback._bytes + _undo_file = MountRollback._undo_file + _index_moved = MountRollback._index_moved + + def __init__(self, root: Path, path: str, verb: str, + files: tuple[str, ...], *, unmount: bool = False) -> None: + self.root = root + self.path = path + self.verb = verb + self.unmount = unmount + self.member = root / path + #: What the loud block of a failed git command is headed with: the + #: words `main` has always used, but for `bump`'s checkout, whose + #: failure has always been named for itself. + self.step = "a git or gh command failed" + self.head = self._head() + self.index = _index_records(root) + self.gitlink = self.index.get(path.encode("utf-8"), []) + self.files = {name: (self._bytes(name), + self.index.get(name.encode("utf-8"), [])) + for name in files} + self.at, self.branch = _member_head(self.member) + self.registration = self._registration() if unmount else [] + self.existed = self.member.is_dir() + #: The commit the gitlink names, which `git submodule update` checks + #: out: its stage-0 record's, or None for one the index holds + #: conflicted. + self.pinned = next((fields[1].decode("ascii") for fields in + (record.partition(b"\t")[0].split() + for record in self.gitlink) + if fields[2:] == [b"0"]), None) + #: Whether `remove` has a checkout to bring back: one with a `.git` + #: of its own. + self.checkout = unmount and os.path.lexists(self.member / ".git") + #: The object store the checkout's own `.git` named. For the + #: sentence only: the restore asks which store git will use. + self.store: Path | None = None + self.legs: list[tuple[str, str, str | None, str | None]] = [] + self.discards = False + #: Why git could not read the checkout before the first write — its + #: object store gone, most often — or None. READ WITH A GUARD (#365): + #: such a member was removed before the rollback existed, and a read + #: that raised here refused the whole `remove` as a LEG's `git-failed` + #: while its dry run exited 0. Its checkout is in no store this + #: rollback can name, so it is never put back from one. + self.unread: str | None = None + if self.checkout: + try: + self.store = Path(_git_bytes( + ["rev-parse", "--absolute-git-dir"], self.member + ).decode("utf-8", "surrogateescape").strip()) + self.legs = self._legs() + self.discards = bool(_git_bytes( + ["--no-optional-locks", "status", "--porcelain=v1", "-z", + "--ignored"], self.member)) + except CommandFailed as exc: + self.unread = _reason(exc) + self.store, self.legs, self.discards = None, [], False + #: Set once `remove`'s checkout has been brought back, which is when + #: what `deinit -f` discarded is known to be gone. + self.emptied = False + #: Set once the registration is written back: the same keys, the + #: section last in the file. + self.rewritten = False + #: Set once the checkout is found to have no store git could bring it + #: back from, by the restore or by the sentence. + self.storeless = False + #: Set once the sentence has named the store the checkout had as + #: still there (`_store_kept`), which the closing words refer to. + self.store_said = False + #: `{leg: why}` for each leg the restore found no store for. + self.legs_left: dict[str, str] = {} + #: Set once the member's checkout, and then its legs that could be, + #: are back, so the commands typed by hand redo neither. + self.member_back = False + self.legs_back = False + #: Where git keeps the store it checked the member out of, when that + #: is not the store the checkout had (an embedded `.git` that + #: `deinit` moved into the holder's git directory). + self.moved_to: str | None = None + + def _legs(self) -> list[tuple[str, str, str | None, str | None]]: + """`(path, its gitlink's commit, the commit it stands at, its + branch)` of each submodule of the MEMBER's that is checked out in + it: a member is an assembly root, and the holder's own bootstrap + (`git submodule update --init --recursive`) checks its legs out + inside the mount. `git submodule update` brings back one level, so + each comes back by name, and only those that were there: one never + checked out has no store, and `update` would clone it.""" + listed = _git_bytes(["ls-files", "-s", "-z"], self.member) + legs: dict[str, tuple[str, str, str | None, str | None]] = {} + for record in listed.split(b"\0"): + head, _, name = record.partition(b"\t") + leg = name.decode("utf-8", "surrogateescape") + if head.startswith(b"160000 ") and leg not in legs and \ + os.path.lexists(self.member / leg / ".git"): + legs[leg] = (leg, head.split()[1].decode("ascii"), + *_member_head(self.member / leg)) + return list(legs.values()) + + def _registration(self) -> list[tuple[str, str]]: + """Every `submodule.*` key and value in the holder's own git config, + in order. ALL OF THEM, not one name's: `deinit` drops the keys of + the name git finds for the path, which is a name `.gitmodules` gives + and not always the path.""" + listed = git_out(["config", "--local", "--list", "-z"], + cwd=self.root, binary=True) + found = [] + for record in listed.split(b"\0"): + key, _, value = record.decode("utf-8", + "surrogateescape").partition("\n") + if key.startswith("submodule."): + found.append((key, value)) + return found + + def _sections_moved(self, now: list[tuple[str, str]]) -> list[str]: + """The `submodule.` sections whose keys differ between `now` + and the record, in the record's order and then any new one's.""" + def of(listed, section): + return [pair for pair in listed + if pair[0].rpartition(".")[0] == section] + + sections = dict.fromkeys(key.rpartition(".")[0] + for key, _ in self.registration + now) + return [section for section in sections + if of(now, section) != of(self.registration, section)] + + def _undo_entries(self, name: str, entries: list[bytes]) -> bool: + """`name`'s index entries put back as `entries` holds them, the way + `MountRollback` puts them back, but compared on the WHOLE index's + listing, so no call here names the path.""" + if _index_records(self.root).get(name.encode("utf-8"), []) == entries: + return False + run(["git", "update-index", "--force-remove", "--", name], + cwd=self.root) + if entries: + _git_input(["update-index", "-z", "--index-info"], self.root, + b"".join(entry + b"\0" for entry in entries)) + return True + + def _undo_registration(self) -> str | None: + now = self._registration() + moved = self._sections_moved(now) + for section in moved: + for key in dict.fromkeys(key for key, _ in now + if key.rpartition(".")[0] == section): + run(["git", "config", "--local", "--unset-all", key], + cwd=self.root) + for key, value in self.registration: + if key.rpartition(".")[0] == section: + run(["git", "config", "--local", "--add", key, value], + cwd=self.root) + if not moved: + return None + self.rewritten = True + return (" and ".join(f"`{section}`" for section in moved) + + " restored in the holder's git config") + + def _storeless(self) -> str | None: + """Why the member's checkout cannot come back from an object store + git has, in the sentence's words; None when it can (#365).""" + if self.unread is not None: + return ("git could not read its checkout before this command " + f"({self.unread}), so no object store here is known to " + "hold it") + why = _unrestorable(self.root, self.path, self.pinned) + return (None if why is None else + why + ", so `git submodule update` was not run") + + def _store_kept(self) -> str: + """`; the object store its checkout had, , is still there`, where + that store is there and is not the one git looks in: what a human + who renamed the section by hand needs to know to bring it back.""" + found = _store_git_uses(self.root, self.path) + if self.store is None or not self.store.is_dir() or ( + found is not None + and found[1].resolve() == self.store.resolve()): + return "" + self.store_said = True + return (f"; the object store its checkout had, " + f"{self.store.as_posix()}, is still there") + + def _made_again(self) -> bool: + """The member's directory made again, empty, if it was there before + and is not now: a gitlink with no directory reads as deleted, which a + later `git commit -a` would record, and an empty one is how a member + never checked out stands.""" + if self.existed and not self.member.is_dir(): + self.member.mkdir(parents=True) + return True + return False + + def _undo_work_tree(self) -> str | None: + if not self.checkout: + return (f"{self.path} made again, empty, as it was" + if self._made_again() else None) + if os.path.lexists(self.member / ".git") and \ + _member_head(self.member) == (self.at, self.branch): + return None + # THE STORE GIT WILL USE IS ASKED FIRST, and `update` is not run + # without it: it would clone (#365). + why = self._storeless() + if why is not None: + self.storeless = True + if self._made_again(): + why += f", and {self.path} was made again, empty" + raise Refusal("member-store-gone", why + self._store_kept()) + found = _store_git_uses(self.root, self.path) + _offline_git(["--literal-pathspecs", "submodule", "update", "--init", + "--no-fetch", "-q", "--", self.path], self.root) + self.emptied = True + if found is not None and self.store is not None and \ + found[1].resolve() != self.store.resolve(): + self.moved_to = found[0] + _head_back(self.member, self.at, self.branch) + self.member_back = True + ready = [] + for leg in self.legs: + try: + left = _unrestorable(self.member, leg[0], leg[1]) + except (CommandFailed, Refusal) as exc: + left = _reason(exc) + if left is None: + ready.append(leg) + else: + self.legs_left[leg[0]] = left + if ready: + _offline_git(["--literal-pathspecs", "submodule", "update", + "--init", "--no-fetch", "-q", "--", + *[leg for leg, _, _, _ in ready]], self.member) + for leg, _, at, branch in ready: + _head_back(self.member / leg, at, branch) + self.legs_back = True + if self.legs_left: + raise Refusal( + "member-store-gone", + f"{self.path} is checked out again, but " + + "; and ".join(f"not {leg} in it: {why}, so `git submodule " + "update` was not run for it" + for leg, why in self.legs_left.items())) + return self._work_tree_said() + + def _head_said(self) -> str: + at = (self.at or "")[:12] + if self.branch is not None: + return f"{self.path} back on {self.branch} at {at}" + return f"{self.path} checked out at {at} again" + + def _work_tree_said(self) -> str: + if not self.checkout: + return f"{self.path} made again, empty, as it was" + if self.at is None: + return f"{self.path} checked out again" + said = (f"{self.path} checked out again at {self.at[:12]}" + + (f" on {self.branch}" if self.branch else "") + + " from the object store `git rm` keeps") + if self.legs: + said += (", and " + " and ".join(leg for leg, _, _, _ in self.legs) + + " in it") + return said + + def _file_by_hand(self, name: str) -> list[str]: + """The commands that put `name` back: its index entries, then its + bytes from them where they are what the index held. Bytes git does + not have — an edit nobody had staged — have no command, and the + sentence's `git status` shows them.""" + data, entries = self.files[name] + typed = [_entries_by_hand(name, entries)] + if self._bytes(name) == data: + return typed + if data is None: + return typed + [f"rm -f -- {shlex.quote(name)}"] + staged = [entry.partition(b"\t")[0].split()[1].decode("ascii") + for entry in entries if entry.partition(b"\t")[0] + .endswith(b" 0")] + try: + blob = _git_bytes(["hash-object", "--stdin", f"--path={name}"], + self.root, data).decode("ascii").strip() + except CommandFailed: + blob = None + if staged and blob == staged[0]: + typed.append(f"git checkout -- {shlex.quote(name)}") + return typed + + def _registration_by_hand(self) -> list[str]: + try: + now = self._registration() + except Refusal: + now = [] + typed = [] + for section in self._sections_moved(now): + typed += [f"git config --local --unset-all {shlex.quote(key)}" + for key in dict.fromkeys( + key for key, _ in now + if key.rpartition(".")[0] == section)] + typed += [f"git config --local --add {shlex.quote(key)} " + f"{shlex.quote(value)}" + for key, value in self.registration + if key.rpartition(".")[0] == section] + return typed + + def _work_tree_by_hand(self) -> list[str]: + """The commands that bring the checkout back by hand, and NONE THAT + COULD CLONE OR FETCH (#365): `git submodule update` only where git + holds the store and the commit now, and under the same backstop the + rollback runs it with. Where git does not, the directory is made + again, empty, and the sentence says where the checkout comes back + from (`undo`). Nothing already back is typed again.""" + made = ([f"mkdir {shlex.quote(self.path)}"] + if self.existed and not self.member.is_dir() else []) + if not self.checkout: + return made + try: + storeless = self._storeless() is not None + except (CommandFailed, Refusal): + storeless = True + if storeless: + self.storeless = True + return made + offline = " ".join(NO_REMOTE) + typed = [] + if not self.member_back: + typed.append(f"git {offline} submodule update --init --no-fetch " + "-- " + shlex.quote(self.path)) + if self.at is not None and (self.branch is not None + or self.pinned != self.at): + typed.append(_head_by_hand(self.path, self.at, self.branch)) + legs = [] if self.legs_back else [ + leg for leg in self.legs if leg[0] not in self.legs_left] + if legs: + typed.append(f"git -C {shlex.quote(self.path)} {offline} " + "submodule update --init --no-fetch -- " + " ".join( + shlex.quote(leg) for leg, _, _, _ in legs)) + typed += [_head_by_hand(f"{self.path}/{leg}", at, branch) + for leg, _, at, branch in legs + if at is not None and branch is not None] + return typed + + def _config_left(self, missing: list[str], typed: list[str]) -> list[str]: + """What a checkout that did not come back left in the holder's git + config, as a `NOT` clause, with the commands that take it out added + to `typed`; [] when it left nothing. + + `git submodule update --init` registers the name it reads before it + checks anything out, so a restore that failed after that, or ran + against a store that was not the member's, leaves `submodule..*` + keys the registration step had already compared and passed (the + review's probe left `submodule.atlas.url` and `.active`). Asked only + once the registration was put back and the checkout was not.""" + if "work tree" not in missing or "registration" in missing: + return [] + try: + now = self._registration() + except Refusal: + return [] + moved = self._sections_moved(now) + if not moved: + return [] + new = list(dict.fromkeys(key for key, value in now + if key.rpartition(".")[0] in moved + and (key, value) not in self.registration)) + gone = list(dict.fromkeys(key for key, value in self.registration + if key.rpartition(".")[0] in moved + and (key, value) not in now)) + typed += self._registration_by_hand() + said = [] + if new: + said.append("it holds " + " and ".join(f"`{key}`" for key in new) + + ", which it did not") + if gone: + said.append("it lost " + " and ".join(f"`{key}`" for key in gone)) + return ["the holder's git config as it was: " + ", and ".join(said)] + + def _steps(self) -> list[tuple]: + """`(key, what it puts back, how, the keys it needs back first, the + commands that do it by hand)` for each step, in the order `undo` + takes them: the index and the files, then what lies outside them.""" + steps = [("gitlink", + f"the gitlink at {self.path} put back in the index as it " + "was", + lambda: self._undo_entries(self.path, self.gitlink), (), + lambda: [_entries_by_hand(self.path, self.gitlink)])] + for name, (data, entries) in self.files.items(): + steps.append(( + name, + (f"{name} restored byte for byte" if data is not None else + f"{name} removed, as there was none before"), + lambda name=name, data=data, entries=entries: + self._undo_file(name, data, entries), + (), lambda name=name: self._file_by_hand(name))) + if not self.unmount: + steps.append(( + "member", self._head_said(), + lambda: _head_back(self.member, self.at, self.branch), (), + lambda: [_head_by_hand(self.path, self.at, self.branch)])) + return steps + steps.append(("registration", + "the holder's `submodule.*` keys put back in its git " + "config", self._undo_registration, (), + self._registration_by_hand)) + steps.append(("work tree", self._work_tree_said(), + self._undo_work_tree, + ("gitlink", GITMODULES, "registration"), + self._work_tree_by_hand)) + return steps + + def _config_moved(self) -> str: + """What the holder's `submodule.*` keys hold now that they did not, + and what they have lost, as `_index_moved` names the index's; "" + when they are the recorded keys and values, wherever in the file.""" + now = self._registration() + if sorted(now) == sorted(self.registration): + return "" + parts = [] + for said, pairs in (("new in it", [pair for pair in now + if pair not in self.registration]), + ("gone from it", [pair for pair in + self.registration + if pair not in now])): + if pairs: + parts.append(f"{said}: " + ", ".join( + f"`{key}={value}`" for key, value in pairs[:5])) + return "the holder's git config " + "; ".join(parts) + + def undo(self) -> str: + """Put the holder back. Returns the sentence a refusal ends with: + what was put back, or what is left and the commands that put it + back, and, where the index (or `remove`'s registration) is not the + one recorded, what differs.""" + now = self._head() + if now is not None and now != self.head: + return (" The commit had already landed, so nothing was undone: " + "`git log -1` in the holder shows it.") + named = {"gitlink": f"the gitlink at {self.path}", + GITMODULES: GITMODULES, "registration": "the registration"} + restored, failed, typed, missing = [], [], [], [] + for key, said, action, needs, by_hand in self._steps(): + lacking = [named[need] for need in needs if need in missing] + if lacking: + missing.append(key) + failed.append(f"{said}: not tried, as it needs " + + " and ".join(lacking) + " back first") + typed += by_hand() + continue + try: + done = action() + except _EntryNotRestored as exc: + missing.append(key) + if exc.bytes_restored: + restored.append(said) + failed.append(f"the index entry of {exc.name} put back as " + f"it was: {_reason(exc.cause)}") + typed.append(_entries_by_hand(exc.name, + self.files[exc.name][1])) + continue + except (CommandFailed, Refusal, OSError) as exc: + missing.append(key) + failed.append(f"{said}: {_reason(exc)}") + typed += by_hand() + continue + if done: + restored.append(done if isinstance(done, str) else said) + noun = "bump" if self.verb == "bump" else "unmount" + if failed: + if self.unmount: + failed += self._config_left(missing, typed) + text = (f" The {noun} was undone only in part" + + (" (" + ", ".join(restored) + ")" if restored else "") + + "; NOT " + "; NOT ".join(failed) + ".") + # NO COMMAND HERE CLONES (#365): a checkout no store git has can + # bring back is said to come back only from its remote, and + # one whose old store the sentence named as still there, that + # it can come back from that store with no network as well + # (review 365-7cca975, NIT-B), with no command to do it. + kept = self.storeless and self.store_said + remote_only = ([self.path] if self.storeless and not kept + else []) + [f"{self.path}/{leg}" + for leg in self.legs_left] + if kept: + text += (f" The checkout of {self.path} can come back from " + "the object store named above with no network, or " + "from its remote; this command did neither.") + if remote_only: + text += (" The checkout of " + " and ".join(remote_only) + + " comes back only from " + + ("its remote" if len(remote_only) == 1 else + "their remotes") + + ", which this command never reaches.") + if typed: + return (text + " Put the rest back by hand, in the holder and " + "in this order: " + "; ".join(f"`{line}`" + for line in typed) + + f". Then `git status` shows whether anything is " + f"left, before running `{self.verb}` again.") + if remote_only or kept: + return (text + " `git status` shows whether anything else is " + f"left, before running `{self.verb}` again.") + return (text + " Put those back by hand — `git status` shows what " + f"is left — before running `{self.verb}` again.") + moved, unread = "", None + try: + moved = "; ".join(part for part in ( + self._index_moved(), + self._config_moved() if self.unmount else "") if part) + except Refusal as exc: + unread = _reason(exc) + undone = ((f" Nothing was committed: the {noun} has been undone — " + + ", ".join(restored) + " —") if restored else + " Nothing was committed,") + if unread is not None: + return (undone + " but the holder could not be read back to " + f"confirm it is as it was before this command: {unread}. " + f"Check it before running `{self.verb}` again: `git " + "ls-files -s` lists what the index now holds.") + if moved: + return (undone + " but the holder is NOT as it was before this " + f"command ({moved}). Put it back before running " + f"`{self.verb}` again: `git ls-files -s` lists what the " + "index now holds.") + did = "bumped" if self.verb == "bump" else "removed" + if not restored: + return (f" Nothing was {did}, and nothing in the holder had " + "changed.") + # "EXACTLY" ONLY WHEN NOTHING BELOW QUALIFIES IT: a registration + # written back is the same keys in another place in the file, a + # checkout git keeps elsewhere now is not byte for byte the one it + # had, and what `deinit -f` discarded is in no store. + discarded = self.emptied and self.discards + held = ("as it was before this command" + if discarded or self.rewritten or self.moved_to else + "exactly as it was before this command") + if discarded: + held += (" but for what `git submodule deinit -f` discarded in " + f"{self.path}, which no object store holds: the " + "uncommitted changes and the untracked and ignored " + "files `git status --ignored` listed there") + text = (f" Nothing was {did}: the {noun} has been undone — " + + ", ".join(restored) + f" — and the holder is {held}.") + if self.rewritten: + text += (" Its git config holds the same keys and values as " + "before, the section put back written last.") + if self.moved_to is not None and self.store is not None: + text += (f" {self.path} now has its object store at " + f"{self.moved_to}, where git keeps it, and no longer at " + f"{self.store.as_posix()}.") + return text + + # --------------------------------------------------------------------------- # bump # --------------------------------------------------------------------------- @@ -2479,60 +3342,17 @@ def _bump_dry_run(family: Family, row: dict, path: str, commit: str, return 0 -def cmd_bump(args) -> int: - """Move one member's gitlink and pin in ONE commit, or with `--dry-run` - print the row it would write and change nothing in the holder (#326). - - The refusals before the fetch are the same under `--dry-run`. The dry run - leaves after the fetch and before the checkout, which is the real run's - first write, and asks what the real run asks after it of the member's - objects instead (`_bump_dry_run`). - """ - family = Family.open(args.family_root) - project = checked_value("--member", repo_basename(args.member)) - row = family.member(project) - if row is None: - raise Refusal( - "member-unknown", - f"{project} is not a member of {family.data.get('name')}", - "Remediation: `family.py add --member /" + project - + "` puts it in the family first.") +def _bump_and_pin(family: Family, row: dict, path: str, commit: str, + trailers: list[str], rollback: PinRollback) -> int: + """The part of `bump` that writes: the checkout, the pin and the ONE + commit, or `nothing to do`. Returns the exit code. Runs inside the + caller's `PinRollback` (#365), which puts back whatever a failure here + leaves; `nothing to do` is no failure, and is left as it always was.""" project = str(row["project"]) - commit = checked_value("--to", args.to).lower() - if not COMMIT_RE.match(commit): - raise Refusal( - "member-to-not-a-commit", - f"--to {args.to!r} is not a 40-hex commit", - "Remediation: pass the full 40 characters. A tag can be moved and " - "a commit cannot; a pin is a commit or it is nothing.") - # THE ROW'S `path:` IS CHECKED, NOT TRUSTED (#299): `member` below, the - # fetch and checkout in it, and every pathspec after them are this path. - path = _member_path(project, row) member = family.root / path - if not (member / ".git").exists(): - raise Refusal( - "member-uninitialized", - f"{path} is not an initialized submodule checkout, so the new " - "commit cannot be resolved or digested", - "Remediation: `git submodule update --init " + path + "`, or " - "`make bootstrap`, which resolves a credential for a private " - "member first.") - fetched = True - try: - run(["git", "fetch", "-q", "origin"], cwd=member) - except CommandFailed: - fetched = False - print(f" note: could not fetch {path} from its origin; the commit " - "must already be in its object store", file=sys.stderr) - if args.dry_run: - return _bump_dry_run(family, row, path, commit, args.trailer, - fetched) - try: - run(["git", "checkout", "-q", commit], cwd=member) - except CommandFailed as exc: - print(exc.loudly(f"checking {path} out at {commit[:12]}"), - file=sys.stderr) - return 2 + rollback.step = f"checking {path} out at {commit[:12]}" + run(["git", "checkout", "-q", commit], cwd=member) + rollback.step = "a git or gh command failed" was = str((row.get("pin") or {}).get("commit") or "") project_id = _read_member_project(member, project, @@ -2540,7 +3360,7 @@ def cmd_bump(args) -> int: digest = tree_digest(member, commit) _, rows = _bumped_rows(family, row, project_id, commit, digest) original = family.path.read_bytes() - family.write_members(rows) + _rewrite_members(family, rows, f"with {project}'s new pin") # THE GITLINK IS RE-STAGED BEFORE GIT IS ASKED, as `_commit_one` does it # first anyway. An index still holding a stray gitlink somebody staged and # never committed makes `git diff HEAD` find a difference the commit then @@ -2575,12 +3395,125 @@ def cmd_bump(args) -> int: f"`members[].pin` in {MANIFEST} move in ONE commit, which is what " "`validate-family.py` checks and what a reviewer reads as one " "change.\n", - [path, MANIFEST], trailers=args.trailer) + [path, MANIFEST], trailers=trailers) print(f" {project:<16} {was[:12]} -> {commit[:12]} tree {digest[:12]}…") print(f" committed {head[:12]}: {path}, {MANIFEST}") return 0 +def _checked_bump_target(family: Family, row: dict, path: str, + commit: str) -> None: + """Every refusal the real `bump` can ask before its checkout, asked as + `bump --dry-run` asks it and in its order (#365): the member's + `project.yaml` at `--to`, read from its objects (`_member_id_at`), and + a `members:` block this tool will not rewrite. + + THE REAL RUN ASKED BOTH AFTER ITS FIRST WRITE. A `--to` whose tree has no + `project.yaml` was refused `member-not-a-project` with the member left + checked out at `--to`, which the holder's own validator then reported, + while `bump --dry-run` refused the same commit and changed nothing. Asked + here, before the rollback is even recorded, the two runs refuse at the + same point in the same words, and the holder is byte for byte as it was. + A `--to` the member's objects do not hold is left to the checkout, which + fails on it in git's words, as it always has, before it writes anything; + a symbolic link is left to the read through the checkout, as the dry run + leaves it. + """ + member = family.root / path + try: + resolved = run(["git", "rev-parse", "-q", "--verify", + f"{commit}^{{commit}}"], cwd=member).lower() + except CommandFailed: + return + project_id = _member_id_at(member, resolved, str(row["project"]), + str(row.get("repository"))) + if project_id is None: + project_id = str(row.get("id") or "") + # THE ROWS THE BUMP WRITES, not the rows the manifest has: a row with no + # `path:` line is the member's own path by default (`_member_path`), and + # only the row `_bumped_rows` makes for it spells one out to rewrite. + _, rows = _bumped_rows(family, row, project_id, commit, + tree_digest(member, commit)) + family.rewritten(rows) + + +def _put_back_on_failure(rollback: PinRollback, write) -> int: + """`write()`, inside `rollback`, as `cmd_add` runs its mount (#365): a + refusal ends with the sentence `undo` returns; a failed git command is + quoted as it ran, headed as it always was, with that sentence under it, + exit 2; anything else is put back and goes on exactly as raised, so + Ctrl-C still stops the program and a bug still shows its traceback.""" + try: + return write() + except Refusal as exc: + exc.detail = _ended(exc.detail) + rollback.undo() + raise + except CommandFailed as exc: + undone = rollback.undo() + print(exc.loudly(rollback.step), file=sys.stderr) + print(undone.strip(), file=sys.stderr) + return 2 + except BaseException: + print(rollback.undo().strip(), file=sys.stderr) + raise + + +def cmd_bump(args) -> int: + """Move one member's gitlink and pin in ONE commit, or with `--dry-run` + print the row it would write and change nothing in the holder (#326). + + The refusals before the fetch are the same under `--dry-run`. The dry run + leaves after the fetch and before the checkout, which is the real run's + first write, and asks of the member's objects what the real run asks + before that checkout too (`_bump_dry_run`, `_checked_bump_target`). The + real run then records the holder and puts it back if anything after the + checkout fails (`PinRollback`, #365). + """ + family = Family.open(args.family_root) + project = checked_value("--member", repo_basename(args.member)) + row = family.member(project) + if row is None: + raise Refusal( + "member-unknown", + f"{project} is not a member of {family.data.get('name')}", + "Remediation: `family.py add --member /" + project + + "` puts it in the family first.") + project = str(row["project"]) + commit = checked_value("--to", args.to).lower() + if not COMMIT_RE.match(commit): + raise Refusal( + "member-to-not-a-commit", + f"--to {args.to!r} is not a 40-hex commit", + "Remediation: pass the full 40 characters. A tag can be moved and " + "a commit cannot; a pin is a commit or it is nothing.") + # THE ROW'S `path:` IS CHECKED, NOT TRUSTED (#299): `member` below, the + # fetch and checkout in it, and every pathspec after them are this path. + path = _member_path(project, row) + member = family.root / path + if not (member / ".git").exists(): + raise Refusal( + "member-uninitialized", + f"{path} is not an initialized submodule checkout, so the new " + "commit cannot be resolved or digested", + "Remediation: `git submodule update --init " + path + "`, or " + "`make bootstrap`, which resolves a credential for a private " + "member first.") + fetched = True + try: + run(["git", "fetch", "-q", "origin"], cwd=member) + except CommandFailed: + fetched = False + print(f" note: could not fetch {path} from its origin; the commit " + "must already be in its object store", file=sys.stderr) + if args.dry_run: + return _bump_dry_run(family, row, path, commit, args.trailer, + fetched) + _checked_bump_target(family, row, path, commit) + rollback = PinRollback(family.root, path, "bump", (MANIFEST,)) + return _put_back_on_failure(rollback, lambda: _bump_and_pin( + family, row, path, commit, args.trailer, rollback)) + + # --------------------------------------------------------------------------- # remove # --------------------------------------------------------------------------- @@ -2657,19 +3590,190 @@ def _registered(root: Path, name: str) -> bool: for record in listed.split(b"\0") if record) +def _sections_giving(root: Path, path: str + ) -> list[tuple[str, str | None, tuple[str, ...]]]: + """`(name, url, keys)` of every submodule the work tree's `.gitmodules` + gives `path` to, in ANY of its `path` lines, in the order the file first + names them (#360). `url` is the last the name gives, or None; `keys` is + every key it holds, once each, in the order git lists them. + + ANY `path` LINE, NOT ONLY THE LAST. `git rm` takes out the sections of + ONE name, the one git's path lookup returns (`_gitmodules_names`), and + every other section that says this path stays in the file `remove` + commits, whatever git makes of its name: a section of a name that gives + the path and then moves on took the path out of git's lookup altogether, + so `git rm` took out nothing and the member's own section stayed. Read + where `git rm` reads, the work tree's file, by `git config --list`, as + `_gitmodules_names` reads it. + """ + if not (root / GITMODULES).is_file(): + return [] + listed = _git_bytes(["config", "-z", "--file", GITMODULES, "--list"], + root) + found: dict[str, dict[str, str]] = {} + giving: list[str] = [] + for record in listed.split(b"\0"): + key, _, value = record.decode("utf-8", + "surrogateescape").partition("\n") + section, _, variable = key.rpartition(".") + if not section.startswith("submodule."): + continue + name = section[len("submodule."):] + found.setdefault(name, {})[variable] = value + if variable == "path" and value == path and name not in giving: + giving.append(name) + return [(name, found[name].get("url"), tuple(found[name])) + for name in giving] + + +def _checked_one_section(root: Path, project: str, path: str) -> None: + """`member-path-registered-twice`: `remove` refuses, before any git + command that writes and under `--dry-run` alike, a `.gitmodules` that + gives the member's path to more than one submodule (#360). + + git maps a path to ONE submodule name, the last section in the file to + give it, so `git submodule deinit` deinitialised that name and `git rm` + took out its sections, and the other stayed: in the commit `remove` + wrote, registering `members/

` with no gitlink and no row, while it + printed `removed` and exited 0. When the other was written FIRST, the + member's own section was the one left, and with it the registration + `deinit` never touched, which a later `add` of the member then wrote + into. `validate-family.py` passed either result: after `remove` the path + has one section, not two. + + REFUSED, NOT CLEANED, as `add` refuses the same file (#317, + `member-path-registered`). Which section is the member's is a person's + to say, and one somebody wrote is not this tool's to drop or rewrite; + so each is named with its url, and #317's by-key removal is printed for + every section not named after the path, `members/

`, the name `family.py + add` gives a member's own. With the others gone, `remove` runs as it + always has. The holder is left byte for byte as it was. + """ + sections = _sections_giving(root, path) + if len(sections) < 2: + return + named = ", ".join( + f"`{name}` ({'no url' if url is None else 'url ' + url})" + for name, url, _ in sections) + others = [(name, keys) for name, _, keys in sections if name != path] + removal = " and ".join( + f"`git config -f {GITMODULES} --unset-all " + f"{shlex.quote('submodule.' + name + '.' + key)}`" + for name, keys in others for key in keys) + which = (f"If `{path}`, the name `family.py add` gives a member's own " + f"section, is {project}'s, take each other one out by hand" + if len(others) < len(sections) else + f"None of them has the name `family.py add` gives a member's " + f"own section, `{path}`, so the commands that follow take out " + f"every one: leave out those of {project}'s own and run the " + "rest") + raise Refusal( + "member-path-registered-twice", + f"{GITMODULES} gives {project}'s path {path} to {len(sections)} " + f"submodules, {named}. git finds ONE of them for a path, and `git " + "rm` takes out only that one's sections, so this command's commit " + + ("would keep the other" if len(sections) == 2 else + "would keep the others") + + f" registering {path} with no gitlink and no row behind it. No git " + "command that writes was run and nothing was written.", + f"Remediation: which of them is {project}'s is a person's to say, " + "and a section somebody wrote is not this tool's to drop or " + f"rewrite. {which}, in a POSIX shell — {removal}, then `git add " + f"{GITMODULES}` — and run the same command again.") + + +def _checked_index_unlocked(root: Path, path: str) -> None: + """`family-index-locked`: the real `remove` refuses a held `index.lock` + BEFORE it unmounts anything (#365). + + `git submodule deinit` takes no index lock, so with one held — another + git command running in the holder, or one that stopped without letting + go — `deinit -f` emptied the member's checkout and dropped its + registration, and `git rm`, the first step that writes the index, then + died on the lock. The lock is where git says it is (`git rev-parse + --git-path index.lock`), which in a linked worktree is not + `.git/index.lock`. A lock taken after this check is the rollback's to + meet. The dry run does not ask: it writes nothing, and a lock is a fact + about the moment. + """ + shown = _git_bytes(["rev-parse", "--git-path", "index.lock"], + root).decode("utf-8", "surrogateescape").strip() + lock = root / shown + if not os.path.lexists(lock): + return + raise Refusal( + "family-index-locked", + f"the holder's index is locked: {lock.as_posix()} exists. `git " + f"submodule deinit` takes no such lock, so `remove` would empty " + f"{path} and drop its registration and then fail at `git rm`, its " + "first write to the index; it was refused before either, and " + "nothing was written.", + "Remediation: another git command is running in the holder, or one " + "that stopped left the lock behind. Let it finish and run the same " + "command again; if no git command is running, delete the lock (`rm " + f"-f {shlex.quote(lock.as_posix())}`) and run the same command " + "again.") + + +def _unmount_and_drop(family: Family, row: dict, path: str) -> int: + """The part of `remove` that writes: the unmount, the row and the ONE + commit. Returns the exit code. Runs inside the caller's `PinRollback` + (#365), which puts back whatever a failure here leaves.""" + project = str(row["project"]) + # THE MEMBER REPOSITORY IS NOT TOUCHED. Removing a member unmounts it from + # this holder and nothing else: the project keeps its name, its history, + # its legs and its gate. A family that could delete a project by dropping + # a row would be a family that confers something. + # + # BOTH CALLS TAKE THE PATH LITERALLY (`--literal-pathspecs`, #299), the + # global option, which `git submodule` honours too: the path is always + # `members/` here, so this is defence in depth that does not + # depend on the row being right. Without it a glob in the path would + # unmount and remove every member it matched. + run(["git", "--literal-pathspecs", "submodule", "deinit", "-f", "-q", + "--", path], cwd=family.root) + run(["git", "--literal-pathspecs", "rm", "-r", "-q", "-f", "--", path], + cwd=family.root) + _rewrite_members(family, [r for r in family.members + if str(r.get("project")) != project], + f"without {project}'s row") + head = _commit_one( + family.root, + f"Remove {project} from the {family.data.get('name')} family\n\n" + f"Unmounted from {path} and dropped from {MANIFEST}. " + f"{row.get('repository')} itself is untouched: it keeps its name, its " + "history, its legs and its gate. Membership was navigation and " + "conferred nothing, so losing it takes nothing away.\n", + [GITMODULES, path, MANIFEST], add=[GITMODULES, MANIFEST]) + print(f" removed {project} ({row.get('repository')}) from {path}") + print(f" committed {head[:12]}: .gitmodules, {path}, {MANIFEST}") + # POSIX for the same reason it is POSIX in `add`: git spells this path + # `.git/modules/`, and so does the human who deletes it. + cached = family.root / ".git" / "modules" / path + if cached.exists(): + print(f" NOTE {cached.as_posix()} still holds that member's object " + "store. `git rm` leaves it on purpose, in case somebody " + "committed inside the mount; delete it (`rm -rf`) before adding " + "this member back.") + return 0 + + def _remove_dry_run(family: Family, row: dict, path: str) -> int: """`remove --dry-run` (#326): what a real `remove` would take out, and no git command that writes. - THE REAL RUN'S FAILURES, IN ITS ORDER. Its first git call, `git - submodule deinit -- `, fails on a path the index holds nothing at, - and the real run prints that failure loudly, the exact command and git's - own `did not match any file(s) known to git`, exit 2. `ls-files - --error-unmatch` over the same LITERAL pathspec fails where it fails, - with the same line from git, and is printed the same way, exit 2; it - lists what `git rm -r` would unstage when it does not fail. - `family.rewritten` then asks `family-members-unrecognised`, which the - real run asks after its unmount, without writing. + THE REAL RUN'S FAILURES, IN ITS ORDER. `family.rewritten` asks + `family-members-unrecognised` first, without writing, because the real + run asks it before its unmount (#365): asked after the listing below, a + gitlink the index does not hold beside a `members:` line this tool will + not rewrite got git's loud failure here and the refusal there. The real + run's first git call that writes, `git submodule deinit -- `, + then fails on a path the index holds nothing at, and the real run prints + that failure loudly, the exact command and git's own `did not match any + file(s) known to git`, exit 2. `ls-files --error-unmatch` over the same + LITERAL pathspec fails where it fails, with the same line from git, and + is printed the same way, exit 2; it lists what `git rm -r` would unstage + when it does not fail. PRINTED: the row, each index entry under the path (the gitlink), the `.gitmodules` section `git rm` drops for it and any it leaves that gives @@ -2681,6 +3785,8 @@ def _remove_dry_run(family: Family, row: dict, path: str) -> int: """ project = str(row["project"]) root = family.root + family.rewritten([r for r in family.members + if str(r.get("project")) != project]) try: listed = _git_bytes(["--literal-pathspecs", "ls-files", "-s", "-z", "--error-unmatch", "--", path], root) @@ -2694,8 +3800,6 @@ def _remove_dry_run(family: Family, row: dict, path: str) -> int: head, _, name = record.partition(b"\t") mode, oid = head.decode("ascii").split()[:2] entries.append((mode, oid, name.decode("utf-8", "replace"))) - family.rewritten([r for r in family.members - if str(r.get("project")) != project]) pinned = str((row.get("pin") or {}).get("commit") or "") print(f"row {project} {row.get('repository')}, pinned at " @@ -2746,45 +3850,27 @@ def cmd_remove(args) -> int: # call: `path: members/*` unmounted every member under one commit naming # one, and `path: .` removed the holder's own files. path = _member_path(project, row) + # A `.gitmodules` that gives the path to a second submodule is refused + # here, before any git command that writes (#360), so the dry run makes + # the same refusal, as it makes every one above. + _checked_one_section(family.root, project, path) # Every refusal above is the same under `--dry-run`, which leaves here, # before the unmount, the real run's first write (#326). if args.dry_run: return _remove_dry_run(family, row, path) - # THE MEMBER REPOSITORY IS NOT TOUCHED. Removing a member unmounts it from - # this holder and nothing else: the project keeps its name, its history, - # its legs and its gate. A family that could delete a project by dropping - # a row would be a family that confers something. - # - # BOTH CALLS TAKE THE PATH LITERALLY (`--literal-pathspecs`, #299), the - # global option, which `git submodule` honours too: the path is always - # `members/` here, so this is defence in depth that does not - # depend on the row being right. Without it a glob in the path would - # unmount and remove every member it matched. - run(["git", "--literal-pathspecs", "submodule", "deinit", "-f", "-q", - "--", path], cwd=family.root) - run(["git", "--literal-pathspecs", "rm", "-r", "-q", "-f", "--", path], - cwd=family.root) - family.write_members([r for r in family.members - if str(r.get("project")) != project]) - head = _commit_one( - family.root, - f"Remove {project} from the {family.data.get('name')} family\n\n" - f"Unmounted from {path} and dropped from {MANIFEST}. " - f"{row.get('repository')} itself is untouched: it keeps its name, its " - "history, its legs and its gate. Membership was navigation and " - "conferred nothing, so losing it takes nothing away.\n", - [GITMODULES, path, MANIFEST], add=[GITMODULES, MANIFEST]) - print(f" removed {project} ({row.get('repository')}) from {path}") - print(f" committed {head[:12]}: .gitmodules, {path}, {MANIFEST}") - # POSIX for the same reason it is POSIX in `add`: git spells this path - # `.git/modules/`, and so does the human who deletes it. - cached = family.root / ".git" / "modules" / path - if cached.exists(): - print(f" NOTE {cached.as_posix()} still holds that member's object " - "store. `git rm` leaves it on purpose, in case somebody " - "committed inside the mount; delete it (`rm -rf`) before adding " - "this member back.") - return 0 + # WHAT CAN BE ASKED BEFORE THE UNMOUNT IS (#365); what fails after it is + # put back (`PinRollback`). A `members:` block this tool will not rewrite + # was refused after `deinit` and `git rm`, with the member unmounted and + # `.gitmodules` changed, where the dry run refused it first; it is asked + # here, as the dry run asks it, so the two refuse in the same words with + # the holder as it was. Then the lock, which only the real run meets. + family.rewritten([r for r in family.members + if str(r.get("project")) != project]) + _checked_index_unlocked(family.root, path) + rollback = PinRollback(family.root, path, "remove", + (GITMODULES, MANIFEST), unmount=True) + return _put_back_on_failure( + rollback, lambda: _unmount_and_drop(family, row, path)) # --------------------------------------------------------------------------- diff --git a/scripts/repo_shape.py b/scripts/repo_shape.py index e32c504f..67b66a78 100644 --- a/scripts/repo_shape.py +++ b/scripts/repo_shape.py @@ -868,9 +868,18 @@ def load_yaml(path: Path) -> Any: # --------------------------------------------------------------------------- -def git_out(args: list[str], cwd: Path, binary: bool = False) -> Any: +def git_out(args: list[str], cwd: Path, binary: bool = False, + env: dict[str, str] | None = None) -> Any: + """`git ` in `cwd`, its stdout, or a `git-failed` refusal. + + `env` is the whole environment git runs in, None for this process's + own. A git that cannot be STARTED (none on PATH) raises `OSError` from + here, never a refusal: whether that is a question the caller cannot + answer is the caller's to say (#368). + """ proc = subprocess.run( - ["git", *args], cwd=str(cwd), capture_output=True, check=False + ["git", *args], cwd=str(cwd), capture_output=True, check=False, + env=env, ) if proc.returncode != 0: raise Refusal( @@ -1030,11 +1039,29 @@ def file_sha256(path: Path) -> str: #: ` SP SP TAB ` and `ls-tree` emits #: ` SP SP TAB ` — the same shape with a DIFFERENT #: third column, so the two cannot share one index. +#: +#: BOTH LISTINGS TAKE THE PATH LITERALLY (#367), with the global +#: `--literal-pathspecs`. A pathspec is a glob to `ls-files`, so a row whose +#: hand-edited `path:` was `members/*` read the first member's gitlink and +#: answered with it, a neighbour's commit, for every caller. Not +#: `:(literal)` magic on the path, which a caller's own exported +#: `GIT_LITERAL_PATHSPECS=1` switches off, and then it matches nothing. GITLINK_LISTINGS = ( - (["ls-files", "-s", "--"], 1, 2), - (["ls-tree", "HEAD", "--"], 2, None), + (["--literal-pathspecs", "ls-files", "-s", "--"], 1, 2), + (["--literal-pathspecs", "ls-tree", "HEAD", "--"], 2, None), ) +#: The two pathspec settings a caller's environment can carry that git +#: refuses BESIDE `--literal-pathspecs` ("global 'literal' pathspec setting +#: is incompatible with all other global pathspec settings", exit 128). Both +#: listings would then fail, and `recorded_gitlink` reads a failed listing as +#: "no gitlink here", so an operator who exports either would see every gate +#: refuse a gitlink that is there. They are left out of those two git calls +#: alone. `GIT_NOGLOB_PATHSPECS` stays: git takes it beside the literal +#: setting, and with it the path is literal anyway. +PATHSPEC_ENV_REFUSED_BESIDE_LITERAL = ("GIT_GLOB_PATHSPECS", + "GIT_ICASE_PATHSPECS") + def recorded_gitlink(repo: Path, path: str) -> str | None: """The commit the SUPERPROJECT records for the submodule at `path`. @@ -1049,10 +1076,17 @@ def recorded_gitlink(repo: Path, path: str) -> str | None: bump and passes a stale pin. HEAD remains the fallback, for a path the commit records and the index does not hold at stage 0 — one the index never had, and one a conflicted merge holds only at stages 1, 2 and 3. + + `path` IS TAKEN LITERALLY (#367): `members/*` is the entry of that name, + which no index holds, and never every gitlink a glob would match. A git + that cannot be started raises `OSError` through this, never "no + gitlink": only a listing git RAN and refused is read as nothing (#368). """ + env = {name: value for name, value in os.environ.items() + if name.upper() not in PATHSPEC_ENV_REFUSED_BESIDE_LITERAL} for args, oid_at, stage_at in GITLINK_LISTINGS: try: - out = git_out([*args, path], cwd=repo) + out = git_out([*args, path], cwd=repo, env=env) except Refusal: continue for line in out.splitlines(): diff --git a/templates/family-root/scripts/validate-family.py b/templates/family-root/scripts/validate-family.py index 72f3a145..ceece161 100755 --- a/templates/family-root/scripts/validate-family.py +++ b/templates/family-root/scripts/validate-family.py @@ -38,13 +38,25 @@ 3. the mounted tree carries a `project.yaml` whose `id` is the `id` this row records — a repository at the right commit is not by itself the project the row claims. - and, across the rows, that no two record ONE `id`: that is one project + A row whose `path:` is not `members/` is the `member-path` + finding and none of the three is asked of it: nothing read through a + path the row may not have is a fact about the member (#367). + 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 REVERSE of every member check (#361): that each gitlink this + repository records in `members/` (the index at stage 0, else HEAD), and + each path a `.gitmodules` section gives, is a path some row names. A + member mounted by hand, a row deleted by hand, a section `family.py + remove` left behind: every check above starts from a row, so none of + them ever asked. Neither needs a member checked out, so `--no-members` + asks them too. A path a row names in another case is still one of these, + and what it says to do depends on which of the two is the member's own: + it names no command that would un-register the member. - 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. @@ -53,8 +65,8 @@ 0 the manifest is valid, every member is in lockstep, every copy matches 1 a FINDING: drift a reviewer can act on 2 a REFUSAL: the question could not be asked — no manifest, an - uninitialized member, an unreadable pin. An unanswerable question is - never an implicit pass. + uninitialized member, an unreadable pin, no git to run. An + unanswerable question is never an implicit pass. """ from __future__ import annotations @@ -64,7 +76,10 @@ import re import shlex import sys +from collections import Counter +from contextlib import contextmanager from pathlib import Path +from typing import Iterator, NamedTuple sys.path.insert(0, str(Path(__file__).resolve().parent)) from repo_shape import ( # noqa: E402 @@ -78,6 +93,9 @@ MANIFEST = "family.yaml" GITMODULES = ".gitmodules" DEFAULT_MEMBERS_DIR = "members" +#: A submodule's entry in a tree or the index, as `ls-files -s` and +#: `ls-tree` spell its mode. +GITLINK_MODE = b"160000" LOCKSTEP = ( "THE LOCKSTEP RULE: a member's gitlink and its `members[].pin.commit` in " @@ -99,6 +117,34 @@ def note(self, text: str) -> None: self.notes.append(f" ok {text}") +@contextmanager +def _git_runs(unread: str) -> Iterator[None]: + """The `git-not-runnable` refusal, exit 2, for a check that reads this + holder through git and could not start it (#368). + + `git_out` raises `OSError` when there is no git to run, and nothing here + caught it: the run ended in a `FileNotFoundError` traceback, which exits + 1, a FINDING's code, though nothing was found. So every check that runs + git, in every mode, runs it inside this, and `unread` says which check + it was and what it could not read. ONE code for the whole validator, + whichever check met the missing git first: it is one fact about the + machine, and `gitmodules-unreadable` keeps meaning a file git ran and + could not read. + + Around the git calls ALONE, and never around a whole check: a + `project.yaml` nobody may read is an `OSError` too, and it is not this. + """ + try: + yield + except OSError as exc: + raise Refusal( + "git-not-runnable", + f"{unread}: git could not be run ({exc.strerror or exc})", + "Remediation: put git on PATH and run this again. This check " + "reads the holder the way git does, and nothing could clone it " + "or fetch its members without git either.") from exc + + def _check_schema_kind_and_id(manifest: dict, report: Report) -> None: """`schema_version`, `kind` and `id` — three fixed-shape fields that carry no lookup and no branch of their own, split from `_check_envelope` for @@ -244,12 +290,14 @@ def _member_project_name(row: dict, seen: dict, report: Report) -> str | None: def _check_member_repository_and_path(row: dict, project: str, - report: Report) -> str: + report: Report) -> str | None: """`repository:` and `path:` against what they must equal for `project`. Split from `_check_member` for #137. Returns `path`, defaulted the same way the row itself defaults it, since the gitlink and digest checks that - follow need the same value. + follow need the same value. Returns None where that is not the one path + the row may have, the `member-path` finding: nothing may be read through + it (#367). """ repository = row.get("repository") if not isinstance(repository, str) or not QUALIFIED_RE.match(repository): @@ -267,7 +315,11 @@ def _check_member_repository_and_path(row: dict, project: str, expected = f"{DEFAULT_MEMBERS_DIR}/{project}" if path != expected: report.finding("member-path", - f"{project}: path is {path!r}, expected {expected!r}") + f"{project}: path is {path!r}, expected {expected!r}, " + f"the one path a row for {project} may have. Its " + "gitlink, digest and project id are not checked " + "through any other.") + return None return path @@ -369,7 +421,21 @@ def _check_member_tree(member: Path, pin: dict, path: str, commit: str, def _check_member(root: Path, row: dict, seen: dict, report: Report) -> None: - """One member: the row itself, the gitlink, the digest, the identity.""" + """One member: the row itself, the gitlink, the digest, the identity. + + NOTHING IS READ THROUGH A PATH THE ROW MAY NOT HAVE (#367). A hand-edited + `path:` is `member-path`, and this member's checks stop once its pin is + read: the pin is a fact of the row, and everything after it is read at + the path. They used to go on. At `members/*` the gitlink listing took the + path for a glob and answered with a neighbour's commit, gathered as this + member's `member-gitlink-mismatch`, and the tree check then refused + `member-uninitialized`, whose remedy cannot help, and whose refusal + dropped both findings unprinted. + + The git each check runs is run inside `_git_runs`, which names the check + when there is no git to run (#368); the identity check reads a file and + runs none. + """ project = _member_project_name(row, seen, report) if project is None: return @@ -382,13 +448,18 @@ def _check_member(root: Path, row: dict, seen: dict, report: Report) -> None: "a mapping") return commit = _pin_commit(pin, project, report) + if path is None: + return # ---- the gitlink ------------------------------------------------------- - _check_member_gitlink(root, path, commit, project, report) + with _git_runs(f"the gitlink recorded at {path} could not be read"): + _check_member_gitlink(root, path, commit, project, report) # ---- the digest and the identity -------------------------------------- member = root / path - _check_member_tree(member, pin, path, commit, project, report) + with _git_runs(f"{project}'s pinned commit and its tree digest could not " + f"be read in {path}"): + _check_member_tree(member, pin, path, commit, project, report) _check_member_identity(member, row, project, report) @@ -656,56 +727,66 @@ 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. +def _gitmodules_sections(root: Path) -> dict[str, dict[str, str]]: + """Every `submodule` section of `.gitmodules`, `{name: {key: value}}`, + each key in the order git lists it: the file read ONCE for the two + questions asked of it, two sections for one path (#300) and a path no + row names (#361). 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. + because nothing could be cloned from it either. EVERY KEY a section + holds is kept, not its path and url alone, because the remedy for a + section no row names takes each of them out (`family.py`'s + `_submodule_paths` says why: a key left behind is inherited). + + No git to run is `git-not-runnable` (#368). It used to be + `gitmodules-unreadable`, which now means only a file git ran and could + not read: the missing git is one fact about the machine, and every check + here that meets it says so under one code. """ 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 - 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 + return {} + with _git_runs(f"{GITMODULES} could not be read as git reads it"): + 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: " + f"{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"): + if section.startswith("submodule."): sections.setdefault(section[len("submodule."):], {})[variable] = value + return sections + + +def _check_gitmodules_paths(sections: dict[str, dict[str, str]], + 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. `sections` is `_gitmodules_sections`'s reading. A + section whose path no row names is `_check_gitmodules_named`'s question + (#361). + """ paths: dict[str, list[str]] = {} for name, keys in sections.items(): if "path" in keys: @@ -730,17 +811,348 @@ def _check_gitmodules_paths(root: Path, report: Report) -> None: f"inherits none of them, then commit {GITMODULES}.") +def _member_gitlinks(root: Path) -> dict[str, str]: + """`{path: commit}` for every gitlink this repository records in + `members/`, the directory spelt in any case: what `_check_gitlinks_named` + asks of each (#361). + + IN THE ORDER `recorded_gitlink` READS ONE: the index at stage 0 first, + HEAD second. A path the index holds at stage 0 as a gitlink is answered + from there, any other gitlink HEAD records from HEAD, so this lists + exactly the paths at which that function finds a gitlink, and each with + the commit it would answer. + + THE WHOLE INDEX AND THE WHOLE TREE, WITH NO PATHSPEC, as `family.py`'s + `_index_entries` lists the index (#302): a pathspec matches `members/` + only in the case it is spelt, and `Members/Zed` is in `members/` on + macOS and Windows. `-z`, so a name git would quote comes back as itself. + + A HEAD git cannot list is read as recording nothing, as + `recorded_gitlink` reads it: a holder before its first commit has none. + An index git cannot list is `gitlinks-unreadable`: this is the one check + of `members/` that `--no-members` runs, and a directory nobody could + list is never an implicit pass. + """ + with _git_runs(f"the gitlinks this repository records in " + f"{DEFAULT_MEMBERS_DIR}/ could not be listed"): + try: + head = git_out(["ls-tree", "-r", "-z", "HEAD"], cwd=root, + binary=True) + except Refusal: + head = b"" + try: + index = git_out(["ls-files", "-s", "-z"], cwd=root, binary=True) + except Refusal as exc: + raise Refusal( + "gitlinks-unreadable", + f"the index could not be listed, so the gitlinks it records " + f"in {DEFAULT_MEMBERS_DIR}/ could not be read: {exc.detail}", + "Remediation: git status in this repository says what is " + "wrong with its index. Mend that, then run this " + "again.") from exc + found: dict[str, str] = {} + for listing, oid_at, stage_at in ((head, 2, None), (index, 1, 2)): + for record in listing.split(b"\0"): + meta, _, name = record.partition(b"\t") + fields = meta.split() + if len(fields) != 3 or fields[0] != GITLINK_MODE: + continue + if stage_at is not None and fields[stage_at] != b"0": + continue + path = name.decode("utf-8", "surrogateescape") + if path.split("/", 1)[0].casefold() == DEFAULT_MEMBERS_DIR: + found[path] = fields[oid_at].decode("ascii").lower() + return found + + +class RowPath(NamedTuple): + """What a row names by the path it gives: `who` names the row in a + finding, and `canonical` is the one path its project may have, + `members/`, or None for a row with no `project:`.""" + + who: str + canonical: str | None + + +def _row_paths(members: list) -> dict[str, RowPath]: + """`{path: RowPath}` for every row that gives a path: its `path:`, or + `members/` where it has none, defaulted as + `_check_member_repository_and_path` defaults it. `who` names the row in + a finding: `the row for `, or, for a row with no `project:` + (`member-project`'s), `a row with no project:`. + + A row NAMES the path it gives, AS WRITTEN (#361), and nothing else: a + hand-edited `path:` names that string, so the member's own mount is then + a gitlink no row names, beside the row's `member-path` finding. The + row's canonical path rides along, because what to say about a path that + folds to the row's depends on which of the two is the canonical one. + """ + named: dict[str, RowPath] = {} + for row in members: + if not isinstance(row, dict): + continue + project = row.get("project") + if not isinstance(project, str) or not project: + project = None + canonical = (f"{DEFAULT_MEMBERS_DIR}/{project}" + if project is not None else None) + if row.get("path"): + path = str(row["path"]) + elif canonical is not None: + path = canonical + else: + continue + named.setdefault(path, RowPath( + f"the row for {project}" if project else "a row with no project:", + canonical)) + return named + + +def _case_variant(path: str, + named: dict[str, RowPath]) -> tuple[str, RowPath] | None: + """`(row path, row)` of a row naming `path` in another case, or None. + + `str.casefold()`, as `member-duplicate` folds (#331): on macOS and + Windows `Members/Atlas` IS `members/Atlas`, so a finding about the one + says which row the other belongs to. + """ + folded = path.casefold() + for row_path, row in named.items(): + if row_path.casefold() == folded: + return row_path, row + return None + + +#: The three things a path that folds to a row's path can be, and so what a +#: finding about it may tell an operator to run: `STRAY`, a second path to +#: the member's own directory, which a command can take out; `ROW_EDITED`, +#: the member's own path, whose row was hand-edited into a case variant of +#: it; and `WRONG_CASE`, the only record of the member there is, in the +#: wrong case. +STRAY, ROW_EDITED, WRONG_CASE = "stray", "row-edited", "wrong-case" + +#: What a gitlink or a section nobody can account for says to do when the +#: fault may be in the row, in `git log -p -- family.yaml`'s words. +RESTORE_ROW = ("If its row was deleted or its path: edited by hand, put the " + "row back as git last had it (git log -p -- family.yaml shows " + "the change).") + + +def _variant_case(path: str, row_path: str, row: RowPath, + holds: set[str]) -> str: + """Which of `STRAY`, `ROW_EDITED` and `WRONG_CASE` the path `path`, which + folds to the path `row_path` that `row` names, is (#361). + + `holds` is every exact path that holds what is being asked about: the + gitlinks, or the paths the sections give. A command that takes `path` out + leaves the member standing in ONE state only, `STRAY`: the row's path is + its canonical `members/` AND that exact path holds a gitlink (a + section) too, so `path` is a second one. In the other two it takes the + member's own record out, and every mode that does not check members would + then say `family ok`: + + - `ROW_EDITED`: `path` IS the row's canonical path and the row spells it + another way, a mount at `members/Nova` and a row written `Members/Nova`. + The row is the hand edit, and the cure is in `family.yaml`. + - `WRONG_CASE`, any other: the row's path is canonical with nothing at it, + so `path` is the only record there is, in the wrong case; or the row's + path is no canonical one either and `path` is a third spelling, which + this does not decide while the row is wrong. + """ + if row_path == row.canonical and row_path in holds: + return STRAY + if path == row.canonical: + return ROW_EDITED + return WRONG_CASE + + +def _variant_said(row_path: str, row: RowPath) -> str: + """The sentence that says a path is a case variant of a row's.""" + return (f" It is a case variant of {row_path}, which {row.who} names, and " + "macOS and Windows hold the two as one directory.") + + +def _variant_advice(kind: str, path: str, row_path: str, row: RowPath, + what: str) -> str: + """What a `ROW_EDITED` or `WRONG_CASE` path says, in place of a command. + + `what` is `gitlink` or `section`. No command is printed in either: the + `git rm --cached` or the `--unset-all` lines that suit a `STRAY` would + take the member's own record out, and a family whose members a runner + could not check out would then pass. A `ROW_EDITED` says the row is what + is wrong, and where to put its path back; a `WRONG_CASE` says the member + is recorded in the wrong case, and sends the operator to `git log -p -- + family.yaml`, as an unnamed path that no row folds to does (`RESTORE_ROW`). + """ + if kind == ROW_EDITED: + return (f" It is {path}, the one path {row.who} may have, and the row " + f"names it as {row_path} instead, which macOS and Windows " + "hold as one directory with it. The row's path: is the hand " + f"edit and this {what} is the member's own: put {path} back " + "in the row's path: (git log -p -- family.yaml shows the " + f"change). No command is printed, since this {what} is not " + "what is wrong.") + return (_variant_said(row_path, row) + + " The member is recorded in the wrong case, so no command is " + f"printed: taking this {what} out could un-register it. " + + RESTORE_ROW) + + +def _check_gitlinks_named(root: Path, gitlinks: dict[str, str], + named: dict[str, RowPath], report: Report) -> None: + """Every gitlink in `members/` is at a path some row names EXACTLY — the + `member-gitlink-unnamed` finding (#361). + + Every member check starts from a row, so a gitlink no row names was + never asked about: a member mounted by hand with `git submodule add`, or + a row deleted by hand. A recursive clone of the holder fetches it all + the same, and so does `bootstrap.py`'s `git submodule update`, while + nothing pins, digests or identifies it. + + EXACTLY, as git compares paths. A gitlink a row names in another case is + still a finding, and says which of three it is (`_variant_case`). A + second path to the member's own directory is taken out of the index + alone with `git rm --cached`, which touches no file, and committed. The + member's own gitlink, which the ROW misspells, and the only record of a + member there is, in the wrong case, print no command: either could + un-register the member. Any other is taken out with a plain `git rm`, + which refuses while its checkout holds changes, then mounted with + `family.py add`: `add` cannot adopt an existing mount, and after `git rm + --cached` alone it refuses the checkout left in the way + (`member-path-taken`). `add` then names the object store `git rm` leaves + in `.git/modules`, if it is still there. `--member` is quoted as the + paths are, its project alone: `` is the operator's to fill in. + """ + holds = set(gitlinks) + for path, commit in sorted(gitlinks.items()): + if path in named: + continue + said = (f"{path} is a gitlink at {commit} that no row of members: " + "names, so a recursive clone of this holder fetches it and " + "nothing here pins, digests or identifies it.") + variant = _case_variant(path, named) + if variant is not None: + row_path, row = variant + kind = _variant_case(path, row_path, row, holds) + if kind != STRAY: + report.finding( + "member-gitlink-unnamed", + said + _variant_advice(kind, path, row_path, row, + "gitlink")) + continue + report.finding( + "member-gitlink-unnamed", + f"{said}{_variant_said(row_path, row)} Take it out of the " + "index alone, which touches no file, and commit " + f"that:\n git rm --cached -- {shlex.quote(path)}") + continue + parts = path.split("/") + member = (shlex.quote(parts[1]) if len(parts) == 2 and parts[1] + else "") + report.finding( + "member-gitlink-unnamed", + f"{said} {RESTORE_ROW} If it was mounted by hand, take it out " + "with the first command below, which refuses while its checkout " + f"holds changes and takes its {GITMODULES} section out too, then " + "mount it with the second, from a checkout of openRepoShape: " + "family.py add writes its row and its gitlink in one commit, and " + "names the object store git rm leaves under .git/modules if one " + "is still there.\n" + f" git rm -- {shlex.quote(path)}\n" + f" {PYTHON} scripts/family.py add --family-root " + f"{shlex.quote(str(root))} --member /{member}") + + +def _check_gitmodules_named(sections: dict[str, dict[str, str]], + gitlinks: dict[str, str], + named: dict[str, RowPath], report: Report) -> None: + """Every path a `.gitmodules` section gives is a path some row names + EXACTLY — the `gitmodules-path-unnamed` finding (#361). + + A section with no row is inert to a clone, which follows gitlinks, but + it is what the next `family.py add` of its name or its path meets: under + its own name git rewrites it in place and the member inherits every key + it held, and under another name `add` refuses `member-path-registered`. + It is what `family.py remove` leaves of two sections for one path + (#360), and a section for `Members/Atlas` is one `add` commits beside + the member's own, since git compares the two exactly: that one is still + a finding, and says which of three it is (`_variant_case`). Only the + section that is a second one beside the member's own is taken out. A + section with no path gives none and is not one of these. + + The remedy is #300's, by key, every key the section holds, one command + to a line: `--remove-section` cannot address a section whose name holds + `]`, and a key left behind is inherited. + + A section at the path of a gitlink whose finding says to `git rm` it + prints no command: that `git rm` takes the section out too, and a + `--unset-all` run after it exits 5, the key being gone. Where two sections give the + path (`gitmodules-path-twice`) it takes out only the one git reads for + it, the last in the file, so each of those keeps its own commands. + """ + holds = {keys["path"] for keys in sections.values() if "path" in keys} + given = Counter(keys["path"] for keys in sections.values() + if "path" in keys) + for name, keys in sections.items(): + path = keys.get("path") + if path is None or path in named: + continue + url = f"url {keys['url']}" if "url" in keys else "no url" + said = (f'{GITMODULES} gives the path {path} to the submodule named ' + f'"{name}" ({url}), and no row of members: names that path, ' + "so nothing pins what it registers, and the next family.py " + "add of its name or its path meets it.") + variant = _case_variant(path, named) + lead = "Take" + if variant is not None: + row_path, row = variant + kind = _variant_case(path, row_path, row, holds) + if kind != STRAY: + report.finding( + "gitmodules-path-unnamed", + said + _variant_advice(kind, path, row_path, row, + "section")) + continue + said += _variant_said(row_path, row) + else: + said += (" If its row was deleted or its path: edited by hand, " + "put the row back as git last had it.") + lead = "Otherwise take" + if path in gitlinks and given[path] == 1: + report.finding( + "gitmodules-path-unnamed", + f"{said} Otherwise the git rm printed above for the " + f"gitlink at {path} takes this section out too, so no " + "command is printed here.") + continue + removal = "".join( + f"\n git config -f {GITMODULES} --unset-all " + f"{shlex.quote(f'submodule.{name}.{key}')}" for key in keys) + report.finding( + "gitmodules-path-unnamed", + f"{said} {lead} the section out by hand, in a POSIX shell, every " + "key it holds, so a submodule later given its name inherits none " + f"of them, then commit {GITMODULES}:{removal}") + + 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. 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. + Split from `main` for #137. Every check that needs no member checked + out comes first, so `--no-members` asks them too: the ones that read a + file alone, `family.yaml`'s names (#331) and ids (#206) and the paths + `.gitmodules` gives twice (#300), then the gitlinks and sections no row + names, which read the index and HEAD as well (#361). """ _check_member_names_unique(members, report) _check_member_ids_unique(root, members, report) - _check_gitmodules_paths(root, report) + sections = _gitmodules_sections(root) + _check_gitmodules_paths(sections, report) + named = _row_paths(members) + gitlinks = _member_gitlinks(root) + _check_gitlinks_named(root, gitlinks, named, report) + _check_gitmodules_named(sections, gitlinks, named, 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 bec25186..41da0e6d 100644 --- a/tests/test_family_add_refusals.py +++ b/tests/test_family_add_refusals.py @@ -1588,8 +1588,7 @@ def ghost_remediation(removal: str, off_disk: bool = False) -> str: "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 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." + f"shell — {removal}, then `git add .gitmodules` — and 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 "")) @@ -1734,7 +1733,9 @@ def test_a_section_that_is_not_another_name_at_the_mount_path_is_added_past( 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.""" + url. The holder passes its own gate, except where the other section's + path is one no row names, which is now a finding of its own (#361) and + the only one: the add did not make it, and did not make it worse.""" if other is not None: path, name = other gitmodules_with(holder, (f'[submodule "{name}"]\n\tpath = {path}\n' @@ -1751,7 +1752,16 @@ def test_a_section_that_is_not_another_name_at_the_mount_path_is_added_past( f"submodule.members/{SPARE}.url", cwd=holder).stdout.strip() == url assert git("status", "--porcelain", cwd=holder).stdout == "" - assert validate(holder).returncode == 0 + checked = validate(holder) + if other is None or other[0] == f"members/{SPARE}": + assert checked.returncode == 0, checked.stderr + checked.stdout + else: + assert checked.returncode == 1, checked.stderr + checked.stdout + assert [line.split(":", 1)[0] for line in checked.stderr.splitlines() + if line.startswith("FINDING")] == [ + "FINDING gitmodules-path-unnamed"], checked.stderr + assert (f".gitmodules gives the path {other[0]} to the submodule " + f'named "{other[1]}"') in checked.stderr offline.assert_left_nothing() @@ -1884,14 +1894,18 @@ def test_a_validator_with_no_git_to_run_refuses_instead_of_crashing( 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.""" + changed for anything else. The code is `git-not-runnable`, the one the + whole validator gives a missing git (#368, + `tests/test_validate_family_without_git.py`), and no longer + `gitmodules-unreadable`, which means a file git ran and could not + read.""" 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): + def no_git(args, cwd, binary=False, env=None): raise FileNotFoundError(2, "No such file or directory", "git") monkeypatch.setattr(module, "git_out", no_git) @@ -1899,11 +1913,11 @@ def no_git(args, cwd, binary=False): out, err = capsys.readouterr() assert code == 2, out + err assert err.splitlines() == [ - "REFUSED gitmodules-unreadable: .gitmodules could not be read as git " + "REFUSED git-not-runnable: .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."] + "Remediation: put git on PATH and run this again. This check reads " + "the holder the way git does, and nothing could clone it or fetch " + "its members without git either."] def test_every_key_of_a_ghost_goes_so_a_later_member_of_its_name_inherits_none( diff --git a/tests/test_family_bump_remove_rollback.py b/tests/test_family_bump_remove_rollback.py new file mode 100644 index 00000000..066591cd --- /dev/null +++ b/tests/test_family_bump_remove_rollback.py @@ -0,0 +1,1228 @@ +# SPDX-License-Identifier: Apache-2.0 +"""`family.py bump` and `remove` refuse what they can before they write, and +put the holder back when they are refused after it (#365); `remove` refuses a +`.gitmodules` that gives the member's path to two submodules (#360). + +#365. A `bump` or `remove` that failed after its first write — a holder +`pre-commit` hook that exits 1, an `index.lock` somebody else held — exited 2 +with `REFUSED a git or gh command failed` and left everything it had done: +`bump` the member checked out at `--to`, `family.yaml` rewritten and both +staged; `remove` the member's checkout emptied by `git submodule deinit -f`, +its registration dropped, the gitlink unstaged, `.gitmodules` and +`family.yaml` changed. Nothing it printed said so. `add` puts the holder back +on the same failures and says what it put back (#203). Two refusals that a +`--dry-run` already asks before it writes were asked by the real runs only +AFTER their first write: `remove` over a `members:` line `family.yaml` this +tool will not rewrite, and `bump --to` a commit whose tree has no +`project.yaml`. Those are now asked first, so the real run and the dry run +refuse at the same point in the same words, and the holder is byte for byte +as it was. What cannot be asked first is put back, and the sentence says what +was put back, or what is left and the commands that put it back, which a +test here types exactly as printed. + +#360. git maps a path to ONE submodule name, so with two sections giving +`members/

`, `git rm` took out one and the commit kept the other, and +`remove` exited 0. Now it refuses, before any git command that writes, in +both orders of the two sections and under `--dry-run` alike, naming each with +its url and printing the by-key removal of the one that is not the member's +own; typed as printed, that removal lets the same `remove` take the member +out and leave no section for its path. + +THE ROLLBACK NEVER CLONES (review 365-5d56e75, MUST-1). `remove`'s checkout +comes back through `git submodule update`, which CLONES where git keeps no +store for the submodule's NAME. The rollback asked about the store the +checkout's own `.git` named instead, so a section renamed by hand had it +clone the member from its url. It now asks about the store git will use, +and runs `update` only where that store holds the commit; under a backstop +(`-c protocol.allow=never`, `--no-fetch`) that a test here makes the check +wrong to prove. A member whose store is gone is removed again, as it was +before the rollback (SHOULD-1), and the registration a rollback writes back +is called the same keys, not the same bytes (NIT-1). + +THE SAME FAMILY, RULES AND OFFLINE ENVIRONMENT as `test_family.py` and +`test_family_dry_run.py`: members are scaffolded into local bare +repositories, nothing is created on GitHub, a `gh` on PATH logs and exits 97, +the operator's git configuration is a file of the test's own, and git's +automatic maintenance is off. Every doubled `.gitmodules` is built in git's +own objects from BYTES (`hash-object -w --stdin`, `update-index +--cacheinfo`, a commit), as `test_family_add_refusals.gitmodules_with` +builds one, so no test depends on how a checkout writes a file. A hook is a +`#!/bin/sh` script, which git for Windows runs with its own `sh`, named by a +LOCAL `core.hooksPath` so a global one cannot point git elsewhere. +""" + +from __future__ import annotations + +import os +import re +import shlex +import subprocess +from pathlib import Path + +import pytest + +from conftest import FILE_PROTOCOL, git, rmtree, run_script +#: The family fixture, its names, the offline environment and the holder's +#: byte-for-byte state, imported from the modules that own them rather than +#: rebuilt: a holder made, or compared, any other way would test a +#: different thing. +from test_family import ( # noqa: F401 + FAMILY, family, family_module, manifest, validate, +) +from test_family_add_refusals import ( # noqa: F401 + INDEX_LOCKED, gitmodules_paths, holder, offline, refusal, + take_out_as_printed, +) +from test_family_dry_run import ( + BUMP_ORDER, commit_in_mount, isolated, registered_keys, state, +) +from test_family_member_path import MEMBER, OTHER, older, raw + +PATH = f"members/{MEMBER}" + +#: What the injected `pre-commit` hook says before it exits 1. +HOOK_SAID = "pre-commit: refusing (injected)" + + +def refusing_hook(root: Path, tmp_path: Path) -> None: + """A `pre-commit` hook in `root` that refuses every commit.""" + hooks = tmp_path / "hooks" + hooks.mkdir() + (hooks / "pre-commit").write_bytes( + f"#!/bin/sh\necho '{HOOK_SAID}' >&2\nexit 1\n".encode("utf-8")) + (hooks / "pre-commit").chmod(0o755) + git("config", "--local", "core.hooksPath", hooks.as_posix(), cwd=root) + + +def index_lock(root: Path) -> Path: + """Where git takes the holder's index lock, as git says it.""" + return root / git("rev-parse", "--git-path", "index.lock", + cwd=root).stdout.strip() + + +def snapshot(root: Path, env: dict) -> dict: + """`test_family_dry_run.state` — HEAD, `ls-files -s -z`, `status + --porcelain=v1 -z`, `family.yaml`, `.gitmodules`, `.git/config`, the + member's HEAD, every byte of the work tree, the holder's own objects — + and the branch the member is on, and the holder's `submodule.*` keys and + values, sorted: what a rollback of `remove` is held to, since `git + config --add` puts a section it brings back last in the file.""" + now = state(root, env) + mount = root / PATH + now["member branch"] = ( + raw(mount, env, "symbolic-ref", "-q", "HEAD", check=False).stdout + if (mount / ".git").exists() else None) + now["registration"] = sorted( + line for line in raw(root, env, "config", "--local", "--list" + ).stdout.decode("utf-8").splitlines() + if line.startswith("submodule.")) + return now + + +#: What a rollback puts back, compared in this order. Not the holder's +#: objects: the refused commit's `git add` stored blobs, which nothing +#: records and `git gc` drops. Not `.git/config`'s bytes for `remove`, +#: whose registration `git config --add` brings back as the last section; +#: its keys and values are compared instead. +BUMP_PUT_BACK = ("member HEAD", "member branch", "family.yaml", "index", + "status", ".gitmodules", ".git/config", "work tree", "HEAD") +REMOVE_PUT_BACK = ("index", ".gitmodules", "family.yaml", "registration", + "work tree", "member HEAD", "member branch", "status", + "HEAD") +#: A refusal made before the first write leaves EVERYTHING, objects and the +#: config's bytes included. +EVERYTHING = (*BUMP_ORDER, "member branch", "registration") + + +def assert_same(before: dict, after: dict, order) -> None: + """`after` is `before`, key by key in `order`, so a failure names the + first fact that moved.""" + for key in order: + assert after[key] == before[key], f"{key} changed" + + +def bump(root: Path, env: dict, to: str, *extra: str): + return run_script(FAMILY, "bump", "--family-root", str(root), "--member", + MEMBER, "--to", to, *extra, env=env) + + +def remove(root: Path, env: dict, *extra: str): + return run_script(FAMILY, "remove", "--family-root", str(root), + "--member", MEMBER, *extra, env=env) + + +def pinned(root: Path, env: dict) -> str: + return raw(root, env, "rev-parse", f"HEAD:{PATH}").stdout.decode( + "ascii").strip() + + +def on_main(root: Path, env: dict) -> str: + """The commit the member stands at, on the branch `git submodule add` + left it on, `main`: the state each sentence below names.""" + mount = root / PATH + assert raw(mount, env, "symbolic-ref", "--short", "-q", "HEAD" + ).stdout.decode("utf-8").strip() == "main", ( + "premise: the member is on its branch, as `add` leaves it") + return raw(mount, env, "rev-parse", "HEAD").stdout.decode("ascii").strip() + + +def loud(command: str, root: Path, code: int) -> list[str]: + """The head of the block a failed git command gets, as it always has.""" + return ["REFUSED a git or gh command failed. THE EXACT COMMAND WAS:", + f" {command}", f" (in {root.resolve()})", f" exit {code}", + "--- output ---"] + + +def bump_undone(*restored: str) -> str: + return ("Nothing was bumped: the bump has been undone — " + + ", ".join(restored) + " — and the holder is exactly as it was " + "before this command.") + + +#: How a fully unwound `remove` ends: its registration comes back as `git +#: config --add` writes it, the section last in the file, so the sentence +#: says the same keys and never calls the holder EXACTLY as it was (review +#: 365-5d56e75, NIT-1). +SAME_KEYS = (" Its git config holds the same keys and values as before, the " + "section put back written last.") + + +def remove_undone(at: str, *, legs: str = "", + held: str = "as it was before this command") -> str: + """The sentence a fully unwound `remove` ends with, every step named.""" + return ("Nothing was removed: the unmount has been undone — " + f"the gitlink at {PATH} put back in the index as it was, " + ".gitmodules restored byte for byte, family.yaml restored byte " + f"for byte, `submodule.{PATH}` restored in the holder's git " + f"config, {PATH} checked out again at {at[:12]} on main from the " + "object store `git rm` keeps" + legs + f" — and the holder is " + f"{held}." + SAME_KEYS) + + +def typed_by_hand(root: Path, sentence: str) -> list[str]: + """Run each command the sentence lists after `in this order:`, exactly + as printed (split as a POSIX shell splits it, run with no shell, each a + `git` command), and return them.""" + listed = sentence.split(" in this order: ", 1)[1].split(". Then ", 1)[0] + commands = re.findall(r"`([^`]*)`", listed) + for command in commands: + words = shlex.split(command) + assert words[0] == "git", command + subprocess.run(words, cwd=str(root), check=True, + capture_output=True) + return commands + + +def in_process(module, monkeypatch, env: dict, argv: list[str]) -> int: + """`family.py` run IN THIS PROCESS, for a failure no subprocess can + inject, in the same offline environment as a subprocess run.""" + for key, value in env.items(): + monkeypatch.setenv(key, value) + return module.main(argv) + + +# --- bump: a refusal after the checkout is put back ---------------------- + +def test_a_bump_a_pre_commit_hook_refuses_puts_the_holder_back( + family, holder, offline, tmp_path): + """THE ISSUE'S FIRST PROBE. The hook refuses the ONE commit after the + member is checked out at `--to`, `family.yaml` rewritten and both paths + staged. The failed command is quoted as it always was, exit 2, and the + sentence under it names what was put back: the gitlink's index entry, + the manifest's bytes and entry, the member back on its branch at the + commit it stood at. The holder is as it was, byte for byte but the + blobs the refused commit stored; and the same bump, the hook gone, runs + at once.""" + env = isolated(offline, tmp_path) + refusing_hook(holder, tmp_path) + target, at = older(family), on_main(holder, env) + assert target != pinned(holder, env), "premise: a bump that moves" + before = snapshot(holder, env) + + result = bump(holder, env, target) + + assert result.returncode == 2, result.stdout + result.stderr + assert result.stdout == "" + assert result.stderr.splitlines() == [ + *loud(f"git commit -q -F - -- :(literal){PATH} :(literal)family.yaml", + holder, 1), + HOOK_SAID, "--- end output ---", + bump_undone(f"the gitlink at {PATH} put back in the index as it was", + "family.yaml restored byte for byte", + f"{PATH} back on main at {at[:12]}")] + assert_same(before, snapshot(holder, env), BUMP_PUT_BACK) + offline.assert_left_nothing() + + git("config", "--local", "--unset", "core.hooksPath", cwd=holder) + again = bump(holder, env, target) + assert again.returncode == 0, again.stdout + again.stderr + assert pinned(holder, env) == target + assert validate(holder).returncode == 0 + + +def test_a_bump_under_a_held_index_lock_puts_the_holder_back( + family, holder, offline, tmp_path): + """The second probe: another git command holds the holder's + `index.lock`. The member's checkout, which is not the holder's index, + goes ahead and so does the manifest's rewrite; the first stage fails on + the lock. Neither index entry had moved, so the sentence names the two + things that had — the manifest's bytes and the member's checkout — and + no `validate-family.py` finding is left behind (it used to report + `member-gitlink-mismatch`).""" + env = isolated(offline, tmp_path) + target, at = older(family), on_main(holder, env) + lock = index_lock(holder) + lock.write_bytes(b"") + try: + before = snapshot(holder, env) + result = bump(holder, env, target) + after = snapshot(holder, env) + finally: + lock.unlink() + + assert result.returncode == 2, result.stdout + result.stderr + lines = result.stderr.splitlines() + assert lines[:5] == loud(f"git --literal-pathspecs add -f -- {PATH}", + holder, 128) + assert re.search(INDEX_LOCKED, result.stderr), result.stderr + assert lines[-1] == bump_undone("family.yaml restored byte for byte", + f"{PATH} back on main at {at[:12]}") + assert_same(before, after, BUMP_PUT_BACK) + assert validate(holder).returncode == 0 + + +# --- remove: a refusal after the unmount is put back ---------------------- + +def test_a_remove_a_pre_commit_hook_refuses_puts_the_holder_back( + family, holder, offline, tmp_path): + """THE ISSUE'S REMOVE PROBE. By the commit, `deinit -f` has emptied the + member's checkout and dropped its registration, and `git rm` has + unstaged the gitlink, deleted the directory and taken the section out + of `.gitmodules`. Each comes back — the checkout from the object store + `git rm` keeps, with no fetch, on its branch — and the sentence names + every one. The same `remove`, the hook gone, then runs at once.""" + env = isolated(offline, tmp_path) + refusing_hook(holder, tmp_path) + at = on_main(holder, env) + before = snapshot(holder, env) + + result = remove(holder, env) + + assert result.returncode == 2, result.stdout + result.stderr + assert result.stdout == "" + assert result.stderr.splitlines() == [ + *loud("git commit -q -F - -- :(literal).gitmodules " + f":(literal){PATH} :(literal)family.yaml", holder, 1), + HOOK_SAID, "--- end output ---", remove_undone(at)] + assert_same(before, snapshot(holder, env), REMOVE_PUT_BACK) + offline.assert_left_nothing() + + git("config", "--local", "--unset", "core.hooksPath", cwd=holder) + again = remove(holder, env) + assert again.returncode == 0, again.stdout + again.stderr + assert [r["project"] for r in manifest(holder)["members"]] == [OTHER] + assert validate(holder).returncode == 0 + + +def test_a_remove_puts_back_the_members_legs_it_had_checked_out( + family, holder, offline, tmp_path): + """A BOOTSTRAPPED HOLDER checks each member's legs out inside the mount + (its `make bootstrap` runs `git submodule update --init --recursive`), + and `deinit -f` deletes them with the rest. `git submodule update` brings + back one level, so the legs that were checked out come back by name, + from the stores inside the member's own, and the holder's work tree is + byte for byte what it was, theirs included.""" + env = isolated(offline, tmp_path) + git(*FILE_PROTOCOL, "submodule", "update", "-q", "--init", + cwd=holder / PATH) + legs = sorted(name for name in ("spec", "code") + if (holder / PATH / name / ".git").exists()) + assert legs == ["code", "spec"], "premise: both legs checked out" + refusing_hook(holder, tmp_path) + at = on_main(holder, env) + before = snapshot(holder, env) + + result = remove(holder, env) + + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.splitlines()[-1] == remove_undone( + at, legs=", and code and spec in it") + assert_same(before, snapshot(holder, env), REMOVE_PUT_BACK) + + +def test_what_deinit_discarded_is_named_not_called_put_back( + family, holder, offline, tmp_path): + """`deinit -f` DISCARDS what no object store holds: an untracked file + in the member, here. The checkout comes back, and the sentence says the + holder is as it was BUT FOR that, never "exactly".""" + env = isolated(offline, tmp_path) + refusing_hook(holder, tmp_path) + (holder / PATH / "SCRATCH.md").write_bytes(b"never committed\n") + at = on_main(holder, env) + before = snapshot(holder, env) + + result = remove(holder, env) + + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.splitlines()[-1] == remove_undone( + at, held="as it was before this command but for what `git submodule " + f"deinit -f` discarded in {PATH}, which no object store holds: the " + "uncommitted changes and the untracked and ignored files `git status " + "--ignored` listed there") + assert not (holder / PATH / "SCRATCH.md").exists() + assert_same(before, snapshot(holder, env), + ("index", ".gitmodules", "family.yaml", "registration", + "member HEAD", "member branch", "HEAD")) + + +def test_a_remove_under_a_held_index_lock_is_refused_before_it_unmounts( + family, holder, offline, tmp_path): + """THE PRE-CHECK. `git submodule deinit` takes no index lock, so with + one held it emptied the member and dropped its registration, and `git + rm` then died on the lock. The lock is refused first, named where git + keeps it, and nothing at all is written: not one byte of the holder, its + git config included, and the lock left where it was.""" + env = isolated(offline, tmp_path) + lock = index_lock(holder) + lock.write_bytes(b"") + try: + before = snapshot(holder, env) + result = remove(holder, env) + after = snapshot(holder, env) + assert lock.is_file() + finally: + lock.unlink() + + assert result.returncode == 2, result.stdout + result.stderr + assert result.stdout == "" + shown = lock.resolve().as_posix() + assert refusal(result.stderr) == ( + f"REFUSED family-index-locked: the holder's index is locked: {shown} " + "exists. `git submodule deinit` takes no such lock, so `remove` " + f"would empty {PATH} and drop its registration and then fail at `git " + "rm`, its first write to the index; it was refused before either, " + "and nothing was written.", + "Remediation: another git command is running in the holder, or one " + "that stopped left the lock behind. Let it finish and run the same " + "command again; if no git command is running, delete the lock (`rm " + f"-f {shlex.quote(shown)}`) and run the same command again.") + assert_same(before, after, EVERYTHING) + + +def test_a_remove_dry_run_under_a_held_index_lock_is_unchanged( + holder, offline, tmp_path): + """`--dry-run` IS UNCHANGED: it writes nothing, so a lock is not its + question, and it prints what a real remove would take out, exit 0.""" + env = isolated(offline, tmp_path) + lock = index_lock(holder) + lock.write_bytes(b"") + try: + dry = remove(holder, env, "--dry-run") + finally: + lock.unlink() + assert dry.returncode == 0, dry.stdout + dry.stderr + assert dry.stdout.splitlines()[-1].startswith("--dry-run: nothing was " + "changed") + + +# --- refusals a dry run asks first are asked first (#365) ----------------- + +def comment_on_members(root: Path, env: dict) -> None: + """A comment on the `members:` line, uncommitted: YAML reads past it and + the line rewrite does not (`family-members-unrecognised`).""" + manifest_path = root / "family.yaml" + text = manifest_path.read_bytes() + assert text.count(b"\nmembers:\n") == 1, "fixture drift" + manifest_path.write_bytes(text.replace( + b"\nmembers:\n", b"\nmembers: # edited by hand\n")) + + +@pytest.mark.parametrize("verb", ["remove", "bump"]) +def test_a_manifest_this_tool_will_not_rewrite_is_refused_before_any_write( + family, holder, offline, tmp_path, verb): + """THE COORDINATOR'S (a). `remove` refused `family-members-unrecognised` + AFTER `deinit` and `git rm`, with the member unmounted and `.gitmodules` + changed, and `bump` after its checkout, while each one's `--dry-run` + refused it first and changed nothing. Both runs now refuse it before + anything is written, in the same words, and the holder is byte for byte + as it was: the real run's stderr is the dry run's.""" + env = isolated(offline, tmp_path) + comment_on_members(holder, env) + extra = ["--to", older(family)] if verb == "bump" else [] + args = [verb, "--family-root", str(holder), "--member", MEMBER, *extra] + before = snapshot(holder, env) + + real = run_script(FAMILY, *args, env=env) + after = snapshot(holder, env) + dry = run_script(FAMILY, *args, "--dry-run", env=env) + + assert real.returncode == dry.returncode == 2, real.stdout + real.stderr + assert real.stdout == dry.stdout == "" + assert real.stderr == dry.stderr + assert refusal(real.stderr)[0].startswith( + "REFUSED family-members-unrecognised: ") + assert_same(before, after, EVERYTHING) + + +def test_a_gitlink_the_index_lacks_and_that_manifest_are_refused_alike( + holder, offline, tmp_path): + """REVIEW 365-5d56e75, NIT-2. With the gitlink gone from the index AND + a `members:` line this tool will not rewrite, the real `remove` refused + `family-members-unrecognised`, which it asks first, while its dry run + asked `ls-files` first and printed git's loud `did not match`. The dry + run asks in the real run's order now: the same refusal, in the same + words, and nothing written.""" + env = isolated(offline, tmp_path) + comment_on_members(holder, env) + raw(holder, env, "update-index", "--force-remove", "--", PATH) + before = snapshot(holder, env) + + real = remove(holder, env) + after = snapshot(holder, env) + dry = remove(holder, env, "--dry-run") + + assert real.returncode == dry.returncode == 2, real.stdout + real.stderr + assert real.stdout == dry.stdout == "" + assert real.stderr == dry.stderr + assert refusal(real.stderr)[0].startswith( + "REFUSED family-members-unrecognised: ") + assert_same(before, after, EVERYTHING) + + +def test_a_to_with_no_project_yaml_is_refused_before_the_checkout( + holder, offline, tmp_path): + """THE COORDINATOR'S (b). `bump --to` a commit whose tree has no + `project.yaml` refused `member-not-a-project` with the member left + checked out at `--to`, and the holder's own validator then reported it. + The member's `project.yaml` at `--to` is read from its objects first, as + the dry run reads it, so the member never moves and the real run's + words are the dry run's.""" + env = isolated(offline, tmp_path) + target = commit_in_mount( + holder, env, lambda mount: (mount / "project.yaml").unlink()) + before = snapshot(holder, env) + + real = bump(holder, env, target) + after = snapshot(holder, env) + dry = bump(holder, env, target, "--dry-run") + + assert real.returncode == dry.returncode == 2, real.stdout + real.stderr + assert real.stdout == dry.stdout == "" + assert real.stderr == dry.stderr + assert refusal(real.stderr)[0].startswith("REFUSED member-not-a-project: ") + assert_same(before, after, EVERYTHING) + assert validate(holder).returncode == 0 + + +# --- #360: two submodules for the member's path --------------------------- + +GHOST = (f'[submodule "ghost"]\n\tpath = {PATH}\n' + "\turl = /nowhere/ghost.git\n").encode("utf-8") + + +def head_gitmodules(root: Path) -> bytes: + """HEAD's `.gitmodules`, as bytes.""" + return subprocess.run(["git", "show", "HEAD:.gitmodules"], + cwd=str(root), capture_output=True, + check=True).stdout + + +def commit_gitmodules(root: Path, data: bytes, message: str) -> None: + """`data` as `.gitmodules`, built in git's objects from BYTES, + committed, and the same bytes written to the work tree: no test here + depends on how a checkout writes a file.""" + 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) + git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-qm", + message, cwd=root) + (root / ".gitmodules").write_bytes(data) + + +def doubled(root: Path, section: bytes, first: bool) -> None: + """HEAD's `.gitmodules` with `section` before (`first`) or after the + member's own, committed (`commit_gitmodules`).""" + data = head_gitmodules(root) + commit_gitmodules(root, section + data if first else data + section, + "A second section for the member's path") + + +def renamed_section(root: Path, name: str) -> bytes: + """HEAD's `.gitmodules` with the member's section renamed by hand to + `name`, its path and url kept, committed (`commit_gitmodules`). git now + keeps the member's store at `modules/`, while its checkout's + `.git` still names `modules/members/`. Returns the url.""" + data = head_gitmodules(root) + renamed = data.replace(f'[submodule "{PATH}"]'.encode("utf-8"), + f'[submodule "{name}"]'.encode("utf-8")) + assert renamed != data, "fixture drift" + commit_gitmodules(root, renamed, "The member's section renamed by hand") + return git("config", "-f", ".gitmodules", f"submodule.{name}.url", + cwd=root).stdout.strip() + + +@pytest.mark.parametrize("first", [False, True], + ids=["after the members own", "before it"]) +def test_two_sections_for_the_members_path_are_refused_in_either_order( + holder, offline, tmp_path, first): + """THE ISSUE'S TWO ORDERS. After the member's own section, git found the + ghost for the path and `remove` committed the member's own section and + kept its registration; before it, the commit kept the ghost. Each is now + refused before any git command that writes, by `remove` and its dry run + in the same words, naming both sections with their urls in the file's + order and printing the by-key removal of the one not named after the + path; the holder is byte for byte as it was. Typed as printed, the + removal lets the same `remove` run: no section of HEAD's `.gitmodules` + gives the path, the holder's git config holds no registration under + either name, and the holder passes its own gate.""" + env = isolated(offline, tmp_path) + url = git("config", "-f", ".gitmodules", f"submodule.{PATH}.url", + cwd=holder).stdout.strip() + doubled(holder, GHOST, first) + before = snapshot(holder, env) + + real = remove(holder, env) + after = snapshot(holder, env) + dry = remove(holder, env, "--dry-run") + + assert real.returncode == dry.returncode == 2, real.stdout + real.stderr + assert real.stdout == dry.stdout == "" + assert real.stderr == dry.stderr + assert_same(before, after, EVERYTHING) + named = [f"`{PATH}` (url {url})", "`ghost` (url /nowhere/ghost.git)"] + said, remediation = refusal(real.stderr) + assert said == ( + f"REFUSED member-path-registered-twice: .gitmodules gives {MEMBER}'s " + f"path {PATH} to 2 submodules, " + + ", ".join(named[::-1] if first else named) + + ". git finds ONE of them for a path, and `git rm` takes out only " + "that one's sections, so this command's commit would keep the other " + f"registering {PATH} with no gitlink and no row behind it. No git " + "command that writes was run and nothing was written.") + assert remediation == ( + f"Remediation: which of them is {MEMBER}'s is a person's to say, and " + "a section somebody wrote is not this tool's to drop or rewrite. If " + f"`{PATH}`, the name `family.py add` gives a member's own section, " + f"is {MEMBER}'s, take each other one out by hand, in a POSIX shell — " + "`git config -f .gitmodules --unset-all submodule.ghost.path` and " + "`git config -f .gitmodules --unset-all submodule.ghost.url`, then " + "`git add .gitmodules` — and run the same command again.") + + assert len(take_out_as_printed(holder, remediation)) == 2 + removed = remove(holder, env) + + assert removed.returncode == 0, removed.stdout + removed.stderr + assert PATH not in gitmodules_paths(holder) + assert registered_keys(holder, env, PATH) == [] + assert registered_keys(holder, env, "ghost") == [] + assert [r["project"] for r in manifest(holder)["members"]] == [OTHER] + assert raw(holder, env, "status", "--porcelain=v1", "-z").stdout == b"" + assert validate(holder).returncode == 0 + + +def test_two_sections_neither_named_after_the_path_are_both_printed( + holder, offline, tmp_path): + """Where NO section has the name `family.py add` gives a member's own, + which one is the member's is not guessed: the removal is printed for + every one, and the remediation says to leave out the member's own.""" + env = isolated(offline, tmp_path) + renamed_section(holder, "old") + doubled(holder, GHOST, first=False) + before = snapshot(holder, env) + + result = remove(holder, env) + + assert result.returncode == 2, result.stdout + result.stderr + assert_same(before, snapshot(holder, env), EVERYTHING) + remediation = refusal(result.stderr)[1] + assert (f"None of them has the name `family.py add` gives a member's " + f"own section, `{PATH}`, so the commands that follow take out " + f"every one: leave out those of {MEMBER}'s own and run the rest, " + "in a POSIX shell — `git config -f .gitmodules --unset-all " + "submodule.old.path` and `git config -f .gitmodules --unset-all " + "submodule.old.url` and `git config -f .gitmodules --unset-all " + "submodule.ghost.path` and `git config -f .gitmodules " + "--unset-all submodule.ghost.url`, then `git add .gitmodules`" + ) in remediation, remediation + + +# --- the restore never clones (review 365-5d56e75, MUST-1) --------------- + +#: The name a member's section is renamed to by hand, below: git then keeps +#: the member's store at `modules/renamed`, where none is. +RENAMED = "renamed" + + +def holds(store: Path, commit: str, work: Path) -> bool: + """Whether the object store `store` holds `commit`. `--work-tree` + because the store names a checkout of its own that may be gone.""" + return subprocess.run( + ["git", "--git-dir", str(store), "--work-tree", str(work), "cat-file", + "-e", f"{commit}^{{commit}}"], capture_output=True).returncode == 0 + + +def foreign_store(store: Path, url: str, tmp_path: Path) -> None: + """A store at `store` that is NOT the member's: one commit of its own, + so git can read its HEAD, and `origin` the member's url, which has the + pinned commit, so a fetch into it would get that commit.""" + work = tmp_path / "foreign" + git("init", "-q", "--separate-git-dir", str(store), str(work), + cwd=tmp_path) + git("commit", "-q", "--allow-empty", "-m", "Not the member's", cwd=work) + git("remote", "add", "origin", url, cwd=work) + + +def configured(env: dict, key: str, value: str) -> dict: + """`env` with one more entry in git's `GIT_CONFIG_COUNT` protocol.""" + count = int(env.get("GIT_CONFIG_COUNT", "0")) + return {**env, "GIT_CONFIG_COUNT": str(count + 1), + f"GIT_CONFIG_KEY_{count}": key, + f"GIT_CONFIG_VALUE_{count}": value} + + +@pytest.mark.parametrize("store", ["absent", "without the commit"]) +def test_a_store_git_would_not_restore_from_is_never_used( + family, holder, offline, tmp_path, store): + """THE REVIEW'S PROBE. The member's section renamed by hand: git then + looks for its store at `modules/renamed`, while the checkout's own + `.git` names `modules/members/`, which the rollback used to + check. With no store under the new name, `git submodule update` cloned + the member from its url; with one that lacks the commit, it would fetch + into it. Neither is run: everything else is put back, the directory is + made again, empty, and the sentence names the store git would use, why + it is not used, and the store the checkout had, still there; it ends by + saying the checkout can come back from that store with no network, or + from its remote, and never that it comes back ONLY from its remote while + its own store is still there (review 365-7cca975, NIT-B), and it prints + no command, so nothing in it could clone. Nothing was cloned or fetched, + and the holder's git config gained no `submodule.renamed.*` key.""" + env = isolated(offline, tmp_path) + url = renamed_section(holder, RENAMED) + other = holder / ".git" / "modules" / RENAMED + if store != "absent": + foreign_store(other, url, tmp_path) + refusing_hook(holder, tmp_path) + at, pin = on_main(holder, env), pinned(holder, env) + own = Path(git("rev-parse", "--absolute-git-dir", cwd=holder / PATH + ).stdout.strip()) + shown = git("rev-parse", "--git-path", f"modules/{RENAMED}", + cwd=holder).stdout.strip() + before = snapshot(holder, env) + + result = remove(holder, env) + + assert result.returncode == 2, result.stdout + result.stderr + why = ("which is not there, and clone it there" if store == "absent" else + f"which does not hold {pin[:12]}, and fetch it") + assert result.stderr.splitlines()[-1] == ( + f"The unmount was undone only in part (the gitlink at {PATH} put back " + "in the index as it was, .gitmodules restored byte for byte, " + f"family.yaml restored byte for byte); NOT {PATH} checked out again " + f"at {at[:12]} on main from the object store `git rm` keeps: git " + f"would check it out of {shown}, {why}, so `git submodule update` " + f"was not run, and {PATH} was made again, empty; the object store its " + f"checkout had, {own.as_posix()}, is still there. The checkout of " + f"{PATH} can come back from the object store named above with no " + "network, or from its remote; this command did neither. `git " + "status` shows whether anything else is left, before running " + "`remove` again.") + assert "comes back only from" not in result.stderr + assert_same(before, snapshot(holder, env), + ("index", ".gitmodules", "family.yaml", "registration", + ".git/config", "HEAD")) + assert list((holder / PATH).iterdir()) == [] + assert not holds(other, pin, holder) + assert holds(own, pin, holder), "the member's own store is untouched" + offline.assert_left_nothing() + + +@pytest.mark.parametrize("store", ["absent", "without the commit"]) +def test_a_wrong_store_check_still_reaches_no_remote( + family, holder, offline, tmp_path, monkeypatch, capsys, store): + """THE BACKSTOP, PROVED WITHOUT THE NETWORK. The store check is made + WRONG here (`_unrestorable` answers that git can restore), so `git + submodule update` runs against the renamed section's store, and the url + is a LOCAL bare repository that has the commit: a clone or a fetch that + happened would succeed, and show. No `GIT_ALLOW_PROTOCOL`, which would + override both. + + - `absent`: `update` would CLONE. The environment allows every protocol + by default (`protocol.allow=always`, as git allows https and ssh), so + only the restore's `-c protocol.allow=never` refuses it. + - `without the commit`: `update` would FETCH into the store. The + environment allows the file protocol BY NAME (`protocol.file.allow`), + which `protocol.allow=never` does not override, so only `--no-fetch` + stops it. + + `OFFLINE_ENV`'s empty `GIT_ALLOW_PROTOCOL` would stop both as well, so it + is taken off here, for each guard to prove itself; the next test proves + that one. + + The step is named as not done, with git's reason; the + `submodule.renamed.*` keys `update --init` wrote are named, with the + commands that take them out; and the commit is in no store but the + member's own.""" + env = isolated(offline, tmp_path) + url = renamed_section(holder, RENAMED) + other = holder / ".git" / "modules" / RENAMED + if store == "absent": + env = configured(env, "protocol.allow", "always") + else: + foreign_store(other, url, tmp_path) + env = configured(env, "protocol.file.allow", "always") + refusing_hook(holder, tmp_path) + at, pin = on_main(holder, env), pinned(holder, env) + module = family_module() + monkeypatch.setattr(module, "_unrestorable", lambda *args: None) + monkeypatch.setattr(module, "OFFLINE_ENV", {"GIT_TERMINAL_PROMPT": "0"}) + monkeypatch.delenv("GIT_ALLOW_PROTOCOL", raising=False) + + code = in_process(module, monkeypatch, env, [ + "remove", "--family-root", str(holder), "--member", MEMBER]) + out, err = capsys.readouterr() + + assert code == 2, out + err + said = err.splitlines()[-1] + assert (f"; NOT {PATH} checked out again at {at[:12]} on main from the " + "object store `git rm` keeps: `git -c protocol.allow=never " + "--literal-pathspecs submodule update --init --no-fetch -q -- " + f"{PATH}`: ") in said, said + if store == "absent": + assert "transport 'file' not allowed" in said, said + left = re.search(r"; NOT the holder's git config as it was: it holds " + r"`([^`]+)` and `([^`]+)`, which it did not\.", said) + assert left and sorted(left.groups()) == [ + f"submodule.{RENAMED}.active", f"submodule.{RENAMED}.url"], said + for key in left.groups(): + assert f"`git config --local --unset-all {key}`" in said, said + assert not holds(other, pin, holder), "nothing was cloned or fetched" + offline.assert_left_nothing() + + +def test_a_callers_protocol_settings_do_not_let_a_wrong_store_check_clone( + family, holder, offline, tmp_path, monkeypatch, capsys): + """THE BACKSTOP'S TWO GAPS (review 365-7cca975, NIT-A). A caller whose + environment says `GIT_ALLOW_PROTOCOL=file`, or whose configuration says + `protocol.file.allow=always`, had `-c protocol.allow=never` overridden, + because `protocol.allow` yields to both: a store check that is wrong + (made so here, as in the test above) and a url that is a LOCAL bare + repository, which a clone would reach and show, and `update` cloned the + member into the store of the renamed section. `_offline_git` now + empties `GIT_ALLOW_PROTOCOL`, which overrides the caller's value and + every `protocol..allow` alike: the clone is refused as a transport + git does not allow, and nothing is cloned. + + The premise is checked first: with both settings, a clone under + `-c protocol.allow=never` of the same url succeeds, which is what the + restore would have done.""" + env = isolated(offline, tmp_path) + url = renamed_section(holder, RENAMED) + other = holder / ".git" / "modules" / RENAMED + env = {**configured(env, "protocol.file.allow", "always"), + "GIT_ALLOW_PROTOCOL": "file"} + premise = subprocess.run( + ["git", "-c", "protocol.allow=never", "clone", "-q", url, + str(tmp_path / "premise")], cwd=str(tmp_path), capture_output=True, + env={**os.environ, **env}) + assert premise.returncode == 0, ( + "premise: the caller's settings defeat `-c protocol.allow=never`: " + + premise.stderr.decode("utf-8", "replace")) + refusing_hook(holder, tmp_path) + at, pin = on_main(holder, env), pinned(holder, env) + module = family_module() + monkeypatch.setattr(module, "_unrestorable", lambda *args: None) + + code = in_process(module, monkeypatch, env, [ + "remove", "--family-root", str(holder), "--member", MEMBER]) + out, err = capsys.readouterr() + + assert code == 2, out + err + said = err.splitlines()[-1] + assert (f"; NOT {PATH} checked out again at {at[:12]} on main from the " + "object store `git rm` keeps: `git -c protocol.allow=never " + "--literal-pathspecs submodule update --init --no-fetch -q -- " + f"{PATH}`: ") in said, said + assert "transport 'file' not allowed" in said, said + assert not holds(other, pin, holder), "nothing was cloned" + offline.assert_left_nothing() + + +def test_a_checkout_whose_git_dir_deinit_absorbed_is_put_back( + family, holder, offline, tmp_path): + """THE RECORDED STORE WAS WRONG THE OTHER WAY TOO. A member checked out + with a `.git` DIRECTORY of its own (cloned into the mount by hand) and + no store in the holder's git directory: `deinit` moves that `.git` into + `.git/modules/` before it empties the checkout, and the rollback + called the store gone, because the one it had recorded was the + directory `deinit` moved. git reuses the store it moved, and so does + the rollback: nothing is cloned, the checkout is back on its branch, and + the sentence says where git keeps its store now, since the member's + `.git` is a file naming it and no longer the directory it was.""" + env = isolated(offline, tmp_path) + mount = holder / PATH + url = git("config", "-f", ".gitmodules", f"submodule.{PATH}.url", + cwd=holder).stdout.strip() + rmtree(mount) + rmtree(holder / ".git" / "modules" / "members" / MEMBER) + git(*FILE_PROTOCOL, "clone", "-q", url, str(mount), cwd=tmp_path) + assert (mount / ".git").is_dir(), "premise: an embedded .git directory" + embedded = Path(git("rev-parse", "--absolute-git-dir", cwd=mount + ).stdout.strip()) + refusing_hook(holder, tmp_path) + at = on_main(holder, env) + assert at == pinned(holder, env), "premise: the clone stands at the pin" + before = snapshot(holder, env) + + result = remove(holder, env) + + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.splitlines()[-1] == remove_undone(at) + ( + f" {PATH} now has its object store at .git/modules/{PATH}, where git " + f"keeps it, and no longer at {embedded.as_posix()}.") + after = snapshot(holder, env) + assert_same(before, after, ("index", ".gitmodules", "family.yaml", + "registration", "member HEAD", + "member branch", "status", "HEAD")) + assert (mount / ".git").is_file() + + def but_its_git(tree: dict) -> dict: + return {name: data for name, data in tree.items() + if not name.startswith(f"{PATH}/.git")} + + assert but_its_git(after["work tree"]) == but_its_git(before["work tree"]) + + +def test_a_leg_no_store_holds_is_left_out_not_cloned( + family, holder, offline, tmp_path, monkeypatch, capsys): + """THE SAME RULE FOR THE MEMBER'S LEGS, which `git submodule update` + brings back by name inside the member. A leg whose store is gone by the + time the rollback runs (taken away here as the manifest's rewrite fails, + after the unmount) would be cloned, so it is not: the member and its + other leg come back, and the sentence names the leg left out and says + its checkout comes back only from its remote.""" + env = isolated(offline, tmp_path) + mount = holder / PATH + git(*FILE_PROTOCOL, "submodule", "update", "-q", "--init", cwd=mount) + assert git("config", "-f", ".gitmodules", "submodule.spec.path", + cwd=mount).stdout.strip() == "spec", "fixture drift" + leg_store = Path(git("rev-parse", "--absolute-git-dir", + cwd=mount / "spec").stdout.strip()) + shown = git("rev-parse", "--git-path", "modules/spec", + cwd=mount).stdout.strip() + module = family_module() + at = on_main(holder, env) + + def store_gone_then_full(path, text): + rmtree(leg_store) + raise OSError(28, "No space left on device", str(path)) + + monkeypatch.setattr(module, "write_lf", store_gone_then_full) + before = snapshot(holder, env) + code = in_process(module, monkeypatch, env, [ + "remove", "--family-root", str(holder), "--member", MEMBER]) + out, err = capsys.readouterr() + + assert code == 2, out + err + said, next_line = refusal(err) + assert said == ( + "REFUSED family-manifest-unwritable: family.yaml could not be " + f"rewritten without {MEMBER}'s row: No space left on device. The " + f"unmount was undone only in part (the gitlink at {PATH} put back in " + "the index as it was, .gitmodules restored byte for byte, " + f"`submodule.{PATH}` restored in the holder's git config); NOT {PATH} " + f"checked out again at {at[:12]} on main from the object store `git " + f"rm` keeps, and code and spec in it: {PATH} is checked out again, " + f"but not spec in it: git would check it out of {shown}, which is " + "not there, and clone it there, so `git submodule update` was not " + f"run for it. The checkout of {PATH}/spec comes back only from its " + "remote, which this command never reaches. `git status` shows " + "whether anything else is left, before running `remove` again.") + assert next_line == ("Remediation: make family.yaml writable, and check " + "the disk has room, then run the same command " + "again.") + assert_same(before, snapshot(holder, env), + ("index", ".gitmodules", "family.yaml", "registration", + "member HEAD", "member branch", "HEAD")) + assert (mount / "code" / ".git").exists() + assert not (mount / "spec" / ".git").exists() + assert not leg_store.exists(), "nothing was cloned" + + +# --- a member whose store is gone (review 365-5d56e75, SHOULD-1) ---------- + +def facts(root: Path, env: dict) -> dict: + """What can be read of a holder whose member's store is gone, where + `git status` itself fails: HEAD, the index, the two files and the + holder's `submodule.*` keys.""" + return { + "HEAD": raw(root, env, "rev-parse", "HEAD").stdout, + "index": raw(root, env, "ls-files", "-s", "-z").stdout, + ".gitmodules": (root / ".gitmodules").read_bytes(), + "family.yaml": (root / "family.yaml").read_bytes(), + "registration": sorted( + line for line in raw(root, env, "config", "--local", "--list" + ).stdout.decode("utf-8").splitlines() + if line.startswith("submodule.")), + } + + +def store_taken_away(root: Path) -> Path: + """The member's object store deleted, its checkout and gitfile kept.""" + store = root / ".git" / "modules" / "members" / MEMBER + rmtree(store) + assert (root / PATH / ".git").is_file() + return store + + +def test_a_member_whose_store_is_gone_is_removed_as_its_dry_run_says( + holder, offline, tmp_path): + """MAIN REMOVED SUCH A MEMBER; THE ROLLBACK'S SET-UP REFUSED IT. With + the store gone, `git status` in the holder fails, and the rollback read + the member with no guard before the first write, so `remove` was + refused `git-failed` with a LEG's remediation while its dry run exited + 0. The read is guarded: the dry run previews exactly what it previews + with the store there, and the real run removes the member in one + commit, exit 0.""" + env = isolated(offline, tmp_path) + with_store = remove(holder, env, "--dry-run") + store_taken_away(holder) + + dry = remove(holder, env, "--dry-run") + real = remove(holder, env) + + assert dry.returncode == with_store.returncode == 0, dry.stderr + assert dry.stdout == with_store.stdout + assert dry.stderr == with_store.stderr == "" + assert real.returncode == 0, real.stdout + real.stderr + assert re.fullmatch(rf" removed {MEMBER} \([^)]*\) from {PATH}", + real.stdout.splitlines()[0]), real.stdout + assert raw(holder, env, "rev-parse", "-q", "--verify", f"HEAD:{PATH}", + check=False).returncode != 0 + assert PATH not in gitmodules_paths(holder) + assert registered_keys(holder, env, PATH) == [] + assert [r["project"] for r in manifest(holder)["members"]] == [OTHER] + assert raw(holder, env, "status", "--porcelain=v1", "-z").stdout == b"" + assert validate(holder).returncode == 0 + + +def test_a_member_whose_store_is_gone_is_put_back_but_its_checkout( + holder, offline, tmp_path): + """The same member under a `pre-commit` hook that refuses: the index, + `.gitmodules`, `family.yaml`, HEAD and the registration are put back. + Its checkout is in no store git has, so it is not put back and nothing + is cloned: the directory is made again, empty, and the sentence names + the checkout it could not restore, with git's own reason, and says it + comes back only from its remote.""" + env = isolated(offline, tmp_path) + store = store_taken_away(holder) + refusing_hook(holder, tmp_path) + before = facts(holder, env) + + result = remove(holder, env) + + assert result.returncode == 2, result.stdout + result.stderr + assert re.fullmatch( + re.escape("The unmount was undone only in part (the gitlink at " + f"{PATH} put back in the index as it was, .gitmodules " + "restored byte for byte, family.yaml restored byte for " + "byte, " + f"`submodule.{PATH}` restored in the holder's git config); " + f"NOT {PATH} checked out again: git could not read its " + "checkout before this command (`git rev-parse " + "--absolute-git-dir`: fatal: ") + + r"[^`]*" + + re.escape("), so no object store here is known to hold it, and " + f"{PATH} was made again, empty. The checkout of {PATH} " + "comes back only from its remote, which this command " + "never reaches. `git status` shows whether anything else " + "is left, before running `remove` again."), + result.stderr.splitlines()[-1]), result.stderr + assert facts(holder, env) == before + assert list((holder / PATH).iterdir()) == [] + assert not store.exists(), "nothing was cloned" + offline.assert_left_nothing() + + +# --- what the rollback says when it cannot finish -------------------------- + +def test_a_remove_whose_unwind_meets_a_lock_lists_the_commands_left( + family, holder, offline, tmp_path, monkeypatch, capsys): + """A LOCK TAKEN AFTER THE PRE-CHECK — here as the manifest's rewrite + fails, after `deinit` and `git rm` — refuses every index write of the + unwind and nothing else. The sentence lists what WAS put back + (`.gitmodules`'s bytes, the registration), what was NOT with git's own + reason, the checkout as not tried for want of the gitlink and the + section, and the commands that put the rest back. With the lock gone, + those commands, typed exactly as printed and in that order, leave the + holder as it was.""" + env = isolated(offline, tmp_path) + module = family_module() + lock = index_lock(holder) + gitlink = pinned(holder, env) + staged = raw(holder, env, "ls-files", "-s", "--", ".gitmodules" + ).stdout.decode("ascii").split()[1] + at = on_main(holder, env) + + def locked_then_full(path, text): + lock.write_bytes(b"") + raise OSError(28, "No space left on device", str(path)) + + monkeypatch.setattr(module, "write_lf", locked_then_full) + before = snapshot(holder, env) + try: + code = in_process(module, monkeypatch, env, [ + "remove", "--family-root", str(holder), "--member", MEMBER]) + finally: + if lock.exists(): + lock.unlink() + out, err = capsys.readouterr() + + assert code == 2, out + err + said, next_line = refusal(err) + assert re.fullmatch( + re.escape( + "REFUSED family-manifest-unwritable: family.yaml could not be " + f"rewritten without {MEMBER}'s row: No space left on device. The " + "unmount was undone only in part (.gitmodules restored byte for " + f"byte, `submodule.{PATH}` restored in the holder's git config); " + f"NOT the gitlink at {PATH} put back in the index as it was: `git " + f"update-index --force-remove -- {PATH}`: ") + INDEX_LOCKED + + re.escape("; NOT the index entry of .gitmodules put back as it " + "was: `git update-index --force-remove -- .gitmodules`: ") + + INDEX_LOCKED + re.escape( + f"; NOT {PATH} checked out again at {at[:12]} on main from the " + "object store `git rm` keeps: not tried, as it needs the gitlink " + f"at {PATH} and .gitmodules back first. Put the rest back by " + "hand, in the holder and in this order: `git update-index --add " + f"--cacheinfo 160000,{gitlink},{PATH}`; `git update-index --add " + f"--cacheinfo 100644,{staged},.gitmodules`; `git -c " + "protocol.allow=never submodule update --init --no-fetch -- " + f"{PATH}`; `git -C {PATH} checkout -q main --`. Then `git status` " + "shows whether anything is left, before running `remove` " + "again."), said), said + assert next_line == ("Remediation: make family.yaml writable, and check " + "the disk has room, then run the same command " + "again.") + + assert len(typed_by_hand(holder, said)) == 4 + assert_same(before, snapshot(holder, env), REMOVE_PUT_BACK) + + +def test_a_bump_whose_unwind_meets_a_lock_lists_the_command_left( + family, holder, offline, tmp_path, monkeypatch, capsys): + """The same for `bump`: a lock taken once the gitlink is staged refuses + the commit's own stage and then the unwind's one index write. The + manifest and the member's checkout are put back; the gitlink's entry is + named with git's reason and the one command that puts it back, which, + typed as printed, leaves the holder as it was.""" + env = isolated(offline, tmp_path) + module = family_module() + lock = index_lock(holder) + gitlink, at, target = pinned(holder, env), on_main(holder, env), \ + older(family) + + def locked_after_the_stage(root, paths): + lock.write_bytes(b"") + return False + + monkeypatch.setattr(module, "_nothing_to_record", locked_after_the_stage) + before = snapshot(holder, env) + try: + code = in_process(module, monkeypatch, env, [ + "bump", "--family-root", str(holder), "--member", MEMBER, + "--to", target]) + finally: + if lock.exists(): + lock.unlink() + out, err = capsys.readouterr() + + assert code == 2, out + err + lines = err.splitlines() + assert lines[:5] == loud( + f"git --literal-pathspecs add -f -- {PATH} family.yaml", holder, 128) + assert re.fullmatch( + re.escape("The bump was undone only in part (family.yaml restored " + f"byte for byte, {PATH} back on main at {at[:12]}); NOT the " + f"gitlink at {PATH} put back in the index as it was: `git " + f"update-index --force-remove -- {PATH}`: ") + INDEX_LOCKED + + re.escape(". Put the rest back by hand, in the holder and in this " + "order: `git update-index --add --cacheinfo " + f"160000,{gitlink},{PATH}`. Then `git status` shows " + "whether anything is left, before running `bump` " + "again."), lines[-1]), lines[-1] + + assert typed_by_hand(holder, lines[-1]) == [ + f"git update-index --add --cacheinfo 160000,{gitlink},{PATH}"] + assert_same(before, snapshot(holder, env), BUMP_PUT_BACK) + + +# --- never over a landed commit, and not only on a refusal ---------------- + +@pytest.mark.parametrize("verb", ["bump", "remove"]) +def test_a_failure_after_the_commit_landed_undoes_nothing( + family, holder, offline, tmp_path, monkeypatch, capsys, verb): + """NEVER OVER A COMMIT THAT LANDED. A failure after `git commit` + succeeded must not unwind: the tree under a landed commit would + contradict its own history. The commit stands, the holder is clean and + valid, and the sentence says where to look.""" + env = isolated(offline, tmp_path) + module = family_module() + commit_one = module._commit_one + + def landed_then_failed(root, *args, **kwargs): + commit_one(root, *args, **kwargs) + raise module.Refusal("git-failed", "injected after the commit", + "Remediation: injected.") + + monkeypatch.setattr(module, "_commit_one", landed_then_failed) + was = git("rev-parse", "HEAD", cwd=holder).stdout.strip() + extra = ["--to", older(family)] if verb == "bump" else [] + code = in_process(module, monkeypatch, env, [ + verb, "--family-root", str(holder), "--member", MEMBER, *extra]) + out, err = capsys.readouterr() + + assert code == 2, out + err + assert refusal(err) == ( + "REFUSED git-failed: injected after the commit. The commit had " + "already landed, so nothing was undone: `git log -1` in the holder " + "shows it.", "Remediation: injected.") + assert git("rev-parse", "HEAD~1", cwd=holder).stdout.strip() == was + assert git("status", "--porcelain", cwd=holder).stdout == "" + assert validate(holder).returncode == 0 + + +@pytest.mark.parametrize("verb", ["bump", "remove"]) +def test_an_interrupt_after_the_first_write_is_put_back_and_raised( + family, holder, offline, tmp_path, monkeypatch, capsys, verb): + """NOT ONLY A REFUSAL OR A FAILED GIT COMMAND. Ctrl-C at the manifest's + rewrite — after `bump`'s checkout, after `remove`'s unmount, and asked + by neither one's checks before its first write — is put back, the + sentence printed, and the interrupt goes on exactly as raised.""" + env = isolated(offline, tmp_path) + module = family_module() + raised = KeyboardInterrupt() + + def interrupted(*args): + raise raised + + monkeypatch.setattr(module, "write_lf", interrupted) + at = on_main(holder, env) + before = snapshot(holder, env) + extra = ["--to", older(family)] if verb == "bump" else [] + with pytest.raises(KeyboardInterrupt) as caught: + in_process(module, monkeypatch, env, [ + verb, "--family-root", str(holder), "--member", MEMBER, *extra]) + assert caught.value is raised + out, err = capsys.readouterr() + + assert out == "" + assert err == (bump_undone(f"{PATH} back on main at {at[:12]}") + if verb == "bump" else + remove_undone(at).replace( + ".gitmodules restored byte for byte, family.yaml " + "restored byte for byte", + ".gitmodules restored byte for byte")) + "\n" + assert_same(before, snapshot(holder, env), + BUMP_PUT_BACK if verb == "bump" else REMOVE_PUT_BACK) diff --git a/tests/test_family_dry_run.py b/tests/test_family_dry_run.py index f418ccf7..b8daf51a 100644 --- a/tests/test_family_dry_run.py +++ b/tests/test_family_dry_run.py @@ -63,8 +63,8 @@ #: The pinned project and the name its renamed copy is added under (#330). from test_family_add_member_id import PINNED, RENAMED from test_family_add_refusals import ( # noqa: F401 - GHOST, gitmodules_with, holder, offline, one_commit_remote, refusal, - sections, + GHOST, gitmodules_paths, gitmodules_with, holder, offline, + one_commit_remote, refusal, sections, take_out_as_printed, ) from test_family_member_path import ( GLOB, MEMBER, OTHER, hand_edit, older, raw, recorded, @@ -375,6 +375,12 @@ def hand_edit_path(root: Path, env: dict) -> None: hand_edit(root, MEMBER, GLOB) +def a_second_section(root: Path, env: dict) -> None: + """#360: a section of another name giving `MEMBER`'s path, committed.""" + commit_sections(root, env, f'[submodule "ghost"]\n\tpath = ' + f'members/{MEMBER}\n') + + #: Each refusal `bump` or `remove` makes before its first write, and how a #: test brings the holder to it: `(verb, --member, --to, set-up, code)`. #: `None` for `bump`'s `--to` is the member's older commit, a real move. @@ -389,6 +395,9 @@ def hand_edit_path(root: Path, env: dict) -> None: "remove-unknown": ("remove", "Nope", None, None, "member-unknown"), "remove-member-path": ("remove", MEMBER, None, hand_edit_path, "member-path"), + "remove-path-registered-twice": ("remove", MEMBER, None, + a_second_section, + "member-path-registered-twice"), } @@ -466,10 +475,11 @@ def test_bump_dry_run_after_a_failed_fetch_says_what_it_resolved_from( def test_bump_dry_run_reads_the_members_manifest_at_to_and_refuses_as_bump( holder, offline, tmp_path): - """WHAT THE REAL RUN ASKS AFTER ITS CHECKOUT, ASKED OF THE OBJECTS. A + """WHAT THE REAL RUN READS AFTER ITS CHECKOUT, ASKED OF THE OBJECTS. A `--to` whose tree has no `project.yaml` is `member-not-a-project` from - both runs, word for word; the dry run asks it without moving the - member's checkout, which the real run leaves at `--to`.""" + both runs, word for word, and neither moves the member's checkout: the + real run asks it of the same objects before its checkout (#365), where + it used to refuse after it and leave the member at `--to`.""" env = isolated(offline, tmp_path) target = commit_in_mount( holder, env, lambda mount: (mount / "project.yaml").unlink()) @@ -484,6 +494,7 @@ def test_bump_dry_run_reads_the_members_manifest_at_to_and_refuses_as_bump( real = bump(holder, env, target) assert real.returncode == 2 assert real.stderr == dry.stderr + assert_as_it_was(holder, env, before, BUMP_ORDER) @pytest.mark.skipif(sys.platform == "win32", @@ -610,51 +621,66 @@ def registered_keys(root: Path, env: dict, name: str) -> list[str]: == f"submodule.{name}"] -#: Hand edits that give `members/` to a section of another name, -#: appended to `.gitmodules` and committed: `(text, the name git rm then drops -#: every section of, or None for none, the other names whose sections say that -#: path and stay)`. Each is what git 2.43 does, probed: the work-tree file is -#: read with overwrite, so a name keeps its LAST path, a name moving away -#: takes the old path out of git's map, and a suspicious name is ignored. +#: Hand edits that give `members/` to a section of ANOTHER name, +#: appended to `.gitmodules` and committed: `(text, the other name)`. Each is +#: a second submodule for the member's path, by any `path` line it has — +#: the last one, an earlier one it then moves away from, or a name git +#: ignores — and each is refused by `remove` and its dry run alike (#360), +#: because `git rm` takes out the sections of ONE name and the commit kept +#: the other. GHOSTS = { "two-names": ( - f'[submodule "ghost"]\n\tpath = members/{MEMBER}\n', - "ghost", [f"members/{MEMBER}"]), + f'[submodule "ghost"]\n\tpath = members/{MEMBER}\n', "ghost"), "last-path-wins": ( '[submodule "ghost"]\n\tpath = members/ELSEWHERE\n' - f'[submodule "ghost"]\n\tpath = members/{MEMBER}\n', - "ghost", [f"members/{MEMBER}"]), + f'[submodule "ghost"]\n\tpath = members/{MEMBER}\n', "ghost"), "path-left-behind": ( f'[submodule "ghost"]\n\tpath = members/{MEMBER}\n' - '[submodule "ghost"]\n\tpath = members/ELSEWHERE\n', - None, [f"members/{MEMBER}", "ghost"]), + '[submodule "ghost"]\n\tpath = members/ELSEWHERE\n', "ghost"), "suspicious-name": ( - f'[submodule "../ghost"]\n\tpath = members/{MEMBER}\n', - f"members/{MEMBER}", ["../ghost"]), + f'[submodule "../ghost"]\n\tpath = members/{MEMBER}\n', "../ghost"), } +#: Hand edits that give the path to the member's OWN name alone, in more +#: than one section: `(text, the name git rm then drops every section of, or +#: None for none, the names whose sections say that path and stay)`. Each is +#: what git 2.43 does, probed: the work-tree file is read with overwrite, so +#: a name keeps its LAST path, and a name moving away takes the old path out +#: of git's map. One name, so no refusal: the dry run says what git does. +ONE_NAME = { + "own-name-comes-back": ( + f'[submodule "members/{MEMBER}"]\n\tpath = members/ELSEWHERE\n' + f'[submodule "members/{MEMBER}"]\n\tpath = members/{MEMBER}\n', + f"members/{MEMBER}", []), + "own-name-moves-away": ( + f'[submodule "members/{MEMBER}"]\n\tpath = members/ELSEWHERE\n', + None, [f"members/{MEMBER}"]), +} -@pytest.mark.parametrize("text, dropped, kept", GHOSTS.values(), ids=GHOSTS) -def test_remove_dry_run_names_the_sections_a_real_remove_drops( - holder, offline, tmp_path, text, dropped, kept): - """A SECOND SECTION FOR THE MEMBER'S PATH — a half-removed member, a hand - edit — and the dry run names the name whose sections `git rm` drops (or - that it drops none), the names it leaves that still say the path, and - the registration `deinit` drops under the same name. Then a REAL - `remove` takes out exactly the sections of the name the dry run named, - and the registration it named, and nothing else: the dry run says what - git will do, not what a reader of the file might expect.""" - env = isolated(offline, tmp_path) - with (holder / ".gitmodules").open("ab") as gitmodules: - gitmodules.write(text.encode("utf-8")) - raw(holder, env, "add", "--", ".gitmodules") - raw(holder, env, "commit", "-q", "-m", "A second section for one path") - listed = sections((holder / ".gitmodules").read_text(encoding="utf-8")) - before = state(holder, env) - - dry = remove(holder, env, "--dry-run") - assert_as_it_was(holder, env, before, REMOVE_ORDER) +def commit_sections(root: Path, env: dict, text: str) -> list[str]: + """`text` appended to `.gitmodules` as BYTES and committed; returns the + sections the file then names, in order.""" + with (root / ".gitmodules").open("ab") as gitmodules: + gitmodules.write(text.encode("utf-8")) + raw(root, env, "add", "--", ".gitmodules") + raw(root, env, "commit", "-q", "-m", "A second section for one path") + return sections((root / ".gitmodules").read_text(encoding="utf-8")) + + +def assert_dry_run_names_what_git_drops(root: Path, env: dict, + listed: list[str], + dropped: str | None, + kept: list[str]) -> None: + """The dry run names the name whose sections `git rm` drops (or that it + drops none), the names it leaves that still say the path, and the + registration `deinit` drops under the same name; then a REAL `remove` + takes out exactly the sections of that name, and the registration it + named, and nothing else: the dry run says what git will do, not what a + reader of the file might expect.""" + before = state(root, env) + dry = remove(root, env, "--dry-run") + assert_as_it_was(root, env, before, REMOVE_ORDER) assert dry.returncode == 0, dry.stdout + dry.stderr lines = dry.stdout.splitlines() path = f"members/{MEMBER}" @@ -662,31 +688,82 @@ def test_remove_dry_run_names_the_sections_a_real_remove_drops( "section of that name" if dropped else "gitmodules none: git reads no section of .gitmodules in the " f"work tree as giving {path}, so none is removed") in lines - assert (" left in place, though it gives the same path: " - + ", ".join(f'[submodule "{name}"]' for name in kept)) in lines + left = [line for line in lines if line.startswith(" left ")] + assert left == ([] if not kept else [ + " left in place, though it gives the same path: " + + ", ".join(f'[submodule "{name}"]' for name in kept)]) named = [line for line in lines if line.startswith("registration ")] assert named == ([f'registration [submodule "{path}"] in the holder\'s ' ".git/config, dropped by `git submodule deinit`"] if dropped == path else []) - real = remove(holder, env) + real = remove(root, env) assert real.returncode == 0, real.stdout + real.stderr - assert sections((holder / ".gitmodules").read_text(encoding="utf-8")) \ + assert sections((root / ".gitmodules").read_text(encoding="utf-8")) \ == [name for name in listed if name != dropped] - assert bool(registered_keys(holder, env, path)) is (not named) + assert bool(registered_keys(root, env, path)) is (not named) + + +@pytest.mark.parametrize("text, other", GHOSTS.values(), ids=GHOSTS) +def test_remove_dry_run_names_the_sections_a_real_remove_drops( + holder, offline, tmp_path, text, other): + """A SECOND SUBMODULE FOR THE MEMBER'S PATH — a half-removed member, a + hand edit — is refused by the dry run and the real run in the same + words, with the holder byte for byte as it was (#360), naming the other + section and printing its by-key removal. Run as printed, that removal + leaves the member's own section, and then the dry run names it as the + one `git rm` drops, and a real `remove` drops exactly that and leaves no + section for the path.""" + env = isolated(offline, tmp_path) + commit_sections(holder, env, text) + before = state(holder, env) + + real = remove(holder, env) + dry = remove(holder, env, "--dry-run") + + assert_as_it_was(holder, env, before, REMOVE_ORDER) + assert dry.returncode == real.returncode == 2, dry.stdout + dry.stderr + assert dry.stdout == real.stdout == "" + assert dry.stderr == real.stderr + said, remediation = refusal(real.stderr) + assert said.startswith("REFUSED member-path-registered-twice: "), said + assert f"`{other}` (no url)" in said + assert take_out_as_printed(holder, remediation), remediation + raw(holder, env, "commit", "-q", "-m", "Take the other section out") + + assert_dry_run_names_what_git_drops( + holder, env, sections((holder / ".gitmodules").read_text( + encoding="utf-8")), f"members/{MEMBER}", []) + assert f"members/{MEMBER}" not in gitmodules_paths(holder) + + +@pytest.mark.parametrize("text, dropped, kept", ONE_NAME.values(), + ids=ONE_NAME) +def test_remove_dry_run_follows_git_through_one_names_sections( + holder, offline, tmp_path, text, dropped, kept): + """ONE NAME, MORE THAN ONE SECTION, NO REFUSAL: only one submodule says + the path, so `remove` runs, and its dry run names what git's own reading + of the file makes of it — the name's sections all dropped when its last + `path` is the member's, none when it moved away.""" + env = isolated(offline, tmp_path) + listed = commit_sections(holder, env, text) + assert_dry_run_names_what_git_drops(holder, env, listed, dropped, kept) # --- both -------------------------------------------------------------------- @pytest.mark.parametrize("verb", ["bump", "remove"]) -def test_a_dry_run_asks_the_manifest_refusal_the_real_run_asks_after_it_writes( +def test_a_dry_run_asks_the_manifest_refusal_the_real_run_asks_first_too( family, holder, offline, tmp_path, verb): - """`family-members-unrecognised` is asked by the real `bump` after its - checkout and by the real `remove` after its unmount. The dry run asks it - too, exit 2, before anything is written. The hand edit is the one + """`family-members-unrecognised` is asked by the real `bump` before its + checkout and by the real `remove` before its unmount (#365; both used to + ask it after, with the holder changed). The dry run asks it too, exit 2, + before anything is written. The hand edit is the one `test_family_add_refusals.py` makes: a comment on the `members:` line, - which YAML reads past and the line rewrite does not, left uncommitted.""" + which YAML reads past and the line rewrite does not, left uncommitted. + The real runs are held to the same holder in + `test_family_bump_remove_rollback.py`.""" env = isolated(offline, tmp_path) manifest_path = holder / "family.yaml" text = manifest_path.read_bytes().decode("utf-8") diff --git a/tests/test_validate_family_duplicates.py b/tests/test_validate_family_duplicates.py index ac71aa92..f39d21ee 100644 --- a/tests/test_validate_family_duplicates.py +++ b/tests/test_validate_family_duplicates.py @@ -29,7 +29,9 @@ member, and a fixture with two directories whose names differ only in case cannot exist on macOS or Windows (`mount_twice` spells its second row `IRRS2` for that reason). That one test says so and skips there; every other row of -this module runs on all three platforms. +this module runs on all three platforms. Each member the holder mounts keeps +a row (`keeping_mounts`): a mount no row names is a finding of its own since +#361. """ from __future__ import annotations @@ -89,6 +91,17 @@ def rewrite(holder, rows: list) -> None: "Rewrite the members", cwd=holder) +def keeping_mounts(holder, rows: list) -> list: + """`rows`, then the holder's own row for each member it mounts that no + row in `rows` gives the path of. A mount no row names is a finding of + its own (#361, `member-gitlink-unnamed` and `gitmodules-path-unnamed`), + and every test here asks about the names `rows` spells, not about that, + so each member the holder mounts keeps a row.""" + given = {row.get("path") for row in rows} + return [*rows, *(row for row in manifest(holder)["members"] + if row["path"] not in given)] + + def findings(result) -> list[str]: """Every `FINDING` line, in the order they were printed.""" return [line for line in result.stderr.splitlines() @@ -156,8 +169,9 @@ def test_rows_that_differ_only_in_case_are_found_under_no_members( """Both rows are named as WRITTEN, the earlier one first, whichever of the two spellings that is, and the finding says why they are one.""" base = irrs_row(holder) - rewrite(holder, [spelled(base, first, "irrs-first"), - spelled(base, second, "irrs-second")]) + rewrite(holder, keeping_mounts(holder, [ + spelled(base, first, "irrs-first"), + spelled(base, second, "irrs-second")])) result = validate(holder, "--no-members") assert result.returncode == 1, result.stderr + result.stdout assert len(findings(result)) == 1, result.stderr @@ -173,7 +187,8 @@ def test_a_case_variant_is_not_the_exact_finding(holder): some disks. A reader told the wrong one goes looking for a row that is not there.""" base = irrs_row(holder) - rewrite(holder, [base, spelled(base, "irrs", "irrs-twin")]) + rewrite(holder, keeping_mounts( + holder, [base, spelled(base, "irrs", "irrs-twin")])) result = validate(holder, "--no-members") assert result.returncode == 1, result.stderr + result.stdout assert len(findings(result)) == 1, result.stderr @@ -192,8 +207,9 @@ def test_rows_that_a_disk_folds_but_lower_does_not_are_found_under_no_members( first, second = pair assert first.lower() != second.lower(), "the point is that lower() fails" base = irrs_row(holder) - rewrite(holder, [spelled(base, first, "irrs-first"), - spelled(base, second, "irrs-second")]) + rewrite(holder, keeping_mounts(holder, [ + spelled(base, first, "irrs-first"), + spelled(base, second, "irrs-second")])) result = run_script(holder / "scripts" / "validate-family.py", "--no-members", cwd=holder, env={"PYTHONUTF8": "1"}) @@ -210,7 +226,8 @@ def test_the_case_finding_does_not_assume_one_project(holder): because `family.py remove` cannot take out a row that was never mounted, which is the only kind a macOS or Windows holder can hold.""" base = irrs_row(holder) - rewrite(holder, [base, spelled(base, "irrs", "irrs-twin")]) + rewrite(holder, keeping_mounts( + holder, [base, spelled(base, "irrs", "irrs-twin")])) result = validate(holder, "--no-members") assert result.returncode == 1, result.stderr + result.stdout (line,) = findings(result) @@ -231,7 +248,8 @@ def test_each_later_row_is_one_finding_against_the_first_that_holds_its_name( base = irrs_row(holder) lower = spelled(base, "irrs", "irrs-two") title = spelled(base, "Irrs", "irrs-three") - rewrite(holder, [base, lower, title, dict(lower)]) + rewrite(holder, keeping_mounts(holder, + [base, lower, title, dict(lower)])) result = validate(holder, "--no-members") assert result.returncode == 1, result.stderr + result.stdout found = findings(result) diff --git a/tests/test_validate_family_member_path_glob.py b/tests/test_validate_family_member_path_glob.py new file mode 100644 index 00000000..681e1467 --- /dev/null +++ b/tests/test_validate_family_member_path_glob.py @@ -0,0 +1,138 @@ +# SPDX-License-Identifier: Apache-2.0 +"""A hand-edited `path:` is `member-path`, and nothing is read through it +(#367); `recorded_gitlink` takes its path literally, for every caller. + +THREE THINGS COMBINED. `_check_member` found a row's `path:` wrong, recorded +`member-path`, and then went on to check the member's gitlink, tree and +identity AT that path. `recorded_gitlink` handed the path to `git ls-files` +as a pathspec, so `members/*` was a glob and answered with the first +member's gitlink, gathered as a false `member-gitlink-mismatch` for this +one. The tree check then refused `member-uninitialized`, whose remedy (`git +submodule update --init`, `make bootstrap`) cannot help, and a refusal +prints none of the findings gathered before it, so the one naming the hand +edit was never seen. + +Two of the issue's three decisions are taken. A row whose path is wrong +stops after its pin is read, and the finding names the one path it may +have. `recorded_gitlink` passes git's global `--literal-pathspecs` to both +of its listings, never `:(literal)` magic, which a caller's exported +`GIT_LITERAL_PATHSPECS=1` turns off; and the two pathspec settings git +refuses beside that option are kept out of those two calls, so a caller who +exports one does not read every gitlink as absent. The third decision, a +refusal that prints the findings gathered before it, is not taken. + +The row's own mount is then a gitlink and a section no row names (#361, +`tests/test_validate_family_unnamed_mounts.py`), reported beside +`member-path`: the row names the path it gives, as written. + +The same family and offline rules as `tests/test_family.py`. The hand edit +is to `family.yaml` alone, written as bytes; nothing named `*` is ever put +on disk, so every test here runs on Windows too. +""" + +from __future__ import annotations + +import pytest + +from conftest import run_script +from repo_shape import recorded_gitlink +from test_family import family, holder, manifest, validate # noqa: F401 + +#: What a hand edit can leave in IRSS's `path:`, from the issue: globs, +#: pathspec magic, a `..` and a trailing slash. Each is `member-path`. +SPELLINGS = ["members/*", "members/I*", "members/[I]*", ":(glob)members/IRSS", + "members/IRSS/..", "members/IRSS/"] + + +def hand_edit(holder, spelling: str) -> None: + """IRSS's row given `path: ""`, as BYTES, so Windows does not + rewrite the manifest with CRLF.""" + manifest_path = holder / "family.yaml" + text = manifest_path.read_bytes() + line = b" path: members/IRSS\n" + assert text.count(line) == 1, "fixture drift: IRSS's path line" + manifest_path.write_bytes(text.replace( + line, f' path: "{spelling}"\n'.encode("utf-8"))) + + +def codes(result) -> list[str]: + return [line.split(":", 1)[0][len("FINDING "):] + for line in result.stderr.splitlines() + if line.startswith("FINDING ")] + + +def gitlinks(holder) -> dict[str, str]: + return {row["project"]: row["pin"]["commit"] + for row in manifest(holder)["members"]} + + +@pytest.mark.parametrize("extra", [(), ("--pins",)], + ids=["members-checked", "pins"]) +@pytest.mark.parametrize("spelling", SPELLINGS) +def test_a_hand_edited_path_is_member_path_and_nothing_read_through_it( + holder, spelling, extra): + """THE ISSUE'S REPRO, in both modes that check members: the run used to + end in `REFUSED member-uninitialized`, exit 2, with the `member-path` + finding and a false `member-gitlink-mismatch` gathered and dropped. Now + `member-path` is printed, naming the one path the row may have, nothing + is read at the path the row gives, and the run is a finding, exit 1. + IRSS's own mount is then a gitlink and a section no row names (#361), + and nothing else is said.""" + hand_edit(holder, spelling) + result = validate(holder, *extra) + assert result.returncode == 1, result.stderr + result.stdout + assert "REFUSED" not in result.stderr, result.stderr + assert "member-uninitialized" not in result.stderr + assert "member-gitlink-mismatch" not in result.stderr + assert codes(result) == ["member-gitlink-unnamed", + "gitmodules-path-unnamed", "member-path"] + assert (f"FINDING member-path: IRSS: path is {spelling!r}, expected " + "'members/IRSS', the one path a row for IRSS may have. Its " + "gitlink, digest and project id are not checked through any " + "other.") in result.stderr.splitlines() + + +def test_the_other_rows_are_still_checked(holder): + """Only the edited row stops: IRRS's gitlink, digest and id are read as + before, and say so.""" + hand_edit(holder, "members/*") + result = validate(holder) + assert " ok members/IRRS: gitlink == pin" in result.stdout + assert " ok members/IRRS: project.yaml id irrs" in result.stdout + assert "members/IRSS: gitlink" not in result.stdout + + +@pytest.mark.parametrize("spelling", ["members/*", "members/I*", + "members/[I]*", ":(glob)members/IRRS"]) +def test_recorded_gitlink_takes_its_path_literally(holder, spelling): + """In a holder with members, and no file named `*` on disk, a glob or a + magic pathspec names no entry, so there is no gitlink there; the + member's own path still answers with its pin. The standard's + `repo_shape.py` is the one under test: every holder and every project + receives a copy of it.""" + pins = gitlinks(holder) + assert not any("*" in entry.name + for entry in (holder / "members").iterdir()) + assert recorded_gitlink(holder, spelling) is None + assert recorded_gitlink(holder, "members/IRRS") == pins["IRRS"] + + +@pytest.mark.parametrize("variable", [ + "GIT_GLOB_PATHSPECS", "GIT_ICASE_PATHSPECS", "GIT_LITERAL_PATHSPECS", + "GIT_NOGLOB_PATHSPECS"]) +def test_a_callers_pathspec_setting_neither_widens_nor_breaks_it( + holder, monkeypatch, variable): + """git refuses `--literal-pathspecs` beside an exported + `GIT_GLOB_PATHSPECS` or `GIT_ICASE_PATHSPECS` (exit 128), and a listing + git refused reads as no gitlink, so those two are kept out of the + listings' environment: the gitlink is found, and a glob still finds + none. `GIT_LITERAL_PATHSPECS`, which turns `:(literal)` magic off, and + `GIT_NOGLOB_PATHSPECS` change nothing either. The holder's own gate, + run with the variable exported, passes.""" + monkeypatch.setenv(variable, "1") + pins = gitlinks(holder) + assert recorded_gitlink(holder, "members/IRRS") == pins["IRRS"] + assert recorded_gitlink(holder, "members/*") is None + result = run_script(holder / "scripts" / "validate-family.py", "--pins", + cwd=holder, env={variable: "1"}) + assert result.returncode == 0, result.stderr + result.stdout diff --git a/tests/test_validate_family_unnamed_mounts.py b/tests/test_validate_family_unnamed_mounts.py new file mode 100644 index 00000000..5791a49a --- /dev/null +++ b/tests/test_validate_family_unnamed_mounts.py @@ -0,0 +1,715 @@ +# SPDX-License-Identifier: Apache-2.0 +"""`validate-family.py` finds a gitlink in `members/`, and a `.gitmodules` +section, whose path no `members:` row names, in every mode (#361). + +EVERY CHECK STARTED FROM A ROW. `_check_members` walked `members:` and asked +each row for its gitlink, digest and identity, and nothing walked back from +what the holder records to a row. So a member mounted by hand with `git +submodule add`, a row deleted by hand, the section `family.py remove` leaves +of two for one path (#360), and a section for `Members/IRRS`, which `add` +commits beside the member's own, all passed the holder's own gate, and the +doctor, which runs that gate, called the holder COMPLIANT. + +TWO FINDINGS, each with its own code, both made before the `--no-members` +return, as `member-duplicate` (#331) is, since a private family's CI falls +back to that mode: `member-gitlink-unnamed` for a gitlink, read from the +index at stage 0, else HEAD, the order `recorded_gitlink` reads one in, and +`gitmodules-path-unnamed` for a section, read as git reads the file, as +`gitmodules-path-twice` (#300) reads it. "Unnamed" is decided EXACTLY, as +git compares paths. A path a row names in another case is still a finding, +and it names that row: macOS and Windows hold the two as one directory. + +A PATH THAT FOLDS TO A ROW'S is one of three things, and what it says to run +depends on which (review of #406, SHOULD-1). A SECOND path to the member's own +directory is taken out with `git rm --cached`. A row HAND-EDITED into a case +variant of its own mount (`path: Members/IRSS` over a mount at +`members/IRSS`) is the row's fault, and a member whose ONLY record is at a +case variant (`members/irss`, the row saying `members/IRSS`) is recorded in +the wrong case: the same command, run in either, takes the member's own +record out of the index, after which `--no-members`, the run a private +family's CI falls back to, says `family ok` over a member that is no longer +there. Those two print no command. + +When a hand mount's gitlink says to `git rm` it, its section finding says that +the `git rm` takes the section out too and prints no `--unset-all` line: run +after it they exit 5, the keys being gone. + +The same family and the same offline rules as `tests/test_family.py`, whose +fixture is imported. EVERY STATE BUT ONE IS BUILT IN GIT'S INDEX AND +OBJECTS, never through a checkout, so each runs on all three platforms: a +gitlink is an index record written as bytes on stdin (`update-index +--index-info`), and a section is appended to `.gitmodules` by +`tests/test_family_add_refusals.py`'s `gitmodules_with`, which hashes the +file's bytes the same way. The one exception runs the printed remedy, and +mounts the spare member for real with `git submodule add`, as +`tests/test_validate_family_duplicates.py` does, because what that remedy +must clear is what a real mount leaves. + +THE CASE-VARIANT STATES HOLD ON macOS AND WINDOWS the same way: a disk that +folds case makes `members/IRSS` and `members/irss` one directory, so none of +them puts a second directory on disk. A gitlink is only ever an index record +(`stage_gitlink`, `git rm --cached`) and a commit of the index, which reads no +directory, so the checkout of `members/IRSS` stays the one directory it is and +the index says whichever spelling the test wants. A section is hashed into +the objects and the one `.gitmodules` file is written beside it, and the one +hand edit, `family.yaml`, is a single file written as bytes. +""" + +from __future__ import annotations + +import shlex +import subprocess + +import pytest + +from conftest import git, rmtree, run_script +from repo_shape import PYTHON +from test_family import ( # noqa: F401 + FAMILY, ORG, SPARE, family, family_module, holder, manifest, validate, +) +from test_family_add_refusals import git_section, gitmodules_with +from test_validate_family_member_path_glob import hand_edit + +MODES = pytest.mark.parametrize( + "extra", [(), ("--pins",), ("--no-members",)], + ids=["members-checked", "pins", "no-members"]) + +#: What every unnamed gitlink says after its path and commit, and what one +#: that is NOT a case variant of a row's path says to do about it. A +#: holder's CI matches on text, so it is pinned here and not rebuilt from the +#: validator's own strings. +GITLINK_SAID = ( + "FINDING member-gitlink-unnamed: {path} is a gitlink at {commit} that " + "no row of members: names, so a recursive clone of this holder fetches " + "it and nothing here pins, digests or identifies it.") +GITLINK_REMEDY = ( + " If its row was deleted or its path: edited by hand, put the row back " + "as git last had it (git log -p -- family.yaml shows the change). If it " + "was mounted by hand, take it out with the first command below, which " + "refuses while its checkout holds changes and takes its .gitmodules " + "section out too, then mount it with the second, from a checkout of " + "openRepoShape: family.py add writes its row and its gitlink in one " + "commit, and names the object store git rm leaves under .git/modules if " + "one is still there.") + +#: The same two halves for an unnamed `.gitmodules` section. +SECTION_SAID = ( + "FINDING gitmodules-path-unnamed: .gitmodules gives the path {path} to " + "the submodule named \"{name}\" ({url}), and no row of members: names " + "that path, so nothing pins what it registers, and the next family.py " + "add of its name or its path meets it.") +SECTION_REMEDY = ( + " If its row was deleted or its path: edited by hand, put the row back " + "as git last had it. Otherwise take the section out by hand, in a POSIX " + "shell, every key it holds, so a submodule later given its name inherits " + "none of them, then commit .gitmodules:") + +#: What a path a row names in another case adds, in place of the "deleted +#: or edited" sentence: the row it belongs to. +VARIANT = (" It is a case variant of {row_path}, which the row for " + "{project} names, and macOS and Windows hold the two as one " + "directory.") + +#: What a path that is the member's own, in a row hand-edited into a case +#: variant of it, says: the row is the hand edit, and no command. `{what}` is +#: `gitlink` or `section`. +ROW_EDITED = ( + " It is {path}, the one path the row for {project} may have, and the row " + "names it as {row_path} instead, which macOS and Windows hold as one " + "directory with it. The row's path: is the hand edit and this {what} is " + "the member's own: put {path} back in the row's path: (git log -p -- " + "family.yaml shows the change). No command is printed, since this {what} " + "is not what is wrong.") + +#: What the only record of a member, in the wrong case, says: no command, and +#: the restore sentence the unnamed gitlink always had. +WRONG_CASE = ( + VARIANT + " The member is recorded in the wrong case, so no command is " + "printed: taking this {what} out could un-register it. If its row was " + "deleted or its path: edited by hand, put the row back as git last had " + "it (git log -p -- family.yaml shows the change).") + +#: What a hand mount's section says once its gitlink's `git rm` takes it out. +SECTION_UNDER_GIT_RM = ( + " If its row was deleted or its path: edited by hand, put the row back as " + "git last had it. Otherwise the git rm printed above for the gitlink at " + "{path} takes this section out too, so no command is printed here.") + + +def findings(result) -> list[str]: + """Every finding WHOLE, in the order printed: its `FINDING` line and + each indented line under it, the commands a remedy prints.""" + found: list[list[str]] = [] + for line in result.stderr.splitlines(): + if line.startswith("FINDING "): + found.append([line]) + elif found and line.startswith(" "): + found[-1].append(line) + return ["\n".join(lines) for lines in found] + + +def codes(result) -> list[str]: + """The code of every finding, in the order printed.""" + return [finding.split(":", 1)[0][len("FINDING "):] + for finding in findings(result)] + + +def commands(finding: str) -> list[str]: + """The commands a finding prints, one to an indented line.""" + return [line.strip() for line in finding.splitlines()[1:]] + + +def stage_gitlink(root, path: str, commit: str) -> None: + """A gitlink at `path` in `root`'s index and nowhere else: an index + record written as BYTES on stdin, so no disk has to hold `path` and no + clone is made. `--index-info` replaces a record already there.""" + subprocess.run(["git", "update-index", "--add", "--index-info"], + cwd=str(root), capture_output=True, check=True, + input=f"160000 {commit}\t{path}\n".encode("utf-8")) + + +def stage_unmerged_gitlink(root, path: str, commit: str) -> None: + """A gitlink at `path` held at the conflict stages 1, 2 and 3 and NOT at + stage 0, as a conflicted merge leaves it. As BYTES on stdin, like + `stage_gitlink`: first the zero-mode record that drops any stage-0 entry + (`--index-info` will not place a higher stage over one), then the three + stages.""" + records = f"0 {'0' * 40}\t{path}\n" + "".join( + f"160000 {commit} {stage}\t{path}\n" for stage in (1, 2, 3)) + subprocess.run(["git", "update-index", "--index-info"], cwd=str(root), + capture_output=True, check=True, + input=records.encode("utf-8")) + + +def commit(root, message: str) -> None: + git("commit", "-qm", message, cwd=root) + + +def record_in_another_case(holder, project: str, spelling: str) -> str: + """`project`'s gitlink taken out of the index at its own path, recorded at + `spelling` instead, and committed: in the index and objects alone, so no + disk has to hold both names and the checkout does not move. Returns the + commit the gitlink names.""" + named = pinned(holder, project) + git("rm", "-q", "--cached", "--", f"members/{project}", cwd=holder) + stage_gitlink(holder, spelling, named) + commit(holder, f"Record {project} at {spelling}") + return named + + +def spare_commits(family) -> tuple[str, str]: + """The spare member's two commits, older first: real commits a gitlink + can name, though the holder never mounts them.""" + work = family["work"] / SPARE + older, newer = (git("rev-parse", rev, cwd=work).stdout.strip() + for rev in ("HEAD~1", "HEAD")) + return older, newer + + +def pinned(holder, project: str) -> str: + return next(row["pin"]["commit"] for row in manifest(holder)["members"] + if row["project"] == project) + + +def with_path_replaced(holder, old: bytes, new: bytes) -> None: + """HEAD's `.gitmodules` with `old` replaced by `new` (once), hashed into + git's objects and committed, and the same bytes written to disk: + `gitmodules_with`'s mechanics for an edit in place rather than an append, + as BYTES, so no platform rewrites a line ending.""" + data = subprocess.run(["git", "show", "HEAD:.gitmodules"], + cwd=str(holder), capture_output=True, + check=True).stdout + assert data.count(old) == 1, data + data = data.replace(old, new) + blob = subprocess.run(["git", "hash-object", "-w", "--stdin"], + cwd=str(holder), input=data, capture_output=True, + check=True).stdout.decode("ascii").strip() + git("update-index", "--cacheinfo", f"100644,{blob},.gitmodules", + cwd=holder) + commit(holder, "A section whose path is another case") + (holder / ".gitmodules").write_bytes(data) + + +def with_section(holder, tmp_path, name: str, path: str, url: str) -> None: + """`[submodule ""]` giving `path` and `url`, appended to HEAD's + `.gitmodules` in git's objects and committed, with the same bytes on + disk.""" + gitmodules_with(holder, git_section(tmp_path / "section", name, + path=path, url=url), "the work tree") + + +def removal(name: str, *keys: str) -> list[str]: + """The commands that take every key of section `name` out, as printed.""" + return [f"git config -f .gitmodules --unset-all " + f"{shlex.quote(f'submodule.{name}.{key}')}" for key in keys] + + +def take_out_as_printed(holder, finding: str) -> None: + """Run every command `finding` prints, split as a POSIX shell splits it + and run with no shell, then commit `.gitmodules`, as it says.""" + for command in commands(finding): + argv = shlex.split(command) + assert argv[0] == "git", command + git(*argv[1:], cwd=holder) + git("add", "--", ".gitmodules", cwd=holder) + commit(holder, "Take the section out as printed") + + +def assert_passes_in_every_mode(holder, members: int) -> None: + for extra in ((), ("--pins",), ("--no-members",)): + result = validate(holder, *extra) + assert result.returncode == 0, (extra, result.stderr + result.stdout) + assert (f"family ok: InkRouter (inkrouter), {members} member(s)" + in result.stdout), extra + + +# --- the issue's three rows -------------------------------------------------- + +@MODES +def test_a_member_mounted_by_hand_is_two_findings(family, holder, tmp_path, + extra): + """ROW (a): what `git submodule add` of a member leaves, with no row: a + gitlink in `members/` that every recursive clone fetches, and the + section that registers it. Neither is named, so each is its own + finding, in every mode: neither needs the member checked out. + + The gitlink says to `git rm` it, and that takes its section out too, so + the section's finding says that and prints no `--unset-all` line: run + after the `git rm` they exit 5, the keys being gone.""" + _, tip = spare_commits(family) + stage_gitlink(holder, f"members/{SPARE}", tip) + with_section(holder, tmp_path, f"members/{SPARE}", f"members/{SPARE}", + f"/nowhere/{SPARE}.git") + result = validate(holder, *extra) + assert result.returncode == 1, result.stderr + result.stdout + assert "REFUSED" not in result.stderr, result.stderr + gitlink, section = findings(result) + assert gitlink.splitlines()[0] == GITLINK_SAID.format( + path=f"members/{SPARE}", commit=tip) + GITLINK_REMEDY + assert commands(gitlink) == [ + f"git rm -- members/{SPARE}", + f"{PYTHON} scripts/family.py add --family-root " + f"{shlex.quote(str(holder.resolve()))} --member /{SPARE}"] + assert section == SECTION_SAID.format( + path=f"members/{SPARE}", name=f"members/{SPARE}", + url=f"url /nowhere/{SPARE}.git") + SECTION_UNDER_GIT_RM.format( + path=f"members/{SPARE}") + assert commands(section) == [] + assert "--unset-all" not in result.stderr + + +@MODES +def test_a_section_no_row_names_is_a_finding(holder, tmp_path, extra): + """ROW (b): a section with no row and no gitlink, the state `family.py + remove` leaves when the file held two sections for one path (#360). A + clone does not follow it, but the next `add` of its name or its path + meets it, so it is a finding, in every mode.""" + with_section(holder, tmp_path, "members/Old", "members/Old", + "/nowhere/old.git") + result = validate(holder, *extra) + assert result.returncode == 1, result.stderr + result.stdout + (section,) = findings(result) + assert section.splitlines()[0] == SECTION_SAID.format( + path="members/Old", name="members/Old", + url="url /nowhere/old.git") + SECTION_REMEDY + assert commands(section) == removal("members/Old", "path", "url") + + +@MODES +def test_a_section_for_a_case_variant_of_a_members_path_names_that_row( + holder, tmp_path, extra): + """ROW (c): a section under another name for `Members/IRRS`, beside + IRRS's own for `members/IRRS`. git compares the two paths exactly, so + `add` committed IRRS's section beside it and `gitmodules-path-twice` + does not pair them, but on macOS and Windows they are one directory. + So it is unnamed, decided exactly, and the finding names the row whose + path it is the variant of.""" + with_section(holder, tmp_path, "ghost", "Members/IRRS", + "/nowhere/ghost.git") + result = validate(holder, *extra) + assert result.returncode == 1, result.stderr + result.stdout + (section,) = findings(result) + assert section.splitlines()[0] == ( + SECTION_SAID.format(path="Members/IRRS", name="ghost", + url="url /nowhere/ghost.git") + + VARIANT.format(row_path="members/IRRS", project="IRRS") + + " Take the section out by hand, in a POSIX shell, every key it " + "holds, so a submodule later given its name inherits none of them, " + "then commit .gitmodules:") + assert commands(section) == removal("ghost", "path", "url") + + +# --- the two findings' edges ------------------------------------------------- + +@MODES +def test_a_gitlink_at_a_case_variant_of_a_members_path_names_that_row( + holder, extra): + """A gitlink at `Members/IRRS` beside IRRS's at `members/IRRS`, in the + index only, so the same on every disk. Its directory IS IRRS's where + case is folded, so its remedy is `git rm --cached`, which touches no + file, and never the plain `git rm` that would take IRRS's checkout with + it there.""" + commit_ = pinned(holder, "IRRS") + stage_gitlink(holder, "Members/IRRS", commit_) + result = validate(holder, *extra) + assert result.returncode == 1, result.stderr + result.stdout + (gitlink,) = findings(result) + assert gitlink.splitlines()[0] == ( + GITLINK_SAID.format(path="Members/IRRS", commit=commit_) + + VARIANT.format(row_path="members/IRRS", project="IRRS") + + " Take it out of the index alone, which touches no file, and " + "commit that:") + assert commands(gitlink) == ["git rm --cached -- Members/IRRS"] + + +def test_the_case_variant_remedy_run_as_printed_clears_it_and_no_file(holder): + """Run as printed, it takes the variant out of the index and leaves + IRRS's checkout and gitlink as they were, and the holder passes.""" + stage_gitlink(holder, "Members/IRRS", pinned(holder, "IRRS")) + (gitlink,) = findings(validate(holder, "--no-members")) + (command,) = commands(gitlink) + git(*shlex.split(command)[1:], cwd=holder) + assert (holder / "members" / "IRRS" / "project.yaml").is_file() + assert_passes_in_every_mode(holder, 2) + + +def test_a_gitlink_is_read_from_the_index_first_and_from_head_second( + family, holder): + """THE ORDER `recorded_gitlink` READS ONE. A gitlink HEAD records at one + commit and the index stages at another is reported at the staged one, + which the next commit will record; once the index no longer holds it, + HEAD's is the gitlink, and it is still a finding until a commit drops + it.""" + older, newer = spare_commits(family) + path = f"members/{SPARE}" + stage_gitlink(holder, path, older) + commit(holder, "A gitlink no row names") + stage_gitlink(holder, path, newer) + (staged,) = findings(validate(holder, "--no-members")) + assert staged.startswith(GITLINK_SAID.format(path=path, commit=newer)) + git("rm", "-q", "--cached", "--", path, cwd=holder) + (committed,) = findings(validate(holder, "--no-members")) + assert committed.startswith(GITLINK_SAID.format(path=path, commit=older)) + commit(holder, "Drop it") + assert_passes_in_every_mode(holder, 2) + + +def test_only_members_is_asked_about(family, holder): + """`members/` in any case, and nothing beside it: a gitlink elsewhere + in the holder is not a member's mount, and no row could name it.""" + _, tip = spare_commits(family) + stage_gitlink(holder, f"tools/{SPARE}", tip) + stage_gitlink(holder, f"membersX/{SPARE}", tip) + stage_gitlink(holder, f"MEMBERS/{SPARE}", tip) + result = validate(holder, "--no-members") + assert result.returncode == 1, result.stderr + result.stdout + (gitlink,) = findings(result) + assert gitlink.startswith(GITLINK_SAID.format(path=f"MEMBERS/{SPARE}", + commit=tip)) + + +def test_a_removed_row_is_named_by_its_gitlink_and_its_section(holder): + """A row deleted by hand leaves the member's gitlink and its section, + and both are found; putting the row back, as both say, clears them.""" + text = (holder / "family.yaml").read_bytes() + rows = manifest(holder)["members"] + irss = next(row for row in rows if row["project"] == "IRSS") + family_module().Family.open(holder).write_members( + [row for row in rows if row["project"] != "IRSS"]) + result = validate(holder, "--no-members") + assert result.returncode == 1, result.stderr + result.stdout + assert codes(result) == ["member-gitlink-unnamed", + "gitmodules-path-unnamed"] + assert findings(result)[0].startswith(GITLINK_SAID.format( + path="members/IRSS", commit=irss["pin"]["commit"]) + GITLINK_REMEDY) + (holder / "family.yaml").write_bytes(text) + assert_passes_in_every_mode(holder, 2) + + +@pytest.mark.parametrize("row", ["b", "c"]) +def test_a_sections_removal_run_as_printed_clears_it(holder, tmp_path, row): + """The commands an unnamed section prints, typed exactly as printed, + take every key out, and the holder passes again.""" + name, path = (("members/Old", "members/Old") if row == "b" else + ("ghost", "Members/IRRS")) + with_section(holder, tmp_path, name, path, "/nowhere/x.git") + (section,) = findings(validate(holder, "--no-members")) + take_out_as_printed(holder, section) + assert name not in (holder / ".gitmodules").read_text(encoding="utf-8") + assert_passes_in_every_mode(holder, 2) + + +def test_an_index_git_cannot_list_is_a_refusal_not_a_pass(holder): + """The gitlink check is the one reader of `members/` that `--no-members` + runs, so an index git cannot list is a question it cannot answer: a + refusal, exit 2, never an implicit pass.""" + (holder / ".git" / "index").write_bytes(b"not an index\n") + result = validate(holder, "--no-members") + assert result.returncode == 2, result.stderr + result.stdout + assert result.stderr.startswith( + "REFUSED gitlinks-unreadable: the index could not be listed, so the " + "gitlinks it records in members/ could not be read: `git ls-files " + "-s -z` in "), result.stderr + assert result.stderr.splitlines()[-1] == ( + "Remediation: git status in this repository says what is wrong with " + "its index. Mend that, then run this again.") + + +# --- a path that folds to a row's: only a second one is taken out ------------ + +def url_of(holder, project: str) -> str: + """The url `family.py add` wrote in `project`'s section.""" + return git("config", "-f", ".gitmodules", "--get", + f"submodule.members/{project}.url", + cwd=holder).stdout.strip() + + +@MODES +def test_a_row_hand_edited_into_a_case_variant_of_its_mount_is_the_edit( + holder, extra): + """STATE (i), REVIEW SHOULD-1: IRSS's row says `path: Members/IRSS` and + the real mount is `members/IRSS`. The mount is the canonical path and the + ROW is the hand edit, so each finding says that and names no command: the + `git rm --cached` and the `--unset-all` lines these used to print took + the member's own gitlink and section out, after which `--no-members` said + `family ok` over a family with one member. Members checked, `member-path` + names the edit beside them; `--no-members` never runs it, which is why + the finding must say it.""" + original = (holder / "family.yaml").read_bytes() + hand_edit(holder, "Members/IRSS") + commit_ = pinned(holder, "IRSS") + result = validate(holder, *extra) + assert result.returncode == 1, result.stderr + result.stdout + assert "REFUSED" not in result.stderr, result.stderr + assert codes(result) == ( + ["member-gitlink-unnamed", "gitmodules-path-unnamed"] + + ([] if extra == ("--no-members",) else ["member-path"])) + said = dict(path="members/IRSS", project="IRSS", row_path="Members/IRSS") + gitlink, section = findings(result)[:2] + assert gitlink == GITLINK_SAID.format( + path="members/IRSS", commit=commit_) + ROW_EDITED.format( + what="gitlink", **said) + assert section == SECTION_SAID.format( + path="members/IRSS", name="members/IRSS", + url=f"url {url_of(holder, 'IRSS')}") + ROW_EDITED.format( + what="section", **said) + assert "git rm" not in result.stderr, result.stderr + assert "--unset-all" not in result.stderr, result.stderr + # Said and done: the path put back in the row clears all three. + (holder / "family.yaml").write_bytes(original) + assert_passes_in_every_mode(holder, 2) + + +@MODES +def test_a_member_recorded_only_in_another_case_says_so_and_prints_no_command( + holder, extra): + """STATE (ii), REVIEW SHOULD-1: IRSS's row is canonical, and the only + gitlink the holder records for it is at `members/irss`. `git rm --cached` + of that took the member's only record out, and `--no-members` then said + `family ok` over a holder that records one gitlink fewer than it has rows. + It says the member is recorded in the wrong case instead, and sends the + operator to git's history, as every other unnamed gitlink does. + + Members checked, the row's own gitlink is read at its exact path, finds + none and is the refusal `member-gitlink-absent`, which prints no finding, + so only `--no-members` shows the finding; a git that matched the pathspec + in any case would find the variant and print it, so either is accepted + there, and what no run may do is print a command.""" + named = record_in_another_case(holder, "IRSS", "members/irss") + result = validate(holder, *extra) + assert "git rm" not in result.stderr + result.stdout + assert "--unset-all" not in result.stderr + result.stdout + if result.returncode == 2: + assert extra != ("--no-members",), result.stderr + assert result.stderr.startswith( + "REFUSED member-gitlink-absent: no gitlink (mode 160000) " + "recorded at 'members/IRSS'."), result.stderr + return + assert result.returncode == 1, result.stderr + result.stdout + (gitlink,) = findings(result) + assert gitlink == GITLINK_SAID.format( + path="members/irss", commit=named) + WRONG_CASE.format( + row_path="members/IRSS", project="IRSS", what="gitlink") + + +@MODES +def test_a_sections_only_path_in_another_case_says_so_and_prints_no_command( + holder, extra): + """The section's own state (ii): IRSS's row is canonical, its gitlink is + where it belongs, and the ONLY section that registers it gives + `members/irss`. `--unset-all` of that left the member with no section.""" + url = url_of(holder, "IRSS") + with_path_replaced(holder, b"\tpath = members/IRSS\n", + b"\tpath = members/irss\n") + result = validate(holder, *extra) + assert result.returncode == 1, result.stderr + result.stdout + (section,) = findings(result) + assert section == SECTION_SAID.format( + path="members/irss", name="members/IRSS", url=f"url {url}" + ) + WRONG_CASE.format(row_path="members/IRSS", project="IRSS", + what="section") + assert "--unset-all" not in result.stderr, result.stderr + + +def test_a_third_spelling_beside_an_edited_row_prints_no_command_either( + holder): + """The row is hand-edited AND a gitlink sits at a third spelling, + `MEMBERS/IRSS`: neither the row's path nor the canonical one. Nothing here + says it is a second path to a member that is standing, so it is not told + to `git rm --cached`; the row is wrong first.""" + hand_edit(holder, "Members/IRSS") + stage_gitlink(holder, "MEMBERS/IRSS", pinned(holder, "IRSS")) + result = validate(holder, "--no-members") + assert result.returncode == 1, result.stderr + result.stdout + third = next(f for f in findings(result) + if f.startswith("FINDING member-gitlink-unnamed: MEMBERS/")) + assert commands(third) == [] + assert "The member is recorded in the wrong case, so no command" in third + assert "git rm" not in result.stderr, result.stderr + + +def test_two_sections_for_a_hand_mounts_path_each_keep_their_commands( + family, holder, tmp_path): + """`git rm` of a gitlink takes out ONE section for its path, the last the + file gives, so the `git rm` finding cannot say it takes out both: where a + path is given twice (`gitmodules-path-twice`), each section keeps its own + `--unset-all` lines. The last assertion is the premise: after the `git + rm` the finding prints, one of the two sections is still there.""" + _, tip = spare_commits(family) + path = f"members/{SPARE}" + stage_gitlink(holder, path, tip) + with_section(holder, tmp_path, path, path, f"/nowhere/{SPARE}.git") + with_section(holder, tmp_path, "second", path, "/nowhere/second.git") + result = validate(holder, "--no-members") + assert result.returncode == 1, result.stderr + result.stdout + assert codes(result) == ["gitmodules-path-twice", "member-gitlink-unnamed", + "gitmodules-path-unnamed", + "gitmodules-path-unnamed"] + first, second = findings(result)[2:] + assert commands(first) == removal(path, "path", "url") + assert commands(second) == removal("second", "path", "url") + assert first.splitlines()[0].endswith(SECTION_REMEDY) + assert "takes this section out too" not in result.stderr + git(*shlex.split(commands(findings(result)[1])[0])[1:], cwd=holder) + listed = git("config", "-f", ".gitmodules", "--list", cwd=holder).stdout + assert listed.count(f".path={path}") == 1, listed + + +# --- a conflicted index records no gitlink at stage 0 ------------------------ + +def test_a_gitlink_held_only_at_conflict_stages_is_not_recorded(family, + holder): + """`_member_gitlinks` reads the index at STAGE 0, as `recorded_gitlink` + does: a conflicted merge holds a path at stages 1, 2 and 3, and none of + them is a commit anybody is about to make. A path HEAD never recorded and + the index holds only so is no gitlink the holder records.""" + _, tip = spare_commits(family) + stage_unmerged_gitlink(holder, f"members/{SPARE}", tip) + listed = git("ls-files", "-s", "--", f"members/{SPARE}", + cwd=holder).stdout.split() + assert listed[2::4] == ["1", "2", "3"], listed + assert_passes_in_every_mode(holder, 2) + + +def test_a_gitlink_head_records_is_head_s_when_the_index_holds_it_unmerged( + family, holder): + """The fallback to HEAD, as `recorded_gitlink` falls back: HEAD's gitlink + is the one named, and no stage of the conflict is, whichever came last.""" + older, newer = spare_commits(family) + path = f"members/{SPARE}" + stage_gitlink(holder, path, older) + commit(holder, "A gitlink no row names") + stage_unmerged_gitlink(holder, path, newer) + result = validate(holder, "--no-members") + assert result.returncode == 1, result.stderr + result.stdout + (gitlink,) = findings(result) + assert gitlink.startswith(GITLINK_SAID.format(path=path, commit=older)) + + +# --- the add remedy quotes the member it names ------------------------------- + +@pytest.mark.parametrize("project,quoted", [ + ("My Proj", "/'My Proj'"), + ("O'Brien", "/'O'\"'\"'Brien'")], ids=["space", "quote"]) +def test_the_add_remedy_quotes_the_member_it_names(family, holder, project, + quoted): + """`--member` is quoted as the paths beside it are, its project alone: + `` is the operator's to fill in, and quoted whole it would be a + literal org of that name. Split as a POSIX shell splits it, the value is + the org and the project as written.""" + _, tip = spare_commits(family) + stage_gitlink(holder, f"members/{project}", tip) + result = validate(holder, "--no-members") + assert result.returncode == 1, result.stderr + result.stdout + (gitlink,) = findings(result) + rm, add = commands(gitlink) + assert add.endswith(f" --member {quoted}"), add + assert shlex.split(add)[-1] == f"/{project}" + assert shlex.split(rm)[-1] == f"members/{project}" + + +# --- the remedy, run as printed, from a real mount --------------------------- + +def test_a_hand_mount_is_taken_out_and_added_back_as_printed(family, holder): + """ROW (a) FOR REAL, and the gitlink's remedy run as printed. `git rm` + takes the checkout and its section out; `family.py add`, run as printed + with the org filled in, then refuses `member-git-dir-cached` for the + object store `git rm` leaves, as the remedy says it will name; with that + gone, the same `add` mounts and pins the member with its row, and the + holder passes in every mode. + + `git rm --cached`, which the issue proposed, is NOT the remedy: it + leaves the checkout at the mount path, and `add` refuses that as + `member-path-taken` before it gets as far as the store.""" + git("-c", "protocol.file.allow=always", "submodule", "add", "-q", "--", + str(family["remotes"] / f"{SPARE}.git"), f"members/{SPARE}", + cwd=holder) + commit(holder, "Mount IRQS by hand") + result = validate(holder) + assert result.returncode == 1, result.stderr + result.stdout + assert codes(result) == ["member-gitlink-unnamed", + "gitmodules-path-unnamed"] + rm, add = commands(findings(result)[0]) + assert commands(findings(result)[1]) == [] + git(*shlex.split(rm)[1:], cwd=holder) + assert not (holder / "members" / SPARE).exists() + assert f"members/{SPARE}" not in (holder / ".gitmodules").read_text( + encoding="utf-8") + argv = [arg.replace("", ORG) for arg in shlex.split(add)] + assert argv[:3] == [PYTHON, "scripts/family.py", "add"], add + + def add_as_printed(): + return run_script(FAMILY, *argv[2:], "--local-remote-dir", + str(family["remotes"])) + + refused = add_as_printed() + assert refused.returncode == 2, refused.stdout + refused.stderr + assert refused.stderr.startswith("REFUSED member-git-dir-cached: ") + rmtree(holder / ".git" / "modules" / "members" / SPARE) + added = add_as_printed() + assert added.returncode == 0, added.stdout + added.stderr + assert git("status", "--porcelain", cwd=holder).stdout == "" + assert_passes_in_every_mode(holder, 3) + + +# --- a holder whose rows name everything ------------------------------------- + +@MODES +def test_a_holder_whose_rows_name_every_gitlink_and_section_passes(holder, + extra): + """THE CONTROL: the family `init` and `add` made, two members, every + gitlink and every section named by a row.""" + result = validate(holder, *extra) + assert result.returncode == 0, result.stderr + result.stdout + assert findings(result) == [] + assert "family ok: InkRouter (inkrouter), 2 member(s)" in result.stdout + + +def test_a_member_added_by_family_py_is_named(family, holder): + """And one more member, added the way a member is added: its gitlink and + its section are its row's, so nothing is unnamed.""" + added = run_script(FAMILY, "add", "--family-root", str(holder), + "--member", f"{ORG}/{SPARE}", "--local-remote-dir", + str(family["remotes"])) + assert added.returncode == 0, added.stdout + added.stderr + assert_passes_in_every_mode(holder, 3) diff --git a/tests/test_validate_family_without_git.py b/tests/test_validate_family_without_git.py new file mode 100644 index 00000000..1ca0e9ca --- /dev/null +++ b/tests/test_validate_family_without_git.py @@ -0,0 +1,183 @@ +# SPDX-License-Identifier: Apache-2.0 +"""`validate-family.py` with no git to run refuses `git-not-runnable`, exit +2, in every mode, naming the check that could not run it (#368). + +`git_out` raises `OSError` (`FileNotFoundError`) when there is no git to +start, and nothing in the validator caught it outside #317's one `.gitmodules` +reader: the default and `--pins` modes ended in a traceback, which exits 1, +a FINDING's code, though nothing was found. The docstring's own exit codes +make 1 "drift a reviewer can act on" and 2 "the question could not be +asked ... never an implicit pass". + +Caught IN THE VALIDATOR, around every check that runs git (#317's +`.gitmodules` reader, #361's gitlink listing, and each member's gitlink and +tree), never in `git_out`, and never around a whole check: a +`project.yaml` nobody may read is an `OSError` too. ONE code for the +validator, whichever check met the missing git first; `gitmodules-unreadable` +keeps meaning a file git ran and could not read. And `recorded_gitlink`'s +`except Refusal` does not turn the missing git into "no gitlink": a +`FileNotFoundError` is no refusal and passes straight through it. + +THE HOLDER'S OWN VALIDATOR, RUN IN THIS PROCESS, with git failing as a +missing program does, as `tests/test_family_add_refusals.py`'s no-git test +runs it: no PATH is changed for anything else, so each test means the same +on every OS. Two names reach git: the validator's own `git_out`, and the +one `recorded_gitlink` and `tree_digest` call inside `repo_shape`, so each +test says which of them fails, and where. +""" + +from __future__ import annotations + +import importlib.util +import sys +from pathlib import Path + +import pytest + +from test_family import family, holder # noqa: F401 + +#: The remediation every `git-not-runnable` refusal ends with. +REMEDIATION = ( + "Remediation: put git on PATH and run this again. This check reads the " + "holder the way git does, and nothing could clone it or fetch its " + "members without git either.") + +MODES = pytest.mark.parametrize( + "extra", [(), ("--pins",), ("--no-members",)], + ids=["members-checked", "pins", "no-members"]) +MEMBER_MODES = pytest.mark.parametrize( + "extra", [(), ("--pins",)], ids=["members-checked", "pins"]) + + +def holder_validator(holder, monkeypatch): + """The holder's `scripts/validate-family.py`, loaded as a module. It + puts its own `scripts/` on `sys.path`, which is put back afterwards.""" + monkeypatch.setattr(sys, "path", list(sys.path)) + spec = importlib.util.spec_from_file_location( + "holder_validate_family_without_git", + holder / "scripts" / "validate-family.py") + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def failing(error: OSError, real=None, *, outside: Path | None = None): + """A `git_out` that raises `error` as a program that will not start + does. With `outside`, only where git runs anywhere but there (in a + member, for instance), and `real` everywhere else.""" + def git_out(args, cwd, binary=False, env=None): + if outside is not None and Path(cwd).resolve() == outside.resolve(): + return real(args, cwd, binary=binary, env=env) + raise error + return git_out + + +def without_git(module, monkeypatch, *, validator: bool, helpers: bool, + error: OSError | None = None, + outside: Path | None = None) -> None: + """Make git fail for the validator's own calls (`validator`), for the + ones `recorded_gitlink` and `tree_digest` make in `repo_shape` + (`helpers`), or both.""" + error = error or FileNotFoundError(2, "No such file or directory", "git") + real = module.git_out + fake = failing(error, real, outside=outside) + if validator: + monkeypatch.setattr(module, "git_out", fake) + if helpers: + monkeypatch.setattr(sys.modules[module.recorded_gitlink.__module__], + "git_out", fake) + + +def run(module, holder, capsys, *extra) -> tuple[int, list[str]]: + code = module.main(["--root", str(holder), *extra]) + out, err = capsys.readouterr() + assert "Traceback" not in out + err + return code, err.splitlines() + + +@MODES +@pytest.mark.parametrize("error", [ + FileNotFoundError(2, "No such file or directory", "git"), + PermissionError(13, "Permission denied", "git")], + ids=["no-git", "git-not-executable"]) +def test_no_git_at_all_is_one_refusal_in_every_mode(holder, monkeypatch, + capsys, extra, error): + """THE HOLDER AS `add` LEFT IT: `.gitmodules` is read first, so that is + the check named. Exit 2 and the one code in every mode, where #317 said + `gitmodules-unreadable`, a code that now means a file git ran and could + not read.""" + module = holder_validator(holder, monkeypatch) + without_git(module, monkeypatch, validator=True, helpers=True, + error=error) + code, err = run(module, holder, capsys, *extra) + assert code == 2, err + assert err == [ + "REFUSED git-not-runnable: .gitmodules could not be read as git " + f"reads it: git could not be run ({error.strerror})", REMEDIATION] + + +@MODES +def test_with_no_gitmodules_the_gitlink_listing_is_named(holder, monkeypatch, + capsys, extra): + """THE ISSUE'S RESIDUE: a holder with member rows and its `.gitmodules` + gone from the working tree, where main still ended in the traceback in + the default and `--pins` modes. The first check to run git is now #361's + listing of `members/`, which every mode runs, `--no-members` included: + where it passed before, having run no git at all, it now refuses.""" + (holder / ".gitmodules").unlink() + module = holder_validator(holder, monkeypatch) + without_git(module, monkeypatch, validator=True, helpers=True) + code, err = run(module, holder, capsys, *extra) + assert code == 2, err + assert err == [ + "REFUSED git-not-runnable: the gitlinks this repository records in " + "members/ could not be listed: git could not be run (No such file " + "or directory)", REMEDIATION] + + +@MEMBER_MODES +def test_a_members_gitlink_that_git_cannot_read_is_named_not_absent( + holder, monkeypatch, capsys, extra): + """GIT FAILS ONLY IN `recorded_gitlink`, so every check before the + members' runs, and the first member's gitlink is the check named. Were + the missing git read as "no gitlink here", this would say + `member-gitlink-absent` and tell the operator to `add` a member that is + mounted.""" + module = holder_validator(holder, monkeypatch) + without_git(module, monkeypatch, validator=False, helpers=True) + code, err = run(module, holder, capsys, *extra) + assert code == 2, err + assert err == [ + "REFUSED git-not-runnable: the gitlink recorded at members/IRRS " + "could not be read: git could not be run (No such file or " + "directory)", REMEDIATION] + + +def test_no_members_runs_no_member_check_that_needs_git(holder, monkeypatch, + capsys): + """And `--no-members`, which reads no member, passes with the same git: + the refusal is the check's, not the mode's.""" + module = holder_validator(holder, monkeypatch) + without_git(module, monkeypatch, validator=False, helpers=True) + code = module.main(["--root", str(holder), "--no-members"]) + out, err = capsys.readouterr() + assert code == 0, out + err + assert "family ok: InkRouter (inkrouter), 2 member(s)" in out + + +@MEMBER_MODES +def test_a_members_tree_that_git_cannot_read_is_named(holder, monkeypatch, + capsys, extra): + """GIT RUNS IN THE HOLDER AND NOT IN A MEMBER, so the gitlink is read + and the pinned commit and its digest, read inside the member, are the + check named.""" + module = holder_validator(holder, monkeypatch) + without_git(module, monkeypatch, validator=True, helpers=True, + outside=holder) + code, err = run(module, holder, capsys, *extra) + assert code == 2, err + assert err == [ + "REFUSED git-not-runnable: IRRS's pinned commit and its tree digest " + "could not be read in members/IRRS: git could not be run (No such " + "file or directory)", REMEDIATION] +