diff --git a/templates/family-root/scripts/validate-family.py b/templates/family-root/scripts/validate-family.py index 3046054a..72f3a145 100755 --- a/templates/family-root/scripts/validate-family.py +++ b/templates/family-root/scripts/validate-family.py @@ -23,6 +23,10 @@ terms. - `shape`: a commit-and-digest pin of the openRepoShape revision this root was cut from, `revision_kind: commit`, never a tag + - across the rows, that no two name ONE `project:`, in any case: + `members/IRRS` and `members/irrs` are one directory on macOS and Windows, + whatever `id:` each row records (#331). This one reads `family.yaml` + alone, so `--no-members` asks it too. - EVERY MEMBER, three ways: 1. the GITLINK this repository records at `members/` equals `members[].pin.commit` — the same lockstep rule an assembly root @@ -222,15 +226,18 @@ def _member_project_name(row: dict, seen: dict, report: Report) -> str | None: """The row's own `project:`, or None when nothing else about this member should run — it has none, or its name was already seen once in `members:`. Split from `_check_member` for #137: the two ways this - member's checks stop before they start, in one place.""" + member's checks stop before they start, in one place. + + A name seen once already stops this row's checks and says nothing: the + finding for it is `_check_member_names_unique`'s (#331), made before any + row is checked so `--no-members` makes it too, and made once, not here as + well.""" project = row.get("project") if not isinstance(project, str) or not project: report.finding("member-project", f"a member row has no `project:`: " f"{row!r}") return None if project in seen: - report.finding("member-duplicate", - f"{project} appears more than once in `members:`") return None seen[project] = True return project @@ -528,6 +535,79 @@ def _check_shape_pin(root: Path, manifest: dict, report: Report) -> None: _check_shape_pin_files(root, pin, rel, report) +def _check_member_names_unique(members: list, report: Report) -> None: + """No two rows of `members:` name ONE project, in any case — the + `member-duplicate` finding (#331). + + It reads `members:` alone, so it runs under `--no-members` too, the run a + private family's CI falls back to when its members cannot be checked out, + and it runs before any row is checked, as `member-duplicate-id` does + (#206). `_member_project_name` used to make this finding, and only the + members-checked loop reaches that, so `--no-members` passed a + `family.yaml` that named one row twice. It is made HERE and once, in + every mode: `_member_project_name` still skips a row it has already seen, + and says nothing. + + Two spellings that differ only in case are one row: `members/IRRS` and + `members/irrs` are ONE directory on a case-insensitive disk (macOS, + Windows), where a holder can mount only one of them, whatever `id:` each + row records. The rows' ids are no help here and `member-duplicate-id`'s + question is a different one: it asks whether one PROJECT is mounted + twice, this asks whether one DIRECTORY is named twice. + + THE FOLD IS `str.casefold()`, as the standard's other same-path-in-any-case + checks use it (`shape_materialize.py`, `adopt-project.py`). `family.py add` + only writes ASCII names (`checked_value`: `[A-Za-z0-9._/@+~-]`), and a + members-checked run refuses any other as `member-repository` + (`QUALIFIED_RE`) or `member-repository-name`. Under `--no-members` neither + runs, so a hand edit can put ANY string in `project:`, and a disk folds + more than ASCII case: the long s (U+017F) folds to `s`, and a final sigma + (U+03C2) to a small sigma (U+03C3), so `IRRS` and `irrs` spelt with the + long s, or a name in capital sigmas and the same name in small ones, are + one directory there, and `lower()` keeps each pair apart. On every name + `family.py` can write the two folds agree, and `Family.member` in + `family.py` compares with `lower()` (#206), so the tool that writes the + rows and this gate cannot disagree about a row the tool wrote. + `casefold()` is no one disk's own fold either: NTFS's table does not fold + the German sharp s (U+00DF) to `ss`, so for such a pair this may name a + collision that disk does not make, and APFS also ignores normalization, + which no fold here handles. It errs on the side of a gate that says one + row has to go. + + A row whose spelling an earlier row already has, to the byte, keeps the + finding it always had. A row that differs only in case, as a + case-insensitive disk folds it, names BOTH rows as written. Either way + each later row is one finding, against the first row that holds its name. + A row with no `project:` is `member-project`'s, and a row that is no + mapping is `member-row`'s: both are skipped here, as + `_check_member_ids_unique` skips them. + """ + spelled: set[str] = set() + first_folded: dict[str, str] = {} + for row in members: + if not isinstance(row, dict): + continue + project = row.get("project") + if not isinstance(project, str) or not project: + continue + if project in spelled: + report.finding("member-duplicate", + f"{project} appears more than once in `members:`") + continue + spelled.add(project) + first = first_folded.setdefault(project.casefold(), project) + if first != project: + report.finding( + "member-duplicate", + f"{first} and {project} are two rows of `members:` whose " + f"`project:` differs only in case, so `members/{first}` and " + f"`members/{project}` are one directory on a case-insensitive " + "disk (macOS, Windows), and a holder there can keep only one " + "of them. Whether the rows pin one project or two, one of " + "them has to go: keep the row whose `repository:` is the " + "member's and drop the other.") + + def _check_member_ids_unique(root: Path, members: list, report: Report) -> None: """No two rows record ONE `id:` — that is one project pinned twice (#206). @@ -658,6 +738,7 @@ def _check_members(root: Path, members: list, no_members: bool, `family.yaml`'s ids (#206) and `.gitmodules`'s paths (#300) — come first, so `--no-members` asks them too. """ + _check_member_names_unique(members, report) _check_member_ids_unique(root, members, report) _check_gitmodules_paths(root, report) if no_members: diff --git a/tests/test_validate_family_duplicates.py b/tests/test_validate_family_duplicates.py new file mode 100644 index 00000000..ac71aa92 --- /dev/null +++ b/tests/test_validate_family_duplicates.py @@ -0,0 +1,327 @@ +# SPDX-License-Identifier: Apache-2.0 +"""`validate-family.py` finds `member-duplicate` under `--no-members` too, and +in any case (#331). + +TWO FAULTS IN ONE FINDING. `member-duplicate` was raised in +`_member_project_name`, which only the members-checked loop reaches, and +`_check_members` returns before that loop under `--no-members`: the run a +private family's CI falls back to when its members cannot be checked out, so +a `family.yaml` that named one row twice passed the one gate that ran. And it +compared `project:` EXACTLY, so rows `IRRS` and `irrs` with distinct ids +passed everywhere, though `members/IRRS` and `members/irrs` are ONE directory +on macOS and Windows. `family.py add` refuses a case variant now, so only a +hand edit makes such rows; this is the gate that finds the hand edit. + +The finding is now its own function, `_check_member_names_unique`, which +reads `members:` alone and is called before the `--no-members` return, as +`member-duplicate-id` (#206, `tests/test_family.py`) is. It is made ONCE, +whatever the mode, and an exact repeat keeps the text it always had. Case is +folded with `str.casefold()`, so a pair a disk holds as one directory and +`str.lower()` keeps apart (the long s, the sigmas) is found too. + +The same family and the same offline rules as `tests/test_family.py`: the +fixture is imported rather than rebuilt, because a holder made any other way +would test a different holder. Members are scaffolded into local bare +repositories and nothing is created on GitHub. + +THE ROWS ARE BUILT IN `family.yaml` AND NEVER ON DISK, except in the one test +that mounts a second member at `members/irrs`: `--no-members` reads no +member, and a fixture with two directories whose names differ only in case +cannot exist on macOS or Windows (`mount_twice` spells its second row `IRRS2` +for that reason). That one test says so and skips there; every other row of +this module runs on all three platforms. +""" + +from __future__ import annotations + +import pytest + +from conftest import git, run_script +from test_family import ( # noqa: F401 + ORG, SPARE, family, family_module, holder, manifest, tree_digest, + validate, +) + +#: The finding an exact repeat has always had, byte for byte. A holder's CI +#: matches on text, so it is pinned here and not rebuilt from a template. +EXACT = "FINDING member-duplicate: IRRS appears more than once in `members:`" + +#: A commit nothing can resolve. A repeated row pinned to it is what makes the +#: members-checked loop refuse (`member-commit-unresolvable`, exit 2, and the +#: finding lost) if `_member_project_name` ever stops skipping a name it has +#: already seen: a repeat copied byte for byte checks out as cleanly as the +#: row it repeats, so it would never notice. +NOWHERE = "0" * 40 + +#: Pairs one case-insensitive disk holds as one directory and `str.lower()` +#: keeps apart, which only `str.casefold()` finds: IRRS and IRR with the long s +#: (U+017F, folds to `s`), and capital sigmas against small ones (`lower()` +#: gives the final sigma U+03C2 where the small spelling has U+03C3). Written as +#: escapes so no editor or console re-encodes them. +FOLDS = { + "long-s": ("IRRS", "irr\u017f"), + "sigma": ("\u03a3\u0391\u03a3", "\u03c3\u03b1\u03c3"), +} + + +def irrs_row(holder) -> dict: + """IRRS's row, as the manifest reads it back.""" + return next(row for row in manifest(holder)["members"] + if row["project"] == "IRRS") + + +def spelled(row: dict, project: str, member_id: str) -> dict: + """`row` under another spelling of the project, with an id of its own. + + The id is DISTINCT on purpose: `member-duplicate-id` asks whether one + project is mounted twice, and every row here is a different question — + whether one DIRECTORY is named twice. + """ + assert member_id != row["id"], "the point is a distinct id" + return {**row, "project": project, "id": member_id, + "repository": f"{ORG}/{project}", "path": f"members/{project}"} + + +def rewrite(holder, rows: list) -> None: + """The holder's `members:` block replaced by `rows`, and committed.""" + family_module().Family.open(holder).write_members(rows) + git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-qam", + "Rewrite the members", cwd=holder) + + +def findings(result) -> list[str]: + """Every `FINDING` line, in the order they were printed.""" + return [line for line in result.stderr.splitlines() + if line.startswith("FINDING")] + + +def case_pair(first: str, second: str) -> str: + """The head of the finding for two rows that differ only in case: both + rows AS WRITTEN, the earlier one first, then what makes them one.""" + return (f"FINDING member-duplicate: {first} and {second} are two rows of " + "`members:` whose `project:` differs only in case, so " + f"`members/{first}` and `members/{second}` are one directory on " + "a case-insensitive disk") + + +# --- one row written twice --------------------------------------------------- + +def test_a_row_written_twice_is_found_under_no_members(holder): + """The run a private family's CI falls back to. The duplicate is a fact of + `family.yaml` alone, so it is found beside the members that run skips.""" + row = irrs_row(holder) + other = next(r for r in manifest(holder)["members"] + if r["project"] == "IRSS") + rewrite(holder, [row, dict(row), other]) + result = validate(holder, "--no-members") + assert result.returncode == 1, result.stderr + result.stdout + assert findings(result) == [EXACT], result.stderr + assert "member(s) SKIPPED (--no-members)" in result.stdout + + +@pytest.mark.parametrize("extra", [(), ("--pins",)], + ids=["members-checked", "pins"]) +@pytest.mark.parametrize("repeat", ["identical", "other-pin"]) +def test_with_members_checked_a_repeat_is_one_finding_not_two(holder, extra, + repeat): + """`_member_project_name` used to make this finding and still stops that + row's checks, silently. A finding made in both places would be one fault + reported twice, and the count is what says so. + + THE REPEAT IS ALSO PINNED TO A COMMIT NOWHERE (`other-pin`), because a + repeat that is byte for byte the row it repeats passes every check the + first one did, so deleting the skip would cost it nothing: it is the row + that would be checked, refused `member-commit-unresolvable` (exit 2) and + lose the finding, that holds the skip in place. + """ + row = irrs_row(holder) + again = (dict(row) if repeat == "identical" else + {**row, "pin": {**row["pin"], "commit": NOWHERE}}) + other = next(r for r in manifest(holder)["members"] + if r["project"] == "IRSS") + rewrite(holder, [row, again, other]) + result = validate(holder, *extra) + assert result.returncode == 1, result.stderr + result.stdout + assert findings(result) == [EXACT], result.stderr + assert result.stderr.count("member-duplicate") == 1, result.stderr + assert "REFUSED" not in result.stderr, result.stderr + + +# --- two rows that differ only in case --------------------------------------- + +@pytest.mark.parametrize("first,second", [("IRRS", "irrs"), ("irrs", "IRRS")], + ids=["upper-first", "lower-first"]) +def test_rows_that_differ_only_in_case_are_found_under_no_members( + holder, first, second): + """Both rows are named as WRITTEN, the earlier one first, whichever of + the two spellings that is, and the finding says why they are one.""" + base = irrs_row(holder) + rewrite(holder, [spelled(base, first, "irrs-first"), + spelled(base, second, "irrs-second")]) + result = validate(holder, "--no-members") + assert result.returncode == 1, result.stderr + result.stdout + assert len(findings(result)) == 1, result.stderr + assert findings(result)[0].startswith(case_pair(first, second)), \ + result.stderr + assert "member-duplicate-id" not in result.stderr + assert "member(s) SKIPPED (--no-members)" in result.stdout + + +def test_a_case_variant_is_not_the_exact_finding(holder): + """The two are different faults in different words: an exact repeat says + it appears more than once, and a case variant says it is one directory on + some disks. A reader told the wrong one goes looking for a row that is + not there.""" + base = irrs_row(holder) + rewrite(holder, [base, spelled(base, "irrs", "irrs-twin")]) + result = validate(holder, "--no-members") + assert result.returncode == 1, result.stderr + result.stdout + assert len(findings(result)) == 1, result.stderr + assert "appears more than once" not in result.stderr, result.stderr + + +@pytest.mark.parametrize("pair", FOLDS.values(), ids=FOLDS.keys()) +def test_rows_that_a_disk_folds_but_lower_does_not_are_found_under_no_members( + holder, pair): + """`--no-members` reads any string as a `project:` (the checks that hold a + name to ASCII run only with members checked), so a hand edit can write a + pair that `str.lower()` keeps apart and a case-insensitive disk does not. + The fold is `casefold()`, as the standard's other same-path-in-any-case + checks use. The child is run in UTF-8 mode so the names print as written + on every platform.""" + first, second = pair + assert first.lower() != second.lower(), "the point is that lower() fails" + base = irrs_row(holder) + rewrite(holder, [spelled(base, first, "irrs-first"), + spelled(base, second, "irrs-second")]) + result = run_script(holder / "scripts" / "validate-family.py", + "--no-members", cwd=holder, + env={"PYTHONUTF8": "1"}) + assert result.returncode == 1, result.stderr + result.stdout + assert len(findings(result)) == 1, result.stderr + assert findings(result)[0].startswith(case_pair(first, second)), \ + result.stderr + + +def test_the_case_finding_does_not_assume_one_project(holder): + """Rows with distinct ids may be two projects (`OrgA/IRRS` and + `OrgB/irrs`): the fault is one DIRECTORY, not one project pinned twice, so + the finding says so and says one row has to go. It names no command, + because `family.py remove` cannot take out a row that was never mounted, + which is the only kind a macOS or Windows holder can hold.""" + base = irrs_row(holder) + rewrite(holder, [base, spelled(base, "irrs", "irrs-twin")]) + result = validate(holder, "--no-members") + assert result.returncode == 1, result.stderr + result.stdout + (line,) = findings(result) + assert line.startswith(case_pair("IRRS", "irrs")), line + assert ("and a holder there can keep only one of them. Whether the rows " + "pin one project or two, one of them has to go: keep the row " + "whose `repository:` is the member's and drop the other." + ) in line, line + assert "pins each project once" not in result.stderr + assert "family.py" not in line + + +def test_each_later_row_is_one_finding_against_the_first_that_holds_its_name( + holder): + """Three spellings of one name and one of them twice: every row after the + first is ONE finding, a case variant names the first row it collides with + and an exact repeat keeps its own text, in file order.""" + base = irrs_row(holder) + lower = spelled(base, "irrs", "irrs-two") + title = spelled(base, "Irrs", "irrs-three") + rewrite(holder, [base, lower, title, dict(lower)]) + result = validate(holder, "--no-members") + assert result.returncode == 1, result.stderr + result.stdout + found = findings(result) + assert len(found) == 3, result.stderr + assert found[0].startswith(case_pair("IRRS", "irrs")), found + assert found[1].startswith(case_pair("IRRS", "Irrs")), found + assert found[2] == ("FINDING member-duplicate: irrs appears more than " + "once in `members:`"), found + + +def test_distinct_names_are_not_folded_into_one(holder): + """A name that merely starts with another's, or ends with a digit, is its + own row: the fold is CASE alone.""" + base = irrs_row(holder) + other = next(r for r in manifest(holder)["members"] + if r["project"] == "IRSS") + rewrite(holder, [base, spelled(base, "IRRS2", "irrs2"), + spelled(base, "IRR", "irr"), other]) + result = validate(holder, "--no-members") + assert result.returncode == 0, result.stderr + result.stdout + assert "family ok: InkRouter (inkrouter), 4 member(s)" in result.stdout + + +# --- a holder with nothing wrong --------------------------------------------- + +@pytest.mark.parametrize("extra", [(), ("--pins",), ("--no-members",)], + ids=["members-checked", "pins", "no-members"]) +def test_distinct_names_pass_in_every_mode(holder, extra): + """The family `init` and `add` made: two members, two names.""" + result = validate(holder, *extra) + assert result.returncode == 0, result.stderr + result.stdout + assert findings(result) == [] + assert "family ok: InkRouter (inkrouter), 2 member(s)" in result.stdout + + +def test_a_row_with_no_project_is_not_this_findings_to_make(holder): + """A row that is no mapping and a row with no `project:` are + `member-row`'s and `member-project`'s, which only the members-checked loop + asks. This one skips them rather than crash on them: a traceback exits 1, + which is a finding's code.""" + # BYTES, so Windows does not rewrite the whole manifest with CRLF. + text = (holder / "family.yaml").read_bytes().decode("utf-8") + (holder / "family.yaml").write_bytes( + (text + " - nameless\n - id: nameless\n").encode("utf-8")) + result = validate(holder, "--no-members") + assert result.returncode == 0, result.stderr + result.stdout + assert "Traceback" not in result.stderr + assert findings(result) == [] + + +# --- both rows mounted ------------------------------------------------------- + +@pytest.fixture +def case_sensitive(tmp_path): + """Skips on a disk that holds `Dir` and `dir` as one, as macOS and + Windows do by default: the second mount cannot exist there.""" + (tmp_path / "CaseProbe").mkdir() + if (tmp_path / "caseprobe").exists(): + pytest.skip("this disk is case-insensitive, so members/IRRS and " + "members/irrs cannot both be mounted; every other row " + "of this module builds its rows in family.yaml and runs " + "here") + + +@pytest.mark.parametrize("extra", [(), ("--pins",), ("--no-members",)], + ids=["members-checked", "pins", "no-members"]) +def test_two_mounted_rows_that_differ_only_in_case_are_one_finding( + family, holder, case_sensitive, extra): + """BOTH ROWS REAL, in every way the gate asks about: mounted at the path + their name makes, pinned at the commit mounted there, digested, and the + `project.yaml` inside says the id the row says — `irrs` mounts a + different member (IRQS, id `irqs`) so the ids are distinct and the only + thing wrong is the name. Members checked or not, it is ONE finding.""" + git("-c", "protocol.file.allow=always", "submodule", "add", "-q", "--", + str(family["remotes"] / f"{SPARE}.git"), "members/irrs", cwd=holder) + mounted = holder / "members" / "irrs" + commit = git("rev-parse", "HEAD", cwd=mounted).stdout.strip() + base = irrs_row(holder) + twin = spelled(base, "irrs", SPARE.lower()) + twin["pin"] = {**base["pin"], "commit": commit, + "tree_sha256": tree_digest(mounted, commit)} + family_module().Family.open(holder).write_members( + [*manifest(holder)["members"], twin]) + git("add", "--", ".gitmodules", "members/irrs", "family.yaml", + cwd=holder) + git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-qm", + "Mount a second member that differs from IRRS only in case", + cwd=holder) + result = validate(holder, *extra) + assert result.returncode == 1, result.stderr + result.stdout + assert len(findings(result)) == 1, result.stderr + assert findings(result)[0].startswith(case_pair("IRRS", "irrs")), \ + result.stderr