diff --git a/README.md b/README.md index 8f161956..824842fb 100644 --- a/README.md +++ b/README.md @@ -1209,7 +1209,7 @@ one verdict, and its exit code: | `COMPLIANT` | 0 | no row is a `FINDING`, and the pin names this standard's commit | | `COMPLIANT, SHAPE BEHIND (n upstream-changed, m upstream-added)`, or `(pin -> , no copied file differs)` | 1 | valid, and the standard has moved on | | `DRIFTED (…)` | 1 | a copy was edited, deleted or no longer maps onto the standard, or a leg (a member, in a holder) is off its pin | -| `MISPLACED (n paths)` | 1 | a tracked path sits in a leg the path policy puts elsewhere, or a submodule in a leg other than the one its `submodules:` record names | +| `MISPLACED (n paths)` | 1 | a tracked path sits in a leg the path policy puts elsewhere (unless `answered_paths:` answers it into that leg), or a submodule in a leg other than the one its `submodules:` record names | | `INVALID ()` | 1 | a validator went red, or (`merge`) the index holds an unmerged path | | `NOT A SHAPE ROOT` | 2 | neither manifest is there — adopt, or scaffold | diff --git a/adopt-project.py b/adopt-project.py index a8b1ef34..0f9f4dae 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -7115,13 +7115,26 @@ def _answered_legs(entries: list, paths: list[str]) -> dict: yet cannot have been answered inconsistently. `execute` refuses all four too: the null one in `_refuse_an_unrunnable_plan`, the other three in #168's `_refuse_what_check_finds`. + + EACH PATH LOOKS UP ITS OWN NAMES (#914), as the doctor's + `answered_questions` does: the entries `_covering` could find covering + it are the names `_covering_names` lists, and one entry covers it where + exactly one of those names is a plan entry's, spelled by one entry. So a + path costs its depth and not the plan's length, with the answer holding + every entry against every path gave. """ - entry_paths = [str(e.get("path")) for e in entries] - legs = {str(e.get("path")): str(e.get("leg")) for e in entries} + spelled: dict[str, int] = {} + legs: dict[str, str] = {} + for entry in entries: + name = str(entry.get("path")) + spelled[name] = spelled.get(name, 0) + 1 + legs[name] = str(entry.get("leg")) out: dict[str, tuple[str, str]] = {} for path in paths: - covering = _covering(entry_paths, path) - if len(covering) == 1 and legs[covering[0]] in LEG_VALUES: + covering = [name for name in dict.fromkeys(_covering_names(path)) + if name in spelled] + if (len(covering) == 1 and spelled[covering[0]] == 1 + and legs[covering[0]] in LEG_VALUES): out[path] = (legs[covering[0]], covering[0]) return out @@ -15118,6 +15131,12 @@ def _account_for(before: dict, after: dict, drops: list[str]) -> tuple[dict, lis A path in TWO places is as much a finding as a path in none — the second is data loss and the first is two repositories owning one file. + + THE DROP IS ASKED BEFORE THE ROOT IS (#914). A path the plan dropped is + gone from the split before the shape writes, so a blob the root holds + at it is the shape's, which happens to be the same bytes: the path is + counted `drop`, as the plan says, and not `root`. Only the count moves; + a dropped path in a leg is counted, or found, as before. """ dropped = {p.rstrip("/") for p in drops} counts = {"spec": 0, "code": 0, "root": 0, "drop": 0} @@ -15125,13 +15144,14 @@ def _account_for(before: dict, after: dict, drops: list[str]) -> tuple[dict, lis for path, oid in sorted(before.items()): landings = [where for where in after.get(path, []) if where.split(":", 1)[1] == oid] - if len(landings) == 1: + drop = any(path == d or path.startswith(d + "/") for d in dropped) + if drop and landings in ([], [f"root:{oid}"]): + counts["drop"] += 1 + elif len(landings) == 1: counts[landings[0].split(":", 1)[0]] += 1 elif landings: findings.append(f"FINDING adopt-duplicated: {_spelled(path)} is " "in " + ", ".join(landings)) - elif any(path == d or path.startswith(d + "/") for d in dropped): - counts["drop"] += 1 else: findings.append(f"FINDING adopt-lost: {_spelled(path)} " f"({oid[:12]}) is in no leg, not in the root " diff --git a/docs/handbook.html b/docs/handbook.html index 1dec4307..75b04198 100644 --- a/docs/handbook.html +++ b/docs/handbook.html @@ -1552,7 +1552,7 @@

Bootstrap is COPIED into the project, not fetched

- +

Reference

Keeping a project’s shape current

@@ -1614,7 +1614,7 @@

Keeping a project’s shape current

- +

Reference

Is this repository compliant? openRepoShape --doctor

@@ -1669,7 +1669,7 @@

Is this repository compliant? openRepoShape --doctor

COMPLIANT0no row is a FINDING, and the pin names this standard’s commit COMPLIANT, SHAPE BEHIND (n upstream-changed, m upstream-added), or (pin <a> -> <b>, no copied file differs)1valid, and the standard has moved on DRIFTED (…)1a copy was edited, deleted or no longer maps onto the standard, or a leg (a member, in a holder) is off its pin - MISPLACED (n paths)1a tracked path sits in a leg the path policy puts elsewhere, or a submodule in a leg other than the one its submodules: record names + MISPLACED (n paths)1a tracked path sits in a leg the path policy puts elsewhere (unless answered_paths: answers it into that leg), or a submodule in a leg other than the one its submodules: record names INVALID (<rows>)1a validator went red, or (merge) the index holds an unmerged path NOT A SHAPE ROOT2neither manifest is there — adopt, or scaffold diff --git a/scripts/shape_materialize.py b/scripts/shape_materialize.py index d7f897cb..1d4dbdb1 100644 --- a/scripts/shape_materialize.py +++ b/scripts/shape_materialize.py @@ -5018,6 +5018,17 @@ def _placeholders_of(line: str) -> list[str]: #: Which placeholder carries the value a finding of the validator is about, #: for the ones a tool is handed a value for. A finding with no row is still #: refused, in the validator's words and without a flag to point at. +#: +#: THE TWO RECORDS' CODES HAVE NO ROW, ON PURPOSE (#914): #189's +#: `submodules:` (`manifest-submodules`, `manifest-submodule-*`) and #525's +#: `answered_paths:` (`manifest-answered-paths`, `manifest-answered-*`), +#: which the validator spells from its `_Record`s. No placeholder carries a row +#: of either: `adopt-project.py execute` appends both to `project.yaml` after +#: the render its gate asks, so no tool is handed a value of theirs and no +#: such finding can arise there today. Should a caller render a record into +#: that question, a finding about one is about no placeholder, and +#: `manifest_judged` answers None: every value is left to the validator, the +#: safe direction, and none is named as one a flag carries. FINDING_PLACEHOLDER = { "manifest-id": "PROJECT_ID", "manifest-topic": "PROJECT_ID", "manifest-name": "PROJECT_NAME", "manifest-elected-by": "ELECTED_BY", diff --git a/shape-doctor.py b/shape-doctor.py index 5c7a6be2..c3783f28 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -241,12 +241,19 @@ `source_commit:`. A row naming a directory of its leg without the `/` a directory's row ends with answers nothing under it, as a plan entry spelled so covers nothing, and the leg's entry names it under `unslashed`, said and -never a finding. - -AN EMPTY `.gitmodules` ASKS NOTHING (#655). One that holds no byte -- what a -plain `git rm` of a leg's only submodule leaves, tracked -- registers no -submodule, so the policy's question of it has no subject. The leg's entry -names it under `empty_gitmodules`, the row says so, and it is no question. +never a finding. A row naming a file of its leg WITH that `/` answers nothing +either, since no file is under it (#914): the leg's entry names it under +`slashed`, said the same way. + +AN EMPTY `.gitmodules` ASKS NOTHING (#655, #914). One that holds no section +-- no byte, which a plain `git rm` of a leg's only submodule leaves tracked, +or only blank lines, comments and a byte-order mark, which git's own reader +passes over -- registers no submodule, so in a leg that holds none the +policy's question of it has no subject. The leg's entry names it under +`empty_gitmodules`, the row says so and how to remove it, and it is no +question. Beside a submodule of its leg it stays the question it was, since +no section registers that submodule: the leg's entry names each under +`unregistered_submodules`, and the row says how to register it or remove it. A SUBMODULE MOVED OR REMOVED SINCE THE ADOPTION (#525). Its row is a person's to change, by hand, in the pull request that bumps the pins; no @@ -632,13 +639,21 @@ def not_in_the_standard(detail: str) -> int: #: leg holds (#525); present only where there is one. UNHELD = "unheld" #: The key of a leg's entry naming its `.gitmodules` where that file holds -#: no byte, so the policy has nothing to ask of it (#655); present only on a -#: leg that has one. +#: no section and the leg no submodule, so the policy has nothing to ask of +#: it (#655, #914); present only on a leg that has one. EMPTY_GITMODULES = "empty_gitmodules" +#: The key of a leg's entry naming each submodule it holds beside a +#: `.gitmodules` that holds no section, so that nothing registers it (#914); +#: present only on a leg that has one. +UNREGISTERED_SUBMODULES = "unregistered_submodules" #: The key of a leg's entry naming each `answered_paths:` row for that leg #: whose path is a directory of it written without its `/` (#655); present #: only on a leg that has one. UNSLASHED = "unslashed" +#: The key of a leg's entry naming each `answered_paths:` row for that leg +#: whose path is a file of it written with the `/` a directory's row ends +#: with (#914); present only on a leg that has one. +SLASHED = "slashed" #: The `why` of a row whose every path in the wrong leg is a submodule moved #: since the adoption (#525). Its NEXT is this doctor again, as the #: `manifest kinds` row's is for a file only a person can mend @@ -8090,8 +8105,10 @@ def held_clauses(detail: dict) -> str: clauses = (kept_submodules(detail.get("submodules") or []), answered_clause(legs), empty_clause(legs), + unregistered_clause(legs), answers_clause(detail.get(ANSWERED_PATHS) or []), unslashed_clause(legs), + slashed_clause(legs), unheld_clause(detail.get(UNHELD) or [])) said = "; ".join(clause for clause in clauses if clause) return f"; {said}" if said else "" @@ -8127,16 +8144,70 @@ def answers_clause(answered: list) -> str: def empty_clause(legs: list) -> str: - """The clause naming each leg's `.gitmodules` that holds no byte, or "" - where none does (#655). SAID, as the one the record answers is: a reader - who knows the policy asks about every `.gitmodules` is owed why this one - was not asked.""" + """The clause naming each leg's `.gitmodules` that holds no section, in a + leg that holds no submodule, or "" where none does (#655, #914). SAID, as + the one the record answers is: a reader who knows the policy asks about + every `.gitmodules` is owed why this one was not asked. AND HOW TO REMOVE + IT (#914), since no record can answer it and nothing needs it. `git rm + -f`, because a plain `git rm` refuses the file where it is staged and not + committed (what a plain `git rm` of a leg's only submodule leaves) or + changed on disk and not staged (measured, git 2.43); `-f` takes it out of + the index and the disk in each state, and discards no section, since the + clause is printed only where the disk copy holds none.""" empty = [path for leg in legs for path in leg.get(EMPTY_GITMODULES) or []] if not empty: return "" return (f"the policy asks nothing of {len(empty)} `.gitmodules` that " - "hold(s) no byte and so register(s) no submodule (#655): " - f"{', '.join(empty)}") + "hold(s) no section and so register(s) no submodule, in a leg " + f"that holds none (#655, #914): {', '.join(empty)}; nothing " + "needs one, and `git rm -f .gitmodules` in its leg removes it") + + +def unregistered_clause(legs: list) -> str: + """The clause naming each submodule a leg holds beside a `.gitmodules` + that holds no section, or "" where there is none (#914). + + SAID, because that `.gitmodules` is not the empty one `empty_clause` + names: it stays the question it was, and the way out is not to remove + it. No section registers the submodule, so a recursive clone of the leg + exits 0 and leaves its directory empty, and `git submodule status` there + exits 128 with `no submodule mapping found in .gitmodules` (measured, + git 2.43). Neither record can say where its repository is, and `git + submodule add` refuses a path the index holds: the person writes its + section, or removes the gitlink. + + THE REMOVAL TYPED AS PRINTED WORKS (measured, git 2.43). `git rm + --cached` dies with `please stage your changes to .gitmodules` where the + leg's tracked `.gitmodules` has an edit that is not staged, so the clause + says to stage it first; and it names the path as the leg spells it, + because the row's own `code/vendor/lib` is the project's spelling, which + the leg does not know. + """ + held = [path for leg in legs + for path in leg.get(UNREGISTERED_SUBMODULES) or []] + if not held: + return "" + return (f"{len(held)} submodule(s) sit beside a `.gitmodules` of their " + "leg that holds no section, so nothing registers them and a " + "recursive clone of the leg leaves each empty (#914): " + f"{first_three(held)}; give each a section with its `path` and " + "`url` in that `.gitmodules`, or, once that `.gitmodules` is " + "staged (`git add .gitmodules`), remove its gitlink with `git rm " + "--cached ` in its leg") + + +def slashed_clause(legs: list) -> str: + """The clause naming each `answered_paths:` row that names a file of its + leg with the `/` a directory's row ends with, or "" where none does + (#914). SAID, as `unslashed_clause` says the other spelling, and for + the same reason: such a row answers nothing, since no file is under a + file, and never a finding.""" + rows = [path for leg in legs for path in leg.get(SLASHED) or []] + if not rows: + return "" + return (f"{len(rows)} row(s) of {THE_ANSWERS} name a file with the `/` " + "only a directory's row ends with, so each answers nothing " + f"(#914): {first_three(rows)}") def unslashed_clause(legs: list) -> str: @@ -8513,9 +8584,12 @@ def answered_questions(adopt, review: list, role: str, files: list, return still, answered -def empty_gitmodules(adopt, review: list, mount: Path) -> tuple: +def empty_gitmodules(adopt, review: list, mount: Path, rel: str, + paths: list, held: list) -> tuple: """The leg's questions less its `.gitmodules` where that file holds no - byte, and that file's path in the row, or [] (#655). + section and the leg no submodule, and that file's path in the row, or + [] (#655, #914); and each submodule the leg holds beside such a file, as + its path in the row, or [] (#914). AN EMPTY `.gitmodules` REGISTERS NO SUBMODULE, so the question rule `ambiguous-gitmodules` asks of it -- which leg owns the submodules it @@ -8525,30 +8599,76 @@ def empty_gitmodules(adopt, review: list, mount: Path) -> tuple: leaves `.gitmodules` out, and `submodules:` answers it only where the leg keeps a submodule. Not asking is the fix, rather than a row a person writes by hand into the manifest to quiet a question that has - no subject. - - NO BYTE, AND A REGULAR FILE, read on this disk as `leg_tracked_files` - reads a size: the file git's submodule commands read. One holding - anything -- a blank line, a comment, a section -- is one only git's own - reader could say registers nothing, so it stays the question it was. - git refuses to index a `.gitmodules` that is a symlink, and reads the - index's copy where the disk has none, so neither is the empty file. - """ + no subject, and the row says how to remove it (`empty_clause`). + + EMPTY IS NO SECTION (#914), as `holds_no_section` reads it: no byte, or + only what git's own config reader passes over -- a byte-order mark, + blank lines, comments. One that holds a section, even one with no key, + stays the question it was. Read on this disk, a regular file and not a + link, as `leg_tracked_files` reads a size: the file git's submodule + commands read. git refuses to index a `.gitmodules` that is a symlink, + and reads the index's copy where the disk has none, so neither is the + empty file. + + BUT NOT BESIDE A SUBMODULE (#914). In a leg that holds a gitlink, a + `.gitmodules` with no section is no emptied file: it fails to register + that gitlink, and the question of which leg owns the leg's submodules + has a subject. So it stays the question it was -- asked, or answered by + `submodules:` (`gitmodules_the_record_answers`) -- and each gitlink + `held` is returned to be named with what to do (`unregistered_clause`). + """ + if adopt.GITMODULES not in paths or not holds_no_section( + mount / adopt.GITMODULES): + return review, [], [] + if held: + return review, [], [f"{rel}/{path}" for path in held] for row in review: - if (row["path_in_leg"] == adopt.GITMODULES - and holds_no_byte(mount / adopt.GITMODULES)): + if row["path_in_leg"] == adopt.GITMODULES: return [other for other in review if other is not row], [ - row["path"]] - return review, [] - - -def holds_no_byte(path: Path) -> bool: - """Is `path` a regular file, not a link, of no byte (#655)?""" + row["path"]], [] + return review, [], [] + + +#: The bytes git's config reader passes over before the first line of a file +#: (#914): a UTF-8 byte-order mark, as Windows Notepad writes one. Only the +#: whole mark, and only at the start: git refuses a part of one. +UTF8_BOM = b"\xef\xbb\xbf" +#: What git's config reader counts as blank between the lines it reads +#: (#914): its own `isspace`, which leaves out the vertical tab and the form +#: feed Python's `bytes.strip` would take, and which git refuses. +CONFIG_BLANKS = b" \t\r" +#: The two bytes that open a comment for git's config reader, to the line's +#: end (#914). +CONFIG_COMMENTS = (b"#", b";") + + +def holds_no_section(path: Path) -> bool: + """Is `path` a regular file, not a link, that holds no section and no + key: no byte, or only a byte-order mark, blank lines and comments, which + git's config reader passes over (#655, #914)? + + git's reader at the top of a file is a loop over bytes: a `\\n` ends a + comment, a blank is passed over, `#` or `;` opens a comment, and + anything else opens a section (`[`), opens a key, or is a bad line. So + a file whose every line, after its blanks, is empty or opens a comment + holds neither, and `git config --file` lists nothing from it and exits + 0 (measured, git 2.43). A file that cannot be read is not known to be + empty. + """ try: found = path.lstat() + if not stat.S_ISREG(found.st_mode): + return False + data = path.read_bytes() except OSError: return False - return stat.S_ISREG(found.st_mode) and found.st_size == 0 + if data.startswith(UTF8_BOM): + data = data[len(UTF8_BOM):] + for line in data.split(b"\n"): + line = line.lstrip(CONFIG_BLANKS) + if line and not line.startswith(CONFIG_COMMENTS): + return False + return True def unslashed_rows(answers: dict, role: str, rel: str, files: list) -> list: @@ -8574,6 +8694,26 @@ def unslashed_rows(answers: dict, role: str, rel: str, files: list) -> list: return [f"{rel}/{path}" for path in rows if path in directories] +def slashed_rows(answers: dict, role: str, rel: str, files: list) -> list: + """Each `answered_paths:` row for THIS leg whose path is a file of it + written with the `/` a directory's row ends with, as its path in the + row (#914). + + SUCH A ROW ANSWERS NOTHING: `_covering` reads a path with its `/` as a + directory's, and no file is under a file. It is `unslashed_rows`' other + spelling, said the same way and for the same reason, and never read as + the file's row nor made a finding. `execute` never writes one, since + `check` finds the plan entry spelled so (`plan-empty-entry`, beside + `plan-uncovered` for the file); a hand edit does. + """ + rows = [path for path, row in answers.items() + if row.get("leg") == role and path.endswith("/")] + if not rows: + return [] + named = set(files) + return [f"{rel}/{path}" for path in rows if path[:-1] in named] + + def audit_leg(adopt, policy, patterns: list, role: str, rel: str, root: Path, record: dict, answers: dict) -> tuple: """ONE leg: its summary, the paths in the wrong leg, the questions, and @@ -8625,14 +8765,16 @@ def audit_leg(adopt, policy, patterns: list, role: str, rel: str, (`gitmodules_the_record_answers`): the file is placed, and the leg's entry names it under `answered` instead of as a question. Otherwise a real adoption, whose leg always holds the `.gitmodules` that - registers its submodules, could never read `ok`. One that holds no byte - asks nothing at all (`empty_gitmodules`, #655). + registers its submodules, could never read `ok`. One that holds no + section, in a leg that holds no submodule, asks nothing at all + (`empty_gitmodules`, #655, #914). AND `answered_paths:` IS READ FOR THE QUESTIONS AND FOR THE PATHS IN THE WRONG LEG ALIKE (#525, #655): `answered_questions` takes from each those - the record answers into this leg, which are returned apart, and + the record answers into this leg, which are returned apart, `unslashed_rows` names the rows for this leg that name a directory - without its `/`. + without its `/`, and `slashed_rows` those that name a file with one + (#914). AND A LEG'S `single-repository.yaml` IS ASKED THE RECORD'S QUESTION ONLY WHERE IT IS THE RECORD (#691). The walk asks it by name; `plan`'s own @@ -8692,8 +8834,9 @@ def audit_leg(adopt, policy, patterns: list, role: str, rel: str, submodules, wrong = leg_submodules(policy, role, rel, held, record) answered = gitmodules_the_record_answers(adopt, submodules, unwritable) misplaced, review, settled = walked_findings(walked, role, rel, answered) - review, empty = empty_gitmodules(adopt, review, mount) paths = [path for path, _ in kept] + review, empty, unregistered = empty_gitmodules(adopt, review, mount, rel, + paths, held) review, recorded = answered_questions(adopt, review, role, paths, answers) misplaced, against = answered_questions(adopt, misplaced, role, paths, answers) @@ -8708,7 +8851,9 @@ def audit_leg(adopt, policy, patterns: list, role: str, rel: str, "misplaced": len(misplaced), "review_required": len(review)}) for key, said in ((ANSWERED, settled), (EMPTY_GITMODULES, empty), - (UNSLASHED, unslashed_rows(answers, role, rel, paths))): + (UNREGISTERED_SUBMODULES, unregistered), + (UNSLASHED, unslashed_rows(answers, role, rel, paths)), + (SLASHED, slashed_rows(answers, role, rel, paths))): if said: # Only when there is one, so that no other leg's entry changes. entry[key] = said diff --git a/tests/test_adopt_answered_legs.py b/tests/test_adopt_answered_legs.py new file mode 100644 index 00000000..bfd3ee61 --- /dev/null +++ b/tests/test_adopt_answered_legs.py @@ -0,0 +1,122 @@ +# SPDX-License-Identifier: Apache-2.0 +"""`_answered_legs` looks up each path's own names (#914's L1-85). + +It held every plan entry against every path through `_covering`, which was +6.90 s for 120,000 files and 300 entries in lane 1's measurement, and 3.65 s +on this branch's host. Each path now looks up the names `_covering_names` +lists, which costs its depth (0.26 s there), and the answer is the one the +old loop gave: these tests hold it to that loop, spelled here, over the +cases where the two could part -- an entry spelled twice, a directory entry +and a file entry over one path, a path that ends in `/`, a null path or +leg -- and over a seeded fuzz. +""" + +from __future__ import annotations + +import importlib.util +import random + +import pytest + +from conftest import ADOPT + + +@pytest.fixture(scope="module") +def adopter(): + """`adopt-project.py` as a module.""" + module_spec = importlib.util.spec_from_file_location( + "adopt_answered_legs", ADOPT) + module = importlib.util.module_from_spec(module_spec) + module_spec.loader.exec_module(module) + return module + + +def held_against_every_entry(adopter, entries: list, paths: list) -> dict: + """The answer as main's loop gave it: every entry held against every + path by `_covering`.""" + entry_paths = [str(entry.get("path")) for entry in entries] + legs = {str(entry.get("path")): str(entry.get("leg")) + for entry in entries} + out = {} + for path in paths: + covering = adopter._covering(entry_paths, path) + if len(covering) == 1 and legs[covering[0]] in adopter.LEG_VALUES: + out[path] = (legs[covering[0]], covering[0]) + return out + + +#: `(entries as `(path, leg)`, paths, what is answered)`. +CASES = [ + pytest.param([("src/", "code")], ["src/a.py", "src/b/c.py", "docs/x"], + {"src/a.py": ("code", "src/"), + "src/b/c.py": ("code", "src/")}, id="a-directory"), + pytest.param([("src/", "code"), ("src/", "spec")], ["src/a.py"], {}, + id="one-entry-spelled-twice"), + pytest.param([("src/", "code"), ("src/a.py", "spec")], + ["src/a.py", "src/b.py"], {"src/b.py": ("code", "src/")}, + id="a-directory-and-a-file-over-one-path"), + pytest.param([("a/", "code")], ["a/"], {"a/": ("code", "a/")}, + id="a-path-that-ends-in-a-slash"), + pytest.param([("x", None), ("y", "elsewhere"), ("z", "drop")], + ["x", "y", "z"], {"z": ("drop", "z")}, + id="an-unanswered-leg-and-an-odd-one"), + pytest.param([(None, "code")], ["None", "x"], {"None": ("code", "None")}, + id="a-null-path"), + pytest.param([("a//", "spec")], ["a//b", "a/b"], + {"a//b": ("spec", "a//")}, id="a-doubled-slash"), +] + + +@pytest.mark.parametrize("entries, paths, answered", CASES) +def test_each_path_is_answered_as_the_old_loop_answered_it( + adopter, entries, paths, answered): + """The cases where a lookup by name could part from the old loop.""" + plan = [{"path": path, "leg": leg} for path, leg in entries] + got = adopter._answered_legs(plan, paths) + assert got == answered + assert got == held_against_every_entry(adopter, plan, paths) + + +def test_a_seeded_fuzz_answers_as_the_old_loop(adopter): + """Small trees, entries drawn from their own paths and prefixes and from + noise, some spelled twice: every answer, in its order, the old loop's.""" + rng = random.Random(914) + alphabet = ["a", "b", "a/", "b/", "/", "", "a/b", "x.py"] + legs = ["spec", "code", "root", "drop", None, "elsewhere"] + for _ in range(3000): + paths = ["".join(rng.choice(alphabet) + for _ in range(rng.randint(1, 4))) + for _ in range(rng.randint(0, 6))] + plan = [] + for _ in range(rng.randint(0, 6)): + if paths and rng.random() < 0.7: + base = rng.choice(paths) + cuts = [index + 1 for index, char in enumerate(base) + if char == "/"] + path = (base[:rng.choice(cuts)] + if cuts and rng.random() < 0.5 else base) + else: + path = "".join(rng.choice(alphabet) + for _ in range(rng.randint(0, 3))) + plan.append({"path": path, "leg": rng.choice(legs)}) + if plan and rng.random() < 0.2: + plan.append(dict(rng.choice(plan))) + got = adopter._answered_legs(plan, paths) + expected = held_against_every_entry(adopter, plan, paths) + assert list(got.items()) == list(expected.items()), (plan, paths) + + +def test_no_entry_is_held_against_every_path(adopter, monkeypatch): + """The lookup is by name: `_covering`, which holds every entry against + one path, is not asked at all, so a path costs its depth and not the + plan's length.""" + def refused(*_): + raise AssertionError("_answered_legs held every entry against a path") + + monkeypatch.setattr(adopter, "_covering", refused) + plan = [{"path": f"pkg{index}/", "leg": "code"} for index in range(300)] + files = [f"pkg{index}/sub/file{leaf}.py" for index in range(300) + for leaf in range(10)] + answered = adopter._answered_legs(plan, files) + assert len(answered) == len(files) + assert answered["pkg7/sub/file3.py"] == ("code", "pkg7/") diff --git a/tests/test_adopt_manifest_values.py b/tests/test_adopt_manifest_values.py index 6feab907..78a7be9c 100644 --- a/tests/test_adopt_manifest_values.py +++ b/tests/test_adopt_manifest_values.py @@ -415,6 +415,51 @@ def test_the_flags_and_the_plan_keys_name_the_same_values(adopter): assert mapped - {"PROJECT_NAME"} <= set(adopter.MANIFEST_FLAGS) +#: A `submodules:` and an `answered_paths:` row each of whose four keys the +#: project's validator refuses, and a third record that is not a list, so +#: that every code the two records' findings can carry is made (#914). +BAD_RECORDS = ( + "submodules:\n" + " - path: ../x\n leg: root\n resolution: 7\n" + " source_commit: nope\n" + " - 7\n" + "answered_paths: 7\n") + + +def test_the_records_codes_have_no_placeholder_row(tmp_path, monkeypatch): + """#914's L1-K4, said rather than mapped. No placeholder carries a row + of `submodules:` or `answered_paths:` -- `execute` appends both after + the render its gate asks -- so the codes the project's validator makes + of them have no `FINDING_PLACEHOLDER` row, and a manifest that held one + is one `manifest_judged` can name no value of: None, every value left to + the validator. Asked of the validator the template ships, over a + manifest of nothing but the records, with every other code ignored.""" + monkeypatch.setattr(shape_materialize, "_VALIDATORS", {}) + copy = tmp_path / "assembly-root" + shutil.copytree(REPO / "templates" / "assembly-root", copy, + ignore=shutil.ignore_patterns("__pycache__", "*.pyc")) + policy = shape_materialize.NamingPolicy.load( + REPO / shape_materialize.NAMING_POLICY_YAML) + stems = ("manifest-submodule", "manifest-answered") + made = set() + for record in ("submodules", "answered_paths"): + body = BAD_RECORDS if record == "submodules" else BAD_RECORDS.replace( + "submodules:", "answered_paths:", 1).replace( + "answered_paths: 7", "submodules: 7") + (copy / "project.yaml").write_text(body, encoding=UTF8) + codes = {shape_materialize._code_of(note) for note in + shape_materialize._validator_notes({}, policy, copy, ())} + ours = {code for code in codes if code.startswith(stems)} + others = tuple(codes - ours) + assert shape_materialize.manifest_judged( + {}, policy, copy, ignore=others) is None, ours + made |= ours + assert made == {f"{stem}{tail}" for stem, plural in ( + ("manifest-submodule", "s"), ("manifest-answered", "-paths")) + for tail in ("-path", "-leg", "-resolution", "-commit", plural)} + assert not made & set(shape_materialize.FINDING_PLACEHOLDER) + + def test_a_value_plan_filled_in_is_named_by_where_it_came_from(adopter): """#529 and L1-39: the four values `plan` fills in when its flag is not given are named by the default, in the words `_flag_fields` and the diff --git a/tests/test_adopt_verify.py b/tests/test_adopt_verify.py index c59cebdc..dfe05d51 100644 --- a/tests/test_adopt_verify.py +++ b/tests/test_adopt_verify.py @@ -20,10 +20,11 @@ as the gitlink checks do. EVERY TEST BUT THE LAST THREE CALLS `_verify` DIRECTLY on hand-made commits, -their gitlinks written with `git update-index --cacheinfo 160000,...`. No `git -filter-repo` is involved, so a machine that has none still runs all of -those. The last three run `execute`, so they need `git filter-repo` and are -skipped without it, as every `execute` test is: one proves `cmd_execute` hands +their gitlinks written with `git update-index --cacheinfo 160000,...`, or its +`_account_for` on one path (#914). No `git filter-repo` is involved, so a +machine that has none still runs all of those. The last three run `execute`, +so they need `git filter-repo` and are skipped without it, as every `execute` +test is: one proves `cmd_execute` hands `_verify` the PLAN'S mount paths and that the assembly clones and bootstraps from them, the other two that a source submodule, and the source's own `project.yaml`, that a broken plan check let into the root fail the run. @@ -439,6 +440,53 @@ def test_a_dropped_project_yaml_is_never_the_sources_own(verify, beside): assert (VERIFIED in out) == (not beside) +def test_an_identical_drop_is_counted_as_dropped(verify): + """#914's L1-87. The source's `project.yaml` holds the very bytes the + root's manifest does, and the plan dropped it: on main the table said + `root 2 of 4` and `drop 0 of 4`, counting the shape's file as the + source's landing. The drop is asked first now, so the dropped path is + counted `drop`, and nothing else moves.""" + verified, out, err = verify(source_files={MANIFEST: ROOT_FILES[MANIFEST]}, + drop=(MANIFEST,)) + assert verified == 0, err + assert [row(out, leg) for leg in ("spec", "code", "root", "drop")] == [ + "spec 1 of 4 source paths", "code 1 of 4 source paths", + "root 1 of 4 source paths", "drop 1 of 4 source paths"] + assert findings(err) == [] + + +#: `(where the source path's blob is afterwards, whether the plan dropped +#: it, the counts that move, the findings)`: only a dropped path whose one +#: landing is the root's moves, from `root` to `drop` (#914's L1-87). +OID = "a" * 40 +DROP_CASES = [ + pytest.param([f"root:{OID}"], True, {"drop": 1}, [], + id="dropped-and-the-root-holds-its-bytes"), + pytest.param([], True, {"drop": 1}, [], id="dropped-and-nowhere"), + pytest.param([f"code:{OID}"], True, {"code": 1}, [], + id="dropped-and-in-a-leg"), + pytest.param([f"code:{OID}", f"root:{OID}"], True, {}, [ + f"FINDING adopt-duplicated: `docs/a.md` is in code:{OID}, root:{OID}"], + id="dropped-and-in-a-leg-and-the-root"), + pytest.param([f"root:{OID}"], False, {"root": 1}, [], + id="kept-in-the-root"), + pytest.param([f"root:{'b' * 40}"], True, {"drop": 1}, [], + id="dropped-and-the-root-holds-other-bytes"), +] + + +@pytest.mark.parametrize("landed, dropped, moved, found", DROP_CASES) +def test_the_drop_is_asked_before_the_root(adopter, landed, dropped, moved, + found): + """`_account_for` on one source path, `docs/a.md`, under a directory's + drop entry where it is dropped.""" + counts, findings_made = adopter._account_for( + {"docs/a.md": OID}, {"docs/a.md": landed}, + ["docs/"] if dropped else ["other/"]) + assert counts == {"spec": 0, "code": 0, "root": 0, "drop": 0, **moved} + assert findings_made == found + + #: `execute` REQUIRES `git filter-repo`; without it, the three tests below are #: skipped, as every `execute` test in this suite is. needs_filter_repo = pytest.mark.skipif( diff --git a/tests/test_doctor_plan_overrides.py b/tests/test_doctor_plan_overrides.py index d397777c..4cb667c0 100644 --- a/tests/test_doctor_plan_overrides.py +++ b/tests/test_doctor_plan_overrides.py @@ -40,13 +40,13 @@ from conftest import REPO, git from test_doctor_recorded_answers import ( - EXAMPLES, EXAMPLES_ANSWER, GITMODULES, RULES_IN_CODE, RULES_PATH, + EXAMPLES, EXAMPLES_ANSWER, GITMODULES, GOLDEN, RULES_IN_CODE, RULES_PATH, adopt_source, edit_manifest, golden_run, placement, record_answers, the_rules_row, tree_of, ) from test_shape_doctor import ( # noqa: F401 - adoption_module, needs_filter_repo, project, scaffolded_here, stage, - standard, + adoption_module, needs_filter_repo, project, record_submodules, + scaffolded_here, stage, stage_gitlink, standard, ) sys.path.insert(0, str(REPO / "scripts")) @@ -248,17 +248,97 @@ def test_an_adoption_answered_against_a_rule_is_compliant_on_first_run( # --- L1-56: an emptied `.gitmodules` asks nothing --------------------------- -@pytest.mark.parametrize("body, asked", [ - ("", False), ("\n", True), ("# no submodule here\n", True)], - ids=["no-byte", "a-blank-line", "a-comment"]) +#: `(body, asked)`: a leg's `.gitmodules` in bytes, and whether the policy +#: still asks of it (#914's L1-84). Every one that is not asked holds no +#: section: no byte, or only what git's own config reader passes over -- a +#: whole UTF-8 byte-order mark at the start, blank lines (space, tab, CR, +#: LF) and comments. A section, even one with no key, and a byte git's +#: reader refuses (part of a byte-order mark, a vertical tab) stay asked. +GITMODULES_BODIES = [ + pytest.param(b"", False, id="no-byte"), + pytest.param(b"\n", False, id="a-blank-line"), + pytest.param(b"\r\n", False, id="a-crlf-line"), + pytest.param(b"\xef\xbb\xbf", False, id="a-byte-order-mark"), + pytest.param(b" \t \n\n", False, id="spaces-and-tabs"), + pytest.param(b"# no submodule here\n", False, id="a-comment"), + pytest.param(b" ; nor here\r\n", False, id="a-semicolon-comment"), + pytest.param(b"\xef\xbb\xbf# saved by Notepad\r\n\r\n", False, + id="a-mark-and-a-comment"), + pytest.param(b'[submodule "x"]\n', True, id="a-section-with-no-key"), + pytest.param(b"\xef\xbb", True, id="part-of-a-byte-order-mark"), + pytest.param(b"\x0b\n", True, id="a-vertical-tab"), +] +#: What the row says of the code leg's `.gitmodules` when it is not asked. +EMPTY_CLAUSE = ("the policy asks nothing of 1 `.gitmodules` that hold(s) no " + "section and so register(s) no submodule, in a leg that " + "holds none (#655, #914): code/.gitmodules; nothing needs " + "one, and `git rm -f .gitmodules` in its leg removes it") +#: A submodule the code leg holds, which no `.gitmodules` section registers. +GITLINK_PATH = "vendor/lib" +#: What the row says of it beside a `.gitmodules` that holds no section. +UNREGISTERED_CLAUSE = ( + "1 submodule(s) sit beside a `.gitmodules` of their leg that holds no " + "section, so nothing registers them and a recursive clone of the leg " + "leaves each empty (#914): code/vendor/lib; give each a section with " + "its `path` and `url` in that `.gitmodules`, or, once that " + "`.gitmodules` is staged (`git add .gitmodules`), remove its gitlink " + "with `git rm --cached ` in its leg") +#: Where the clause says the gitlink's path as its leg spells it, which is +#: not the row's own `code/vendor/lib`: typed in the leg, that path matches +#: nothing and `git rm --cached` exits 128. +LEG_PATH = "" + + +def stage_bytes(leg: Path, name: str, body: bytes) -> None: + """`stage`, with the file's bytes as given: a byte-order mark and a CR + are what is under test, so no text layer may add or drop one.""" + (leg / name).write_bytes(body) + git("add", "--", name, cwd=leg) + + +def printed_git(clause: str) -> list: + """Each `git ...` command a clause prints between backticks, as its + arguments after `git`, in the order it prints them. The clause's + `` is typed as the gitlink's own path there.""" + return [command.replace(LEG_PATH, GITLINK_PATH).split()[1:] + for command in clause.split("`")[1::2] + if command.startswith("git ")] + + +def typed_in(leg: Path, clause: str) -> None: + """Type in `leg`, in order, each command `clause` prints, as the person + it advises would, and assert that each exits 0: what a row prints is + held to what git does with it (#914), not to the row's own spelling.""" + commands = printed_git(clause) + assert commands, clause + for args in commands: + done = git(*args, cwd=leg, check=False) + assert done.returncode == 0, (args, done.returncode, done.stderr) + + +def git_lists_nothing(leg: Path) -> bool: + """Does git's own config reader read the leg's `.gitmodules` cleanly + and find no key in it?""" + listed = git("config", "--file", GITMODULES, "--list", cwd=leg, + check=False) + return listed.returncode == 0 and listed.stdout == "" + + +@pytest.mark.parametrize("body, asked", GITMODULES_BODIES) def test_an_empty_gitmodules_is_asked_nothing(standard, project, body, asked): - """A `.gitmodules` that holds no byte registers no submodule, so the - policy's question of it -- which leg owns the submodules it registers - -- has nothing to ask: no question, the row `ok`, and said. One holding - anything at all is a file only git's own reader can say registers - nothing, and it stays the question it was.""" - stage(project / "code", GITMODULES, body) + """A `.gitmodules` that holds no section registers no submodule, so in a + leg that holds none the policy's question of it -- which leg owns the + submodules it registers -- has nothing to ask: no question, the row + `ok`, and said with how to remove it, since no record can answer it + (#914's L1-84; #655 asked nothing only of one that holds no byte). Empty + as git's own reader reads it: each body not asked lists no key and + exits 0 under `git config --file`. One holding a section, or a byte git + refuses, stays the question it was.""" + leg = project / "code" + stage_bytes(leg, GITMODULES, body) + if not asked: + assert git_lists_nothing(leg), body result, verdict, row = placement(standard, project) assert result.returncode == 0, result.stdout + result.stderr assert verdict == COMPLIANT @@ -267,9 +347,87 @@ def test_an_empty_gitmodules_is_asked_nothing(standard, project, body, assert row["status"] == ("note" if asked else "ok"), row assert leg_entry(row, "code").get("empty_gitmodules") == ( None if asked else ["code/.gitmodules"]), leg_entry(row, "code") - clause = ("the policy asks nothing of 1 `.gitmodules` that hold(s) no " - "byte and so register(s) no submodule (#655): code/.gitmodules") - assert (clause in row["reason"]) != asked, row["reason"] + assert (EMPTY_CLAUSE in row["reason"]) != asked, row["reason"] + assert "unregistered_submodules" not in leg_entry(row, "code") + if not asked: + # THE ADVICE TYPED AS PRINTED: the file here is staged and not + # committed, the state a plain `git rm` refuses (rc 1, `has changes + # staged in the index`), and `-f` leaves it in no index. + typed_in(leg, EMPTY_CLAUSE) + assert git("ls-files", "--", GITMODULES, cwd=leg).stdout == "" + + +#: What a leg's `.gitmodules` held before its section was emptied on disk. +SECTIONED = (f'[submodule "{GITLINK_PATH}"]\n\tpath = {GITLINK_PATH}\n' + "\turl = https://example.invalid/lib.git\n").encode("utf-8") + + +@pytest.mark.parametrize("body, recorded, unstaged", [ + (b"", False, False), (b"\n", False, False), (b"# none\n", False, False), + (b"", True, False), (b"", False, True), +], ids=["no-byte", "a-blank-line", "a-comment", "no-byte-and-recorded", + "no-byte-and-an-unstaged-edit"]) +def test_a_sectionless_gitmodules_beside_a_submodule_is_said( + standard, project, body, recorded, unstaged): + """#914's L1-84, its second case. Beside a submodule of its leg, a + `.gitmodules` that holds no section is no emptied file: it registers + none of the leg's submodules, and on main it was set aside as if it + were, with no row naming the submodule nothing registers. Now it stays + the question it was -- asked, or answered where `submodules:` places + every submodule of the leg -- and the row names the submodule with what + to do: a section for it, or its gitlink removed. + + THE REMOVAL TYPED AS PRINTED, in each state. `unstaged` is a tracked + `.gitmodules` whose section was emptied on disk and not staged: it reads + as holding none, so the row names the gitlink beside it, and `git rm + --cached` there dies (rc 128, `please stage your changes to + .gitmodules`) before it looks at the gitlink. The row says to stage the + file first, and the two commands it prints, typed in order and with the + path as the leg spells it, exit 0 and leave the gitlink out of the + index.""" + leg = project / "code" + stage_bytes(leg, GITMODULES, SECTIONED if unstaged else body) + if unstaged: + (leg / GITMODULES).write_bytes(body) + stage_gitlink(leg, GITLINK_PATH) + if recorded: + record_submodules(project, (GITLINK_PATH, "code")) + result, verdict, row = placement(standard, project) + assert result.returncode == 0, result.stdout + result.stderr + assert verdict == COMPLIANT + code = leg_entry(row, "code") + questions = [item["path"] for item in row["detail"]["review_required"]] + assert questions == ([] if recorded else ["code/.gitmodules"]), row + assert code.get("answered") == ( + ["code/.gitmodules"] if recorded else None), code + assert "empty_gitmodules" not in code, code + assert code["unregistered_submodules"] == ["code/vendor/lib"], code + assert UNREGISTERED_CLAUSE in row["reason"], row["reason"] + assert "the policy asks nothing of" not in row["reason"], row["reason"] + if unstaged: + alone = git("rm", "--cached", "--", GITLINK_PATH, cwd=leg, + check=False) + assert (alone.returncode, "stage your changes to .gitmodules" in ( + alone.stderr)) == (128, True), alone.stderr + typed_in(leg, UNREGISTERED_CLAUSE) + assert git("ls-files", "--stage", "--", GITLINK_PATH, + cwd=leg).stdout == "" + + +def test_a_gitmodules_that_registers_its_submodule_names_none(standard, + project): + """The control: a `.gitmodules` with a section for the leg's submodule + is the question it always was, and nothing is named unregistered.""" + leg = project / "code" + stage_bytes(leg, GITMODULES, ( + f'[submodule "{GITLINK_PATH}"]\n\tpath = {GITLINK_PATH}\n' + "\turl = https://example.invalid/lib.git\n").encode("utf-8")) + stage_gitlink(leg, GITLINK_PATH) + row = placement(standard, project)[2] + assert [item["path"] for item in row["detail"]["review_required"]] == [ + "code/.gitmodules"], row + assert "unregistered_submodules" not in leg_entry(row, "code") + assert "nothing registers them" not in row["reason"], row["reason"] @pytest.fixture @@ -339,6 +497,42 @@ def test_a_directory_row_without_its_slash_is_said(standard, project, golden, assert ("without the `/`" in row["reason"]) == bool(named), row["reason"] +# --- #914's L1-85: a file's row with a `/` ---------------------------------- + +@pytest.mark.parametrize("rows, named, status", [ + (((f"{GOLDEN[0]}/", "spec"), (GOLDEN[1], "spec")), + [f"spec/{GOLDEN[0]}/"], "note"), + (((GOLDEN[0], "spec"), (GOLDEN[1], "spec")), [], "ok"), + (((f"{GOLDEN[0]}/", "code"),), [], "note"), + ((("examples/golden/", "spec"),), [], "ok"), +], ids=["a-file-row-with-a-slash", "file-rows", "a-row-for-the-other-leg", + "a-directory-row"]) +def test_a_file_row_with_a_slash_is_said(standard, project, rows, named, + status): + """`_covering` reads a row with its `/` as a directory's, and no file is + under a file, so `examples/golden/case.json/` answers nothing and the + file's question stays (#914's L1-85: on main nothing said so). The + leg's entry and the row now say why, as for a directory's row without + its `/`: said, never read as the file's row, never a finding. A row for + a leg that holds no such file is not named, nor is a directory's row.""" + golden_run(project) + record_answers(project, *rows) + result, verdict, row = placement(standard, project) + assert result.returncode == 0, result.stdout + result.stderr + assert verdict == COMPLIANT + named_here = [path for leg in row["detail"]["legs"] + for path in leg.get("slashed") or []] + assert named_here == named, row["detail"]["legs"] + assert row["status"] == status, row + clause = [f"1 row(s) of project.yaml's `answered_paths:` name a file " + f"with the `/` only a directory's row ends with, so each " + f"answers nothing (#914): {path}" for path in named] + assert [text for text in clause if text in row["reason"]] == clause, \ + row["reason"] + assert ("name a file with the `/`" in row["reason"]) == bool(named), \ + row["reason"] + + # --- L1-59: what a directory's row answers is said where it is read --------- def test_a_directory_row_answers_a_file_added_later_and_says_so(standard, @@ -364,6 +558,20 @@ def test_a_directory_row_answers_a_file_added_later_and_says_so(standard, assert LATER in said(validator) +def test_the_verdict_table_names_the_records_exception(): + """#914's L1-85: README.md's verdict table, and the handbook's copy of + it, say that `answered_paths:` keeps a path off MISPLACED, which they + did not.""" + readme = (REPO / "README.md").read_text(encoding="utf-8") + [row] = [line for line in readme.splitlines() + if line.startswith("| `MISPLACED (n paths)` |")] + assert ("puts elsewhere (unless `answered_paths:` answers it into that " + "leg), or a submodule") in row, row + page = (REPO / "docs" / "handbook.html").read_text(encoding="utf-8") + assert ("puts elsewhere (unless answered_paths: answers it " + "into that leg), or a submodule") in page + + def test_agents_md_says_the_record_holds_answers_against_a_rule(): """The one line AGENTS.md gives the record says what it now holds.""" text = said((REPO / "AGENTS.md").read_text(encoding="utf-8"))