diff --git a/scripts/family.py b/scripts/family.py index 432998b1..52dd568b 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -125,7 +125,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 +389,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 +416,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) 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 -----------------------------------------------------------