Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions apps/desktop/electron/git-review-ops.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,43 @@ test('gitFor accepts an internally resolved git binary path containing spaces',
assert.doesNotThrow(() => gitFor(process.cwd(), 'C:\\Program Files\\Git\\cmd\\git.exe'))
})

test('gitFor does not log simple-git custom-binary warnings for a spaced binary path', () => {
const warnings: unknown[][] = []
const original = console.warn

console.warn = (...args: unknown[]) => {
warnings.push(args)
}

try {
for (let i = 0; i < 5; i += 1) {
gitFor(process.cwd(), 'C:\\Program Files\\Git\\cmd\\git.exe')
}
} finally {
console.warn = original
}

assert.deepEqual(warnings, [])
})

test('gitFor restores console.warn so unrelated warnings still surface', () => {
const warnings: unknown[][] = []
const original = console.warn

console.warn = (...args: unknown[]) => {
warnings.push(args)
}

try {
gitFor(process.cwd(), 'C:\\Program Files\\Git\\cmd\\git.exe')
console.warn('unrelated warning')
} finally {
console.warn = original
}

assert.deepEqual(warnings, [['unrelated warning']])
})

test('gitFor runs git through a spaced binary path', async () => {
if (process.platform !== 'win32') {
return
Expand Down
47 changes: 40 additions & 7 deletions apps/desktop/electron/git-review-ops.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,34 @@ function runGh(args, cwd, ghBin): Promise<{ ok: boolean; stdout: string }> {
})
}

// simple-git logs this to `console.warn` on EVERY construction that opts into
// `unsafe.allowUnsafeCustomBinary`. We opt in deliberately (see `gitFor`), so
// the warning carries no information — but the coding rail rebuilds a client on
// every status poll, which turned the desktop log into hundreds of identical
// lines and buried real errors.
const SIMPLE_GIT_UNSAFE_BINARY_WARNING = 'Invalid value supplied for custom binary'

// Run `build` with simple-git's known custom-binary warning suppressed. The
// suppression is synchronous and scoped to the construction call, so unrelated
// warnings (including any emitted later by the returned client) still surface.
function withoutUnsafeBinaryWarning<T>(build: () => T): T {
const original = console.warn

console.warn = (...args: unknown[]) => {
if (typeof args[0] === 'string' && args[0].startsWith(SIMPLE_GIT_UNSAFE_BINARY_WARNING)) {
return
}

original.apply(console, args as [])
}

try {
return build()
} finally {
console.warn = original
}
}

function gitFor(cwd, gitBin) {
// `gitBin` is resolved inside the Electron main process from known install
// locations or PATH — never renderer/user input. simple-git's custom-binary
Expand All @@ -51,13 +79,18 @@ function gitFor(cwd, gitBin) {
// For spaced paths, opt into simple-git's trusted-binary escape hatch instead
// of falling back to PATH (often absent in GUI-launched apps, and PATH lookup
// could resolve a repo-local git.exe).
return simpleGit({
baseDir: cwd,
binary: gitBin || 'git',
maxConcurrentProcesses: 4,
trimmed: false,
...(gitBin && /\s/.test(gitBin) ? { unsafe: { allowUnsafeCustomBinary: true } } : {})
})
const unsafeBinary = Boolean(gitBin && /\s/.test(gitBin))

const build = () =>
simpleGit({
baseDir: cwd,
binary: gitBin || 'git',
maxConcurrentProcesses: 4,
trimmed: false,
...(unsafeBinary ? { unsafe: { allowUnsafeCustomBinary: true } } : {})
})

return unsafeBinary ? withoutUnsafeBinaryWarning(build) : build()
}

// simple-git reports renames as `old => new` (and `dir/{old => new}/f`); resolve
Expand Down
Loading