Skip to content

Commit ba7b046

Browse files
committed
fix(web/shell): #107 review — a case-only rename on a folder that ignores case is refused whole, never a deletion
The bug. A folder on macOS or Windows (the usual File System Access folder) does not tell upper and lower case apart, nor an accent written as one character from one written as a letter and a combining mark, and the base answers for A.txt with what it has at a.txt. The overlay keys paths by their spelling, so `mv a.txt A.txt` staged "A.txt, the bytes the base already has there" (judged no change) and "a.txt removed", and the change set was only `delete a.txt`. The visitor's terminal deleted the file straight from the folder; an agent run sent the deletion to the review. The commit layers' case checks could not see it: the write they compare with had already gone. Reproduced through the real engine on a case-insensitive workspace before the fix: `mv a.txt A.txt` (file gone), `mv src/a.txt SRC/a.txt`, `mv a.txt t.txt; mv t.txt A.txt`, `rm a.txt; echo new > A.txt`, `rm -r src; echo n > SRC/new.txt`, NFD to NFC (all lose data); `mv d D` of a folder (the engine sees D as there and moves d into it: applied 3 of 4, as d/d/f.txt); `cp a.txt A.txt` (nothing, status 0). The fix (overlay-fs.js, spellingClash, asked by changes() before the set is worked out). The spellings a command used (what it staged, what the base answered for, and every folder above them) are grouped by case- and accent-folded text; only a group of two or more spellings, one of them changed by the command, is looked at, so a command without such a pair asks the base nothing more. For those the base decides: each spelling is resolved name by name against its folder's listing, and two spellings that are one entry there (or one that the base answers for and that cannot be matched to exactly one listed name) refuse the whole set as `incomplete`, with both names and how to do it instead (two commands). A folder that tells case apart keeps two names as two files, and its case-only rename stays a write and a delete for the commit to judge. One spelling of an entry is not refused: `echo x > A.txt` changes a.txt and `rm A.txt` removes it, as that disk does. Refused although a disk would have managed: reading under one spelling and changing under another in one command (`cat A.txt; rm a.txt`). Tests. shell-engine-overlay.test.js, over a base that ignores case and accent form: mv a.txt A.txt is refused and names both; eight ways of reaching one entry by two spellings are refused and the disk is unchanged (one of them names the folder two levels up, which only the folders-above part of the check sees); a name the base answers for that matches no single listed name is refused ("may be"); and pins that must not move: one spelling per entry, reads under two spellings, no extra base call without a pair, and a case-sensitive base. shell-runtime-engine.test.js, the real engine core through the runtime and commit.js over the Map workspace with the new `ignoreCase` (test/helpers/shell-runtime-fakes.js): the terminal's mv a.txt A.txt and mv d D are refused and nothing moves, echo > A.txt lands in a.txt, the two-command rename works; the agent's review is never handed the deletion. The five tests aimed at the bug fail on 187fd21 (the change set was `delete /w/a.txt`; in the terminal the file was gone) and pass here; the pins pass on both. NOTES.md §5/§6/§8 and docs/WEB.md say what the engine now refuses and what it still does not emulate.
1 parent 6c57cef commit ba7b046

6 files changed

Lines changed: 302 additions & 9 deletions

File tree

‎docs/WEB.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,9 @@ and verification see it. It is refused *whole* when any change is: outside the w
236236
endings; over 300 files; a file with unsaved changes in the editor, or changed on disk since the command read it; a
237237
case-only rename. The model is told which, and that **none** of the command's changes were applied. The visitor's
238238
own terminal writes what its commands change straight to the folder (still whole-or-nothing, never recursive,
239-
never to a trash).
239+
never to a trash). On a folder that does not tell upper and lower case apart (macOS, Windows) the shell itself
240+
refuses a command that changed one file or folder under two spellings (`mv a.txt A.txt`), so in both a case-only
241+
rename is refused, never a deletion; it takes two commands (`mv a.txt t.txt`, then `mv t.txt A.txt`).
240242

241243
**What it is for.** Reading and reshaping files: `grep -rn TODO src | wc -l`, `find . -name '*.md' | xargs wc -w`,
242244
a `sed` over twelve files, `jq` on a JSON file, a batch rename, `diff a b`. Not for running the project's tests or

