From c98acaccdaf275daa4dc3e04313028e57968f07f Mon Sep 17 00:00:00 2001 From: Neil Date: Fri, 11 Sep 2026 22:32:44 -0700 Subject: [PATCH] perf(git): normalize tracked discard paths once per operation --- .../git/source-control/discard-changes.ts | 12 +- src/main/git/source-control/git-pathspec.ts | 12 +- .../status-discard-and-bulk-staging.test.ts | 25 ++++ src/relay/git-handler-discard-operations.ts | 21 +--- .../git-handler-working-tree-changes.test.ts | 30 +++++ src/shared/git-tracked-pathspecs.test.ts | 118 ++++++++++++++++++ src/shared/git-tracked-pathspecs.ts | 37 ++++++ 7 files changed, 218 insertions(+), 37 deletions(-) create mode 100644 src/shared/git-tracked-pathspecs.test.ts create mode 100644 src/shared/git-tracked-pathspecs.ts diff --git a/src/main/git/source-control/discard-changes.ts b/src/main/git/source-control/discard-changes.ts index a390ca741dc..d162f1d5096 100644 --- a/src/main/git/source-control/discard-changes.ts +++ b/src/main/git/source-control/discard-changes.ts @@ -7,7 +7,8 @@ import type { GitRuntimeOptions } from '../git-runtime-options' import { gitOptionsForWorktree } from '../git-runtime-options' import { gitExecFileAsync } from '../runner' import { invalidateGitReadCaches } from './git-read-cache-invalidation' -import { bulkPathspecCommands, isTrackedPathSpec, literalPathspec } from './git-pathspec' +import { partitionTrackedPathSpecs } from '../../../shared/git-tracked-pathspecs' +import { bulkPathspecCommands, literalPathspec } from './git-pathspec' /** * Discard working tree changes for a file. @@ -117,14 +118,7 @@ export async function bulkDiscardChanges( } const trackedPathSpecs = await listTrackedPathSpecs(worktreePath, filePaths, options) - const trackedPaths: string[] = [] - const untrackedPaths: string[] = [] - filePaths.forEach((filePath) => { - const targetPaths = isTrackedPathSpec(filePath, trackedPathSpecs) - ? trackedPaths - : untrackedPaths - targetPaths.push(filePath) - }) + const { trackedPaths, untrackedPaths } = partitionTrackedPathSpecs(filePaths, trackedPathSpecs) await removeSafeUntrackedDiscardTargets( worktreePath, untrackedPaths, diff --git a/src/main/git/source-control/git-pathspec.ts b/src/main/git/source-control/git-pathspec.ts index c4fe23d697e..34b9670b6d8 100644 --- a/src/main/git/source-control/git-pathspec.ts +++ b/src/main/git/source-control/git-pathspec.ts @@ -16,9 +16,7 @@ const BULK_CHUNK_SIZE = 100 */ const POSIX_COMMAND_LINE_BUDGET = 128_000 -function normalizeGitPathForCompare(filePath: string): string { - return filePath.replace(/\\/g, '/').replace(/\/+$/, '') -} +export { isTrackedPathSpec } from '../../../shared/git-tracked-pathspecs' export function literalPathspec(filePath: string, options: GitRuntimeOptions): string { // Why: Git inside WSL needs POSIX paths, but host paths must stay literal, so convert backslashes only for WSL. @@ -26,14 +24,6 @@ export function literalPathspec(filePath: string, options: GitRuntimeOptions): s return `:(literal)${runtimePath}` } -export function isTrackedPathSpec(filePath: string, trackedPaths: readonly string[]): boolean { - const normalized = normalizeGitPathForCompare(filePath) - return trackedPaths.some((trackedPath) => { - const normalizedTracked = normalizeGitPathForCompare(trackedPath) - return normalizedTracked === normalized || normalizedTracked.startsWith(`${normalized}/`) - }) -} - /** * Length of the line the OS will actually be handed, wrapper included. * diff --git a/src/main/git/status-discard-and-bulk-staging.test.ts b/src/main/git/status-discard-and-bulk-staging.test.ts index 8e0b12623c4..2c6ec2058c3 100644 --- a/src/main/git/status-discard-and-bulk-staging.test.ts +++ b/src/main/git/status-discard-and-bulk-staging.test.ts @@ -222,6 +222,31 @@ describe('bulk git helpers', () => { expect(rmMock).not.toHaveBeenCalled() }) + it('preserves bulk discard action selection and original path order for path edges', async () => { + const filePaths = ['new', 'docs\\', '[ab].txt', 'docs///', 'new', 'src/file', 'docs\\'] + gitExecFileAsyncMock + .mockResolvedValueOnce({ stdout: 'docs/readme\0src/file-extra\0[ab].txt\0' }) + .mockResolvedValue({ stdout: '' }) + + await bulkDiscardChanges('/repo', filePaths) + + expect(gitExecFileAsyncMock.mock.calls.map(([args]) => args)).toEqual([ + ['ls-files', '-z', '--', ...filePaths.map((filePath) => `:(literal)${filePath}`)], + [ + 'restore', + '--worktree', + '--source=HEAD', + '--', + ':(literal)docs\\', + ':(literal)[ab].txt', + ':(literal)docs///', + ':(literal)docs\\' + ], + ['clean', '-ffdx', '--', ':(literal)new', ':(literal)new', ':(literal)src/file'] + ]) + expect(rmMock).not.toHaveBeenCalled() + }) + it('handles large tracked path lists during bulk discard classification', async () => { const trackedStdout = Array.from({ length: 150_000 }, (_, index) => `docs/file-${index}.ts`) .join('\0') diff --git a/src/relay/git-handler-discard-operations.ts b/src/relay/git-handler-discard-operations.ts index c87d13c74ff..a50bcf599a8 100644 --- a/src/relay/git-handler-discard-operations.ts +++ b/src/relay/git-handler-discard-operations.ts @@ -4,23 +4,12 @@ import { removeSafeUntrackedDiscardTarget, removeSafeUntrackedDiscardTargets } from '../shared/git-discard-path-safety' +import { partitionTrackedPathSpecs } from '../shared/git-tracked-pathspecs' import { detectConflictOperation } from './git-handler-status-ops' const BULK_CHUNK_SIZE = GIT_BULK_CHUNK_SIZE export class GitHandlerDiscardOperations extends GitHandlerOperationContext { - private normalizeGitPathForCompare(filePath: string): string { - return filePath.replace(/\\/g, '/').replace(/\/+$/, '') - } - - private isTrackedPathSpec(filePath: string, trackedPaths: readonly string[]): boolean { - const normalized = this.normalizeGitPathForCompare(filePath) - return trackedPaths.some((trackedPath) => { - const normalizedTracked = this.normalizeGitPathForCompare(trackedPath) - return normalizedTracked === normalized || normalizedTracked.startsWith(`${normalized}/`) - }) - } - private assertInWorktree(worktreePath: string, filePath: string): string { const resolved = path.resolve(worktreePath, filePath) const rel = path.relative(path.resolve(worktreePath), resolved) @@ -98,11 +87,9 @@ export class GitHandlerDiscardOperations extends GitHandlerOperationContext { } } - const trackedPaths = filePaths.filter((filePath) => - this.isTrackedPathSpec(filePath, trackedPathSpecs) - ) - const untrackedPaths = filePaths.filter( - (filePath) => !this.isTrackedPathSpec(filePath, trackedPathSpecs) + const { trackedPaths, untrackedPaths } = partitionTrackedPathSpecs( + filePaths, + trackedPathSpecs ) await removeSafeUntrackedDiscardTargets( worktreePath, diff --git a/src/relay/git-handler-working-tree-changes.test.ts b/src/relay/git-handler-working-tree-changes.test.ts index 6a0048c5b69..d28c2127162 100644 --- a/src/relay/git-handler-working-tree-changes.test.ts +++ b/src/relay/git-handler-working-tree-changes.test.ts @@ -303,6 +303,36 @@ describe('GitHandler', () => { await expect(fs.access(path.join(tmpDir, 'new.txt'))).rejects.toThrow() }) + it('preserves bulk discard action selection and original path order for path edges', async () => { + const filePaths = ['new', 'docs\\', '[ab].txt', 'docs///', 'new', 'src/file', 'docs\\'] + const gitMock = vi + .spyOn( + handler as unknown as { + git: (args: string[], cwd: string) => Promise<{ stdout: string; stderr: string }> + }, + 'git' + ) + .mockResolvedValueOnce({ stdout: 'docs/readme\0src/file-extra\0[ab].txt\0', stderr: '' }) + .mockResolvedValue({ stdout: '', stderr: '' }) + + await dispatcher.callRequest('git.bulkDiscard', { worktreePath: tmpDir, filePaths }) + + expect(gitMock.mock.calls.map(([args]) => args)).toEqual([ + ['ls-files', '-z', '--', ...filePaths.map((filePath) => `:(literal)${filePath}`)], + [ + 'restore', + '--worktree', + '--source=HEAD', + '--', + ':(literal)docs\\', + ':(literal)[ab].txt', + ':(literal)docs///', + ':(literal)docs\\' + ], + ['clean', '-ffdx', '--', ':(literal)new', ':(literal)new', ':(literal)src/file'] + ]) + }) + it('handles large tracked path lists during bulk discard classification', async () => { const trackedStdout = Array.from({ length: 150_000 }, (_, index) => `docs/file-${index}.ts`) .join('\0') diff --git a/src/shared/git-tracked-pathspecs.test.ts b/src/shared/git-tracked-pathspecs.test.ts new file mode 100644 index 00000000000..5bd1611319d --- /dev/null +++ b/src/shared/git-tracked-pathspecs.test.ts @@ -0,0 +1,118 @@ +import { describe, expect, it, vi } from 'vitest' +import { isTrackedPathSpec, partitionTrackedPathSpecs } from './git-tracked-pathspecs' + +function previousPartition(filePaths: readonly string[], trackedPaths: readonly string[]) { + const normalize = (value: string) => value.replace(/\\/g, '/').replace(/\/+$/, '') + const isTracked = (filePath: string) => { + const normalized = normalize(filePath) + return trackedPaths.some((trackedPath) => { + const normalizedTracked = normalize(trackedPath) + return normalizedTracked === normalized || normalizedTracked.startsWith(`${normalized}/`) + }) + } + return { + trackedPaths: filePaths.filter(isTracked), + untrackedPaths: filePaths.filter((filePath) => !isTracked(filePath)) + } +} + +function countNormalizations(run: () => unknown): number { + const replace = vi.spyOn(String.prototype, 'replace') + try { + run() + return replace.mock.calls.filter( + ([pattern]) => pattern instanceof RegExp && pattern.source === '\\\\' + ).length + } finally { + replace.mockRestore() + } +} + +describe('tracked pathspec partition', () => { + it.each([ + ['docs', ['docs/readme.md'], true], + ['doc', ['docs/readme.md'], false], + ['docs/file', ['docs/file-extra'], false], + ['docs///', ['docs\\readme.md'], true], + ['src\\file.ts\\', ['src/file.ts///'], true], + ['DOCS', ['docs/readme.md'], false], + ['./docs', ['docs/readme.md'], false], + ['docs//file', ['docs/file'], false], + ['docs/../file', ['file'], false], + ['[ab].txt', ['a.txt'], false], + ['[ab].txt', ['[ab].txt'], true], + [':(glob)*', ['a.txt'], false], + ['a b/é', ['a b/é/file\nname'], true], + ['é', ['e\u0301'], false], + ['C:\\repo\\docs', ['C:/repo/docs/file'], true], + ['\\\\host\\share', ['//host/share/file'], true], + ['', [], false], + ['', ['relative'], false], + ['', ['/absolute'], true], + ['/', [''], true] + ] as const)('keeps matching semantics for %j against %j', (request, tracked, expected) => { + expect(isTrackedPathSpec(request, tracked)).toBe(expected) + expect(partitionTrackedPathSpecs([request], tracked)).toEqual( + previousPartition([request], tracked) + ) + }) + + it('preserves original spelling, duplicates and relative order in both action lists', () => { + const requests = ['new', 'docs\\', '[ab].txt', 'docs///', 'new', 'src/file', 'docs\\'] + const tracked = ['docs/readme', 'src/file-extra', '[ab].txt', 'docs/readme'] + expect(partitionTrackedPathSpecs(requests, tracked)).toEqual({ + trackedPaths: ['docs\\', '[ab].txt', 'docs///', 'docs\\'], + untrackedPaths: ['new', 'new', 'src/file'] + }) + }) + + it('matches the previous implementation across combinations of path edges', () => { + const paths = [ + '', + '/', + '.', + './a', + 'a', + 'a/', + 'a//', + 'a/b', + 'a\\b', + 'a//b', + 'ab', + 'A', + '../a', + 'a/../b', + '[a]', + '*', + 'a b', + 'é', + 'e\u0301', + '/a', + '//host/share', + 'C:\\a' + ] + for (const tracked of [[], paths, ...paths.map((entry) => [entry])]) { + expect(partitionTrackedPathSpecs(paths, tracked)).toEqual(previousPartition(paths, tracked)) + } + }) + + it('normalizes each visited tracked entry once and each request once per operation', () => { + const requests = Array.from({ length: 64 }, (_, index) => `missing/${index}`) + const tracked = Array.from({ length: 256 }, (_, index) => `docs\\file-${index}///`) + expect(countNormalizations(() => previousPartition(requests, tracked))).toBe(32_896) + expect(countNormalizations(() => partitionTrackedPathSpecs(requests, tracked))).toBe(320) + expect(countNormalizations(() => partitionTrackedPathSpecs(requests, tracked))).toBe(320) + }) + + it('retains early exit for a selected directory with many tracked descendants', () => { + const tracked = Array.from({ length: 150_000 }, (_, index) => `docs/file-${index}`) + expect(countNormalizations(() => previousPartition(['docs'], tracked))).toBe(4) + expect(countNormalizations(() => partitionTrackedPathSpecs(['docs'], tracked))).toBe(2) + expect(countNormalizations(() => partitionTrackedPathSpecs([], tracked))).toBe(0) + }) + + it('does not reuse tracked evidence across operations', () => { + expect(partitionTrackedPathSpecs(['docs'], ['docs/file']).trackedPaths).toEqual(['docs']) + expect(partitionTrackedPathSpecs(['docs'], []).untrackedPaths).toEqual(['docs']) + }) +}) diff --git a/src/shared/git-tracked-pathspecs.ts b/src/shared/git-tracked-pathspecs.ts new file mode 100644 index 00000000000..7a717c2a374 --- /dev/null +++ b/src/shared/git-tracked-pathspecs.ts @@ -0,0 +1,37 @@ +function normalizeGitPathForCompare(filePath: string): string { + return filePath.replace(/\\/g, '/').replace(/\/+$/, '') +} + +function createTrackedPathSpecMatcher( + trackedPaths: readonly string[] +): (filePath: string) => boolean { + // Normalize lazily so selecting a directory still stops at its first tracked descendant. + const normalizedTrackedPaths: string[] = [] + return (filePath) => { + const normalized = normalizeGitPathForCompare(filePath) + const descendantPrefix = `${normalized}/` + return trackedPaths.some((trackedPath, index) => { + const normalizedTracked = (normalizedTrackedPaths[index] ??= + normalizeGitPathForCompare(trackedPath)) + return normalizedTracked === normalized || normalizedTracked.startsWith(descendantPrefix) + }) + } +} + +export function isTrackedPathSpec(filePath: string, trackedPaths: readonly string[]): boolean { + return createTrackedPathSpecMatcher(trackedPaths)(filePath) +} + +export function partitionTrackedPathSpecs( + filePaths: readonly string[], + trackedPathSpecs: readonly string[] +): { trackedPaths: string[]; untrackedPaths: string[] } { + const isTracked = createTrackedPathSpecMatcher(trackedPathSpecs) + const trackedPaths: string[] = [] + const untrackedPaths: string[] = [] + // Keep original spellings, duplicates and order: these arrays select restore versus clean. + for (const filePath of filePaths) { + ;(isTracked(filePath) ? trackedPaths : untrackedPaths).push(filePath) + } + return { trackedPaths, untrackedPaths } +}