Complete docs/IMPROVEMENT_PLAN.md implementation (Phases 1-8) - #88
Merged
Conversation
Phases 1-6: regression harness, native Git mutation boundary, raw/effective config separation, shared reconciliation, checkout/removal/reset/recovery, command contracts and read-only inspection (R01-R27, R31 PASS). Phase 7: single native-Git mutation path; retired backend mutations, gitoxide-core/prodash deps and gix transport features; main.rs imports the library (no duplicate compilation); in-place config updates; real 1/10/100 benchmarks plus before/after measurement (add-one 4.9x/24x/61x linear to flat, all other paths at parity, identical Git invocation counts, no-op state identity held) (R28 PASS). Phase 8: required stable Linux/macOS/Windows CI plus informational beta/nightly, explicit MSRV gate, locked nextest/doctests, package job; gated release (tag verification, archive linkage/smoke checks, locked publish); immutable action SHAs; consolidated advisory policy with retired ignores removed (R29 PARTIAL - hosted execution unrun, R30 PASS). Review follow-ups: reopen preserves without_gix policy (with regression test), removed dead panicking conversion, fixed verify-tag prerequisites, corrected stale strategy docs and test names.
Native 'git submodule sync' looks the child remote up by URL first since Git 2.51 (repo_remote_from_url), so with a renamed child remote it maintains remote.selected.url and never creates remote.origin.url. expected_synced_urls still expected the branch-default/origin remote, failing nuke rebuild verification on hosted CI (Git 2.55.0) with 'Git did not synchronize the selected child URL'. Mirror native selection in both URL branches: prefer the child remote whose fetch URL matches the resolved URL, falling back to branch-default/origin. Enrich the mismatch error with expected vs actual values. Tests: existing r22 relative-URL test is the regression test (red pre-fix, green post-fix on Git 2.55.0); add r22 absolute-URL sibling covering the other branch. Verified green on Git 2.43.0/2.50.1/2.55.0, full suite 633/633, fmt clean, no new clippy warnings.
Covers the relative-superproject-remote configuration in expected_synced_urls: with parent remote '../super.git' and TOML URL '../reachable.git', native sync re-bases one '../' per submodule path component below the child (Git get_up_path), so lib/child gets '../../../reachable.git'. Verified against native 'git submodule sync', which writes the identical value.
git --git-path resolves storage through the filesystem while the expected path is joined from discovery strings, so on Windows the two spellings differ (8.3 short names, separator style) for the same directory and every add/init/sync/update preflight bails. Canonicalize both sides (resolving through the nearest existing ancestor when preflight runs before Git creates storage) so the containment check stays fail-closed without false positives. Report both spellings on mismatch. Tests: unit coverage for canonical_storage_form (existing, missing-leaf, and missing-nested paths). End-to-end proof is the Windows CI suite.
registered_name_for_path compared the raw .gitmodules path bytes against the normalized OS path, so on Windows (forward slashes in storage, backslashes in normalized paths) every lookup missed: change/sync silently applied nothing, and preflight reported phantom path changes. Fold separators before comparing on Windows; byte-exact elsewhere. Tests: unit coverage locking the per-platform contract; the existing r14/onboarding/spaced-path Windows failures are the end-to-end proof.
…odule doc Since Git 2.46 every commit spawns a detached maintenance run that transiently creates and asynchronously deletes .git/objects/maintenance.lock. Byte-exact tree snapshots raced that background deletion on loaded macOS runners (reset_batch_preflights_later_nested_repository_collision and r17_phase6_missing_implicit_config_explains_import_but_add_can_create_it). Also split the overlong first paragraph of the git_ops module doc, which tripped clippy::too_long_first_doc_paragraph under Rust 1.98.
…tput
validated_child compared git's forward-slash top-level against the
verbatim-UNC canonical root, so every checkout validation failed on Windows
('outside the requested path' / 'resolves to unrelated worktree', 196 tests).
Canonicalize the reported top-level before comparing, and canonicalize the
RepositoryContext worktree root the same way.
Store add paths with Git separators so submod.toml stays portable, redact
scheme-less //user:pass@ URLs that Git echoes without 'file:', and treat
OS-unstatable names (InvalidFilename) as absent in existence probes.
Tests: unit coverage for each normalization; completion syntax check probes
for a working bash instead of assuming one.
plan_add stringified the validated native path, so Windows wrote backslash spellings to submod.toml while every contract expects forward slashes. Reuse stored_submodule_path so both add paths share one portable spelling.
…ments PathBuf::join silently drops '.' components, so the alternate spelling collapsed to the canonical path on Linux (macOS masked it via /var). Format the spelling instead so the normalization under test is exercised.
Path == compares components, so no dot spelling can differ from its canonical form. A winding other/../sub spelling differs component-wise on every platform while resolving to the same directory.
…; portable display asserts Reuse gitmodules_path_matches for the post-add .gitmodules record check so git's forward-slash recording matches the native request spelling, and canonicalize the discovered git/common directories like the worktree root. Compare the canonical recovery directory in the reset display test, and spell recursive list paths with Git separators.
…e git Spawning git tens of thousands of times while Defender scans every file operation costs an order of magnitude on Windows. Exclude the workspace and temp trees (ephemeral runner, safe), and set in-memory git accelerators in the harness gitconfig every fixture command uses.
Resolving a relative URL against a Windows-spelled base (file://C:\...) propagated backslashes into the recorded URL, defeating exact-match remote lookups and the recorded-URL contract. Absolute URL results now use forward slashes; relative results keep their spelling.
Closed
Closed
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.
Completes the improvement plan. Prior group work (Phases 1-6, accepted) recovered and ported; Phase 7 finished from its mid-refactor state; Phase 8 gates added; adversarial review follow-ups applied.
What changed
Verification (local, this machine)
Known limits