Cut Windows wrapper overhead for read-only and small commands#809
Cut Windows wrapper overhead for read-only and small commands#809
Conversation
1. try_find_repository_no_git_exec: catch construction errors from repository_from_discovered_paths and return Ok(None) so the git-exec fallback in find_repository still runs. Previously, errors like canonicalize failures or bad core.worktree config would propagate and silently skip hooks. 2. is_read_only_branch_invocation: split list-mode triggers from list-output modifiers (-v, --verbose, --no-color, --no-column, --ignore-case). Only triggers should classify as read-only when positional args are present. `git branch -v feature` creates a branch — it is NOT read-only. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When GIT_DIR, GIT_WORK_TREE, or GIT_CEILING_DIRECTORIES environment variables are set, the filesystem-based fast discovery path may find a different repository than git's own discovery logic. Fall back to the git-exec path (git rev-parse) which honours these env vars. Addresses Devin review feedback on PR #809. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When GIT_DIR, GIT_WORK_TREE, or GIT_CEILING_DIRECTORIES environment variables are set, the filesystem-based fast discovery path may find a different repository than git's own discovery logic. Fall back to the git-exec path (git rev-parse) which honours these env vars. Addresses Devin review feedback on PR #809. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1. try_find_repository_no_git_exec: catch construction errors from repository_from_discovered_paths and return Ok(None) so the git-exec fallback in find_repository still runs. Previously, errors like canonicalize failures or bad core.worktree config would propagate and silently skip hooks. 2. is_read_only_branch_invocation: split list-mode triggers from list-output modifiers (-v, --verbose, --no-color, --no-column, --ignore-case). Only triggers should classify as read-only when positional args are present. `git branch -v feature` creates a branch — it is NOT read-only. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When GIT_DIR, GIT_WORK_TREE, or GIT_CEILING_DIRECTORIES environment variables are set, the filesystem-based fast discovery path may find a different repository than git's own discovery logic. Fall back to the git-exec path (git rev-parse) which honours these env vars. Addresses Devin review feedback on PR #809. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
c04e950 to
d6ae99e
Compare
Add high-volume read-only commands that IDEs (VS Code, JetBrains),
git clients (GitLens, Graphite CLI), and other tools call frequently:
Unconditionally read-only (added to is_definitely_read_only_command):
ls-remote, show-ref, cherry, show-branch, whatchanged, verify-pack,
check-ref-format, fsck, column, fmt-merge-msg, get-tar-commit-id,
patch-id, stripspace
Subcommand classifiers (added to is_read_only_invocation):
symbolic-ref: read when 1 positional, write when 2+, reject -d/--delete
reflog: deny-list (only expire/delete mutate); catches implicit show
with refs/flags like `git reflog HEAD`, `git reflog --all`
notes: allow-list (list/show/get-ref); must be conservative because
--ref flag can precede the subcommand
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When GIT_DIR, GIT_WORK_TREE, or GIT_CEILING_DIRECTORIES environment variables are set, the filesystem-based fast discovery path may find a different repository than git's own discovery logic. Fall back to the git-exec path (git rev-parse) which honours these env vars. Addresses Devin review feedback on PR #809. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
d6ae99e to
5d8e67a
Compare
- Revert Clap removal from main.rs to preserve help/usage behavior - Restore is_repo_creating early-exit for clone/init in async mode that was dropped during the refactor, preventing the daemon from receiving misleading wrapper state for repo-creating commands - Port async_mode_post_commit_shows_stats_with_commit_alias test from PR #801 which is now subsumed by this PR's alias resolution changes - Fix clippy match_like_matches_macro warning in is_read_only_notes_invocation Co-Authored-By: Sasha Varlamov <sasha@sashavarlamov.com>
|
|
Co-Authored-By: Sasha Varlamov <sasha@sashavarlamov.com>
…fe_to_rebase into normalize_global_args_for_repo_root, DRY resolve_command_base_dir Co-Authored-By: Sasha Varlamov <sasha@sashavarlamov.com>
… non-async mode Co-Authored-By: Sasha Varlamov <sasha@sashavarlamov.com>
Co-Authored-By: Sasha Varlamov <sasha@sashavarlamov.com>
| command_args_contain_any(&parsed.command_args, &list_mode_triggers) | ||
| || parsed.pos_command(0).is_none() | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 pos_command ignores -- separator, causing false read-only classification for dash-prefixed branch/tag names
The is_read_only_branch_invocation and is_read_only_tag_invocation classifiers use parsed.pos_command(0).is_none() to distinguish list mode (read-only) from creation mode (mutating). However, pos_command at src/git/cli_parser.rs:60-96 does not handle the -- end-of-options separator — it treats -- and any subsequent dash-prefixed arguments as flags to skip rather than positional arguments. This means git branch -- -v (which creates a branch named "-v") is incorrectly classified as read-only and bypasses all git-ai hooks, losing attribution tracking for that commit.
Example trace for git branch -- -v
command_args = ["--", "-v"]
- command_args_contain_any(mutating_flags): no match
- command_args_contain_any(list_mode_triggers): no match
- pos_command(0): "--" starts with '-' → skip; "-v" starts with '-' → skip; returns None
- Result: read-only (WRONG — this creates branch "-v")
Prompt for agents
The pos_command() method in src/git/cli_parser.rs (lines 60-96) needs to handle the -- end-of-options separator. After encountering --, all remaining arguments should be treated as positional regardless of whether they start with -. Add a saw_separator boolean that flips to true when -- is encountered, and when true, treat every subsequent argument as positional. This will fix the false read-only classification in is_read_only_branch_invocation and is_read_only_tag_invocation for commands like git branch -- -v or git tag -- -v1.0.
Was this helpful? React with 👍 or 👎 to provide feedback.
…_invocation in wrapper dispatch Co-Authored-By: Sasha Varlamov <sasha@sashavarlamov.com>
Summary
status,log,diff,show,rev-parse,ls-files,blame,grep,help,version, etc.) and help/version invocations so they skip all hooks and daemon statest = status --shortuse the fast path and aliases to mutating commands do notalias.cm = commit), subsuming Fix post-commit stats not showing with git aliases #801git rev-parsesubprocesses to the existing filesystem-based fast path--git-dir/--work-treeinitpassthroughs unconditionally (no post-hooks);clonepassthroughs only in async mode sopost_clone_hookstill runs in non-async modeis_definitely_read_only_command(unconditionally read-only regardless of args) get the fast path. Mixed-mode commands likebranch,tag,stash,remote,configare NOT fast-pathed — they always go through the full wrapper. A broaderis_read_only_invocationclassifier exists incommand_classification.rswith per-command arg inspection but is not wired into the critical dispatch path to keep risk low.Root cause
The Windows slowdown was mostly wrapper preflight overhead, not the proxied git command itself.
Before these changes, a wrapper invocation could do several expensive things before the real git command even ran:
That architecture is especially expensive on Windows, where each extra child process has noticeable startup cost.
What changed
This PR attacks the problem in three narrow ways:
Read-only passthrough classification — commands that are unconditionally read-only (the
is_definitely_read_only_commandlist), plus--helpand no-command invocations, skip all hooks and daemon state. The list only includes commands that are read-only regardless of their arguments. Mixed-mode commands (branch,tag,stash,remote,config, etc.) are not included.Alias-aware passthrough — wrapper alias expansion now happens once before the async/sync branch point and the passthrough/read-only decision. Read-only aliases (e.g.
alias.st = status --short) get the fast path; mutating aliases still go through the full wrapper path. This also fixes the bug where commit aliases (e.g.alias.cm = commit) didn't trigger post-commit stats display in async mode.Shared wrapper preflight —
find_repository()now uses the existing filesystem-based discovery fast path for common global-arg shapes instead of immediately shelling out togit rev-parse. Falls back to git-exec discovery whenGIT_DIR,GIT_WORK_TREE, orGIT_CEILING_DIRECTORIESenv vars are set. Internal helper git subprocesses now force all trace2 streams off. Repo-root normalization preserves original args when they contain flags that aren't safe to rebase (e.g. relative--git-dir/--work-tree).Benchmarks
All numbers below are from the same Windows machine/repo style, using stopwatch timing around the wrapper binary.
git status(forced sync wrapper path)git branch --show-currentPre-broader-fix wrapper warm runs:
Patched wrapper warm runs:
Plain git reference:
git branch <name> HEADPre-broader-fix wrapper:
Patched wrapper warm runs:
Plain git reference:
Additional passthrough cases
Warm wrapper runs after the latest startup/pass-through changes:
git --version: 116ms, 163ms, 153ms, 126msgit remote -v: 150ms, 167ms, 170ms, 201ms, 229msgit config --list --show-origin: 216ms, 376ms, 275ms, 232ms, 253msPlain git reference:
git --version: 82ms, 102ms, 94ms, 115msgit remote -v: 123ms, 165ms, 175ms, 165msgit config --list --show-origin: 166ms, 244ms, 95ms, 136ms, 147msNotes
This does not produce a literal 10x win for every command. After these changes, the remaining floor is mostly actual git work plus Windows process startup for launching the wrapper binary itself. Hitting another step-function improvement across all commands will likely require a bigger architectural move, like a thinner launcher or persistent proxy process.
Updates since last revision
main.rs— the startup trim wasn't worth changing help/usage behaviorasync_mode_post_commit_shows_stats_with_commit_alias) — this PR's early alias resolution subsumes Fix post-commit stats not showing with git aliases #801 entirelymatch_like_matches_macrowarning inis_read_only_notes_invocationshould_passthrough_read_only_command()andresolve_wrapper_invocation()which were single-line wrappers; alias resolution and read-only checks are now inline inhandle_git()normalize_global_args_for_repo_root— merged the separateglobal_args_are_safe_to_rebase()guard into the normalization loop itself (single pass: normalize-Cargs and bail if an unsafe flag is encountered). DRY'dresolve_command_base_dirto reuseapply_global_c_arg.post_clone_hook— the clone/init early-exit was unconditional, which madepost_clone_hookunreachable in non-async mode. Nowinitpassthroughs unconditionally (it has no post-hooks) andclonepassthroughs only inside theasync_modeblock so the non-async clone handler withpost_clone_hookis still reached.is_definitely_read_only_commandonly — removedis_read_only_invocation(the broader arg-inspecting classifier) from the wrapper dispatch path. The fast path now only catches commands that are unconditionally read-only by name. Mixed-mode commands (branch,tag,config,stash, etc.) always go through the full wrapper. This trades a small amount of fast-path coverage for significantly less risk and complexity in the critical dispatch path.Validation
cargo test --package git-ai passthrough_read_only_command -- --nocapturecargo test --package git-ai wrapper_resolves_ -- --nocapturecargo test --package git-ai resolve_fast_discovery_base_dir -- --nocapturecargo test --package git-ai normalize_global_args_for_repo_root -- --nocapturecargo test --package git-ai find_repository_preserves_relative_git_dir_and_work_tree_for_internal_commands -- --nocapturecargo test --package git-ai find_repository_in_path_supports_bare_repositories -- --nocapturecargo test --package git-ai find_repository_in_path_worktree_uses_common_dir_for_isolated_storage -- --nocapturecargo test --package git-ai test_empty_allowlist_allows_everything -- --nocapturecargo test --package git-ai --test async_mode async_mode_post_commit_shows_stats_with_commit_alias -- --nocapturecargo test --package git-ai --test notes_sync_regression notes_sync_clone_fetches_authorship_notes_from_origin -- --nocapturecargo clippy --package git-ai -- -D warningscargo build --releaseReview & Testing Checklist for Human
This PR touches the wrapper's main dispatch path — every git invocation goes through it. 4 items to verify:
initexits unconditionally,cloneexits only in async mode. Verifygit clone <url>in non-async mode still runspost_clone_hook(fetches authorship notes from origin). Verify async-mode clone still skips daemon telemetry. The non-async clone handler also checksskip_hooks, which the async early-exit does not — confirm this difference is acceptable.git config alias.cm commit, thengit cm -m "test"with AI content and verify post-commit stats appear. The ported test covers this but manual verification on a real repo is worthwhile.normalize_global_args_for_repo_rootsingle-pass safety: The function normalizes and bails mid-loop if an unsafe flag is encountered, returning the original args. Verify that bailing mid-loop is equivalent to the previous two-pass behavior in all edge cases — particularly when-Cargs appear before and after an unsafe flag.Notes
proxy_to_gitstill re-parses args for trace2 suppression (minor redundancy, not a correctness issue).is_read_only_invocationclassifier and its per-command sub-classifiers (is_read_only_branch_invocation,is_read_only_tag_invocation, etc.) remain incommand_classification.rsas tested library code but are not used in the critical wrapper dispatch path. They can be wired in later if desired after more validation.Link to Devin session: https://app.devin.ai/sessions/7bcaf7c2296b4a4699df61a164ed5986
Requested by: @svarlamov