From 90aba6a48712148081b13846662114252787f415 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Thu, 15 Oct 2026 18:12:08 +0000 Subject: [PATCH 1/2] update-shape.py: `apply` refuses to keep an `--accept-local` copy that imports a name the module it writes beside it no longer defines, before anything is written, so a kept `scripts/bootstrap.py` no longer passes the validators and then dies on an ImportError (#604) #562 (#536) asked one direction only: a copy this run WRITES that imports from a module `--accept-local` KEEPS. The reverse went unasked. A project whose committed edit had `scripts/bootstrap.py` import a name from `scripts/repo_shape.py` (row `locally-modified`) ran `apply --at --accept-local scripts/bootstrap.py`, got exit 0, `pins ok` and `manifest ok`, and its next `bootstrap.py --help` died with `ImportError: cannot import name ...`. The validators caught it only where the kept copy is itself a validator, and then only by rolling back after the writes. Upstream does drop public names: cac6bb20 took `link_manifest_path` and `declared_neutral_pins` out of `repo_shape.py`. The mechanism. `_refuse_kept_copies_missing_names`, at its one call site in `cmd_apply` before `confirm`, now asks both directions and raises one refusal: - A written importer and a kept module (#536, `_kept_copies_lacking`), unchanged in what it asks. - A kept importer and a written module (#604, `_kept_copies_importing_dropped`): each kept `.py` copy (a `both` or `locally-modified` row `--accept-local` named) is read from its bytes on disk with `_names_imported`; each module it imports is resolved with `_module_beside`; where that path is a copy this run writes (an `upstream-changed` row's target bytes, or an `--add` file's), a name is refused only when the module's bytes on disk define it now and the bytes the run writes do not (`_names_dropped`). `_names_defined(path)` keeps its behaviour (an `OSError` is "cannot tell") and now reads through `_names_defined_in(data)`, split out so the written module is read from its target bytes under the same rules. The rulings. Only what THIS RUN takes away is refused: a name the bytes on disk lack too already stops the kept copy, which is the project's doing, and a module with no file yet (an `--add` file) defines nothing to take away. The code stays `update-accept-local-missing-name`, and one refusal lists every pair, sorted by module and then importer. #536's lines are unchanged; a reverse line reads ` scripts/repo_shape.py (written) no longer defines shell_line, which scripts/bootstrap.py (kept) imports from it`. The opening sentence, the validators clause and the remediation are each true of every line in either direction: the clause is said only when no importer named, written or kept, is one of `kind.validators`. The remediation names the human's two exits for each side: change the kept copy (merge the upstream's version where it lacks a name; stop taking the name where the written module dropped it), commit, and run again with the same `--accept-local`; or drop the edit, which for a `both` row is `update-conflict`'s merge again, settled on the upstream's side. It names `git show :` of every kept copy listed, names `--accept-local` in prose only, and prints no NEXT line. Exit 2; nothing is written. No `--help` text changes, and no README.md or AGENTS.md statement became false. What it cannot tell, it does not refuse, in either direction: a kept importer whose bytes do not parse; a star import or a plain `import M`; a module, in the bytes on disk or the bytes the run writes, that does not parse, or that has a star import or a module `__getattr__`. There the validators answer after the writes, as before. Tests, appended to `tests/test_update_shape_kept_helper.py` on #562's harness (76 tests before, 105 now). A second project and holder are cut at `kept-base`, a branch off A whose `repo_shape.py` gains two names no shipped copy imports; `dropped` takes one away, and `both-ways` also has the holder's `siblings.py` import a name its `bootstrap.py` gains. - #604's case end to end: exit 2 under the code, the line naming `scripts/repo_shape.py` (written), `scripts/bootstrap.py` (kept) and the name, the tree, refs and HEAD byte-identical, no `copied` line, no NEXT line; the premise is measured (the kept `bootstrap.py` beside the target's `repo_shape.py` stops on that ImportError at `--help`). - With `--branch`: refused before any branch exists. - A kept `validate-pins.py` importing the dropped name: refused, without the validators clause. - Controls: a name the module lacked before too, and a name the written module still defines, each apply with exit 0, and the kept copy starts after the run exactly where it started before. - A family holder keeping a `bootstrap.py` that imports the dropped name: refused. - Both directions in one run (the holder at `both-ways`): one refusal, both kinds of line, sorted; also in process, with and without a kept validator. - In process, over pairs of bytes: the reverse line and its remediation; each case ruling 3 lists, the two "not taken away" cases and an `--add` module with no file on disk, none of which refuses; and the scope (another directory, a non-`.py` kept copy, a module the run leaves alone, a module no written copy is). Mutants, each killed: the reverse check removed (8 failed; the issue's case applies with `pins ok`, `manifest ok`, `check` then says `unchanged 11`, and `scripts/bootstrap.py --help` exits 1 on the ImportError); the "defined on disk now" condition dropped (7 failed, the lacked-before control among them); the written module read from disk instead of its target bytes (8 failed, #604's end-to-end case among them). Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Opus 5.5 --- tests/test_update_shape_kept_helper.py | 544 ++++++++++++++++++++++--- update-shape.py | 328 +++++++++++---- 2 files changed, 733 insertions(+), 139 deletions(-) diff --git a/tests/test_update_shape_kept_helper.py b/tests/test_update_shape_kept_helper.py index 99b695b0..5cd00195 100644 --- a/tests/test_update_shape_kept_helper.py +++ b/tests/test_update_shape_kept_helper.py @@ -1,15 +1,20 @@ # SPDX-License-Identifier: Apache-2.0 -"""`apply --accept-local` keeping a helper that a copy it writes imports a -name from, which the kept bytes do not define (#536). +"""`apply --accept-local` keeping a copy that disagrees with a copy it +writes beside it on a name one imports from the other, in both directions: +a kept helper that lacks a name a written copy imports (#536), and a kept +copy that imports a name the written helper no longer defines (#604). THE BREAK NOTHING REPORTED. Since #456 both roots' `bootstrap.py` import `spelled` and `escape_unencodable_stdout` from `scripts/repo_shape.py`. A project that kept its own edit of `repo_shape.py` with `--accept-local` while `apply` copied the new `bootstrap.py` passed its validators, read `unchanged` everywhere in the next `check`, and met an `ImportError` the -first time anybody ran `make bootstrap`. `apply` now refuses that run as -`update-accept-local-missing-name`, before it asks for a yes or writes a -byte. +first time anybody ran `make bootstrap`. THE REVERSE BROKE THE SAME WAY: a +project that kept its own `bootstrap.py`, edited to import a name from +`repo_shape.py`, while `apply` copied a `repo_shape.py` that had dropped +it, got `pins ok`, `manifest ok`, and the same `ImportError`. `apply` now +refuses either run as `update-accept-local-missing-name`, before it asks +for a yes or writes a byte, and one refusal lists both kinds of line. THE STANDARD'S NEXT COMMITS ARE REAL COMMITS IN A CLONE OF THIS REPOSITORY. `Atlas` and the family holder `InkRouter` are cut at A, the clone's HEAD, @@ -17,7 +22,7 @@ standard is: * `new-name`: `repo_shape.py` gains `NEW_NAME`, and the assembly root's - `bootstrap.py` imports it: the issue's case. + `bootstrap.py` imports it: #536's case. * `no-new-name`: both files change and no name is added: the control. * `added`: `repo_shape.py` gains `NEW_NAME`, and a file the copy lists now name imports it, so it reaches a project only by `--add`. @@ -27,11 +32,23 @@ `validate-manifest.py` imports it: a copy that is one of the root's validators, which would go red after the writes and be rolled back. +The reverse direction (#604) needs a project cut where `repo_shape.py` +still defines the name the standard then drops, so a second `Atlas` and +`InkRouter` are cut at `kept-base`, a branch off A where `repo_shape.py` +gains `DROPPED_NAME` and `STILL_NAME`, which no shipped copy imports. Off +`kept-base`: + + * `dropped`: `repo_shape.py` loses `DROPPED_NAME` again: #604's case. + * `both-ways`: the same, and the holder's `siblings.py` imports a name + its `bootstrap.py` gains, so a holder that keeps its own `bootstrap.py` + meets both directions in one run. + The project's own edit is committed, as a project's is, so `check` reads -`repo_shape.py` as `both` and `--branch` meets this refusal rather than -`update-branch-uncommitted`. The readers are also held, in process, to the -real importers on this checkout, and to what they cannot tell. Every file is -written as BYTES, so no platform's newline reaches a digest. +the edited copy as `both` or `locally-modified`, and `--branch` meets this +refusal rather than `update-branch-uncommitted`. The readers are also held, +in process, to the real importers on this checkout, and to what they cannot +tell. Every file is written as BYTES, so no platform's newline reaches a +digest. """ from __future__ import annotations @@ -45,7 +62,7 @@ import pytest -from conftest import REPO, git, run_script +from conftest import ORG, REPO, cloned_just_pushed, git, run_script #: The scaffold every update test starts from, and the in-process load of #: `update-shape.py`, imported from the module that owns them; the fixture is #: found by pytest here because it is imported. @@ -91,6 +108,18 @@ NO_NAME_LINE = b"\n# An upstream change that adds no name.\n" LOCAL_EDIT = b"\n# This project edited its own copy.\n" +#: The names `kept-base` adds to `repo_shape.py`, which no shipped copy +#: imports: `dropped` takes the first away again and keeps the second. +DROPPED_NAME = "kept_importer_dropped_upstream" +STILL_NAME = "kept_importer_still_defined" +#: A name `repo_shape.py` defines at no commit here. +NEVER_NAME = "kept_importer_never_defined" +#: The name `both-ways` adds to the holder's `bootstrap.py`, and has its +#: `siblings.py` import. +BOOT_NAME = "kept_bootstrap_added_upstream" +FAMILY_SCRIPTS = "templates/family-root/scripts" +VALIDATE_PINS_COPY = "scripts/validate-pins.py" + #: Every importer of `repo_shape` the standard ships into a root. IMPORTERS = ( "templates/assembly-root/scripts/bootstrap.py", @@ -118,6 +147,38 @@ def adding_no_name(data: bytes) -> bytes: return data + NO_NAME_LINE +def definition(name: str) -> bytes: + """A function named `name`, as a commit of the standard's adds one.""" + return (f"\n\ndef {name}() -> str:\n" + f" \"\"\"A name a commit of the standard's adds.\"\"\"\n" + f" return \"{name}\"\n").encode("utf-8") + + +def defining(*names: str): + """An edit that appends a definition of each of `names`.""" + return lambda data: data + b"".join(definition(name) for name in names) + + +def dropping(name: str): + """An edit that takes `definition(name)` out again.""" + def edit(data: bytes) -> bytes: + assert definition(name) in data, f"{name} is not defined there" + return data.replace(definition(name), b"", 1) + return edit + + +def importing(name: str, module: str = "repo_shape"): + """An edit that has a copy import `name` from `module`, on a line of + its own before its own `from import (`.""" + anchor = f"from {module} import (".encode("utf-8") + line = f"from {module} import {name} # noqa: E402\n".encode("utf-8") + + def edit(data: bytes) -> bytes: + assert anchor in data, f"the copy no longer opens its {module} list" + return data.replace(anchor, line + anchor, 1) + return edit + + def branch_off(upstream: Path, a: str, name: str, edits: dict) -> str: """`edits` (`{path: bytes -> bytes}`) as one commit on a branch off A.""" git("checkout", "-q", "-b", name, a, cwd=upstream) @@ -134,12 +195,8 @@ def naming_the_added_check(data: bytes) -> bytes: ADDED_CHECK).encode("utf-8") -@pytest.fixture(scope="module") -def standard(tmp_path_factory) -> dict: - """A clone at A, `Atlas` and `InkRouter` cut from it, and five branches - off A, one per case.""" - base, upstream, a, clone = scaffold_upstream_and_project( - tmp_path_factory, "update-shape-kept-helper") +def cut_family(upstream: Path, base: Path) -> Path: + """The holder `InkRouter`, cut under `base` from `upstream`'s HEAD.""" created = run_script( upstream / "scripts" / "family.py", "init", "--org", FAMILY, "--family", FAMILY, "--created-by", "Test Human", @@ -147,6 +204,30 @@ def standard(tmp_path_factory) -> dict: "--local-remote-dir", str(base / "remotes"), "--work-dir", str(base / "fam")) assert created.returncode == 0, created.stderr + created.stdout + return base / "fam" / FAMILY + + +def cut_project(upstream: Path, base: Path) -> Path: + """`Atlas`, scaffolded under `base` from `upstream`'s HEAD, as a clone + of what the scaffold pushed: `scaffold_upstream_and_project`'s cut, at + a commit of this module's choosing.""" + made = run_script( + upstream / "scaffold-project.py", "--org", ORG, "--project", PROJECT, + "--elected-by", "Test Human", "--elected-on", "2026-09-02", + "--local-remote-dir", str(base / "remotes"), + "--work-dir", str(base / "work")) + assert made.returncode == 0, made.stderr + made.stdout + return cloned_just_pushed(base / "remotes" / f"{PROJECT}.git", + base / "clone" / PROJECT) + + +@pytest.fixture(scope="module") +def standard(tmp_path_factory) -> dict: + """A clone at A, `Atlas` and `InkRouter` cut from it, and five branches + off A, one per case.""" + base, upstream, a, clone = scaffold_upstream_and_project( + tmp_path_factory, "update-shape-kept-helper") + holder = cut_family(upstream, base) assembly = "templates/assembly-root" commits = {"a": a} @@ -166,19 +247,43 @@ def standard(tmp_path_factory) -> dict: commits["val-name"] = branch_off(upstream, a, "val-name", { REPO_SHAPE: defining_the_new_name, f"{assembly}/{VALIDATE_MANIFEST}": importing_the_new_name}) - return {"upstream": upstream, "clone": clone, - "holder": base / "fam" / FAMILY, **commits} + return {"upstream": upstream, "clone": clone, "holder": holder, + **commits} -def kept_edit(source: Path, target: Path) -> Path: - """A private copy of `source`, with its own edit to `repo_shape.py` - COMMITTED: `check` reads that copy as `both` against every branch.""" +@pytest.fixture(scope="module") +def dropping_standard(standard, tmp_path_factory) -> dict: + """The reverse direction (#604), in the same clone: `kept-base`, a + branch off A whose `repo_shape.py` gains two names no shipped copy + imports; `Atlas` and `InkRouter` cut from it; and `dropped` and + `both-ways`, the two branches off it the module docstring names.""" + upstream = standard["upstream"] + base = tmp_path_factory.mktemp("update-shape-kept-importer") + kept_base = branch_off(upstream, standard["a"], "kept-base", { + REPO_SHAPE: defining(DROPPED_NAME, STILL_NAME)}) + clone = cut_project(upstream, base) + holder = cut_family(upstream, base) + commits = {"kept-base": kept_base} + commits["dropped"] = branch_off(upstream, kept_base, "dropped", { + REPO_SHAPE: dropping(DROPPED_NAME)}) + commits["both-ways"] = branch_off(upstream, kept_base, "both-ways", { + REPO_SHAPE: dropping(DROPPED_NAME), + f"{FAMILY_SCRIPTS}/bootstrap.py": defining(BOOT_NAME), + f"{FAMILY_SCRIPTS}/siblings.py": importing(BOOT_NAME, "bootstrap")}) + return {"upstream": upstream, "clone": clone, "holder": holder, + **commits} + + +def kept_edit(source: Path, target: Path, rel: str = REPO_SHAPE, + edit=lambda data: data + LOCAL_EDIT) -> Path: + """A private copy of `source`, with its own `edit` to `rel` COMMITTED: + by default to `repo_shape.py`, which `check` then reads as `both` + against every branch off A.""" shutil.copytree(source, target, symlinks=True) - path = target / REPO_SHAPE - path.write_bytes(path.read_bytes() + LOCAL_EDIT) + path = target / rel + path.write_bytes(edit(path.read_bytes())) git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "-m", - "The project edits its own repo_shape.py", "--", REPO_SHAPE, - cwd=target) + f"The project edits its own {path.name}", "--", rel, cwd=target) return target @@ -225,23 +330,54 @@ def lacking_line(written: str) -> str: "imports from it") +def dropped_line(kept: str, name: str = DROPPED_NAME) -> str: + """#604's line: the written `repo_shape.py` drops `name`, and the kept + copy `kept` imports it.""" + return (f" {REPO_SHAPE} (written) no longer defines {name}, which " + f"{kept} (kept) imports from it") + + +def detail_lines(stderr: str) -> list[str]: + """The refusal's lines, one per pair, between `REFUSED` and + `Remediation:`.""" + detail = stderr.split(f"REFUSED {REFUSAL}:", 1)[1] + return [line for line in detail.split("Remediation:", 1)[0].splitlines() + if line.startswith(" ")] + + def assert_refused_writing_nothing(result, root: Path, before: dict, - written: str) -> None: - """Exit 2 under this refusal, naming the kept copy, the copy that - imports from it and the name; the tree, the refs and HEAD as they were; + line: str) -> None: + """Exit 2 under this refusal, with `line`, which names the kept copy, + the written one and the name; the tree, the refs and HEAD as they were; no `copied` line; and no NEXT line, which would be the tool passing `--accept-local` for the human.""" assert result.returncode == 2, result.stdout + result.stderr assert f"REFUSED {REFUSAL}:" in result.stderr, result.stderr - assert lacking_line(written) in result.stderr, result.stderr + assert line in detail_lines(result.stderr), result.stderr assert "--accept-local" in result.stderr.split("Remediation:", 1)[1] assert "NEXT" not in result.stdout + result.stderr - assert not [line for line in result.stdout.splitlines() - if line.split()[:1] == ["copied"]], result.stdout + assert not [said for said in result.stdout.splitlines() + if said.split()[:1] == ["copied"]], result.stdout assert snapshot(root) == before, "a refused apply writes nothing at all" assert git("status", "--porcelain", cwd=root).stdout == "" +def assert_stops_on_the_import(beside: Path, copies: dict, name: str) -> None: + """THE PREMISE, MEASURED: `bootstrap.py --help`, in a `scripts/` that + holds `copies` (`{file name: bytes}`) and nothing else, stops on an + `ImportError` that names `name`.""" + beside.mkdir(parents=True) + for file, data in copies.items(): + (beside / file).write_bytes(data) + started = run_script(beside / "bootstrap.py", "--help", + env={"PYTHONDONTWRITEBYTECODE": "1"}) + assert started.returncode != 0, started.stdout + assert "ImportError" in started.stderr, ( + "bootstrap.py did not stop on an ImportError: " + started.stderr) + assert name in started.stderr, ( + f"the ImportError does not name {name}: " + started.stderr) + + # --- end to end -------------------------------------------------------------- def test_keeping_a_helper_that_lacks_a_name_bootstrap_imports_is_refused( @@ -264,7 +400,8 @@ def test_keeping_a_helper_that_lacks_a_name_bootstrap_imports_is_refused( assert verdicts(checked.stdout)[BOOTSTRAP] == "upstream-changed" result = apply(root, standard, "new-name", "--accept-local", REPO_SHAPE) - assert_refused_writing_nothing(result, root, before, BOOTSTRAP) + assert_refused_writing_nothing(result, root, before, + lacking_line(BOOTSTRAP)) assert (f"`git show {standard['new-name']}:{REPO_SHAPE}` for " f"{REPO_SHAPE}") in result.stderr assert "`update-conflict`'s merge by hand again" in result.stderr @@ -272,18 +409,11 @@ def test_keeping_a_helper_that_lacks_a_name_bootstrap_imports_is_refused( "bootstrap.py is none of the validators, and the refusal says so: " + result.stderr) - beside = root.parent / "beside" / "scripts" - beside.mkdir(parents=True) - (beside / "repo_shape.py").write_bytes((root / REPO_SHAPE).read_bytes()) - (beside / "bootstrap.py").write_bytes( - blob(standard, "new-name", f"templates/assembly-root/{BOOTSTRAP}")) - started = run_script(beside / "bootstrap.py", "--help", - env={"PYTHONDONTWRITEBYTECODE": "1"}) - assert started.returncode != 0, started.stdout - assert "ImportError" in started.stderr, ( - "bootstrap.py did not stop on an ImportError: " + started.stderr) - assert NEW_NAME in started.stderr, ( - f"the ImportError does not name {NEW_NAME}: " + started.stderr) + assert_stops_on_the_import(root.parent / "beside" / "scripts", { + "repo_shape.py": (root / REPO_SHAPE).read_bytes(), + "bootstrap.py": blob(standard, "new-name", + f"templates/assembly-root/{BOOTSTRAP}")}, + NEW_NAME) def test_keeping_a_helper_a_validator_imports_is_refused_without_the_clause( @@ -295,7 +425,8 @@ def test_keeping_a_helper_a_validator_imports_is_refused_without_the_clause( written, and does not say the validators pass.""" before = snapshot(root) result = apply(root, standard, "val-name", "--accept-local", REPO_SHAPE) - assert_refused_writing_nothing(result, root, before, VALIDATE_MANIFEST) + assert_refused_writing_nothing(result, root, before, + lacking_line(VALIDATE_MANIFEST)) assert VALIDATORS_PASS not in result.stderr, ( "validate-manifest.py is one of the validators, which would not " "pass: " + result.stderr) @@ -307,7 +438,8 @@ def test_with_branch_it_is_refused_before_any_branch_exists(root, standard): before = snapshot(root) result = apply(root, standard, "new-name", "--accept-local", REPO_SHAPE, "--branch", BRANCH) - assert_refused_writing_nothing(result, root, before, BOOTSTRAP) + assert_refused_writing_nothing(result, root, before, + lacking_line(BOOTSTRAP)) assert git("branch", "--list", BRANCH, cwd=root).stdout == "" @@ -352,7 +484,8 @@ def test_an_add_file_importing_a_name_the_kept_helper_lacks_is_refused( before = snapshot(root) result = apply(root, standard, "added", "--accept-local", REPO_SHAPE, "--add", ADDED_CHECK) - assert_refused_writing_nothing(result, root, before, ADDED_CHECK) + assert_refused_writing_nothing(result, root, before, + lacking_line(ADDED_CHECK)) assert not (root / ADDED_CHECK).exists() @@ -363,7 +496,8 @@ def test_a_family_holder_keeping_its_helper_is_refused_for_siblings( bootstrap` does in a project.""" before = snapshot(holder) result = apply(holder, standard, "family", "--accept-local", REPO_SHAPE) - assert_refused_writing_nothing(result, holder, before, SIBLINGS) + assert_refused_writing_nothing(result, holder, before, + lacking_line(SIBLINGS)) # --- the readers, over the real importers on this checkout ------------------- @@ -493,15 +627,17 @@ def imports(*names: str, module: str = "repo_shape") -> bytes: def refusal(update_shape, root: Path, kept: dict, written: dict, added: dict | None = None, state: str = "both", accepted: set | None = None, writing: str = "upstream-changed", - kind=None): + kind=None, current: dict | None = None): """`_refuse_kept_copies_missing_names` over rows made by hand: `kept` (`{path: bytes on disk}`) in `state`, `written` (`{path: target bytes}`) - in `writing`, and `added` as `--add` files, in a root of `kind` (a + in `writing`, with `current` (`{path: bytes on disk}`) for those that + have a file now, and `added` as `--add` files, in a root of `kind` (a project's unless one is given). The refusal, or None.""" rows = [] - for path, data in kept.items(): + for path, data in {**(current or {}), **kept}.items(): (root / path).parent.mkdir(parents=True, exist_ok=True) (root / path).write_bytes(data) + for path in kept: row = update_shape.Row(path, "") row.state, row.source = state, path rows.append(row) @@ -884,3 +1020,307 @@ def test_every_kept_copy_and_every_importer_is_named(update_shape, tmp_path): {BOOTSTRAP: imports("NEW")}) assert (f"{REPO_SHAPE} changed upstream too, so dropping the edit there " "is `update-conflict`'s merge by hand again") in both.remediation + + +# --- the reverse direction, end to end (#604) -------------------------------- + +def importing_root(dropping_standard: dict, tmp_path: Path, + rel: str = BOOTSTRAP, name: str = DROPPED_NAME) -> Path: + """A private copy of the project cut at `kept-base`, whose own edit, + committed, has `rel` import `name` from `repo_shape`.""" + return kept_edit(dropping_standard["clone"], tmp_path / PROJECT, rel, + importing(name)) + + +@pytest.fixture +def importing_holder(dropping_standard, tmp_path) -> Path: + """The holder cut at `kept-base`, whose own `bootstrap.py` imports + `DROPPED_NAME`, committed.""" + return kept_edit(dropping_standard["holder"], tmp_path / FAMILY, + BOOTSTRAP, importing(DROPPED_NAME)) + + +def test_keeping_a_copy_that_imports_a_name_the_run_drops_is_refused( + dropping_standard, tmp_path): + """#604's case: the project's committed edit has `bootstrap.py` import + a name its pinned `repo_shape.py` defines, so that row is + `locally-modified`, and the target drops the name, so `repo_shape.py` + is `upstream-changed`. `--accept-local scripts/bootstrap.py` would keep + the importer while the run writes a module that no longer defines what + it takes. Refused before the yes, with the tree, the pin and the refs + untouched; the row is not `both`, so `update-conflict` is not named. + + THE PREMISE IS MEASURED, NOT ASSUMED: the kept `bootstrap.py` beside + the target's `repo_shape.py` dies on that import at `--help`.""" + standard = dropping_standard + root = importing_root(standard, tmp_path) + before = snapshot(root) + checked = run_script(UPDATE, "check", "--root", str(root), + "--upstream", str(standard["upstream"]), + "--at", standard["dropped"]) + assert verdicts(checked.stdout)[BOOTSTRAP] == "locally-modified", \ + checked.stdout + assert verdicts(checked.stdout)[REPO_SHAPE] == "upstream-changed" + + result = apply(root, standard, "dropped", "--accept-local", BOOTSTRAP) + assert_refused_writing_nothing(result, root, before, + dropped_line(BOOTSTRAP)) + remedy = result.stderr.split("Remediation:", 1)[1] + assert (f"`git show {standard['dropped']}:templates/assembly-root/" + f"{BOOTSTRAP}` for {BOOTSTRAP}") in remedy, remedy + assert "so it no longer takes that name" in remedy, remedy + assert "update-conflict" not in remedy, remedy + assert VALIDATORS_PASS in result.stderr, ( + "bootstrap.py is none of the validators, and the refusal says so: " + + result.stderr) + + assert_stops_on_the_import(tmp_path / "beside" / "scripts", { + "repo_shape.py": blob(standard, "dropped", REPO_SHAPE), + "bootstrap.py": (root / BOOTSTRAP).read_bytes()}, DROPPED_NAME) + + +def test_with_branch_a_dropped_name_is_refused_before_any_branch_exists( + dropping_standard, tmp_path): + """`--branch` meets the same refusal, before `git checkout -b`.""" + root = importing_root(dropping_standard, tmp_path) + before = snapshot(root) + result = apply(root, dropping_standard, "dropped", "--accept-local", + BOOTSTRAP, "--branch", BRANCH) + assert_refused_writing_nothing(result, root, before, + dropped_line(BOOTSTRAP)) + assert git("branch", "--list", BRANCH, cwd=root).stdout == "" + + +def test_a_kept_validator_importing_a_dropped_name_omits_the_clause( + dropping_standard, tmp_path): + """The one case the validators did catch: a kept `validate-pins.py` + that imports the name runs after the writes, goes red, and the run is + rolled back. It is refused first now, before the yes and before a byte + is written, and the refusal does not say the validators pass, which + would be false of it.""" + root = importing_root(dropping_standard, tmp_path, VALIDATE_PINS_COPY) + before = snapshot(root) + result = apply(root, dropping_standard, "dropped", "--accept-local", + VALIDATE_PINS_COPY) + assert_refused_writing_nothing(result, root, before, + dropped_line(VALIDATE_PINS_COPY)) + assert VALIDATORS_PASS not in result.stderr, result.stderr + + +@pytest.mark.parametrize("name, starts", [ + pytest.param(NEVER_NAME, False, id="a name the module lacked before too"), + pytest.param(STILL_NAME, True, + id="a name the written module still defines"), +]) +def test_a_kept_copy_importing_what_this_run_does_not_take_away_applies( + dropping_standard, tmp_path, name, starts): + """Only what THIS RUN takes away is refused. A name `repo_shape.py` + lacked at the pin too stops the kept `bootstrap.py` before this run and + after it, which is the project's doing, not the run's; a name the + written `repo_shape.py` still defines is no disagreement at all. Both + apply: `repo_shape.py` is copied, the kept edit is re-pinned from the + local bytes, and the kept copy starts after the run exactly where it + started before it.""" + root = importing_root(dropping_standard, tmp_path, name=name) + kept = (root / BOOTSTRAP).read_bytes() + + def started() -> bool: + return run_script(root / BOOTSTRAP, "--help", + env={"PYTHONDONTWRITEBYTECODE": "1"}).returncode == 0 + + assert started() is starts, "the kept copy before the run" + result = apply(root, dropping_standard, "dropped", "--accept-local", + BOOTSTRAP) + assert result.returncode == 0, result.stdout + result.stderr + assert f" copied {REPO_SHAPE}" in result.stdout + assert (root / REPO_SHAPE).read_bytes() == blob( + dropping_standard, "dropped", REPO_SHAPE) + assert (root / BOOTSTRAP).read_bytes() == kept + assert pin_rows(root)[BOOTSTRAP] == file_sha256(root / BOOTSTRAP) + assert started() is starts, "the kept copy after the run" + + +def test_a_holder_keeping_a_bootstrap_that_imports_a_dropped_name_is_refused( + dropping_standard, importing_holder): + """The holder's own `bootstrap.py` imports from the same + `repo_shape.py`, and `make bootstrap`, `make siblings`, `park` and + `resume` would all stop on it.""" + before = snapshot(importing_holder) + result = apply(importing_holder, dropping_standard, "dropped", + "--accept-local", BOOTSTRAP) + assert_refused_writing_nothing(result, importing_holder, before, + dropped_line(BOOTSTRAP)) + + +def test_both_directions_in_one_run_are_one_refusal(dropping_standard, + importing_holder): + """`both-ways` drops the name the holder's kept `bootstrap.py` imports, + AND has `siblings.py` import a name only the upstream's `bootstrap.py` + defines: the kept copy is the module of one line and the importer of + the other. One refusal names both, sorted by module, says the + validators pass (none of the importers is `validate-family.py`), and + sends the `both` row to `update-conflict`'s merge.""" + checked = run_script(UPDATE, "check", "--root", str(importing_holder), + "--upstream", str(dropping_standard["upstream"]), + "--at", dropping_standard["both-ways"]) + assert verdicts(checked.stdout)[BOOTSTRAP] == "both", checked.stdout + before = snapshot(importing_holder) + result = apply(importing_holder, dropping_standard, "both-ways", + "--accept-local", BOOTSTRAP) + assert_refused_writing_nothing(result, importing_holder, before, + dropped_line(BOOTSTRAP)) + assert result.stderr.count("REFUSED") == 1, result.stderr + assert detail_lines(result.stderr) == [ + f" {BOOTSTRAP} (kept) lacks {BOOT_NAME}, which {SIBLINGS} " + "imports from it", + dropped_line(BOOTSTRAP), + ], result.stderr + assert VALIDATORS_PASS in result.stderr, result.stderr + assert (f"{BOOTSTRAP} changed upstream too, so dropping the edit there " + "is `update-conflict`'s merge by hand again") in result.stderr + + +# --- the reverse direction, in process (#604) -------------------------------- + +#: A kept copy that imports `GONE` and `KEPT` from `repo_shape`; that +#: module's bytes on disk, which define both; and the bytes this run writes +#: there, which drop `GONE`. +TAKES_GONE = imports("GONE", "KEPT") +DEFINES_GONE = b"GONE = 1\nKEPT = 1\n" +DROPS_GONE = b"KEPT = 1\n" +GETATTR = b"def __getattr__(name):\n return name\n" + + +def reverse(update_shape, root: Path, importer: bytes = TAKES_GONE, + now: bytes = DEFINES_GONE, after: bytes = DROPS_GONE, + state: str = "locally-modified"): + """`refusal` over a kept `bootstrap.py` (`importer`, in `state`) and a + written `repo_shape.py`: `now` on disk, `after` the bytes this run + writes there.""" + return refusal(update_shape, root, {BOOTSTRAP: importer}, + {REPO_SHAPE: after}, state=state, + current={REPO_SHAPE: now}) + + +def test_a_kept_copy_importing_a_name_the_written_module_drops_is_refused( + update_shape, tmp_path): + """The reverse line names only what is taken away: `KEPT`, which the + written bytes still define, is not in it. The remediation names + `--accept-local` in prose, the upstream's own copy of the kept file, + and `update-conflict` only for a `both` row.""" + refused = reverse(update_shape, tmp_path / "modified") + assert refused is not None, "the written repo_shape.py drops GONE" + assert refused.code == REFUSAL, refused.code + assert [line for line in refused.detail.splitlines() + if line.startswith(" ")] == [ + f" {REPO_SHAPE} (written) no longer defines GONE, which " + f"{BOOTSTRAP} (kept) imports from it"], refused.detail + assert f"`git show {'a' * 40}:{BOOTSTRAP}` for {BOOTSTRAP}" \ + in refused.remediation, refused.remediation + assert "--accept-local" in refused.remediation + assert "update-conflict" not in refused.remediation + both = reverse(update_shape, tmp_path / "both", state="both") + assert (f"{BOOTSTRAP} changed upstream too, so dropping the edit there " + "is `update-conflict`'s merge by hand again") in both.remediation + + +@pytest.mark.parametrize("importer, now, after", [ + pytest.param(TAKES_GONE + b"def broken(:\n", DEFINES_GONE, DROPS_GONE, + id="a kept importer that does not parse"), + pytest.param(b"from repo_shape import *\n", DEFINES_GONE, DROPS_GONE, + id="a kept importer's star import"), + pytest.param(b"import repo_shape\nrepo_shape.GONE()\n", DEFINES_GONE, + DROPS_GONE, id="a kept importer's import of the module"), + pytest.param(b"try:\n from repo_shape import GONE\n" + b"except ImportError:\n GONE = None\n", + DEFINES_GONE, DROPS_GONE, id="a kept importer's fallback"), + pytest.param(TAKES_GONE, DEFINES_GONE, b"def broken(:\n", + id="written bytes that do not parse"), + pytest.param(TAKES_GONE, DEFINES_GONE, DROPS_GONE + b"\x00", + id="written bytes with a null byte"), + pytest.param(TAKES_GONE, DEFINES_GONE, + DROPS_GONE + b"from elsewhere import *\n", + id="written bytes with a star import"), + pytest.param(TAKES_GONE, DEFINES_GONE, DROPS_GONE + GETATTR, + id="written bytes with a module __getattr__"), + pytest.param(TAKES_GONE, b"GONE = (\n", DROPS_GONE, + id="bytes on disk that do not parse"), + pytest.param(TAKES_GONE, b"\xff = 1\n", DROPS_GONE, + id="bytes on disk that are not UTF-8"), + pytest.param(TAKES_GONE, b"from elsewhere import *\n", DROPS_GONE, + id="bytes on disk with a star import"), + pytest.param(TAKES_GONE, DEFINES_GONE + GETATTR, DROPS_GONE, + id="bytes on disk with a module __getattr__"), + pytest.param(TAKES_GONE, DROPS_GONE, DROPS_GONE + b"OTHER = 1\n", + id="a name the bytes on disk lack too"), + pytest.param(TAKES_GONE, DEFINES_GONE, DEFINES_GONE + b"OTHER = 1\n", + id="a name the written bytes still define"), +]) +def test_what_the_reverse_cannot_tell_or_the_run_keeps_it_does_not_refuse( + update_shape, tmp_path, importer, now, after): + """Ruling 3 of #604, the forward check's rule: what it cannot tell, it + does not refuse, and the validators answer after the writes. And what + this run does not take away is not its doing.""" + assert reverse(update_shape, tmp_path, importer, now, after) is None + + +def test_a_module_this_run_adds_takes_nothing_away(update_shape, tmp_path): + """An `--add` file has no bytes on disk yet, so it defines nothing a + kept copy could lose: whatever the kept copy imports from it, it stops + on that today, without this run.""" + assert refusal(update_shape, tmp_path, + {BOOTSTRAP: imports("GONE", module="added_helper")}, {}, + added={"scripts/added_helper.py": DROPS_GONE}, + state="locally-modified") is None + + +@pytest.mark.parametrize("kept, writing", [ + pytest.param({"tools/bootstrap.py": TAKES_GONE}, "upstream-changed", + id="a kept importer in another directory"), + pytest.param({"Makefile": TAKES_GONE}, "upstream-changed", + id="a kept copy that is not .py"), + pytest.param({BOOTSTRAP: TAKES_GONE}, "unchanged", + id="a module this run leaves alone"), + pytest.param({BOOTSTRAP: imports("GONE", module="other_helper")}, + "upstream-changed", id="a module no written copy is"), +]) +def test_a_module_this_run_does_not_write_is_not_asked_of_a_kept_importer( + update_shape, tmp_path, kept, writing): + assert refusal(update_shape, tmp_path, kept, {REPO_SHAPE: DROPS_GONE}, + state="locally-modified", writing=writing, + current={REPO_SHAPE: DEFINES_GONE}) is None + + +@pytest.mark.parametrize("importer, said", [ + pytest.param("scripts/check.py", True, id="no importer is a validator"), + pytest.param(VALIDATE_PINS_COPY, False, + id="the kept importer is a validator"), +]) +def test_both_directions_are_one_refusal_with_every_pair_sorted( + update_shape, tmp_path, importer, said): + """A kept `repo_shape.py` lacking a name a written `bootstrap.py` takes + (#536), and a kept copy taking a name a written `paths.py` drops + (#604): one refusal, one line per pair, sorted by module, each saying + which side is kept. The remediation gives each side's change and the + upstream's copy of each kept file, and the validators clause is said + only where no importer, written or kept, is one of them.""" + refused = refusal( + update_shape, tmp_path, + {REPO_SHAPE: b"OLD = 1\n", importer: imports("GONE", module="paths")}, + {BOOTSTRAP: imports("OLD", "NEW"), "scripts/paths.py": DROPS_GONE}, + state="locally-modified", current={"scripts/paths.py": DEFINES_GONE}) + assert refused is not None, "both pairs disagree" + assert [line for line in refused.detail.splitlines() + if line.startswith(" ")] == [ + f" scripts/paths.py (written) no longer defines GONE, which " + f"{importer} (kept) imports from it", + f" {REPO_SHAPE} (kept) lacks NEW, which {BOOTSTRAP} imports from " + "it", + ], refused.detail + for change in ("so it defines every name above", + "so it no longer takes that name"): + assert change in refused.remediation, refused.remediation + for path in (importer, REPO_SHAPE): + assert f"`git show {'a' * 40}:{path}` for {path}" \ + in refused.remediation, refused.remediation + assert (VALIDATORS_PASS in refused.detail) is said, refused.detail diff --git a/update-shape.py b/update-shape.py index 9fe07057..ae3b022e 100755 --- a/update-shape.py +++ b/update-shape.py @@ -3620,9 +3620,9 @@ def _nothing_to_do(rows: list[Row], pinned: str, target: str, def _refuse_kept_copies_missing_names(root: Path, rows: list[Row], taking: list[Addition], accepted: set, target: str, kind: Kind) -> None: - """`update-accept-local-missing-name` (#536): a copy this run writes - imports a name from a module `--accept-local` keeps, and the kept bytes - do not define it. + """`update-accept-local-missing-name` (#536, #604): a copy + `--accept-local` keeps and a copy this run writes beside it disagree on + a name. One imports it from the other, whose bytes do not define it. THE VALIDATORS CANNOT SEE IT, AND NOTHING ELSE RUNS THAT COPY. Since #456 moved `spelled` and `escape_unencodable_stdout` into @@ -3640,96 +3640,104 @@ def _refuse_kept_copies_missing_names(root: Path, rows: list[Row], written, so nothing is rolled back, and nobody is asked for a yes to a run that would leave a copy that cannot start. - UNLESS THE COPY IS ONE OF THE VALIDATORS. `kind.validators` are what + UNLESS AN IMPORTER IS ONE OF THE VALIDATORS. `kind.validators` are what `_refuse_red_validators` runs after the writes, so a validator that - imports a name the kept copy lacks goes red there, and the run is - rolled back. This still refuses it first, before the yes and before a - byte is written, but says the validators pass only when no copy it - names is one of them: a clause that is false of one copy in the list - is false. - - WHAT IS ASKED, AND OF WHAT. The importers are the copies this run - WRITES, read from the bytes it is about to write: each - `upstream-changed` row and each `--add` file whose path ends `.py`. A - row the run leaves alone is not asked about: an edit that already broke - an untouched importer is the project's doing, not this run's. A module - is resolved the way every shipped importer resolves it, its own - directory first on `sys.path`: `from M import ...` in `/.py` - is the copy at `/M.py`, a dotted `M` naming that path's - subdirectories. So the same code covers `repo_shape.py` today, and any - helper kept beside its importer later, a holder's `bootstrap.py` under - its `siblings.py` among them. A module that resolves to no kept copy is - not asked about. `_names_imported` reads what a copy takes, and - `_names_defined` what a kept copy binds. - - WHAT IT CANNOT TELL, IT DOES NOT REFUSE. A star import, or `import M` - and an attribute, names nothing this can read; nor does a copy, kept or - written, whose bytes Python could not parse either; nor a kept copy - with a star import or a module `__getattr__` of its own. There the - validators answer after the writes, as they did before #536, and roll - back a kept copy that cannot import. A name bound by `:=`, a `match` - capture, `type X =` or a `global` declared in a function is read as - defined (`_bound_at` says what it reads as a binding). A name bound only - through `globals()`, `setattr` on the module or `exec` reads as - missing, and is refused; one bound and then deleted (`NAME = 1`, then - `del NAME`) reads as defined, so that copy's `ImportError` is still its - first run's to meet. + imports a name its module lacks, whether this run writes the validator + or keeps it, goes red there, and the run is rolled back. This still + refuses it first, before the yes and before a byte is written, but says + the validators pass only when no importer it names is one of them: a + clause that is false of one line in the list is false. + + WHAT IS ASKED, AND OF WHAT: BOTH DIRECTIONS, between a KEPT copy, a + `both` or `locally-modified` row `--accept-local` named, read from its + bytes on disk, and a copy this run WRITES, read from the bytes it is + about to write: each `upstream-changed` row and each `--add` file. + + * A WRITTEN IMPORTER AND A KEPT MODULE (#536, + `_kept_copies_lacking`): each written `.py` copy's imports, against + what the kept bytes define. + * A KEPT IMPORTER AND A WRITTEN MODULE (#604, + `_kept_copies_importing_dropped`): each kept `.py` copy's imports, + against what the module's written bytes define. Upstream does drop + public names: cac6bb20 took `link_manifest_path` and + `declared_neutral_pins` out of `scripts/repo_shape.py`. A project + whose kept `bootstrap.py` imported a name dropped like that got + `pins ok`, `manifest ok`, and an `ImportError` from its next + `bootstrap.py --help`. Only what THIS RUN takes away is refused: a + name the module's bytes on disk define now and its written bytes do + not. A name the bytes on disk lack too already stops the kept copy + today, and a module with no file yet, an `--add` file, defines + nothing to take away. + + A row the run leaves alone is not asked about, in either direction: an + edit that already broke an untouched importer is the project's doing, + not this run's. A module is resolved the way every shipped importer + resolves it, its own directory first on `sys.path`: `from M import ...` + in `/.py` is the copy at `/M.py`, a dotted `M` naming + that path's subdirectories. So the same code covers `repo_shape.py` + today, and any helper kept or written beside its importer later, a + holder's `bootstrap.py` under its `siblings.py` among them. A module + that resolves to no copy on the other side is not asked about. + `_names_imported` reads what a copy takes, and `_names_defined` (a file) + or `_names_defined_in` (bytes) what a module binds. + + WHAT IT CANNOT TELL, IT DOES NOT REFUSE, in either direction. A star + import, or `import M` and an attribute, names nothing this can read; + nor does an importer, kept or written, whose bytes Python could not + parse either; nor a module, in its kept bytes, its bytes on disk or the + bytes this run writes, that Python could not parse or that has a star + import or a module `__getattr__` of its own. There the validators + answer after the writes, as they did before #536, and roll the run back + where a validator is the copy that cannot import. A name bound by `:=`, + a `match` capture, `type X =` or a `global` declared in a function is + read as defined (`_bound_at` says what it reads as a binding). A name + bound only through `globals()`, `setattr` on the module or `exec` reads + as missing: refused where a kept module binds it that way (#536), or + where the written bytes do and the bytes on disk bound it plainly + (#604). One bound and then deleted (`NAME = 1`, then `del NAME`) reads + as defined, so that copy's `ImportError` is still its first run's to + meet. """ kept = {row.path: row for row in rows if row.state in (BOTH, LOCALLY_MODIFIED) and row.path in accepted} if not kept: return - lacking = _kept_copies_lacking(root, rows, taking, kept) - if not lacking: - return - helpers = sorted({helper for helper, _, _ in lacking}) - upstreams = "; ".join(f"`git show {target}:{kept[helper].source}` for " - f"{helper}" for helper in helpers) - both = [helper for helper in helpers if kept[helper].state == BOTH] - validated = any(path in kind.validators for _, path, _ in lacking) - raise Refusal( - "update-accept-local-missing-name", - "--accept-local keeps this project's own bytes of a module that a " - "copy this run writes imports from, and those bytes lack a name the " - "copy imports, so the copy would stop on an ImportError the first " - "time it runs" - + ("" if validated else - ", though the validators, which never import it, pass") - + ":\n" - + "\n".join(f" {helper} (kept) lacks {', '.join(names)}, which " - f"{path} imports from it" - for helper, path, names in lacking), - "Remediation: whether the project keeps its edit is the human's " - "decision, and a kept copy must still define what the copies " - "written beside it import. Either merge the upstream's version into " - "the kept copy by hand, so it defines every name above, commit it, " - "and run this again with the same --accept-local; or drop the edit, " - "so the upstream's copy is taken: write the upstream's bytes over " - "it, commit that, and run this again without --accept-local on that " - f"path. In a shape clone the upstream's bytes are {upstreams}." - + (f" {', '.join(both)} changed upstream too, so dropping the edit " - "there is `update-conflict`'s merge by hand again, settled on the " - "upstream's side." if both else "") - + " Nothing was written.") + written = _written_copies(rows, taking) + pairs = sorted(_kept_copies_lacking(root, written, kept) + + _kept_copies_importing_dropped(root, written, kept)) + if pairs: + raise _missing_name_refusal(pairs, kept, target, kind) -def _kept_copies_lacking(root: Path, rows: list[Row], taking: list[Addition], - kept: dict) -> list[tuple[str, str, list[str]]]: - """`(kept copy, written copy, [the names it lacks])` for each written - `.py` copy that imports from a kept one, sorted (#536). Split out of - `_refuse_kept_copies_missing_names`, which says what is asked of what; - each kept copy is read once, however many copies import from it. +#: Which side of a pair `update-accept-local-missing-name` names is the +#: MODULE (#604): the kept bytes lack a name a written copy imports (#536), +#: or the written bytes drop one a kept copy imports. +KEPT_MODULE = "kept" +WRITTEN_MODULE = "written" + + +def _written_copies(rows: list[Row], + taking: list[Addition]) -> dict[str, bytes | None]: + """`{path: the bytes this run writes there}`: each `upstream-changed` + row's target bytes, and each `--add` file's (#536, #604). """ - written = sorted( - [(row.path, row.target_bytes) for row in rows - if row.state == UPSTREAM_CHANGED] - + [(add.path, add.target_bytes) for add in taking], - key=lambda pair: pair[0]) - imports = [(path, module, names) for path, data in written + written = {row.path: row.target_bytes for row in rows + if row.state == UPSTREAM_CHANGED} + written.update((add.path, add.target_bytes) for add in taking) + return written + + +def _kept_copies_lacking(root: Path, written: dict, kept: dict) -> list: + """`(kept module, written copy, [the names it lacks], KEPT_MODULE)` for + each written `.py` copy that imports from a kept one (#536). Split out + of `_refuse_kept_copies_missing_names`, which says what is asked of + what; each kept copy is read once, however many copies import from it. + """ + imports = [(path, module, names) for path, data in sorted(written.items()) if path.endswith(".py") and data is not None for module, names in sorted(_names_imported(data).items())] defined: dict[str, set[str] | None] = {} - lacking: list[tuple[str, str, list[str]]] = [] + lacking = [] for path, module, names in imports: helper = _module_beside(path, module) if helper not in kept: @@ -3739,8 +3747,139 @@ def _kept_copies_lacking(root: Path, rows: list[Row], taking: list[Addition], known = defined[helper] missing = sorted(names - known) if known is not None else [] if missing: - lacking.append((helper, path, missing)) - return sorted(lacking) + lacking.append((helper, path, missing, KEPT_MODULE)) + return lacking + + +def _kept_copies_importing_dropped(root: Path, written: dict, + kept: dict) -> list: + """`(written module, kept copy, [the names this run drops], + WRITTEN_MODULE)` for each kept `.py` copy that imports from a module + this run writes (#604): the reverse of `_kept_copies_lacking`, which + `_refuse_kept_copies_missing_names` says the why of. Each written + module is read once, however many kept copies import from it. + """ + dropped: dict[str, set[str]] = {} + pairs = [] + for path in sorted(kept): + for module, names in sorted(_imports_on_disk(root, path).items()): + beside = _module_beside(path, module) + if beside not in written: + continue + if beside not in dropped: + dropped[beside] = _names_dropped(root, beside, + written[beside]) + missing = sorted(names & dropped[beside]) + if missing: + pairs.append((beside, path, missing, WRITTEN_MODULE)) + return pairs + + +def _imports_on_disk(root: Path, path: str) -> dict[str, set[str]]: + """What the kept copy at `path` imports, read from its bytes on disk by + `_names_imported` (#604). Nothing for a copy that is not `.py`, or whose + bytes cannot be read: what this cannot tell, it does not refuse. + """ + if not path.endswith(".py"): + return {} + try: + data = (root / path).read_bytes() + except OSError: + return {} + return _names_imported(data) + + +def _names_dropped(root: Path, path: str, data: bytes | None) -> set[str]: + """The names the module at `path` defines now, in its bytes on disk, + and not in `data`, the bytes this run writes there (#604): what this + run takes away from it. + + EMPTY WHERE EITHER SIDE CANNOT BE TOLD (`_names_defined`, + `_names_defined_in`): bytes Python could not parse, a star import, a + module `__getattr__`. And empty for a module with nothing on disk yet, + an `--add` file, which defines nothing this run could take away. + """ + now = _names_defined(root / path) + after = _names_defined_in(data) if data is not None else None + if now is None or after is None: + return set() + return now - after + + +def _missing_name_refusal(pairs: list, kept: dict, target: str, + kind: Kind) -> Refusal: + """The one `update-accept-local-missing-name` refusal, one line per + pair, in either direction (#536, #604). Every sentence in it is true + of every line it lists, whichever side of the line is the kept copy; + the validators clause is said only where no importer named is one of + them, as `_refuse_kept_copies_missing_names` says why. + """ + validated = any(importer in kind.validators + for _, importer, _, _ in pairs) + return Refusal( + "update-accept-local-missing-name", + "--accept-local keeps this project's own bytes of a copy that " + "disagrees with a copy this run writes beside it: one of the two " + "imports a name from the other that the other's bytes do not " + "define, so the importer would stop on an ImportError the first " + "time it runs" + + ("" if validated else + ", though the validators, which never import it, pass") + + ":\n" + "\n".join(_missing_name_line(*pair) for pair in pairs), + _missing_name_remediation(pairs, kept, target)) + + +def _missing_name_line(module: str, importer: str, names: list[str], + side: str) -> str: + """One pair's line: which copy is kept, which written, and the names. + """ + said = ", ".join(names) + if side == KEPT_MODULE: + return (f" {module} (kept) lacks {said}, which {importer} " + "imports from it") + return (f" {module} (written) no longer defines {said}, which " + f"{importer} (kept) imports from it") + + +def _missing_name_remediation(pairs: list, kept: dict, target: str) -> str: + """The human's two exits, for each kept copy the lines name (#536, + #604). Keeping the edit is the human's decision: change the kept copy + so it agrees, as the side it is on asks, commit it, and run again with + the same `--accept-local`; or drop the edit, so the upstream's copy is + taken. `--accept-local` is named in prose, and no NEXT line passes it. + """ + named = sorted({module if side == KEPT_MODULE else importer + for module, importer, _, side in pairs}) + upstreams = "; ".join(f"`git show {target}:{kept[path].source}` for " + f"{path}" for path in named) + both = [path for path in named if kept[path].state == BOTH] + sides = {side for *_, side in pairs} + changes = [] + if KEPT_MODULE in sides: + changes.append("merge the upstream's version by hand into a kept " + "copy that lacks a name, so it defines every name " + "above") + if WRITTEN_MODULE in sides: + changes.append("change a kept copy that imports a name the module " + "written beside it no longer defines, so it no " + "longer takes that name") + return ( + "Remediation: whether the project keeps its edit is the human's " + "decision, and a kept copy must still agree with the copies " + "written beside it on every name one imports from the other. " + "Either " + ", and ".join(changes) + ", commit it, and run this " + "again with the same --accept-local; or drop the edit, so the " + "upstream's copy is taken: write the upstream's bytes over it, " + "commit that, and run this again without --accept-local on that " + f"path. In a shape clone the upstream's bytes are {upstreams}" + + (", and for a kept copy that imports a name they show what the " + "upstream's own copy of that file imports instead" + if WRITTEN_MODULE in sides else "") + + "." + + (f" {', '.join(both)} changed upstream too, so dropping the edit " + "there is `update-conflict`'s merge by hand again, settled on the " + "upstream's side." if both else "") + + " Nothing was written.") def _module_beside(path: str, module: str) -> str: @@ -3886,8 +4025,23 @@ def _is_type_checking(test: ast.expr) -> bool: def _names_defined(path: Path) -> set[str] | None: - """Every name the kept copy at `path` binds at module scope (#536), as - `_bound_at` reads a binding, or None when it cannot tell. + """Every name the copy at `path` binds at module scope (#536), as + `_names_defined_in` reads its bytes, or None when it cannot tell: also + where the bytes cannot be read, which for a module with no file yet, an + `--add` file, is a module that defines nothing to take away (#604). + """ + try: + data = path.read_bytes() + except OSError: + return None + return _names_defined_in(data) + + +def _names_defined_in(data: bytes) -> set[str] | None: + """Every name a module's bytes bind at module scope, as `_bound_at` + reads a binding, or None when it cannot tell (#536). Split out of + `_names_defined` for #604, which reads a module from the bytes this run + is about to write there, under the same rules as a file on disk. PARSED FROM THE BYTES, as Python's own import reads them, so a BOM or a coding cookie means what it means to Python. A kept copy is the @@ -3902,8 +4056,8 @@ def _names_defined(path: Path) -> set[str] | None: module `__getattr__`, which answers for any name it likes. """ try: - module = ast.parse(path.read_bytes()) - except (OSError, *UNPARSABLE): + module = ast.parse(data) + except UNPARSABLE: return None names = _bound_at(module.body) return None if names & {"*", "__getattr__"} else names From ece91f634c41c809cb224383e8e24d8abcb1df75 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Thu, 15 Oct 2026 18:47:58 +0000 Subject: [PATCH 2/2] update-shape.py: the missing-name refusal's remediation names which lines each exit settles when it holds both directions, a kept non-.py copy is tested to be passed over, and _names_defined answers a path holding a NUL as before (#604) Fix round 1 for R604's review of 90aba6a4 (APPROVE WITH NITS), as the coordinator ruled. F1. When one refusal held both kinds of line, its remediation said "merge the upstream's version by hand into a kept copy that lacks a name, so it defines every name above", which is false of a `(written) no longer defines` line's names: those the kept copy must stop importing, not define. And "Either A, and B, commit it" put two changes, possibly to two files, under one "it". Each exit now names the lines it settles ("so it defines every name its `(kept) lacks` line names"; "so it no longer takes any name its `(written) no longer defines` line names"), and the sentence says "commit each". A refusal that holds one kind of line gives that kind's exit alone. F2. The "a kept copy that is not .py" case sat at the root, where `repo_shape` resolves to `repo_shape.py` and not `scripts/repo_shape.py`, so the `.py` filter never decided it and R604's mutant r6 (no filter on a kept copy) survived. It is now `scripts/notes.txt`, and so is #562's forward twin (a written copy that is not `.py`). F3. `_names_defined` caught only `OSError` around the read once #604 split the parse out, so a path holding a NUL raised the `ValueError` that base had answered with None. It now catches what base caught around the read: `OSError`, `ValueError` and `MemoryError`. `apply` cannot reach either input, since a row path holding a NUL is refused first. F4. The pair the two listers return is one `Disagreement` NamedTuple (module, importer, names, side), so `_missing_name_refusal` and `_missing_name_remediation` read it by field name. No behaviour change. Tests (`tests/test_update_shape_kept_helper.py`, 105 to 108): the assertions that pinned the old words now pin each exit's new words and "commit each", end to end for both directions in one run and in process; a one-direction refusal of each kind is held to give only its own exit; the two non-`.py` cases sit under `scripts/`; and `_names_defined` is held to answer None for a missing file, a path holding a NUL and a `MemoryError` on the read. Mutants over these bytes, each against the whole file, all killed: the reverse check removed (9 failed); "defined on disk now" dropped (7); the written module read from disk (9); r6, no `.py` filter on a kept copy (1); its forward twin, no `.py` filter on a written copy (1); the old remediation words (6); `except OSError` alone in `_names_defined` (1). Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Opus 5.5 --- tests/test_update_shape_kept_helper.py | 62 +++++++++++++++++++++++--- update-shape.py | 44 ++++++++++++------ 2 files changed, 85 insertions(+), 21 deletions(-) diff --git a/tests/test_update_shape_kept_helper.py b/tests/test_update_shape_kept_helper.py index 5cd00195..92c90cbc 100644 --- a/tests/test_update_shape_kept_helper.py +++ b/tests/test_update_shape_kept_helper.py @@ -85,6 +85,13 @@ #: What the refusal says only when no copy it names is one of the root's #: validators: those import what they need, and would go red. VALIDATORS_PASS = "though the validators, which never import it, pass" +#: Each exit the remediation gives, in the words that name the lines it +#: settles: the merge for a kept copy a `(kept) lacks` line names (#536), +#: and the change for one a `(written) no longer defines` line names (#604). +#: A refusal that holds both says "commit each". +LACKS_EXIT = "so it defines every name its `(kept) lacks` line names" +DROPS_EXIT = ("so it no longer takes any name its `(written) no longer " + "defines` line names") #: The file `added` names in the assembly root's copy lists. ADDED_CHECK = "scripts/kept-helper-check.py" BRANCH = "shape/update-kept-helper" @@ -970,7 +977,7 @@ def test_a_handler_that_misses_a_missing_name_still_refuses(update_shape, @pytest.mark.parametrize("written, how", [ pytest.param({"tools/bootstrap.py": imports("NEW")}, "upstream-changed", id="an importer in another directory"), - pytest.param({"Makefile": imports("NEW")}, "upstream-changed", + pytest.param({"scripts/notes.txt": imports("NEW")}, "upstream-changed", id="a written copy that is not .py"), pytest.param({BOOTSTRAP: imports("NEW")}, "unchanged", id="an importer this run leaves alone"), @@ -1068,7 +1075,8 @@ def test_keeping_a_copy_that_imports_a_name_the_run_drops_is_refused( remedy = result.stderr.split("Remediation:", 1)[1] assert (f"`git show {standard['dropped']}:templates/assembly-root/" f"{BOOTSTRAP}` for {BOOTSTRAP}") in remedy, remedy - assert "so it no longer takes that name" in remedy, remedy + assert DROPS_EXIT in remedy, remedy + assert LACKS_EXIT not in remedy, remedy assert "update-conflict" not in remedy, remedy assert VALIDATORS_PASS in result.stderr, ( "bootstrap.py is none of the validators, and the refusal says so: " @@ -1158,7 +1166,8 @@ def test_both_directions_in_one_run_are_one_refusal(dropping_standard, AND has `siblings.py` import a name only the upstream's `bootstrap.py` defines: the kept copy is the module of one line and the importer of the other. One refusal names both, sorted by module, says the - validators pass (none of the importers is `validate-family.py`), and + validators pass (none of the importers is `validate-family.py`), gives + each exit in the words of the lines it settles, with "commit each", and sends the `both` row to `update-conflict`'s merge.""" checked = run_script(UPDATE, "check", "--root", str(importing_holder), "--upstream", str(dropping_standard["upstream"]), @@ -1176,6 +1185,9 @@ def test_both_directions_in_one_run_are_one_refusal(dropping_standard, dropped_line(BOOTSTRAP), ], result.stderr assert VALIDATORS_PASS in result.stderr, result.stderr + remedy = result.stderr.split("Remediation:", 1)[1] + for words in (LACKS_EXIT, DROPS_EXIT, ", commit each, and run this"): + assert words in remedy, remedy assert (f"{BOOTSTRAP} changed upstream too, so dropping the edit there " "is `update-conflict`'s merge by hand again") in result.stderr @@ -1277,7 +1289,7 @@ def test_a_module_this_run_adds_takes_nothing_away(update_shape, tmp_path): @pytest.mark.parametrize("kept, writing", [ pytest.param({"tools/bootstrap.py": TAKES_GONE}, "upstream-changed", id="a kept importer in another directory"), - pytest.param({"Makefile": TAKES_GONE}, "upstream-changed", + pytest.param({"scripts/notes.txt": TAKES_GONE}, "upstream-changed", id="a kept copy that is not .py"), pytest.param({BOOTSTRAP: TAKES_GONE}, "unchanged", id="a module this run leaves alone"), @@ -1317,10 +1329,46 @@ def test_both_directions_are_one_refusal_with_every_pair_sorted( f" {REPO_SHAPE} (kept) lacks NEW, which {BOOTSTRAP} imports from " "it", ], refused.detail - for change in ("so it defines every name above", - "so it no longer takes that name"): - assert change in refused.remediation, refused.remediation + for words in (LACKS_EXIT, DROPS_EXIT, ", commit each, and run this"): + assert words in refused.remediation, refused.remediation for path in (importer, REPO_SHAPE): assert f"`git show {'a' * 40}:{path}` for {path}" \ in refused.remediation, refused.remediation assert (VALIDATORS_PASS in refused.detail) is said, refused.detail + + +@pytest.mark.parametrize("kept, written, current, said, unsaid", [ + pytest.param({REPO_SHAPE: b"OLD = 1\n"}, {BOOTSTRAP: imports("NEW")}, + None, LACKS_EXIT, DROPS_EXIT, + id="a kept copy that lacks a name (#536)"), + pytest.param({BOOTSTRAP: TAKES_GONE}, {REPO_SHAPE: DROPS_GONE}, + {REPO_SHAPE: DEFINES_GONE}, DROPS_EXIT, LACKS_EXIT, + id="a kept copy that imports a dropped name (#604)"), +]) +def test_a_one_direction_refusal_gives_only_its_own_exit( + update_shape, tmp_path, kept, written, current, said, unsaid): + """A refusal that holds one kind of line gives that kind's exit alone: + the other's words would name lines it does not list.""" + refused = refusal(update_shape, tmp_path, kept, written, + state="locally-modified", current=current) + assert refused is not None, "the pair disagrees" + assert said in refused.remediation, refused.remediation + assert unsaid not in refused.remediation, refused.remediation + + +def test_a_path_it_cannot_read_is_one_it_cannot_tell(update_shape, tmp_path, + monkeypatch): + """`_names_defined` answers None for a path whose bytes cannot be read, + as it did before #604 split the parse out of it: a missing file, the + `ValueError` a path holding a NUL raises, and a `MemoryError`, raised + by a stand-in because no test can rely on running out of memory.""" + assert update_shape._names_defined(tmp_path / "missing.py") is None + assert update_shape._names_defined(tmp_path / "a\x00b.py") is None + path = tmp_path / "repo_shape.py" + path.write_bytes(b"OLD = 1\n") + + def too_big(*_args, **_kwargs): + raise MemoryError("too big to read") + + monkeypatch.setattr(type(path), "read_bytes", too_big) + assert update_shape._names_defined(path) is None diff --git a/update-shape.py b/update-shape.py index ae3b022e..507d4ab6 100755 --- a/update-shape.py +++ b/update-shape.py @@ -156,6 +156,7 @@ import tempfile from collections.abc import Iterator from pathlib import Path +from typing import NamedTuple SHAPE_ROOT = Path(__file__).resolve().parent sys.path.insert(0, str(SHAPE_ROOT / "scripts")) @@ -3716,6 +3717,16 @@ def _refuse_kept_copies_missing_names(root: Path, rows: list[Row], WRITTEN_MODULE = "written" +class Disagreement(NamedTuple): + """One line of `update-accept-local-missing-name` (#536, #604): + `importer` imports `names` from `module`, and `side` says whether the + module is the kept copy (`KEPT_MODULE`) or the written one.""" + module: str + importer: str + names: list + side: str + + def _written_copies(rows: list[Row], taking: list[Addition]) -> dict[str, bytes | None]: """`{path: the bytes this run writes there}`: each `upstream-changed` @@ -3747,7 +3758,7 @@ def _kept_copies_lacking(root: Path, written: dict, kept: dict) -> list: known = defined[helper] missing = sorted(names - known) if known is not None else [] if missing: - lacking.append((helper, path, missing, KEPT_MODULE)) + lacking.append(Disagreement(helper, path, missing, KEPT_MODULE)) return lacking @@ -3771,7 +3782,8 @@ def _kept_copies_importing_dropped(root: Path, written: dict, written[beside]) missing = sorted(names & dropped[beside]) if missing: - pairs.append((beside, path, missing, WRITTEN_MODULE)) + pairs.append(Disagreement(beside, path, missing, + WRITTEN_MODULE)) return pairs @@ -3814,8 +3826,7 @@ def _missing_name_refusal(pairs: list, kept: dict, target: str, the validators clause is said only where no importer named is one of them, as `_refuse_kept_copies_missing_names` says why. """ - validated = any(importer in kind.validators - for _, importer, _, _ in pairs) + validated = any(pair.importer in kind.validators for pair in pairs) return Refusal( "update-accept-local-missing-name", "--accept-local keeps this project's own bytes of a copy that " @@ -3844,30 +3855,33 @@ def _missing_name_line(module: str, importer: str, names: list[str], def _missing_name_remediation(pairs: list, kept: dict, target: str) -> str: """The human's two exits, for each kept copy the lines name (#536, #604). Keeping the edit is the human's decision: change the kept copy - so it agrees, as the side it is on asks, commit it, and run again with - the same `--accept-local`; or drop the edit, so the upstream's copy is - taken. `--accept-local` is named in prose, and no NEXT line passes it. + so it agrees, as the side it is on asks, commit each change, and run + again with the same `--accept-local`; or drop the edit, so the + upstream's copy is taken. Each change names the lines it settles, so + neither is read as settling the other's. `--accept-local` is named in + prose, and no NEXT line passes it. """ - named = sorted({module if side == KEPT_MODULE else importer - for module, importer, _, side in pairs}) + named = sorted({pair.module if pair.side == KEPT_MODULE else pair.importer + for pair in pairs}) upstreams = "; ".join(f"`git show {target}:{kept[path].source}` for " f"{path}" for path in named) both = [path for path in named if kept[path].state == BOTH] - sides = {side for *_, side in pairs} + sides = {pair.side for pair in pairs} changes = [] if KEPT_MODULE in sides: changes.append("merge the upstream's version by hand into a kept " "copy that lacks a name, so it defines every name " - "above") + "its `(kept) lacks` line names") if WRITTEN_MODULE in sides: changes.append("change a kept copy that imports a name the module " "written beside it no longer defines, so it no " - "longer takes that name") + "longer takes any name its `(written) no longer " + "defines` line names") return ( "Remediation: whether the project keeps its edit is the human's " "decision, and a kept copy must still agree with the copies " "written beside it on every name one imports from the other. " - "Either " + ", and ".join(changes) + ", commit it, and run this " + "Either " + ", and ".join(changes) + ", commit each, and run this " "again with the same --accept-local; or drop the edit, so the " "upstream's copy is taken: write the upstream's bytes over it, " "commit that, and run this again without --accept-local on that " @@ -4029,10 +4043,12 @@ def _names_defined(path: Path) -> set[str] | None: `_names_defined_in` reads its bytes, or None when it cannot tell: also where the bytes cannot be read, which for a module with no file yet, an `--add` file, is a module that defines nothing to take away (#604). + Unreadable is what it was before the parse was split out: an `OSError`, + the `ValueError` of a path holding a NUL, or a `MemoryError`. """ try: data = path.read_bytes() - except OSError: + except (OSError, ValueError, MemoryError): return None return _names_defined_in(data)