diff --git a/shape-doctor.py b/shape-doctor.py index 3dc65fc9..2d59a9c7 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -24,7 +24,10 @@ else: `--placement-plan `, below. The upstream a project's copies are compared against is THE CHECKOUT THIS FILE IS RUN FROM, which is the whole of why the verdict is offline: no clone, no authenticated call, and the same -answer on a machine that has never spoken to github.com. It runs the project's +answer on a machine that has never spoken to github.com. A partial clone +(`git clone --filter`) fetches what it lacks on a read, so it is asked first, +without fetching, whether it holds what the comparison reads, and the verdict +is CANNOT ANSWER where it does not (#683). It runs the project's own validators OFFLINE: `run_validator` starts each child with no token, no `gh` configuration and a proxy that goes nowhere (#297), so they are offline whatever their own contract says. ONE ROW IS THE EXCEPTION AND IT IS NAMED: @@ -312,10 +315,10 @@ 3 usage or environment -- nothing here is a statement about the tree that was pointed at: no such root, this file is not sitting in a checkout of the standard, or THIS CHECKOUT cannot answer (a pin - naming a commit it does not carry; a validator of its own that - crashed or cannot be given its `PYTHONPATH`, its template copy or its - naming script, #278, #410; or its own naming policy not loading, - #678) + naming a commit it does not carry; a partial clone lacking what the + comparison reads, #683; a validator of its own that crashed or + cannot be given its `PYTHONPATH`, its template copy or its naming + script, #278, #410; or its own naming policy not loading, #678) THE VERDICT NAMES THE MOST SPECIFIC FINDING; THE TABLE NAMES EVERY ONE. Drift outranks a red validator on that line for one concrete reason: an edited shape @@ -2686,6 +2689,18 @@ def with_merged_away_rows(ctx: Context, row: Row, output: str) -> Row: (`linked_record_row`), so both of this row's sentences were false. The same `linked_record` question that row asks keeps this one the gate until the link is replaced; the doctor run again then names `check`. + + AND ONLY WHERE THAT `apply` IS NOT REFUSED (#676). A pinned COPY kept + as a link is no record `linked_record` asks about: `check` reads that + root, calls the copy `copy-is-link` and says REFUSAL AHEAD over it, and + the `apply` it prints is refused, `update-rewrite-symlink`, writing + nothing. So "the `apply` it prints drops its pin row" was false there, + and of a `both`, an `upstream-removed`, an `unmapped` row or a file + `apply` would rewrite that has a hard link. `refusals_ahead` asks + `check`'s own two questions; where it names anything, the row keeps the + gate, says what `check` reports, merged-away and REFUSAL AHEAD both, + and leaves the sentence about `apply` out. Where the doctor cannot ask + them -- the `shape currency` row says why -- it keeps the gate too. """ if row.status != FINDING or row.detail.get("exit") != 1 \ or row.detail.get("crashed"): @@ -2696,14 +2711,27 @@ def with_merged_away_rows(ctx: Context, row: Row, output: str) -> Row: us = ctx.update_shape() if us.linked_record(ctx.root, us.root_kind(ctx.root)) is not None: return row + ahead = refusals_ahead(ctx, us) + if ahead is None: + return row one = len(merged) == 1 copies = ", ".join(f"{path} (merged into {original})" for path, original in merged.items()) - reason = (f"{row.reason}. {copies} " - + ("is a collision copy" if one else "are collision copies") - + " the root merged away, so `update-shape.py check` reports " - + ("it" if one else "them") + " `merged-away`, and the " - "`apply` it prints drops " + said = (f"{row.reason}. {copies} " + + ("is a collision copy" if one else "are collision copies") + + " the root merged away, so `update-shape.py check` reports " + + ("it" if one else "them") + " `merged-away`") + if ahead: + refused = ", ".join(f"{path} ({why})" for path, why in ahead.items()) + return Row(row.id, row.label, row.status, + f"{said}, but it also says REFUSAL AHEAD over {refused}, " + "and the `apply` it prints refuses while " + + ("that is" if len(ahead) == 1 else "those are") + + " unresolved, so it drops no pin row yet", + row.next_command, + {**row.detail, "merged_away": merged, + "refusal_ahead": ahead}, why=row.why) + reason = (f"{said}, and the `apply` it prints drops " + ("its pin row" if one else "their pin rows") + " in the re-pin commit: the step the `shape currency` row " "names too") @@ -2714,6 +2742,46 @@ def with_merged_away_rows(ctx: Context, row: Row, output: str) -> Row: {**row.detail, "merged_away": merged}) +def refusals_ahead(ctx: Context, us) -> dict | None: + """`{path: why}` for what `update-shape.py check` names under `REFUSAL + AHEAD` for this root, or None where the doctor cannot ask (#676). + + `check`'S OWN TWO QUESTIONS, over the rows its own `prepare` classifies: + each row that is a conflict (`Row.is_conflict`: `both`, + `upstream-removed`, `unmapped`, `copy-missing`, `copy-is-link`, never + `merged-away`), by its state, and each file `apply` would rewrite in + place that has another name (`hard_linked_rewrites`, #422), as + `hard-link`. `apply` refuses while any of them is there, so its re-pin + commit drops no merged-away row. + + NONE WHERE THE QUESTION CANNOT BE ASKED: the pin does not read, the + checkout of the standard is a partial clone short of what the + comparison reads, where `prepare` would fetch (`partial_clone_row`, + #683), or `prepare` refuses. The `shape currency` row says which, and + the `pins` row keeps its gate. + """ + try: + pin, _rows = us.read_pin(ctx.root) + except Refusal: + return None + if partial_clone_row(ctx, str(pin["commit"]).lower()) is not None: + return None + args = argparse.Namespace(root=str(ctx.root), upstream=str(ctx.shape), + at=None) + try: + root, _pin, rows, _added, upstream, _pinned, _target, kind = \ + us.prepare(args) + except Refusal: + return None + try: + ahead = {row.path: row.state for row in rows if row.is_conflict} + for path, _names in us.hard_linked_rewrites(root, rows, kind): + ahead.setdefault(path, "hard-link") + finally: + upstream.close() + return ahead + + def check_family(ctx: Context) -> Row: own, template = FAMILY_GATE if ctx.manifest_refusal is not None: @@ -3995,16 +4063,61 @@ def one_regular_mode(held: dict) -> bool: def partial_clone(shape: Path) -> bool: """Whether the checkout of the standard at `shape` is a partial clone: - its config names a promisor remote or a partial-clone filter (#547). + its config names a remote git fetches a missing object from (#547). A READ THERE CAN FETCH. `git show` of a blob a `--filter` clone left out asks its promisor remote for it, and Brett Heap's ruling for `taken_with_the_pin` is never to fetch, clone or touch the network. So - `pin_restore` proves no copy there and every one stays the hand merge. - The config is only read, which fetches nothing and writes nothing. - """ - return bool(git_answer(["config", "--get-regexp", r"^(extensions\." - r"partialclone|remote\..*\.promisor)$"], shape)) + `pin_restore` proves no copy there and every one stays the hand merge, + and the `shape currency` row asks first, without fetching, whether the + clone holds what it reads (`partial_clone_row`, #683). Which remotes + those are is `promisor_remotes`' answer. + """ + return bool(promisor_remotes(shape)) + + +#: The config keys `promisor_remotes` reads: git's, in `promisor-remote.c`. +PROMISOR_KEYS = (r"^(remote\..*\.(promisor|partialclonefilter)" + r"|extensions\.partialclone)$") + + +def promisor_remotes(shape: Path) -> list: + """The remotes git fetches a missing object from in the checkout at + `shape`, by the rule git 2.43's `promisor-remote.c` reads its config + with (#683): a remote whose `promisor` is true, a remote with a + `partialclonefilter`, and the remote `extensions.partialclone` names, + that one last, as git asks them. Each name once, in the config's order. + + `promisor = false` ALONE IS NOT ONE (#683). `partial_clone` matched + `remote..promisor` whatever its value, so `false` counted. + Measured with git 2.43 on a `--filter=blob:none` clone: with `promisor + = false` and no `partialclonefilter`, a read of a blob the clone lacks + fails and fetches nothing; with the filter kept beside it, the read + fetches, as with `promisor = true`, a bare `promisor` key or + `extensions.partialclone = origin` alone. So `false` is git's `false`, + read by `git config --type=bool`, and a filter makes a remote one on + its own. A missing object where no remote is one is a read that fails, + which `update-shape.py` refuses (`update-upstream-unreadable`). The + config is only read, which fetches nothing and writes nothing. + """ + true = git_answer(["config", "--type=bool", "-z", "--get-regexp", + r"^remote\..*\.promisor$"], shape, binary=True) + promised = {key for key, _newline, value in ( + record.partition(b"\n") for record in (true or b"").split(b"\0")) + if value == b"true"} + listed = git_answer(["config", "-z", "--get-regexp", PROMISOR_KEYS], + shape, binary=True) + names: list[str] = [] + named = None + for record in (listed or b"").split(b"\0"): + key, _newline, value = record.partition(b"\n") + if key == b"extensions.partialclone": + named = value + elif key.endswith(b".partialclonefilter") or key in promised: + name = key[len(b"remote."):key.rindex(b".")] + names.append(name.decode("utf-8", "replace")) + last = [] if named is None else [named.decode("utf-8", "replace")] + return [name for name in dict.fromkeys(names) if name not in last] + last def as_the_standard_wrote_it(ctx: Context, us, kind, standard, copy: str, @@ -4022,7 +4135,8 @@ def as_the_standard_wrote_it(ctx: Context, us, kind, standard, copy: str, try: source = us.source_for(copy, standard, commit, kind) wrote = standard.blob(commit, source) if source else None - except (OSError, ValueError): + except (Refusal, OSError, ValueError): + # `Refusal`: a read `Upstream.blob` could not make (#683). return False return wrote is not None and wrote == held @@ -4321,6 +4435,112 @@ def unreadable_pin_row(ctx: Context, exc: Refusal) -> Row: command, detail, why=why) +#: A line `rev-list --missing=print` writes for an object the clone lacks: +#: `?` and the object's id, SHA-1 or SHA-256. +MISSING_OBJECT = re.compile(rb"\?(?:[0-9a-f]{40}|[0-9a-f]{64})") + + +def objects_lacking(shape: Path, commit: str) -> int | None: + """How many of the objects `commit` and its tree reach the checkout at + `shape` lacks, asked without fetching (#683), or None where git cannot + list them there. + + `git rev-list --objects --no-walk --missing=print` walks the commit's + tree and prints `?` for each object it does not hold, and + `--missing=` turns git's lazy fetch off for its run: measured with git + 2.43, no fetch in a trace2 log, and the same objects missing after it. + It is asked before any `ls-tree`, which fetches a tree a + `--filter=tree:0` clone lacks. A commit the clone does not hold is + `bad object` there, exit 128, and None, which `partial_clone_row` + counts as lacking: `update-shape.py`'s first read of that commit, its + `rev-parse`, fetches it (measured with git 2.43). + """ + listed = git_answer(["rev-list", "--objects", "--no-walk", + "--missing=print", commit], shape, binary=True) + if listed is None: + return None + return sum(1 for line in listed.split(b"\n") + if MISSING_OBJECT.fullmatch(line)) + + +#: `partial_clone_row`'s `error` (#683). +PARTIAL_CLONE_LACKS = "shape-doctor-partial-clone" + + +def partial_clone_row(ctx: Context, pinned: str) -> Row | None: + """The `shape currency` row for a checkout of the standard that is a + partial clone and lacks an object the comparison reads, or None where + it is no partial clone or lacks none of them (#683). + + THE DOCTOR FETCHED, AND OFFLINE IT CALLED UNEDITED COPIES `both`. + `update-shape.py`'s comparison reads each copy's blob at the pinned + commit and at this standard's, and a `--filter` clone fetches a blob it + lacks on that read. Measured on #683 with git 2.43, + a `--filter=blob:none` clone made five lazy fetches for the five copies + the standard had changed since the pin; with its remote out of reach + the reads failed, `Upstream.blob` read each failure as "no such file at + the pinned commit", and the row said `DRIFTED (5 both)` of copies + nobody edited. So before anything reads this checkout, where + `partial_clone` holds, `objects_lacking` asks of the pinned commit and + of HEAD whether every object their trees reach is here. + + THE WHOLE TREE, NOT THE COPIES' PATHS. `--missing=print` names a missing + object by its id alone, and which path it sits at is read from the tree + that holds it, which a `--filter=tree:0` clone lacks too; and the paths + the comparison reads are `prepare`'s, whose second definition here + would be one to drift. So a partial clone pinned behind its HEAD, where + the clone lacks what its checkout never needed, is this row even where + no copy changed. That is the cost of never fetching, and the NEXT + below ends it. A partial clone whose pin names its HEAD lacks nothing + and answers as a full clone does. + + CANNOT ANSWER, as the row for a checkout short of the pin's commit is: + `environment`, exit 3, nothing said about the repository. THE NEXT IS + THAT ROW'S, MADE TO FIT: there, the fetch a person runs to put in this + checkout what it lacks. Here `fetch --all` would put in no blob, since + a promisor remote's fetch keeps its `partialclonefilter`, so the NEXT + is `fetch --refetch --no-filter` from the remote a read would fetch + from: measured with git 2.43, it leaves the clone lacking nothing and + its config as it was, and the doctor run again answers. It is the + person's fetch, never this doctor's, and the `why` names the other way, + a full clone of the standard, as the precedent's names a clone that + has the commit. + """ + remotes = promisor_remotes(ctx.shape) + if not remotes: + return None + standard = head_of(ctx.shape) + lacking = {commit: objects_lacking(ctx.shape, commit) + for commit in dict.fromkeys((pinned, standard)) if commit} + lacking = {commit: count for commit, count in lacking.items() + if count != 0} + if not lacking: + return None + said = [] + for commit, count in lacking.items(): + which = ("the pinned commit" if commit == pinned + else "this standard's commit") + said.append(f"git cannot list {which} {commit[:12]} there" + if count is None else + f"it lacks {count} object{'' if count == 1 else 's'} " + f"at {which} {commit[:12]}") + remote = remotes[0] + return Row("shape-currency", LABEL_SHAPE_CURRENCY, FINDING, + "the copies cannot be compared without fetching: this " + "checkout of the standard is a partial clone, and " + + " and ".join(said), + f"git -C {quote_arg(ctx.shape)} fetch --refetch --no-filter " + f"{quote_arg(remote)}", + {"error": PARTIAL_CLONE_LACKS, "environment": True, + "pinned": pinned, "standard": standard, "remote": remote, + "lacking": lacking}, + why="this checkout of the standard is a partial clone, and " + "a read of an object it lacks fetches it from " + f"{remote}, which this doctor never does; fetch every " + f"object {remote} holds, then run the doctor again, or " + "run it from a full clone of the standard") + + def check_shape_currency(ctx: Context) -> Row: """`update-shape.py check`, summarised, against THIS checkout. @@ -4340,6 +4560,15 @@ def check_shape_currency(ctx: Context) -> Row: not by a list of refusal codes or a match on their text, and a refusal `read_pin` or `open_upstream` learns later lands on the right side without this file being told. + + A PARTIAL CLONE IS ASKED BEFORE ANYTHING READS IT (#683). Once the pin + reads, and before `prepare` opens this checkout, `partial_clone_row` + asks, without fetching, whether a partial clone holds every object the + comparison reads, and where it does not this row is CANNOT ANSWER with + a fetch of its own for a person to run. A read that fails anyway -- + a clone that lacks objects and has no promisor remote, a damaged one -- + is `update-upstream-unreadable` from `prepare`, CANNOT ANSWER too, and + never a copy called `both`. """ us = ctx.update_shape() # A PIN OR MANIFEST KEPT AS A LINK IS ASKED ABOUT FIRST, before anything @@ -4352,13 +4581,27 @@ def check_shape_currency(ctx: Context) -> Row: args = argparse.Namespace(root=str(ctx.root), upstream=str(ctx.shape), at=None) try: - us.read_pin(ctx.root) + pin, _rows = us.read_pin(ctx.root) except Refusal as exc: return unreadable_pin_row(ctx, exc) + partial = partial_clone_row(ctx, str(pin["commit"]).lower()) + if partial is not None: + return partial try: _root, _pin, rows, additions, upstream, pinned, target, _kind = \ us.prepare(args) except Refusal as exc: + if exc.code == us.UPSTREAM_UNREADABLE: + # A READ THAT FAILED, NOT A COMMIT THAT IS NOT HERE (#683), so + # the `fetch --all` below would bring nothing to read it. + return Row("shape-currency", LABEL_SHAPE_CURRENCY, FINDING, + f"the copies cannot be compared: {exc.detail}", None, + {"error": exc.code, "environment": True}, + why="git could not read what this checkout of the " + "standard holds at that commit, so no copy was " + "compared and none is called both over it; run " + "the doctor from a clone of the standard that " + "git reads whole") # NOT A VERDICT ABOUT THE REPOSITORY. A pin naming a commit THIS # checkout does not carry -- a fork, a branch pin, a clone made # before the commit landed -- is this standard being unable to @@ -4376,17 +4619,19 @@ def check_shape_currency(ctx: Context) -> Row: added = [add for add in additions if not add.present] reported = list(rows) + added counts = us.counted(reported) - # A MERGED-AWAY ROW IS COUNTED WHERE THERE IS ONE (#502, review of PR - # #512). The five states every root has a word for are always - # printed; without `merged-away` beside them the counts fell one - # short of the rows for each such row, and the path `named` below - # stood under no count, in the reason of the very row whose `apply` - # drops it. Zero is left unsaid, so a root with no such row reads - # as it always did. - states = (us.UNCHANGED, us.UPSTREAM_CHANGED, us.LOCALLY_MODIFIED, + # EVERY STATE `check` COUNTS IS COUNTED WHERE THERE IS ONE (#502, + # review of PR #512; #676). The five states every root has a word + # for are always printed; any other state of `update-shape.py`'s + # `ORDER` follows them, in that order, where its count is not zero. + # Without them the counts fell one short of the rows for each such + # row -- `merged-away` until #502, `copy-is-link`, `copy-missing`, + # `upstream-removed`, `unmapped` and `already-at-target` until #676 + # -- and the path `named` below stood under no count. Zero is left + # unsaid, so a root with none of them reads as it always did. + always = (us.UNCHANGED, us.UPSTREAM_CHANGED, us.LOCALLY_MODIFIED, us.BOTH, us.UPSTREAM_ADDED) - if counts.get(us.MERGED_AWAY): - states += (us.MERGED_AWAY,) + states = always + tuple(state for state in us.ORDER + if state not in always and counts.get(state)) summary = " / ".join(f"{counts.get(state, 0)} {state}" for state in states) by_state: dict[str, list[str]] = {} diff --git a/tests/test_doctor_partial_clone.py b/tests/test_doctor_partial_clone.py new file mode 100644 index 00000000..a24b9f82 --- /dev/null +++ b/tests/test_doctor_partial_clone.py @@ -0,0 +1,301 @@ +# SPDX-License-Identifier: Apache-2.0 +"""`shape-doctor.py` run from a partial clone of the standard (#683). + +THE DEFECT, measured on main d4a7fab9 with git 2.43 (#460's L10 and L14). +`check_shape_currency` handed the checkout to `update-shape.py`'s +`prepare`, whose `Upstream.blob` read each copy with `git show`, and a +`--filter=blob:none` clone fetches a blob it lacks on that read: five lazy +fetches, from a doctor whose docstring, AGENTS.md and README.md say it +fetches nothing. With the clone's remote out of reach each read failed, +`Upstream.blob` read the failure as "no such file at the pinned commit", +and the row called the five copies the standard had changed since the pin +`both`: `DRIFTED (5 both)` of copies nobody edited, and `update-shape.py +check`, the row's own NEXT, the same. + +NOW a partial clone is asked first, with `rev-list --missing=print`, which +never fetches, whether it holds every object at the pinned commit and at +HEAD. Where it does not, the row is CANNOT ANSWER, exit 3, and its NEXT is +a fetch for a person to run. A read that fails is +`update-upstream-unreadable`, never a None. And `partial_clone` reads a +remote as git does: `promisor = false` alone is not one, a +`partialclonefilter` is, and so is `extensions.partialclone`, which no test +reached before (#460 L14). + +"FETCHED NOTHING" IS ASKED OF GIT, NOT OF THE NETWORK: `rev-list --objects +--all --missing=print` lists the same missing objects before and after the +run, and lists one at least, so the question has something to answer. +Every remote is a path on this disk reached through `file://`, as +`tests/test_doctor_pin_restore_merges.py` makes its partial clone, and a +remote out of reach is a URL that leads nowhere. Config is appended as +bytes, and nothing is fed to git as text. +""" + +from __future__ import annotations + +import json +import subprocess +from pathlib import Path + +import pytest + +from conftest import FILE_PROTOCOL, git, load_without_bytecode, run_script +from test_shape_doctor import ( # noqa: F401 + UPSTREAM_TARGET, doctor, doctor_module, pasted, project, rows_of, + run_pasted, scaffolded_here, standard, standard_ahead, verdict_line, +) + +#: The doctor's `error` for a partial clone that lacks what the comparison +#: reads, and `update-shape.py`'s for a read that failed. +PARTIAL = "shape-doctor-partial-clone" +UNREADABLE = "update-upstream-unreadable" + +#: What the verdict line opens with for the row this file is about. +CANNOT_ANSWER = ("CANNOT ANSWER (shape currency: the copies cannot be " + "compared without fetching") + + +def head(repo: Path) -> str: + """The commit `repo`'s HEAD names, 40 hex.""" + return git("rev-parse", "HEAD", cwd=repo).stdout.strip() + + +@pytest.fixture +def moved(standard, tmp_path) -> dict: + """The standard at C1, the commit the project is pinned at, and a copy + of it one commit on, C2, which changes one copied template. A + `--filter=blob:none` clone of C2 holds the blobs its checkout needed, + so it lacks C1's copy of that template: one blob, the one a comparison + against the pin reads.""" + ahead = standard_ahead(standard, tmp_path, UPSTREAM_TARGET, + "\n# an upstream fix, after this project was cut\n", + "An upstream fix to a copied file") + git("config", "uploadpack.allowFilter", "true", cwd=ahead) + return {"ahead": ahead, "c1": head(standard), "c2": head(ahead)} + + +def partial_clone_of(source: Path, dest: Path, filter_spec: str = "blob:none", + *options: str) -> Path: + """A `--filter` clone of `source` through `file://`, as a person makes + one by hand. Its own maintenance is off, so no background `gc` a fetch + starts outlives the test.""" + cloned = subprocess.run( + ["git", *FILE_PROTOCOL, "clone", "-q", "--no-local", + f"--filter={filter_spec}", *options, source.as_uri(), str(dest)], + capture_output=True, check=False) + assert cloned.returncode == 0, cloned.stdout + cloned.stderr + git("config", "gc.auto", "0", cwd=dest) + git("config", "maintenance.auto", "false", cwd=dest) + return dest + + +def out_of_reach(clone: Path, tmp_path: Path) -> Path: + """`clone`, its remote now a URL that leads nowhere.""" + git("remote", "set-url", "origin", (tmp_path / "nowhere.git").as_uri(), + cwd=clone) + return clone + + +def missing(clone: Path) -> set: + """Every object `clone` lacks, asked of git without fetching.""" + listed = subprocess.run( + ["git", "rev-list", "--objects", "--all", "--missing=print"], + cwd=str(clone), capture_output=True, check=True).stdout + return {line for line in listed.splitlines() if line.startswith(b"?")} + + +def currency_of(result) -> dict: + """The `shape currency` row of a `--json` run.""" + return rows_of(result)["shape-currency"] + + +def doctored(clone: Path, project: Path) -> tuple: + """`(result, missing before, missing after)` for a `--json` doctor run + from `clone`, the missing objects asked of git on either side of it.""" + before = missing(clone) + assert before, "the clone lacks nothing, so nothing can be fetched" + result = doctor(clone, project, "--json") + return result, before, missing(clone) + + +@pytest.mark.parametrize("filter_spec, reachable", [ + ("blob:none", True), ("blob:none", False), ("tree:0", True), + ("tree:0", False)]) +def test_a_partial_clone_behind_its_pin_cannot_answer_and_fetches_nothing( + moved, project, tmp_path, filter_spec, reachable): + """The doctor run from a partial clone of C2, against a root pinned at + C1: CANNOT ANSWER, exit 3, and nothing fetched, whether the clone's + remote answers or not. A `tree:0` clone lacks C1's root tree as well, + and is asked before any `ls-tree`, which would fetch it.""" + clone = partial_clone_of(moved["ahead"], tmp_path / "partial", + filter_spec) + if not reachable: + out_of_reach(clone, tmp_path) + result, before, after = doctored(clone, project) + assert result.returncode == 3, result.stdout + result.stderr + assert json.loads(result.stdout)["verdict"].startswith(CANNOT_ANSWER) + assert currency_of(result)["detail"]["error"] == PARTIAL + assert after == before, "the doctor fetched" + + +def test_the_row_names_the_commit_short_and_the_fetch_that_ends_it( + moved, project, tmp_path): + """What the row says of the clone it could not read: the pinned commit, + how many objects it lacks there, the promisor remote, and a NEXT that + is that remote's whole fetch, never `fetch --all`, which keeps the + clone's filter and brings in no blob.""" + clone = partial_clone_of(moved["ahead"], tmp_path / "partial") + row = currency_of(doctor(clone, project, "--json")) + assert row["detail"] == { + "error": PARTIAL, "environment": True, "pinned": moved["c1"], + "standard": moved["c2"], "remote": "origin", + "lacking": {moved["c1"]: 1}}, row + assert pasted(row["next"]) == [ + "git", "-C", str(clone.resolve()), "fetch", "--refetch", + "--no-filter", "origin"] + assert row["why"].startswith("this checkout of the standard is a " + "partial clone"), row["why"] + + +def test_the_next_run_as_printed_lets_the_doctor_answer(moved, project, + tmp_path): + """The NEXT, run as a person pastes it, leaves the clone lacking + nothing, and the doctor run again answers what a full clone of C2 + answers: one copy behind.""" + clone = partial_clone_of(moved["ahead"], tmp_path / "partial") + run_pasted(currency_of(doctor(clone, project, "--json"))["next"]) + assert missing(clone) == set() + again = doctor(clone, project) + assert again.returncode == 1, again.stdout + again.stderr + assert verdict_line(again).startswith( + "COMPLIANT, SHAPE BEHIND (1 upstream-changed, 0 upstream-added)"), \ + again.stdout + + +def checked_out_at_c1(moved: dict, tmp_path: Path) -> Path: + """A partial clone of C2 that checked out C1 and never C2: it holds + every object at C1, and lacks C2's copy of the template.""" + clone = partial_clone_of(moved["ahead"], tmp_path / "partial", + "blob:none", "--no-checkout") + git(*FILE_PROTOCOL, "checkout", "-q", "--detach", moved["c1"], cwd=clone) + return clone + + +def test_a_partial_clone_pinned_at_its_own_head_answers_as_a_full_clone( + moved, standard, project, tmp_path): + """HEAD is C1, the commit the pin names: the clone lacks C2's copy of + the template, which nothing here reads, and nothing at C1. The verdict + is the full clone's, and the blob it lacks is still not fetched.""" + clone = checked_out_at_c1(moved, tmp_path) + result, before, after = doctored(clone, project) + full = doctor(standard, project, "--json") + assert result.returncode == 0, result.stdout + result.stderr + assert json.loads(result.stdout)["verdict"] == \ + json.loads(full.stdout)["verdict"] + assert after == before, "the doctor fetched" + + +def test_the_commit_head_names_is_asked_too(moved, project, tmp_path): + """The clone holds every object at C1, the pinned commit, and HEAD is + moved to C2 without a checkout, so the template's blob the comparison + reads at the target is the one it lacks. The row names HEAD's commit + alone, and nothing is fetched, with the remote in reach.""" + clone = checked_out_at_c1(moved, tmp_path) + git("update-ref", "--no-deref", "HEAD", moved["c2"], cwd=clone) + result, before, after = doctored(clone, project) + assert currency_of(result)["detail"]["lacking"] == {moved["c2"]: 1} + assert after == before, "the doctor fetched" + + +def test_promisor_false_alone_is_a_read_that_fails_never_a_fetch_or_both( + moved, project, tmp_path): + """`remote.origin.promisor = false` with no `partialclonefilter` beside + it: git fetches nothing there, with its remote in reach, so the doctor + does not call the clone partial. The read of C1's blob fails, and the + row is `update-upstream-unreadable`, CANNOT ANSWER, with no fetch to + name; never `both`.""" + clone = partial_clone_of(moved["ahead"], tmp_path / "partial") + git("config", "remote.origin.promisor", "false", cwd=clone) + git("config", "--unset", "remote.origin.partialclonefilter", cwd=clone) + result, before, after = doctored(clone, project) + assert result.returncode == 3, result.stdout + result.stderr + row = currency_of(result) + assert row["detail"]["error"] == UNREADABLE, row + assert row["next"] is None, row + assert after == before, "git fetched" + + +@pytest.mark.parametrize("filter_spec", ["blob:none", "tree:0"]) +def test_update_shape_check_refuses_a_read_that_failed(moved, project, + tmp_path, filter_spec): + """`update-shape.py check --upstream` an unreachable partial clone of + C2: the read of C1's copy fails, and `check` refuses, naming the read, + rather than print the copy `both`. A `tree:0` clone cannot list C1's + files either, and that is the same refusal.""" + clone = out_of_reach(partial_clone_of( + moved["ahead"], tmp_path / "partial", filter_spec), tmp_path) + result = run_script(clone / "update-shape.py", "check", "--root", + str(project), "--upstream", str(clone)) + assert result.returncode == 2, result.stdout + result.stderr + assert f"REFUSED {UNREADABLE}: {clone.resolve()}" in result.stderr, \ + result.stderr + assert "both" not in result.stdout, result.stdout + + +#: Config appended to a full clone's `.git/config`, as bytes, and the +#: remotes git fetches a missing object from under it, in the order it +#: asks them: `promisor-remote.c`'s reading in git 2.43, measured on #683. +PROMISOR_CONFIGS = { + "none": (b"", []), + "extensions.partialclone": ( + b"[extensions]\n\tpartialclone = origin\n", ["origin"]), + "promisor = false": (b'[remote "origin"]\n\tpromisor = false\n', []), + "promisor = true": (b'[remote "origin"]\n\tpromisor = true\n', + ["origin"]), + "a bare promisor": (b'[remote "origin"]\n\tpromisor\n', ["origin"]), + "a filter beside promisor = false": ( + b'[remote "origin"]\n\tpromisor = false\n' + b"\tpartialclonefilter = blob:none\n", ["origin"]), + "the extension's remote last": ( + b"[extensions]\n\tpartialclone = origin\n" + b'[remote "origin"]\n\tpromisor = true\n' + b'[remote "mirror"]\n\tpromisor = true\n', ["mirror", "origin"]), +} + + +@pytest.mark.parametrize("case", list(PROMISOR_CONFIGS)) +def test_partial_clone_reads_a_promisor_remote_as_git_does(standard, + tmp_path, case): + """`partial_clone` on a full `--no-local` clone of the standard, with + `case`'s config appended. With none appended it is False, and with + `extensions.partialclone` alone True (#460 L14: no test reached that + alternative, and a regexp without it survived every one). `promisor = + false` alone is False: git fetches nothing there, a read of a missing + object fails, and that is `update-upstream-unreadable`'s to refuse.""" + appended, remotes = PROMISOR_CONFIGS[case] + clone = tmp_path / "full" + cloned = subprocess.run( + ["git", *FILE_PROTOCOL, "clone", "-q", "--no-local", + standard.as_uri(), str(clone)], capture_output=True, check=False) + assert cloned.returncode == 0, cloned.stdout + cloned.stderr + with (clone / ".git" / "config").open("ab") as config: + config.write(appended) + module = doctor_module(standard) + assert module.promisor_remotes(clone) == remotes + assert module.partial_clone(clone) is bool(remotes) + + +@pytest.mark.parametrize("path, found", [ + (UPSTREAM_TARGET, True), ("templates/assembly-root", False), + ("templates/assembly-root/no-such-file", False)]) +def test_upstream_blob_is_a_files_bytes_or_none_where_there_is_no_file( + standard, path, found): + """`Upstream.blob` on a full clone: a file's bytes, as git holds them, + and None for a path the tree does not have AND for a directory, whose + `git show` listing it once returned as though it were a file's bytes.""" + module = load_without_bytecode(standard / "update-shape.py", + "update_shape_partial_clone") + upstream = module.Upstream(standard, str(standard)) + held = subprocess.run(["git", "cat-file", "blob", + f"{head(standard)}:{path}"], cwd=str(standard), + capture_output=True, check=False).stdout + assert upstream.blob(head(standard), path) == (held if found else None) diff --git a/tests/test_update_shape_merged_away.py b/tests/test_update_shape_merged_away.py index 0979928e..05e2d98a 100644 --- a/tests/test_update_shape_merged_away.py +++ b/tests/test_update_shape_merged_away.py @@ -29,6 +29,7 @@ from __future__ import annotations import json +import os import shlex import shutil import sys @@ -42,6 +43,11 @@ commit_all, doctor, keep_as_committed_link, rows_of, standard, verdict_line, ) +#: A partial clone of the standard one commit on, from the file that owns +#: the doctor's partial-clone question (#683). +from test_doctor_partial_clone import ( # noqa: F401 + doctored, moved, partial_clone_of, +) from test_update_shape import STATES, next_line_of sys.path.insert(0, str(REPO / "scripts")) @@ -391,14 +397,108 @@ def test_the_currency_rows_counts_name_a_merged_away_row(standard, root): counts now sum to the rows.""" merge_away(root, ".gitignore") currency = rows_of(doctor(standard, root, "--json"))["shape-currency"] + assert_the_counts_sum_to_the_rows(currency, " / 1 merged-away") + + +def assert_the_counts_sum_to_the_rows(currency: dict, tail: str) -> None: + """The `shape currency` row's printed counts end with `tail` and add up + to every row `check` classified (`detail.counts`).""" summary = currency["reason"].split(" (pinned ", 1)[0] - assert summary.endswith(" / 1 merged-away"), currency["reason"] + assert summary.endswith(tail), currency["reason"] said = {state: int(count) for count, state in (part.split(" ", 1) for part in summary.split(" / "))} assert sum(said.values()) == sum(currency["detail"]["counts"].values()), \ (said, currency["detail"]["counts"]) +#: A pinned copy at its own name that nothing reads as it runs, so keeping +#: it as a link changes no gate's answer: `check` calls it `copy-is-link`. +LINKED_COPY = "AGENTS-shape.md" + + +@pytest.fixture +def linked_beside_merged(standard, root, tmp_path) -> dict: + """#676's root: `shape/Makefile` merged away and `AGENTS-shape.md` kept + as a committed symlink to the same bytes. The gate's one finding is the + merged-away row's; `check` says REFUSAL AHEAD over the link, which this + asserts first, and the doctor's `--json` rows.""" + merge_away(root, "Makefile") + keep_as_committed_link(root, LINKED_COPY, tmp_path) + checked = update_shape(standard, root, "check") + states = verdicts(checked.stdout) + assert states["shape/Makefile"] == "merged-away", checked.stdout + assert states[LINKED_COPY] == "copy-is-link", checked.stdout + assert f"REFUSAL AHEAD: `apply` will refuse while these are unresolved " \ + f"— {LINKED_COPY}" in checked.stdout, checked.stdout + return rows_of(doctor(standard, root, "--json")) + + +def assert_the_pins_row_keeps_its_gate(pins_row: dict, ahead: dict) -> None: + """The `pins` row beside a REFUSAL AHEAD `ahead` (#676): its NEXT is the + gate, not `check`, its reason names what `check` says, merged-away and + REFUSAL AHEAD both, and no `apply` that drops a row.""" + assert pins_row["detail"]["refusal_ahead"] == ahead, pins_row + assert "update-shape.py" not in pins_row["next"], pins_row["next"] + assert "the `apply` it prints drops" not in pins_row["reason"], \ + pins_row["reason"] + said = ", ".join(f"{path} ({why})" for path, why in ahead.items()) + assert f"reports it `merged-away`, but it also says REFUSAL AHEAD over " \ + f"{said}" in pins_row["reason"], pins_row["reason"] + + +@WINDOWS_SKIP +def test_the_pins_row_keeps_the_gate_beside_a_copy_kept_as_a_link( + linked_beside_merged): + """A pinned COPY kept as a link beside a merged-away row (#676): no + record `linked_record` asks about, so the `pins` row named `check` and + said the `apply` it prints drops the row, while `check` says REFUSAL + AHEAD over the link and that `apply` exits 2 writing nothing.""" + assert_the_pins_row_keeps_its_gate(linked_beside_merged["pins"], + {LINKED_COPY: "copy-is-link"}) + + +@WINDOWS_SKIP +def test_the_currency_rows_counts_name_every_state_check_counts( + linked_beside_merged): + """The `shape currency` reason counted the five original states and + `merged-away` alone (#676): over this root it read `... / 1 merged-away` + over twelve rows, eleven counted, where `check` prints `merged-away 1 + copy-is-link 1`. Every state of `ORDER` with a count now follows the + five, in that order.""" + assert_the_counts_sum_to_the_rows(linked_beside_merged["shape-currency"], + " / 1 merged-away / 1 copy-is-link") + + +def test_the_pins_row_keeps_the_gate_beside_a_hard_linked_pin(standard, root, + tmp_path): + """The other half of `check`'s REFUSAL AHEAD (#422): the pin, which the + re-pin rewrites, with a second name. `apply` refuses it, + `update-rewrite-hardlink`, so it drops no merged-away row either.""" + merge_away(root, "Makefile") + os.link(root / SHAPE_PIN, tmp_path / "pin-elsewhere.yaml") + checked = update_shape(standard, root, "check") + assert "REFUSAL AHEAD: `apply` will refuse to rewrite these in place" \ + in checked.stdout, checked.stdout + rows = rows_of(doctor(standard, root, "--json")) + assert_the_pins_row_keeps_its_gate(rows["pins"], {SHAPE_PIN: "hard-link"}) + + +def test_the_pins_row_keeps_the_gate_where_check_would_fetch(standard, root, + moved, tmp_path): + """Run from a partial clone of the standard one commit on, which lacks + the pinned commit's copy of the Makefile template (#683), the source + of `shape/Makefile`, a row `check` reads: its question cannot be asked + without fetching, so the `pins` row keeps its gate and says nothing + of `apply`, and nothing is fetched.""" + merge_away(root, ".gitignore") + clone = partial_clone_of(moved["ahead"], tmp_path / "partial") + result, before, after = doctored(clone, root) + assert after == before, "the doctor fetched" + pins_row = rows_of(result)["pins"] + assert "merged_away" not in pins_row["detail"], pins_row + assert "update-shape.py" not in pins_row["next"], pins_row["next"] + + def test_the_plans_follow_up_says_what_happens_to_the_row(adopted_here): """The follow-up for each pinned collision copy says that its row goes, and by which step; `shape/README.md`, a rendered file with no row, is diff --git a/update-shape.py b/update-shape.py index 6475d12f..bc6eb703 100755 --- a/update-shape.py +++ b/update-shape.py @@ -357,6 +357,8 @@ def __init__(self, path: Path, label: str, temporary: Path | None = None): #: this machine rather than the standard. self.repository = SHAPE_REPOSITORY self._temporary = temporary + #: `files_at`'s answers, one listing per revision asked about. + self._files: dict[str, frozenset] = {} def close(self) -> None: if self._temporary is not None: @@ -389,22 +391,94 @@ def resolve(self, rev: str | None) -> str: return out.lower() def blob(self, rev: str, path: str) -> bytes | None: - """The bytes of `path` at `rev`, or None when it is not there.""" - proc = subprocess.run(["git", "show", f"{rev}:{path}"], + """The bytes of the file `path` at `rev`, or None when `rev` holds no + file there. + + A READ THAT FAILS IS NOT AN ABSENCE (#683). This was one `git show`, + and any exit but 0 was None, which every caller reads as "the + standard has no such file at that commit". From a partial clone + (`git clone --filter`) whose remote was out of reach, the read of a + blob the clone left out failed, and `_row_verdict` called each copy + the standard had changed since the pin `both`: not a verbatim copy + of a blob that was never there. So where the read fails, `files_at` + says whether `rev` lists a file at `path`. None where it lists none + -- a path the tree does not have, or a directory or a gitlink there, + which hold no file's bytes -- and `update-upstream-unreadable` where + it lists one, so `check` and `apply` refuse rather than guess. + + `git cat-file blob` READS IT, and not `git show`: the same `: + ` and the same bytes for every file, but a directory is not + read as the listing `git show` prints for one. Where the clone is a + partial one, git fetches a blob it lacks from the promisor remote on + this read, as it did on `git show`'s; `shape-doctor.py` asks first, + without fetching, whether any read would need one. + """ + read = subprocess.run(["git", "cat-file", "blob", f"{rev}:{path}"], cwd=str(self.path), capture_output=True, check=False) - return proc.stdout if proc.returncode == 0 else None + if read.returncode == 0: + return read.stdout + if path.encode("utf-8", "surrogateescape") not in self.files_at(rev): + return None + raise unreadable_upstream( + f"{self.label} holds {path!r} at {rev[:12]}, and git could not " + f"read it", read) + + def files_at(self, rev: str) -> frozenset: + """Every path `rev` lists as a file, as git's bytes: `ls-tree -r`'s + blob records, a symlink's included (#683). Read once per revision, + and only where a read has failed, to tell a path that is not there + from one that could not be read. A listing that fails is not an + empty tree: it is `update-upstream-unreadable` too.""" + if rev not in self._files: + listed = subprocess.run( + ["git", "ls-tree", "-r", "-z", "--full-tree", rev], + cwd=str(self.path), capture_output=True, check=False) + if listed.returncode != 0: + raise unreadable_upstream( + f"{self.label} could not list the files at {rev[:12]}", + listed) + self._files[rev] = frozenset( + record.partition(b"\t")[2] + for record in listed.stdout.split(b"\0") + if record.split(b" ", 2)[1:2] == [b"blob"]) + return self._files[rev] def tree_sha256(self, rev: str) -> str: return tree_digest(self.path, rev) +#: The refusal for a read of the upstream that failed (#683). +UPSTREAM_UNREADABLE = "update-upstream-unreadable" + + +def unreadable_upstream(said: str, proc: subprocess.CompletedProcess + ) -> Refusal: + """`update-upstream-unreadable`: `said`, then git's exit and its own + words, one line each run of them, so a refusal is one line to read.""" + words = "; ".join(line.strip() for line in proc.stderr.decode( + "utf-8", "replace").splitlines() if line.strip()) + return Refusal( + UPSTREAM_UNREADABLE, + f"{said} (git exit {proc.returncode}: {words or 'no message'})", + "Remediation: a file the comparison reads could not be read, so this " + "refuses rather than guess a verdict without it. Where --upstream is " + "a partial clone (`git clone --filter`), git fetches what a read " + "needs from that clone's remote, and that fetch failed: reach the " + "remote and run this again, or point --upstream at a full clone of " + "openRepoShape.") + + def open_upstream(spec: str) -> Upstream: """`--upstream` as a repository this tool can read. - A PATH IS USED IN PLACE AND NOTHING IS FETCHED. That is deliberate: it is - what lets the tests run with no network, and it is also the honest answer - for an organisation whose fork is the upstream. `owner/repo` is bare-cloned + A PATH IS USED IN PLACE AND THIS TOOL FETCHES NOTHING INTO IT. That is + deliberate: it is what lets the tests run with no network, and it is + also the honest answer for an organisation whose fork is the upstream. + Where the path is a partial clone (`git clone --filter`), git itself + fetches each object a read needs that the clone lacks from the clone's + promisor remote, and a read whose fetch fails refuses as + `update-upstream-unreadable` (#683). `owner/repo` is bare-cloned into a temporary directory, which is the one place this tool fetches the shape, and it refuses loudly rather than degrading to a guess. It is not the only place `apply` can touch a network (#411): the project's own @@ -689,10 +763,12 @@ def _classify_row(root: Path, row: Row, upstream: Upstream, pinned: str, certain. THE PINNED BLOB IS THE EXCEPTION and is passed on as `None` when the standard carried no such source at the pinned commit: that is not a hole in the comparison but an upstream change, and the ladder reads it as - one. The reads are left in the order they were written in — the root's - bytes, then the pinned blob, then the target blob — because - `Upstream.blob` shells out to git and a reordering would change what a - failing run prints. + one. Only then: a read that FAILED is `Upstream.blob`'s refusal, never a + `None`, or a partial clone out of reach of its remote made every copy + the standard changed since the pin a `both` (#683). The reads are left + in the order they were written in — the root's bytes, then the pinned + blob, then the target blob — because `Upstream.blob` shells out to git + and a reordering would change what a failing run prints. """ target_path = root / row.path if not target_path.is_file():