fix(mv): resolve self-reference removal when source path shape mismatches#7
Merged
Merged
Conversation
…ches ReplacementPlan keys originated from three different path shapes (user argument, WalkBuilder output, and canonicalized destinations) but were compared by literal PathBuf equality. When a user passed a bare relative source such as `examples/main.md` alongside `root="."`, WalkBuilder produced `./examples/main.md`, so the self-reference entry could never be removed; after `fs::rename` moved the source away, `apply_replacements` then tried to read the stale pre-move path and failed with `IoRead`, with rollback masking the effect. Fix by funnelling plan-key lookups (`remove`, `contains_key`, inequality checks) through `find_matching_plan_key`, which compares keys via `canonical_plan_key` (`canonicalize` → `resolve_path` fallback). The plan's stored keys keep their original textual form so dry-run JSON output is unaffected. Regression tests cover the bare-relative-source + self-reference and external-reference scenarios. Refs #6
Closed
3 tasks
StudentWeis
added a commit
that referenced
this pull request
Apr 25, 2026
… prefix (#9) collect_markdown_files now strips the leading './' that WalkBuilder produces when root is '.', so every returned path matches the shape a user would type on the command line. With this normalisation in place the transient canonical_plan_key / find_matching_plan_key workaround from #7 is redundant — all ReplacementPlan key operations revert to plain HashMap lookups, honouring KISS. The two regression tests from #6 continue to pass, confirming the source-level fix is sufficient. A new test locks in the 'no ./ prefix' invariant. Refs #8
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.
Summary
Fix
mvfailing withIO error reading '<./source>'when the source file contains a self-reference and the caller passes a bare relative source path alongsideroot=".". The move now succeeds and rewrites the self-reference, matching the behaviour users already get for canonical or./-prefixed sources.Linked Issue
Closes #6
Changes
src/core/mv.rs: addcanonical_plan_key+find_matching_plan_keyhelpers and funnel everyReplacementPlanlookup (remove,contains_key, inequality checks) through them. The plan's stored keys keep their original textual shape so dry-run JSON output is unaffected.tests/lib_mv_tests.rs: two regression tests that reproduce the bug from aTempDircwd — one for the self-reference path (previously failed withIoRead), and one locking in the external-reference rewrite under the same bare-relative-source shape.Testing
scripts/precheck.shpasses locally (cargo clippy: No issues found,cargo test: 274 passed, 1 ignoredacross 9 suites)examples/main.mdnow succeeds where it previously failed