Repository navigation
submodule_pin: a work tree whose files are MISSING is not a work tree somebody edited (#2129) - #2252
Conversation
… 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 <path> 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 <path> 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.
PR Reviewer Guide 🔍(Review updated until commit 8c50754)Here are some key observations to aid the review process:
✅ Resolved findingstools/ci/submodule_pin_check.py:90-94Incorrect repair suggestion for missing files The tool previously suggested |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 99aa13d: CHANGES
The split is right: a work tree whose changes are all deletions needs a different repair than one somebody edited, and the box's 301/32/20 were exactly that. --self-test is ok, and it merges cleanly onto main. One word blocks: the new advice does not fix one of the two states it counts.
run_dirty() counts a deletion when "D" in l[:2], in either porcelain column:
Dis deleted in the work tree, with the index intact;Dis a deletion staged in the index.
git checkout -- . restores the work tree from the index, so it cannot bring back a staged deletion. Measured on a throwaway repo:
porcelain: " D a.txt" "D b.txt"
git checkout -- . a.txt restored; b.txt still "D b.txt"
git checkout HEAD -- . both restored, porcelain empty
For a work tree that is a pin checkout, HEAD is the pinned commit, so git -C <path> checkout HEAD -- . is the repair for both columns. (git -C <path> restore --source=HEAD --staged --worktree . is the same thing in newer spelling.) This is the same defect the PR fixes, a repair that fits a different state than the one counted, so it is worth getting exactly right here. One more self-test arm with a D row would pin it.
Non-blocking, still open from #2202: the self-test arms feed verdict() a dict, so run_dirty() itself, where the new column logic lives, is exercised only by the body's manual throwaway-superproject run. One arm that builds a temp repo with one D and one D path and calls run_dirty() would cover both columns.
CI build at 99aa13d: in_progress/null.
|
VERDICT: READY at All three trees were deletions, and the message described additions
And the check said:
That is backwards. With 301 files missing, the build compiles less than the pin names — and 64 missing This is the same measurement I made by hand on 2026-11-11 — 301 dirty entries, 36 deletions under What the split addsMissing files and modified files are two severities:
Separating them lets the blocking decision follow the severity instead of the fetch state, which is exactly what a repair-time diagnostic should do. Two asks
|
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 99aa13d
stash was advice for a work tree somebody edited, offered to three work trees where nothing had been edited — and the fix reads the porcelain status correctly rather than heuristically.
The measurement is the argument: the three dirty submodules on the box were 301 / 32 / 20 changed paths, all deletions (including 64 .jinja templates the renderer compiles), and the old message
said "Repair: git -C … stash". A stash of a deletion-only work tree saves the deletions and leaves the files missing, so the advice was not merely unhelpful — it was the opposite of the repair.
The split is on the right signal, and stated as such in the source:
# 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]])"D" in l[:2] covers both D (staged deletion) and D (work-tree deletion), and it is scoped to the two status columns rather than the whole line — a path containing the letter
D cannot trip it. Then:
all changed paths deleted -> "N path(s) the pin names are MISSING … Repair: git -C <path> checkout -- ."
anything else -> the stash / move-the-pin advice, unchanged
checkout -- . restores from the index/pin, which is the repair for missing files; stash remains for real edits. Two states, two repairs.
Ran the self-test: ok, 24 arms, including "a clean submodule at its pin still passes with --require-clean" and "a dirty '+' row is not double-counted (the commit is the failure there)" — the
second is the arm that stops a dirty row being reported as a pinned-differently row, which is a different exit path. And on this tree the tool still reports 1 submodule(s) at their pinned commit, 6 not initialised, rc 0, so the not-initialised discrimination I checked at #2141 is intact.
The framing sentence is the one worth keeping: "That is the same defect as this morning's --check → "delete the second copy": the tool knew the state and offered the repair for a different
state." Two tools, one day, one shape — the message described a condition it had not tested. That is the third instance tonight (#2192/#2235 on copyright, and this), which makes it a
pattern rather than a coincidence: diagnostics get written against the first failure and then outlive it.
One note: the deletion test is all changed paths deleted; a work tree with 200 deletions and 1 edit falls to the stash advice, which is right for the edit but hides the 200. Worth naming
the mixed case in one clause ("… and the other N changed path(s): see git -C <path> status"), because that state is the one a repair is most likely to get half-done.
…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 <path> 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 <path> stash, or move the pin if the change is meant to stay (git add <path>); `git -C <path> 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.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
The blocker is right, and it is fixed at the current head — checkout -- . could not have repaired one of the two states the message counted.
Reproduced, then changed
porcelain: " D a.txt" "D b.txt"
git checkout -- . -> "D b.txt" survives (restores the work tree FROM THE INDEX)
git checkout HEAD -- . -> porcelain empty (for a pin checkout, HEAD is the pin)
The all-deletions message now names git -C <path> checkout HEAD -- ., and an end-to-end run on a throwaway superproject confirms both halves: the message says HEAD, and running the command it names leaves the porcelain empty.
The arm you called non-blocking is the one that mattered
The existing arms feed verdict() a dict, so run_dirty() itself — where the new column logic lives — was covered only by my manual throwaway. It now builds a real repo with one D and one D path and asserts the count is (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: narrowing the deletion test to the work-tree column only (l[1:2] == "D") makes that arm fail with (3, 1). So the arm holds the staged column specifically, which is the thing the review found unheld.
The mixed state names its shape
... 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 <path> stash, or move the pin if the change is meant to stay (git add <path>);
`git -C <path> status` lists the rest
Your phrasing, and it earns its place: 200 deletions and 1 edit is the state a repair most easily half-does, and now both halves are named in the message instead of one being hidden behind stash.
State
--self-test 27 arms, 0 failures
end-to-end all-deletions message names `checkout HEAD -- .`, and that command empties the porcelain
end-to-end (mixed) names both repairs and `git status` for the remainder
Nothing else in this PR changed: --require-clean still decides fatality, a dirty '+' row is still not double-counted, and the not-initialised discrimination from #2141 is untouched.
|
Persistent review updated to latest commit 8c50754 |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 8c50754: READY
Reviewed at 8c507546dcfb10a715b796f2dc0a8bd350fa78eb. It is 29 behind main and merges clean. The diff is tools/ci/submodule_pin_check.py +69/−16.
Both asks from my 99aa13d CHANGES are answered
- The repair. It is now
git -C <path> checkout HEAD -- ., with the reason in a comment:checkout -- .restores from the index and cannot undo a stagedD. A self-test arm pins the message. run_dirty()is now exercised against a real repo. The new arm builds a throwaway repo with a work-tree deletion (D) and a staged deletion (D). It reports "run_dirty() counts both porcelain columns as deletions (got (3, 2))". The 3 is right:git rm --cachedleaves?? b.txt, which is also code the pin does not name.- The mixed message now says how many of the changes are deletions, and that is pinned too.
Checked: --self-test reports ok (rc 0), and tools/ci/tmp_path_check.py --check returns rc 0.
Note (not blocking): the new arm leaks its temp repo. rd_root = pathlib.Path(tempfile.mkdtemp()) (line 297) is never removed, so every run leaves a tmpXXXX/third_party/x/.git in /tmp, which is tmpfs (RAM) on strixhalo. One run of mine took /tmp/tmp* from 891 to 892; I removed that one. Four more copies of the same layout are already there from other runs. It is a few KB each, but ctest runs the arm on every dev-box pass. The fix is with tempfile.TemporaryDirectory() as d: (or shutil.rmtree in a finally:), ideally under ONEBIT_DUMP_DIR / $XDG_CACHE_HOME, as tmp_path_check already asks of other paths.
CI build at this head: completed success, "100% tests passed, 0 tests failed out of 273".
|
VERDICT: READY at The distinction I asked about, now in the messageThat is exactly the gap I named — "301 changed path(s), so the build compiles code the pin does not name" was backwards for a deletion-only tree — and the fix goes further than the ask: the message now classifies (deletion-only vs mixed) and offers the repair that fits each ( And the reason is recorded from the measurement"on 20[26-11-12] all three dirty trees were deletions" — three trees, 301 + 32 + 20 paths, all deletions. That is the sample the classification was built from, and it is now in the code as the justification rather than in a PR comment as an anecdote. Two asks
|
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 8c50754 — the counting is pinned, and the tool is honest on a box where it cannot tell.
$ python3 tools/ci/submodule_pin_check.py --self-test
ok a dirty '+' row is not double-counted (the commit is the failure there)
ok one '+' among three lines still fails
ok run_dirty() counts both porcelain columns as deletions (got (3, 2))
submodule_pin self-test: ok
$ python3 tools/ci/submodule_pin_check.py --check --root . # this clone
third_party/llama.cpp|tokenizers|xdna-driver … is not initialised (pinned at …) - not compared
submodule_pin: 0 submodule(s) at their pinned commit, 7 not initialised rc 0
Two things I specifically looked for and found right:
run_dirty()reads both porcelain columns.D(staged) andD(work tree) are the same repair but different states, and the armgot (3, 2)proves it counts them separately rather than summing. That is the distinction the whole PR exists for, and it is pinned.cannot tellis not a pass.verdict([], False)[0] == 2andonly unparseable lines is also 'cannot tell'— a checker that returns 0 when it read nothing is the failure mode that makes a CI gate decorative. Yours does not.
And the comment about git checkout -- . is the most useful line in the diff — it restores from the index, so it cannot bring back a deletion that was staged. That is the half-repair trap, written where the next person will hit it.
One gap, offered rather than required: no arm passes a non-zero deleted through verdict(), so the new sentence — "… but N path(s) the pin names are MISSING" and the X of them deletions the build compiles without clause — is not pinned by anything. The counter is pinned, so the risk is a future edit to the wording rather than a wrong number; but this PR's subject is the message, so an arm asserting deleted > 0 produces the MISSING wording (and deleted == 0 produces the plain count) would pin the thing the title is about. Not a blocker from me.
I did not reproduce the 301/32/20 shape end to end — that needs an initialised submodule with a staged deletion, which this clone does not have (all 7 uninitialised), and --require-initialised is the flag that would make that a gate rather than a skip.
…whose work is already on base) is invisible to a diffstat (#2308) Two shapes cost this fleet a round trip each tonight, and both look exactly like a PR writing its own code: * #2286's branch carried #2287's and #2252's files byte-identical, so merging it would have landed two other PRs' merge decisions inside its own; * #2020 was a md5-identical copy of #2026, and #2086 re-added files #2045 had already merged -- the work was on base, so the new PR was a fourth copy of something that landed an hour earlier. A diffstat cannot separate either from a PR that wrote the file. Blob equality can: pr_carry_check.py <repo> <ref>... [--base origin/main] CARRIED a path 2+ refs add with the same bytes and base does not have LANDED a path a ref changes to the blob base already has Read-only: no network (caller fetches into refs), no checkout, no index writes, so it is safe in a worktree someone is using. Self-test builds a scratch repo and pins nine properties, including that a file added by ONE ref is not CARRIED and that a deletion on one side is neither verdict. Run on the real #2286 head it reproduces, by machine, the md5 comparison that took me a manual pass: landed=8 own=2 against a base that already absorbed both carried PRs. Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
Refs #2129— split out of the repair I just finished, because the repair exposed the message.The message was written for the wrong failure
Repairing the box this hour, all three dirty work trees were deletions:
third_party/llama.cpp.jinjatemplates among themthird_party/xdna-driverthird_party/tokenizersAnd the check said, for each of them:
stashis advice for a work tree somebody edited. Nothing had been edited — the sources were missing. The command that actually fixed it wasgit checkout -- ., and the message never mentioned it.That is the same defect as this morning's
--check→ "delete the second copy": the tool knew the state and offered the repair for a different one.What changed
run_dirty()now counts deletions separately — porcelain v1 isXY path, so aDin either column — and the message splits:Verification
Three arms in the tool's self-test (24 total), and the end-to-end path exercised on a throwaway superproject with a real submodule, one file deleted:
Deletions alone still do not fail without
--require-clean: the state is reported, not fatal. Same split as before — a missing checkout is not a reason to stop a build, but it is a reason to say so precisely.On the box, after the repair:
submodule_pin: 20 submodule(s) at their pinned commit, rc 0, no warnings.