From 34e4d9a50324ce8b93516f5c45906374264ff6ca Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 22 Jul 2026 16:47:55 -0700 Subject: [PATCH] fix(worktrees): tighten include matching and scan bounds --- src/main/git/worktree-include-file.test.ts | 41 +++++++++++++ src/main/git/worktree-include-file.ts | 67 +++++++++++++--------- src/main/git/worktree-include-flow.test.ts | 5 +- src/main/git/worktree-include-glob.ts | 33 +++++++---- src/main/git/worktree-include-pattern.ts | 52 +++++++++++++---- src/main/ipc/worktree-apfs-clone.ts | 20 ++++--- src/main/ipc/worktree-symlinks.test.ts | 3 + 7 files changed, 162 insertions(+), 59 deletions(-) diff --git a/src/main/git/worktree-include-file.test.ts b/src/main/git/worktree-include-file.test.ts index c01a6c01b12..1e57e4fc427 100644 --- a/src/main/git/worktree-include-file.test.ts +++ b/src/main/git/worktree-include-file.test.ts @@ -89,6 +89,21 @@ describe('parseWorktreeIncludePatterns', () => { expect(glob.glob && matchesWorktreeIncludeGlob(glob.glob, longNonMatch)).toBe(false) }) + it('keeps recursive directory globs on segment boundaries', () => { + const leading = compileWorktreeIncludeGlob('**/.env') + const nested = compileWorktreeIncludeGlob('foo/**/bar') + const adjacent = compileWorktreeIncludeGlob('foo/**bar') + + expect(matchesWorktreeIncludeGlob(leading, '.env')).toBe(true) + expect(matchesWorktreeIncludeGlob(leading, 'nested/.env')).toBe(true) + expect(matchesWorktreeIncludeGlob(leading, 'my.env')).toBe(false) + expect(matchesWorktreeIncludeGlob(nested, 'foo/bar')).toBe(true) + expect(matchesWorktreeIncludeGlob(nested, 'foo/nested/bar')).toBe(true) + expect(matchesWorktreeIncludeGlob(nested, 'foo/xbar')).toBe(false) + expect(matchesWorktreeIncludeGlob(adjacent, 'foo/xbar')).toBe(true) + expect(matchesWorktreeIncludeGlob(adjacent, 'foo/x/bar')).toBe(false) + }) + it('matches literals and globs case-insensitively when the repository requires it', () => { const literal = parseWorktreeIncludePatterns('.env\n') const glob = parseWorktreeIncludePatterns('config/**/secret.json\n') @@ -304,6 +319,32 @@ describe('resolveWorktreeIncludePaths', () => { expect(targetedArgs).toContain(':(exclude,literal)node_modules') }) + it('does not expand a collapsed directory excluded by trailing negation', async () => { + writeInclude('*\n!node_modules/\n') + mockGit({ collapsedEntries: ['node_modules/', 'cache/'], ignored: ['cache'] }) + + await expect(resolveWorktreeIncludePaths(repo)).resolves.toEqual(['cache']) + const targetedArgs = gitExecFileAsyncMock.mock.calls.find( + ([args]) => args.includes('ls-files') && !args.includes('--directory') + )?.[0] + expect(targetedArgs).toContain(':(exclude,literal)node_modules') + }) + + it('scans an excluded directory when a later include can restore a descendant', async () => { + writeInclude('*\n!node_modules/\n.env\n') + mockGit({ + collapsedEntries: ['node_modules/'], + targetedEntries: ['node_modules/pkg/.env'], + ignored: ['node_modules/pkg/.env'] + }) + + await expect(resolveWorktreeIncludePaths(repo)).resolves.toEqual(['node_modules/pkg/.env']) + const targetedArgs = gitExecFileAsyncMock.mock.calls.find( + ([args]) => args.includes('ls-files') && !args.includes('--directory') + )?.[0] + expect(targetedArgs).not.toContain(':(exclude,literal)node_modules') + }) + it('preserves nested matches when covered-directory exclusions exceed the command budget', async () => { writeInclude('covered-*/\n.env\n') const coveredDirectories = Array.from( diff --git a/src/main/git/worktree-include-file.ts b/src/main/git/worktree-include-file.ts index b1138f7a749..dc83593a444 100644 --- a/src/main/git/worktree-include-file.ts +++ b/src/main/git/worktree-include-file.ts @@ -11,7 +11,7 @@ import { type GitignoredEntry } from './worktree-include-git-enumeration' import { - isIncludedByWorktreePatterns, + getWorktreeIncludePatternDecision, parseWorktreeIncludePatterns, WORKTREE_INCLUDE_MATCH_STEP_BUDGET, type WorktreeIncludePattern @@ -121,6 +121,7 @@ export async function resolveWorktreeIncludePaths( const ignoreCase = await getWorktreeIncludeIgnoreCase(repoPath, options, ignoreCaseTimeout) const candidates = new Map() + const excludedCoveredDirectories = new Set() let candidateBytes = 0 const matchBudget = { remaining: WORKTREE_INCLUDE_MATCH_STEP_BUDGET } const pruneCoveredCandidateDescendants = (): void => { @@ -138,33 +139,39 @@ export async function resolveWorktreeIncludePaths( } } const addCandidate = (entry: GitignoredEntry): void => { - if ( - isSafeIncludeCandidate(entry.relativePath) && - isIncludedByWorktreePatterns( - patterns, - entry.relativePath, - entry.isDirectory, - matchBudget, - ignoreCase - ) - ) { - if (hasCandidateDirectoryAncestor(candidates, entry.relativePath)) { - return - } - if ( - !candidates.has(entry.relativePath) && - candidates.size >= WORKTREE_INCLUDE_MAX_CANDIDATES - ) { - throw new Error(`${WORKTREE_INCLUDE_FILE} matched too many paths`) - } - if (!candidates.has(entry.relativePath)) { - candidateBytes += Buffer.byteLength(entry.relativePath) + 1 - if (candidateBytes > WORKTREE_INCLUDE_MAX_CANDIDATE_BYTES) { - throw new Error(`${WORKTREE_INCLUDE_FILE} matched paths exceed its byte budget`) - } - } - candidates.set(entry.relativePath, entry) + if (!isSafeIncludeCandidate(entry.relativePath)) { + return } + const decision = getWorktreeIncludePatternDecision( + patterns, + entry.relativePath, + entry.isDirectory, + matchBudget, + ignoreCase + ) + if (!decision.included) { + if (entry.isDirectory && entry.coversDescendants && decision.excludesDescendants) { + excludedCoveredDirectories.add(entry.relativePath) + } + return + } + excludedCoveredDirectories.delete(entry.relativePath) + if (hasCandidateDirectoryAncestor(candidates, entry.relativePath)) { + return + } + if ( + !candidates.has(entry.relativePath) && + candidates.size >= WORKTREE_INCLUDE_MAX_CANDIDATES + ) { + throw new Error(`${WORKTREE_INCLUDE_FILE} matched too many paths`) + } + if (!candidates.has(entry.relativePath)) { + candidateBytes += Buffer.byteLength(entry.relativePath) + 1 + if (candidateBytes > WORKTREE_INCLUDE_MAX_CANDIDATE_BYTES) { + throw new Error(`${WORKTREE_INCLUDE_FILE} matched paths exceed its byte budget`) + } + } + candidates.set(entry.relativePath, entry) } // Why: anchored literals cannot be discovered more cheaply than a direct probe; @@ -273,6 +280,12 @@ export async function resolveWorktreeIncludePaths( const coveredDirectoryPathspecs = Array.from(candidates.values()) .filter((entry) => entry.isDirectory && entry.coversDescendants) .map((entry) => `:(exclude,literal)${entry.relativePath}`) + coveredDirectoryPathspecs.push( + ...Array.from( + excludedCoveredDirectories, + (relativePath) => `:(exclude,literal)${relativePath}` + ) + ) for (const pathspecs of chunkWorktreeIncludePathspecs( filePathspecs, coveredDirectoryPathspecs diff --git a/src/main/git/worktree-include-flow.test.ts b/src/main/git/worktree-include-flow.test.ts index b390ab33557..801c934ac5c 100644 --- a/src/main/git/worktree-include-flow.test.ts +++ b/src/main/git/worktree-include-flow.test.ts @@ -34,13 +34,14 @@ describe('worktree include creation flow', () => { it('copies only matched ignored paths and leaves ordinary removal clean', async () => { await git(['config', 'core.ignoreCase', 'true']) - await writeFile(join(primary, '.gitignore'), '.env\ncache/\nparent/\nmixed/local.txt\n') + await writeFile(join(primary, '.gitignore'), '.env\nmy.env\ncache/\nparent/\nmixed/local.txt\n') await writeFile( join(primary, '.worktreeinclude'), - '.env\ncache/\nbuild/\n/mixed/\ntracked.txt\n' + '**/.env\ncache/\nbuild/\n/mixed/\ntracked.txt\n' ) await writeFile(join(primary, 'tracked.txt'), 'tracked\n') await writeFile(join(primary, '.env'), 'ROOT=1\n') + await writeFile(join(primary, 'my.env'), 'NOT_INCLUDED=1\n') await mkdir(join(primary, 'apps', 'web'), { recursive: true }) await writeFile(join(primary, 'apps', 'web', '.env'), 'NESTED=1\n') await mkdir(join(primary, 'case')) diff --git a/src/main/git/worktree-include-glob.ts b/src/main/git/worktree-include-glob.ts index f8a110bc5f5..dc800c1e528 100644 --- a/src/main/git/worktree-include-glob.ts +++ b/src/main/git/worktree-include-glob.ts @@ -3,7 +3,7 @@ type WorktreeIncludeGlobToken = | { kind: 'one' } | { kind: 'segment-star' } | { kind: 'recursive-star' } - | { kind: 'optional-slash' } + | { kind: 'recursive-directory' } export type CompiledWorktreeIncludeGlob = { regExp: RegExp | null @@ -20,18 +20,24 @@ export function compileWorktreeIncludeGlob(pattern: string): CompiledWorktreeInc const char = pattern[index] if (char === '*') { starCount++ - if (pattern[index + 1] === '*') { + let starEnd = index + while (pattern[starEnd + 1] === '*') { + starEnd++ + } + if (starEnd > index && pattern[starEnd + 1] === '/') { + // Why: `**/` consumes complete directory prefixes; making only the slash + // optional incorrectly lets `foo/**/bar` match `foo/xbar`. + tokens.push({ kind: 'recursive-directory' }) + regex += '(?:.*/)?' + index = starEnd + 1 + } else if (starEnd > index && starEnd === pattern.length - 1) { tokens.push({ kind: 'recursive-star' }) regex += '.*' - index++ - if (pattern[index + 1] === '/') { - tokens.push({ kind: 'optional-slash' }) - regex += '/?' - index++ - } + index = starEnd } else { tokens.push({ kind: 'segment-star' }) regex += '[^/]*' + index = starEnd } } else if (char === '?') { tokens.push({ kind: 'one' }) @@ -42,7 +48,10 @@ export function compileWorktreeIncludeGlob(pattern: string): CompiledWorktreeInc } } const variableWidthIndex = tokens.findIndex( - (token) => token.kind === 'segment-star' || token.kind === 'recursive-star' + (token) => + token.kind === 'segment-star' || + token.kind === 'recursive-star' || + token.kind === 'recursive-directory' ) return { // Why: multiple variable-width groups can make JS regex backtracking exponential. @@ -92,11 +101,13 @@ export function matchesWorktreeIncludeGlob( const canConsume = token.kind === 'recursive-star' || comparableValue[index - 1] !== '/' current[index] = previous[index] || (canConsume && current[index - 1]) ? 1 : 0 } - } else if (token.kind === 'optional-slash') { + } else if (token.kind === 'recursive-directory') { current[0] = previous[0] + let canConsume = previous[0] === 1 for (let index = 1; index <= value.length; index++) { + canConsume ||= previous[index - 1] === 1 current[index] = - previous[index] || (comparableValue[index - 1] === '/' && previous[index - 1]) ? 1 : 0 + previous[index] || (canConsume && comparableValue[index - 1] === '/') ? 1 : 0 } } else { for (let index = 1; index <= value.length; index++) { diff --git a/src/main/git/worktree-include-pattern.ts b/src/main/git/worktree-include-pattern.ts index 18161d05604..02bc5b86f23 100644 --- a/src/main/git/worktree-include-pattern.ts +++ b/src/main/git/worktree-include-pattern.ts @@ -22,6 +22,11 @@ export type WorktreeIncludePattern = { export type WorktreeIncludeMatchBudget = { remaining: number } +export type WorktreeIncludePatternDecision = { + included: boolean + excludesDescendants: boolean +} + export function parseWorktreeIncludePatterns(content: string): WorktreeIncludePattern[] { const patterns: WorktreeIncludePattern[] = [] for (const rawLine of content.split(/\r?\n/)) { @@ -125,6 +130,40 @@ function patternMatches( return false } +export function getWorktreeIncludePatternDecision( + patterns: readonly WorktreeIncludePattern[], + relativePath: string, + isDirectory: boolean, + budget: WorktreeIncludeMatchBudget, + ignoreCase: boolean = false +): WorktreeIncludePatternDecision { + // Why: gitignore patterns are order-sensitive, so negations must use last-match-wins. + let included = false + let lastMatchedIndex = -1 + let lastPositiveIndex = -1 + const caseFoldedRelativePath = ignoreCase ? relativePath.toLowerCase() : relativePath + for (let index = 0; index < patterns.length; index++) { + const pattern = patterns[index] + if (!pattern.negated) { + lastPositiveIndex = index + } + if ( + patternMatches(pattern, relativePath, caseFoldedRelativePath, isDirectory, budget, ignoreCase) + ) { + included = !pattern.negated + lastMatchedIndex = index + } + } + return { + included, + // Why: a final directory exclusion covers descendants too, so Git need not expand the collapsed tree. + excludesDescendants: + lastMatchedIndex >= 0 && + patterns[lastMatchedIndex].negated && + lastMatchedIndex > lastPositiveIndex + } +} + export function isIncludedByWorktreePatterns( patterns: readonly WorktreeIncludePattern[], relativePath: string, @@ -132,15 +171,6 @@ export function isIncludedByWorktreePatterns( budget: WorktreeIncludeMatchBudget, ignoreCase: boolean = false ): boolean { - // Why: gitignore patterns are order-sensitive, so negations must use last-match-wins. - let included = false - const caseFoldedRelativePath = ignoreCase ? relativePath.toLowerCase() : relativePath - for (const pattern of patterns) { - if ( - patternMatches(pattern, relativePath, caseFoldedRelativePath, isDirectory, budget, ignoreCase) - ) { - included = !pattern.negated - } - } - return included + return getWorktreeIncludePatternDecision(patterns, relativePath, isDirectory, budget, ignoreCase) + .included } diff --git a/src/main/ipc/worktree-apfs-clone.ts b/src/main/ipc/worktree-apfs-clone.ts index 2f8469b6029..5a4c53e8612 100644 --- a/src/main/ipc/worktree-apfs-clone.ts +++ b/src/main/ipc/worktree-apfs-clone.ts @@ -1,4 +1,4 @@ -import { execFile } from 'node:child_process' +import { execFile, type ExecFileOptions } from 'node:child_process' import { randomUUID } from 'node:crypto' import { mkdir, stat, lstat, rm, link, rmdir, chmod } from 'node:fs/promises' import { dirname, resolve, sep } from 'node:path' @@ -6,10 +6,12 @@ import { promisify } from 'node:util' type ExecFileAsync = ( file: string, - args: readonly string[] + args: readonly string[], + options?: Pick ) => Promise<{ stdout: string; stderr: string }> const execFileAsync = promisify(execFile) as ExecFileAsync +const APFS_FILESYSTEM_PROBE_TIMEOUT_MS = 5_000 export type ApfsCloneDeps = { execFileAsync: ExecFileAsync @@ -55,16 +57,18 @@ async function getDarwinFilesystemInfo( path: string, deps: ApfsCloneDeps ): Promise { - const { stdout: dfOutput } = await deps.execFileAsync('/bin/df', ['-P', path]) + const { stdout: dfOutput } = await deps.execFileAsync('/bin/df', ['-P', path], { + timeout: APFS_FILESYSTEM_PROBE_TIMEOUT_MS + }) const device = dfOutput.trim().split(/\r?\n/)[1]?.trim().split(/\s+/)[0] if (!device) { throw new Error(`Could not resolve filesystem device for ${path}`) } - const { stdout: diskutilOutput } = await deps.execFileAsync('/usr/sbin/diskutil', [ - 'info', - '-plist', - device - ]) + const { stdout: diskutilOutput } = await deps.execFileAsync( + '/usr/sbin/diskutil', + ['info', '-plist', device], + { timeout: APFS_FILESYSTEM_PROBE_TIMEOUT_MS } + ) const filesystemNameMatch = /FilesystemName<\/key>\s*([^<]+)<\/string>/u.exec( diskutilOutput ) diff --git a/src/main/ipc/worktree-symlinks.test.ts b/src/main/ipc/worktree-symlinks.test.ts index 266afe170e2..af0c4ae8c14 100644 --- a/src/main/ipc/worktree-symlinks.test.ts +++ b/src/main/ipc/worktree-symlinks.test.ts @@ -617,6 +617,9 @@ describe('createWorktreeCopiedPaths', () => { ([file]) => file === '/bin/df' || file === '/usr/sbin/diskutil' ) expect(filesystemProbeCalls).toHaveLength(2) + expect( + filesystemProbeCalls.every(([, , execOptions]) => execOptions?.timeout === 5_000) + ).toBe(true) const copyCalls = execFileAsyncMock.mock.calls.filter(([file]) => file === '/bin/cp') expect(copyCalls).toHaveLength(diskutilError ? 0 : 2) expect(copyCalls.every(([, args]) => !args.includes('-L'))).toBe(true)