diff --git a/docs/cli.md b/docs/cli.md index 08721707..94b40038 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -653,6 +653,7 @@ options: ``` usage: family.py add [-h] --family-root FAMILY_ROOT --member ORG/PROJECT [--at COMMIT] [--local-remote-dir LOCAL_REMOTE_DIR] + [--trailer "KEY: VALUE"] [--dry-run] options: -h, --help show this help message and exit @@ -664,6 +665,15 @@ options: --local-remote-dir LOCAL_REMOTE_DIR resolve the member to a bare repository here instead of github.com (the TEST path) + --trailer "KEY: VALUE" + append this `: ` line to the commit this + writes; repeatable, kept in the order given + --dry-run print the row, the gitlink and the .gitmodules section + this would write, and its trailers, and change nothing + in the holder: it READS the member, as a real add + does, from throwaway clones in temporary directories + outside the holder, which it deletes, and mounts, + writes, stages and commits nothing ``` #### `scripts/family.py bump` diff --git a/scripts/family.py b/scripts/family.py index 432998b1..3bc79e61 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -54,8 +54,8 @@ EXIT CODES: 0 done, or a `--dry-run` printed what it would do · 1 nothing to do · 2 a refusal, and a refusal made before the first write is the same -refusal under `--dry-run`. `init`, `bump`, `remove` and `siblings` take -`--dry-run`; `add` takes none yet (#326). +refusal under `--dry-run`. Every subcommand takes `--dry-run`: `init`, `add`, +`bump`, `remove` and `siblings` (#326). `siblings` exits with the template script's own code, unchanged. A `bump` whose commit would record nothing — the member is already pinned at `--to`, in the gitlink and in `family.yaml` alike — is `nothing`: it says `nothing to do`, as @@ -74,12 +74,26 @@ `remove` makes before its first write and prints the row, the gitlink, the `.gitmodules` section, the holder's registration of the member and the directory it would remove, and runs no git command that writes. +`add --dry-run` makes every refusal `add` makes before its first write, the +member's own included, and READS THE MEMBER to ask them, as a real add does: +a bare clone from its remote into a temporary directory outside the holder, +deleted whatever the answer. It reads the member once more the same way, at +the commit that clone resolved, for the `id:` and the digest the row records, +and prints that row, the gitlink, the `.gitmodules` section, the registration +and the directory the mount would make, and the trailers its commit would end +with; it never runs `git submodule add` and writes nothing of the holder's. WHAT ONLY THE WRITE ITSELF CAN ANSWER IS NOT ASKED. A member whose local changes `git checkout` would refuse to overwrite gets exit 0 and a row from `bump --dry-run`, and exit 2 from the real `bump`'s checkout; an unmerged `.gitmodules` is refused by the real `remove`'s `git rm` and not by its dry -run. Each is git's own refusal, made with the holder unchanged. +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 +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. """ from __future__ import annotations @@ -125,7 +139,20 @@ #: `/`, the only spelling `--member` accepts. A bare name would #: have to guess an owner, and a family whose members are in another #: organisation is an ordinary case (an estate's neutral products are). -MEMBER_RE = re.compile(r"^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$") +#: +#: EACH NAME BEGINS WITH A LETTER OR A DIGIT, as the holder's own +#: `validate-family.py` requires of a row's `repository:` (`QUALIFIED_RE`, +#: `member-repository`), because `add` writes `--member` verbatim as that row. +#: Without it `testorg/_Zed`, `testorg/.Zed`, `_org/Zed` and `.org/Zed` were +#: mounted, pinned and committed with exit 0, and the gate the holder ships +#: failed the commit `add` had just reported (#366). THIS IS A THIRD SPELLING +#: OF THAT PATTERN, not an import of it: the validator reads the holder's own +#: copy of `repo_shape.py`, so a constant moved there would reach an existing +#: holder only through `update-shape.py`. `tests/test_family_member_name.py` +#: reads every spelling and fails when they differ, so it is one rule kept in +#: three places and never three rules. Change one and the test names the rest. +MEMBER_RE = re.compile( + r"^[A-Za-z0-9][A-Za-z0-9_.-]*/[A-Za-z0-9][A-Za-z0-9_.-]*$") #: The suffix a LEG's name carries after its assembly root's — the #: `(?:-spec|-code)?` that `contracts/repository-naming.yaml`'s leg pattern @@ -376,25 +403,21 @@ def _member_row(project: str, project_id: str, repository: str, commit: str, def _checked_member(raw: str) -> tuple[str, str]: """`--member` as `(org/Project, Project)`.""" name = raw.strip() - if not MEMBER_RE.match(name): - raise Refusal( - "member-malformed", - f"--member {raw!r} is not `/`", - "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("/") + # (#299), AND IT IS SAID SO BEFORE THE PATTERN IS ASKED (#366). `/..` + # once matched `MEMBER_RE`, 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 pattern refuses both now, for beginning with a dot, but + # in its general sentence, and what is wrong with `.` and `..` is that + # they are no name at all, which that sentence does not say. So the clause + # stays, FIRST, for the more specific one; it is read only on a name with + # a `/`, since a bare `.` has no owner to blame and the general sentence + # asks for one. An owner of `.` or `..` names no account. + owner, slash, project = name.partition("/") for which, segment in (("owner", owner), ("project", project)): - if segment not in (".", ".."): + if not slash or segment not in (".", ".."): continue if which == "owner": why = "no organisation or account is called that" @@ -407,8 +430,25 @@ def _checked_member(raw: str) -> tuple[str, str]: 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`).") + "real name beginning with a letter or a digit; `.` and `..` are " + "neither, though a name may contain a dot after that (`Ze.d`).") + # BEFORE THE MEMBER IS READ, a name the holder's own gate would refuse is + # refused here (#366): `add` writes `--member` verbatim as the row's + # `repository:`, and `validate-family.py` fails a row whose owner or + # project begins with anything but a letter or a digit. + if not MEMBER_RE.match(name): + raise Refusal( + "member-malformed", + f"--member {raw!r} is not `/`: each name begins " + "with a letter or a digit and goes on with letters, digits, `.`, " + "`_` and `-`", + "Remediation: name the owner — a family's members may live in " + "another organisation, so there is no owner to assume — and " + "begin neither name with `.`, `_` or `-`: the holder's " + "`validate-family.py` refuses such a `repository:` as " + "`member-repository`, so a commit that wrote one would fail the " + "gate the holder ships. For `remove` and `bump`, the bare " + "`` is what is wanted.") checked_value("--member", name) return name, repo_basename(name) @@ -1927,9 +1967,11 @@ def undo(self) -> str: def _mount_and_pin(family: Family, repository: str, project: str, path: str, url: str, protocol: list[str], at: str | None, - rollback: MountRollback) -> tuple[str, str, str, str]: + rollback: MountRollback, + trailers: list[str] = ()) -> tuple[str, str, str, str]: """The part of `add` that writes: `git submodule add`, the pin and the - ONE commit. Returns `(commit, digest, project id, the new HEAD)`. + ONE commit, ending with `trailers`, as `bump`'s does (#326). Returns + `(commit, digest, project id, the new HEAD)`. Every step runs inside the caller's `MountRollback` and moves its `step` on first, so a git command that fails is named by what `add` was doing. @@ -2004,13 +2046,145 @@ def _mount_and_pin(family: Family, repository: str, project: str, path: str, "which is the whole of the lockstep rule.\n\nIt remains a whole " "project: its own assembly root, its own legs, its own gate. " "Membership confers nothing.\n", - [GITMODULES, path, MANIFEST]) + [GITMODULES, path, MANIFEST], trailers=trailers) return commit, digest, project_id, head +#: The `id:` `add --dry-run` prints for a member whose `project.yaml` is a +#: symbolic link: the objects hold only the link's text, and only a checkout +#: reads through it, so the id is the real run's to read and none is guessed. +LINKED_ID = "(read through the link by the real run)" + + +def _resolves(root: Path, spec: str) -> bool: + """Whether `spec` names an object in the holder (`git rev-parse -q + --verify`). `git submodule add` asks it of `:.gitmodules` and + `HEAD:.gitmodules` when the work tree has no `.gitmodules`, and refuses + to write the file if either answers.""" + try: + git_out(["rev-parse", "-q", "--verify", spec], cwd=root) + except Refusal: + return False + return True + + +def _add_dry_run(family: Family, repository: str, project: str, path: str, + url: str, protocol: list[str], commit: str, at: str | None, + trailers: list[str]) -> int: + """`add --dry-run` (#326), entered once every refusal `add` makes before + its first write has been asked — the holder's (`_checked_add_target`), + the member's (`_probe_member`) and whether the `id:` the probe read is + one a row already pins (`_checked_member_id`, #330, which `cmd_add` asks + of the probe's reading before it comes here, so the dry run refuses + `member-duplicate-id` through the one function the real run does) — and + before `MountRollback` is taken or anything is mounted: what a real + `add` would write, and no write to the holder. + + THE MEMBER IS READ A SECOND TIME, AS THE REAL RUN READS IT A SECOND + TIME. A real `add` reads the member twice: the probe's throwaway clone, + which asks the refusals, and the mount, whose reading the row records. + The dry run has no mount, so its second reading is a second throwaway + clone, made as the probe makes its own: bare, in a temporary directory + OUTSIDE the holder, and deleted whatever the answer, so nothing the + holder has is written. It is read at the commit the probe resolved, + which is the one the mount checks out unless the member moves on in + between: the `id:` by `_member_id_at`, which reads `project.yaml` with + the function the mount's checkout is read with, and the digest by + `tree_digest`, which reads objects and never a checkout, so the digest + printed is the one the real run computes from the same tree, never a + guess. The row is `_member_row`'s, as the real run's is. + + A LINKED `project.yaml` IS LEFT TO THE REAL RUN, as the probe leaves it + to the mount: the objects hold only the link's text and a checkout reads + through it, so the `id:` is printed as the real run's to read + (`LINKED_ID`) and never guessed. + + PRINTED, in `bump --dry-run`'s layout: the commit and its digest; the + gitlink `git submodule add` stages at the path; the `.gitmodules` section + it writes, under the name git gives it (the path), with the url git is + handed, and whether the file is new; the registration it writes in the + holder's `.git/config`, or the one there whose url it rewrites; the + directory it checks the member out into and the object store it keeps, + where git keeps it (`_module_dir`); the row as `family.yaml` would carry + it; and the trailers its commit would end with (`_print_trailers`). + + GIT'S OWN REFUSAL AT THE MOUNT IS NAMED, NOT MADE. With no `.gitmodules` + in the work tree and one in the index or HEAD, `git submodule add` + refuses to write the file (`please make sure that the .gitmodules file + is in the working tree`) before it writes anything, and the real run + exits 2 there. Only git can say that sentence, so the dry run says what + will happen in a `note` and how to put the file back, and exits 0, as + `bump --dry-run` does for the checkout's own refusal. + """ + scratch = Path(tempfile.mkdtemp(prefix="openreposhape-member-")) + try: + clone = scratch / "member.git" + run(["git", *protocol, "clone", "-q", "--bare", "--", url, str(clone)]) + project_id = _member_id_at(clone, commit, project, repository) + digest = tree_digest(clone, commit) + except CommandFailed as exc: + print(exc.loudly(f"reading {repository}"), file=sys.stderr) + print(UNREADABLE_HINT, file=sys.stderr) + return 2 + finally: + try: + _rmtree(scratch) + except OSError as exc: + print(f" note: the throwaway clone {scratch} could not be " + f"deleted ({exc.strerror or exc}); nothing in the holder " + "refers to it", file=sys.stderr) + root = family.root + linked = project_id is None + row = _member_row(project, LINKED_ID if linked else project_id, + repository, commit, digest) + store, _ = _module_dir(root, path) + print(f"member {project} {repository} at {path}") + print(f"commit none -> {commit[:12]}") + print(f"tree {digest[:12]}… ({digest})") + if linked: + print("note project.yaml is a symbolic link at " + f"{commit[:12]}, which only a checkout reads through: the real " + "run reads the `id:` through the link, and the row below leaves " + "it to that run") + if at is None: + print("note without --at a real add pins the tip it finds " + f"when it mounts, which is {commit[:12]} unless {repository} " + f"moves on first; --at {commit} pins this one") + print(f"gitlink {path} none -> {commit[:12]}") + on_disk = os.path.lexists(root / GITMODULES) + elsewhere = [] if on_disk else [ + where for where, spec in (("the index", f":{GITMODULES}"), + ("HEAD", f"HEAD:{GITMODULES}")) + if _resolves(root, spec)] + print(f'gitmodules [submodule "{path}"] path {path}, url {url}' + + ("" if on_disk or elsewhere else f", in a new {GITMODULES}")) + if elsewhere: + print(f"note {GITMODULES} is in {' and '.join(elsewhere)} but " + "not in the work tree, where `git submodule add` refuses to " + "write it: the real run exits 2 at its mount with the holder " + "unchanged. Put back the copy git read first: `git checkout -- " + f"{GITMODULES}` (the index's), or `git checkout HEAD -- " + f"{GITMODULES}`") + print(f'registration [submodule "{path}"] in the holder\'s .git/config, ' + + ("its url rewritten" if _registered(root, path) else "written") + + " by `git submodule add`") + print(f"directory {path} the member checked out at {commit[:12]}; " + f"its object store at {store}") + print(f"row as {MANIFEST} would carry it:") + for line in _member_lines(row): + print(line) + _print_trailers(trailers) + print(f"\n--dry-run: nothing was changed in the holder. {repository} was " + "read from throwaway clones in temporary directories outside the " + f"holder, now deleted; {GITMODULES}, {MANIFEST}, the index, the " + "work tree and the holder's git directory are as they were.") + return 0 + + def cmd_add(args) -> int: """Mount one member and pin it in ONE commit, or refuse and leave the - holder exactly as it was (#203). + holder exactly as it was (#203); or with `--dry-run`, print what it + would write and change nothing in the holder (#326, `_add_dry_run`). IN THIS ORDER, AND ONLY THIS ORDER: the holder's refusals (`_checked_add_target`), then the member's (`_probe_member`, of a @@ -2046,12 +2220,18 @@ def cmd_add(args) -> int: if probed_id is not None: _checked_member_id(family, repository, project, probed_id) + # Every refusal above is the same under `--dry-run`, which leaves here, + # before `MountRollback` is taken and anything is mounted (#326). + if args.dry_run: + return _add_dry_run(family, repository, project, path, url, + protocol, commit, at, args.trailer) + rollback = MountRollback(family.root, path) print(f"mounting {repository} at {path}", flush=True) try: commit, digest, project_id, head = _mount_and_pin( family, repository, project, path, url, protocol, - commit if at else None, rollback) + commit if at else None, rollback, args.trailer) except Refusal as exc: exc.detail = _ended(exc.detail) + rollback.undo() raise @@ -2703,6 +2883,18 @@ def build_parser() -> argparse.ArgumentParser: add.add_argument("--local-remote-dir", type=Path, default=None, help="resolve the member to a bare repository here " "instead of github.com (the TEST path)") + add.add_argument("--trailer", action="append", metavar='"KEY: VALUE"', + default=[], type=trailer_line, + help="append this `: ` line to the commit " + "this writes; repeatable, kept in the order given") + add.add_argument("--dry-run", action="store_true", + help="print the row, the gitlink and the .gitmodules " + "section this would write, and its trailers, and " + "change nothing in the holder: it READS the member, " + "as a real add does, from throwaway clones in " + "temporary directories outside the holder, which " + "it deletes, and mounts, writes, stages and commits " + "nothing") add.set_defaults(func=cmd_add) bump = subparsers.add_parser("bump", help="move a member's pin") diff --git a/tests/test_family_add_refusals.py b/tests/test_family_add_refusals.py index 3f744dc1..bec25186 100644 --- a/tests/test_family_add_refusals.py +++ b/tests/test_family_add_refusals.py @@ -24,8 +24,8 @@ `gh` on PATH logs and exits non-zero so a call to it would be seen. (On Windows a `#!` shim is not executable and the log stays empty whatever happens; `add` with `--local-remote-dir` has no `gh` call to make, which is -what the POSIX runs prove.) `add` has no `--dry-run`, so there is none to -test here. +what the POSIX runs prove.) `add --dry-run` asks these same refusals and is +tested in `test_family_dry_run.py` (#326). """ from __future__ import annotations diff --git a/tests/test_family_dry_run.py b/tests/test_family_dry_run.py index 98660d59..f418ccf7 100644 --- a/tests/test_family_dry_run.py +++ b/tests/test_family_dry_run.py @@ -1,6 +1,6 @@ # SPDX-License-Identifier: Apache-2.0 -"""`family.py bump --dry-run` and `remove --dry-run` print what the real run -would do and change nothing in the holder (#326). +"""`family.py add --dry-run`, `bump --dry-run` and `remove --dry-run` print +what the real run would do and change nothing in the holder (#326). `bump`, `add` and `remove` took no `--dry-run`, though the file's exit-code paragraph said a dry run prints and exits 0 and `bump-leg.py` has one. The @@ -15,8 +15,14 @@ 1 for `nothing to do`; and it never checks the member out, rewrites `family.yaml`, stages or commits. `remove --dry-run` makes every refusal `remove` makes and prints the row, the gitlink, the `.gitmodules` section by -name and the directory, and changes nothing. `add` takes none yet: the -changes to it in flight land first. +name and the directory, and changes nothing. `add --dry-run`, the second part +of #326, makes every refusal `add` makes before its first write, the +member's own included; it reads the member as a real add does, from +throwaway clones in temporary directories outside the holder that are gone +afterwards, and prints the row, the gitlink, the `.gitmodules` section, the +registration, the directory and the trailers; it runs no `git submodule +add` and writes nothing of the holder's: no `.gitmodules`, `family.yaml`, +index, work tree, `.git/config` or `.git/modules`. "CHANGES NOTHING" IS ASKED AS BYTES, the way `test_family_member_path.py` and `test_family_add_refusals.py` ask it of a refusal: `git ls-files -s -z`, `git @@ -35,7 +41,10 @@ import os import re +import shutil +import subprocess import sys +import tempfile from pathlib import Path import pytest @@ -47,11 +56,15 @@ #: 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, ORG, family, family_module, manifest, nothing_to_do_line, + FAMILY, ORG, SPARE, family, family_module, manifest, nothing_to_do_line, validate, ) +from test_family_add_excludes import empty_holder # noqa: F401 +#: 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 - holder, offline, refusal, sections, + GHOST, gitmodules_with, holder, offline, one_commit_remote, refusal, + sections, ) from test_family_member_path import ( GLOB, MEMBER, OTHER, hand_edit, older, raw, recorded, @@ -753,3 +766,652 @@ def test_a_dry_run_runs_no_git_command_that_writes( assert ("fetch" in ran) is (verb == "bump") assert ("hash-object" in ran) is (to == "pinned"), ( "premise: `nothing to do` is asked of `hash-object`, and only then") + + +# --- add --------------------------------------------------------------------- + +#: The mount path of the member every `add` here adds: the spare, which the +#: family has scaffolded and not added. +SPARE_PATH = f"members/{SPARE}" + +#: The order an `add --dry-run` test compares a holder's state in: the +#: facts `git submodule add`, the real run's first write, changes first. +ADD_ORDER = ("index", ".gitmodules", ".git/modules", ".git/config", + "family.yaml", "status", "work tree", "objects", "HEAD") + + +def add_state(root: Path, env: dict) -> dict: + """Everything `add --dry-run` must leave as it found it, as bytes: what + `state` reads but a member's HEAD, with a file that is not there as + None (absent stays absent), HEAD read as a holder with no commit reads + it, and the holder's `.git/modules` listed two deep, which is where a + mount keeps its object store. In a holder that is a linked worktree, + `.git` is a file and the three read from inside it are None.""" + def read(name: str) -> bytes | None: + target = root / name + return target.read_bytes() if target.is_file() else None + + modules = root / ".git" / "modules" + return { + "objects": objects(root) if (root / ".git").is_dir() else None, + "HEAD": raw(root, env, "rev-parse", "-q", "--verify", "HEAD", + check=False).stdout, + "index": raw(root, env, "ls-files", "-s", "-z").stdout, + "status": raw(root, env, "status", "--porcelain=v1", "-z").stdout, + "family.yaml": read("family.yaml"), + ".gitmodules": read(".gitmodules"), + ".git/config": read(".git/config"), + ".git/modules": (sorted(path.relative_to(modules).as_posix() + for depth in ("*", "*/*") + for path in modules.glob(depth)) + if modules.is_dir() else None), + "work tree": work_tree(root), + } + + +def assert_add_as_it_was(root: Path, env: dict, before: dict) -> None: + """`root` is in the state `before` recorded, compared in `ADD_ORDER`, + so a failure names the first fact that moved.""" + now = add_state(root, env) + for key in ADD_ORDER: + assert now[key] == before[key], f"{key} changed" + + +def add(root: Path, env: dict, remotes: Path, *extra: str, + member: str = f"{ORG}/{SPARE}"): + return run_script(FAMILY, "add", "--family-root", str(root), "--member", + member, "--local-remote-dir", str(remotes), *extra, + env=env) + + +def field(lines: list[str], label: str) -> str: + """The one line of a dry run's output that `label` starts, without the + label and the padding that lines every value up at column 13.""" + found = [line[13:] for line in lines if line.startswith(f"{label:<13}")] + assert len(found) == 1, (label, lines) + return found[0] + + +def tip_of(family, project: str = SPARE) -> str: + return git("rev-parse", "main", + cwd=family["remotes"] / f"{project}.git").stdout.strip() + + +def gitmodules_value(root: Path, env: dict, key: str) -> str: + """`submodule..` as git reads the holder's + `.gitmodules`.""" + return raw(root, env, "config", "-f", ".gitmodules", "--get", + f"submodule.{SPARE_PATH}.{key}").stdout.decode("utf-8").strip() + + +@pytest.mark.parametrize("which", ["holder", "empty_holder"], + ids=["two-members", "first-member"]) +def test_add_dry_run_prints_what_a_real_add_writes_and_changes_nothing( + family, offline, tmp_path, request, which): + """THE RULING'S FIRST TEST, for `add`. A real add previewed: the row, + the gitlink, the `.gitmodules` section (in a new file for a family's + first member) and the rest of what the mount makes, and not one byte of + the holder moved: HEAD, the index, `git status`, `family.yaml`, + `.gitmodules` (absent stays absent), `.git/config`, the `.git/modules` + listing, the work tree and the holder's own objects. No `gh` call, and + no temporary clone left behind. Then the real add writes exactly the + row, the section and the gitlink the dry run printed, and the holder + validates.""" + root = request.getfixturevalue(which) + env = isolated(offline, tmp_path) + first = which == "empty_holder" + assert (root / ".gitmodules").exists() is not first, "premise" + assert registered_keys(root, env, SPARE_PATH) == [], "premise" + tip = tip_of(family) + before = add_state(root, env) + + dry = add(root, env, family["remotes"], "--dry-run") + + # FIRST, WHATEVER IT PRINTED: nothing moved, and nothing was left. + assert_add_as_it_was(root, env, before) + offline.assert_left_nothing() + assert dry.returncode == 0, dry.stdout + dry.stderr + assert dry.stderr == "" + lines = dry.stdout.splitlines() + assert lines[:3] == [ + f"checking {ORG}/{SPARE} before mounting it", + f"member {SPARE} {ORG}/{SPARE} at {SPARE_PATH}", + f"commit none -> {tip[:12]}"] + assert field(lines, "gitlink") == f"{SPARE_PATH} none -> {tip[:12]}" + section = re.fullmatch( + rf'\[submodule "{SPARE_PATH}"\] path {SPARE_PATH}, url (.+?)' + r"(, in a new \.gitmodules)?", field(lines, "gitmodules")) + assert section, field(lines, "gitmodules") + assert bool(section.group(2)) is first + assert field(lines, "registration") == ( + f'[submodule "{SPARE_PATH}"] in the holder\'s .git/config, written ' + "by `git submodule add`") + assert field(lines, "directory") == ( + f"{SPARE_PATH} the member checked out at {tip[:12]}; its object " + f"store at .git/modules/{SPARE_PATH}") + row = printed_row(dry.stdout) + assert row[0] == f" - project: {SPARE}" + assert f' commit: "{tip}"' in row + assert "trailers none" in lines + assert lines[-1] == ( + f"--dry-run: nothing was changed in the holder. {ORG}/{SPARE} was " + "read from throwaway clones in temporary directories outside the " + "holder, now deleted; .gitmodules, family.yaml, the index, the work " + "tree and the holder's git directory are as they were.") + was = sections(before[".gitmodules"].decode("utf-8")) if not first else [] + + real = add(root, env, family["remotes"]) + + assert real.returncode == 0, real.stdout + real.stderr + text = (root / "family.yaml").read_text(encoding="utf-8") + assert "\n".join(row) + "\n" in text, "the row printed is the row written" + written = {r["project"]: r for r in manifest(root)["members"]}[SPARE] + assert f"({written['pin']['tree_sha256']})" in lines[3], ( + "the tree line carries the digest written") + assert pinned_at(root, env, SPARE) == tip + now = sections((root / ".gitmodules").read_text(encoding="utf-8")) + assert [name for name in now if name not in was] == [SPARE_PATH] + assert gitmodules_value(root, env, "path") == SPARE_PATH + assert gitmodules_value(root, env, "url") == section.group(1) + assert registered_keys(root, env, SPARE_PATH) + assert (root / SPARE_PATH / "project.yaml").is_file() + assert (root / ".git" / "modules" / SPARE_PATH).is_dir() + assert trailers_of(root) == [], "no --trailer, no trailer" + assert validate(root).returncode == 0 + offline.assert_left_nothing() + + +def something_at_the_mount_path(root: Path) -> None: + """A directory at the mount path that no row names: a half-finished + add, or a hand's.""" + (root / SPARE_PATH).mkdir(parents=True) + (root / SPARE_PATH / "README").write_bytes(b"left here by hand\n") + + +def a_section_under_another_name(root: Path) -> None: + """#300's ghost: a section named `ghost` giving the mount path, in the + work tree only, a hand edit nobody staged.""" + gitmodules_with(root, GHOST, "the work tree only") + + +def the_mount_paths_name_given_to_another_path(root: Path) -> None: + """A section NAMED the mount path that gives another path (#242).""" + with (root / ".gitmodules").open("ab") as gitmodules: + gitmodules.write(f'[submodule "{SPARE_PATH}"]\n\tpath = ' + "members/ELSEWHERE\n\turl = /nowhere/else.git\n" + .encode("utf-8")) + + +def a_cached_store(root: Path) -> None: + """The object store a removed member leaves under `.git/modules`.""" + (root / ".git" / "modules" / SPARE_PATH).mkdir(parents=True) + + +def a_members_line_edited_by_hand(root: Path) -> None: + """A comment on the `members:` line, which YAML reads past and the line + rewrite does not, left uncommitted.""" + manifest_path = root / "family.yaml" + text = manifest_path.read_bytes().decode("utf-8") + assert text.count("\nmembers:\n") == 1, "fixture drift" + manifest_path.write_bytes(text.replace( + "\nmembers:\n", "\nmembers: # edited by hand\n").encode("utf-8")) + + +def a_renamed_copy_of_a_member(root: Path) -> Path: + """#330's case: a bare copy of the pinned `IRRS` under a name no row has, + `IRRS2`, so `holding` passes it and its `project.yaml` still records the + id `irrs` the row `IRRS` pins. It is a copy of the remote the holder's + own `.gitmodules` names, as a repository renamed at the forge is to + anybody who clones it by the new name, and it is made BESIDE the holder, + never in the family's remotes, which every test of the module shares. + Returns the directory of remotes the add is to read the member from.""" + source = git("config", "-f", ".gitmodules", "--get", + f"submodule.members/{PINNED}.url", cwd=root).stdout.strip() + remotes = root.parent / "renamed-remotes" + remotes.mkdir() + git("clone", "-q", "--bare", source, str(remotes / f"{RENAMED}.git"), + cwd=root.parent) + return remotes + + +#: Each refusal `add` makes before its first write that a fixture provokes +#: cheaply, in `add`'s own order, as `(--member, more arguments, set-up, +#: what stderr starts with)`: the holder's own refusals first, then the +#: member's, which need the probe's throwaway clone, and last the member's +#: `id:` against the rows (#330), which needs what that clone read. A +#: set-up that needs the member read from somewhere other than the family's +#: remotes returns that directory of remotes; the others return None. +ADD_REFUSALS = { + "malformed": (SPARE, (), None, "REFUSED member-malformed: "), + "malformed-dot": (f"{ORG}/..", (), None, "REFUSED member-malformed: "), + # #399's rule: a name that BEGINS with `.`, `_` or `-` is refused by the + # pattern, in its general sentence, in either position. (An owner that + # begins with `-` cannot be said as `--member /`: + # argparse reads it as an option, so that spelling is the real run's + # test in `test_family_member_name.py`.) + "malformed-project-underscore": (f"{ORG}/_{SPARE}", (), None, + "REFUSED member-malformed: "), + "malformed-project-hyphen": (f"{ORG}/-{SPARE}", (), None, + "REFUSED member-malformed: "), + "malformed-project-dot": (f"{ORG}/.{SPARE}", (), None, + "REFUSED member-malformed: "), + "malformed-owner-underscore": (f"_{ORG}/{SPARE}", (), None, + "REFUSED member-malformed: "), + "malformed-owner-dot": (f".{ORG}/{SPARE}", (), None, + "REFUSED member-malformed: "), + "duplicate-row": (f"{ORG}/{MEMBER}", (), None, + "REFUSED member-already-present: "), + "duplicate-row-in-another-case": ( + f"{ORG.lower()}/{MEMBER.lower()}", (), None, + "REFUSED member-already-present: "), + "path-taken": (f"{ORG}/{SPARE}", (), something_at_the_mount_path, + "REFUSED member-path-taken: "), + "name-taken": (f"{ORG}/{SPARE}", (), + the_mount_paths_name_given_to_another_path, + "REFUSED member-name-taken: "), + "path-registered": (f"{ORG}/{SPARE}", (), a_section_under_another_name, + "REFUSED member-path-registered: "), + "git-dir-cached": (f"{ORG}/{SPARE}", (), a_cached_store, + "REFUSED member-git-dir-cached: "), + "members-unrecognised": (f"{ORG}/{SPARE}", (), + a_members_line_edited_by_hand, + "REFUSED family-members-unrecognised: "), + "at-not-a-commit": (f"{ORG}/{SPARE}", ("--at", "abc1234"), None, + "REFUSED member-at-not-a-commit: "), + "at-unknown": (f"{ORG}/{SPARE}", ("--at", "0" * 40), None, + "REFUSED member-at-unknown: "), + "a-leg": (f"{ORG}/{MEMBER}-spec", (), None, + "REFUSED member-not-a-project: "), + "unreadable": (f"{ORG}/IRNOPE", (), None, + f"REFUSED reading {ORG}/IRNOPE. THE EXACT COMMAND WAS:"), + "duplicate-id": (f"{ORG}/{RENAMED}", (), a_renamed_copy_of_a_member, + "REFUSED member-duplicate-id: "), +} + + +def scratch_free(text: str) -> str: + """`text` with the random part of a throwaway clone's directory name + taken out: the one thing two runs over the same holder print + differently, in the exact command of a clone that failed.""" + return re.sub(r"openreposhape-member-[^/\\\s]+", "openreposhape-member-*", + text) + + +@pytest.mark.parametrize("member, extra, set_up, said", + ADD_REFUSALS.values(), ids=ADD_REFUSALS) +def test_a_refusal_add_makes_before_it_mounts_is_the_same_under_dry_run( + family, holder, offline, tmp_path, member, extra, set_up, said): + """THE RULING: every refusal `add` makes before its first write is the + same refusal under `--dry-run`, the same code and exit 2, whether it is + the holder's (asked before the member is read) or the member's (asked + of the probe's throwaway clone). The real run and the dry run, over the + same holder, print the same stdout and the same stderr, and leave the + holder as it was, every hand edit a set-up made still in place, and no + temporary clone behind.""" + env = isolated(offline, tmp_path) + remotes = family["remotes"] + if set_up is not None: + remotes = set_up(holder) or remotes + before = add_state(holder, env) + + real = add(holder, env, remotes, *extra, member=member) + dry = add(holder, env, remotes, *extra, "--dry-run", member=member) + + assert dry.returncode == real.returncode == 2, dry.stdout + dry.stderr + assert dry.stdout == real.stdout + assert scratch_free(dry.stderr) == scratch_free(real.stderr) + assert dry.stderr.startswith(said), dry.stderr + assert_add_as_it_was(holder, env, before) + offline.assert_left_nothing() + + +def test_add_dry_run_prints_the_trailers_the_real_commit_ends_with( + family, holder, offline, tmp_path): + """`--trailer` with `--dry-run` prints the lines, in order, after the row + and before the closing line, worded as `bump --dry-run` words them. The + real add then ends its one commit with exactly those lines. `add` takes + the same repeatable `--trailer` as `bump` (#326).""" + env = isolated(offline, tmp_path) + trailers = ("--trailer", TRAILERS[0], "--trailer", TRAILERS[1]) + + dry = add(holder, env, family["remotes"], "--dry-run", *trailers) + + assert dry.returncode == 0, dry.stdout + dry.stderr + lines = dry.stdout.splitlines() + at = lines.index("trailers 2 to append, in order:") + assert lines[at + 1:at + 3] == [f" {TRAILERS[0]}", f" {TRAILERS[1]}"] + assert lines.index(ROW_HEADER) < at + assert lines[-1].startswith("--dry-run: ") + + real = add(holder, env, family["remotes"], *trailers) + + assert real.returncode == 0, real.stdout + real.stderr + assert trailers_of(holder) == list(TRAILERS) + + +def test_add_dry_run_with_at_previews_that_commit(family, holder, offline, + tmp_path): + """`--at` an older commit: the dry run previews THAT commit and has no + `without --at` note, and the real add pins the commit printed.""" + env = isolated(offline, tmp_path) + older_spare = git("rev-parse", "main~1", + cwd=family["remotes"] / f"{SPARE}.git").stdout.strip() + assert older_spare != tip_of(family), "premise" + + dry = add(holder, env, family["remotes"], "--at", older_spare, + "--dry-run") + plain = add(holder, env, family["remotes"], "--dry-run") + + assert dry.returncode == plain.returncode == 0, dry.stderr + plain.stderr + lines = dry.stdout.splitlines() + assert field(lines, "commit") == f"none -> {older_spare[:12]}" + assert f' commit: "{older_spare}"' in printed_row(dry.stdout) + assert not any(line.startswith("note without --at") + for line in lines) + assert field(plain.stdout.splitlines(), "note") == ( + "without --at a real add pins the tip it finds when it mounts, " + f"which is {tip_of(family)[:12]} unless {ORG}/{SPARE} moves on " + f"first; --at {tip_of(family)} pins this one") + + real = add(holder, env, family["remotes"], "--at", older_spare) + + assert real.returncode == 0, real.stdout + real.stderr + assert pinned_at(holder, env, SPARE) == older_spare + assert "\n".join(printed_row(dry.stdout)) + "\n" in ( + holder / "family.yaml").read_text(encoding="utf-8") + + +def test_add_dry_run_names_a_registration_the_mount_rewrites( + family, holder, offline, tmp_path): + """A `submodule.members/

