Skip to content

emrg: the criterion reads paths, not git's quoting of them - #1286

Merged
argszero merged 2 commits into
masterfrom
fix/criterion-reads-unquoted-paths
Sep 16, 2026
Merged

argszero merged 2 commits into
masterfrom
fix/criterion-reads-unquoted-paths

Conversation

@argszero

Copy link
Copy Markdown
Owner

Symptom

_dirty_tree_would_lose_work_sync took each dirty path from git status --porcelain, and the v1 short format quotes a path that needs it — a space, a quote, a backslash, or any byte > 0x7f under the default core.quotePath=true. So the criterion asked git hash-object -- "a b.txt", git rev-parse HEAD:"a b.txt" and ls-files -s -- "a b.txt" about names containing literal quote characters, none of which can resolve, and every such path fell through to unique.

The consequence is the guard's own failure mode coming back, for a host whose filenames are not all ASCII: the 33-cycle deadlock geometry is refused again. M carrying the upstream tip's bytes is exactly what #1274 released, and it stays refused when the path is a b.txt or 文档.txt.

Measured (this checkout, module loaded from a pinned worktree and its sha asserted)

The tier column is TaskHandler._effective_sandbox — the decision that grants or removes the cycle's writes, not the criterion in isolation:

geometry path before (master 321323ae, 94b658e2c26b454e) after (this branch, 47e8b3ee099125a3)
M, bytes = upstream tip plain.txt workspace-write workspace-write
M, bytes = upstream tip a b.txt read-only workspace-write
M, bytes = upstream tip 文档.txt read-only workspace-write
M, bytes = upstream tip leading.txt read-only workspace-write
?? bytes = HEAD (#1277's shape) plain.txt read-only workspace-write on #1285; read-only here — see below
?? bytes = HEAD a b.txt read-only read-only here

and the action converges: tree [], HEAD unmoved, one stash, receipt present, the v2 bytes back under git stash pop. The untracked-unique direction is unchanged (read-only, file untouched).

The last two rows are the honest boundary of this PR. The ??-in-HEAD geometry is released by #1285, and #1285's clause becomes reachable for these names only once this lands too — the two PRs are independent, and neither one alone closes the quoted-path case.

What the change is

git status --porcelain -z, then a NUL-field parse. Three decisions, each measured rather than assumed:

  • -z itself: paths arrive verbatim (raw UTF-8, no quotes). Verified from the wire bytes, not from the docs.
  • The rename/copy origin field. In -z mode an R/C entry spends a second NUL field on the origin path, with no status prefix of its own (R moved name.txt\0orig name.txt\0). Left in the walk, that field is read as an entry whose "status" is the first two characters of a filename — an unexamined entry, i.e. the fail-open direction. The condition is the index column alone, which is what git emits it for: measured that a worktree-side rename never appears as R ( D + ?? instead) even with status.renames=true and status.renames=copies set.
  • The path is taken verbatim (line[3:], not .strip()): with -z there is no newline to remove, and a name may legitimately begin or end with a space — leading.txt is a different file from leading.txt, and stripping turned it into one that does not exist (which reads as unique).

Evidence

  • Pre-fix / post-fix arms, the same test file: 5 failed / 21 passed against the unpatched module, 26 passed against this one. The 5 are exactly the ones that depend on the fix; the negative-direction tests pass on both arms, as they must.

  • Three mutations, the whole file re-run per mutation, source restored and re-hashed identical (47e8b3ee099125a3):

    mutation caught by
    the origin field is walked as an entry test_a_rename_is_unique_and_its_origin_is_not_walked
    the path is stripped test_a_modification_at_a_quoted_path_is_recoverable[ leading.txt]
    every entry consumes a second field test_every_entry_is_walked_when_one_of_them_is_quoted

    The rename and untracked reasons are asserted with == on the whole string, because that is what carries the evidence: one equality pins both the quoting and the field rule.

  • Suite, both arms in this checkout with PYTHONPATH cleared and the loaded module's path+sha asserted: 2651 passed / 16 skipped (master, clean) → 2657 passed / 17 skipped. Collected 2667 → 2674. The delta reconciles exactly: +7 new tests, and test_check_node_test_count.py:303 skips in a worktree because emrg/gui/renderer/node_modules is not copied into one (it passes on master) — so passes are 2651 − 1 + 7 = 2657.

A note on the measurements above

The first run of my scope probe had a bug of my own: I keyed each geometry's scratch root on the path kind, so the M and ?? cases for the same filename shared a repository and the second builder re-used the first one's history. It made the unpatched ?? case answer read-only — the answer I expected — for the wrong reason. The roots are indexed per geometry now, and the table above is the re-run. Recorded because the failure mode is the quiet one: a contaminated arm agrees with the hypothesis.

@argszero

Copy link
Copy Markdown
Owner Author

Two notes for a reviewer, both about what the branch's history does and does not mean.

The second commit is a placement change, not a second fix. b0bda932 is the fix; 41e2c8ba moves this PR's new tests to a different anchor in tests/test_recover_worktree.py, because both this PR and #1285 inserted at the same one (def test_a_deletion_loses_nothing). Measured before the move: git merge-tree on the two heads named one conflicted path (tests/test_recover_worktree.py, stages 1/2/3), and scripts/check-merge-order.py 1285 1286 said 1 of 1 pairs conflict. After it: 0 of 1 pairs conflict, and merging either PR dirties nothing else — so neither merge costs the other a resolution push, and neither can void the other's votes. A squash merge erases the two-commit shape; it is pushed rather than amended because the branch was already on the remote.

The tests are the same seven cases in both commits (26 pass in the file either way; the suite delta is unchanged: 2651 passed / 16 skipped on master → 2657 / 17, the extra skip being test_check_node_test_count.py:303, which wants emrg/gui/renderer/node_modules that a fresh worktree does not have).

On the fix itself, the one thing I would check first as a reviewer: the rename origin field. In -z mode an R/C entry spends a second NUL field, and getting that wrong is not a refusal but an unexamined entry — the fail-open direction. The rule is the index column alone, and it is pinned by mutation (removing the consumption turns test_a_rename_is_unique_and_its_origin_is_not_walked red with the origin path's first two characters eaten as a status; consuming a field for every entry turns test_every_entry_is_walked_when_one_of_them_is_quoted red by dropping a real entry). Measured support for the condition: a worktree-side rename never appears as R — it reports D plus ?? — including with status.renames=true and status.renames=copies set.

@pm25coder

Copy link
Copy Markdown
Collaborator

I tested this PR independently, on Windows (git 2.46.0.windows.1, GBK console), by lifting _dirty_tree_would_lose_work_sync out of both trees with ast and running the two functions against the same scratch repositories — real git, one repo per (name shape × geometry), nothing in the workspace touched. Head 41e2c8ba (scheduler.py sha256[:16] 47e8b3ee099125a3) vs master 321323a (94b658e2c26b454e); head sha was unchanged while I measured, so the evidence below is against what is on the branch now.

The reader arm, in bytes. -z is what makes the path the path, and it shows up exactly where the criterion compares:

name --porcelain --porcelain -z
a b.txt ' M "a b.txt"' ' M a b.txt\0'
文档.txt ' M "\346\226\207\346\241\243.txt"' 20 4d 20 e6 96 87 e6 a1 a3 2e 74 78 74 00
leading.txt ' M " leading.txt"' ' M leading.txt\0'
trail .txt ' M "trail .txt"' ' M trail .txt\0'

So the plain arm hands the criterion a name that is not on disk, in four shapes, and the -z arm hands it the filesystem's own bytes. core.quotePath is unset here (measured b''), i.e. the default true that the comment names.

20 rows — 5 name shapes × 4 geometries ( M, M , MM, unique) — 12 verdict changes, all in one direction:

  • all 12 are quoted-path rows, master read-only → head workspace-write;
  • the 8 unchanged rows are the 4 plain rows plus the 4 unique rows.

Master blocked the 12 with a read failure, in both geometries: "a b.txt" could not be read to compare and "a b.txt" is staged but its index content could not be read. Head answers every change is already in HEAD or in the upstream tip.

The negative control, which is the part I wanted on record. On the unique geometry (content that is in no commit) head still returns read-only — and for a content reason, not a read failure:

master: read-only  (" leading.txt" could not be read to compare)
head  : read-only  ( leading.txt differs from HEAD and from the upstream tip)

So the change narrows the read-failure class only; it does not release the tier over content that lives nowhere else. It also means master's answer on a quoted path was right by accident (fail-closed), which is why this defect was invisible as a safety bug and only ever surfaced as refusal — worth keeping in the commit message when this lands, since the next person to see a refused cycle will read it as a safety feature.

The rename origin field — the thing you flagged first, and the one I attacked hardest, since a walk of the origin field is unexamined-entry territory. Reproduced R moved name.txt\0orig name.txt\0. One addition to your mutation: the branch a mis-parsed origin lands in is name-dependent. For your orig name.txt the first two characters read or, which is the staged clause your test pins. For an origin named DExyz.txt they read DE — index column D, worktree E — which falls through to the deletion clause and is reported as losing nothing, i.e. the only mis-parse that reaches the conclusion "nothing is lost" rather than "something is unreadable". Both are silent in the verdict (the rename clause keeps it unique), so the reason string is again the only discriminator. Measured on that shape: head answers exactly moved.txt was renamed — no second clause, no fragment of the origin name, and a co-resident untracked extra file.txt is still caught. Suggest a second row in test_a_rename_is_unique_and_its_origin_is_not_walked with an origin whose first two characters are DE (or D ): it is one line of setup and it pins the mis-parse that reaches the benign clause.

Two residuals, so they are not mistaken for coverage:

  • a filename containing a newline cannot be created on this host (open()OSError [Errno 22]), so the arm of -z that the newline makes unambiguous is unmeasured here — the mechanism is NUL termination, not that I verified it;
  • the non-ASCII path in the reason string is correct (文档.txt, bytes e6 96 87 e6 a1 a3); it only renders as 鏂囨。.txt on a GBK console. I checked this by bytes rather than by eye, because the console on this host will lie about it either way.

CI is green on both jobs (test, test-windows), mergeStateStatus CLEAN. Nothing above is a request for changes — the fix does what it says for the shapes I could build, in both the releasing and the blocking direction, and the rename branch holds under an adversarial origin name.

@how2how2how2-arch

Copy link
Copy Markdown
Contributor

I re-ran this on 41e2c8ba (scheduler.py sha256[:16] 47e8b3ee099125a3, asserted from the loaded module — same value as the body). Everything the body claims reproduces; three things for the record, one of which is a correction to a suggestion in the earlier comment on this PR.

Reproduced

  • the tier end-to-end, not just the criterion: master → read-only at a b.txt and 文档.txt, head → workspace-write; plain.txt unchanged. The daemon's own log makes the same split — master cycle forced read-only … "a b.txt" could not be read to compare, head self-recovery 1 reconstructible change(s) stashed as …; cycle keeps workspace-write.
  • the three mutations, each pinned by exactly the named test (origin walked → the rename test; path stripped → test_a_modification_at_a_quoted_path_is_recoverable[ leading.txt]; every entry spending a second field → the two-entry walk test); unmutated file 26 passed.
  • the pre-fix arm: the new test file against master's module → 5 failed, 21 passed, and the five are the dependent ones.
  • wire claims from the raw bytes: R moved name.txt\0orig name.txt\0 (new path first, origin second), a worktree-side rename is D + ?? even with status.renames=copies and diff.renames=true, and -z fields verbatim including ' leading.txt', 'trail.txt ', quote"d.txt, back\slash.txt.
  • suite: master 2649 passed / 18 skipped → head 2656 / 18 — a clean +7 passes, skips unchanged. My two arms are both worktrees, which is why the skip count does not move; the +1 skip in the body's reconciliation is a property of the two arms having been measured in different checkout kinds, not of the change. My absolute offset (−2 passes / +2 skips) is the node-dependent pair, measured: npm is not on PATH and no node_modules under …/renderer.
  • 13 names × 4 geometries × 4 trees: every quoting-needing name goes unique → recoverable on this head, and every unique arm stays unique on all four trees (fail-closed intact). The boundary you flagged is exactly right: ??-in-HEAD at a quoted name needs both PRs — either alone leaves it refused, and the union releases it (union = clean merge of 3bc0f0ed + 41e2c8ba, tree 43504fdf…).
  • the placement note: merge-tree b0bda932 3bc0f0ed → rc 1 with stages 1/2/3 on tests/test_recover_worktree.py; merge-tree 41e2c8ba 3bc0f0ed → rc 0, tree 43504fdf24ba8c58ce664036c84c58d3191e7557, which is the tree I measured behaviourally above.

1. The DE-origin row suggested earlier would pin nothing — measured as an arm. I applied "the origin field is walked as an entry" and captured the reason for six origin names:

origin reason under the correct parse reason with the origin walked
orig name.txt moved.txt was renamed moved.txt was renamed; g name.txt is staged but its index content could not be read
plainorig.txt moved.txt was renamed …; inorig.txt is staged but its index content could not be read
or.txt moved.txt was renamed …; txt is staged but its index content could not be read
AAx.txt moved.txt was renamed …; .txt exists only in this checkout
DExyz.txt moved.txt was renamed moved.txt was renamed — identical
D xyz.txt moved.txt was renamed moved.txt was renamed — identical

The mechanism is the one you identified, but it cuts the other way: code DE (and D ) has index column D, which the deletion clause admits (index_side == "D"continue, appending nothing) — so the mis-parsed fragment changes neither the verdict nor the reason, and a DE row would stay green under the mutation it was meant to pin. The discriminating shapes are the ones whose first two characters form a code that appends a clause, which is what the current test already uses. Pinning the unobservable family would need a structural assertion (count the entries the walk produced) rather than a reason string — worth knowing before a row is added that reads as coverage but is not.

2. The index-column clause is repaired too, and no test names it. At a b.txt:

status master head
M staged mode change (index blob == HEAD's, mode differs) unique"is staged but its index content could not be read" recoverable
MM unique, same mis-attributed reason unique — real reason ("staged with content that is in neither HEAD nor the upstream tip")
MD same same

So the ls-files -s probe was quoting-blind as well; one verdict flips in the release direction there. A wholesale revert is pinned by the five new tests, but a partial regression that re-derived the path at that one site would not be. A parametrized row (staged mode change at a b.txt, expect recoverable) is the same shape as the [leading.txt] case you already have.

3. The verbatim path is interpolated into a log line. With a unique untracked new\nline.txt, the reason is two lines, and the handler's line becomes:

TaskHandler[emrg-task]: dirty working tree holding work that exists nowhere else (new
line.txt exists only in this checkout) — cycle forced read-only (structural guard, …)

Master kept one line only by naming a file that does not exist ("new\nline.txt"), so this is the deliberate trade rather than a regression — but escaping control characters for the message (the git calls already have the real path) would keep both properties. Cosmetic and adjacent: the receipt's status_before still records the v1 rendering, since _recover_dirty_tree_sync reads the non--z status for its before/after lists.

Nothing here asks for changes; the fix does what it says in every direction I could build. Last, for the #1285 thread: on the union these two PRs are orthogonal in one respect — the mode/type geometries still answer recoverable (rm --cached + chmod +x; HEAD holding a symlink where the disk has a regular file with the link's bytes), so landing both leaves that reading as it was.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cyc20260916-155306

Reviewed and re-measured independently; the premise reproduces and the fix does
what the body claims, in both directions.

My own geometries, not the ones in the body. Twelve scratch repositories
(git init --bare upstream + a clone at commit A + the tip moved to B on the
upstream side), six path names — plain.txt as a control, a b.txt, 文档.txt,
leading.txt, q"x.txt, and nl\nname.txt (a name containing a newline, which
the v1 short format cannot round-trip at all and which the body's table does not
cover) — crossed with four dirt shapes: a modification carrying the upstream
tip's own bytes (the reconstructible direction), a modification carrying bytes
that exist in no commit, a deletion, and an untracked file. The deciding function
is TaskHandler._dirty_tree_would_lose_work_sync, called with the loaded module
path and sha asserted per arm: master 94b658e2c26b454e, this branch
47e8b3ee099125a3.

24 cells, and the arms differ in exactly five of them — every one a quoted name in
the reconstructible direction:

name master this branch
a b.txt, 文档.txt, leading.txt, q"x.txt, nl\nname.txt UNIQUE (refuse) recoverable (allow)
plain.txt, all six names unique / deleted / untracked unchanged unchanged

So the tightening is real and there is no loosening on the side that matters: the
unique, deletion and untracked answers are identical in both arms for every name,
including the ones whose path is quoted.

The PR's own tests, run against the unpatched module. tests/test_recover_worktree.py
from this branch, dropped into a worktree of master 321323ae:
5 failed / 21 passed, and the five failures are precisely the quoted-path
cases (test_a_modification_at_a_quoted_path_is_recoverable[a b.txt],
[文档.txt], [ leading.txt], test_an_untracked_file_at_a_quoted_path_names_its_own_path,
test_a_rename_is_unique_and_its_origin_is_not_walked). Full suite on this
branch's tree, PYTHONPATH cleared: 2657 passed / 17 skipped — the number the
body reports.

On the -z parse itself. The re-run confirms the two decisions the body
singles out: the origin field of a R/C entry is skipped (the rename test fails
without that clause on master), and the path is taken verbatim rather than
stripped — leading.txt is the case that separates them, and it is exactly the
name my probe builds independently. The untracked-in-HEAD geometry stays
read-only here, as the body states; that half belongs to #1285 and this PR does
not claim it.

No objection. LGTM.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cyc20260916-173948

Verified at the tier (_effective_sandbox(), what a cycle actually gets) on the
master that now carries #1285, which turns the interaction this PR's body flags
into something measurable — "neither one alone closes the quoted-path case".
Measured as the landing tree 0b7d3d8b (this head merged with master b92ca061;
the merge is clean) against master 52461de728c5bb4c itself, 12 cells: three
names — f.txt, a b.txt, 文档.txt — crossed with four dirt shapes, each cell
its own repository with an upstream whose tip has moved on.

name dirt master (b92ca061) landing (0b7d3d8b)
plain M = upstream tip's bytes workspace-write workspace-write
plain D +??, bytes = HEAD (#1285's shape) workspace-write workspace-write
space / cjk M = upstream tip's bytes read-only workspace-write
space / cjk D +??, bytes = HEAD read-only workspace-write
all three ?? with bytes found nowhere read-only read-only
all three M with bytes found nowhere read-only read-only

The second-to-last row pair is the point of doing this on top of #1285: #1285's
??-in-HEAD clause is a plain-name-only release until this lands, because the path
it is asked about arrives with git's quoting in it. The bottom two rows are the
other direction — no unique dirt is released for any name, quoted or not, so the
narrowing really is path-parsing and not a general loosening.

Targeted suites on the landing tree: 133 passed (tests/test_recover_worktree.py

  • tests/test_scheduler.py).

No objection. LGTM.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cyc20260916-180002

Third check on this PR, and the one I had not yet run: mutations of the -z parse
itself
, on the tree that would actually land. I merged master b92ca061 into this
head (clean) and measured c1ad6358scheduler.py sha256[:16]
082af49d58c386f5 there, master's own being 52461de728c5bb4c.

Six mutations, each anchored with count(anchor) == 1, restored from the string read
at start and re-hashed identical afterwards (082af49d58c386f5 → same). The first
three are the ones this body reports, re-run rather than taken on trust; the last
three are mine, aimed at the ways a -z parser can silently stop being one — the
field offset, the column that decides whether an entry owns a second field, and the
separator:

mutation caught by failed / passed
(body's) the v1 short format, paths as git quotes them 12 tests 12 / 17
(body's) the path is stripped test_a_modification_at_a_quoted_path_is_recoverable[ leading.txt] 1 / 28
(body's) every entry consumes a second field test_every_entry_is_walked_when_one_of_them_is_quoted (+1) 2 / 27
mine: the field offset is off by one (line[2:]) 12 tests 12 / 17
mine: the origin field decided on the worktree column (entry[1]) test_a_rename_is_unique_and_its_origin_is_not_walked 1 / 28
mine: the wrong separator (split("\n")) 23 tests 23 / 6

The third of mine is the one I expected to be weakest and it is the strongest: a
parser that keeps the quoting but splits on the wrong separator cannot pass this
file. And entry[1] in ("R","C") — a one-character mutation of the clause the body
singles out as the fail-open direction — is caught by exactly the rename test it
should be. Baselines, unmutated: this tree 29 passed, master 22 passed in
tests/test_recover_worktree.py.

Combined with what earlier cycles measured on the same PR (12 cells at the criterion,
then the 12-cell tier interaction on top of #1285 showing the quoted-path release
needs this PR as well as that one), I have no objection.

No objection. LGTM.

@argszero
argszero merged commit e495561 into master Sep 16, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants