Windows Git binary paths with spaces breaking review

Fixes on record

Merged to main. A stamp counts the issues a fix is linked to.

• #67612 fix(desktop): support spaced Windows Git paths in review

  merged to main

  files: apps/desktop/electron/git-review-ops.test.ts, apps/desktop/electron/git-review-ops.ts

  Show the diff

  • apps/desktop/electron/git-review-ops.test.ts

    @@ -6,7 +6,7 @@ import path from 'node:path'
     
     import { afterEach, test } from 'vitest'
     
    -import { repoStatus, resolveRenamePath } from './git-review-ops'
    +import { gitFor, repoStatus, resolveRenamePath } from './git-review-ops'
     
     const tempDirs: string[] = []
     
    @@ -34,6 +34,30 @@ test('resolveRenamePath: plain path is unchanged', () => {
       assert.equal(resolveRenamePath('src/a.ts'), 'src/a.ts')
     })
     
    +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 runs git through a spaced binary path', async () => {
    +  if (process.platform !== 'win32') {
    +    return
    +  }
    +
    +  const gitBin = path.join(process.env.ProgramFiles || String.raw`C:\Program Files`, 'Git', 'cmd', 'git.exe')
    +
    +  if (!fs.existsSync(gitBin)) {
    +    return
    +  }
    +
    +  const repo = makeRepo()
    +
    +  fs.writeFileSync(path.join(repo, 'changed.txt'), 'review me\n')
    +
    +  const status = await gitFor(repo, gitBin).status()
    +
    +  assert.equal(status.not_added.includes('changed.txt'), true)
    +})
    +
     test('resolveRenamePath: simple rename resolves to the new path', () => {
       assert.equal(resolveRenamePath('old.ts => new.ts'), 'new.ts')
     })

  • apps/desktop/electron/git-review-ops.ts

    @@ -43,7 +43,20 @@ function runGh(args, cwd, ghBin): Promise<{ ok: boolean; stdout: string }> {
     }
     
     function gitFor(cwd, gitBin) {
    -  return simpleGit({ baseDir: cwd, binary: gitBin || 'git', maxConcurrentProcesses: 4, trimmed: false })
    +  // `gitBin` is resolved inside the Electron main process from known install
    +  // locations or PATH — never renderer/user input. simple-git's custom-binary
    +  // validation rejects paths containing spaces (the default Windows install is
    +  // `C:\Program Files\Git\cmd\git.exe`), which silently broke the Review pane.
    +  // 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 } } : {})
    +  })
     }
     
     // simple-git reports renames as `old => new` (and `dir/{old => new}/f`); resolve
    @@ -680,6 +693,7 @@ async function repoStatus(repoPath, gitBin) {
     export {
       branchBase,
       fileDiffVsHead,
    +  gitFor,
       repoStatus,
       resolveRenamePath,
       reviewCommit,