Handle git aliases in hook processing#378
Handle git aliases in hook processing#378cnaples79 wants to merge 1 commit intogit-ai-project:mainfrom
Conversation
|
|
|
@cnaples79 Thank you so much for your first contribution to Git AI! Excited to get this functionality merged in! Do you have a some time to add more end-to-end tests via the I think gix-config is production-ready: https://github.com/GitoxideLabs/gitoxide/tree/main/gix-config I'm open to merging without a native git config implementation, however, in that case, I think we should add a benchmark to verify we're not adding more than 5-10ms per command. Thanks again and happy to talk this through https://calendly.com/acunniffe/meeting-with-git-ai-authors anytime |
|
@cnaples79 I would love to be able to merge this soon. Have you had a chance to look at the CLA? |
|
@svarlamov I've signed the CLA! Sorry I missed your first comment. I can definitely take some time for your requests, it may be a couple days though I'm currently out of town. |
|
Just a heads up #376 is being kindly picked up by @bernoussama so will probably have to try to sync up these PRs. Probably the fix for #376 first and hopefully it'll all just work with this PR. Thanks folks! |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| #[cfg(test)] | ||
| mod tests { | ||
| use super::{ParsedGitInvocation, expand_alias_with_resolver}; | ||
|
|
||
| fn base_invocation(command: &str, args: &[&str]) -> ParsedGitInvocation { | ||
| ParsedGitInvocation { | ||
| global_args: vec![], | ||
| command: Some(command.to_string()), | ||
| command_args: args.iter().map(|s| s.to_string()).collect(), | ||
| saw_end_of_opts: false, | ||
| is_help: false, | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn expands_simple_alias() { | ||
| let parsed = base_invocation("co", &["main"]); | ||
| let resolved = expand_alias_with_resolver(parsed, |alias| { | ||
| (alias == "co").then(|| "checkout".to_string()) | ||
| }); | ||
| assert_eq!(resolved.command.as_deref(), Some("checkout")); | ||
| assert_eq!(resolved.command_args, vec!["main"]); | ||
| } | ||
|
|
||
| #[test] | ||
| fn expands_alias_with_arguments() { | ||
| let parsed = base_invocation("unstage", &["file.txt"]); | ||
| let resolved = expand_alias_with_resolver(parsed, |alias| { | ||
| (alias == "unstage").then(|| "reset HEAD --".to_string()) | ||
| }); | ||
| assert_eq!(resolved.command.as_deref(), Some("reset")); | ||
| assert_eq!(resolved.command_args, vec!["HEAD", "--", "file.txt"]); | ||
| } | ||
|
|
||
| #[test] | ||
| fn ignores_shell_aliases() { | ||
| let parsed = base_invocation("cleanup", &["-n"]); | ||
| let resolved = expand_alias_with_resolver(parsed, |_alias| { | ||
| Some("!f() { echo hi; }".to_string()) | ||
| }); | ||
| assert_eq!(resolved.command.as_deref(), Some("cleanup")); | ||
| assert_eq!(resolved.command_args, vec!["-n"]); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Duplicate mod tests blocks cause compilation error
The PR introduces a new #[cfg(test)] mod tests block at line 515, but there was already an existing #[cfg(test)] mod tests block at line 654. In Rust, having two modules with the same name in the same scope is a compilation error.
Root Cause
The PR added new tests for expand_alias_with_resolver in a new mod tests block, but the file already had an existing mod tests block with tests for other functions like pos_command, derive_directory_from_url, and extract_clone_target_directory.
Impact: The code will fail to compile with an error like the name 'tests' is defined multiple times.
Prompt for agents
The new tests for expand_alias_with_resolver should be merged into the existing mod tests block at line 654 instead of creating a new mod tests block. Move the test functions (base_invocation, expands_simple_alias, expands_alias_with_arguments, ignores_shell_aliases) from lines 515-558 into the existing mod tests block at line 654, and remove the duplicate #[cfg(test)] mod tests declaration at line 515.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let new_command = tokens.remove(0); | ||
| parsed.command = Some(new_command); | ||
| // Prepend alias arguments to existing command args | ||
| tokens.append(&mut parsed.command_args); | ||
| parsed.command_args = tokens; | ||
| } | ||
| } | ||
|
|
||
| parsed |
There was a problem hiding this comment.
🟡 is_help flag not updated after alias expansion
After alias expansion, the is_help field of ParsedGitInvocation is not recalculated. If a user has an alias like h = help, running git h will have is_help = false even though the expanded command is help.
Root Cause
The is_help field is computed during parse_git_cli_args() at src/git/cli_parser.rs:501-504 based on the original command and args. When expand_alias_with_resolver() changes the command (e.g., from h to help), the is_help field is not updated.
In src/commands/git_handlers.rs:145 and src/commands/git_handlers.rs:152, is_help is used to decide whether to run hooks:
if parsed_args.command.as_deref() == Some("clone") && !parsed_args.is_help && !skip_hooks {let exit_status = if !parsed_args.is_help && has_repo && !skip_hooks {Impact: When a user runs an alias that expands to help or includes -h/--help, hooks will incorrectly run. This could cause unnecessary side effects when the user is just asking for help.
| let new_command = tokens.remove(0); | |
| parsed.command = Some(new_command); | |
| // Prepend alias arguments to existing command args | |
| tokens.append(&mut parsed.command_args); | |
| parsed.command_args = tokens; | |
| } | |
| } | |
| parsed | |
| let new_command = tokens.remove(0); | |
| parsed.command = Some(new_command.clone()); | |
| // Prepend alias arguments to existing command args | |
| tokens.append(&mut parsed.command_args); | |
| parsed.command_args = tokens; | |
| // Recalculate is_help after alias expansion | |
| parsed.is_help = new_command == "help" | |
| || parsed.command_args.iter().any(|t| t == "--help" || t == "-h"); | |
Was this helpful? React with 👍 or 👎 to provide feedback.
3 participants
Summary
Rationale
Commands invoked via git aliases (e.g., , ) were bypassing git-ai hooks. Resolving aliases to their underlying commands ensures hooks still run.
Changes
Test Plan
Closes #377