From 99aa13dbf3c7172cf933c0a635315f81d54209b8 Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Thu, 12 Nov 2026 01:01:27 -0400 Subject: [PATCH 1/2] submodule_pin: a work tree whose files are MISSING is not a work tree somebody edited (engine#2129) The dirty-at-pin message said, in every case: ... has 301 changed path(s), so the build compiles code the pin does not name. Repair: git -C third_party/llama.cpp stash, or move the pin if the change is meant to stay That is advice for an edited work tree. On 2026-11-12, repairing the box, all three dirty work trees were **deletions**: llama.cpp 301, xdna-driver 32, tokenizers 20 -- files the pin names that were absent from disk, 64 of them `.jinja` templates the renderer compiles. `stash` is not the repair for that and does not say what went wrong: nothing was edited, the sources are *missing*. The right command is `git -C checkout -- .`, which is what actually fixed the box. So `run_dirty()` now counts deletions separately (porcelain v1 `XY path`, `D` in either column) and the message splits: all changed paths deleted -> "... N path(s) the pin names are MISSING from its work tree, so the build compiles without them. Repair: git -C checkout -- ." anything else -> the stash / move-the-pin advice, unchanged Three arms in the tool's own self-test, plus an end-to-end check on a throwaway superproject whose submodule has one file deleted: submodule_pin: third_party/sub is at the pinned 3f71c9a1e119 but 1 path(s) the pin names are MISSING from its work tree, so the build compiles without them. Repair: git -C third_party/sub checkout -- . Deletions alone still do not fail without `--require-clean` -- the state is reported, not fatal, which is the same split as before: a missing checkout is not a reason to stop a build, but it is a reason to say so. --- tools/ci/submodule_pin_check.py | 52 +++++++++++++++++++++++---------- 1 file changed, 36 insertions(+), 16 deletions(-) diff --git a/tools/ci/submodule_pin_check.py b/tools/ci/submodule_pin_check.py index e36be0cdf..bebe83b30 100644 --- a/tools/ci/submodule_pin_check.py +++ b/tools/ci/submodule_pin_check.py @@ -84,14 +84,20 @@ def verdict(rows: list[tuple[str, str, str, str]], require_initialised: bool, bad = unknown = uninit = dirty_at_pin = 0 for flag, sha, path, describe in rows: if flag == " ": - n = (dirty or {}).get(path, 0) + n, ndel = (dirty or {}).get(path, (0, 0)) if n: dirty_at_pin += 1 - out.append( - f"submodule_pin: {path} is at the pinned {sha[:12]} but its work tree has {n} changed " - f"path(s), so the build compiles code the pin does not name. Repair: git -C {path} stash, or " - f"move the pin if the change is meant to stay (git add {path})" - ) + if ndel == n: + out.append( + f"submodule_pin: {path} is at the pinned {sha[:12]} but {n} path(s) the pin names are MISSING " + f"from its work tree, so the build compiles without them. Repair: git -C {path} checkout -- ." + ) + else: + out.append( + f"submodule_pin: {path} is at the pinned {sha[:12]} but its work tree has {n} changed " + f"path(s), so the build compiles code the pin does not name. Repair: git -C {path} stash, or " + f"move the pin if the change is meant to stay (git add {path})" + ) if require_clean: bad += 1 continue @@ -146,13 +152,17 @@ def run_git(root: pathlib.Path) -> tuple[str, int]: return p.stdout, 0 -def run_dirty(root: pathlib.Path, rows: list[tuple[str, str, str, str]]) -> dict[str, int]: - """{path: changed-path count} for the submodules that ARE at their pin. +def run_dirty(root: pathlib.Path, rows: list[tuple[str, str, str, str]]) -> dict[str, tuple[int, int]]: + """{path: (changed, deleted)} for the submodules that ARE at their pin. Only those: a '+' or 'U' row already fails and its dirty count would be noise. `git status --porcelain` inside the submodule is the only thing that reports this state - `git submodule status` prints ' ' for it. + + Deletions are counted separately because the repair differs and the count alone cannot tell them apart: on 2026-11-12 + all three dirty work trees on the box were 301/32/20 **deletions** (files the pin names, absent from disk, 64 of them + `.jinja` templates the renderer compiles), and the message then said `stash` - advice for a work tree somebody edited. """ - out: dict[str, int] = {} + out: dict[str, tuple[int, int]] = {} for flag, _sha, path, _describe in rows: if flag != " ": continue @@ -162,9 +172,11 @@ def run_dirty(root: pathlib.Path, rows: list[tuple[str, str, str, str]]) -> dict except OSError: continue if p.returncode == 0: - n = len([l for l in p.stdout.splitlines() if l.strip()]) - if n: - out[path] = n + rows_changed = [l for l in p.stdout.splitlines() if l.strip()] + if rows_changed: + # porcelain v1 is `XY path`: X is the index, Y the work tree, so a deletion shows as `D ` or ` D` + deleted = len([l for l in rows_changed if "D" in l[:2]]) + out[path] = (len(rows_changed), deleted) return out @@ -242,15 +254,23 @@ def check(ok: bool, what: str) -> None: "... and so does the uninitialised one", ) # engine#2129, the half `git submodule status` cannot report: at the pin, dirty work tree - check(verdict(parse_status(at_pin), False, {"third_party/llama.cpp": 301})[0] == 0, + # deletions-only is its own state with its own repair (2026-11-12: 301/32/20 deletions on the box, message said `stash`) + check(any("MISSING" in m for m in verdict(parse_status(at_pin), False, {"third_party/llama.cpp": (301, 301)})[1]), + "a work tree whose paths are all DELETED says so, and names checkout -- as the repair") + check(any("stash" in m for m in verdict(parse_status(at_pin), False, {"third_party/llama.cpp": (301, 200)})[1]), + "a mixed work tree keeps the stash/pin advice") + check(verdict(parse_status(at_pin), False, {"third_party/llama.cpp": (301, 301)})[0] == 0, + "...and deletions alone still do not fail without --require-clean") + + check(verdict(parse_status(at_pin), False, {"third_party/llama.cpp": (301, 0)})[0] == 0, "at the pin with 301 dirty paths -> pass by default (a deliberate local patch is a real thing)") - check(any("301 changed" in m for m in verdict(parse_status(at_pin), False, {"third_party/llama.cpp": 301})[1]), + check(any("301 changed" in m for m in verdict(parse_status(at_pin), False, {"third_party/llama.cpp": (301, 0)})[1]), "... but it IS reported, with its count") - check(verdict(parse_status(at_pin), False, {"third_party/llama.cpp": 301}, True)[0] == 1, + check(verdict(parse_status(at_pin), False, {"third_party/llama.cpp": (301, 0)}, True)[0] == 1, "... and FAILS with --require-clean") check(verdict(parse_status(at_pin), False, {}, True)[0] == 0, "a clean submodule at its pin still passes with --require-clean") - check(not any("changed" in m for m in verdict(parse_status(ahead), False, {"third_party/llama.cpp": 5})[1]), + check(not any("changed" in m for m in verdict(parse_status(ahead), False, {"third_party/llama.cpp": (5, 0)})[1]), "a dirty '+' row is not double-counted (the commit is the failure there)") # one bad among several still fails: the check is not 'all lines' or 'the first line' From 8c507546dcfb10a715b796f2dc0a8bd350fa78eb Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Thu, 12 Nov 2026 10:10:44 -0400 Subject: [PATCH 2/2] submodule_pin: the deletion repair is `checkout HEAD -- .`, because `checkout -- .` cannot undo a staged deletion The review at 99aa13d is right and the measurement is one command. `run_dirty()` counts a deletion in either porcelain column -- ` D` (work tree) and `D ` (staged in the index) -- but the repair it printed, `git checkout -- .`, restores the work tree FROM THE INDEX and so cannot bring back a staged deletion: porcelain: " D a.txt" "D b.txt" -> after `checkout -- .` "D b.txt" survives -> after `checkout HEAD -- .` porcelain empty For a pin checkout HEAD is the pinned commit, so `git -C checkout HEAD -- .` is the repair for both columns. The advice now says that, and an arm builds a real repo with one of each column to keep it honest. **And the arm the review called non-blocking is the one that matters most**: the existing arms feed `verdict()` a dict, so `run_dirty()` itself -- where the new column logic lives -- was covered only by hand. It now runs against a throwaway repo with one ` D` and one `D ` path and asserts the count: `(3, 2)` -- three changed, two deletions, the third being the file `git rm --cached` leaves untracked, which is also code the pin does not name. Negative control: with the deletion test narrowed to the work-tree column only, the arm fails with `(3, 1)`. The mixed state now names its own shape, since that is the one a repair half-does: ... has 3 changed path(s), 2 of them deletions the build compiles without, so the build compiles code the pin does not name. Repair: git -C stash, or move the pin if the change is meant to stay (git add ); `git -C status` lists the rest 27 arms, 0 failures. End-to-end on a throwaway superproject: the all-deletions message names `checkout HEAD -- .` and that command leaves the porcelain empty; the mixed message names both repairs and `git status` for the remainder. --- tools/ci/submodule_pin_check.py | 43 +++++++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 5 deletions(-) diff --git a/tools/ci/submodule_pin_check.py b/tools/ci/submodule_pin_check.py index bebe83b30..ab0e50b6e 100644 --- a/tools/ci/submodule_pin_check.py +++ b/tools/ci/submodule_pin_check.py @@ -87,16 +87,24 @@ def verdict(rows: list[tuple[str, str, str, str]], require_initialised: bool, n, ndel = (dirty or {}).get(path, (0, 0)) if n: dirty_at_pin += 1 + # `checkout -- .` restores the work tree FROM THE INDEX, so it cannot bring back a deletion staged in the + # index (`D `) -- `checkout HEAD -- .` restores both columns, and for a pin checkout HEAD is the pin. Measured + # on a throwaway repo: after `checkout -- .` the `D ` row is still there; after `checkout HEAD -- .` the + # porcelain is empty. if ndel == n: out.append( f"submodule_pin: {path} is at the pinned {sha[:12]} but {n} path(s) the pin names are MISSING " - f"from its work tree, so the build compiles without them. Repair: git -C {path} checkout -- ." + f"from its work tree, so the build compiles without them. " + f"Repair: git -C {path} checkout HEAD -- ." ) else: + # Mixed: say how many of the changes are deletions, because that is the state a repair half-does. out.append( f"submodule_pin: {path} is at the pinned {sha[:12]} but its work tree has {n} changed " - f"path(s), so the build compiles code the pin does not name. Repair: git -C {path} stash, or " - f"move the pin if the change is meant to stay (git add {path})" + f"path(s){', ' + str(ndel) + ' of them deletions the build compiles without' if ndel else ''}" + f", so the build compiles code the pin does not name. Repair: git -C {path} stash, or " + f"move the pin if the change is meant to stay (git add {path}); " + f"`git -C {path} status` lists the rest" ) if require_clean: bad += 1 @@ -255,8 +263,14 @@ def check(ok: bool, what: str) -> None: ) # engine#2129, the half `git submodule status` cannot report: at the pin, dirty work tree # deletions-only is its own state with its own repair (2026-11-12: 301/32/20 deletions on the box, message said `stash`) - check(any("MISSING" in m for m in verdict(parse_status(at_pin), False, {"third_party/llama.cpp": (301, 301)})[1]), - "a work tree whose paths are all DELETED says so, and names checkout -- as the repair") + _del_msgs = verdict(parse_status(at_pin), False, {"third_party/llama.cpp": (301, 301)})[1] + check(any("MISSING" in m for m in _del_msgs), + "a work tree whose paths are all DELETED says so") + check(any("checkout HEAD -- ." in m for m in _del_msgs), + "...and names `checkout HEAD -- .`: bare `checkout -- .` restores from the index and cannot undo a STAGED deletion") + check(any("200" in m or "of them deletions" in m for m in + verdict(parse_status(at_pin), False, {"third_party/llama.cpp": (201, 200)})[1]), + "a mixed work tree names how many of its changes are deletions") check(any("stash" in m for m in verdict(parse_status(at_pin), False, {"third_party/llama.cpp": (301, 200)})[1]), "a mixed work tree keeps the stash/pin advice") check(verdict(parse_status(at_pin), False, {"third_party/llama.cpp": (301, 301)})[0] == 0, @@ -277,6 +291,25 @@ def check(ok: bool, what: str) -> None: check(verdict(parse_status(at_pin + ahead + at_pin), False)[0] == 1, "one '+' among three lines still fails") + # run_dirty() itself -- where the column logic lives -- against a real repo with one work-tree deletion (` D`) and one + # staged deletion (`D `). The arms above feed verdict() a dict, so without this the new logic is only covered by hand. + import tempfile as _tf + rd_root = pathlib.Path(_tf.mkdtemp()) + sub = rd_root / "third_party" / "x" + sub.mkdir(parents=True) + def _git(*args): + return subprocess.run(["git", "-C", str(sub), *args], capture_output=True, text=True, check=False) + _git("init", "-q") + _git("config", "user.email", "t@t"); _git("config", "user.name", "t") + (sub / "a.txt").write_text("a\n"); (sub / "b.txt").write_text("b\n") + _git("add", "."); _git("commit", "-qm", "base") + _git("rm", "-q", "--cached", "b.txt"); (sub / "a.txt").unlink() + got = run_dirty(rd_root, [(" ", "0" * 40, "third_party/x", "")]) + # 3 changed, 2 deleted: `git rm --cached b.txt` leaves the file untracked (`?? b.txt`), and an untracked path is also + # code the pin does not name -- so the count is right and the intent of the arm is the DELETION count, both columns. + check(got.get("third_party/x") == (3, 2), + f"run_dirty() counts both porcelain columns as deletions (got {got.get('third_party/x')})") + print("submodule_pin self-test:", "ok" if bad == 0 else f"{bad} FAILED") return 0 if bad == 0 else 1