diff --git a/DESIGN.md b/DESIGN.md index f759eb5..d337e7d 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -1065,3 +1065,20 @@ on v1.10.6 and about one in four alone, because the server sometimes never ended The delay is measured, not derived: the cause is inside Bun. The looped test is what will notice if a Bun upgrade changes this. + +## 26. Round 17: "1 untracked" with nothing in Changes + +The status bar counted untracked files from `git status`, while Changes leaves out `.DS_Store` and +everything under a heavy folder, so a Mac folder could show `main* 1 untracked` over an empty list. + +| Finding | Fix | +| --- | --- | +| An untracked `.DS_Store` counted, made the branch dirty and could colour its folder, though the tree and Changes both hide it | `compute()` drops those `??` records. A tracked `.DS_Store` is a real change and stays | +| An unignored heavy folder (`target/`) counted with no row in Changes, but the tree does show it, so its files must stay untracked there (badge, gutter) | The record stays and is named in `GitStatus.unlisted`, which the client leaves out of the count | +| Status folds an untracked folder into one `dir/` record, so a folder holding only a `.DS_Store` or a `node_modules` counted too | `markUnlistedDirs` asks `ls-files` about those folders, only when there are some. It runs on every status, so `-x` skips heavy folders and `--directory` stops at the first real file: 9 ms instead of 54 ms on a 50,000-file untracked folder | +| The heavy-name test ran on the whole path and on the last segment: a plain file named `target` or `venv` was missing from Changes and search | `isHeavyFile` (`index.ts`) tests folders only, plus `.DS_Store`. Status tests the root-relative path, so a viewed folder inside `target/` is not affected by the name above it | +| A folder whose name is not UTF-8 decodes lossily and would match nothing as a pathspec, reading as empty | Such names are not checked and keep counting | + +`FileIndex.apply` keeps its own test: its watcher paths can be the heavy folder itself, which must +not restart a build. A nested repository still counts as one untracked folder with no row in +Changes: status cannot tell it from a plain folder, and it does exist. diff --git a/src/client/git.ts b/src/client/git.ts index c53bc47..75e6ea6 100644 --- a/src/client/git.ts +++ b/src/client/git.ts @@ -14,6 +14,8 @@ export interface GitStatus { head: { hash: string; subject: string; author: string; time: number } | null; /** The viewed folder is itself git-ignored: everything in it is ignored (git lists nothing inside). */ rootIgnored?: boolean; + /** `files` keys with no row in Changes: shown as untracked in the tree, not counted. */ + unlisted?: string[]; } /** Display status, VS Code style: Modified, Added, Untracked, Deleted, Renamed, Conflict, Ignored. */ @@ -69,13 +71,14 @@ export class GitState { this.prefixes = new Map(); this.dirs.clear(); this.counts = { changed: 0, staged: 0, untracked: 0, conflicts: 0 }; + const unlisted = new Set(s.unlisted); for (const [raw, xy] of Object.entries(s.files ?? {})) { const g = describe(xy); const p = nfc(raw); if (p.endsWith("/")) this.prefixes.set(p.slice(0, -1), g); else this.files.set(p, g); if (g.letter === "I") continue; - if (g.letter === "U") this.counts.untracked++; + if (g.letter === "U") this.counts.untracked += unlisted.has(raw) ? 0 : 1; else if (g.letter === "C") this.counts.conflicts++; else this.counts.changed++; if (xy[0] !== "." && xy[0] !== "?" && !CONFLICT.has(xy)) this.counts.staged++; diff --git a/src/server/git.ts b/src/server/git.ts index dce3ad0..c15abc5 100644 --- a/src/server/git.ts +++ b/src/server/git.ts @@ -1,5 +1,5 @@ import { watch, type FSWatcher } from "node:fs"; -import { HEAVY_DIRS } from "./index.ts"; +import { HEAVY_DIRS, isHeavyFile } from "./index.ts"; import { existsSync, lstatSync, readFileSync } from "node:fs"; import path from "node:path"; import { looksBinary, openRegular } from "./files.ts"; @@ -40,6 +40,8 @@ export interface GitStatus { files: Record; /** The viewed folder is itself git-ignored (git lists only the folder, so `files` has no "!!" entries). */ rootIgnored: boolean; + /** `files` keys that are really untracked but have no row in Changes: the tree shows them, the count does not. */ + unlisted: string[]; head: { hash: string; subject: string; author: string; time: number } | null; } @@ -80,7 +82,6 @@ export interface Changes { subjects: string[]; } -/** Most entries a Changes listing carries (tracked diffs and untracked files each). */ export interface HistoryEntry { oid: string; /** First parent; null for a root commit. */ @@ -96,7 +97,10 @@ export interface HistoryEntry { status: "M" | "A" | "D" | "R" | "C" | "T"; } +/** Most entries a Changes listing carries (tracked diffs and untracked files each). */ export const MAX_CHANGES = 1000; +/** Above this many untracked folders in one status, they are counted without checking what they hold. */ +const MAX_UNTRACKED_DIR_CHECKS = 1000; /** Commits listed by File History at most. */ export const MAX_HISTORY = 500; /** Untracked files whose lines are counted (the rest show no counts). */ @@ -270,7 +274,7 @@ export class Git { this.fresh(); const r = await this.git(["ls-files", "-z", "--cached", "--others", "--exclude-standard", "--deduplicate", "--", "."]); if (r.code !== 0) return null; - return new TextDecoder().decode(r.out).split("\0").filter((f) => f && !f.split("/").some((seg) => HEAVY_DIRS.has(seg))); + return new TextDecoder().decode(r.out).split("\0").filter((f) => f && !isHeavyFile(f)); } /** Current status; concurrent callers share one `git status` run. */ @@ -294,7 +298,7 @@ export class Git { ]); const s: GitStatus = { repo: true, branch: null, detached: false, oid: null, upstream: null, ahead: 0, behind: 0, - state: this.opState(), stash: 0, files: {}, rootIgnored: false, head: null, + state: this.opState(), stash: 0, files: {}, rootIgnored: false, unlisted: [], head: null, }; if (stash.code === 0) s.stash = Number(new TextDecoder().decode(stash.out).trim()) || 0; if (head.code === 0) { @@ -312,7 +316,13 @@ export class Git { } if (!p.startsWith(this.prefix)) return; const rel = p.slice(this.prefix.length); - if (rel) s.files[rel] = xy; + if (!rel) return; + if (xy === "??" && isHeavyFile(rel)) { + // The tree hides .DS_Store, so it must not colour a folder either; heavy folders it shows. + if (rel === ".DS_Store" || rel.endsWith("/.DS_Store")) return; + s.unlisted.push(rel); + } + s.files[rel] = xy; }; for (let i = 0; i < recs.length; i++) { const r = recs[i]!; @@ -343,9 +353,31 @@ export class Git { else if (kind === "?") add(r.slice(2), "??"); else if (kind === "!") add(r.slice(2), "!!"); } + await this.markUnlistedDirs(s); return s; } + /** + * Status folds an untracked folder into one "dir/" record, even when all it holds is what Changes + * leaves out (a lone .DS_Store, a node_modules). Such a record would count with nothing to list. + */ + private async markUnlistedDirs(s: GitStatus) { + // A name that is not UTF-8 does not survive decoding: as a pathspec it would match nothing. + const dirs = Object.keys(s.files).filter((p) => p.endsWith("/") && s.files[p] === "??" && !s.unlisted.includes(p) && !p.includes("\uFFFD")); + if (!dirs.length || dirs.length > MAX_UNTRACKED_DIR_CHECKS) return; + // Runs on every status: -x never enters a heavy folder, --directory stops at the first real file. + // A name in the viewed folder's own path stays out: -x tests the folders above the pathspec too. + const above = this.prefix.split("/"); + const skip = [...HEAVY_DIRS].filter((name) => !above.includes(name)).flatMap((name) => ["-x", name === ".DS_Store" ? name : `${name}/`]); + const r = await this.git(["--literal-pathspecs", "ls-files", "-z", "--others", "--exclude-standard", "--directory", "--no-empty-directory", ...skip, "--", ...dirs]); + if (r.code !== 0) return; + const kept = new Set(); + for (const f of new TextDecoder().decode(r.out).split("\0")) { + for (let i = f.indexOf("/"); i >= 0; i = f.indexOf("/", i + 1)) kept.add(f.slice(0, i + 1)); + } + for (const d of dirs) if (!kept.has(d)) s.unlisted.push(d); + } + private opState(): string | null { // Operation markers are per worktree: only this worktree's git dir, never the common dir // (a merge in the main checkout must not show as "merging" in a linked worktree). @@ -556,7 +588,7 @@ export class Git { const all = new TextDecoder() .decode(r.out) .split("\0") - .filter((f) => f && !f.endsWith("/") && !f.split("/").some((seg) => HEAVY_DIRS.has(seg))); + .filter((f) => f && !f.endsWith("/") && !isHeavyFile(f)); const paths = all.slice(0, MAX_CHANGES); const files: ChangeEntry[] = paths.map((p) => ({ path: p, status: "?", added: null, removed: null, binary: false })); // A few files at a time through one 64 KiB buffer each: never 200 MiB of buffers per request. diff --git a/src/server/index.ts b/src/server/index.ts index b83cfb0..298931d 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -8,6 +8,15 @@ export const HEAVY_DIRS = new Set([ ".pytest_cache", ".gradle", ".terraform", ".DS_Store", "target", ".zig-cache", "zig-out", ]); +/** + * A file left out of listings: anything under a heavy folder, and .DS_Store itself. A plain file + * that only shares a folder's name (`target`, `venv`) is listed. "dir/" is a folder. + */ +export function isHeavyFile(p: string): boolean { + const segs = p.split("/"); + return segs.pop() === ".DS_Store" || segs.some((seg) => HEAVY_DIRS.has(seg)); +} + const MAX_FILES = 200_000; /** diff --git a/test/client.git.test.ts b/test/client.git.test.ts index b10898d..b990c11 100644 --- a/test/client.git.test.ts +++ b/test/client.git.test.ts @@ -27,6 +27,16 @@ describe("GitState", () => { expect(g.counts).toEqual({ changed: 3, staged: 2, untracked: 1, conflicts: 1 }); }); + test("unlisted records stay untracked in the tree but do not count", () => { + const g = new GitState(); + g.set({ ...st({ "target/": "??", "lib/pkg/": "??", "a.txt": "??" }), unlisted: ["target/", "lib/pkg/"] }); + expect(g.counts.untracked).toBe(1); + expect(g.file("target/foo.rs")).toMatchObject({ letter: "U" }); + expect(g.dir("lib")).toBe("U"); + g.set({ ...st({ "target/": "??" }), unlisted: ["target/"] }); + expect(g.counts).toEqual({ changed: 0, staged: 0, untracked: 0, conflicts: 0 }); + }); + test("directories take the strongest status inside; prefixes cover descendants", () => { const g = new GitState(); g.set(st({ "src/a/x.ts": ".M", "src/a/y.ts": "??", "src/b/z.ts": "UU", "build/": "!!", "tmp/": "??" })); diff --git a/test/servergit.changes.test.ts b/test/servergit.changes.test.ts index e81df2d..e71b046 100644 --- a/test/servergit.changes.test.ts +++ b/test/servergit.changes.test.ts @@ -1,5 +1,6 @@ import { afterAll, beforeAll, expect, test } from "bun:test"; import { mkdtemp, mkdir, realpath, rm, writeFile, rename } from "node:fs/promises"; +import { mkdirSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; import { Git, isOid, MAX_CHANGES } from "../src/server/git.ts"; @@ -286,3 +287,110 @@ test("the list is capped and reports truncation; untracked symlinks are not read const lc = await lg.changes("head"); expect(lc.files).toEqual([{ path: "escape", status: "?", added: null, removed: null, binary: false }]); }); + +test("status counts as untracked exactly what Changes lists", async () => { + const r = path.join(dir, "hidden-untracked"); + await mkdir(path.join(r, "sub", "inner"), { recursive: true }); + sh(["init", "-q", "-b", "main"], r); + await writeFile(path.join(r, ".gitignore"), "ignored/\n"); + await writeFile(path.join(r, "sub", "s.txt"), "s\n"); + await mkdir(path.join(r, "lib", "target"), { recursive: true }); + await writeFile(path.join(r, "lib", "target", "t.txt"), "t\n"); + sh(["add", "."], r); + sh(["commit", "-qm", "init"], r); + + const put = async (rel: string) => { + await mkdir(path.dirname(path.join(r, rel)), { recursive: true }); + await writeFile(path.join(r, rel), "x\n"); + }; + // Metadata is gone altogether; folders that hold nothing listable stay for the tree, uncounted. + for (const f of [".DS_Store", "sub/.DS_Store", "only/.DS_Store", "only/deep/.DS_Store", "target/out.o", "pkg/node_modules/m/i.js", "ignored/x"]) await put(f); + let g = await git(r); + expect((await g.changes("head")).files).toEqual([]); + let s = await g.status(); + expect(s.files).toEqual({ "ignored/": "!!", "only/": "??", "pkg/": "??", "target/": "??" }); + expect(s.unlisted.sort()).toEqual(["only/", "pkg/", "target/"]); + + // Real files around them count, a folder holding one is one record, special characters are literal. + await put("real.txt"); + await put("mixed/.DS_Store"); + await put("mixed/new file.txt"); + await put("glob[1]*/keep.txt"); + g = await git(r); + expect((await g.changes("head")).files.map((f) => f.path).sort()).toEqual(["glob[1]*/keep.txt", "mixed/new file.txt", "real.txt"]); + s = await g.status(); + expect(s.files).toMatchObject({ "glob[1]*/": "??", "mixed/": "??", "real.txt": "??" }); + expect(s.unlisted.sort()).toEqual(["only/", "pkg/", "target/"]); + + // A plain file that only shares a heavy folder's name is an ordinary file, alone or in a new folder. + await put("venv"); + await put("plain/target"); + g = await git(r); + expect((await g.changes("head")).files.map((f) => f.path)).toEqual(expect.arrayContaining(["venv", "plain/target"])); + s = await g.status(); + expect(s.files).toMatchObject({ venv: "??", "plain/": "??" }); + expect(s.unlisted).not.toContain("venv"); + expect(s.unlisted).not.toContain("plain/"); + + // A nested repository really is there: it counts, though Changes has no row for it. + await put("nested/f.txt"); + sh(["init", "-q"], path.join(r, "nested")); + g = await git(r); + s = await g.status(); + expect(s.files["nested/"]).toBe("??"); + expect(s.unlisted).not.toContain("nested/"); + await rm(path.join(r, "nested"), { recursive: true }); + + // Viewing a subfolder: same rule, root-relative records. + await put("sub/only/.DS_Store"); + await put("sub/kept/k.txt"); + g = await git(path.join(r, "sub")); + s = await g.status(); + expect(s.files).toEqual({ "kept/": "??", "only/": "??" }); + expect(s.unlisted).toEqual(["only/"]); + + // Viewing a folder inside a heavy-named one: the name above the root does not matter. + await put("lib/target/new.txt"); + await put("lib/target/d/x.txt"); + g = await git(path.join(r, "lib", "target")); + expect((await g.changes("head")).files.map((f) => f.path).sort()).toEqual(["d/x.txt", "new.txt"]); + s = await g.status(); + expect(s.files).toEqual({ "d/": "??", "new.txt": "??" }); + expect(s.unlisted).toEqual([]); + + // A .DS_Store that is tracked is a real change and stays visible. + sh(["add", "-f", "sub/.DS_Store"], r); + sh(["commit", "-qm", "track it"], r); + await writeFile(path.join(r, "sub", ".DS_Store"), "changed\n"); + g = await git(path.join(r, "sub")); + expect((await g.status()).files[".DS_Store"]).toBe(".M"); +}); + +test("an untracked folder with a name that is not UTF-8 still counts (Linux)", async () => { + const r = path.join(dir, "latin1"); + await mkdir(r); + sh(["init", "-q", "-b", "main"], r); + const named = Buffer.concat([Buffer.from(r + "/caf"), Buffer.from([0xe9])]); + try { + mkdirSync(named); + } catch { + return; // APFS refuses such names + } + writeFileSync(Buffer.concat([named, Buffer.from("/f.txt")]), "x\n"); + const s = await (await git(r)).status(); + expect(Object.keys(s.files)).toEqual(["caf�/"]); + expect(s.unlisted).toEqual([]); +}); + +test("past the check limit, untracked folders are counted unchecked", async () => { + const r = path.join(dir, "many-dirs"); + await mkdir(r); + sh(["init", "-q", "-b", "main"], r); + for (let i = 0; i <= 1000; i++) { + await mkdir(path.join(r, `d${i}`)); + await writeFile(path.join(r, `d${i}`, ".DS_Store"), ""); + } + const s = await (await git(r)).status(); + expect(Object.keys(s.files)).toHaveLength(1001); + expect(s.unlisted).toEqual([]); +});