perf(desktop): drop Git Credential Manager from the packaged Git runtime - #3473
Conversation
The bundled Git distribution ships Git Credential Manager and the .NET
runtime it needs. Maka cannot reach either: every git invocation sets
`credential.helper=` explicitly (`git-workspace-service.ts:1257`, `:2873`),
so the helper is not merely unused — it is switched off at the call site.
Credentials live in Maka's own `credentials.json` under the permission
contract in SECURITY.md.
Measured on two macOS arm64 packages built from the same tree, one with
this change and one without:
| | unpruned | pruned | delta |
| --- | ---: | ---: | ---: |
| `Maka.app` | 602 MB | 481 MB | **−121 MB** |
| bundled Git | 160 MB | 42 MB | −118 MB |
| `libexec/git-core` files | 414 | 191 | −223 |
The 223 files are 203 `.dll`, 16 `.dylib`, and 5 credential-manager
entry points. The `.dll` files are `PE32+ Mono/.Net assembly` — Windows
IL that macOS never loads.
Excluded by name rather than by directory because the payload is
interleaved with git's own commands inside one flat `libexec/git-core`.
That makes over-matching the risk worth testing rather than assuming, so
`verify-packaged-app` now asserts both halves: the commands Maka dispatches
to as separate programs must be present, and nothing from the .NET runtime
may be. Checking only the absence would pass just as well for an empty
directory.
Verified against both packages under the environment
`isolatedGitEnvironment` actually builds — `GIT_EXEC_PATH`,
`GIT_TEMPLATE_DIR`, `GIT_CONFIG_NOSYSTEM`, `credential.helper=` — running
every subcommand `git-workspace-service.ts` invokes:
init · config · add · commit · status · rev-parse
cat-file · for-each-ref · worktree list · worktree add
All pass on the pruned build with no warnings, and the unpruned build
passes the same set, so the comparison isolates the change rather than a
pre-existing failure. `git --version` reports 2.53.0 on both and
`share/git-core/templates` is untouched.
The assertion is scoped to POSIX. The Windows distribution has a different
layout and its own `.dll` set that git itself loads, so this exclusion does
not apply there; trimming Windows needs its own measurement first.
Fixes apache#3428
Generated-by: Claude Opus 5
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for measuring this instead of removing bundled files speculatively. The product boundary is sound: Maka disables Git credential helpers at every call site, and the Windows Git layout is correctly kept out of this POSIX filter. I found one small completeness gap in the current exclusion/verifier pair, noted inline.
AI-assisted review disclosure: OpenAI Codex performed an independent exact-head review. I verified the packaged-tree evidence, platform scope, live checks, and retained finding, and I made the final review decision.
| // and `.dylib` appear nowhere else in the distribution, and every one of the | ||
| // 16 dylibs is a .NET, Avalonia or Skia runtime library. | ||
| const GIT_CREDENTIAL_MANAGER_EXCLUDES = [ | ||
| '!libexec/git-core/*.dll', |
There was a problem hiding this comment.
[P2] These patterns only match files directly under git-core, but dugite 3.2.2 also carries nested locale DLLs such as libexec/git-core/zh-Hant/System.CommandLine.resources.dll. Electron Builder therefore leaves 13 resource DLLs (about 251 KiB), while the verifier's top-level readdir() misses them too. Could we make the DLL/dylib filters recursive, recursively scan leftovers in the verifier, and add one nested fixture? That makes the stated “remove GCM and its runtime” contract complete without broadening the platform scope.
|
One thing worth stating plainly about what CI just proved and what it did not. The passing CI does not package macOS on a pull request. If a committer would rather see that from CI before merging, the honest options are to run |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks @Joob1n — reviewed at exact head 078e98745ddd6f6f41d0e3247e6ee0abf08ea152. The core of this change is sound: no P0/P1, and the credential path is genuinely unreachable, so removing the payload does not change any user-visible behaviour.
What holds up
git-workspace-service.ts prefixes every git invocation with -c credential.helper= and -c credential.interactive=never, and configureManagedRepository additionally sets a repo-level empty credential.helper. credential.helper appears in no other file, and the bundled etc/gitconfig configures no helper either. A 401 against that configuration fails fast with fatal: unable to get password from user rather than hanging for input, and the execFile timeout backstops it. So GCM was already unreachable before this PR — dropping it is dead-weight removal, not a behaviour change.
The completeness half of assertPackagedGitIsComplete is the right instinct: a name filter over a flat directory that also holds git's own commands can over-match, and asserting only absence would pass for an empty directory too.
Two gaps, both from the same root cause — the exclusion list and the verifier are written against macOS file types (.dll / .dylib) and against the top level of libexec/git-core only. Details inline; neither is reachable at runtime, so both are P2 rather than P1.
- 13 localisation directories (
cs,de,es,fr,it,ja,ko,pl,pt-BR,ru,tr,zh-Hans,zh-Hant), each holdingSystem.CommandLine.resources.dll, plusNOTICEanduninstall.sh, survive both the exclusion globs and the verifier. - On Linux the self-contained GCM binary is removed by name, but its two native UI libraries (
libSkiaSharp.so~9.2 MiB,libHarfBuzzSharp.so~2.1 MiB) are neither excluded nor detected, since.soappears in neither the glob list nor the verifier regex.
P3, informational — the Windows distribution uses mingw64/libexec/git-core, which none of these globs match, so Windows is not trimmed at all. assertGitTree skips the assertion there and the comment states this is deliberate, so code and intent agree; flagging only so the PR title's scope is read as macOS plus part of Linux.
Sending this as a comment rather than an approval only because concrete P2 items remain; nothing here blocks the approach, and the fix is additive to the lists you already have.
This review was AI-assisted. It is not a substitute for independent human review by a committer.
| '!libexec/git-core/*.dylib', | ||
| '!libexec/git-core/git-credential-manager*', | ||
| '!libexec/git-core/createdump', | ||
| ]; |
There was a problem hiding this comment.
[P2] These globs only match the top level of libexec/git-core, and only macOS/Windows file types.
Two categories survive:
- Localisation subdirectories.
!libexec/git-core/*.dlldoes not matchlibexec/git-core/cs/System.CommandLine.resources.dll. All 13 GCM locale directories ship (~260 KiB total), together withNOTICEanduninstall.sh, which match no pattern at all. - Linux native libraries. There is no
*.soentry. On the ubuntu-x64 distribution the GCM binary itself is caught bygit-credential-manager*, butlibSkiaSharp.so(~9.2 MiB) andlibHarfBuzzSharp.so(~2.1 MiB) remain — roughly 11 MiB of the claimed saving is not actually removed there.
Also worth reconciling: the comment above says ".dll and .dylib appear nowhere else in the distribution", which holds for the flat directory but not for the locale subdirectories.
Adding '!libexec/git-core/*.so', '!libexec/git-core/*/' (or the explicit locale list), '!libexec/git-core/NOTICE' and '!libexec/git-core/uninstall.sh' closes both without changing the approach.
| // packaged Git distribution: Maka sets `credential.helper=` on every git | ||
| // invocation, so nothing can reach them. Naming the entry point rather | ||
| // than the runtime keeps this readable; `assertPackagedGitIsComplete` | ||
| // covers the rest by listing the directory. |
There was a problem hiding this comment.
[P2] "covers the rest by listing the directory" is not what the implementation does.
assertPackagedGitIsComplete filters with entry.isFile(), so directory entries are dropped before the name test ever runs — the locale subdirectories carrying System.CommandLine.resources.dll cannot be detected. The name test itself is also narrower than "the rest": NOTICE and uninstall.sh match neither branch.
The comment is the part most likely to mislead later: someone extending the exclusion list will reasonably trust that the verifier is a backstop for anything they miss, when it currently backstops only top-level .dll / .dylib / git-credential-manager*. Either narrow the wording to what is actually asserted, or widen the assertion to match the claim.
| const runtimeLeftovers = entries | ||
| .filter((entry) => entry.isFile()) | ||
| .map((entry) => entry.name) | ||
| .filter((name) => /\.(?:dll|dylib)$/u.test(name) || name.startsWith('git-credential-manager')); |
There was a problem hiding this comment.
[P2] The regex covers .dll and .dylib but not .so, so this assertion cannot fail on Linux even when the .NET/Avalonia runtime libraries are still present.
Combined with the missing *.so glob in electron-builder.config.mjs, the Linux path has no guard at either layer: nothing removes libSkiaSharp.so / libHarfBuzzSharp.so, and nothing reports that they are still there.
Extending to /\.(?:dll|dylib|so)$/u and recursing (or checking entry.isDirectory() for the locale directories) would make this match the guarantee the comment above describes.
…l too Review found the exclusion and the verifier both scanned only the top of `libexec/git-core`, and checking a freshly built package confirms it: 13 localisation directories survived, each holding one `System.CommandLine.resources.dll`, along with GCM's own `NOTICE` and `uninstall.sh`. The verifier reported that tree as clean. A top-level-only scan is exactly the shape of check that passes while the thing it guards is still there, which is worse than no check — so both sides now recurse. `.so` joins the extension globs for the Linux distribution, where GCM is self-contained and its `libSkiaSharp.so` / `libHarfBuzzSharp.so` sit beside the binary. Neither was excluded nor detected before. Git's own commands in this directory are executables and shell scripts, never `.dll` / `.dylib` / `.so`, so the extension globs still cannot reach them. Measured on a rebuilt macOS arm64 package: | | before this fix | after | | --- | ---: | ---: | | `libexec/git-core` top-level files | 191 | 176 | | subdirectories | 14 | 1 (`mergetools`, git's own) | | `.dll` / `.dylib` / `.so`, recursive | 13 | **0** | | GCM `NOTICE` / `uninstall.sh` | 2 | 0 | `git`, `git-remote-http`, `git-http-fetch` and `git-shell` are still present. The remaining size delta is small — those resource assemblies are a megabyte between them — but a verifier that says "clean" has to mean it. Generated-by: Claude Opus 5
|
Both P2 items confirmed against a freshly built package and fixed in You were right that the exclusion and the verifier both stopped at the top level. Checking a real artifact: The verifier reported that tree as clean. That is the part worth naming: a top-level-only scan is exactly the shape of check that passes while the thing it guards is still shipping, which is worse than no check at all. Both sides now recurse, and the failure message names paths relative to
Rebuilt and remeasured:
On the P3. Agreed, and I would rather the scope be read from the code than from the title. The exclusion globs and Related: the passing |
|
Correcting something I said in my previous comment, and adding the licensing check I should have run before saying it. I wrote that "git keeps its own notices in The conclusion survives, but it needed checking rather than assuming, so here is the check. The removed It attributes exactly two projects — So the notice covers only material that is no longer shipped, and removing it with that material is correct rather than merely convenient. It is also what the repository's own attribution model asks for — the root Separately, the recursive detector does not over-reach into git's own tree: |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — and thanks for taking both earlier points rather than patching just the symptom.
What this solves / how: Git Credential Manager ships inside the packaged Git runtime but the app never uses it, so it was dead weight in every build. This excludes it at package time and makes the packaging verifier prove it's actually gone.
Both findings from the previous round are addressed at this head:
- The exclusion set is now recursive (
**/*) and covers.soalongside the Windows/macOS binaries, plusNOTICEanduninstall.sh— which also picks up the locale subdirectories the earlier flat globs walked straight past. assertPackagedGitIsCompletenow recurses instead of filtering to top-level files, so directories are no longer dropped before the check runs, and the extension pattern includes.so.
That second one is the one worth calling out: the verifier and the exclusion list have to agree about what "gone" means, and previously each had its own partial idea of it. They now share the same shape, so a future addition to one is likely to be caught by the other rather than silently passing.
test and package are both green at this head.
LGTM.
AI-assisted review. This was a re-check of the two previously filed findings against the current head, not a fresh review.
Summary
The bundled Git distribution ships Git Credential Manager and the .NET runtime it needs. Maka cannot reach either: every git invocation sets
credential.helper=explicitly (git-workspace-service.ts:1257,:2873), so the helper is not merely unused — it is switched off at the call site. Credentials live in Maka's owncredentials.jsonunder the permission contract in SECURITY.md.Measured on two macOS arm64 packages built from the same tree, one with this change and one without:
Maka.applibexec/git-corefilesThe 223 files are 203
.dll, 16.dylib, and 5 credential-manager entry points. The.dllfiles arePE32+ Mono/.Net assembly— Windows IL that macOS never loads.Fixes #3428
Review focus
Excluded by name, not by directory — so over-matching is the risk. The payload is interleaved with git's own commands inside one flat
libexec/git-core; there is no directory to drop. A git that lostgit-remote-httpfails at clone time in a user's hands, not in packaging.So
verify-packaged-appasserts both halves: the commands git dispatches to as separate programs must be present, and nothing from the .NET runtime may be. Checking only the absence would pass just as well for an empty directory.The verification runs under the environment
isolatedGitEnvironmentactually builds —GIT_EXEC_PATH,GIT_TEMPLATE_DIR,GIT_CONFIG_NOSYSTEM,credential.helper=— not a bare invocation. Every subcommandgit-workspace-service.tsdrives:All pass on the pruned build with no warnings. The unpruned build passes the same set, so the comparison isolates this change rather than reporting a pre-existing failure.
git --versionis 2.53.0 on both, andshare/git-core/templatesis untouched — the exclusion only names paths underlibexec/git-core.Scoped to POSIX. The Windows distribution has a different layout (
git/cmd/git.exe) and its own.dllset that git itself loads, so neither the exclusion nor the assertion applies there. Trimming Windows needs its own measurement first, and I have not done one.Relationship to the other size items
Same origin as #3146: both are part of the 151 MB #2199 added when it bundled a hermetic Git runtime. #3146 explicitly scoped this payload out and said it would be filed separately; this is that filing, so the two do not overlap.
One correction to the original issue text, which I wrote and have since disproved: it attributed macOS packaging time to signing this payload, claiming ~78% of signing round trips over 281 executables. Both figures were wrong —
codesignsigns Mach-O, and the tree has 31 of those, not 281; the 216.dllfiles are Windows IL thatcodesigndoes not touch on macOS. The issue body carries the full correction. What survives is the size finding, which is what this PR claims.Verification
scripts/verify-packaged-app.test.mjs: 14/14npm run check:release: 87/87npm run format:check: 1570 files, cleanbiome lint .: 2509 files, cleanNot run: Windows packaging, and the DMG step — my local signing identity stops after the
.app, so the DMG delta is not measured here. The.appnumbers are from real artifacts.AI use
Select exactly one:
Tool(s) and scope: Claude Opus 5 (Claude Code) — wrote the exclusion and the packaged-tree assertion, built and compared both packages, ran the verification above. Reviewed and directed by me.
Generated-bytrailer is on the commit.Checklist
Does this PR entail a change in behavior?