diff --git a/tests/test_update_shape_kept_helper.py b/tests/test_update_shape_kept_helper.py index 99b695b0..92c90cbc 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. @@ -68,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" @@ -91,6 +115,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 +154,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 +202,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 +211,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 +254,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 +337,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 +407,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 +416,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 +432,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 +445,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 +491,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 +503,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 +634,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) @@ -834,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"), @@ -884,3 +1027,348 @@ 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 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: " + + 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`), 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"]), + "--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 + 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 + + +# --- 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({"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"), + 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 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 fb4473fb..6475d12f 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")) @@ -3636,9 +3637,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 @@ -3656,96 +3657,114 @@ 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) + + +#: 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" + + +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` + row's target bytes, and each `--add` file's (#536, #604). + """ + 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, 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. +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. """ - 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 + 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: @@ -3755,8 +3774,142 @@ 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(Disagreement(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(Disagreement(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(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 " + "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 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({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 = {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 " + "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 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 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 " + 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: @@ -3902,8 +4055,25 @@ 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). + 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, ValueError, MemoryError): + 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 @@ -3918,8 +4088,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