diff --git a/scripts/family.py b/scripts/family.py index 88125222..583037b2 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -264,13 +264,81 @@ def _member_lines(row: dict) -> list[str]: ] +def _member_path(project: str, row: dict | None = None) -> str: + """The ONE path a member has, `members/` (#299); given the + member's row, that path or the refusal that says the row names another. + + THE TOOL WRITES `path:` ITSELF, as this and nothing else (`_member_row`), + and `bump` and `remove` used to read it back and hand it to git as it + stood. Git reads a path as a pathspec: `path: members/*` made `remove + --member Atlas` exit 0 with one commit that unmounted EVERY member while + dropping only Atlas's row, and `path: .` made it `git rm -r -f` the + holder's own files and die on the missing manifest. A hand edit is outside + the supported path (AGENTS.md: do not hand-edit the `members:` block), and + the rule this asks is the one `validate-family.py` already records as + `member-path`; but the validator runs after a commit, in CI and in the + doctor, never in front of these two verbs, so they ask it themselves, + BEFORE ANY GIT CALL AND ANY WRITE. + + A row with no `path:` keeps defaulting to it, as it always has, and the + comparison is EXACT, as the validator's is: `members/IRRS/`, `members/irrs` + and a neighbour's path are each a different string and each refused. The + validator keeps its own spelling of this rule on purpose. It ships INTO + every holder, as `repo_shape.py` does, so sharing it would move every + holder's pin for a refusal of a hand edit. + + A ROW'S PROJECT HAS TO BE A DIRECTORY NAME TOO. Its `path:` is checked + against `members/`, so a row that says `project: ..` and `path: + members/..` is EQUAL to the expected path and passed the comparison, and + `remove` then ran `git rm -r -f -- members/..` over the holder's root, + while `project: .` and `path: members/.` unmounted every member in one + commit naming one (#299). Each takes two hand edits, the project and the + path, and `add` cannot write either: `_checked_member` refuses a `.` or + `..` segment as `member-malformed` before the member is read, so + `_member_row` is never reached with one. A project that + does not make `members/` ONE directory directly under `members/` + has no path of its own to be compared with, so it is refused here, under + the same code, only when a row is given: what `add` writes (`_member_row`, + no row) is as it was. + """ + expected = f"{MEMBERS_DIR}/{project}" + if row is not None and (project in ("", ".", "..") or "/" in project): + names = {".": f"the {MEMBERS_DIR} directory itself", + "..": "the holder's root"}.get( + project, f"no single directory directly under {MEMBERS_DIR}/") + raise Refusal( + "member-path", + f"{project!r} is not a project name: its path would be " + f"{expected!r}, which is {names}, not a member. Acting on it " + "would unmount more than one member or remove the holder's own " + "files, so no git command was run and nothing was written.", + "Remediation: this tool never writes such a row, so it is a hand " + f"edit. Restore {MANIFEST} from git (`git checkout -- " + f"{MANIFEST}` if the edit is not committed), or put the row's " + "`project:` and `path:` back as the tool wrote them " + f"(`project: `, `path: {MEMBERS_DIR}/`), then " + "run the command again naming that member.") + declared = expected if row is None else str(row.get("path") or expected) + if declared != expected: + raise Refusal( + "member-path", + f"{project}: path is {declared!r} in {MANIFEST}, expected " + f"{expected!r}. Git would read that value as a pathspec, so no " + "git command was run and nothing was written.", + "Remediation: this tool writes that field itself, so a different " + f"value is a hand edit. Put `path: {expected}` back in " + f"{MANIFEST}, or `git checkout -- {MANIFEST}` if the edit is not " + "committed, then run the same command again.") + return expected + + def _member_row(project: str, project_id: str, repository: str, commit: str, digest: str) -> dict: return { "project": project, "id": project_id, "repository": repository, - "path": f"{MEMBERS_DIR}/{project}", + "path": _member_path(project), "pin": {"revision_kind": "commit", "commit": commit, "tree_sha256": digest}, } @@ -291,6 +359,32 @@ def _checked_member(raw: str) -> tuple[str, str]: "Remediation: name the owner — a family's members may live in " "another organisation, so there is no owner to assume. For " "`remove` and `bump`, the bare `` is what is wanted.") + # A SEGMENT THAT IS `.` OR `..` IS A DIRECTORY REFERENCE, NOT A NAME + # (#299). `MEMBER_RE`'s class admits the dot, for `.github`, so `/..` + # matched, and its mount path `members/..` is the holder's root: on a + # fresh holder `_checked_mount_path` found the root's own files "in the + # index" under it and told the human to `git rm -r --cached -- members/..`, + # which stages the deletion of every file the holder has. The class is + # left as it is; only the two names that are no name are refused, before + # the member is read or anything is mounted. An owner of `.` or `..` + # names no account. + owner, _, project = name.partition("/") + for which, segment in (("owner", owner), ("project", project)): + if segment not in (".", ".."): + continue + if which == "owner": + why = "no organisation or account is called that" + else: + where = ("the members directory itself" if segment == "." + else "the holder's root") + why = f"it would mount at {MEMBERS_DIR}/{segment}, {where}" + raise Refusal( + "member-malformed", + f"--member {raw!r} has `{segment}` as its {which}, which is a " + f"directory reference and not a name: {why}", + "Remediation: name the repository as `/`, each a " + "real name; `.` and `..` are neither, though a name may contain " + "a dot (`.github`).") checked_value("--member", name) return name, repo_basename(name) @@ -425,12 +519,47 @@ def _commit_one(root: Path, message: str, paths: list[str], scaffold and `update-shape.py` share, so it is called here rather than spelled a second time; `bump`'s re-stage before its nothing-to-record check is the same call, for the same reason. + + THE COMMIT READS THEM LITERALLY TOO (#299). The stage above was literal + and `git commit -- ` was not, so a path that is a glob to git + committed whatever else it matched: with `members/*` named, the move of + every OTHER member's gitlink somebody had staged went into this commit + beside the one it was for. `bump` and `remove` hand it a path that is + always `members/` (`_member_path`); the magic is the guard that + does not depend on that being so. + + PER-PATHSPEC `:(literal)`, NOT THE GLOBAL `--literal-pathspecs`, ON THIS + ONE CALL. The global option works by putting `GIT_LITERAL_PATHSPECS=1` in + git's environment, and every child of `git commit` inherits it: the + holder's `pre-commit`, `commit-msg` and `post-commit` hooks, its own or the + operator's `core.hooksPath`. A hook that picks the staged files with a + glob (`git diff --cached --name-only -- '*.yaml'`) then matched nothing, + and a linter behind it checked nothing, on every `add`, `bump` and + `remove` commit. The magic is spelled on each path and reaches no hook. It + is right for a path `git rm` took out of the index and the tree, which the + commit still matches in HEAD. `deinit`, `rm` and `diff` run no COMMIT + hooks, so they keep the global option (`rm`, like `force_stage`'s `add`, + writes the index, so a `post-index-change` hook still sees the variable + after them; that is theirs and predates this). + + THE CALLER'S OWN `GIT_LITERAL_PATHSPECS` IS DROPPED FROM THIS ONE COMMIT'S + ENVIRONMENT. The variable turns pathspec magic OFF, and `:(literal)` is + magic: with it exported (a safety default, or a `family.py` run from a + hook or alias of a `git --literal-pathspecs` command, which exports it to + its children) git took `:(literal)family.yaml` as a file of that name, + found none, and every `add`, `bump` and `remove` commit failed, `bump` and + `remove` leaving the pin or the unmount staged with no rollback. The + commit's paths are literal by their magic and need no help from the + environment. Only this variable: the other pathspec variables turn magic + ON or change only how a glob matches, and the magic still reads. """ force_stage(root, paths if add is None else add) message, trailer_args = commit_trailers(message, trailers) - args = ["git", "commit", "-q", "-F", "-", *trailer_args, "--", *paths] + args = ["git", "commit", "-q", "-F", "-", *trailer_args, "--", + *[f":(literal){path}" for path in paths]] check_program(args) env = dict(os.environ) + env.pop("GIT_LITERAL_PATHSPECS", None) for key, fallback in (("GIT_AUTHOR_NAME", "openRepoShape family"), ("GIT_COMMITTER_NAME", "openRepoShape family"), ("GIT_AUTHOR_EMAIL", "family@openreposhape.invalid"), @@ -1620,7 +1749,7 @@ def cmd_add(args) -> int: """ family = Family.open(args.family_root) repository, project = _checked_member(args.member) - path = f"{MEMBERS_DIR}/{project}" + path = _member_path(project) at = _checked_add_target(family, repository, project, path, args.at) url = _member_url(repository, args.local_remote_dir) @@ -1681,10 +1810,15 @@ def _nothing_to_record(root: Path, paths: list[str]) -> bool: its HEAD is. The caller has staged the gitlink first, as `_commit_one` would, so a stray one in the index is not a difference. Exit 1 is git's "there is a difference"; any other failure is a real one and propagates. + + THE PATHS ARE TAKEN LITERALLY (`--literal-pathspecs`, #299), as the + commit this answers for takes them: a glob here would find the + difference in a neighbouring member and answer "something to record" for + a member that has nothing, so the commit would go ahead. """ try: - run(["git", "diff", "--quiet", "--ignore-submodules=dirty", "HEAD", - "--", *paths], cwd=root) + run(["git", "--literal-pathspecs", "diff", "--quiet", + "--ignore-submodules=dirty", "HEAD", "--", *paths], cwd=root) except CommandFailed as exc: if exc.code == 1: return False @@ -1710,7 +1844,9 @@ def cmd_bump(args) -> int: f"--to {args.to!r} is not a 40-hex commit", "Remediation: pass the full 40 characters. A tag can be moved and " "a commit cannot; a pin is a commit or it is nothing.") - path = str(row.get("path") or f"{MEMBERS_DIR}/{project}") + # THE ROW'S `path:` IS CHECKED, NOT TRUSTED (#299): `member` below, the + # fetch and checkout in it, and every pathspec after them are this path. + path = _member_path(project, row) member = family.root / path if not (member / ".git").exists(): raise Refusal( @@ -1799,13 +1935,24 @@ def cmd_remove(args) -> int: "something that is not a member is a no-op worth refusing, " "because the argument is usually a typo for something that is.") project = str(row["project"]) - path = str(row.get("path") or f"{MEMBERS_DIR}/{project}") + # THE ROW'S `path:` IS CHECKED, NOT TRUSTED (#299), before the first git + # call: `path: members/*` unmounted every member under one commit naming + # one, and `path: .` removed the holder's own files. + path = _member_path(project, row) # THE MEMBER REPOSITORY IS NOT TOUCHED. Removing a member unmounts it from # this holder and nothing else: the project keeps its name, its history, # its legs and its gate. A family that could delete a project by dropping # a row would be a family that confers something. - run(["git", "submodule", "deinit", "-f", "-q", "--", path], cwd=family.root) - run(["git", "rm", "-r", "-q", "-f", "--", path], cwd=family.root) + # + # BOTH CALLS TAKE THE PATH LITERALLY (`--literal-pathspecs`, #299), the + # global option, which `git submodule` honours too: the path is always + # `members/` here, so this is defence in depth that does not + # depend on the row being right. Without it a glob in the path would + # unmount and remove every member it matched. + run(["git", "--literal-pathspecs", "submodule", "deinit", "-f", "-q", + "--", path], cwd=family.root) + run(["git", "--literal-pathspecs", "rm", "-r", "-q", "-f", "--", path], + cwd=family.root) family.write_members([r for r in family.members if str(r.get("project")) != project]) head = _commit_one( diff --git a/tests/test_family_add_refusals.py b/tests/test_family_add_refusals.py index 0d1b7627..c46759b0 100644 --- a/tests/test_family_add_refusals.py +++ b/tests/test_family_add_refusals.py @@ -732,7 +732,8 @@ def test_a_commit_that_fails_unwinds_the_mount_and_the_pin(family, holder, f"mounting {ORG}/{SPARE} at members/{SPARE}\n") assert result.stderr.splitlines() == [ f"REFUSED committing {SPARE} to the family. THE EXACT COMMAND WAS:", - f" git commit -q -F - -- .gitmodules members/{SPARE} family.yaml", + " git commit -q -F - -- :(literal).gitmodules " + f":(literal)members/{SPARE} :(literal)family.yaml", f" (in {holder.resolve()})", " exit 1", "--- output ---", diff --git a/tests/test_family_member_path.py b/tests/test_family_member_path.py new file mode 100644 index 00000000..c9f72759 --- /dev/null +++ b/tests/test_family_member_path.py @@ -0,0 +1,672 @@ +# SPDX-License-Identifier: Apache-2.0 +"""`family.py bump` and `remove` act on the one path a member can have, or +refuse (#299). + +Both verbs took the member's path from the manifest row's `path:` and handed +it to git as a PATHSPEC, with no check and no `--literal-pathspecs`. The tool +writes that field itself, as `members/` and nothing else, so a +different value is a hand edit; and a hand edit to `path: members/*` made +`remove --member Atlas` exit 0 with ONE commit that unmounted every member +while dropping only Atlas's row, and `path: .` made it `git rm -r -f` the +holder's own files and die on the missing manifest. `validate-family.py` has a +rule for it (`member-path`), but it runs after a commit, never in front of +these two verbs. So they refuse `member-path` themselves, before any git call +and any write; and every git call that takes the path takes it literally (the +global `--literal-pathspecs` on `deinit`, `rm` and `diff`; per-pathspec +`:(literal)` on the commit, which runs the holder's hooks and must not hand +them `GIT_LITERAL_PATHSPECS`), which does not depend on the row being right. + +THE ISSUE'S MEMBERS ARE `Atlas` AND `Borg`; this family's are `IRRS` and +`IRSS`, so `IRRS` is the row a test edits and `IRSS` is the neighbour that must +be left as it was. The same family, rules and offline environment as +`test_family.py`, `test_family_add_refusals.py` and +`test_family_add_excludes.py`: members are scaffolded into local bare +repositories, nothing is created on GitHub, a `gh` on PATH logs and exits 97, +and the operator's git configuration is a file of the test's own +(`GIT_CONFIG_GLOBAL`, `GIT_CONFIG_NOSYSTEM`). No test pushes to the fixture's +own remotes. + +THE ROW'S PROJECT HAS TO BE A DIRECTORY NAME TOO. A row hand-edited to +`project: .` and `path: members/.`, or `project: ..` and `path: members/..`, +IS `members/` and passes the comparison above; `remove --member .` +then unmounted every member in one commit naming one, and `--member ..` `git +rm -r -f`'d the holder's root and died on the missing manifest. The same +`member-path` refusal covers them (the second commit of #299). + +A refusal is asked of the holder the way `test_family_add_refusals.py` asks it +of `add`: not "is the sentence printed" but "is everything exactly as it was", +down to the bytes of `git ls-files -s -z` and `git status --porcelain=v1 -z`, +with the hand edit itself still sitting uncommitted in the work tree. +""" + +from __future__ import annotations + +import os +import shutil +import subprocess +import sys +from pathlib import Path + +import pytest + +from conftest import git, run_script +#: The family fixture, its names and the offline environment, imported from +#: the modules that own them rather than rebuilt: a holder made any other way +#: would test a different holder. +from test_family import ( # noqa: F401 + FAMILY, MEMBERS, NAME, ORG, SPARE, commit_in_member, family, + family_module, manifest, validate, +) +from test_family_add_excludes import empty_holder # noqa: F401 +from test_family_add_refusals import ( # noqa: F401 + holder, offline, refusal, snapshot, +) +from test_scaffold_ignored_shape_files import excluding + +#: The row a test edits, and the neighbour that must come out untouched. +MEMBER, OTHER = MEMBERS +GLOB = "members/*" + +#: What a hand edit can leave in `path:`. Each is a string `validate-family.py` +#: refuses as `member-path`, and each is one a verb used to act on: a glob +#: over every member, the holder itself, a NEIGHBOUR'S path (no glob needed to +#: unmount the wrong member), the member's own path with a trailing slash +#: (git reads it as the same path) and in another case (a case-insensitive disk +#: reads it as the same path). The comparison is exact, as the validator's is. +SPELLINGS = { + "glob": GLOB, + "dot": ".", + "neighbour": f"members/{OTHER}", + "trailing-slash": f"members/{MEMBER}/", + "case": f"members/{MEMBER.lower()}", +} + +#: A name Windows refuses in a file name: `*` is one of the characters its +#: file systems reserve. Only the test that needs a directory of that name is +#: skipped there; every refusal row needs none and runs everywhere. +NO_STAR_FILES = pytest.mark.skipif( + sys.platform == "win32", + reason="Windows refuses `*` in a file name, so the directory `members/*` " + "this test makes cannot exist there; every other test in this " + "module needs no such directory and runs on every platform") + + +def isolated(offline, tmp_path: Path) -> dict: + """`offline`'s environment plus a git configuration that is the test's + own: no run here reads the global or the system configuration of whoever + runs the suite, and every commit has an identity to be made under.""" + return {**offline.env, **excluding(tmp_path / "operator", b""), + "GIT_CONFIG_NOSYSTEM": "1", + "GIT_AUTHOR_NAME": "t", "GIT_AUTHOR_EMAIL": "t@t.invalid", + "GIT_COMMITTER_NAME": "t", "GIT_COMMITTER_EMAIL": "t@t.invalid"} + + +def raw(root: Path, env: dict, *args: str, + check: bool = True) -> subprocess.CompletedProcess: + """`git` in `root` under `env`, its output as BYTES.""" + proc = subprocess.run(["git", *args], cwd=str(root), capture_output=True, + check=False, env={**os.environ, **env}) + if check: + assert proc.returncode == 0, (f"git {' '.join(args)} in {root}:\n" + f"{proc.stderr!r}") + return proc + + +def hand_edit(root: Path, project: str, path: str | None) -> None: + """`project`'s row in `family.yaml` given `path:` `path` (or none when + `path` is None), UNCOMMITTED: what a person who edits the manifest by hand + leaves behind. The manifest reads it back as typed, so a test is never + about a value the reader changed on the way.""" + manifest_path = root / "family.yaml" + text = manifest_path.read_bytes().decode("utf-8") + line = f" path: members/{project}\n" + assert text.count(line) == 1, "fixture drift: the row's own path line" + manifest_path.write_bytes(text.replace( + line, "" if path is None else f" path: {path}\n").encode("utf-8")) + row = {r["project"]: r for r in manifest(root)["members"]}[project] + assert row.get("path") == path + + +def hand_edit_project(root: Path, project: str, as_project: str) -> None: + """`project`'s row renamed to `as_project` AND given the path that name + makes, `members/`: the two hand edits that leave a row equal + to the path the tool would compute for it. UNCOMMITTED, as `hand_edit` + leaves it.""" + manifest_path = root / "family.yaml" + text = manifest_path.read_bytes().decode("utf-8") + for old, new in ( + (f" - project: {project}\n", f" - project: {as_project}\n"), + (f" path: members/{project}\n", + f" path: members/{as_project}\n")): + assert text.count(old) == 1, "fixture drift: the row's own lines" + text = text.replace(old, new) + manifest_path.write_bytes(text.encode("utf-8")) + row = {r["project"]: r for r in manifest(root)["members"]}[as_project] + assert row["path"] == f"members/{as_project}" + + +def state(root: Path, env: dict) -> dict: + """Everything a refused verb must leave exactly as it found it.""" + return { + "HEAD": raw(root, env, "rev-parse", "HEAD").stdout, + "index": raw(root, env, "ls-files", "-s", "-z").stdout, + "status": raw(root, env, "status", "--porcelain=v1", "-z").stdout, + "family.yaml": (root / "family.yaml").read_bytes(), + "the rest": snapshot(root), + } + + +def commits(root: Path, env: dict) -> int: + return int(raw(root, env, "rev-list", "--count", "HEAD").stdout) + + +def committed(root: Path, env: dict) -> set[str]: + return set(raw(root, env, "show", "--name-only", "--format=", "HEAD" + ).stdout.decode("utf-8").split()) + + +def older(family) -> str: + """A commit of `MEMBER` that is not the one the holder pins, so a bump to + it has something to record.""" + return git("rev-parse", "main~1", cwd=family["remotes"] / f"{MEMBER}.git" + ).stdout.strip() + + +def verb_args(verb: str, root: Path, family) -> list[str]: + extra = ["--to", older(family)] if verb == "bump" else [] + return [verb, "--family-root", str(root), "--member", MEMBER, *extra] + + +def neighbour_intact(root: Path, env: dict) -> None: + """The member NOT named is still mounted, pinned and checked out.""" + entry = raw(root, env, "ls-tree", "HEAD", "--", f"members/{OTHER}" + ).stdout + assert entry.startswith(b"160000 commit "), entry + assert [r["project"] for r in manifest(root)["members"]] == list(MEMBERS) + assert (root / "members" / OTHER / "project.yaml").is_file() + + +# --- the refusal ------------------------------------------------------------ + +@pytest.mark.parametrize("spelling", SPELLINGS.values(), ids=SPELLINGS) +@pytest.mark.parametrize("verb", ["remove", "bump"]) +def test_a_row_whose_path_is_not_the_members_is_refused_and_nothing_moves( + family, holder, offline, tmp_path, verb, spelling): + """THE ISSUE'S PROBES. `remove` with `path: members/*` exited 0 with ONE + commit that deleted every member's gitlink; with `path: .` it removed the + holder's own files and died with a traceback. Now: exit 2, the refusal + the validator's own finding is named for, the row's value and the + expected one in it, the remedy that puts the field back, and the holder + byte for byte as it was, the hand edit still uncommitted.""" + env = isolated(offline, tmp_path) + hand_edit(holder, MEMBER, spelling) + assert raw(holder, env, "status", "--porcelain=v1", "-z").stdout == ( + b" M family.yaml\0"), "premise: the hand edit is the only change" + before = state(holder, env) + + result = run_script(FAMILY, *verb_args(verb, holder, family), env=env) + + assert result.returncode == 2, result.stdout + result.stderr + assert result.stdout == "", "refused before anything was reported" + said, remedy = refusal(result.stderr) + assert said == ( + f"REFUSED member-path: {MEMBER}: path is {spelling!r} in family.yaml, " + f"expected 'members/{MEMBER}'. Git would read that value as a " + "pathspec, so no git command was run and nothing was written.") + assert remedy == ( + "Remediation: this tool writes that field itself, so a different " + f"value is a hand edit. Put `path: members/{MEMBER}` back in " + "family.yaml, or `git checkout -- family.yaml` if the edit is not " + "committed, then run the same command again.") + assert state(holder, env) == before + neighbour_intact(holder, env) + assert not offline.gh_log.exists(), offline.gh_log.read_text() + + +@pytest.mark.parametrize("verb", ["remove", "bump"]) +def test_the_refusal_is_made_before_any_git_call( + family, holder, offline, tmp_path, monkeypatch, capsys, verb): + """"Nothing was written" is what the holder shows; this is the other half + of the ruling, that no git command is RUN either. Run in this process with + every subprocess forbidden: a verb that reached git before the check + fails here on the call, whatever the end state says.""" + module = family_module() + hand_edit(holder, MEMBER, GLOB) + before = (holder / "family.yaml").read_bytes() + args = verb_args(verb, holder, family) + + def forbidden(argv, *rest, **named): + raise AssertionError(f"a program was run before the refusal: {argv}") + + monkeypatch.setattr(subprocess, "run", forbidden) + assert module.main(args) == 2 + err = capsys.readouterr().err + assert err.startswith("REFUSED member-path: ") + assert (holder / "family.yaml").read_bytes() == before + + +@NO_STAR_FILES +def test_bump_refuses_a_row_naming_a_directory_that_is_really_there( + family, holder, offline, tmp_path): + """THE ISSUE'S ONE WAY `bump` GOT PAST ITS OWN `member-uninitialized`: a + checkout, `.git` and all, in a directory literally named `members/*`. It + then fetched and checked that out at `--to`, and staged and committed it + as a gitlink of its own. The refusal comes before all of that: the clone + stays where it was.""" + env = isolated(offline, tmp_path) + git("clone", "-q", str(family["remotes"] / f"{MEMBER}.git"), GLOB, + cwd=holder) + star = holder / "members" / "*" + assert (star / ".git").exists(), "premise: the literal checkout is there" + tip = raw(star, env, "rev-parse", "HEAD").stdout + hand_edit(holder, MEMBER, GLOB) + before = state(holder, env) + + result = run_script(FAMILY, *verb_args("bump", holder, family), env=env) + + assert result.returncode == 2, result.stdout + result.stderr + assert refusal(result.stderr)[0].startswith("REFUSED member-path: ") + assert raw(star, env, "rev-parse", "HEAD").stdout == tip + assert state(holder, env) == before + neighbour_intact(holder, env) + + +DOT_ROWS = { + "dot": (".", "the members directory itself"), + "dotdot": ("..", "the holder's root"), +} + + +@pytest.mark.parametrize("project, names", DOT_ROWS.values(), ids=DOT_ROWS) +@pytest.mark.parametrize("verb", ["remove", "bump"]) +def test_a_row_whose_project_is_not_a_directory_name_is_refused( + family, holder, offline, tmp_path, verb, project, names): + """`project: .` with `path: members/.`, and `project: ..` with `path: + members/..`: the row's path EQUALS `members/`, so only the + project can be refused. `remove` used to exit 0 with one commit deleting + every member's gitlink for the first, and to stage the holder's own files + for deletion and die on a `FileNotFoundError` for the second. Now the + same `member-path` refusal, exit 2, and the holder byte for byte as it + was, both members still mounted, pinned and checked out.""" + env = isolated(offline, tmp_path) + hand_edit_project(holder, MEMBER, project) + assert raw(holder, env, "status", "--porcelain=v1", "-z").stdout == ( + b" M family.yaml\0"), "premise: the hand edit is the only change" + before = state(holder, env) + args = verb_args(verb, holder, family) + args[args.index(MEMBER)] = project + + result = run_script(FAMILY, *args, env=env) + + assert result.returncode == 2, result.stdout + result.stderr + assert result.stdout == "", "refused before anything was reported" + said, remedy = refusal(result.stderr) + assert said == ( + f"REFUSED member-path: {project!r} is not a project name: its path " + f"would be 'members/{project}', which is {names}, not a member. " + "Acting on it would unmount more than one member or remove the " + "holder's own files, so no git command was run and nothing was " + "written.") + assert remedy == ( + "Remediation: this tool never writes such a row, so it is a hand " + "edit. Restore family.yaml from git (`git checkout -- family.yaml` " + "if the edit is not committed), or put the row's `project:` and " + "`path:` back as the tool wrote them (`project: `, `path: " + "members/`), then run the command again naming that " + "member.") + assert state(holder, env) == before + for mounted in MEMBERS: + entry = raw(holder, env, "ls-tree", "HEAD", "--", f"members/{mounted}" + ).stdout + assert entry.startswith(b"160000 commit "), entry + assert (holder / "members" / mounted / "project.yaml").is_file() + assert [r["project"] for r in manifest(holder)["members"]] == [project, + OTHER] + assert not offline.gh_log.exists(), offline.gh_log.read_text() + + +# --- `add`: a `.` or `..` segment is not a name ----------------------------- + +#: `--member` spellings with a segment that is a directory reference, and the +#: sentence the refusal ends on: the one thing wrong with each. +DIRECTORY_REFERENCES = { + "project-dotdot": (f"{ORG}/..", "..", "project", + "it would mount at members/.., the holder's root"), + "project-dot": (f"{ORG}/.", ".", "project", + "it would mount at members/., the members directory " + "itself"), + "owner-dotdot": (f"../{SPARE}", "..", "owner", + "no organisation or account is called that"), + "owner-dot": (f"./{SPARE}", ".", "owner", + "no organisation or account is called that"), +} + + +@pytest.mark.parametrize("spelling, segment, which, why", + DIRECTORY_REFERENCES.values(), + ids=DIRECTORY_REFERENCES) +@pytest.mark.parametrize("kind", ["fresh-holder", "holder-with-members"]) +def test_add_refuses_a_dot_segment_as_malformed_before_anything_is_read( + family, offline, tmp_path, request, kind, spelling, segment, which, + why): + """`add --member /..` matched `MEMBER_RE` (its class admits the dot, + for `.github`) and its mount path `members/..` is the holder's root. On a + FRESH holder `member-path-taken` then found the root's own files in the + index under it and told the human to run `git rm -r --cached -- + members/..`, which stages the deletion of EVERY file the holder has. A + segment of `.` or `..` is a directory reference, not a name, so it is the + argument refusal `member-malformed`, before the member is read: not one + line on stdout (`checking ... before mounting it` comes first otherwise), + no clone, no `gh` call, and the holder byte for byte as it was.""" + env = isolated(offline, tmp_path) + if kind == "fresh-holder": + root = request.getfixturevalue("empty_holder") + assert not (root / "members").exists(), "premise: a fresh holder" + remotes = tmp_path / "remotes" + else: + root = request.getfixturevalue("holder") + assert (root / "members").is_dir() + remotes = family["remotes"] + before = state(root, env) + + result = run_script(FAMILY, "add", "--family-root", str(root), + "--member", spelling, "--local-remote-dir", + str(remotes), env=env) + + assert result.returncode == 2, result.stdout + result.stderr + assert result.stdout == "", "refused before the member was even read" + said, remedy = refusal(result.stderr) + assert said == ( + f"REFUSED member-malformed: --member {spelling!r} has `{segment}` as " + f"its {which}, which is a directory reference and not a name: {why}") + assert remedy == ( + "Remediation: name the repository as `/`, each a real " + "name; `.` and `..` are neither, though a name may contain a dot " + "(`.github`).") + assert state(root, env) == before + offline.assert_left_nothing() + + +def test_add_still_takes_a_name_that_contains_a_dot(): + """`MEMBER_RE`'s class is not narrowed: `.github` is a name, and only the + two directory references are refused.""" + module = family_module() + assert module._checked_member(f"{ORG}/.github") == (f"{ORG}/.github", + ".github") + assert module._checked_member(f".github/{SPARE}")[1] == SPARE + assert module._checked_member(f"{ORG}/a.b")[1] == "a.b" + + +# --- the controls ----------------------------------------------------------- + +@pytest.mark.parametrize("edit", ["as-written", "no-path-line"]) +@pytest.mark.parametrize("verb", ["remove", "bump"]) +def test_a_row_with_the_members_own_path_or_none_still_exits_zero_in_one_commit( + family, holder, offline, tmp_path, verb, edit): + """THE CONTROLS: the row the tool writes, and a row with no `path:` at + all, which keeps defaulting to `members/`. Each is still ONE + commit of exactly the paths the verb is for, and the holder still + validates.""" + env = isolated(offline, tmp_path) + if edit == "no-path-line": + hand_edit(holder, MEMBER, None) + was = commits(holder, env) + + result = run_script(FAMILY, *verb_args(verb, holder, family), env=env) + + assert result.returncode == 0, result.stdout + result.stderr + assert commits(holder, env) == was + 1 + if verb == "remove": + assert committed(holder, env) == {".gitmodules", "family.yaml", + f"members/{MEMBER}"} + assert [r["project"] for r in manifest(holder)["members"]] == [OTHER] + else: + assert committed(holder, env) == {"family.yaml", f"members/{MEMBER}"} + row = {r["project"]: r for r in manifest(holder)["members"]}[MEMBER] + assert row["path"] == f"members/{MEMBER}" + assert row["pin"]["commit"] == older(family) + assert validate(holder).returncode == 0 + assert not offline.gh_log.exists() + + +def test_the_one_definition_is_the_path_add_writes_and_the_check_of_a_row(): + """`_member_path` is what `_member_row` (the row `add` writes) calls, and + what `bump` and `remove` ask of a row: the same string, and a refusal for + any other, with no disk and no git.""" + module = family_module() + row = module._member_row(MEMBER, "irrs", f"{ORG}/{MEMBER}", "a" * 40, + "b" * 64) + assert row["path"] == module._member_path(MEMBER) == f"members/{MEMBER}" + for kept in ({}, {"path": None}, {"path": ""}, {"path": row["path"]}): + assert module._member_path(MEMBER, kept) == row["path"], kept + for spelling in (*SPELLINGS.values(), f"members\\{MEMBER}", + f"./members/{MEMBER}", " members/" + MEMBER): + with pytest.raises(module.Refusal) as caught: + module._member_path(MEMBER, {"path": spelling}) + assert caught.value.code == "member-path", spelling + # A project that is not one directory name has no path to compare: the row + # is refused under the same code, whatever its `path:` says (or does not). + for project in ("", ".", "..", "a/b"): + for named in ({}, {"path": f"members/{project}"}): + with pytest.raises(module.Refusal) as caught: + module._member_path(project, named) + assert caught.value.code == "member-path", (project, named) + # What `add` writes, with no row, is as it was. + assert module._member_path("..") == "members/.." + assert module._member_row("..", "x", f"{ORG}/..", "a" * 40, "b" * 64 + )["path"] == "members/.." + + +# --- literal pathspecs ------------------------------------------------------ + +def reached_the_neighbour(call: str, root: Path, env: dict, + proc: subprocess.CompletedProcess) -> bool: + """Did `call`, given the glob `members/*`, act on `OTHER`, a member it was + never named for? Each answer is read from git, never from an exit code + alone.""" + if call == "deinit": + return not (root / "members" / OTHER / "project.yaml").exists() + if call == "rm": + return raw(root, env, "ls-files", "-s", "--", f"members/{OTHER}" + ).stdout == b"" + if call == "diff": + return proc.returncode == 1 + return proc.returncode == 0 and f"members/{OTHER}" in committed(root, env) + + +GIT_CALLS = { + "deinit": ("submodule", "deinit", "-f", "-q", "--", GLOB), + "rm": ("rm", "-r", "-q", "-f", "--", GLOB), + "diff": ("diff", "--quiet", "--ignore-submodules=dirty", "HEAD", "--", + GLOB), + "commit": ("commit", "-q", "-m", "a wrong commit", "--", GLOB), +} + + +def literal_form(call: str) -> list[str]: + """`call` as the tool spells it: the global option in front, except on the + commit, whose path is `:(literal)`-prefixed instead (see `_commit_one`).""" + if call == "commit": + return [word if word != GLOB else f":(literal){GLOB}" + for word in GIT_CALLS[call]] + return ["--literal-pathspecs", *GIT_CALLS[call]] + + +@pytest.mark.parametrize("call", GIT_CALLS) +def test_a_literal_spelling_keeps_a_glob_off_the_neighbour_in_each_call( + holder, offline, tmp_path, call): + """EACH GIT CALL `bump` AND `remove` MAKE WITH THE MEMBER'S PATH, run with + the path `members/*`, on THIS git and on every runner's: the plain call + reaches the neighbour (the premise: a test whose glob matched nothing + would pass on any code), and the same call in the tool's literal spelling + does not: the GLOBAL `--literal-pathspecs` for `deinit`, `rm` and `diff`, + `:(literal)` on the path for the commit. `git submodule` is its own + command, which is why `deinit` is here; it honours the global option, + which reaches it as `GIT_LITERAL_PATHSPECS` in the environment.""" + env = isolated(offline, tmp_path) + literal_holder = tmp_path / "literal" + shutil.copytree(holder, literal_holder, symlinks=True) + if call in ("diff", "commit"): + # A neighbour with something to find: its gitlink moved and staged. + for root in (holder, literal_holder): + commit_in_member(root / "members" / OTHER) + raw(root, env, "add", "--", f"members/{OTHER}") + + plain = raw(holder, env, *GIT_CALLS[call], check=False) + assert reached_the_neighbour(call, holder, env, plain), ( + "premise: without the flag, the glob reaches the neighbour; " + + plain.stderr.decode("utf-8", "replace")) + + literal = raw(literal_holder, env, *literal_form(call), check=False) + assert not reached_the_neighbour(call, literal_holder, env, literal), ( + literal.stderr.decode("utf-8", "replace")) + assert (literal_holder / "members" / OTHER / "project.yaml").is_file() + assert raw(literal_holder, env, "ls-tree", "HEAD", "--", + f"members/{OTHER}").stdout.startswith(b"160000 commit ") + + +def recorded(monkeypatch) -> list[list[str]]: + """Every program this process runs from now on, argv by argv, still run + for real.""" + calls: list[list[str]] = [] + real = subprocess.run + + def record(argv, *rest, **named): + calls.append(list(argv)) + return real(argv, *rest, **named) + + monkeypatch.setattr(subprocess, "run", record) + return calls + + +def subcommand(argv: list[str], subcommands: set[str]) -> str: + return next(word for i, word in enumerate(argv) + if i and word in subcommands) + + +@pytest.mark.parametrize("verb, flagged, commit_paths", [ + ("remove", {"submodule", "rm"}, + [".gitmodules", f"members/{OTHER}", "family.yaml"]), + ("bump", {"add", "diff"}, [f"members/{MEMBER}", "family.yaml"])]) +def test_every_git_call_that_takes_the_members_path_is_literal( + family, holder, offline, tmp_path, monkeypatch, verb, flagged, + commit_paths): + """THE LITERAL SPELLING IS THERE on every call a real run of the verb makes + with the member's path. `remove`'s `deinit` and `rm`, and `bump`'s two + stages and its `diff`, carry `--literal-pathspecs` in the place git takes + global options. The COMMIT of both carries `:(literal)` on each of its + paths and NOT the global option: that option is `GIT_LITERAL_PATHSPECS` in + the environment of every hook the commit runs. The set of calls is exact, + so a new call that names the path is a test to extend rather than a call + that goes unchecked.""" + module = family_module() + for key, value in isolated(offline, tmp_path).items(): + monkeypatch.setenv(key, value) + calls = recorded(monkeypatch) + path = f"members/{OTHER if verb == 'remove' else MEMBER}" + args = verb_args(verb, holder, family) + if verb == "remove": + args[args.index(MEMBER)] = OTHER + subcommands = {*flagged, "commit"} + + assert module.main(args) == 0 + + named = [argv for argv in calls if argv[0] == "git" + and (path in argv or f":(literal){path}" in argv)] + assert {subcommand(argv, subcommands) for argv in named} == subcommands + for argv in named: + options = argv[1:argv.index(subcommand(argv, subcommands))] + if subcommand(argv, subcommands) == "commit": + assert "--literal-pathspecs" not in options, argv + assert argv[argv.index("--") + 1:] == [ + f":(literal){each}" for each in commit_paths], argv + else: + assert "--literal-pathspecs" in options, argv + + +def with_a_glob_hook(root: Path, tmp_path: Path) -> Path: + """A `pre-commit` hook in `root` that writes what a glob-picking hook sees: + the variable git's own `--literal-pathspecs` would put in its environment, + and the staged files `git diff --cached --name-only -- '*.yaml'` finds, one + per line. Returns the log. `sh`, which git runs hooks with on every OS + (git for Windows ships its own); LOCAL `core.hooksPath`, so a global one + cannot point git somewhere this hook is not.""" + log = tmp_path / "hook.log" + hooks = tmp_path / "hooks" + hooks.mkdir() + (hooks / "pre-commit").write_bytes(( + "#!/bin/sh\n" + f"{{ echo \"literal=${{GIT_LITERAL_PATHSPECS-unset}}\"\n" + " git diff --cached --name-only -- '*.yaml'; } >> " + f"'{log.as_posix()}'\n" + "exit 0\n").encode("utf-8")) + (hooks / "pre-commit").chmod(0o755) + git("config", "--local", "core.hooksPath", hooks.as_posix(), cwd=root) + return log + + +@pytest.mark.parametrize("verb", ["add", "bump", "remove"]) +def test_the_commit_hooks_see_no_literal_pathspecs_and_find_the_manifest( + family, holder, offline, tmp_path, verb): + """THE GLOBAL `--literal-pathspecs` ON THE COMMIT LEAKED INTO THE HOLDER'S + HOOKS. It is `GIT_LITERAL_PATHSPECS=1` in git's environment, which every + child of `git commit` inherits, so a `pre-commit` hook that picked the + staged files with a glob matched nothing on every `add`, `bump` and + `remove` commit: a linter behind it checked nothing and said nothing. The + commit takes `:(literal)` on its paths now. Through a real `add`, `bump` + and `remove`, the hook sees no such variable, and its glob finds the + manifest the commit is of.""" + env = isolated(offline, tmp_path) + log = with_a_glob_hook(holder, tmp_path) + if verb == "add": + args = ["add", "--family-root", str(holder), "--member", + f"{ORG}/{SPARE}", "--local-remote-dir", + str(family["remotes"])] + else: + args = verb_args(verb, holder, family) + + result = run_script(FAMILY, *args, env=env) + + assert result.returncode == 0, result.stdout + result.stderr + assert log.is_file(), "premise: the hook ran" + assert log.read_bytes().decode("utf-8").split() == ["literal=unset", + "family.yaml"] + + +#: What each verb's ONE commit is of, as git reports it. +COMMITTED = { + "add": {".gitmodules", f"members/{SPARE}", "family.yaml"}, + "bump": {f"members/{MEMBER}", "family.yaml"}, + "remove": {".gitmodules", f"members/{MEMBER}", "family.yaml"}, +} + + +@pytest.mark.parametrize("verb", ["add", "bump", "remove"]) +def test_a_caller_who_exports_literal_pathspecs_still_gets_one_commit( + family, holder, offline, tmp_path, verb): + """`:(literal)` is pathspec MAGIC, and `GIT_LITERAL_PATHSPECS=1` turns + magic off: with it in the caller's environment git read `:(literal) + family.yaml` as a file of that name and found none, so every commit of + `add`, `bump` and `remove` failed (`bump` and `remove` with the pin or the + unmount left staged and no rollback), where main and the first form of + this fix exited 0. A safety default, or a `family.py` run from a hook or an + alias of `git --literal-pathspecs`, puts it there. The commit drops that + one variable from its own environment: with it exported, each verb exits + 0 in ONE commit of exactly the paths it is for, and leaves nothing staged + behind.""" + env = {**isolated(offline, tmp_path), "GIT_LITERAL_PATHSPECS": "1"} + was = commits(holder, env) + if verb == "add": + args = ["add", "--family-root", str(holder), "--member", + f"{ORG}/{SPARE}", "--local-remote-dir", + str(family["remotes"])] + else: + args = verb_args(verb, holder, family) + + result = run_script(FAMILY, *args, env=env) + + assert result.returncode == 0, result.stdout + result.stderr + assert commits(holder, env) == was + 1 + assert committed(holder, env) == COMMITTED[verb] + assert raw(holder, env, "status", "--porcelain=v1", "-z").stdout == b"" + assert validate(holder).returncode == 0