diff --git a/src/main/git/command-runner/git-command-resolution.ts b/src/main/git/command-runner/git-command-resolution.ts index 750034dc0369..a70dd1368040 100644 --- a/src/main/git/command-runner/git-command-resolution.ts +++ b/src/main/git/command-runner/git-command-resolution.ts @@ -124,6 +124,7 @@ export function resolveGitCommandWithoutProbe( return resolveCommand('git', wslGitArgs(args, options), options.cwd, options.wslDistro, { useWslLoginShell: Boolean(options.wslDistro), captureLoginShellOutput, + env: options.env, terminationBarrier: options.terminationBarrier }) } diff --git a/src/main/git/command-runner/wsl-command-resolution-object-store.test.ts b/src/main/git/command-runner/wsl-command-resolution-object-store.test.ts new file mode 100644 index 000000000000..9585a2e88745 --- /dev/null +++ b/src/main/git/command-runner/wsl-command-resolution-object-store.test.ts @@ -0,0 +1,44 @@ +import { runProcess } from '@orca/process-host' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { resolveCommand } from './wsl-command-resolution' + +const originalPlatform = process.platform +const QUARANTINE = { + GIT_OBJECT_DIRECTORY: '/home/me/repo/.git/objects/tmp_objdir-orca-merge-tree-x', + GIT_ALTERNATE_OBJECT_DIRECTORIES: '/home/me/repo/.git/objects' +} + +/** What WSL Git would see when a login profile has set `profileAlternates` before the payload runs. */ +async function alternatesSeenByWslGit(profileAlternates: string | undefined): Promise { + const resolved = resolveCommand('git', [], undefined, 'Ubuntu', { + env: { ...process.env, ...QUARANTINE }, + wslShellCommand: 'printenv GIT_ALTERNATE_OBJECT_DIRECTORIES' + }) + const payload = resolved.args.at(-1) ?? '' + const profile = + profileAlternates === undefined + ? '' + : `export GIT_ALTERNATE_OBJECT_DIRECTORIES='${profileAlternates}'; ` + const result = await runProcess({ program: 'sh', args: ['-c', `${profile}${payload}`] }) + expect(result.code).toBe(0) + return result.stdout.trim() +} + +// Why pin this: a WSL login profile that sets GIT_ALTERNATE_OBJECT_DIRECTORIES used to replace a +// WSLENV-forwarded value, so a quarantined merge-tree lost the real object store and failed. +describe.skipIf(originalPlatform === 'win32')('WSL Git object-store variables', () => { + beforeEach(() => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + }) + + afterEach(() => { + Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) + }) + + it('reach WSL Git after the login profile, appending to its alternates', async () => { + await expect(alternatesSeenByWslGit(undefined)).resolves.toBe('/home/me/repo/.git/objects') + await expect(alternatesSeenByWslGit('/shared/objects')).resolves.toBe( + '/shared/objects:/home/me/repo/.git/objects' + ) + }) +}) diff --git a/src/main/git/command-runner/wsl-command-resolution.ts b/src/main/git/command-runner/wsl-command-resolution.ts index cb5d9de41f43..eebf7f10ebdc 100644 --- a/src/main/git/command-runner/wsl-command-resolution.ts +++ b/src/main/git/command-runner/wsl-command-resolution.ts @@ -23,6 +23,25 @@ const GIT_OUTPUT_LOCALE_ENV_ARGS = Object.entries(UNTRANSLATED_GIT_OUTPUT_ENV).m ([key, value]) => `${key}=${value}` ) +// Why on the command string: a login profile runs first and could replace WSLENV-forwarded values; +// alternates append to whatever the profile set, as Git's own scratch object stores do. +function wslGitObjectStorePrefix(env: NodeJS.ProcessEnv | undefined): string { + const objectDirectory = env?.GIT_OBJECT_DIRECTORY + const alternates = env?.GIT_ALTERNATE_OBJECT_DIRECTORIES + // Why compare with process.env: only a quarantine's own values, never a Windows-side inherited one. + if ( + objectDirectory === undefined || + alternates === undefined || + objectDirectory === process.env.GIT_OBJECT_DIRECTORY + ) { + return '' + } + return ( + `GIT_OBJECT_DIRECTORY=${quotePosixShell(objectDirectory)} ` + + `GIT_ALTERNATE_OBJECT_DIRECTORIES="\${GIT_ALTERNATE_OBJECT_DIRECTORIES:+$GIT_ALTERNATE_OBJECT_DIRECTORIES:}"${quotePosixShell(alternates)} ` + ) +} + export type ResolvedCommand = { binary: string args: string[] @@ -92,7 +111,10 @@ export function resolveCommand( const translatedArgs = translateArgsForWsl(args) // Why: env on wsl.exe stays Windows-side (WSLENV forwards only named vars), so the locale must ride the command string (issue #7808). - const localePrefix = command === 'git' ? `${GIT_OUTPUT_LOCALE_SHELL_PREFIX} ` : '' + const localePrefix = + command === 'git' + ? `${GIT_OUTPUT_LOCALE_SHELL_PREFIX} ${wslGitObjectStorePrefix(options.env)}` + : '' const escapedCommand = options.wslShellCommand ?? quotePosixShell(command) // Why: shell-escape each arg to prevent word splitting / glob expansion inside the bash -c string. const escapedArgs = translatedArgs.map(quotePosixShell) diff --git a/src/main/git/local-git-object-quarantine.test.ts b/src/main/git/local-git-object-quarantine.test.ts new file mode 100644 index 000000000000..0ef6758a0dbe --- /dev/null +++ b/src/main/git/local-git-object-quarantine.test.ts @@ -0,0 +1,135 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type * as GitObjectQuarantineModule from '../../shared/git-object-quarantine' +import type { GitObjectQuarantine, GitObjectsDirectory } from '../../shared/git-object-quarantine' +import { + createLocalGitObjectQuarantine, + inheritedLocalObjectStore, + localGitObjectsDirectory +} from './local-git-object-quarantine' +import { + resetWslLinkedWorktreeGitRoutingForTests, + seedWslLinkedWorktreeGitRoutingForTests +} from './wsl-linked-worktree-git-routing' + +const { readRepoCommonDirFromGitMock } = vi.hoisted(() => ({ + readRepoCommonDirFromGitMock: vi.fn() +})) + +vi.mock('./worktree-list-reader', () => ({ + readRepoCommonDirFromGit: readRepoCommonDirFromGitMock +})) +vi.mock('../../shared/git-object-quarantine', async (importOriginal) => ({ + ...(await importOriginal()), + // Why: hands the resolved objects dir straight to the command; these tests pin routing, not disk. + createGitObjectQuarantine: ( + resolve: () => Promise + ): GitObjectQuarantine => ({ + run: async (command) => { + const objects = await resolve() + return command( + objects + ? { + GIT_OBJECT_DIRECTORY: `${objects.gitPath}/scratch`, + GIT_ALTERNATE_OBJECT_DIRECTORIES: objects.gitPath + } + : undefined + ) + } + }) +})) + +describe('localGitObjectsDirectory', () => { + it('translates a WSL Git common dir into a path the Windows host can open', () => { + expect(localGitObjectsDirectory('/home/me/repo/.git', 'Ubuntu')).toEqual({ + hostPath: '\\\\wsl.localhost\\Ubuntu\\home\\me\\repo\\.git\\objects', + gitPath: '/home/me/repo/.git/objects' + }) + }) + + it('hands WSL Git the drvfs spelling of a drive-path common dir (Git < 2.31 prints it relative)', () => { + expect(localGitObjectsDirectory('C:\\repo\\.git', 'Ubuntu')).toEqual({ + hostPath: 'C:\\repo\\.git\\objects', + gitPath: '/mnt/c/repo/.git/objects' + }) + expect(localGitObjectsDirectory('/mnt/c/repo/.git', 'Ubuntu')).toEqual({ + hostPath: 'C:\\repo\\.git\\objects', + gitPath: '/mnt/c/repo/.git/objects' + }) + }) + + it('uses one spelling for native Git', () => { + expect(localGitObjectsDirectory('/Users/me/repo/.git', undefined)).toEqual({ + hostPath: '/Users/me/repo/.git/objects', + gitPath: '/Users/me/repo/.git/objects' + }) + expect(localGitObjectsDirectory('C:\\repo\\.git', undefined)).toEqual({ + hostPath: 'C:\\repo\\.git\\objects', + gitPath: 'C:\\repo\\.git\\objects' + }) + }) +}) + +describe('createLocalGitObjectQuarantine on a Windows host', () => { + const originalPlatform = process.platform + + beforeEach(() => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + resetWslLinkedWorktreeGitRoutingForTests() + readRepoCommonDirFromGitMock.mockReset() + }) + + afterEach(() => { + Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) + resetWslLinkedWorktreeGitRoutingForTests() + }) + + const envSeenBy = (repoPath: string, options: { wslDistro?: string } = {}) => + createLocalGitObjectQuarantine(repoPath, options).run(async (env) => env) + + it('runs unquarantined when a WSL-authored linked worktree routes to Windows Git', async () => { + readRepoCommonDirFromGitMock.mockResolvedValue('/mnt/c/repo/.git') + seedWslLinkedWorktreeGitRoutingForTests('C:\\wt') + + await expect(envSeenBy('C:\\wt', { wslDistro: 'Ubuntu' })).resolves.toBeUndefined() + }) +}) + +describe('inheritedLocalObjectStore', () => { + const ALTERNATES = { GIT_ALTERNATE_OBJECT_DIRECTORIES: '/shared/objects' } + + it('passes native Git’s inherited alternates on and refuses an inherited object dir', () => { + expect(inheritedLocalObjectStore(undefined, ALTERNATES, 'darwin')).toEqual({ + inheritedAlternates: '/shared/objects' + }) + expect( + inheritedLocalObjectStore(undefined, { GIT_OBJECT_DIRECTORY: 'C:\\objects' }, 'win32') + ).toBeUndefined() + }) + + it('ignores Windows values WSLENV does not forward, since WSL Git never saw them', () => { + expect(inheritedLocalObjectStore('Ubuntu', ALTERNATES, 'win32')).toEqual({}) + expect( + inheritedLocalObjectStore( + 'Ubuntu', + { WSLENV: 'GIT_OBJECT_DIRECTORY/p:GIT_ALTERNATE_OBJECT_DIRECTORIES' }, + 'win32' + ) + ).toEqual({}) + expect( + inheritedLocalObjectStore('Ubuntu', { GIT_OBJECT_DIRECTORY: 'C:\\objects' }, 'win32') + ).toEqual({}) + }) + + it.each(['GIT_ALTERNATE_OBJECT_DIRECTORIES', 'GIT_OBJECT_DIRECTORY/p'])( + 'runs unquarantined when WSLENV forwards %s to WSL Git', + (token) => { + const env = { + GIT_ALTERNATE_OBJECT_DIRECTORIES: 'C:\\shared', + GIT_OBJECT_DIRECTORY: 'C:\\objects', + WSLENV: `ORCA_X/u:${token}` + } + + expect(inheritedLocalObjectStore('Ubuntu', env, 'win32')).toBeUndefined() + } + ) +}) diff --git a/src/main/git/local-git-object-quarantine.ts b/src/main/git/local-git-object-quarantine.ts new file mode 100644 index 000000000000..068709b9f774 --- /dev/null +++ b/src/main/git/local-git-object-quarantine.ts @@ -0,0 +1,82 @@ +import { isWindowsAbsolutePathLike } from '../../shared/cross-platform-path' +import type { GitAdmissionTier } from '../../shared/rpc-contract/git-admission-tier-params' +import { + createGitObjectQuarantine, + inheritedObjectStore, + pathApiForGitPath, + type GitObjectsDirectory +} from '../../shared/git-object-quarantine' +import { toLinuxPath, toWindowsWslPath } from '../../shared/wsl-paths' +import { parseWslPath } from '../wsl' +import { usesHostGitForWslLinkedWorktree } from './wsl-linked-worktree-git-routing' +import { readRepoCommonDirFromGit } from './worktree-list-reader' + +export type LocalGitObjectQuarantine = { + /** `env` is a full process env for `gitExecFileAsync`, or undefined to run unquarantined. */ + run: (command: (env: NodeJS.ProcessEnv | undefined) => Promise) => Promise +} + +/** `objects/` in Git's spelling and, for WSL Git, translated to one the main process can open. */ +export function localGitObjectsDirectory( + commonDir: string, + wslDistro: string | undefined +): GitObjectsDirectory { + const hostCommonDir = + wslDistro && !isWindowsAbsolutePathLike(commonDir) + ? toWindowsWslPath(commonDir, wslDistro) + : commonDir + // Why: Git < 2.31 prints a relative common dir, resolved against a `C:\` repo path; WSL Git needs `/mnt/c`. + const gitCommonDir = wslDistro ? toLinuxPath(commonDir) : commonDir + return { + hostPath: pathApiForGitPath(hostCommonDir).join(hostCommonDir, 'objects'), + gitPath: pathApiForGitPath(gitCommonDir).join(gitCommonDir, 'objects') + } +} + +const OBJECT_STORE_VARIABLES = ['GIT_OBJECT_DIRECTORY', 'GIT_ALTERNATE_OBJECT_DIRECTORIES'] + +/** The object-store variables local Git would inherit, or undefined when they cannot be mirrored. */ +export function inheritedLocalObjectStore( + wslDistro: string | undefined, + env: NodeJS.ProcessEnv = process.env, + platform: NodeJS.Platform = process.platform +): Pick | undefined { + if (platform !== 'win32' || !wslDistro) { + return inheritedObjectStore(env) + } + // Why: wsl.exe forwards only what WSLENV names, maybe path-translated (`/p`); skip rather than guess. + const forwarded = new Set((env.WSLENV ?? '').split(':').map((token) => token.split('/')[0])) + return OBJECT_STORE_VARIABLES.some((key) => forwarded.has(key) && env[key] !== undefined) + ? undefined + : {} +} + +/** Scratch object store for throwaway Git writes on the local (native or WSL) host. */ +export function createLocalGitObjectQuarantine( + repoPath: string, + options: { wslDistro?: string; signal?: AbortSignal; admissionTier?: GitAdmissionTier } = {} +): LocalGitObjectQuarantine { + const wslDistro = parseWslPath(repoPath)?.distro ?? options.wslDistro + const quarantine = createGitObjectQuarantine(async () => { + const inherited = inheritedLocalObjectStore(wslDistro) + if (!inherited) { + return undefined + } + const commonDir = await readRepoCommonDirFromGit(repoPath, { + ...(options.wslDistro ? { wslDistro: options.wslDistro } : {}), + ...(options.signal ? { signal: options.signal } : {}), + ...(options.admissionTier ? { admissionTier: options.admissionTier } : {}) + }) + // Why after the read: that first Git call is what settles the WSL linked-worktree route, + // and Windows Git on that route may not share the spelling resolved above. + if (!commonDir || usesHostGitForWslLinkedWorktree(repoPath, options.wslDistro)) { + return undefined + } + return { ...localGitObjectsDirectory(commonDir, wslDistro), ...inherited } + }) + return { + run: (command) => + // Why no WSLENV: the WSL runner puts both variables on WSL Git's command line. + quarantine.run((env) => command(env ? { ...process.env, ...env } : undefined)) + } +} diff --git a/src/main/git/merge-tree-object-quarantine-real-git.test.ts b/src/main/git/merge-tree-object-quarantine-real-git.test.ts new file mode 100644 index 000000000000..bca3b91168a9 --- /dev/null +++ b/src/main/git/merge-tree-object-quarantine-real-git.test.ts @@ -0,0 +1,339 @@ +import { existsSync, readdirSync, writeFileSync } from 'node:fs' +import type * as NodeFsPromises from 'node:fs/promises' +import { basename, dirname, join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { GIT_OBJECT_QUARANTINE_DIR_PREFIX } from '../../shared/git-object-quarantine' +import { + createDivergentRepoFixture, + type DivergentRepoFixture +} from '../../shared/git-object-quarantine-real-git.test-fixture' +import { + __resetPRConflictSummaryCachesForTests, + getPRConflictSummary +} from '../github/conflict-summary' +import { __resetPRConflictSummaryDerivationCachesForTests } from '../github/conflict-summary-cache' +import { getLocalGitCapabilityCache } from './git-capability-state' +import { createLocalGitObjectQuarantine } from './local-git-object-quarantine' +import { gitExecFileAsync } from './runner' +import { deleteBranchAfterWorktreeRemoval } from './worktree-branch-removal' + +const scratchFoldersMade = vi.hoisted((): string[] => []) + +vi.mock('node:fs/promises', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + mkdtemp: async (prefix: string) => { + const made = await actual.mkdtemp(prefix) + if (basename(made).startsWith(GIT_OBJECT_QUARANTINE_DIR_PREFIX)) { + scratchFoldersMade.push(made) + } + return made + } + } +}) + +/** Records what a first successful check teaches the host; the cold path has its own test. */ +function rememberMergeTreeWritesTrees(): void { + getLocalGitCapabilityCache().rememberSupported('merge-tree-write-tree') +} + +// Why: every divergent `merge-tree --write-tree` used to leave unreachable loose objects in the +// real store; enough of them keeps Git's auto-gc re-running forever on large repositories. +describe('merge-tree runs against a scratch object store (real Git)', () => { + let fixture: DivergentRepoFixture + + beforeEach(() => { + __resetPRConflictSummaryCachesForTests() + rememberMergeTreeWritesTrees() + scratchFoldersMade.length = 0 + fixture = createDivergentRepoFixture() + }) + + afterEach(() => { + fixture.dispose() + }) + + function unquarantinedConflictFiles(cwd: string, base: string, head: string): string[] { + const mergeBase = fixture.git(cwd, 'merge-base', head, base).trim() + let stdout: string + try { + stdout = fixture.git( + cwd, + 'merge-tree', + '--write-tree', + '--name-only', + '-z', + '--no-messages', + '--merge-base', + mergeBase, + head, + base + ) + } catch (error) { + // A conflicted merge exits 1 but still prints the file list. + stdout = error instanceof Error && 'stdout' in error ? String(error.stdout) : '' + } + return stdout.split('\0').filter(Boolean).slice(1) + } + + it.each([ + ['the main worktree', (f: DivergentRepoFixture) => f.repoPath], + ['a linked worktree', (f: DivergentRepoFixture) => f.linkedPath] + ])('conflict summary from %s writes no loose objects', async (_label, pickCwd) => { + const cwd = pickCwd(fixture) + const base = fixture.git(cwd, 'rev-parse', 'main').trim() + const head = fixture.git(cwd, 'rev-parse', 'feature-conflict').trim() + const before = fixture.looseObjectCount() + + const summary = await getPRConflictSummary(cwd, 'main', base, head) + + expect(fixture.looseObjectCount()).toBe(before) + expect(fixture.scratchDirectories()).toEqual([]) + expect(summary?.files).toEqual(['shared.txt']) + // The unquarantined run proves the fixture really writes objects and the output matches. + expect(unquarantinedConflictFiles(cwd, base, head)).toEqual(summary?.files) + expect(fixture.looseObjectCount()).toBeGreaterThan(before) + }) + + it.each([ + ['the main worktree', (f: DivergentRepoFixture) => f.repoPath], + ['a linked worktree', (f: DivergentRepoFixture) => f.linkedPath] + ])('branch cleanup from %s writes no loose objects', async (_label, pickCwd) => { + const cwd = pickCwd(fixture) + fixture.git(cwd, 'config', 'branch.feature-extra.base', 'refs/heads/main') + fixture.git(cwd, 'config', 'branch.feature-squashed.base', 'refs/heads/main') + const extraHead = fixture.git(cwd, 'rev-parse', 'feature-extra').trim() + const squashedHead = fixture.git(cwd, 'rev-parse', 'feature-squashed').trim() + const before = fixture.looseObjectCount() + + const extra = await deleteBranchAfterWorktreeRemoval(cwd, 'feature-extra', extraHead, {}) + const squashed = await deleteBranchAfterWorktreeRemoval( + cwd, + 'feature-squashed', + squashedHead, + {} + ) + + expect(fixture.looseObjectCount()).toBe(before) + expect(fixture.scratchDirectories()).toEqual([]) + expect(extra).toEqual({ preservedBranch: { branchName: 'feature-extra', head: extraHead } }) + expect(squashed).toEqual({}) + // Same verdicts as an unquarantined merge-tree, which does write objects. + const mainTree = fixture.git(cwd, 'rev-parse', 'main^{tree}').trim() + const mergeTree = (branch: string): string => + fixture.git(cwd, 'merge-tree', '--write-tree', 'main', branch).trim() + expect(mergeTree('feature-extra')).not.toBe(mainTree) + expect(mergeTree(squashedHead)).toBe(mainTree) + expect(fixture.looseObjectCount()).toBeGreaterThan(before) + }) + + it('keeps file contents a partial clone downloads, so the next check does not fetch them again', async () => { + const clone = fixture.createPartialClone() + const git = (...args: string[]): string => fixture.git(clone.clonePath, ...args).trim() + const base = git('rev-parse', 'origin/main') + const head = git('rev-parse', 'origin/feature-conflict') + const packsBefore = clone.packCount() + const looseBefore = clone.looseObjectCount() + + const first = await getPRConflictSummary(clone.clonePath, 'main', base, head) + const packsAfterFirst = clone.packCount() + __resetPRConflictSummaryDerivationCachesForTests() + const second = await getPRConflictSummary(clone.clonePath, 'main', base, head) + + expect(first?.files).toEqual(['shared.txt']) + expect(second?.files).toEqual(['shared.txt']) + // The first check fetched the missing blobs into the real store; the second found them there. + expect(packsAfterFirst).toBeGreaterThan(packsBefore) + expect(clone.packCount()).toBe(packsAfterFirst) + expect(clone.looseObjectCount()).toBe(looseBefore) + expect(scratchFoldersMade).toHaveLength(2) + expect(clone.scratchDirectories()).toEqual([]) + }) + + it('lists a partial clone’s conflicts offline once an earlier check downloaded the contents', async () => { + const clone = fixture.createPartialClone() + const git = (...args: string[]): string => fixture.git(clone.clonePath, ...args).trim() + const base = git('rev-parse', 'origin/main') + const head = git('rev-parse', 'origin/feature-conflict') + + const online = await getPRConflictSummary(clone.clonePath, 'main', base, head) + git('remote', 'set-url', 'origin', join(dirname(clone.clonePath), 'unreachable-origin')) + __resetPRConflictSummaryDerivationCachesForTests() + const offline = await getPRConflictSummary(clone.clonePath, 'main', base, head) + + expect(online?.files).toEqual(['shared.txt']) + expect(offline?.files).toEqual(['shared.txt']) + expect(scratchFoldersMade).toHaveLength(2) + expect(clone.scratchDirectories()).toEqual([]) + }) + + it('makes no scratch folder until a check shows this Git writes trees, then one per check', async () => { + __resetPRConflictSummaryCachesForTests() + const base = fixture.git(fixture.repoPath, 'rev-parse', 'main').trim() + const head = fixture.git(fixture.repoPath, 'rev-parse', 'feature-conflict').trim() + + const first = await getPRConflictSummary(fixture.repoPath, 'main', base, head) + expect(scratchFoldersMade).toEqual([]) + __resetPRConflictSummaryDerivationCachesForTests() + const second = await getPRConflictSummary(fixture.repoPath, 'main', base, head) + + expect(first?.files).toEqual(['shared.txt']) + expect(second?.files).toEqual(['shared.txt']) + expect(scratchFoldersMade).toHaveLength(1) + expect(fixture.scratchDirectories()).toEqual([]) + }) + + it('removes the scratch directory when the Git command fails', async () => { + const quarantine = createLocalGitObjectQuarantine(fixture.linkedPath) + let scratch: string | undefined + + await expect( + quarantine.run(async (env) => { + scratch = env?.GIT_OBJECT_DIRECTORY + expect(scratch && existsSync(scratch)).toBe(true) + return gitExecFileAsync(['merge-tree', '--write-tree', 'main', 'no-such-branch'], { + cwd: fixture.linkedPath, + env + }) + }) + ).rejects.toThrow() + + expect(scratch).toBeDefined() + expect(existsSync(scratch ?? '')).toBe(false) + expect(fixture.scratchDirectories()).toEqual([]) + }) + + it('removes the scratch directory when the command is aborted', async () => { + const quarantine = createLocalGitObjectQuarantine(fixture.repoPath) + const controller = new AbortController() + let scratch: string | undefined + + await expect( + quarantine.run(async (env) => { + scratch = env?.GIT_OBJECT_DIRECTORY + const running = gitExecFileAsync(['merge-tree', '--write-tree', 'main', 'feature-extra'], { + cwd: fixture.repoPath, + env, + signal: controller.signal + }) + controller.abort() + return running + }) + ).rejects.toThrow() + + expect(scratch).toBeDefined() + expect(existsSync(scratch ?? '')).toBe(false) + }) +}) + +// Why: the quarantine must leave Git seeing exactly the object stores it sees without it. +describe('merge-tree quarantine keeps the object stores Git would otherwise see (real Git)', () => { + let fixture: DivergentRepoFixture + + beforeEach(() => { + __resetPRConflictSummaryCachesForTests() + rememberMergeTreeWritesTrees() + scratchFoldersMade.length = 0 + fixture = createDivergentRepoFixture() + }) + + afterEach(() => { + vi.unstubAllEnvs() + fixture.dispose() + }) + + const BRANCHES = ['main', 'feature-conflict', 'feature-extra', 'feature-squashed'] + const fixtureHead = (branch: string): string => + fixture.git(fixture.repoPath, 'rev-parse', branch).trim() + + function emptyRepo(name: string): string { + const repo = join(dirname(fixture.repoPath), name) + fixture.git(dirname(fixture.repoPath), 'init', '--quiet', repo) + return repo + } + + /** Points `repo`'s branches at the fixture's commits; their objects must already be reachable. */ + function addFixtureBranches(repo: string): void { + for (const branch of BRANCHES) { + fixture.git(repo, 'update-ref', `refs/heads/${branch}`, fixtureHead(branch)) + } + fixture.git(repo, 'symbolic-ref', 'HEAD', 'refs/heads/main') + fixture.git(repo, 'config', 'branch.feature-extra.base', 'refs/heads/main') + fixture.git(repo, 'config', 'branch.feature-squashed.base', 'refs/heads/main') + } + + const looseObjectsIn = (repo: string): number => { + const objects = join(repo, '.git', 'objects') + return readdirSync(objects) + .filter((entry) => /^[0-9a-f]{2}$/.test(entry)) + .reduce((count, entry) => count + readdirSync(join(objects, entry)).length, 0) + } + + async function expectUnquarantinedVerdicts(repo: string): Promise { + const summary = await getPRConflictSummary( + repo, + 'main', + fixtureHead('main'), + fixtureHead('feature-conflict') + ) + const extra = await deleteBranchAfterWorktreeRemoval( + repo, + 'feature-extra', + fixtureHead('feature-extra'), + {} + ) + const squashed = await deleteBranchAfterWorktreeRemoval( + repo, + 'feature-squashed', + fixtureHead('feature-squashed'), + {} + ) + + expect(summary?.files).toEqual(['shared.txt']) + expect(extra).toEqual({ + preservedBranch: { branchName: 'feature-extra', head: fixtureHead('feature-extra') } + }) + expect(squashed).toEqual({}) + expect(fixture.git(repo, 'branch', '--list', 'feature-squashed')).toBe('') + } + + it('reads objects reachable only through inherited GIT_ALTERNATE_OBJECT_DIRECTORIES', async () => { + const repo = emptyRepo('reached-by-env-alternates') + vi.stubEnv('GIT_ALTERNATE_OBJECT_DIRECTORIES', join(fixture.commonDir, 'objects')) + addFixtureBranches(repo) + + await expectUnquarantinedVerdicts(repo) + + expect(looseObjectsIn(repo)).toBe(0) + }) + + it('leaves an inherited GIT_OBJECT_DIRECTORY in charge, writing where Git would', async () => { + const repo = emptyRepo('inherited-object-dir') + vi.stubEnv('GIT_OBJECT_DIRECTORY', join(fixture.commonDir, 'objects')) + addFixtureBranches(repo) + const before = fixture.looseObjectCount() + + await expectUnquarantinedVerdicts(repo) + + // Unquarantined, Git writes the merge results into the store it was handed. + expect(fixture.looseObjectCount()).toBeGreaterThan(before) + expect(fixture.scratchDirectories()).toEqual([]) + }) + + it('reads through an info/alternates chain at the deepest nesting Git accepts', async () => { + let linked = join(fixture.commonDir, 'objects') + let repo = '' + for (let depth = 1; depth <= 6; depth++) { + repo = emptyRepo(`alternates-chain-${depth}`) + writeFileSync(join(repo, '.git', 'objects', 'info', 'alternates'), `${linked}\n`) + linked = join(repo, '.git', 'objects') + } + addFixtureBranches(repo) + + await expectUnquarantinedVerdicts(repo) + + // A store with its own alternates runs unquarantined, so Git writes where it always has. + expect(looseObjectsIn(repo)).toBeGreaterThan(0) + }) +}) diff --git a/src/main/git/worktree-branch-removal.ts b/src/main/git/worktree-branch-removal.ts index 4120b1240693..2acb0dbff11d 100644 --- a/src/main/git/worktree-branch-removal.ts +++ b/src/main/git/worktree-branch-removal.ts @@ -1,11 +1,13 @@ import { branchHasNoUnmergedChangesWithLazyTargetRefresh, - getBranchCleanupTargetRefs + getBranchCleanupTargetRefs, + type GitBranchCleanupExecOptions } from '../../shared/git-branch-cleanup' import type { RemoveWorktreeResult } from '../../shared/worktree/create-types' import { withLocalGitCapabilityCacheForExecution } from './git-capability-state' import { withRepoRefMaintenancePaused } from './local-repo-ref-maintenance' import { gitExecFileAsync } from './runner' +import { createLocalGitObjectQuarantine } from './local-git-object-quarantine' import { isBranchReservedByWorktreeOperation } from '../../shared/git-worktree-admin' import { parseWorktreeList } from '../../shared/git-worktree-porcelain-parser' import { isBranchCheckedOutInWorktreeError } from '../../shared/git-branch-delete-refusal' @@ -108,11 +110,16 @@ async function deleteAlreadyMergedBranchAfterSafeDeleteFailure( branchHead: string, options: GitWorktreeExecOptions = {} ): Promise { - const runGit = (args: string[], execOptions?: { stdin?: string }) => - gitExecFileAsync(args, { - ...gitExecOptions(repoPath, options), - ...(execOptions?.stdin !== undefined ? { stdin: execOptions.stdin } : {}) - }) + const quarantine = createLocalGitObjectQuarantine(repoPath, options) + const runGit = (args: string[], execOptions?: GitBranchCleanupExecOptions) => { + const run = (env: NodeJS.ProcessEnv | undefined) => + gitExecFileAsync(args, { + ...gitExecOptions(repoPath, options), + ...(execOptions?.stdin !== undefined ? { stdin: execOptions.stdin } : {}), + ...(env ? { env } : {}) + }) + return execOptions?.discardWrittenObjects ? quarantine.run(run) : run(undefined) + } const targetRefs = await getBranchCleanupTargetRefs(runGit, branchName) // Why: squash merges rewrite commit IDs, so `branch -d` rejects already-merged branches; delete only when Git proves no unmerged tree changes. const hasNoUnmergedChanges = await withLocalGitCapabilityCacheForExecution( diff --git a/src/main/github/client-merge-queue-auto-merge.test.ts b/src/main/github/client-merge-queue-auto-merge.test.ts index 60694245d483..c129f8055b18 100644 --- a/src/main/github/client-merge-queue-auto-merge.test.ts +++ b/src/main/github/client-merge-queue-auto-merge.test.ts @@ -9,6 +9,9 @@ const { clientMocks, moduleMocks } = await vi.hoisted(async () => { vi.mock('./gh-utils', () => moduleMocks.ghUtilsModuleMock(clientMocks)) vi.mock('../git/runner', () => moduleMocks.gitRunnerModuleMock(clientMocks)) +vi.mock('../git/local-git-object-quarantine', () => + moduleMocks.localGitObjectQuarantineModuleMock() +) vi.mock('./local-git-config-signature', () => moduleMocks.localGitConfigSignatureModuleMock(clientMocks) ) diff --git a/src/main/github/client-pr-conflict-summary.test.ts b/src/main/github/client-pr-conflict-summary.test.ts index 79455340ff7c..a73bc1e58ffd 100644 --- a/src/main/github/client-pr-conflict-summary.test.ts +++ b/src/main/github/client-pr-conflict-summary.test.ts @@ -9,6 +9,9 @@ const { clientMocks, moduleMocks } = await vi.hoisted(async () => { vi.mock('./gh-utils', () => moduleMocks.ghUtilsModuleMock(clientMocks)) vi.mock('../git/runner', () => moduleMocks.gitRunnerModuleMock(clientMocks)) +vi.mock('../git/local-git-object-quarantine', () => + moduleMocks.localGitObjectQuarantineModuleMock() +) vi.mock('./local-git-config-signature', () => moduleMocks.localGitConfigSignatureModuleMock(clientMocks) ) diff --git a/src/main/github/client-test-mocks.ts b/src/main/github/client-test-mocks.ts index 4868f41fb11c..11762e87b7ce 100644 --- a/src/main/github/client-test-mocks.ts +++ b/src/main/github/client-test-mocks.ts @@ -129,6 +129,19 @@ export function gitRunnerModuleMock(mocks: GitHubClientMocks): GitRunnerModuleMo } } +export type LocalGitObjectQuarantineModuleMock = { + createLocalGitObjectQuarantine: () => { + run: (command: (env: undefined) => Promise) => Promise + } +} + +// Why: client tests script git calls in order; the scratch object store adds a common-dir read. +export function localGitObjectQuarantineModuleMock(): LocalGitObjectQuarantineModuleMock { + return { + createLocalGitObjectQuarantine: () => ({ run: (command) => command(undefined) }) + } +} + export type LocalGitConfigSignatureModuleMock = { readLocalGitConfigSignature: Mock } diff --git a/src/main/github/conflict-summary.test.ts b/src/main/github/conflict-summary.test.ts index 36d96407301b..bbe212820b81 100644 --- a/src/main/github/conflict-summary.test.ts +++ b/src/main/github/conflict-summary.test.ts @@ -1,3 +1,7 @@ +import { mkdirSync, mkdtempSync, readdirSync, realpathSync, rmSync } from 'node:fs' +import type * as NodeFsPromises from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { isAbsolute, join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' // Why these tests exist: the conflict-summary derivation used to re-run a @@ -8,26 +12,52 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' // throttle window. const gitExecFileAsyncMock = vi.hoisted(() => vi.fn()) +const scratchFolderPrefixes = vi.hoisted((): string[] => []) vi.mock('../git/runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock })) +// Why: off Windows a WSL objects path is relative, so a real scratch dir would land in the cwd. +vi.mock('node:fs/promises', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + mkdtemp: async (prefix: string) => { + scratchFolderPrefixes.push(prefix) + if (!isAbsolute(prefix)) { + throw new Error('no object store in this test') + } + return actual.mkdtemp(prefix) + } + } +}) +import { GIT_CAPABILITY_RETRY_INTERVAL_MS } from '../../shared/git-capability-cache' +import { getLocalGitCapabilityCache } from '../git/git-capability-state' import { CONFLICT_SUMMARY_BASE_FETCH_WINDOW_MS } from './conflict-summary-cache' import { __resetPRConflictSummaryCachesForTests, getPRConflictSummary } from './conflict-summary' type GitResult = { stdout: string } type GitHandler = (argv: string[]) => Promise +// Why its own key: once merge-tree is known to work, its quarantine reads the common dir per derivation. +const COMMON_DIR_COMMAND = 'rev-parse --git-common-dir' + const defaultHandlers: Record = { fetch: async () => ({ stdout: '' }), 'rev-parse': async () => ({ stdout: 'base-tip-1\n' }), 'merge-base': async () => ({ stdout: 'merge-base-1\n' }), 'rev-list': async () => ({ stdout: '3\n' }), - 'merge-tree': async () => ({ stdout: 'tree-oid\u0000src/conflict.ts\u0000' }) + 'merge-tree': async () => ({ stdout: 'tree-oid\u0000src/conflict.ts\u0000' }), + [COMMON_DIR_COMMAND]: async () => ({ stdout: '.git\n' }) +} + +function commandKey(argv: string[]): string { + return argv[0] === 'rev-parse' && argv.includes('--git-common-dir') ? COMMON_DIR_COMMAND : argv[0] } function mockGitDispatch(overrides: Record = {}): void { gitExecFileAsyncMock.mockImplementation((argv: string[]) => { - const handler = overrides[argv[0]] ?? defaultHandlers[argv[0]] + const key = commandKey(argv) + const handler = overrides[key] ?? defaultHandlers[key] if (!handler) { return Promise.reject(new Error(`unexpected git command: ${argv.join(' ')}`)) } @@ -40,7 +70,7 @@ function spawnCount(command?: string): number { if (!command) { return calls.length } - return calls.filter(([argv]) => Array.isArray(argv) && argv[0] === command).length + return calls.filter(([argv]) => Array.isArray(argv) && commandKey(argv) === command).length } const expectedSummary = { @@ -65,6 +95,7 @@ describe('getPRConflictSummary caching', () => { vi.useFakeTimers() vi.setSystemTime(1_750_000_000_000) gitExecFileAsyncMock.mockReset() + scratchFolderPrefixes.length = 0 __resetPRConflictSummaryCachesForTests() }) @@ -300,13 +331,16 @@ describe('getPRConflictSummary caching', () => { it('preserves background admission across the complete WSL derivation chain', async () => { mockGitDispatch() + // Known support adds the quarantine's common-dir read to the chain. + getLocalGitCapabilityCache({ wslDistro: 'Ubuntu' }).rememberSupported('merge-tree-write-tree') await getPRConflictSummary('/repo-root', 'main', 'github-base-oid', 'head-oid-1', { wslDistro: 'Ubuntu', admissionTier: 'background' }) - expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(5) + expect(spawnCount(COMMON_DIR_COMMAND)).toBe(1) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(6) for (const [, options] of gitExecFileAsyncMock.mock.calls) { expect(options).toEqual( expect.objectContaining({ @@ -365,4 +399,62 @@ describe('getPRConflictSummary caching', () => { expect(spawnCount('merge-base')).toBe(2) expect(spawnCount('merge-tree')).toBe(1) }) + + it('makes no scratch folder for a Git that rejects merge-tree --write-tree, retries included', async () => { + // Git 2.25's exact rejection of both argument shapes. + const rejected = Object.assign(new Error('Command failed: git merge-tree --write-tree'), { + code: 129, + stdout: '', + stderr: 'usage: git merge-tree \n' + }) + mockGitDispatch({ 'merge-tree': () => Promise.reject(rejected) }) + + await expect(deriveSummary('head-oid-1')).resolves.toBeUndefined() + vi.setSystemTime(1_750_000_000_000 + GIT_CAPABILITY_RETRY_INTERVAL_MS + 1_000) + await expect(deriveSummary('head-oid-2')).resolves.toBeUndefined() + + expect(spawnCount('merge-tree')).toBe(2) + expect(spawnCount(COMMON_DIR_COMMAND)).toBe(0) + expect(scratchFolderPrefixes).toEqual([]) + expect(gitExecFileAsyncMock.mock.calls.filter(([, options]) => options?.env)).toEqual([]) + }) + + it('runs the legacy merge-tree fallback against the scratch object store too', async () => { + const commonDir = join( + realpathSync(mkdtempSync(join(tmpdir(), 'orca-conflict-summary-'))), + '.git' + ) + mkdirSync(join(commonDir, 'objects'), { recursive: true }) + try { + mockGitDispatch({ + [COMMON_DIR_COMMAND]: async () => ({ stdout: `${commonDir}\n` }), + 'merge-tree': async (argv) => { + if (argv.includes('--merge-base')) { + throw Object.assign(new Error("error: unknown option `merge-base'"), { stderr: '' }) + } + return { stdout: 'tree-oid\u0000src/conflict.ts\u0000' } + } + }) + + // The first check learns that this Git writes trees; the next one is quarantined. + await expect(deriveSummary('head-oid-1')).resolves.toEqual(expectedSummary) + await expect(deriveSummary('head-oid-2')).resolves.toEqual(expectedSummary) + + const legacy = gitExecFileAsyncMock.mock.calls.filter( + ([argv]) => argv[0] === 'merge-tree' && !argv.includes('--merge-base') + ) + expect(legacy).toHaveLength(2) + expect(legacy[0]?.[1]?.env).toBeUndefined() + expect(legacy[1]?.[1]?.env).toMatchObject({ + GIT_OBJECT_DIRECTORY: expect.stringContaining( + join(commonDir, 'objects', 'tmp_objdir-orca-merge-tree-') + ), + GIT_ALTERNATE_OBJECT_DIRECTORIES: join(commonDir, 'objects') + }) + expect(spawnCount('merge-tree')).toBe(3) + expect(readdirSync(join(commonDir, 'objects'))).toEqual([]) + } finally { + rmSync(join(commonDir, '..'), { recursive: true, force: true }) + } + }) }) diff --git a/src/main/github/conflict-summary.ts b/src/main/github/conflict-summary.ts index 470f85733676..ff19a692f5e3 100644 --- a/src/main/github/conflict-summary.ts +++ b/src/main/github/conflict-summary.ts @@ -5,6 +5,7 @@ import { } from '../../shared/git-merge-tree-capability' import { gitExecFileAsync } from '../git/runner' import { gitOptionsForWorktree, type GitRuntimeOptions } from '../git/git-runtime-options' +import { createLocalGitObjectQuarantine } from '../git/local-git-object-quarantine' import { clearGitCapabilityStateForTests, withLocalGitCapabilityCacheForExecution @@ -234,20 +235,31 @@ async function loadConflictingFiles( headOid, baseOid ] + // Why: only the file list is read; the merged tree it writes would pile up as loose objects. + const quarantine = createLocalGitObjectQuarantine(repoPath, localGitOptions) return withLocalGitCapabilityCacheForExecution( { cwd: repoPath, wslDistro: localGitOptions.wslDistro }, - (capabilities) => - capabilities.runWithFallback( + (capabilities) => { + const runMergeTree = (args: string[]): Promise<{ stdout: string }> => { + const run = (env: NodeJS.ProcessEnv | undefined) => + gitExecFileAsync(args, { + ...gitOptionsForWorktree(repoPath, localGitOptions), + ...(env ? { env } : {}) + }) + // Why only once supported: an old Git still gets the folder, and Git < 2.35 can't prune a leftover. + return capabilities.isKnownSupported('merge-tree-write-tree') + ? quarantine.run(run) + : run(undefined) + } + return capabilities.runWithFallback( 'merge-tree-write-tree', () => capabilities.runWithFallback( 'merge-tree-merge-base', async () => { try { - const result = await gitExecFileAsync(modernArgs, { - ...gitOptionsForWorktree(repoPath, localGitOptions) - }) + const result = await runMergeTree(modernArgs) return parseMergeTreeNameOnlyOutput(result.stdout) } catch (error) { if (isUnsupportedMergeTreeWriteTreeError(error)) { @@ -262,7 +274,7 @@ async function loadConflictingFiles( throw error } }, - () => loadConflictingFilesWithLegacyMergeTree(repoPath, legacyArgs, localGitOptions), + () => loadConflictingFilesWithLegacyMergeTree(runMergeTree, legacyArgs), isUnsupportedMergeTreeMergeBaseError ), async () => { @@ -272,18 +284,16 @@ async function loadConflictingFiles( }, isUnsupportedMergeTreeWriteTreeError ) + } ) } async function loadConflictingFilesWithLegacyMergeTree( - repoPath: string, - legacyArgs: string[], - localGitOptions: LocalGitExecOptions + runMergeTree: (args: string[]) => Promise<{ stdout: string }>, + legacyArgs: string[] ): Promise { try { - const result = await gitExecFileAsync(legacyArgs, { - ...gitOptionsForWorktree(repoPath, localGitOptions) - }) + const result = await runMergeTree(legacyArgs) return parseMergeTreeNameOnlyOutput(result.stdout) } catch (fallbackError) { const fallbackStdout = getGitErrorOutput(fallbackError, 'stdout') diff --git a/src/shared/git-binary-compatibility.test.ts b/src/shared/git-binary-compatibility.test.ts index c61efa3a7ee8..0ca1d658966f 100644 --- a/src/shared/git-binary-compatibility.test.ts +++ b/src/shared/git-binary-compatibility.test.ts @@ -1,4 +1,14 @@ -import { mkdir, mkdtemp, readFile, realpath, rm, unlink, utimes, writeFile } from 'node:fs/promises' +import { + mkdir, + mkdtemp, + readdir, + readFile, + realpath, + rm, + unlink, + utimes, + writeFile +} from 'node:fs/promises' import { tmpdir } from 'node:os' import { dirname, join } from 'node:path' import { afterAll, beforeAll, describe, expect, it } from 'vitest' @@ -7,6 +17,15 @@ import { isUnsupportedMergeTreeWriteTreeError } from './git-merge-tree-capability' import { isBranchCheckedOutInWorktreeError } from './git-branch-delete-refusal' +import { + branchHasNoUnmergedChangesOnAnyTarget, + type GitBranchCleanupExec +} from './git-branch-cleanup' +import { GitCapabilityCache } from './git-capability-cache' +import { + createGitObjectQuarantine, + GIT_OBJECT_QUARANTINE_DIR_PREFIX +} from './git-object-quarantine' import { isForEachRefExcludeUnsupportedError } from './git-ref-command-capabilities' import { isNoWriteFetchHeadUnsupportedError } from './git-fetch-head-capability' import { @@ -549,6 +568,42 @@ describeBinaryCompatibility('real Git binary compatibility', () => { } }) + // Why pin this: a scratch folder made around a Git that rejects --write-tree can outlive a crash, + // and Git before 2.35 never prunes it; only a Git known to write trees gets one. + it('makes a merge-tree scratch object store only once this Git is known to write trees', async () => { + const branch = 'compat-cleanup-merged' + await runGit(['branch', branch, 'HEAD']) + const objectsPath = join(repoPath, '.git', 'objects') + const quarantine = createGitObjectQuarantine(async () => ({ + hostPath: objectsPath, + gitPath: image ? '/repo/.git/objects' : objectsPath + })) + let scratchFolders = 0 + const runCleanupGit: GitBranchCleanupExec = (args, options) => + options?.discardWrittenObjects + ? quarantine.run((env) => { + scratchFolders += env ? 1 : 0 + return runGit(args, env) + }) + : runGit(args) + const capabilities = new GitCapabilityCache() + const check = () => + branchHasNoUnmergedChangesOnAnyTarget(runCleanupGit, branch, ['HEAD'], capabilities) + + try { + await expect(check()).resolves.toBe(supports(2, 38)) + await expect(check()).resolves.toBe(supports(2, 38)) + expect(scratchFolders).toBe(supports(2, 38) ? 1 : 0) + expect( + (await readdir(objectsPath)).filter((entry) => + entry.startsWith(GIT_OBJECT_QUARANTINE_DIR_PREFIX) + ) + ).toEqual([]) + } finally { + await runGit(['branch', '-D', branch]) + } + }) + it('supports exact show-ref probes', async () => { const head = (await runGit(['rev-parse', 'HEAD'])).stdout.trim() const originRef = 'refs/remotes/origin/compat-exact' diff --git a/src/shared/git-branch-cleanup.test.ts b/src/shared/git-branch-cleanup.test.ts index ab59cdb8bb88..5a35ba63a97c 100644 --- a/src/shared/git-branch-cleanup.test.ts +++ b/src/shared/git-branch-cleanup.test.ts @@ -107,6 +107,69 @@ describe('branchHasNoUnmergedChangesOnAnyTarget', () => { expect(runGit).toHaveBeenCalledWith(['patch-id', '--stable'], { stdin: 'branch-diff' }) expect(runGit).toHaveBeenCalledWith(['patch-id', '--stable'], { stdin: 'squash-diff' }) expect(runGit).not.toHaveBeenCalledWith(['cherry', '-v', 'target', 'refs/heads/feature/test']) + // Only merge-tree's throwaway tree writes go to a scratch object store. + const discarding = vi + .mocked(runGit) + .mock.calls.filter(([, options]) => options?.discardWrittenObjects) + .map(([args]) => args[0]) + expect(discarding.length).toBeGreaterThan(0) + expect(new Set(discarding)).toEqual(new Set(['merge-tree'])) + }) + + describe('a merge-tree that exits 1', () => { + function exitOne(stdout: string): Error { + return Object.assign(new Error('Command failed: git merge-tree --write-tree'), { + code: 1, + stdout, + stderr: '' + }) + } + + async function scratchStoreRequestsOverTwoChecks(failure: Error): Promise { + // No branch-only merge commits, so each check runs merge-tree exactly once. + const runGit = baseProofResponses({ + 'merge-tree --write-tree target refs/heads/feature/test': failure, + 'rev-list --right-only --merges --count target...refs/heads/feature/test': '0\n' + }) + const capabilities = new GitCapabilityCache() + const check = () => + branchHasNoUnmergedChangesOnAnyTarget( + runGit, + 'feature/test', + ['refs/remotes/origin/main'], + capabilities + ) + + await expect(check()).resolves.toBe(false) + await expect(check()).resolves.toBe(false) + + return vi + .mocked(runGit) + .mock.calls.filter(([args]) => args[0] === 'merge-tree') + .map(([, options]) => options?.discardWrittenObjects) + } + + it.each([ + ['SHA-1', 'a'.repeat(40)], + ['SHA-256', 'b'.repeat(64)] + ])( + 'learns support from a conflict (%s tree ID), so the next check asks for a scratch store', + async (_label, treeId) => { + const conflict = exitOne(`${treeId}\n100644 ${'c'.repeat(40)} 1\tshared.txt\n\n`) + + await expect(scratchStoreRequestsOverTwoChecks(conflict)).resolves.toEqual([false, true]) + } + ) + + it.each([ + ['empty stdout', ''], + ['stdout that is not a tree ID', 'merged-tree\n'] + ])('does not record support from %s', async (_label, stdout) => { + await expect(scratchStoreRequestsOverTwoChecks(exitOne(stdout))).resolves.toEqual([ + false, + false + ]) + }) }) it('preserves a branch with merge commits when no target squash commit matches', async () => { diff --git a/src/shared/git-branch-cleanup.ts b/src/shared/git-branch-cleanup.ts index 6766bb97d343..3a9d7375b0dc 100644 --- a/src/shared/git-branch-cleanup.ts +++ b/src/shared/git-branch-cleanup.ts @@ -1,9 +1,15 @@ import type { GitCapabilityCache } from './git-capability-cache' import { isUnsupportedMergeTreeWriteTreeError } from './git-merge-tree-capability' +export type GitBranchCleanupExecOptions = { + stdin?: string + /** The command's object writes are throwaway; run it against a scratch object store. */ + discardWrittenObjects?: boolean +} + export type GitBranchCleanupExec = ( argv: string[], - options?: { stdin?: string } + options?: GitBranchCleanupExecOptions ) => Promise<{ stdout: string }> const SQUASH_PATCH_SCAN_LIMIT = 200 @@ -106,6 +112,15 @@ async function hasBranchOnlyMergeCommits( return Number(stdout ?? 0) > 0 } +/** `merge-tree --write-tree` exits 1 on a conflict after printing the merged tree's ID first. */ +function printedMergedTreeId(error: unknown): boolean { + if (typeof error !== 'object' || error === null || !('stdout' in error)) { + return false + } + const firstLine = typeof error.stdout === 'string' ? error.stdout.split('\n', 1)[0].trim() : '' + return /^(?:[0-9a-f]{40}|[0-9a-f]{64})$/.test(firstLine) +} + async function branchMergesWithoutTreeChanges( runGit: GitBranchCleanupExec, targetOid: string, @@ -117,7 +132,19 @@ async function branchMergesWithoutTreeChanges( try { return await capabilities.runWithFallback( 'merge-tree-write-tree', - async () => (await runGit(args)).stdout.trim() || null, + async () => { + // Why only once supported: an old Git still gets the folder, and Git < 2.35 can't prune a leftover. + const discardWrittenObjects = capabilities.isKnownSupported('merge-tree-write-tree') + try { + return (await runGit(args, { discardWrittenObjects })).stdout.trim() || null + } catch (error) { + // Why: a conflict still proves --write-tree ran; returning lets the cache record it. + if (printedMergedTreeId(error)) { + return null + } + throw error + } + }, async () => null, isUnsupportedMergeTreeWriteTreeError ) diff --git a/src/shared/git-object-quarantine-real-git.test-fixture.ts b/src/shared/git-object-quarantine-real-git.test-fixture.ts new file mode 100644 index 000000000000..e6c0d7c5df80 --- /dev/null +++ b/src/shared/git-object-quarantine-real-git.test-fixture.ts @@ -0,0 +1,134 @@ +import { execFileSync } from 'node:child_process' +import { mkdtempSync, readdirSync, realpathSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { pathToFileURL } from 'node:url' +import { GIT_OBJECT_QUARANTINE_DIR_PREFIX } from './git-object-quarantine' + +/** + * A repository where `merge-tree --write-tree` has real work to do: + * - `feature-conflict` conflicts with `main` on `shared.txt` (checked out in a linked worktree); + * - `feature-extra` adds a file `main` lacks, so merging it produces a brand-new tree; + * - `feature-squashed` adds a file `main` later received through a separate (squash) commit. + */ +export type DivergentRepoFixture = { + repoPath: string + linkedPath: string + commonDir: string + git: (cwd: string, ...args: string[]) => string + /** Files under the real object store's `objects/xx` fan-out directories. */ + looseObjectCount: () => number + scratchDirectories: () => string[] + addWorktree: (name: string, branch: string) => string + /** A blob-less clone of the repo, so merge-tree must fetch file contents on demand. */ + createPartialClone: () => PartialCloneFixture + dispose: () => void +} + +export type PartialCloneFixture = { + clonePath: string + looseObjectCount: () => number + /** Packs in the clone's real `objects/pack`, each named by its `.idx`. */ + packCount: () => number + scratchDirectories: () => string[] +} + +function looseObjectCountIn(objectsDir: string): number { + return readdirSync(objectsDir) + .filter((entry) => /^[0-9a-f]{2}$/.test(entry)) + .reduce((count, entry) => count + readdirSync(join(objectsDir, entry)).length, 0) +} + +function scratchDirectoriesIn(objectsDir: string): string[] { + return readdirSync(objectsDir).filter((entry) => + entry.startsWith(GIT_OBJECT_QUARANTINE_DIR_PREFIX) + ) +} + +export function createDivergentRepoFixture(): DivergentRepoFixture { + const root = realpathSync(mkdtempSync(join(tmpdir(), 'orca-merge-tree-quarantine-'))) + const repoPath = join(root, 'repo') + const git = (cwd: string, ...args: string[]): string => + execFileSync('git', args, { cwd, encoding: 'utf8' }) + const write = (name: string, content: string): void => { + writeFileSync(join(repoPath, name), content) + } + const commit = (message: string): void => { + git(repoPath, 'add', '-A') + git(repoPath, 'commit', '--quiet', '-m', message) + } + + execFileSync('git', ['init', '--quiet', repoPath]) + git(repoPath, 'config', 'user.name', 'Orca Test') + git(repoPath, 'config', 'user.email', 'orca@example.test') + git(repoPath, 'config', 'commit.gpgSign', 'false') + git(repoPath, 'config', 'core.hooksPath', '.git/no-hooks') + git(repoPath, 'config', 'gc.auto', '0') + git(repoPath, 'checkout', '--quiet', '-b', 'main') + write('shared.txt', 'base\n') + commit('base') + + git(repoPath, 'checkout', '--quiet', '-b', 'feature-conflict') + write('shared.txt', 'feature\n') + write('conflict-only.txt', 'feature only\n') + commit('feature conflict') + + git(repoPath, 'checkout', '--quiet', '-b', 'feature-extra', 'main') + write('extra.txt', 'extra\n') + commit('feature extra') + + git(repoPath, 'checkout', '--quiet', '-b', 'feature-squashed', 'main') + write('squashed.txt', 'squashed\n') + commit('feature squashed') + + git(repoPath, 'checkout', '--quiet', 'main') + write('shared.txt', 'main\n') + write('main-only.txt', 'main only\n') + commit('main moves on') + write('squashed.txt', 'squashed\n') + commit('squash merge of feature-squashed') + + const linkedPath = join(root, 'linked') + git(repoPath, 'worktree', 'add', '--quiet', linkedPath, 'feature-conflict') + // Why: start from a packed store so every loose object is one the code under test wrote. + git(repoPath, 'gc', '--quiet', '--prune=now') + + const commonDir = join(repoPath, '.git') + const objectsDir = join(commonDir, 'objects') + return { + repoPath, + linkedPath, + commonDir, + git, + looseObjectCount: () => looseObjectCountIn(objectsDir), + scratchDirectories: () => scratchDirectoriesIn(objectsDir), + addWorktree: (name, branch) => { + const worktreePath = join(root, name) + git(repoPath, 'worktree', 'add', '--quiet', worktreePath, branch) + return worktreePath + }, + createPartialClone: () => { + git(repoPath, 'config', 'uploadpack.allowFilter', 'true') + const clonePath = join(root, 'partial') + git( + root, + 'clone', + '--quiet', + '--no-checkout', + '--filter=blob:none', + pathToFileURL(repoPath).href, + clonePath + ) + const cloneObjectsDir = join(clonePath, '.git', 'objects') + return { + clonePath, + looseObjectCount: () => looseObjectCountIn(cloneObjectsDir), + packCount: () => + readdirSync(join(cloneObjectsDir, 'pack')).filter((entry) => entry.endsWith('.idx')) + .length, + scratchDirectories: () => scratchDirectoriesIn(cloneObjectsDir) + } + }, + dispose: () => rmSync(root, { recursive: true, force: true }) + } +} diff --git a/src/shared/git-object-quarantine.test.ts b/src/shared/git-object-quarantine.test.ts new file mode 100644 index 000000000000..ae07ae0012cc --- /dev/null +++ b/src/shared/git-object-quarantine.test.ts @@ -0,0 +1,212 @@ +import { + chmodSync, + existsSync, + mkdirSync, + mkdtempSync, + readdirSync, + readFileSync, + rmSync, + writeFileSync +} from 'node:fs' +import type * as NodeFsPromises from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { basename, join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + createGitObjectQuarantine, + GIT_OBJECT_QUARANTINE_DIR_PREFIX, + type GitObjectQuarantineEnv +} from './git-object-quarantine' + +const { failedRenameTargets, renamedTargets } = vi.hoisted(() => { + const renamedTargets: string[] = [] + return { failedRenameTargets: new Set(), renamedTargets } +}) + +vi.mock('node:fs/promises', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + rename: async (from: string, to: string) => { + if (failedRenameTargets.has(basename(to))) { + throw Object.assign(new Error('EBUSY: resource busy or locked'), { code: 'EBUSY' }) + } + await actual.rename(from, to) + renamedTargets.push(basename(to)) + } + } +}) + +describe('createGitObjectQuarantine', () => { + let root: string + let objects: string + + beforeEach(() => { + root = mkdtempSync(join(tmpdir(), 'orca-object-quarantine-')) + objects = join(root, 'objects') + mkdirSync(join(objects, 'pack'), { recursive: true }) + }) + + afterEach(() => { + failedRenameTargets.clear() + renamedTargets.length = 0 + vi.restoreAllMocks() + rmSync(root, { recursive: true, force: true }) + }) + + const resolveNative = () => async () => ({ hostPath: objects, gitPath: objects }) + const scratchDirs = (): string[] => + readdirSync(objects).filter((entry) => entry.startsWith(GIT_OBJECT_QUARANTINE_DIR_PREFIX)) + + it.each([ + ['the objects dir is unknown', async () => undefined], + [ + 'resolving the objects dir fails', + async () => { + throw new Error('x') + } + ], + [ + 'the scratch dir cannot be created', + async () => ({ hostPath: '/no/such/objects', gitPath: '/no/such/objects' }) + ] + ])('runs the command unquarantined when %s', async (_label, resolve) => { + const command = vi.fn(async () => 'ok') + + await expect(createGitObjectQuarantine(resolve).run(command)).resolves.toBe('ok') + + expect(command).toHaveBeenCalledWith(undefined) + }) + + it.each([ + ['/plain/objects', '/plain/objects'], + ['/odd:dir/objects', '"/odd:dir/objects"'], + ['/quo"te\\dir/objects', '"/quo\\"te\\\\dir/objects"'], + ['C:\\repo\\.git\\objects', 'C:\\repo\\.git\\objects'], + ['C:\\a;b\\.git\\objects', '"C:\\\\a;b\\\\.git\\\\objects"'] + ])('spells alternates for %s so Git reads one entry', async (gitPath, expected) => { + let seen: GitObjectQuarantineEnv | undefined + + await createGitObjectQuarantine(async () => ({ hostPath: objects, gitPath })).run( + async (env) => { + seen = env + } + ) + + expect(seen?.GIT_ALTERNATE_OBJECT_DIRECTORIES).toBe(expected) + }) + + it.each([ + ['/repo/.git/objects', '/shared/objects:/more', '/shared/objects:/more:/repo/.git/objects'], + ['C:\\repo\\.git\\objects', 'D:\\shared', 'D:\\shared;C:\\repo\\.git\\objects'], + [ + '\\\\server\\share\\repo\\.git\\objects', + '\\\\server\\shared', + '\\\\server\\shared;\\\\server\\share\\repo\\.git\\objects' + ] + ])( + 'appends the real store to inherited alternates for %s, as Git’s own temporary stores do', + async (gitPath, inheritedAlternates, expected) => { + let seen: GitObjectQuarantineEnv | undefined + + await createGitObjectQuarantine(async () => ({ + hostPath: objects, + gitPath, + inheritedAlternates + })).run(async (env) => { + seen = env + }) + + expect(seen?.GIT_ALTERNATE_OBJECT_DIRECTORIES).toBe(expected) + } + ) + + it.each([ + ['has an alternates file', (info: string) => writeFileSync(join(info, 'alternates'), '')], + [ + 'has an alternates file this process cannot inspect', + (info: string) => { + writeFileSync(join(info, 'alternates'), '/elsewhere/objects\n') + chmodSync(info, 0o000) + } + ] + ])('runs unquarantined when the real store %s', async (_label, prepare) => { + const info = join(objects, 'info') + mkdirSync(info) + prepare(info) + const command = vi.fn(async () => 'ok') + + try { + await expect(createGitObjectQuarantine(resolveNative()).run(command)).resolves.toBe('ok') + } finally { + chmodSync(info, 0o755) + } + + expect(command).toHaveBeenCalledWith(undefined) + expect(scratchDirs()).toEqual([]) + }) + + it('moves packs Git fetched into the scratch dir to the real store, index last', async () => { + await createGitObjectQuarantine(resolveNative()).run(async (env) => { + const scratchPack = join(env?.GIT_OBJECT_DIRECTORY ?? '', 'pack') + mkdirSync(scratchPack) + for (const file of ['pack-abc1.idx', 'pack-abc1.pack', 'pack-abc1.promisor']) { + writeFileSync(join(scratchPack, file), file) + } + // A pack without its index is invisible to Git, so moving it is harmless. + writeFileSync(join(scratchPack, 'pack-def2.pack'), 'partial') + writeFileSync(join(scratchPack, 'tmp_pack_XYZ'), 'partial') + mkdirSync(join(env?.GIT_OBJECT_DIRECTORY ?? '', 'ab')) + writeFileSync(join(env?.GIT_OBJECT_DIRECTORY ?? '', 'ab', 'cdef'), 'merge-tree output') + }) + + expect(readdirSync(join(objects, 'pack')).sort()).toEqual([ + 'pack-abc1.idx', + 'pack-abc1.pack', + 'pack-abc1.promisor', + 'pack-def2.pack' + ]) + expect(renamedTargets.at(-1)).toBe('pack-abc1.idx') + expect(existsSync(join(objects, 'ab'))).toBe(false) + expect(scratchDirs()).toEqual([]) + }) + + it('leaves a pack the real store already has untouched', async () => { + writeFileSync(join(objects, 'pack', 'pack-abc1.pack'), 'installed') + writeFileSync(join(objects, 'pack', 'pack-abc1.idx'), 'installed') + + await createGitObjectQuarantine(resolveNative()).run(async (env) => { + const scratchPack = join(env?.GIT_OBJECT_DIRECTORY ?? '', 'pack') + mkdirSync(scratchPack) + writeFileSync(join(scratchPack, 'pack-abc1.pack'), 'refetched') + writeFileSync(join(scratchPack, 'pack-abc1.idx'), 'refetched') + }) + + expect(readFileSync(join(objects, 'pack', 'pack-abc1.pack'), 'utf8')).toBe('installed') + expect(readFileSync(join(objects, 'pack', 'pack-abc1.idx'), 'utf8')).toBe('installed') + }) + + it('stops at a failed pack move, still removing the scratch dir and returning the result', async () => { + failedRenameTargets.add('pack-abc1.pack') + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + + await expect( + createGitObjectQuarantine(resolveNative()).run(async (env) => { + const scratchPack = join(env?.GIT_OBJECT_DIRECTORY ?? '', 'pack') + mkdirSync(scratchPack) + for (const file of ['pack-abc1.idx', 'pack-abc1.pack']) { + writeFileSync(join(scratchPack, file), file) + } + return 'ok' + }) + ).resolves.toBe('ok') + + // The index never lands without its pack. + expect(readdirSync(join(objects, 'pack'))).toEqual([]) + expect(scratchDirs()).toEqual([]) + expect(warn).toHaveBeenCalledWith( + '[git-object-quarantine] could not keep a fetched pack', + expect.any(Error) + ) + }) +}) diff --git a/src/shared/git-object-quarantine.ts b/src/shared/git-object-quarantine.ts new file mode 100644 index 000000000000..e3cd608e9b46 --- /dev/null +++ b/src/shared/git-object-quarantine.ts @@ -0,0 +1,176 @@ +import { lstat, mkdtemp, readdir, rename, stat } from 'node:fs/promises' +import { posix, win32 } from 'node:path' +import { isWindowsAbsolutePathLike } from './cross-platform-path' +import { removeTree } from './windows-transient-lock-removal' + +/** + * Runs a Git command whose object writes are throwaway (`merge-tree --write-tree` + * only wants its stdout) against a scratch object directory, so nothing it + * writes lands in the repository's real object store as unreachable loose + * objects. Reads still see every real object through the alternates variable. + */ + +export type GitObjectQuarantineEnv = { + GIT_OBJECT_DIRECTORY: string + GIT_ALTERNATE_OBJECT_DIRECTORIES: string +} + +export type GitObjectsDirectory = { + /** How this process spells `objects/` when creating and deleting the scratch dir. */ + hostPath: string + /** How the Git child spells the same directory (differs for WSL). */ + gitPath: string + /** GIT_ALTERNATE_OBJECT_DIRECTORIES as Git would inherit it unquarantined. */ + inheritedAlternates?: string +} + +export type GitObjectQuarantine = { + run: (command: (env: GitObjectQuarantineEnv | undefined) => Promise) => Promise +} + +// Why inside `objects/` with a `tmp_objdir` prefix: that is where Git puts its own +// quarantine dirs, it is on the Git host for native, WSL and SSH alike, and +// `git gc` (2.35+) expires stale `tmp_*` dirs too. +export const GIT_OBJECT_QUARANTINE_DIR_PREFIX = 'tmp_objdir-orca-merge-tree-' + +/** Decided by path syntax, not by platform: a Windows main process drives WSL Git. */ +export function pathApiForGitPath(value: string): typeof posix { + return isWindowsAbsolutePathLike(value) ? win32 : posix +} + +// Why: Git splits this variable on `:` (`;` for Git for Windows) and C-unquotes a leading `"`. +function quoteAlternate(path: string, windowsGit: boolean): string { + const needsQuoting = windowsGit ? /[;"]/ : /[:"\\]/ + if (!needsQuoting.test(path)) { + return path + } + return `"${path.replace(/[\\"]/g, (char) => `\\${char}`)}"` +} + +/** + * Which inherited object-store variables a quarantine can mirror, as `GitObjectsDirectory` fields; + * undefined when it cannot: an inherited GIT_OBJECT_DIRECTORY is the store Git reads and writes. + */ +export function inheritedObjectStore( + env: Record +): Pick | undefined { + if (env.GIT_OBJECT_DIRECTORY !== undefined) { + return undefined + } + const alternates = env.GIT_ALTERNATE_OBJECT_DIRECTORIES + return alternates ? { inheritedAlternates: alternates } : {} +} + +function isMissingFileError(error: unknown): boolean { + return ( + error instanceof Error && + 'code' in error && + (error.code === 'ENOENT' || error.code === 'ENOTDIR') + ) +} + +/** + * The alternates for a quarantined run, or undefined to run unquarantined. Inherited value first, + * as Git's own temporary object dirs append. A real store with its own alternates is not + * quarantined: as an alternate instead of the primary, its chain would link one level deeper. + */ +async function quarantineAlternates(objects: GitObjectsDirectory): Promise { + const alternatesFile = pathApiForGitPath(objects.hostPath).join( + objects.hostPath, + 'info', + 'alternates' + ) + const hasAlternatesFile = await stat(alternatesFile).then( + () => true, + (error: unknown) => !isMissingFileError(error) + ) + if (hasAlternatesFile) { + return undefined + } + const windowsGit = isWindowsAbsolutePathLike(objects.gitPath) + const realStore = quoteAlternate(objects.gitPath, windowsGit) + return objects.inheritedAlternates + ? `${objects.inheritedAlternates}${windowsGit ? ';' : ':'}${realStore}` + : realStore +} + +/** + * A partial clone fetches missing blobs on demand, and Git files that download + * as a pack in the scratch dir. Keep those packs so the next check does not + * download the same blobs again; merge-tree's own writes are loose objects. + */ +async function keepFetchedPacks(scratchHostPath: string, objectsHostPath: string): Promise { + const path = pathApiForGitPath(objectsHostPath) + const scratchPackDir = path.join(scratchHostPath, 'pack') + // Why `pack-` only: Git's in-progress `tmp_pack_*` files are not usable packs. + const files = (await readdir(scratchPackDir).catch(() => [])).filter((file) => + file.startsWith('pack-') + ) + // Why `.idx` last: Git finds a pack through its index, so the rest must be in place first. + const ordered = [ + ...files.filter((file) => !file.endsWith('.idx')), + ...files.filter((file) => file.endsWith('.idx')) + ] + for (const file of ordered) { + const target = path.join(objectsHostPath, 'pack', file) + // Why skip: pack names are content hashes, and Git also leaves an existing file in place. + const exists = await lstat(target).then( + () => true, + () => false + ) + if (!exists) { + // Why no per-file catch: stopping at the first failure keeps an index from landing without its pack. + await rename(path.join(scratchPackDir, file), target) + } + } +} + +/** Resolves the objects dir once per quarantine; each run gets its own scratch dir. */ +export function createGitObjectQuarantine( + resolveObjectsDirectory: () => Promise +): GitObjectQuarantine { + let objectsDirectory: Promise | undefined + const resolveOnce = (): Promise => { + objectsDirectory ??= resolveObjectsDirectory().catch(() => undefined) + return objectsDirectory + } + + return { + async run(command) { + const objects = await resolveOnce() + const alternates = objects ? await quarantineAlternates(objects) : undefined + let scratchHostPath: string | undefined + if (objects && alternates !== undefined) { + const path = pathApiForGitPath(objects.hostPath) + scratchHostPath = await mkdtemp( + path.join(objects.hostPath, GIT_OBJECT_QUARANTINE_DIR_PREFIX) + ).catch(() => undefined) + } + if (!objects || alternates === undefined || !scratchHostPath) { + // Why: bookkeeping must not block the user's action; run unquarantined. + return command(undefined) + } + try { + return await command({ + GIT_OBJECT_DIRECTORY: pathApiForGitPath(objects.gitPath).join( + objects.gitPath, + pathApiForGitPath(scratchHostPath).basename(scratchHostPath) + ), + GIT_ALTERNATE_OBJECT_DIRECTORIES: alternates + }) + } finally { + await keepFetchedPacks(scratchHostPath, objects.hostPath).catch((error: unknown) => { + console.warn('[git-object-quarantine] could not keep a fetched pack', error) + }) + await removeTree(scratchHostPath).catch((error: unknown) => { + // Why: quarantined only once this host's Git ran --write-tree (2.38+), whose gc prunes `tmp_*`. + console.warn( + '[git-object-quarantine] could not remove scratch dir', + scratchHostPath, + error + ) + }) + } + } + } +}