diff --git a/src/commands/git_handlers.rs b/src/commands/git_handlers.rs index 937586348..d511ee369 100644 --- a/src/commands/git_handlers.rs +++ b/src/commands/git_handlers.rs @@ -110,39 +110,45 @@ pub fn handle_git(args: &[String]) { return; } - // Async mode: wrapper should behave as a pure passthrough to git, - // but capture and send authoritative pre/post state to the daemon. - if config::Config::get().feature_flags().async_mode { - let parsed = parse_git_cli_args(args); + let mut parsed_args = parse_git_cli_args(args); + + let mut repository_option = find_repository(&parsed_args.global_args).ok(); + + // Resolve aliases early so the read-only check sees the real command. + if let Some(repo) = repository_option.as_ref() + && let Some(resolved) = resolve_alias_invocation(&parsed_args, repo) + { + parsed_args = resolved; + } - // Read-only commands don't need wrapper state (the daemon fast-paths - // their trace events and never processes them through the normalizer). - // Skip the invocation_id so we can also suppress trace2 for them, - // avoiding unnecessary daemon work and wrapper_states memory leaks. - let is_read_only = parsed + // Fast-path: commands that are unconditionally read-only (status, log, diff, + // etc.) or help/version invocations can skip all hooks and daemon state. + let is_read_only = parsed_args.is_help + || parsed_args.command.is_none() + || parsed_args .command .as_deref() .is_some_and(crate::git::command_classification::is_definitely_read_only_command); - if is_read_only { - let exit_status = proxy_to_git(args, false, None, None); - exit_with_status(exit_status); - } + if is_read_only { + let exit_status = proxy_to_git(&parsed_args.to_invocation_vec(), false, None, None); + exit_with_status(exit_status); + } - // Repo-creating commands (clone, init) have no meaningful pre/post - // repo state — the target repo doesn't exist yet. The wrapper would - // either capture nothing (clone from outside a repo) or the wrong - // repo (clone from inside a different repo). Skip the invocation_id - // so the daemon doesn't wait for wrapper state that never arrives or - // is misleading; trace2 events still flow normally (trace2 suppression - // requires *both* no invocation_id and a read-only command). - let is_repo_creating = parsed - .command - .as_deref() - .is_some_and(|cmd| matches!(cmd, "clone" | "init")); + // `init` has no post-hooks, so it can always passthrough early. + if parsed_args.command.as_deref() == Some("init") && !parsed_args.is_help { + let exit_status = proxy_to_git(&parsed_args.to_invocation_vec(), false, None, None); + exit_with_status(exit_status); + } - if is_repo_creating { - let exit_status = proxy_to_git(args, false, None, None); + // Async mode: wrapper should behave as a pure passthrough to git, + // but capture and send authoritative pre/post state to the daemon. + if config::Config::get().feature_flags().async_mode { + // Clone in async mode has no meaningful pre/post repo state — the + // target repo doesn't exist yet. Skip daemon telemetry so the daemon + // doesn't wait for wrapper state that never arrives or is misleading. + if parsed_args.command.as_deref() == Some("clone") && !parsed_args.is_help { + let exit_status = proxy_to_git(&parsed_args.to_invocation_vec(), false, None, None); exit_with_status(exit_status); } @@ -153,8 +159,7 @@ pub fn handle_git(args: &[String]) { debug_log(&format!("wrapper: daemon telemetry init failed: {}", e)); } - let repository = find_repository(&parsed.global_args).ok(); - let worktree = repository.as_ref().and_then(|r| r.workdir().ok()); + let worktree = repository_option.as_ref().and_then(|r| r.workdir().ok()); let pre_state = worktree .as_deref() @@ -165,7 +170,12 @@ pub fn handle_git(args: &[String]) { // processes the atexit trace event and starts the wrapper state timeout. send_wrapper_pre_state_to_daemon(&invocation_id, worktree.as_deref(), &pre_state); - let exit_status = proxy_to_git(args, false, None, Some(&invocation_id)); + let exit_status = proxy_to_git( + &parsed_args.to_invocation_vec(), + false, + None, + Some(&invocation_id), + ); let post_state = worktree .as_deref() @@ -176,19 +186,15 @@ pub fn handle_git(args: &[String]) { // After a successful commit, wait briefly for the daemon to produce an // authorship note so we can show stats inline (same UX as plain wrapper mode). if exit_status.success() - && parsed.command.as_deref() == Some("commit") - && let Some(repo) = repository.as_ref() + && parsed_args.command.as_deref() == Some("commit") + && let Some(repo) = repository_option.as_ref() { - maybe_show_async_post_commit_stats(&parsed, repo); + maybe_show_async_post_commit_stats(&parsed_args, repo); } exit_with_status(exit_status); } - let mut parsed_args = parse_git_cli_args(args); - - let mut repository_option = find_repository(&parsed_args.global_args).ok(); - let has_repo = repository_option.is_some(); let config = config::Config::get(); @@ -227,10 +233,6 @@ pub fn handle_git(args: &[String]) { let repository = repository_option.as_mut().unwrap(); - if let Some(resolved) = resolve_alias_invocation(&parsed_args, repository) { - parsed_args = resolved; - } - let pre_command_start = Instant::now(); run_pre_command_hooks(&mut command_hooks_context, &mut parsed_args, repository); let pre_command_duration = pre_command_start.elapsed(); @@ -812,10 +814,12 @@ fn proxy_to_git( // trace2 events for the daemon to match wrapper state entries. let suppress_trace2 = wrapper_invocation_id.is_none() && { let parsed = parse_git_cli_args(args); - parsed - .command - .as_deref() - .is_some_and(crate::git::command_classification::is_definitely_read_only_command) + parsed.is_help + || parsed.command.is_none() + || parsed + .command + .as_deref() + .is_some_and(crate::git::command_classification::is_definitely_read_only_command) }; // Use spawn for interactive commands @@ -1002,7 +1006,10 @@ fn in_shell_completion_context() -> bool { #[cfg(test)] mod tests { use super::parse_alias_tokens; - use super::{parse_git_cli_args, resolve_child_git_hooks_path_override}; + use super::{ + parse_git_cli_args, resolve_alias_invocation, resolve_child_git_hooks_path_override, + }; + use crate::git::command_classification::is_definitely_read_only_command; use crate::git::find_repository_in_path; use std::process::Command; use tempfile::tempdir; @@ -1129,6 +1136,146 @@ mod tests { ); } + #[test] + fn passthrough_read_only_command_for_status() { + assert!(is_definitely_read_only_command("status")); + } + + #[test] + fn passthrough_read_only_command_rejects_mutating_commands() { + assert!(!is_definitely_read_only_command("commit")); + } + + #[test] + fn passthrough_read_only_command_accepts_help_invocations() { + let parsed = parse_git_cli_args(&["status".to_string(), "--help".to_string()]); + assert!(parsed.is_help); + } + + #[test] + fn passthrough_read_only_command_accepts_top_level_version() { + // `git --version` is rewritten to `git version` by the parser. + let parsed = parse_git_cli_args(&["--version".to_string()]); + assert!( + parsed.is_help + || parsed.command.is_none() + || parsed + .command + .as_deref() + .is_some_and(is_definitely_read_only_command) + ); + } + + #[test] + fn wrapper_resolves_read_only_alias_before_passthrough() { + let temp = tempdir().expect("tempdir should create"); + let output = Command::new("git") + .args(["init", "-q"]) + .current_dir(temp.path()) + .output() + .expect("git init should run"); + assert!( + output.status.success(), + "git init failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let alias_output = Command::new("git") + .args(["config", "alias.st", "status --short"]) + .current_dir(temp.path()) + .output() + .expect("git config alias should run"); + assert!( + alias_output.status.success(), + "git config alias failed: {}", + String::from_utf8_lossy(&alias_output.stderr) + ); + + let repo = find_repository_in_path(&temp.path().to_string_lossy()) + .expect("repository should be discovered"); + let parsed = parse_git_cli_args(&["st".to_string()]); + let resolved = resolve_alias_invocation(&parsed, &repo).unwrap_or_else(|| parsed.clone()); + + assert_eq!(resolved.command.as_deref(), Some("status")); + assert!(resolved.command_args.iter().any(|arg| arg == "--short")); + assert!(is_definitely_read_only_command( + resolved.command.as_deref().unwrap() + )); + } + + #[test] + fn wrapper_resolves_mutating_alias_before_passthrough() { + let temp = tempdir().expect("tempdir should create"); + let output = Command::new("git") + .args(["init", "-q"]) + .current_dir(temp.path()) + .output() + .expect("git init should run"); + assert!( + output.status.success(), + "git init failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let alias_output = Command::new("git") + .args(["config", "alias.ci", "commit"]) + .current_dir(temp.path()) + .output() + .expect("git config alias should run"); + assert!( + alias_output.status.success(), + "git config alias failed: {}", + String::from_utf8_lossy(&alias_output.stderr) + ); + + let repo = find_repository_in_path(&temp.path().to_string_lossy()) + .expect("repository should be discovered"); + let parsed = parse_git_cli_args(&["ci".to_string(), "-m".to_string(), "msg".to_string()]); + let resolved = resolve_alias_invocation(&parsed, &repo).unwrap_or_else(|| parsed.clone()); + + assert_eq!(resolved.command.as_deref(), Some("commit")); + assert!(!is_definitely_read_only_command( + resolved.command.as_deref().unwrap() + )); + } + + #[test] + fn wrapper_does_not_passthrough_builtin_name_shadowed_by_mutating_alias() { + let temp = tempdir().expect("tempdir should create"); + let output = Command::new("git") + .args(["init", "-q"]) + .current_dir(temp.path()) + .output() + .expect("git init should run"); + assert!( + output.status.success(), + "git init failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let alias_output = Command::new("git") + .args(["config", "alias.status", "branch aliased HEAD"]) + .current_dir(temp.path()) + .output() + .expect("git config alias should run"); + assert!( + alias_output.status.success(), + "git config alias failed: {}", + String::from_utf8_lossy(&alias_output.stderr) + ); + + let repo = find_repository_in_path(&temp.path().to_string_lossy()) + .expect("repository should be discovered"); + let parsed = parse_git_cli_args(&["status".to_string()]); + + let resolved = resolve_alias_invocation(&parsed, &repo).unwrap_or_else(|| parsed.clone()); + assert_eq!(resolved.command.as_deref(), Some("branch")); + assert!(resolved.command_args.iter().any(|arg| arg == "aliased")); + assert!(!is_definitely_read_only_command( + resolved.command.as_deref().unwrap() + )); + } + #[cfg(unix)] #[test] fn exit_status_was_interrupted_on_sigint() { diff --git a/src/config.rs b/src/config.rs index 7311d2dcc..eb32187ad 100644 --- a/src/config.rs +++ b/src/config.rs @@ -209,6 +209,10 @@ impl Config { } pub fn is_allowed_repository(&self, repository: &Option) -> bool { + if self.allow_repositories.is_empty() && self.exclude_repositories.is_empty() { + return true; + } + // Fetch remotes once and reuse for both exclude and allow checks let remotes = repository .as_ref() diff --git a/src/git/command_classification.rs b/src/git/command_classification.rs index 035caeadc..6bfbb9088 100644 --- a/src/git/command_classification.rs +++ b/src/git/command_classification.rs @@ -1,43 +1,306 @@ +use crate::git::cli_parser::ParsedGitInvocation; + /// Returns true if the given git subcommand is guaranteed to never mutate /// repository state (refs, objects, config, worktree). Used to skip expensive /// trace2 ingestion work and suppress trace2 emission for read-only commands. +/// +/// This list covers both porcelain and plumbing commands that IDEs (VS Code, +/// JetBrains), git clients (GitLens, Graphite CLI), and other tools call at +/// high frequency. Only commands that are unconditionally read-only regardless +/// of arguments belong here; commands with mixed read/write modes (symbolic-ref, +/// reflog, notes, etc.) are handled by `is_read_only_invocation` instead. pub fn is_definitely_read_only_command(command: &str) -> bool { matches!( command, + // Porcelain read-only "blame" - | "cat-file" - | "check-attr" - | "check-ignore" - | "check-mailmap" - | "count-objects" + | "cherry" | "describe" | "diff" + | "grep" + | "help" + | "log" + | "shortlog" + | "show" + | "show-branch" + | "status" + | "version" + | "whatchanged" + // Plumbing — object/ref inspection + | "cat-file" | "diff-files" | "diff-index" | "diff-tree" | "for-each-ref" - | "grep" - | "help" - | "log" | "ls-files" + | "ls-remote" | "ls-tree" | "merge-base" | "name-rev" | "rev-list" | "rev-parse" - | "shortlog" - | "show" - | "status" + | "show-ref" | "var" | "verify-commit" + | "verify-pack" | "verify-tag" - | "version" + // Plumbing — query/validation helpers + | "check-attr" + | "check-ignore" + | "check-mailmap" + | "check-ref-format" + | "column" + | "count-objects" + | "fmt-merge-msg" + | "fsck" + | "get-tar-commit-id" + | "patch-id" + | "stripspace" + ) +} + +pub fn is_read_only_invocation(parsed: &ParsedGitInvocation) -> bool { + if parsed.is_help || parsed.command.is_none() { + return true; + } + + if parsed + .command + .as_deref() + .is_some_and(is_definitely_read_only_command) + { + return true; + } + + match parsed.command.as_deref() { + Some("branch") => is_read_only_branch_invocation(parsed), + Some("stash") => is_read_only_stash_invocation(parsed), + Some("tag") => is_read_only_tag_invocation(parsed), + Some("remote") => is_read_only_remote_invocation(parsed), + Some("config") => is_read_only_config_invocation(parsed), + Some("worktree") => is_read_only_worktree_invocation(parsed), + Some("submodule") => is_read_only_submodule_invocation(parsed), + Some("symbolic-ref") => is_read_only_symbolic_ref_invocation(parsed), + Some("reflog") => is_read_only_reflog_invocation(parsed), + Some("notes") => is_read_only_notes_invocation(parsed), + _ => false, + } +} + +fn command_args_contain_any(command_args: &[String], flags: &[&str]) -> bool { + command_args.iter().any(|arg| { + flags + .iter() + .any(|flag| arg == flag || arg.starts_with(&format!("{flag}="))) + }) +} + +fn is_read_only_branch_invocation(parsed: &ParsedGitInvocation) -> bool { + let mutating_flags = [ + "-c", + "-C", + "-d", + "-D", + "-f", + "-m", + "-M", + "-u", + "--copy", + "--create-reflog", + "--delete", + "--delete-force", + "--edit-description", + "--force", + "--move", + "--no-track", + "--recurse-submodules", + "--set-upstream-to", + "--track", + "--unset-upstream", + ]; + if command_args_contain_any(&parsed.command_args, &mutating_flags) { + return false; + } + + // Flags that *trigger* list mode — their presence alone means read-only. + let list_mode_triggers = [ + "--all", + "--contains", + "--format", + "--list", + "--merged", + "--no-contains", + "--no-merged", + "--points-at", + "--remotes", + "--show-current", + "--sort", + "-a", + "-l", + "-r", + ]; + + // Flags that only *modify* list output (e.g. -v, --no-color) but do NOT + // trigger list mode on their own. `git branch -v feature` creates a branch + // named "feature" — -v only means "verbose" in list mode, it does not + // activate list mode. + command_args_contain_any(&parsed.command_args, &list_mode_triggers) + || parsed.pos_command(0).is_none() +} + +fn is_read_only_stash_invocation(parsed: &ParsedGitInvocation) -> bool { + matches!( + parsed.command_args.first().map(String::as_str), + Some("list" | "show") + ) +} + +fn is_read_only_tag_invocation(parsed: &ParsedGitInvocation) -> bool { + let mutating_flags = [ + "-a", + "-d", + "-e", + "-f", + "-F", + "-m", + "-s", + "-u", + "--annotate", + "--cleanup", + "--create-reflog", + "--delete", + "--edit", + "--file", + "--force", + "--local-user", + "--message", + "--no-sign", + "--sign", + "--trailer", + ]; + if command_args_contain_any(&parsed.command_args, &mutating_flags) { + return false; + } + + let read_only_listing_flags = [ + "--column", + "--contains", + "--format", + "--ignore-case", + "--list", + "--merged", + "--no-column", + "--no-contains", + "--no-merged", + "--points-at", + "--sort", + "-l", + ]; + + command_args_contain_any(&parsed.command_args, &read_only_listing_flags) + || parsed.pos_command(0).is_none() +} + +fn is_read_only_remote_invocation(parsed: &ParsedGitInvocation) -> bool { + let mutating_subcommands = [ + "add", + "rename", + "remove", + "rm", + "set-head", + "set-branches", + "set-url", + "prune", + "update", + ]; + + match parsed.pos_command(0).as_deref() { + None => true, + Some(subcommand) if mutating_subcommands.contains(&subcommand) => false, + Some("show" | "get-url") => true, + Some(_) => false, + } +} + +fn is_read_only_config_invocation(parsed: &ParsedGitInvocation) -> bool { + let mutating_flags = [ + "--add", + "--replace-all", + "--unset", + "--unset-all", + "--rename-section", + "--remove-section", + "--edit", + ]; + if command_args_contain_any(&parsed.command_args, &mutating_flags) { + return false; + } + + // Only explicit query actions are safe to fast-path. Other config flags like + // --type or --show-origin are modifiers and can still participate in writes. + let read_only_actions = [ + "--blob", + "--get", + "--get-all", + "--get-regexp", + "--get-urlmatch", + "--list", + "-l", + ]; + + command_args_contain_any(&parsed.command_args, &read_only_actions) +} + +fn is_read_only_worktree_invocation(parsed: &ParsedGitInvocation) -> bool { + matches!( + parsed.command_args.first().map(String::as_str), + Some("list") + ) +} + +fn is_read_only_submodule_invocation(parsed: &ParsedGitInvocation) -> bool { + matches!( + parsed.command_args.first().map(String::as_str), + Some("status" | "summary") + ) +} + +/// `git symbolic-ref HEAD` reads; `git symbolic-ref HEAD refs/heads/main` writes. +/// -d/--delete and -m (reason) with a target are also mutating. +fn is_read_only_symbolic_ref_invocation(parsed: &ParsedGitInvocation) -> bool { + if command_args_contain_any(&parsed.command_args, &["-d", "--delete"]) { + return false; + } + // Read mode: exactly one positional (the ref name to read). + // Write mode: two positionals (ref name + target). + parsed.pos_command(1).is_none() +} + +/// Only `git reflog expire` and `git reflog delete` are mutating. +/// Everything else — bare `git reflog`, `git reflog show`, `git reflog HEAD`, +/// `git reflog --all`, `git reflog exists` — is read-only (bare reflog and +/// unrecognized first args are interpreted by git as `reflog show`). +fn is_read_only_reflog_invocation(parsed: &ParsedGitInvocation) -> bool { + !matches!( + parsed.command_args.first().map(String::as_str), + Some("expire" | "delete") + ) +} + +/// `git notes list`, `git notes show` are read-only. +/// `git notes add/append/copy/edit/merge/prune/remove` are mutating. +/// Bare `git notes` defaults to `list`. +fn is_read_only_notes_invocation(parsed: &ParsedGitInvocation) -> bool { + matches!( + parsed.command_args.first().map(String::as_str), + None | Some("list" | "show" | "get-ref") ) } #[cfg(test)] mod tests { use super::*; + use crate::git::cli_parser::parse_git_cli_args; #[test] fn read_only_commands_detected() { @@ -48,6 +311,16 @@ mod tests { assert!(is_definitely_read_only_command("log")); assert!(is_definitely_read_only_command("cat-file")); assert!(is_definitely_read_only_command("ls-files")); + // High-volume plumbing used by IDEs and git clients + assert!(is_definitely_read_only_command("ls-remote")); + assert!(is_definitely_read_only_command("show-ref")); + assert!(is_definitely_read_only_command("cherry")); + assert!(is_definitely_read_only_command("show-branch")); + assert!(is_definitely_read_only_command("for-each-ref")); + assert!(is_definitely_read_only_command("verify-pack")); + assert!(is_definitely_read_only_command("check-ref-format")); + assert!(is_definitely_read_only_command("fsck")); + assert!(is_definitely_read_only_command("whatchanged")); } #[test] @@ -68,4 +341,278 @@ mod tests { assert!(!is_definitely_read_only_command("my-custom-alias")); assert!(!is_definitely_read_only_command("")); } + + #[test] + fn read_only_invocation_detects_branch_show_current() { + let parsed = parse_git_cli_args(&["branch".to_string(), "--show-current".to_string()]); + assert!(is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_detects_branch_listing_without_positionals() { + let parsed = parse_git_cli_args(&["branch".to_string(), "-v".to_string()]); + assert!(is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_rejects_branch_creation() { + let parsed = parse_git_cli_args(&["branch".to_string(), "feature".to_string()]); + assert!(!is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_rejects_branch_create_with_verbose_flag() { + // `git branch -v feature` creates a branch; -v is a list-output modifier, + // not a list-mode trigger. + let parsed = parse_git_cli_args(&[ + "branch".to_string(), + "-v".to_string(), + "feature".to_string(), + ]); + assert!(!is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_detects_tag_listing() { + let parsed = parse_git_cli_args(&["tag".to_string(), "--list".to_string()]); + assert!(is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_rejects_tag_creation() { + let parsed = parse_git_cli_args(&["tag".to_string(), "v1.2.3".to_string()]); + assert!(!is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_detects_stash_list() { + let parsed = parse_git_cli_args(&["stash".to_string(), "list".to_string()]); + assert!(is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_detects_top_level_version() { + let parsed = parse_git_cli_args(&["--version".to_string()]); + assert!(is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_detects_commit_help() { + let parsed = parse_git_cli_args(&["commit".to_string(), "--help".to_string()]); + assert!(is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_detects_remote_listing() { + let parsed = parse_git_cli_args(&["remote".to_string(), "-v".to_string()]); + assert!(is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_rejects_remote_add() { + let parsed = parse_git_cli_args(&[ + "remote".to_string(), + "add".to_string(), + "origin".to_string(), + "https://example.com/repo".to_string(), + ]); + assert!(!is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_detects_config_list() { + let parsed = parse_git_cli_args(&[ + "config".to_string(), + "--list".to_string(), + "--show-origin".to_string(), + ]); + assert!(is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_rejects_config_set() { + let parsed = parse_git_cli_args(&[ + "config".to_string(), + "--add".to_string(), + "demo.key".to_string(), + "value".to_string(), + ]); + assert!(!is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_rejects_config_write_with_type_modifier() { + let parsed = parse_git_cli_args(&[ + "config".to_string(), + "--type=bool".to_string(), + "demo.enabled".to_string(), + "true".to_string(), + ]); + assert!(!is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_accepts_config_get_with_modifiers() { + let parsed = parse_git_cli_args(&[ + "config".to_string(), + "--show-origin".to_string(), + "--type=bool".to_string(), + "--get".to_string(), + "demo.enabled".to_string(), + ]); + assert!(is_read_only_invocation(&parsed)); + } + + // symbolic-ref classifier + #[test] + fn read_only_invocation_detects_symbolic_ref_read() { + let parsed = parse_git_cli_args(&["symbolic-ref".to_string(), "HEAD".to_string()]); + assert!(is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_detects_symbolic_ref_read_with_short() { + let parsed = parse_git_cli_args(&[ + "symbolic-ref".to_string(), + "--short".to_string(), + "HEAD".to_string(), + ]); + assert!(is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_rejects_symbolic_ref_write() { + let parsed = parse_git_cli_args(&[ + "symbolic-ref".to_string(), + "HEAD".to_string(), + "refs/heads/main".to_string(), + ]); + assert!(!is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_rejects_symbolic_ref_delete() { + let parsed = parse_git_cli_args(&[ + "symbolic-ref".to_string(), + "--delete".to_string(), + "HEAD".to_string(), + ]); + assert!(!is_read_only_invocation(&parsed)); + } + + // reflog classifier + #[test] + fn read_only_invocation_detects_bare_reflog() { + let parsed = parse_git_cli_args(&["reflog".to_string()]); + assert!(is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_detects_reflog_show() { + let parsed = + parse_git_cli_args(&["reflog".to_string(), "show".to_string(), "HEAD".to_string()]); + assert!(is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_detects_reflog_implicit_show_with_ref() { + // `git reflog HEAD` is `git reflog show HEAD` + let parsed = parse_git_cli_args(&["reflog".to_string(), "HEAD".to_string()]); + assert!(is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_detects_reflog_implicit_show_with_flags() { + // `git reflog --all` is `git reflog show --all` + let parsed = parse_git_cli_args(&["reflog".to_string(), "--all".to_string()]); + assert!(is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_detects_reflog_exists() { + let parsed = parse_git_cli_args(&[ + "reflog".to_string(), + "exists".to_string(), + "HEAD".to_string(), + ]); + assert!(is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_rejects_reflog_expire() { + let parsed = parse_git_cli_args(&[ + "reflog".to_string(), + "expire".to_string(), + "--all".to_string(), + ]); + assert!(!is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_rejects_reflog_delete() { + let parsed = parse_git_cli_args(&[ + "reflog".to_string(), + "delete".to_string(), + "HEAD@{0}".to_string(), + ]); + assert!(!is_read_only_invocation(&parsed)); + } + + // notes classifier + #[test] + fn read_only_invocation_detects_bare_notes() { + let parsed = parse_git_cli_args(&["notes".to_string()]); + assert!(is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_detects_notes_list() { + let parsed = parse_git_cli_args(&["notes".to_string(), "list".to_string()]); + assert!(is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_detects_notes_show() { + let parsed = + parse_git_cli_args(&["notes".to_string(), "show".to_string(), "HEAD".to_string()]); + assert!(is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_rejects_notes_add() { + let parsed = parse_git_cli_args(&[ + "notes".to_string(), + "add".to_string(), + "-m".to_string(), + "note".to_string(), + ]); + assert!(!is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_rejects_notes_remove() { + let parsed = parse_git_cli_args(&[ + "notes".to_string(), + "remove".to_string(), + "HEAD".to_string(), + ]); + assert!(!is_read_only_invocation(&parsed)); + } + + // ls-remote and show-ref as invocations + #[test] + fn read_only_invocation_detects_ls_remote() { + let parsed = parse_git_cli_args(&[ + "ls-remote".to_string(), + "--heads".to_string(), + "origin".to_string(), + ]); + assert!(is_read_only_invocation(&parsed)); + } + + #[test] + fn read_only_invocation_detects_show_ref() { + let parsed = parse_git_cli_args(&["show-ref".to_string(), "--heads".to_string()]); + assert!(is_read_only_invocation(&parsed)); + } } diff --git a/src/git/repository.rs b/src/git/repository.rs index 410f730e0..00e4f0df4 100644 --- a/src/git/repository.rs +++ b/src/git/repository.rs @@ -95,6 +95,19 @@ fn args_with_disabled_hooks_if_needed(args: &[String]) -> Vec { out } +fn apply_internal_git_command_env(cmd: &mut Command) { + cmd.env_remove("GIT_EXTERNAL_DIFF"); + cmd.env_remove("GIT_DIFF_OPTS"); + + // Internal helper git invocations never need trace2 output. Disabling every + // trace2 stream here avoids extra named-pipe/process overhead on Windows when + // users have global trace2 targets configured for the wrapper daemon. + cmd.env("GIT_TRACE2", "0"); + cmd.env("GIT_TRACE2_EVENT", "0"); + cmd.env("GIT_TRACE2_PERF", "0"); + cmd.env("GIT_TRACE2_BRIEF", "0"); +} + fn first_git_subcommand_index(args: &[String]) -> Option { let mut index = 0usize; @@ -2354,6 +2367,10 @@ impl Repository { } pub fn find_repository(global_args: &[String]) -> Result { + if let Some(repository) = try_find_repository_no_git_exec(global_args)? { + return Ok(repository); + } + let mut rev_parse_args = global_args.to_owned(); rev_parse_args.push("rev-parse".to_string()); // Use --git-dir instead of --absolute-git-dir for compatibility with Git < 2.13 @@ -2440,21 +2457,12 @@ pub fn find_repository(global_args: &[String]) -> Result } // Ensure all internal git commands use a stable repository root consistently. - let mut normalized_global_args = global_args.to_owned(); let command_root = if is_bare { git_dir.display().to_string() } else { workdir.display().to_string() }; - - if normalized_global_args.is_empty() { - normalized_global_args = vec!["-C".to_string(), command_root]; - } else if normalized_global_args.len() == 2 - && normalized_global_args[0] == "-C" - && normalized_global_args[1] != command_root - { - normalized_global_args[1] = command_root; - } + let normalized_global_args = normalize_global_args_for_repo_root(global_args, &command_root); // Canonicalize workdir for reliable path comparisons (especially on Windows) // On Windows, canonical paths use the \\?\ UNC prefix, which makes path.starts_with() @@ -2491,6 +2499,147 @@ pub fn find_repository(global_args: &[String]) -> Result }) } +fn try_find_repository_no_git_exec( + global_args: &[String], +) -> Result, GitAiError> { + // When GIT_DIR, GIT_WORK_TREE, or GIT_CEILING_DIRECTORIES are set, the + // filesystem walk may disagree with git's own discovery logic. Fall back + // to the git-exec path so those env vars are honoured. + if std::env::var_os("GIT_DIR").is_some() + || std::env::var_os("GIT_WORK_TREE").is_some() + || std::env::var_os("GIT_CEILING_DIRECTORIES").is_some() + { + return Ok(None); + } + + let Some(base_dir) = resolve_fast_discovery_base_dir(global_args)? else { + return Ok(None); + }; + + let paths = match discover_repository_paths_no_git_exec(&base_dir) { + Ok(paths) => paths, + Err(_) => return Ok(None), + }; + + let normalized_global_args = + normalize_global_args_for_repo_root(global_args, &paths.command_root.to_string_lossy()); + + // If construction fails (e.g. canonicalize fails due to permissions/symlinks, + // or core.worktree config makes the assumed workdir incorrect), fall back to + // the git-exec discovery path rather than propagating the error. + match repository_from_discovered_paths( + normalized_global_args, + &paths.workdir, + &paths.git_dir, + &paths.git_common_dir, + ) { + Ok(repo) => Ok(Some(repo)), + Err(_) => Ok(None), + } +} + +fn resolve_fast_discovery_base_dir(global_args: &[String]) -> Result, GitAiError> { + let mut base: Option = None; + let mut idx = 0usize; + + while idx < global_args.len() { + let arg = &global_args[idx]; + + if arg == "-C" { + let path_arg = global_args.get(idx + 1).ok_or_else(|| { + GitAiError::Generic("Missing path after -C in global git args".to_string()) + })?; + base = Some(apply_global_c_arg(base.as_ref(), path_arg)?); + idx += 2; + continue; + } + + if let Some(path_arg) = arg.strip_prefix("-C") + && !path_arg.is_empty() + { + base = Some(apply_global_c_arg(base.as_ref(), path_arg)?); + idx += 1; + continue; + } + + if is_fast_discovery_safe_global_flag(arg) { + idx += 1; + continue; + } + + return Ok(None); + } + + Ok(Some(match base { + Some(base) => base, + None => std::env::current_dir().map_err(GitAiError::IoError)?, + })) +} + +fn apply_global_c_arg(base: Option<&PathBuf>, path_arg: &str) -> Result { + let next_base = PathBuf::from(path_arg); + Ok(if next_base.is_absolute() { + next_base + } else { + let current = match base { + Some(existing) => existing.clone(), + None => std::env::current_dir().map_err(GitAiError::IoError)?, + }; + current.join(next_base) + }) +} + +fn is_fast_discovery_safe_global_flag(arg: &str) -> bool { + matches!( + arg, + "-p" | "--paginate" + | "-P" + | "--no-pager" + | "--no-replace-objects" + | "--no-lazy-fetch" + | "--no-optional-locks" + | "--no-advice" + | "--literal-pathspecs" + | "--glob-pathspecs" + | "--noglob-pathspecs" + | "--icase-pathspecs" + ) +} + +/// Replace any `-C` arguments with a single `-C `, keeping other +/// global flags intact. When the args contain flags that aren't safe to rebase +/// (e.g. relative `--git-dir`/`--work-tree`), the original args are returned +/// unchanged so their meaning is preserved. +fn normalize_global_args_for_repo_root(global_args: &[String], command_root: &str) -> Vec { + let mut normalized = vec!["-C".to_string(), command_root.to_string()]; + let mut idx = 0usize; + + while idx < global_args.len() { + let arg = &global_args[idx]; + + // Strip existing -C args (we prepended the canonical root above). + if arg == "-C" { + idx += 2; + continue; + } + if arg.starts_with("-C") && arg.len() > 2 { + idx += 1; + continue; + } + + // If the arg isn't a harmless global flag, we can't safely rebase. + // Return the original args unchanged. + if !is_fast_discovery_safe_global_flag(arg) { + return global_args.to_vec(); + } + + normalized.push(arg.clone()); + idx += 1; + } + + normalized +} + fn resolve_command_base_dir(global_args: &[String]) -> Result { let mut base: Option = None; let mut idx = 0usize; @@ -2500,17 +2649,7 @@ fn resolve_command_base_dir(global_args: &[String]) -> Result existing.clone(), - None => std::env::current_dir().map_err(GitAiError::IoError)?, - }; - current.join(next_base) - }); + base = Some(apply_global_c_arg(base.as_ref(), path_arg)?); idx += 2; continue; } @@ -2807,7 +2946,7 @@ pub fn from_bare_repository(git_dir: &Path) -> Result { } fn repository_from_discovered_paths( - command_root: &Path, + global_args: Vec, workdir: &Path, git_dir: &Path, git_common_dir: &Path, @@ -2847,7 +2986,7 @@ fn repository_from_discovered_paths( }; Ok(Repository { - global_args: vec!["-C".to_string(), command_root.to_string_lossy().to_string()], + global_args, storage, git_dir: git_dir.to_path_buf(), git_common_dir: git_common_dir.to_path_buf(), @@ -2865,8 +3004,10 @@ fn repository_from_discovered_paths( pub fn discover_repository_in_path_no_git_exec(path: &Path) -> Result { let paths = discover_repository_paths_no_git_exec(path)?; + let global_args = + normalize_global_args_for_repo_root(&[], &paths.command_root.to_string_lossy()); repository_from_discovered_paths( - &paths.command_root, + global_args, &paths.workdir, &paths.git_dir, &paths.git_common_dir, @@ -3028,8 +3169,7 @@ pub fn exec_git_allow_nonzero_with_profile( args_with_internal_git_profile(&args_with_disabled_hooks_if_needed(args), profile); let mut cmd = Command::new(config::Config::get().git_cmd()); cmd.args(&effective_args); - cmd.env_remove("GIT_EXTERNAL_DIFF"); - cmd.env_remove("GIT_DIFF_OPTS"); + apply_internal_git_command_env(&mut cmd); #[cfg(windows)] { @@ -3082,8 +3222,7 @@ pub fn exec_git_stdin_with_profile( .stdin(std::process::Stdio::piped()) .stdout(std::process::Stdio::piped()) .stderr(std::process::Stdio::piped()); - cmd.env_remove("GIT_EXTERNAL_DIFF"); - cmd.env_remove("GIT_DIFF_OPTS"); + apply_internal_git_command_env(&mut cmd); #[cfg(windows)] { @@ -3159,8 +3298,7 @@ pub fn exec_git_stdin_with_env_with_profile( for (k, v) in env.iter() { cmd.env(k, v); } - cmd.env_remove("GIT_EXTERNAL_DIFF"); - cmd.env_remove("GIT_DIFF_OPTS"); + apply_internal_git_command_env(&mut cmd); #[cfg(windows)] { @@ -3394,6 +3532,7 @@ fn parse_hunk_header(line: &str) -> Option<(Vec, bool)> { #[cfg(test)] mod tests { use super::*; + use serial_test::serial; use std::fs; use std::path::{Path, PathBuf}; use std::process::Command; @@ -3827,6 +3966,124 @@ index 0000000..abc1234 100644 assert_eq!(resolved, base.join("nested").join("..").join("repo")); } + #[test] + fn resolve_fast_discovery_base_dir_supports_common_global_flags() { + let temp = tempfile::tempdir().expect("tempdir"); + let base = temp.path().join("root"); + let args = vec![ + "--no-pager".to_string(), + "-C".to_string(), + base.to_string_lossy().to_string(), + "--literal-pathspecs".to_string(), + ]; + + let resolved = resolve_fast_discovery_base_dir(&args).expect("resolve fast discovery dir"); + assert_eq!(resolved, Some(base)); + } + + #[test] + fn resolve_fast_discovery_base_dir_rejects_git_dir_override() { + let args = vec!["--git-dir".to_string(), ".git".to_string()]; + let resolved = resolve_fast_discovery_base_dir(&args).expect("resolve fast discovery dir"); + assert_eq!(resolved, None); + } + + #[test] + #[serial] + fn fast_discovery_skipped_when_git_dir_env_set() { + struct EnvGuard(&'static str, Option); + impl Drop for EnvGuard { + fn drop(&mut self) { + unsafe { + match &self.1 { + Some(val) => std::env::set_var(self.0, val), + None => std::env::remove_var(self.0), + } + } + } + } + + let prev = std::env::var_os("GIT_DIR"); + let _guard = EnvGuard("GIT_DIR", prev); + unsafe { std::env::set_var("GIT_DIR", "/tmp/other-repo/.git") }; + + let result = try_find_repository_no_git_exec(&[]).expect("should not error"); + assert!( + result.is_none(), + "fast path must be skipped when GIT_DIR is set" + ); + } + + #[test] + fn normalize_global_args_for_repo_root_collapses_chained_c_args() { + let args = vec![ + "--no-pager".to_string(), + "-C".to_string(), + "nested".to_string(), + "--literal-pathspecs".to_string(), + ]; + + let normalized = normalize_global_args_for_repo_root(&args, "/repo/root"); + assert_eq!( + normalized, + vec![ + "-C".to_string(), + "/repo/root".to_string(), + "--no-pager".to_string(), + "--literal-pathspecs".to_string(), + ] + ); + } + + #[test] + fn normalize_global_args_for_repo_root_preserves_relative_git_dir_and_work_tree() { + let args = vec![ + "--git-dir".to_string(), + "sub/repo/.git".to_string(), + "--work-tree".to_string(), + "sub/repo".to_string(), + ]; + + let normalized = normalize_global_args_for_repo_root(&args, "/repo/root"); + assert_eq!(normalized, args); + } + + #[test] + #[serial] + fn find_repository_preserves_relative_git_dir_and_work_tree_for_internal_commands() { + struct CurrentDirGuard(PathBuf); + + impl Drop for CurrentDirGuard { + fn drop(&mut self) { + std::env::set_current_dir(&self.0).expect("restore current dir"); + } + } + + let temp = tempfile::tempdir().expect("tempdir"); + let outer = temp.path().join("outer"); + let repo_dir = outer.join("sub").join("repo"); + fs::create_dir_all(&repo_dir).expect("create repo dir"); + + run_git(&repo_dir, &["init"]); + + let previous_dir = std::env::current_dir().expect("current dir"); + let _guard = CurrentDirGuard(previous_dir); + std::env::set_current_dir(&outer).expect("set current dir"); + + let args = vec![ + "--git-dir".to_string(), + "sub/repo/.git".to_string(), + "--work-tree".to_string(), + "sub/repo".to_string(), + ]; + let repo = find_repository(&args).expect("find repository"); + let output = repo + .git(&["config", "--get", "core.repositoryformatversion"]) + .expect("internal git command should preserve relative repo args"); + + assert_eq!(output.trim(), "0"); + } + #[test] fn find_repository_in_path_supports_bare_repositories() { let temp = tempfile::tempdir().expect("tempdir"); diff --git a/tests/async_mode.rs b/tests/async_mode.rs index f7ef67521..6fe930fde 100644 --- a/tests/async_mode.rs +++ b/tests/async_mode.rs @@ -715,6 +715,40 @@ fn async_mode_post_commit_still_processing_when_no_daemon() { ); } +#[test] +fn async_mode_post_commit_shows_stats_with_commit_alias() { + let repo = TestRepo::new_with_mode(GitTestMode::WrapperDaemon); + + // Configure a git alias: cm = commit (mimics common user setup) + repo.git(&["config", "alias.cm", "commit"]).unwrap(); + + // Base commit (human only). + let mut file = repo.filename("alias_test.txt"); + file.set_contents(crate::lines!["Base line 1", "Base line 2"]); + repo.stage_all_and_commit("Base commit").unwrap(); + + // Add AI-attributed lines. + file.insert_at(2, crate::lines!["AI line 1".ai(), "AI line 2".ai()]); + + repo.git(&["add", "-A"]).expect("add should succeed"); + + // Commit using the alias instead of "commit" directly. + let output = repo + .git_with_env( + &["cm", "-m", "AI additions via alias"], + &[("GIT_AI_TEST_FORCE_TTY", "1")], + None, + ) + .expect("aliased commit should succeed"); + + // The wrapper should resolve the alias and still show the stats bar. + assert!( + output.contains("you") && output.contains("ai"), + "expected stats output when committing via alias, got:\n{}", + output + ); +} + #[test] fn async_mode_post_commit_skips_stats_for_large_commit() { let repo = TestRepo::new_with_mode(GitTestMode::WrapperDaemon);