Fix review findings: injection, Windows doctor, config detection, worktree, docs#1
Merged
Conversation
Reject control characters in signing keys (a newline could otherwise inject an arbitrary section — e.g. core.sshCommand — into the fragment git includes for routed repos), escape control characters when rendering as defense in depth, and validate every field of an edit up front so a rejected patch is a no-op instead of leaving an earlier field already written.
git config --global resolves to a single file (~/.gitconfig once it exists), so detecting and removing the include or user.useConfigOnly went blind to the XDG file when ~/.gitconfig appeared later. Scan the concrete global files directly, and let uninstall clean whichever one holds them.
doctor recomputed a route's canonical path and compared it to the stored gitdir without to_git_path, so on Windows (where fs::canonicalize yields \\?\C:\...) every routed directory was falsely flagged as non-canonical.
doctor counted only managed entries, so a duplicate split across a managed route and a hand-added [includeIf] block (git applies both) went unreported.
git matches gitdir: against the .git directory under the main repository, so a route on a linked worktree's or submodule's own path is never applied; use now warns instead of silently recording a dead route.
The backup name used a 1-second-resolution timestamp and fs::copy overwrites, so two mutating init runs in the same second lost a backup; pick a free name.
Saving routes (crash-safe via atomic_write) first means an interruption leaves a present-but-unrouted fragment rather than a route pointing at a deleted fragment.
A bare repo at a routed root IS matched (routes are stored with a trailing slash); correct the README caveat, and reopen an Unreleased changelog section documenting these fixes.
End-to-end coverage for signing-key rejection, atomic edits, XDG include detection/uninstall, useConfigOnly idempotency, a nonexistent GIT_CONFIG_GLOBAL target, doctor's non-canonical and duplicate diagnostics, and the linked-worktree warning.
The repo merges PRs with a merge commit (squash is disabled), so every commit is preserved in master; tell contributors to make each commit a clean Conventional Commit rather than relying on the PR title.
The setup commit in the (unrouted) dev/main repo had no user.name/email. CI runners — unlike a dev machine, where git auto-derives an ident from the OS user — have an empty ident, so the commit (and the test) failed on Linux and Windows.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A full review of the codebase surfaced several correctness, cross-platform and
input-validation issues. This PR fixes them, adds regression tests, and corrects
a couple of stale doc claims. One focused commit per finding (the repo merges
with merge commits, so each commit is preserved).
Fixes
Security / input validation
fragment — a newline in
--signing-keycould otherwise inject an arbitrarygitconfig section (e.g.
core.sshCommand) into the fragment git includes forevery routed repo. The
editpath was already safe (git config --file).Cross-platform correctness
doctorcompared a route's recomputed canonical path against the storedgitdir without
to_git_path, so on Windows (\\?\C:\...) every routeddirectory was falsely flagged as non-canonical, with advice that never cleared.
Git-semantics correctness
init/doctor/uninstalldetected the include anduser.useConfigOnlyviagit config --global, which resolves to a single file; they went blind to theXDG global config once
~/.gitconfigappeared later (souninstallcouldleave the include behind). They now scan the concrete global files directly.
usewarns when routing a linked worktree's or submodule's own path, wheregit matches the main repository and the route would never apply.
Robustness
editvalidates all fields before writing any (no partial write on a rejectedpatch).
deleterewrites the routes file (crash-safe) before removing the fragment.doctorflags a duplicate gitdir route even when one side is a hand-added(preserved)
[includeIf]block.initno longer clobbers a same-second backup of the global config.Docs
matched (routes are stored with a trailing slash). Corrected.
## [Unreleased]changelog section.Conventional Commit.
Tests
test for the
doctorcanonical comparison that runs on the macOS/Linux hosts.cargo fmt --check,cargo clippy --all-targets -- -D warnings, andcargo test(51 unit + 48 integration) all green.