fix(desktop): handle Windows simple-git binary paths with spaces

[tuancookiez-hub]

What does this PR do?

Fixes a Windows-only bug where the desktop review pane shows "No diff to show" because simple-git rejects binary paths containing spaces (e.g. C:\Program Files\Git\cmd\git.exe — the default Git for Windows install path).

resolveGitBinary() in main.ts correctly resolves the git binary to an absolute path. When that path contains spaces, simple-git's internal spawn fails silently — every git.status(), git.diffSummary(), etc. call throws, and the review pane's catch blocks return empty results.

Root Cause

gitFor() in apps/desktop/electron/git-review-ops.ts passes the resolved binary path directly to simpleGit():

// Before
function gitFor(cwd, gitBin) {
  return simpleGit({ baseDir: cwd, binary: gitBin || 'git', maxConcurrentProcesses: 4, trimmed: false })
}

simple-git does not quote the binary path when spawning, so a space in C:\Program Files\Git\... breaks the spawn.

Fix

Guard against spaces in the binary path — fall back to bare 'git' (which Node resolves via PATH):

// After
function gitFor(cwd, gitBin) {
  const binary = gitBin && !gitBin.includes(' ') ? gitBin : 'git'
  return simpleGit({ baseDir: cwd, binary, maxConcurrentProcesses: 4, trimmed: false })
}

On Windows, git is always on PATH regardless of install location (the installer adds it), so the fallback is safe. On macOS/Linux, resolveGitBinary() returns a PATH-resolved 'git' (no spaces), so behavior is unchanged.

Note: the two execFile(gitBin || 'git', ...) calls in the same file are not affected — Node's execFile properly quotes arguments with spaces.

Related Issue

Closes #64810

Type of Change

• Bug fix (non-breaking change which fixes an issue)

How to Test

Reproduction (Windows, before fix):

1. Install Git for Windows to the default path (C:\Program Files\Git\)
2. Open Hermes Desktop, open a git repo workspace
3. Press Ctrl+G to open the review pane
4. Observe: "No diff to show" for all files

After fix:

• Review pane shows files and diffs normally.

Non-Windows / PATH-only: No change in behavior — git is still resolved from PATH.

Checklist

• Code follows the style guide (single-word names, no else, early returns)
• Change is surgical — one function, one guard clause
• No new env vars or config surface

[teknium1]

Thanks for identifying the simple-git validation failure — the premise is confirmed on current main: gitFor() passes gitBin directly to simple-git (apps/desktop/electron/git-review-ops.ts:45-46), and the locked simple-git 3.36.0 supports unsafe.allowUnsafeCustomBinary for this case.

Problems

• The new fallback to bare git loses the resolved executable. resolveGitBinary() explicitly supports Hermes PortableGit installations that are never on PATH (apps/desktop/electron/main.ts:1829-1864), so this can replace a valid space-containing binary with an unavailable command.
• apps/desktop/electron/git-review-ops.test.ts:5-21 has no coverage for a space-containing custom binary path.

Suggested changes

• Preserve gitBin and enable simple-git's documented unsafe.allowUnsafeCustomBinary option for the resolver-provided binary instead of falling back to PATH.
• Add a focused construction regression test for C:\\Program Files\\Git\\cmd\\git.exe.

Automated hermes-sweeper review.

[teknium1]

This fallback breaks the resolver's PortableGit path: resolveGitBinary() intentionally selects %LOCALAPPDATA%\\hermes\\git before PATH and documents that it is never on PATH (apps/desktop/electron/main.ts:1829-1864). Preserve gitBin and use simple-git's unsafe.allowUnsafeCustomBinary option instead.

[teknium1]

This is now fixed on main via PR #67612 (merged commit e361c5e). The landed fix takes the same direction you identified — keep the internally resolved absolute binary and use simple-git's unsafe.allowUnsafeCustomBinary escape hatch, gated to whitespace-containing paths in gitFor().

Thanks for staying on this bug across both iterations (#60156 and this v2) — credit to you, @unsupportedpastels (#64713, whose commit landed with authorship preserved), and @wenyi-xydigit (#55337, first submitter) for converging on the fix.

[wenyi-xydigit]

This is now fixed on main via PR #67612 (merged commit e361c5e). The landed fix takes the same direction you identified — keep the internally resolved absolute binary and use simple-git's unsafe.allowUnsafeCustomBinary escape hatch, gated to whitespace-containing paths in gitFor().

Thanks for staying on this bug across both iterations (#60156 and this v2) — credit to you, @unsupportedpastels (#64713, whose commit landed with authorship preserved), and @wenyi-xydigit (#55337, first submitter) for converging on the fix.

Thank you for the update and for the kind shout-out! I'm really glad to see this fix land on main.

A big thanks to @unsupportedpastels for taking the final implementation across the finish line, and to the maintainers for reviewing and merging it. It's great to see the community collaborate on this — from the initial report (#55337) through the iterations to a solid resolution.

I'll keep an eye out for the next release to verify the fix in action. Thanks again to everyone involved!