From 3c933e05587a40ce64b4b1aa6bd03624e6051b47 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Tue, 20 Oct 2026 17:48:39 +0000 Subject: [PATCH 1/2] adopt-project.py and shape-doctor.py: a `.gitmodules` that holds no section asks nothing and says how to remove it, one beside an unregistered submodule is said, a file's `answered_paths:` row with a `/` is said, an identical drop counts as `drop`, `_answered_legs` is linear, and the records' validator codes have no placeholder row by statement (#914) #457's L1-84, L1-85, L1-87 and L1-K4, filed as #914 at 5017830: - L1-84 (shape-doctor.py): `empty_gitmodules` asks nothing of a `.gitmodules` that holds no section (no byte, or only a whole UTF-8 byte-order mark, blank lines and `#`/`;` comments: what git's config reader passes over), in a leg that holds no submodule, and the row says `git rm .gitmodules` in its leg removes it. Beside a submodule of its leg it stays the question it was, and the leg's entry names each submodule under `unregistered_submodules`, with what to do. - L1-85: README.md's verdict table, and the handbook's copy of it, name `answered_paths:` in the MISPLACED row (no line added); the handbook's two markers for that section move to its new blob. `_answered_legs` looks up each path's own names (3.65 s -> 0.26 s for 120,000 files and 300 entries, the same answer). The doctor names an `answered_paths:` row that spells a file with a `/` under `slashed`, said, never a finding. - L1-87: `_account_for` asks whether a path was dropped before it counts the root's same blob as the path's landing. - L1-K4: `FINDING_PLACEHOLDER`'s comment says why the two records' code families have no row, and a test holds it to the codes the shipped validator makes. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- adopt-project.py | 34 ++++- docs/handbook.html | 6 +- scripts/shape_materialize.py | 11 ++ shape-doctor.py | 212 ++++++++++++++++++++++------ tests/test_adopt_answered_legs.py | 122 ++++++++++++++++ tests/test_adopt_manifest_values.py | 45 ++++++ tests/test_adopt_verify.py | 56 +++++++- tests/test_doctor_plan_overrides.py | 181 ++++++++++++++++++++++-- 9 files changed, 599 insertions(+), 70 deletions(-) create mode 100644 tests/test_adopt_answered_legs.py 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..9111a8eb 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,57 @@ 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.""" 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 .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. + """ + 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 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 +8571,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 +8586,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 +8681,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 +8752,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 +8821,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 +8838,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..80ac7445 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,72 @@ 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 .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 remove its gitlink " + "with `git rm --cached ` in its leg") + + +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 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 +322,55 @@ 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") + + +@pytest.mark.parametrize("body, recorded", [ + (b"", False), (b"\n", False), (b"# none\n", False), (b"", True), +], ids=["no-byte", "a-blank-line", "a-comment", "no-byte-and-recorded"]) +def test_a_sectionless_gitmodules_beside_a_submodule_is_said( + standard, project, body, recorded): + """#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.""" + leg = project / "code" + stage_bytes(leg, GITMODULES, 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"] + + +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 +440,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 +501,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")) From b58a4e2ccae04f6cd8f0f27c32f8eaec4b9fb822 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Tue, 20 Oct 2026 21:44:32 +0000 Subject: [PATCH 2/2] shape-doctor.py: the row's advice to remove an empty `.gitmodules` is `git rm -f`, which exits 0 where a plain `git rm` refuses the file staged or changed, and its removal of a gitlink beside a sectionless one says to stage that `.gitmodules` first and names the path as the leg spells it, so each command typed as printed works (#914) RV1073's review of #1073 found the doctor printing advice that fails when typed. - `git rm .gitmodules` exits 1 where the leg's `.gitmodules` is staged and not committed (what a plain `git rm` of the leg's only submodule leaves) or changed on disk and not staged. `git rm -f .gitmodules` exits 0 in all four states measured and leaves the file in neither the index nor the disk. It discards no section, since the clause is printed only where the disk copy holds none. - `git rm --cached ` exits 128, `please stage your changes to .gitmodules`, where the leg's tracked `.gitmodules` has an edit that is not staged. The clause now says `once that .gitmodules is staged (git add .gitmodules)`, and `` for the path, since the row's own `code/vendor/lib` is the project's spelling and `git rm --cached` of it in the leg exits 128. tests/test_doctor_plan_overrides.py types each printed command in the leg and asserts exit 0, and an empty `git ls-files .gitmodules` or no gitlink left in the index; a new case holds the unstaged edit. Co-Authored-By: Claude Sonnet 5.5 --- shape-doctor.py | 21 ++++++-- tests/test_doctor_plan_overrides.py | 75 +++++++++++++++++++++++++---- 2 files changed, 83 insertions(+), 13 deletions(-) diff --git a/shape-doctor.py b/shape-doctor.py index 9111a8eb..c3783f28 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -8148,14 +8148,19 @@ def empty_clause(legs: list) -> str: 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.""" + 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 section and so register(s) no submodule, in a leg " f"that holds none (#655, #914): {', '.join(empty)}; nothing " - "needs one, and `git rm .gitmodules` in its leg removes it") + "needs one, and `git rm -f .gitmodules` in its leg removes it") def unregistered_clause(legs: list) -> str: @@ -8170,6 +8175,13 @@ def unregistered_clause(legs: list) -> str: 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 []] @@ -8179,8 +8191,9 @@ def unregistered_clause(legs: list) -> str: "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 remove its gitlink with `git rm " - "--cached ` in its leg") + "`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: diff --git a/tests/test_doctor_plan_overrides.py b/tests/test_doctor_plan_overrides.py index 80ac7445..4cb667c0 100644 --- a/tests/test_doctor_plan_overrides.py +++ b/tests/test_doctor_plan_overrides.py @@ -272,7 +272,7 @@ def test_an_adoption_answered_against_a_rule_is_compliant_on_first_run( 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 .gitmodules` in its leg removes it") + "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. @@ -280,8 +280,13 @@ def test_an_adoption_answered_against_a_rule_is_compliant_on_first_run( "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 remove its gitlink " - "with `git rm --cached ` in its leg") + "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: @@ -291,6 +296,26 @@ def stage_bytes(leg: Path, name: str, body: bytes) -> None: 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?""" @@ -324,22 +349,46 @@ def test_an_empty_gitmodules_is_asked_nothing(standard, project, body, None if asked else ["code/.gitmodules"]), leg_entry(row, "code") 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", [ - (b"", False), (b"\n", False), (b"# none\n", False), (b"", True), -], ids=["no-byte", "a-blank-line", "a-comment", "no-byte-and-recorded"]) + +@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): + 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.""" + 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, body) + 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")) @@ -355,6 +404,14 @@ def test_a_sectionless_gitmodules_beside_a_submodule_is_said( 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,