From e2f3702684d9080bdfcf890d07270462fcf1b66a Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Fri, 25 Sep 2026 09:51:43 +0000 Subject: [PATCH] docs(storage): name the checks that answer recurring review findings (#3374) Comment-only. The review bot re-derives findings from the code on every push, so put the refuting fact next to each flagged line: the pristine check's hidden-path coverage, sparse checkouts being skip-worktree, the lock-safe unregister-then-delete order, the hook parity test for device names, and why the stores/ lstat is not a race guard. Co-Authored-By: Claude Opus 5.5 (1M context) --- gitnexus/src/cli/clean.ts | 7 ++++++- gitnexus/src/core/shared-store-analyze.ts | 2 ++ gitnexus/src/storage/git.ts | 2 ++ gitnexus/src/storage/storage-slot.ts | 3 ++- 4 files changed, 12 insertions(+), 2 deletions(-) diff --git a/gitnexus/src/cli/clean.ts b/gitnexus/src/cli/clean.ts index b39d5ebbe..1ac98cdf4 100644 --- a/gitnexus/src/cli/clean.ts +++ b/gitnexus/src/cli/clean.ts @@ -176,7 +176,9 @@ const collectSharedStores = async (force: boolean): Promise => { }); // Stray files (`.DS_Store`) are not stores, and a symlink is not followed: // reclaim deletes under the root it is given. A store another collector - // removed meanwhile is simply gone. + // removed meanwhile is simply gone. The lstat skips a stray link; it is not + // a race guard, since stores/ belongs to the user running clean and anyone + // able to swap an entry there can already delete the store itself. const roots: string[] = []; for (const name of names) { const root = path.join(storesDir, name); @@ -388,6 +390,9 @@ export const cleanCommand = async (options?: { for (const entry of entries) { try { const storagePath = await requireDeletableStoragePath(entry); + // A shared slot is unregistered before it is deleted: its lock file + // lives inside it, so it must go last. A failed delete throws with the + // `clean --gc --force` recovery (shared-store-clean.test.ts). await removeCheckoutStorage(storagePath, () => unregisterRepo(entry.path), entry.path); console.log(t('clean.deletedRepo', { name: entry.name, storagePath })); reportReclaim(await reclaimAfterSlotRemoval(storagePath)); diff --git a/gitnexus/src/core/shared-store-analyze.ts b/gitnexus/src/core/shared-store-analyze.ts index fefa62afa..f51f58785 100644 --- a/gitnexus/src/core/shared-store-analyze.ts +++ b/gitnexus/src/core/shared-store-analyze.ts @@ -391,6 +391,8 @@ export const publishSharedGraph = async ( // commit graph never changes, so a shared copy would stay short for good. !meta.embeddingCheckpoint && // A sparse or partial checkout builds a graph missing the files it hides. + // Every sparse mode marks those entries skip-worktree, which this rejects + // (git-utils.test.ts covers no-cone, cone and sparse-index checkouts). isWorkingTreePristine(repoPath); // Every pointer change and the reclaim that follows run under one publish // lock, so a concurrent reclaim never sees a half-recorded reference. diff --git a/gitnexus/src/storage/git.ts b/gitnexus/src/storage/git.ts index 945439fcc..0d0e138e4 100644 --- a/gitnexus/src/storage/git.ts +++ b/gitnexus/src/storage/git.ts @@ -129,6 +129,8 @@ export const listWorkingTreeDirtyPaths = (repoPath: string): string[] | null => * git failure. */ export const isWorkingTreePristine = (repoPath: string): boolean => { + // Includes every skip-worktree and assume-unchanged path (listHiddenIndexPaths, + // `git ls-files -v`), so the `--stage` pass below only has to find gitlinks. if (listWorkingTreeDirtyPaths(repoPath)?.length !== 0) return false; try { const out = execFileSync('git', ['ls-files', '--stage', '-z', '--'], { diff --git a/gitnexus/src/storage/storage-slot.ts b/gitnexus/src/storage/storage-slot.ts index 5bc8640ce..b8b0da4a2 100644 --- a/gitnexus/src/storage/storage-slot.ts +++ b/gitnexus/src/storage/storage-slot.ts @@ -31,7 +31,8 @@ export const sanitizeSlotBasename = ( const candidate = sanitized.slice(0, end) || 'repository'; // Exact device names were always prefixed. Windows also reserves them with // an extension (`CON.txt`); apply that only there, so existing POSIX slot - // names stay stable. + // names stay stable. All four registry-query.cjs hook copies mirror this; + // hooks-shared-store.test.ts checks hook-vs-TS parity on both platforms. const reserved = platform === 'win32' ? /^(con|prn|aux|nul|com[1-9]|lpt[1-9])(\..*)?$/i