` registration the holder already has, from + a member removed by hand: `git submodule add` rewrites its url, and the + dry run says so rather than that it writes one. The real add leaves the + url the dry run printed.""" + env = isolated(offline, tmp_path) + raw(holder, env, "config", "--local", f"submodule.{SPARE_PATH}.url", + "/where/it/used/to/live.git") + before = add_state(holder, env) + + dry = add(holder, env, family["remotes"], "--dry-run") + + assert_add_as_it_was(holder, env, before) + assert dry.returncode == 0, dry.stdout + dry.stderr + lines = dry.stdout.splitlines() + assert field(lines, "registration") == ( + f'[submodule "{SPARE_PATH}"] in the holder\'s .git/config, its url ' + "rewritten by `git submodule add`") + url = field(lines, "gitmodules").rpartition(", url ")[2] + + real = add(holder, env, family["remotes"]) + + assert real.returncode == 0, real.stdout + real.stderr + assert raw(holder, env, "config", "--local", "--get", + f"submodule.{SPARE_PATH}.url").stdout.decode( + "utf-8").strip() == url + + +@pytest.mark.parametrize("taken_out, held_in", [ + ("from the work tree", "the index and HEAD"), + ("from the work tree and the index", "HEAD")], + ids=["unstaged-deletion", "staged-deletion"]) +def test_add_dry_run_names_the_refusal_git_makes_at_the_mount( + family, holder, offline, tmp_path, taken_out, held_in): + """GIT'S OWN REFUSAL, NAMED AND NOT MADE. With no `.gitmodules` in the + work tree and one in the index or HEAD, `git submodule add` refuses to + write the file, before it writes anything, and the real add exits 2 + there with the holder unchanged. Only git can say that sentence, so the + dry run says in a note what will happen and how to put the file back, + and exits 0, never claiming a new `.gitmodules`.""" + env = isolated(offline, tmp_path) + if taken_out == "from the work tree": + (holder / ".gitmodules").unlink() + else: + raw(holder, env, "rm", "-q", "--", ".gitmodules") + before = add_state(holder, env) + + dry = add(holder, env, family["remotes"], "--dry-run") + + assert_add_as_it_was(holder, env, before) + assert dry.returncode == 0, dry.stdout + dry.stderr + assert ", in a new .gitmodules" not in dry.stdout + assert (f"note .gitmodules is in {held_in} but not in the work " + "tree, where `git submodule add` refuses to write it: the real " + "run exits 2 at its mount with the holder unchanged. Put back " + "the copy git read first: `git checkout -- .gitmodules` (the " + "index's), or `git checkout HEAD -- .gitmodules`" + ) in dry.stdout.splitlines() + + real = add(holder, env, family["remotes"]) + + assert real.returncode == 2, real.stdout + real.stderr + assert ("fatal: please make sure that the .gitmodules file is in the " + "working tree") in real.stderr + assert_add_as_it_was(holder, env, before) + offline.assert_left_nothing() + + +@pytest.mark.skipif(sys.platform == "win32", + reason="a tracked symlink is a plain file in a Windows " + "checkout, which the real add refuses") +def test_add_dry_run_leaves_a_linked_manifest_to_the_real_run( + holder, offline, tmp_path): + """A `project.yaml` that is a SYMLINK is only its link text in the + member's objects, and the real add's mount reads through it. The dry + run does not refuse it, since the real run pins it; it says the `id:` + is the real run's to read and prints no guess. The real add then writes + the printed row with the id the link leads to. (The link is built in + the member's INDEX, not on a disk.)""" + env = isolated(offline, tmp_path) + remotes = tmp_path / "remotes" + remotes.mkdir() + work = tmp_path / "IRLN-work" + git("init", "-q", str(work), cwd=tmp_path) + (work / "manifest").mkdir() + (work / "manifest" / "project.yaml").write_bytes( + b"schema_version: 1\nkind: project-manifest\nid: irln\nname: IRLN\n") + git("add", "--", "manifest/project.yaml", cwd=work) + link = subprocess.run(["git", "hash-object", "-w", "--stdin"], + cwd=str(work), input=b"manifest/project.yaml", + capture_output=True, check=True).stdout + git("update-index", "--add", "--cacheinfo", + f"120000,{link.decode('ascii').strip()},project.yaml", cwd=work) + git("commit", "-qm", "A linked manifest", cwd=work) + git("clone", "-q", "--bare", str(work), str(remotes / "IRLN.git"), + cwd=tmp_path) + tip = git("rev-parse", "HEAD", cwd=remotes / "IRLN.git").stdout.strip() + before = add_state(holder, env) + + dry = add(holder, env, remotes, "--dry-run", member=f"{ORG}/IRLN") + + assert_add_as_it_was(holder, env, before) + assert dry.returncode == 0, dry.stdout + dry.stderr + lines = dry.stdout.splitlines() + assert (f"note project.yaml is a symbolic link at {tip[:12]}, " + "which only a checkout reads through: the real run reads the " + "`id:` through the link, and the row below leaves it to that run" + ) in lines + row = printed_row(dry.stdout) + assert row[1] == " id: (read through the link by the real run)" + + real = add(holder, env, remotes, member=f"{ORG}/IRLN") + + assert real.returncode == 0, real.stdout + real.stderr + text = (holder / "family.yaml").read_text(encoding="utf-8") + assert "\n".join([row[0], " id: irln", *row[2:]]) + "\n" in text + offline.assert_left_nothing() + + +def test_add_dry_run_reads_the_member_outside_the_holder_and_writes_nothing( + family, holder, offline, tmp_path, monkeypatch): + """IN PROCESS, every program the dry run starts recorded. The member is + read from two bare clones, the probe's and the dry run's own, each in a + directory under the run's TEMP and none in the holder; and the holder's + `git status`, asked while the dry run's own clone is still there, at the + moment its digest is computed, is the status before the run: a clone + made inside the holder would show in it, even one deleted afterwards. No + git command run in the holder writes, `git add --dry-run`, the real + run's own check of the mount path, aside; none runs `git submodule`.""" + module = family_module() + env = isolated(offline, tmp_path) + for key, value in env.items(): + monkeypatch.setenv(key, value) + monkeypatch.setattr(tempfile, "tempdir", str(offline.temp)) + before = add_state(holder, env) + real_run, real_digest = subprocess.run, module.tree_digest + calls: list[tuple[list[str], object]] = [] + digested: list[tuple[Path, bytes]] = [] + + def record(argv, *rest, **named): + calls.append((list(argv), named.get("cwd"))) + return real_run(argv, *rest, **named) + + def digest(repo, rev): + status = real_run(["git", "status", "--porcelain=v1", "-z"], + cwd=str(holder), capture_output=True, + check=True).stdout + digested.append((Path(repo), status)) + return real_digest(repo, rev) + + monkeypatch.setattr(subprocess, "run", record) + monkeypatch.setattr(module, "tree_digest", digest) + + code = module.main(["add", "--family-root", str(holder), "--member", + f"{ORG}/{SPARE}", "--local-remote-dir", + str(family["remotes"]), "--dry-run"]) + ran = list(calls) + + assert code == 0 + assert len(digested) == 1, "premise: one digest, of the dry run's clone" + (repo, status), = digested + assert status == before["status"], ( + "the holder's status moved while the member was being read") + temp, root = offline.temp.resolve(), holder.resolve() + assert temp in repo.resolve().parents and root not in repo.resolve( + ).parents, repo + gits = [(argv, cwd) for argv, cwd in ran if Path(argv[0]).name == "git"] + clones = [argv for argv, _ in gits if subcommand_of(argv) == "clone"] + assert len(clones) == 2, "the probe's clone and the dry run's own" + assert all(temp in Path(argv[-1]).resolve().parents for argv in clones) + in_holder = [argv for argv, cwd in gits + if cwd is not None and Path(cwd).resolve() == root] + assert in_holder, "premise: the holder was asked" + assert [argv for argv in in_holder if writes(argv) and not ( + subcommand_of(argv) == "add" and "--dry-run" in argv)] == [] + assert "submodule" not in {subcommand_of(argv) for argv, _ in gits} + assert_add_as_it_was(holder, env, before) + offline.assert_left_nothing() + + +def test_add_dry_run_prints_the_id_the_members_manifest_records( + holder, offline, tmp_path): + """NEVER AN INVENTED ID (#326's third ruling). Every member the family + fixture scaffolds records `id:` as its name lowercased, so a dry run + that printed `project.lower()` would pass every other test here. This + member is `IRDI` and its `project.yaml` says `id: routing-core`: the dry + run prints that id, read from the member, and the real add then writes + exactly the row the dry run printed.""" + env = isolated(offline, tmp_path) + remotes = one_commit_remote(tmp_path, "IRDI", { + "project.yaml": b"schema_version: 1\nkind: project-manifest\n" + b"id: routing-core\nname: IRDI\n"}) + before = add_state(holder, env) + + dry = add(holder, env, remotes, "--dry-run", member=f"{ORG}/IRDI") + + assert_add_as_it_was(holder, env, before) + assert dry.returncode == 0, dry.stdout + dry.stderr + row = printed_row(dry.stdout) + assert row[:2] == [" - project: IRDI", " id: routing-core"] + + real = add(holder, env, remotes, member=f"{ORG}/IRDI") + + assert real.returncode == 0, real.stdout + real.stderr + assert "\n".join(row) + "\n" in (holder / "family.yaml").read_text( + encoding="utf-8"), "the row printed is the row written" + assert {r["project"]: r for r in manifest(holder)["members"] + }["IRDI"]["id"] == "routing-core" + offline.assert_left_nothing() + + +def test_add_dry_run_removes_its_own_clone_when_reading_the_member_fails( + family, holder, offline, tmp_path, monkeypatch, capsys): + """RULING 2's "removed afterwards, also on a refusal", for the dry run's + OWN clone. Every other refusal here is made before that clone exists. + Here the probe reads the member, and the member's remote is gone by the + time the dry run clones it a second time, as it would be if it were + renamed or made private in between. That clone fails in git, and the + failure is printed as every failed reading of the member is: the exact + command, then the hint, exit 2. The directory the dry run made for its + clone is gone, nothing else is left in TEMP, and the holder is as it + was. In process, so the remote can be moved between the two clones.""" + module = family_module() + env = isolated(offline, tmp_path) + for key, value in env.items(): + monkeypatch.setenv(key, value) + monkeypatch.setattr(tempfile, "tempdir", str(offline.temp)) + remotes = tmp_path / "remotes" + remotes.mkdir() + shutil.copytree(family["remotes"] / f"{SPARE}.git", + remotes / f"{SPARE}.git", symlinks=True) + before = add_state(holder, env) + real_run = module.run + clones: list[Path] = [] + + def run(args, *rest, **named): + if "clone" in args: + clones.append(Path(args[-1])) + if len(clones) == 2: + (remotes / f"{SPARE}.git").rename(remotes / f"{SPARE}.gone") + return real_run(args, *rest, **named) + + monkeypatch.setattr(module, "run", run) + + code = module.main(["add", "--family-root", str(holder), "--member", + f"{ORG}/{SPARE}", "--local-remote-dir", str(remotes), + "--dry-run"]) + out, err = capsys.readouterr() + + assert code == 2, out + err + assert len(clones) == 2, "premise: the dry run's own clone was the one" + assert out == f"checking {ORG}/{SPARE} before mounting it\n" + lines = err.splitlines() + assert lines[0] == (f"REFUSED reading {ORG}/{SPARE}. THE EXACT COMMAND " + "WAS:") + assert "clone -q --bare -- " in lines[1] + assert lines[-1] == module.UNREADABLE_HINT + assert not clones[1].parent.exists(), "the dry run's clone was left" + assert_add_as_it_was(holder, env, before) + offline.assert_left_nothing() + + +def test_add_dry_run_names_the_store_git_keeps_for_a_linked_worktree( + family, holder, offline, tmp_path): + """THE OBJECT STORE IS NAMED WHERE GIT KEEPS IT (`_module_dir`), not + where it usually is. A holder that is a LINKED WORKTREE, a lane's usual + holder, has a `.git` FILE, and git keeps a mount's store under + `.git/worktrees//modules/`, not `.git/modules/`. The dry run + names that store, it does not exist yet, and the real add then puts + the store exactly there.""" + env = isolated(offline, tmp_path) + lane = tmp_path / "lane" + git("worktree", "add", "-q", "-b", "add-irqs", str(lane), cwd=holder) + assert (lane / ".git").is_file(), "premise: a linked worktree" + before = add_state(lane, env) + + dry = add(lane, env, family["remotes"], "--dry-run") + + assert_add_as_it_was(lane, env, before) + assert dry.returncode == 0, dry.stdout + dry.stderr + directory = field(dry.stdout.splitlines(), "directory") + said = (f"{SPARE_PATH} the member checked out at " + f"{tip_of(family)[:12]}; its object store at ") + assert directory.startswith(said), directory + store = lane / directory[len(said):] + assert not store.exists(), "premise: no store before the real add" + + real = add(lane, env, family["remotes"]) + + assert real.returncode == 0, real.stdout + real.stderr + assert (store / "config").is_file(), ( + f"the store printed, {store}, is not the one git made") + offline.assert_left_nothing() diff --git a/tests/test_family_member_name.py b/tests/test_family_member_name.py new file mode 100644 index 00000000..6263925b --- /dev/null +++ b/tests/test_family_member_name.py @@ -0,0 +1,260 @@ +# SPDX-License-Identifier: Apache-2.0 +"""`family.py add` refuses a member name the holder's own validator refuses +(#366). + +TWO TOOLS OF THIS STANDARD DISAGREED about one rule. `add --member` checked +its argument against `MEMBER_RE`, whose character class let an owner or a +project BEGIN with `.`, `_` or `-`; the holder's `validate-family.py` holds a +row's `repository:` to `QUALIFIED_RE`, the same class with a letter or a digit +required first. `add` writes `--member` verbatim as that row, so +`testorg/_Zed`, `testorg/.Zed`, `_org/Zed` and `.org/Zed` were mounted, pinned +and committed with exit 0 and the commit `add` reported failed the gate the +holder ships, as `member-repository`. (`testorg/-Zed` got as far too. +`--member=-org/Zed`, the one way a leading `-` reaches the OWNER, passed the +pattern and was refused later by `checked_value` as `unsafe-value`; the +pattern is asked first now, so it is `member-malformed` like the rest.) + +Now `MEMBER_RE` takes `QUALIFIED_RE`'s shape and the name is refused as +`member-malformed` BEFORE THE MEMBER IS READ: exit 2, nothing on stdout (the +first line `add` prints says it is checking the member), no `gh` call and no +throwaway clone, the holder byte for byte as it was. A name with a `.`, `_` or +`-` anywhere after the first character is still a name, and still adds. + +ONE RULE, THREE SPELLINGS, AND A TEST THAT HOLDS THEM TOGETHER. `QUALIFIED_RE` +is spelled in the family root's `validate-family.py` and the assembly root's +`validate-manifest.py`, and both copies ship into projects that run them from +their own copy of `repo_shape.py`. A constant moved into `repo_shape.py` would +reach an existing holder only through `update-shape.py`, so `family.py`, which +runs from the standard's checkout, spells it a third time. The pinned-equality +test below finds every spelling by READING the source and fails when any +differs: a person who changes the rule in one place is told about the rest. + +THE MEMBERS ARE `IRQS`, the family fixture's spare, copied into a bare +repository under each name a test needs (the issue's own repro does the same +with `Zed`), so `add` has a real remote to read under every spelling: a +refusal here is for the NAME, never for a missing remote. Same family, rules +and offline environment as `test_family.py`, `test_family_add_refusals.py` and +`test_family_member_path.py`: 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. +""" + +from __future__ import annotations + +import ast +import re +from pathlib import Path + +import pytest + +from conftest import REPO, git, run_script +from test_family import ( # noqa: F401 + FAMILY, ORG, SPARE, family, family_module, manifest, validate, +) +from test_family_add_refusals import holder, offline, refusal # noqa: F401 +from test_family_member_path import commits, committed, isolated, state + +#: `--member` spellings the holder's validator refuses and `add` used to +#: write: each segment's first character, in each position, and `.github`, +#: whose comment kept the class loose "for" it. A row naming +#: `testorg/.github` is refused by the validator as well, so nothing a holder +#: can keep depended on `add` admitting it. Every project here has a bare +#: remote of the same name (`remotes_for`), so only the name is wrong. +REFUSED = { + "project-underscore": f"{ORG}/_{SPARE}", + "project-dot": f"{ORG}/.{SPARE}", + "project-hyphen": f"{ORG}/-{SPARE}", + "owner-underscore": f"_{ORG}/{SPARE}", + "owner-dot": f".{ORG}/{SPARE}", + "owner-hyphen": f"-{ORG}/{SPARE}", + "project-dot-github": f"{ORG}/.github", +} + +#: Names with a `.`, `_` or `-` after their first character: the controls. +ADDED = { + "project-inner-dot": f"{ORG}/IR.QS", + "project-inner-underscore": f"{ORG}/IR_QS", + "project-inner-hyphen": f"{ORG}/IR-QS", + "owner-inner-dot": f"Ink.Router/{SPARE}", +} + +#: The remotes `remotes_for` makes: a bare copy of the spare member under each +#: project name the two tables above use, `IRQS` itself included. +PROJECTS = sorted({spelling.partition("/")[2] + for spelling in (*REFUSED.values(), *ADDED.values())}) + + +def remotes_for(family, tmp_path: Path) -> Path: + """A directory of bare remotes, one `.git` per name in + `PROJECTS`, each a copy of the spare member's own. `add + --local-remote-dir` takes the member's repository from + `