‎web/shell/engine/NOTES.md‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -536,8 +536,21 @@ system's (`grep -r`, `find`, `rm -rv`, `cp -rv`); `exec cmd` goes on with the sc
536536
* A change set cannot say "this path was a folder and is a file now" (or the reverse): writes are applied before
537537
deletions on purpose (`mv a b` writes b and then removes a, so a failure leaves both). Such a run is withheld as
538538
`incomplete` with the reason, and the next command can do it in two steps. The consistency suite counts these.
539-
* A case-only rename (`mv a.txt A.txt`) on a disk that ignores case is a write and a delete of "the same" file; only
540-
commit.js's case-fold check defends against it (the engine does not know what kind of disk it is on).
539+
* The overlay keys a path by its spelling; a folder on macOS or Windows (the usual File System Access folder) does not
540+
tell upper and lower case apart, nor an accent written as one character (NFC) from one written as a letter and a
541+
combining mark (NFD), and answers for `A.txt` with what it has at `a.txt`. Inside a run the engine does not emulate
542+
that disk: after `rm a.txt`, `cat A.txt` still finds the base's file. What it does is refuse, whole, a run that
543+
CHANGED an entry under one spelling while it saw or changed the same entry under another (`spellingClash`): the net
544+
difference of such a run is wrong (`mv a.txt A.txt` was "A.txt: the bytes the base already has there", which is no
545+
change, and "a.txt removed", and the set deleted the file; the commit's own case checks never saw a write to compare
546+
with). Only spellings that fold to the same text are looked at (a run without such a pair costs nothing more); for
547+
those the base decides, by its answer for the spelling and its folder's listing, so a folder that tells case apart
548+
keeps two names as two files and a case-only rename there is a write and a delete for the commit to judge. One
549+
spelling of an entry is not refused (`echo x > A.txt` changes `a.txt`, `rm A.txt` removes it, as that disk does).
550+
Refused although a disk would have managed: reading under one spelling and changing under another (`cat A.txt; rm
551+
a.txt`). Not seen: two spellings that JavaScript's case mapping does not fold together and a disk does (rare
552+
scripts). `mv d D` of a folder is, to the engine, a move of `d` INTO `D` (it sees `D` as there): refused the same way
553+
(before, it was applied in part, as `d/d`). A case-only rename is two commands: `mv a.txt t.txt`, then `mv t.txt A.txt`.
541554
* `sed -n 'w nodir/f'` does not report the missing folder (the engine swallows the failed write of `w`).
542555
* Functions, aliases and options do not persist; `expandAliases` makes a session's aliases work as variables only.
543556
* A redirection writes to the PATH when it ends, not to an open descriptor (the overlay has none): `{ rm -rf d; echo hi;
@@ -654,8 +667,8 @@ a script that ended with status 0 with its changes withheld ends with status 1.
654667
The change set is the NET difference: a rewrite with the same bytes, create-then-delete and an append of nothing are not
655668
changes; deletions below a removed folder are listed (`viaDir`), deepest first; `before` is read lazily and only for what
656669
stays a change, under the read caps (`null` beyond them). If the set cannot be represented (more than `entries`, a
657-
folder that became a file, a folder below a removed one that the host would not list, the host failing while it was
658-
worked out) `changes` is empty and `stats.withheld` is `'incomplete'` and the notes say why. Buffers are transferred, each once,
670+
folder that became a file, a folder below a removed one that the host would not list, one entry changed under two
671+
spellings on a folder that ignores case (§5), the host failing while it was worked out) `changes` is empty and `stats.withheld` is `'incomplete'` and the notes say why. Buffers are transferred, each once,
659672
whole (a view into a larger buffer is copied first). If posting `done` throws, a smaller `done` (how `error`) is sent.
660673

661674
## 7. Size and speed
@@ -679,7 +692,7 @@ whole (a view into a larger buffer is copied first). If posting `done` throws, a
679692
| File | Tests | What |
680693
| --- | --- | --- |
681694
| `shell-engine-patches.test.js` | 94 | the vendored file (size, sha256, integrity), imports, anchors counted independently of the script, the patched hash, `--check`, refusal of a wrong file, each patch's behaviour (LC-1 .. LC-71; LC-19 and LC-20 under three process zones; LC-29 in every place that makes an escape; LC-31 against bash's own answers; LC-32 .. LC-60 each against what GNU printed for the same script, recorded once; LC-61 .. LC-71 each by a case GNU answered, which is different from GNU again with the patch taken out (`loadEngineWithout`), and LC-70 by a time ratio) |
682-
| `shell-engine-overlay.test.js` | 66 | mounts and relocation (both directions of overlap, `/proc`, a folder at `/mnt`), layout, caps before reads and their boundaries, notes and their cap, denied and its de-duplication, writes that are refused by the shell and writes a file system would refuse, the content write of a redirection after its opening, tombstones, opaque folders, staged files below a removed or moved folder, net change set and its order, a name with a lone surrogate, memory areas and their byte accounting, identity, devices, bytes that are not UTF-8, a base that fails |
695+
| `shell-engine-overlay.test.js` | 71 | mounts and relocation (both directions of overlap, `/proc`, a folder at `/mnt`), layout, caps before reads and their boundaries, notes and their cap, denied and its de-duplication, writes that are refused by the shell and writes a file system would refuse, the content write of a redirection after its opening, tombstones, opaque folders, staged files below a removed or moved folder, net change set and its order, a name with a lone surrogate, memory areas and their byte accounting, identity, devices, bytes that are not UTF-8, a base that fails, a base that ignores case and accent form (one entry under two spellings: eight ways refused, what still works, what costs nothing more) and one that does not |
683696
| `shell-engine-core.test.js` | 59 | handshake, program list pinned by name, streaming, decoding, limits table (every number), limit/deadline/abort/error classification and wording, exit-path flushes, never rejects, seven redirections whose target the command removes, late answers, env/cwd round trip, `/tmp`, relocation, change-set hand-over, caps from the host, `init.commands`, `tmpFiles` and `tmpEntries`, the zone of the process, names that are not UTF-8 (and bytes that are UTF-8: `dé` made with printf), a working directory that is canonical, `du` and `cp -u` at the traversal limit (the limit exactly, and one over), the stand-in engine contract |
684697
| `shell-engine-compat.test.js` | 94 | every normaliser case by case (including what is NOT dropped), wrappers through the engine, `od`, `cmp`, `du`, the sed scanner and its `-f` files, `cp`/`mv` onto `name/`, same file, `-T`, `-rv`, `mkdir -pv`, `touch -r -d -t`, `head`/`tail` long forms, `sort -k` (`parseSortKey`, the 72-spec grid), `xargs` (options, input, batches by size, `-s` at 1310/1311 items, 4-byte characters, `-t` order, NUL and `-E` warnings, status codes, `-p`/`-o`, the quote), the sweep of every option that needs a value, the `-z` family (`zero.plan`, refusals, `tail -z` headers, `grep -Z -z`, `sed -z`), `tr.js`, `datestr.parse` and `diff.parseOptions`/`shiftBoundaries`, `ls -v` and `sort -V` (`versionCompare`: six recorded sets and every pair antisymmetric), `ls -I --hide -X -w -n`, `seq -f`, `touch`/`tee` outside the folders, awk operands repeated, the exported lists pinned; run under three process zones. Expected text of the GNU-recorded tests: GNU as docker ran it, once |
685698
| `shell-engine-cpmv.test.js` | 4 | 1,778 `cp`/`mv` command lines (every source shape against every destination shape, a seeded sample of the options, and the same file and the repeated source under `-u`/`-n`) against GNU coreutils 8.32, compared on status, what -v printed, the listing, what each changed file holds and what was said; 24 listed with a reason, the list may only shrink (`web/test/fixtures/shell/cpmv/`, recorded once) |

‎web/shell/engine/overlay-fs.js‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1107,6 +1107,12 @@ function createOverlay(o) {
11071107
*/
11081108
async function changes() {
11091109
try {
1110+
const clash = await spellingClash();
1111+
if (clash) {
1112+
stats.stagedBytes = stagedBytes;
1113+
stats.stagedEntries = entries.size;
1114+
return { changes: [], refused: clash };
1115+
}
11101116
return await workOutChanges();
11111117
} catch (e) {
11121118
const why = String(e && /** @type {any} */ (e).message || e).replace(/^[A-Z]+: /, '').slice(0, 160);
@@ -1116,6 +1122,84 @@ function createOverlay(o) {
11161122
}
11171123
}
11181124

1125+
/**
1126+
* One entry under two spellings. A folder on macOS or Windows (the usual File System Access folder) does not tell
1127+
* upper and lower case apart, nor two ways of writing an accent (NFC and NFD), and the base answers for `A.txt` with
1128+
* what it has at `a.txt`. This overlay keys paths by their spelling, so it takes them for two files, and the net
1129+
* difference comes out wrong: `mv a.txt A.txt` stages "A.txt, the bytes the base already has there" (no change) and
1130+
* "a.txt removed" (a change), and the set deletes the file. The commit's own case checks cannot see that: only the
1131+
* delete is left in the set. So a command that changed an entry under one spelling while it saw or changed the same
1132+
* entry under another has its whole set refused, with both names.
1133+
*
1134+
* Only spellings that fold to the same text are looked at, so a command without such a pair asks the base nothing
1135+
* more. For those the base decides: a spelling is another name of an entry when the base answers for it but its
1136+
* folder's listing has a different name for it. A folder that tells case apart has no answer for the other
1137+
* spelling, and two names there stay two files (a case-only rename is then a write and a delete, which the commit
1138+
* judges). One spelling of an entry on such a folder is not refused: `echo x > A.txt` changes `a.txt`, as it would
1139+
* on that disk. Reading an entry under one spelling and changing it under another is refused although a disk would
1140+
* have done something sensible with it: it cannot be told apart from a rename by what is left at the end.
1141+
* @returns {Promise<string | null>} why the set is refused, or null
1142+
*/
1143+
async function spellingClash() {
1144+
const fold = (/** @type {string} */ s) => Array.from(s.normalize('NFC'), (ch) => ch.toUpperCase().toLowerCase()).join('').normalize('NFC');
1145+
/** @type {Map<string, Set<string>>} folded text -> the spellings this command used (a path and every folder above it) */
1146+
const groups = new Map();
1147+
const add = (/** @type {string} */ p) => {
1148+
const r = mountOf(p);
1149+
if (!r) { return; }
1150+
for (let q = p; q !== r.m.at && q !== '/'; q = parentOf(q)) {
1151+
const k = fold(q);
1152+
let g = groups.get(k);
1153+
if (!g) { g = new Set(); groups.set(k, g); }
1154+
if (g.has(q)) { break; } // and so is every folder above it
1155+
g.add(q);
1156+
}
1157+
};
1158+
for (const p of entries.keys()) { add(p); }
1159+
for (const [p, s] of baseStats) { if (s) { add(p); } }
1160+
1161+
/**
1162+
* The path the base has for p: each name as its folder lists it. A name the base has nothing for is new from
1163+
* there down and is kept as written. null: the base answers for a name that is not in its folder's listing, and
1164+
* not exactly one listed name folds to it, so which entry it is cannot be told.
1165+
* @param {string} p @returns {Promise<string | null>}
1166+
*/
1167+
async function realPath(p) {
1168+
const r = /** @type {{ m: typeof mounts[number], host: string }} */ (mountOf(p));
1169+
const names = p.slice(r.m.at.length + 1).split('/');
1170+
let real = r.m.at;
1171+
for (let i = 0; i < names.length; i++) {
1172+
const spelled = r.m.at + '/' + names.slice(0, i + 1).join('/');
1173+
if (!(await baseStat(spelled, toHost(spelled)))) { return [real, ...names.slice(i)].join('/'); }
1174+
const listed = (await baseList(real, toHost(real))).map(([n]) => n);
1175+
if (listed.includes(names[i])) { real = join(real, names[i]); continue; }
1176+
const hits = listed.filter((n) => fold(n) === fold(names[i]));
1177+
if (hits.length !== 1) { return null; }
1178+
real = join(real, hits[0]);
1179+
}
1180+
return real;
1181+
}
1182+
1183+
for (const g of groups.values()) {
1184+
if (g.size < 2) { continue; }
1185+
const spellings = [...g].sort();
1186+
if (!spellings.some((q) => entries.has(q))) { continue; } // only read under both: nothing to get wrong
1187+
/** @type {Map<string, string>} the base's path -> the first spelling that is it */
1188+
const seen = new Map();
1189+
for (const q of spellings) {
1190+
const real = await realPath(q);
1191+
const other = real === null ? spellings.find((o) => o !== q) : seen.get(real);
1192+
if (other !== undefined) {
1193+
// two names that look the same on screen (é as one character, and as e and a combining accent) are said to be that
1194+
const looks = other.normalize('NFC') === q.normalize('NFC') ? ' (they look the same: an accent is written two ways)' : '';
1195+
return `${displayText(other)} and ${displayText(q)}${looks} ${real === null ? 'may be' : 'are'} one name here: this folder does not tell upper and lower case, or two ways of writing an accent, apart, and the command used both. To change only the case of a name, move it to another name in one command and to the new name in the next`;
1196+
}
1197+
seen.set(/** @type {string} */ (real), q);
1198+
}
1199+
}
1200+
return null;
1201+
}
1202+
11191203
async function workOutChanges() {
11201204
/** @type {Change[]} */
11211205
const mk = [];

‎web/test/helpers/shell-runtime-fakes.js‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,8 @@
55
* goes through deps.setTimeout)
66
* createWorkspace() a Map-backed vscode.workspace.fs with the error shapes of the real one (FileSystemError
77
* with `code` 'FileNotFound' and `name` 'EntryNotFound (FileSystemError)'), failure and
8-
* hang injection, a call log, and workspaceFolders with the change event
8+
* hang injection, a call log, and workspaceFolders with the change event; with `ignoreCase`
9+
* it is a disk that ignores case and the form of an accent (macOS, Windows)
910
* createEngines() a factory of fake engine workers that speak README section 4: they answer `init`, run a
1011
* script the test writes with the engine-side helpers (fs read, out, done), and can
1112
* crash, hang, or keep talking after they were terminated
@@ -88,7 +89,11 @@ const te = new TextEncoder();
8889
const td = new TextDecoder();
8990

9091
/**
91-
* @param {{ folders?: Array<{ name: string, uri: string }> }} [o]
92+
* @param {{ folders?: Array<{ name: string, uri: string }>, ignoreCase?: boolean }} [o]
93+
* `ignoreCase`: the disk of a folder on macOS or Windows (the usual File System Access folder). A path finds the entry
94+
* whatever the case of its letters and the form of its accents, and an entry keeps the name it was made with: a
95+
* write to `A.txt` where `a.txt` is changes `a.txt`. The helpers below that read the workspace for a test (`text`,
96+
* `exists`, `list` ...) take a path as it is spelled, so a test sees the names the disk really has.
9297
*/
9398
function createWorkspace(o) {
9499
/** @type {Map<string, { type: number, data: Uint8Array, mtime: number }>} */ const nodes = new Map();
@@ -102,6 +107,19 @@ function createWorkspace(o) {
102107
const pre = k.endsWith('/') ? k : k + '/';
103108
return Array.from(nodes.keys()).filter((n) => n.startsWith(pre) && !n.slice(pre.length).includes('/'));
104109
};
110+
const fold = (/** @type {string} */ s) => s.normalize('NFC').toLowerCase();
111+
/** The key of what the disk finds for `k`: itself, or on a disk that ignores case each name matched to the one that is there. */
112+
function named(/** @type {string} */ k) {
113+
if (!(o && o.ignoreCase) || nodes.has(k)) { return k; }
114+
const scheme = k.slice(0, k.indexOf(':') + 1);
115+
let at = scheme;
116+
for (const name of k.slice(scheme.length).split('/').filter(Boolean)) {
117+
const want = at + '/' + name;
118+
const hit = nodes.has(want) ? want : childrenOf(at === scheme ? scheme + '/' : at).find((c) => fold(c.slice(c.lastIndexOf('/') + 1)) === fold(name));
119+
at = hit || want;
120+
}
121+
return at === scheme ? scheme + '/' : at;
122+
}
105123
/** Is a file on the way to `k`? (what a real disk answers ENOTDIR for) */
106124
function underAFile(/** @type {string} */ k) {
107125
const floor = k.indexOf(':') + 1;
@@ -121,7 +139,7 @@ function createWorkspace(o) {
121139
if ('error' in r) { throw r.error; } // whatever it is: `undefined`, a string and a number are thrown too
122140
}
123141
}
124-
return k;
142+
return named(k);
125143
}
126144
function mkdirp(/** @type {string} */ k) {
127145
const scheme = k.slice(0, k.indexOf(':') + 1);

0 commit comments

Comments
 (0)