docs(install): recommend global hooks as primary setup path#855
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements global git hook support for hk using Git 2.54+ config-based hooks, adding --global, --legacy, and --from-hook flags to manage these hooks safely across repositories. Documentation and tests were updated to support these features. Review feedback identified several compilation issues due to missing info! macro imports and the use of unstable let_chains syntax, as well as an opportunity to optimize the removal of git configuration sections.
| @@ -1,15 +1,48 @@ | |||
| use crate::{Result, config::Config, env, git_util}; | |||
| use eyre::bail; | |||
| use log::warn; | |||
There was a problem hiding this comment.
The info! macro is used later in this file (line 269) but is not imported. This will cause a compilation error unless the log macros are globally available via a crate-level #[macro_use]. Given that warn is explicitly imported, info should be as well for consistency and correctness.
| use log::warn; | |
| use log::{info, warn}; |
| if let Ok(output) = Command::new("git") | ||
| .args(["config", "--global", "--get", key.as_str()]) | ||
| .output() | ||
| && output.status.success() | ||
| && !output.stdout.is_empty() | ||
| { | ||
| overlapping.push(event); | ||
| } |
There was a problem hiding this comment.
This code uses the let_chains feature (if let ... && ...), which is currently unstable in Rust (see tracking issue #53667). Unless this project specifically requires a nightly toolchain and has the feature enabled, this will cause a compilation error on stable Rust. It is safer to use nested if statements.
| if let Ok(output) = Command::new("git") | |
| .args(["config", "--global", "--get", key.as_str()]) | |
| .output() | |
| && output.status.success() | |
| && !output.stdout.is_empty() | |
| { | |
| overlapping.push(event); | |
| } | |
| if let Ok(output) = Command::new("git") | |
| .args(["config", "--global", "--get", key.as_str()]) | |
| .output() | |
| { | |
| if output.status.success() && !output.stdout.is_empty() { | |
| overlapping.push(event); | |
| } | |
| } |
| @@ -1,33 +1,25 @@ | |||
| use crate::{Result, git_util}; | |||
| use crate::{Result, cli::install}; | |||
| pub(crate) fn remove_config_entries(scope: &str) -> Result<usize> { | ||
| let output = Command::new("git") | ||
| .args([ | ||
| "config", | ||
| scope, | ||
| "--name-only", | ||
| "--get-regexp", | ||
| "^hook\\.hk-", | ||
| ]) | ||
| .output()?; | ||
| // git config --get-regexp: 0 = matches, 1 = no matches, ≥2 = real error | ||
| // (e.g. unreadable config). Don't conflate "nothing to remove" with a | ||
| // failed uninstall. | ||
| let code = output.status.code().unwrap_or(1); | ||
| if code == 1 { | ||
| return Ok(0); | ||
| } | ||
| if !output.status.success() { | ||
| bail!( | ||
| "git config --get-regexp failed (exit {}): {}", | ||
| code, | ||
| String::from_utf8_lossy(&output.stderr).trim() | ||
| ); | ||
| } | ||
| let keys: Vec<String> = String::from_utf8_lossy(&output.stdout) | ||
| .lines() | ||
| .map(|s| s.trim().to_string()) | ||
| .filter(|s| !s.is_empty()) | ||
| .collect(); | ||
| // Dedupe since multi-valued keys appear once per value. | ||
| let mut seen = std::collections::BTreeSet::new(); | ||
| let mut removed = 0; | ||
| for key in keys { | ||
| if seen.insert(key.clone()) { | ||
| run_git(&["config", scope, "--unset-all", key.as_str()])?; | ||
| // Count one per hook event, not one per key (command + event). | ||
| if key.ends_with(".command") { | ||
| removed += 1; | ||
| } | ||
| let hook_file = hooks.join(hook); | ||
| xx::file::write(&hook_file, git_hook_content(&command, hook))?; | ||
| xx::file::make_executable(&hook_file)?; | ||
| println!("Installed hk hook: {}", hook_file.display()); | ||
| } | ||
| Ok(()) | ||
| } | ||
| Ok(removed) | ||
| } |
There was a problem hiding this comment.
The current implementation of remove_config_entries is inefficient as it spawns a new git config --unset-all process for every single key found. For a standard installation with 9 hooks, this results in 18 process spawns. Additionally, unsetting keys individually leaves empty section headers (e.g., [hook "hk-pre-commit"]) in the git config file.
A more efficient and cleaner approach is to identify the unique section names and use git config --remove-section, which removes all keys within the section and the section header itself in a single call per hook.
pub(crate) fn remove_config_entries(scope: &str) -> Result<usize> {
let output = Command::new("git")
.args(["config", scope, "--name-only", "--get-regexp", "^hook\\.hk-"])
.output()?;
// git config --get-regexp: 0 = matches, 1 = no matches, ≥2 = real error
// (e.g. unreadable config). Don't conflate "nothing to remove" with a
// failed uninstall.
let code = output.status.code().unwrap_or(1);
if code == 1 {
return Ok(0);
}
if !output.status.success() {
bail!(
"git config --get-regexp failed (exit {}): {}",
code,
String::from_utf8_lossy(&output.stderr).trim()
);
}
let stdout = String::from_utf8_lossy(&output.stdout);
let mut sections = std::collections::BTreeSet::new();
for key in stdout.lines().filter(|s| !s.trim().is_empty()) {
if let Some(section) = key.rsplitn(2, '.').nth(1) {
sections.insert(section.to_string());
}
}
let removed = sections.len();
for section in sections {
run_git(&["config", scope, "--remove-section", §ion])?;
}
Ok(removed)
}
Greptile SummaryThis PR promotes
Confidence Score: 4/5Safe to merge after addressing the run_git stderr omission; all other changes are docs/copy and well-guarded logic. One P1 remains in run_git: failures from git config writes (the primary write path for the new feature) surface no diagnostic information, making debugging hard for users. All other findings are P2 or lower. src/cli/install.rs — run_git stderr handling Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[hk install] --> B{--global?}
B -- yes --> C{Git >= 2.54?}
C -- no --> D[bail! requires Git 2.54+]
C -- yes --> E[remove_config_entries --global]
E --> F[write_config_hook --global for each DEFAULT_GLOBAL_EVENT]
F --> G[Print success message]
B -- no --> H[use_config_hooks = !legacy && git>=2.54]
H --> I[Config::get — load hk.pkl]
I --> J[remove_local_shims + remove_local_config_entries]
J --> K{events empty?}
K -- yes --> L[warn and return]
K -- no --> M{use_config_hooks?}
M -- yes --> N[install_local_config]
N --> O[warn_if_global_overlap]
M -- no --> P[install_local_shims]
N --> Q[write_config_hook --local per event]
P --> R[write shim file per event in .git/hooks/]
|
Lead with `hk install --global` in getting_started.md and the install CLI reference. The global install is a silent no-op in repos without an `hk.pkl`, so it's safe to enable everywhere and saves users from re-running `hk install` in every clone. Per-repo install is now framed as an alternative for pre-Git-2.54 or selective-repo use. Also renames the pre-existing "Global Configuration" section to "Global `hkrc` Configuration" to disambiguate from global hooks. Regenerates docs/cli from updated clap docstrings. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5305307 to
c47b15a
Compare
Summary
Leads the install docs with
hk install --globalas the recommended setup path. The global install is a silent no-op in repos without anhk.pkl, so it's safe to enable once per machine and avoids re-runninghk installin every clone.Follow-up to #853 (the config-based hooks +
--globalfeature that this documents).What changed
hk installis now framed as an alternative for pre-Git-2.54 or selective-repo use. The overlap warning (Git aggregateshook.<name>.commandacross scopes) and the manual~/.gitconfigsetup are retained as subsections.Installstruct docstring and--globalflag help sohk install --helpleads with the recommendation.hk.usage.kdl,docs/cli/commands.json— regenerated viahk usage→usage g markdown."version": "0.0.0"+"private": trueso the bun workspace resolves cleanly (the pre-pushdocs:buildhook was failing on missing version).Why
New users land on getting_started.md; leading with per-repo
hk installmade them re-install in every clone and gave no signal that the global option existed. The--from-hookshort-circuit (also from #853) makes global genuinely safe to enable everywhere, so it deserves the headline.Test plan
hk check --allpasses.🤖 Generated with Claude Code