/.git`, whatever the owner is called.""" + remotes = tmp_path / "remotes" + remotes.mkdir() + for project in PROJECTS: + git("clone", "-q", "--bare", str(family["remotes"] / f"{SPARE}.git"), + str(remotes / f"{project}.git"), cwd=tmp_path) + return remotes + + +def add(holder: Path, spelling: str, remotes: Path, env: dict): + """`family.py add --member=`: the `=` form, because a spelling + that begins with `-` is read by `argparse` as an option in the other.""" + return run_script(FAMILY, "add", "--family-root", str(holder), + f"--member={spelling}", "--local-remote-dir", + str(remotes), env=env) + + +# --- the refusal ------------------------------------------------------------ + +@pytest.mark.parametrize("spelling", REFUSED.values(), ids=REFUSED) +def test_add_refuses_a_name_the_validator_refuses_before_reading_the_member( + family, holder, offline, tmp_path, spelling): + """Exit 2, `member-malformed`, not one line on stdout (`checking ... + before mounting it` comes first otherwise), no `gh` call, no throwaway + clone left, and the holder as it was: the same HEAD, index entries and + status, byte for byte, and no row, mount or registration of the name.""" + env = isolated(offline, tmp_path) + remotes = remotes_for(family, tmp_path) + before = state(holder, env) + + result = add(holder, spelling, remotes, 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} is not " + "`/`: each name begins with a letter or a digit and " + "goes on with letters, digits, `.`, `_` and `-`") + assert remedy.startswith("Remediation: name the owner") + assert "begin neither name with `.`, `_` or `-`" in remedy + assert state(holder, env) == before + assert not (holder / "members" / spelling.partition("/")[2]).exists() + offline.assert_left_nothing() + + +# --- the control ------------------------------------------------------------ + +@pytest.mark.parametrize("spelling", ADDED.values(), ids=ADDED) +def test_a_name_with_a_dot_underscore_or_hyphen_after_its_first_character_adds( + family, holder, offline, tmp_path, spelling): + """A name is not refused for HOLDING one of the three, only for beginning + with it: still ONE commit of `.gitmodules`, the mount and `family.yaml`, + the row naming the argument verbatim, and the holder's own + `validate-family.py` passing over it.""" + env = isolated(offline, tmp_path) + remotes = remotes_for(family, tmp_path) + project = spelling.partition("/")[2] + was = commits(holder, env) + + result = add(holder, spelling, remotes, env) + + assert result.returncode == 0, result.stdout + result.stderr + assert commits(holder, env) == was + 1 + assert committed(holder, env) == {".gitmodules", "family.yaml", + f"members/{project}"} + rows = {row["project"]: row for row in manifest(holder)["members"]} + assert rows[project]["repository"] == spelling + gate = validate(holder) + assert gate.returncode == 0, gate.stdout + gate.stderr + assert not offline.gh_log.exists(), offline.gh_log.read_text() + + +# --- one rule, three spellings ---------------------------------------------- + +#: Where the rule is spelled today, and under which name. The first is what +#: `add` holds a `--member` to; the other two are what a holder and an +#: assembly root hold a row's `repository:` and a manifest's to. +SPELLINGS = { + "scripts/family.py": "MEMBER_RE", + "templates/family-root/scripts/validate-family.py": "QUALIFIED_RE", + "templates/assembly-root/scripts/validate-manifest.py": "QUALIFIED_RE", +} + + +def pattern_spelled(source: bytes, name: str) -> list[str]: + """The pattern of every module-level ` = re.compile(r"...")` in + `source`, READ and never run: the two validators import a sibling + `repo_shape.py` of their own, and a test that executed them would be + testing whatever that import found. A spelling this cannot read as ONE + string literal and no flags is a failure to say so, never a skip.""" + found = [] + for node in ast.parse(source).body: + if not (isinstance(node, ast.Assign) and len(node.targets) == 1 + and isinstance(node.targets[0], ast.Name) + and node.targets[0].id == name): + continue + call = node.value + assert (isinstance(call, ast.Call) and not call.keywords + and len(call.args) == 1 + and isinstance(call.args[0], ast.Constant) + and isinstance(call.args[0].value, str) + and ast.unparse(call.func) == "re.compile"), ( + f"{name} is not `re.compile()`: {ast.unparse(node)}") + found.append(call.args[0].value) + return found + + +def every_spelling() -> dict[str, str]: + """`{path: pattern}`: the three places above, and any OTHER tracked + module (outside `tests/`) that spells `QUALIFIED_RE`, so a fourth copy is + pinned the day it is written rather than the day someone remembers it.""" + listed = git("ls-files", "-z", "--", "*.py", cwd=REPO).stdout + paths = sorted(p for p in listed.split("\0") if p + and not p.startswith("tests/")) + spelled: dict[str, str] = {} + for path in paths: + source = (REPO / path).read_bytes() + names = [SPELLINGS[path]] if path in SPELLINGS else ["QUALIFIED_RE"] + for name in names: + patterns = pattern_spelled(source, name) + assert len(patterns) <= 1, f"{path} spells {name} twice" + if patterns: + spelled[path] = patterns[0] + return spelled + + +def test_every_spelling_of_the_member_name_rule_is_the_same_pattern(): + """`MEMBER_RE` and both `QUALIFIED_RE` copies are ONE rule. The three + expected places must each be found (a rename that hid one would otherwise + leave this test checking two), and every spelling found is the same + string. To change the rule, change them together.""" + spelled = every_spelling() + gone = sorted(set(SPELLINGS) - set(spelled)) + assert not gone, ( + f"a spelling of the rule is gone: {gone} -- if it moved, update " + "SPELLINGS; if it was deleted, so is this pin") + assert len(set(spelled.values())) == 1, ( + "the member-name rule is spelled differently in:\n" + "\n".join( + f" {path}: {pattern}" + for path, pattern in sorted(spelled.items()))) + + +#: Names the one rule must judge the same everywhere it is spelled: the five +#: of the issue, `.github` and `...`, the leading `-` an option would carry, +#: and the shapes that were never names at all, against names with a `.`, +#: `_` or `-` after the first character. +ACCEPTED = [f"{ORG}/{SPARE}", "testorg/Ze.d", "testorg/Zed_", "testorg/Zed-", + "Ink.Router/Zed", "a/b", "9/9"] +NOT_ACCEPTED = ["testorg/_Zed", "testorg/.Zed", "testorg/-Zed", "_org/Zed", + ".org/Zed", "-org/Zed", "testorg/.github", "testorg/...", + "testorg/.", "testorg/..", "./Zed", "../Zed", "/Zed", + "testorg/", "testorg", "a/b/c", ""] + + +def test_every_spelling_judges_a_corpus_of_names_alike(): + """The strings above are one pattern; this says what that pattern MEANS, + so a reader sees the rule and a change that keeps the spellings equal but + moves the rule (to admit `.github`, say) has to move this too.""" + spelled = every_spelling() + assert spelled, "premise: at least one spelling was found" + for path, pattern in spelled.items(): + rule = re.compile(pattern) + for name in ACCEPTED: + assert rule.match(name), f"{path} refuses {name!r}" + for name in NOT_ACCEPTED: + assert not rule.match(name), f"{path} admits {name!r}" + module = family_module() + for name in ACCEPTED: + assert module._checked_member(name)[0] == name diff --git a/tests/test_family_member_path.py b/tests/test_family_member_path.py index c9f72759..f5c2bd2d 100644 --- a/tests/test_family_member_path.py +++ b/tests/test_family_member_path.py @@ -381,20 +381,21 @@ def test_add_refuses_a_dot_segment_as_malformed_before_anything_is_read( 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`).") + "name beginning with a letter or a digit; `.` and `..` are neither, " + "though a name may contain a dot after that (`Ze.d`).") 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.""" + """A dot AFTER a name's first character is still a name. `.github` is not + one any more: `MEMBER_RE` takes `validate-family.py`'s shape, in which a + name begins with a letter or a digit (#366), and the holder's own gate + refused a row naming it, so `add` can no longer write one. + `tests/test_family_member_name.py` holds that rule.""" 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" + assert module._checked_member(f"a.b/{SPARE}")[1] == SPARE # --- the controls -----------------------------------------------------------