Fix review findings: injection, Windows doctor, config detection, worktree, docs

CHANGELOG.md

@@ -5,6 +5,31 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [Unreleased]
+
+### Fixed
+
+- `doctor` no longer reports every routed directory as differing from its
+  canonical path on Windows (it now compares in the same forward-slash,
+  de-UNC'd form routes are stored in).
+- `create`/`edit` reject control characters in a signing key, and rendered
+  fragments escape control characters, closing a gitconfig-section injection
+  via a newline-bearing `--signing-key`.
+- `init`, `doctor` and `uninstall` find the git-id include and
+  `user.useConfigOnly` even when they live in `$XDG_CONFIG_HOME/git/config` and
+  a `~/.gitconfig` is later created; `uninstall` now removes them from whichever
+  global config file holds them.
+- `use` warns when routing a linked worktree's or submodule's own path, where
+  git matches against the main repository and the route would never apply.
+- `edit` validates all fields before writing any, so a rejected edit no longer
+  leaves an earlier field already changed.
+- `delete` rewrites the routes file before removing the fragment, so an
+  interruption cannot leave a route pointing at a deleted fragment.
+- `doctor` also flags a duplicate `gitdir` route when one side is a hand-added
+  (preserved) `[includeIf]` block.
+- `init` no longer overwrites an earlier same-second backup of the global
+  config.
+
 ## [0.2.0]
 
 ### Added
@@ -65,5 +90,6 @@ Initial release.
   formula pushed to the tap.
 - Dual licensed under MIT OR Apache-2.0.
 
+[Unreleased]: https://github.com/jmarette/git-id/compare/v0.2.0...HEAD
 [0.2.0]: https://github.com/jmarette/git-id/compare/v0.1.0...v0.2.0
 [0.1.0]: https://github.com/jmarette/git-id/releases/tag/v0.1.0

README.md

@@ -166,8 +166,9 @@ $ git-id completions nushell | save -f ~/.config/nushell/completions/git-id.nu
   `.git` directory, which lives under the *main* repository. A worktree placed
   under a routed directory whose main repo lives elsewhere keeps the main
   repo's identity. `git id which` detects and explains this.
-- **Bare repository exactly at a routed root** is not matched (repositories
-  *below* the routed directory, bare or not, are fine).
+- **Bare repositories** route like any other: because routes are stored with a
+  trailing slash, a route matches a bare repo whose git directory *is* the
+  routed directory itself, as well as every repository below it.
 - **Symlinked paths** are stored resolved (e.g. `/tmp/...` becomes
   `/private/tmp/...` on macOS) because that is what Git matches against.
 - `git id --help` tries to open a `git-id` man page and fails; use
@@ -221,8 +222,8 @@ Contributions are welcome.
 
 Commit messages follow [Conventional Commits](https://www.conventionalcommits.org)
 (`feat:`, `fix:`, `docs:`, `refactor:`, `test:`, `chore:`). Pull requests are
-squash-merged, so only the PR title has to follow the convention — your own
-commits can stay messy.
+merged with a merge commit (not squashed), so every commit is preserved in
+`master` — keep each commit focused and give it a Conventional Commits message.
 
 Contributions are dual-licensed under MIT OR Apache-2.0; see
 [License](#license).

src/commands/create.rs

@@ -44,10 +44,17 @@ pub fn run(env: &Env, args: &CreateArgs) -> Result<ExitCode> {
         }
     };
     let signing_key = match &args.signing_key {
-        Some(v) if !v.is_empty() => Some(v.clone()),
+        Some(v) if !v.is_empty() => {
+            store::validate_signing_key(v)?;
+            Some(v.clone())
+        }
         Some(_) => None,
         None if pure_interactive => {
-            let v = prompt::ask("Signing key (user.signingkey)", true, |_| Ok(()))?;
+            let v = prompt::ask(
+                "Signing key (user.signingkey)",
+                true,
+                store::validate_signing_key,
+            )?;
             if v.is_empty() { None } else { Some(v) }
         }
         None => None,

src/commands/delete.rs

@@ -40,13 +40,17 @@ pub fn run(env: &Env, args: &DeleteArgs) -> Result<ExitCode> {
         }
     }
 
-    if fragment_exists {
-        store::remove(env, &args.name)?;
-    }
+    // Rewrite routes first (atomic_write makes this step crash-safe), then
+    // remove the fragment: a mid-operation failure leaves a present-but-unrouted
+    // fragment (harmless; doctor reports it as info) rather than a route
+    // pointing at a deleted fragment.
     let removed = model.remove_identity(&args.name);
     if !removed.is_empty() {
         routes::save(env, &model)?;
     }
+    if fragment_exists {
+        store::remove(env, &args.name)?;
+    }
     if removed.is_empty() {
         println!("Deleted identity `{}`.", args.name);
     } else {

src/commands/doctor.rs

@@ -69,14 +69,26 @@ pub fn run(env: &Env) -> Result<ExitCode> {
         ));
     }
 
+    // Hand-added `[includeIf "gitdir:..."]` blocks live in `preserved`; count
+    // them alongside managed entries so a duplicate split across a managed
+    // route and a preserved block (git applies both) is still flagged.
+    let preserved_gitdirs: Vec<String> = model
+        .preserved
+        .iter()
+        .filter_map(|block| block.lines().next())
+        .filter_map(routes::parse_gitdir_header)
+        .collect();
     let mut counts: HashMap<&str, u32> = HashMap::new();
     for entry in &model.entries {
         *counts.entry(entry.gitdir.as_str()).or_insert(0) += 1;
     }
+    for gitdir in &preserved_gitdirs {
+        *counts.entry(gitdir.as_str()).or_insert(0) += 1;
+    }
     for (gitdir, n) in counts {
         if n > 1 {
             d.error(&format!(
-                "{n} routes exist for {gitdir} — run `git-id use` on it again to deduplicate"
+                "{n} routes exist for {gitdir} — git applies all of them (last wins); remove the duplicate `[includeIf]` block(s) from routes.gitconfig"
             ));
         }
     }
@@ -114,7 +126,11 @@ pub fn run(env: &Env) -> Result<ExitCode> {
         } else {
             let (canon, _) =
                 paths::canonicalize_anchored(Path::new(entry.gitdir.trim_end_matches('/')));
-            let canon_slash = paths::ensure_trailing_slash(canon.to_string_lossy().into_owned());
+            // Compare in git-path form (forward slashes, de-UNC'd) so the stored
+            // gitdir is not falsely flagged as non-canonical on Windows, where
+            // fs::canonicalize yields a `\\?\C:\...` path. Identity on Unix.
+            let canon_slash =
+                paths::ensure_trailing_slash(paths::to_git_path(&canon.to_string_lossy()));
             if canon_slash != entry.gitdir {
                 d.warn(&format!(
                     "route {pretty} differs from the directory's canonical path {canon_slash} — git matches canonical paths, re-run `git-id use` on it"
@@ -149,7 +165,7 @@ pub fn run(env: &Env) -> Result<ExitCode> {
         }
     }
 
-    if gitcfg::global_get("user.useConfigOnly")?.as_deref() == Some("true") {
+    if init::useconfigonly_is_enabled(env)? {
         d.ok("user.useConfigOnly is enabled — commits require an explicit identity");
     } else {
         d.info(

src/commands/init.rs

@@ -10,14 +10,53 @@ use crate::env::Env;
 use crate::paths::display_pretty;
 use crate::{gitcfg, paths, prompt, routes};
 
+/// The global-level config files git actually reads and that git-id may have
+/// written to, in increasing precedence order, existing files only. Mirrors
+/// git: an explicit `GIT_CONFIG_GLOBAL` is the *only* global file; otherwise
+/// git reads BOTH `$XDG_CONFIG_HOME/git/config` and `~/.gitconfig` (the latter
+/// wins). We scan these directly rather than `git config --global`, because
+/// `--global` resolves to a single file (`~/.gitconfig` once it exists) and so
+/// goes blind to an include we legitimately wrote into the XDG file before
+/// `~/.gitconfig` came to exist.
+pub fn global_config_files(env: &Env) -> Vec<PathBuf> {
+    let candidates = if let Some(global) = &env.git_config_global {
+        vec![global.clone()]
+    } else {
+        vec![
+            env.config_base.join("git").join("config"),
+            env.home.join(".gitconfig"),
+        ]
+    };
+    candidates.into_iter().filter(|p| p.exists()).collect()
+}
+
 /// Whether the global git config already includes our routes file.
 pub fn include_is_installed(env: &Env) -> Result<bool> {
     // Compare in git-path form so a path Git normalized to forward slashes on
     // Windows still matches what we wrote (and stays byte-identical on Unix).
     let want = paths::to_git_path(routes_path_str(env)?);
-    Ok(gitcfg::global_get_all("include.path")?.iter().any(|p| {
-        paths::to_git_path(p) == want || paths::expand_tilde(p, &env.home) == env.routes_file
-    }))
+    for file in global_config_files(env) {
+        for p in gitcfg::get_file_all(&file, "include.path")? {
+            if paths::to_git_path(&p) == want
+                || paths::expand_tilde(&p, &env.home) == env.routes_file
+            {
+                return Ok(true);
+            }
+        }
+    }
+    Ok(false)
+}
+
+/// Whether `user.useConfigOnly` resolves to true across the global config
+/// files (later files win, mirroring git's precedence).
+pub fn useconfigonly_is_enabled(env: &Env) -> Result<bool> {
+    let mut enabled = false;
+    for file in global_config_files(env) {
+        if let Some(value) = gitcfg::get_file_bool(&file, "user.useConfigOnly")? {
+            enabled = value;
+        }
+    }
+    Ok(enabled)
 }
 
 pub fn routes_path_str(env: &Env) -> Result<&str> {
@@ -47,7 +86,15 @@ impl Backup {
             .file_name()
             .and_then(|n| n.to_str())
             .unwrap_or("gitconfig");
-        let bak = target.with_file_name(format!("{name}.bak-{}", timestamp_utc()?));
+        // The timestamp has 1-second resolution, so find a free name rather than
+        // letting fs::copy silently clobber a same-second backup.
+        let ts = timestamp_utc()?;
+        let mut bak = target.with_file_name(format!("{name}.bak-{ts}"));
+        let mut n = 1;
+        while bak.exists() {
+            bak = target.with_file_name(format!("{name}.bak-{ts}-{n}"));
+            n += 1;
+        }
         fs::copy(&target, &bak)
             .with_context(|| format!("cannot back up {} to {}", target.display(), bak.display()))?;
         self.path = Some(bak);
@@ -84,7 +131,7 @@ pub fn run(env: &Env, args: &InitArgs) -> Result<ExitCode> {
         Declined,
         SkippedNonInteractive,
     }
-    let already_on = gitcfg::global_get("user.useConfigOnly")?.as_deref() == Some("true");
+    let already_on = useconfigonly_is_enabled(env)?;
     let uco = if already_on {
         Uco::AlreadyEnabled
     } else {

src/commands/uninstall.rs

@@ -18,7 +18,7 @@ const ROUTES_INCLUDE_REGEX: &str = r"git-id/routes\.gitconfig$";
 pub fn run(env: &Env, args: &UninstallArgs) -> Result<ExitCode> {
     let include = init::include_is_installed(env)?;
     let dir_exists = env.config_dir.exists();
-    let use_config_only = gitcfg::global_get("user.useConfigOnly")?.as_deref() == Some("true");
+    let use_config_only = init::useconfigonly_is_enabled(env)?;
 
     if !include && !dir_exists && !use_config_only {
         println!("git-id is not set up — nothing to remove.");
@@ -47,11 +47,16 @@ pub fn run(env: &Env, args: &UninstallArgs) -> Result<ExitCode> {
         }
     }
 
-    if include {
-        gitcfg::global_unset_all_matching("include.path", ROUTES_INCLUDE_REGEX)?;
-    }
-    if use_config_only {
-        gitcfg::global_unset("user.useConfigOnly")?;
+    // Clean every global config file git reads, not just the one `--global`
+    // resolves to: the include (or useConfigOnly) may live in the XDG file even
+    // when ~/.gitconfig now exists, and `--global` would never reach it.
+    for file in init::global_config_files(env) {
+        if include {
+            gitcfg::unset_all_matching_file(&file, "include.path", ROUTES_INCLUDE_REGEX)?;
+        }
+        if use_config_only {
+            gitcfg::unset_file(&file, "user.useConfigOnly")?;
+        }
     }
     if dir_exists {
         fs::remove_dir_all(&env.config_dir)

src/commands/use.rs

@@ -32,13 +32,28 @@ pub fn run(env: &Env, args: &UseArgs) -> Result<ExitCode> {
         eprintln!(
             "warning: {pretty_dir} does not exist yet — the route is recorded and will apply once it is created"
         );
-    } else if let Some(top) = gitcfg::show_toplevel(&nd.path)? {
-        let top_slash = paths::ensure_trailing_slash(top.to_string_lossy().into_owned());
-        if top_slash != nd.gitdir {
-            eprintln!(
-                "note: {pretty_dir} is inside the repository {} — the route affects repositories under it, not that repository itself",
-                display_pretty(&top_slash, &env.home)
-            );
+    } else {
+        let git_dir = gitcfg::absolute_git_dir(&nd.path)?;
+        let top = gitcfg::show_toplevel(&nd.path)?;
+        // Linked worktrees and submodules: git matches `gitdir:` against the
+        // real .git directory, which lives under the *main* repository — so a
+        // route on this directory's own path is recorded but never applied.
+        if let (Some(gd), Some(top)) = (&git_dir, &top) {
+            if !gd.starts_with(top) {
+                eprintln!(
+                    "warning: {pretty_dir} is a linked worktree or submodule whose git directory is {} — git matches routes against the main repository's location, so this route will NOT apply here; route the main repository instead",
+                    display_pretty(&gd.to_string_lossy(), &env.home)
+                );
+            }
+        }
+        if let Some(top) = &top {
+            let top_slash = paths::ensure_trailing_slash(top.to_string_lossy().into_owned());
+            if top_slash != nd.gitdir {
+                eprintln!(
+                    "note: {pretty_dir} is inside the repository {} — the route affects repositories under it, not that repository itself",
+                    display_pretty(&top_slash, &env.home)
+                );
+            }
         }
     }
 

src/gitcfg.rs

@@ -105,28 +105,21 @@ pub fn unset_file(file: &Path, key: &str) -> Result<()> {
     }
 }
 
-/// `git config --global --get <key>` -> None when absent (or when no global
-/// config file exists at all).
-pub fn global_get(key: &str) -> Result<Option<String>> {
-    let out = run(Command::new("git").args(["config", "--global", "--get", key]))?;
-    match out.status.code() {
-        Some(0) => Ok(Some(stdout_trimmed(&out))),
-        Some(1) => Ok(None),
-        _ => bail!(
-            "`git config --global --get {key}` failed: {}",
-            stderr_trimmed(&out)
-        ),
-    }
-}
-
-/// All values of a multi-valued global key; empty when absent.
-pub fn global_get_all(key: &str) -> Result<Vec<String>> {
-    let out = run(Command::new("git").args(["config", "--global", "--get-all", key]))?;
+/// All values of a multi-valued key in a specific file; empty when the key is
+/// absent (exit 1). The file is expected to exist — callers filter to existing
+/// files so a missing-file read error is never silently swallowed.
+pub fn get_file_all(file: &Path, key: &str) -> Result<Vec<String>> {
+    let out = run(Command::new("git")
+        .arg("config")
+        .arg("--file")
+        .arg(file)
+        .args(["--get-all", key]))?;
     match out.status.code() {
         Some(0) => Ok(stdout_trimmed(&out).lines().map(str::to_string).collect()),
         Some(1) => Ok(Vec::new()),
         _ => bail!(
-            "`git config --global --get-all {key}` failed: {}",
+            "`git config --file {} --get-all {key}` failed: {}",
+            file.display(),
             stderr_trimmed(&out)
         ),
     }
@@ -144,27 +137,19 @@ pub fn global_set(key: &str, value: &str) -> Result<()> {
     check(&out, &format!("`git config --global {key}`"))
 }
 
-/// Unset a single-valued global key; a missing key (exit 5) is fine.
-pub fn global_unset(key: &str) -> Result<()> {
-    let out = run(Command::new("git").args(["config", "--global", "--unset", key]))?;
-    match out.status.code() {
-        Some(0) | Some(5) => Ok(()),
-        _ => bail!(
-            "`git config --global --unset {key}` failed: {}",
-            stderr_trimmed(&out)
-        ),
-    }
-}
-
-/// Remove every global value of `key` matching `value_regex`; no match
-/// (exit 5) is fine.
-pub fn global_unset_all_matching(key: &str, value_regex: &str) -> Result<()> {
-    let out =
-        run(Command::new("git").args(["config", "--global", "--unset-all", key, value_regex]))?;
+/// Remove every value of `key` in a specific file matching `value_regex`; no
+/// match (exit 5) is fine.
+pub fn unset_all_matching_file(file: &Path, key: &str, value_regex: &str) -> Result<()> {
+    let out = run(Command::new("git")
+        .arg("config")
+        .arg("--file")
+        .arg(file)
+        .args(["--unset-all", key, value_regex]))?;
     match out.status.code() {
         Some(0) | Some(5) => Ok(()),
         _ => bail!(
-            "`git config --global --unset-all {key}` failed: {}",
+            "`git config --file {} --unset-all {key}` failed: {}",
+            file.display(),
             stderr_trimmed(&out)
         ),
     }