diff --git a/src/core/mv.rs b/src/core/mv.rs index 36ba8ff..3866b27 100644 --- a/src/core/mv.rs +++ b/src/core/mv.rs @@ -11,8 +11,8 @@ use super::{ model::{LinkReplacement, MoveChange, MoveChangeKind, MovePreview, MoveTransaction}, progress::ProgressReporter, util::{ - collect_markdown_files, is_external_url, relative_path, strip_utf8_bom_prefix, - url_decode_link, + collect_markdown_files, is_external_url, relative_path, resolve_path, + strip_utf8_bom_prefix, url_decode_link, }, }; use crate::{LinkType, MdrefError, Reference, Result, core::pathdiff::diff_paths, find_links}; @@ -29,6 +29,37 @@ enum RegularFileMoveMethod { // LinkReplacement and MoveTransaction are now defined in the model module +/// Resolve a path to a canonical form suitable for **semantic comparison** +/// between `ReplacementPlan` keys. +/// +/// Paths enter the plan from several sources — user-supplied `source`/`dest` +/// arguments, `WalkBuilder` output via `collect_markdown_files`, and +/// `validate_move_paths` results — each with a different textual shape for the +/// same filesystem entry (`examples/main.md` vs `./examples/main.md` vs a fully +/// canonicalised absolute path). Direct `PathBuf` equality misses these, which +/// is how #6 sneaked in. +/// +/// We deliberately **do not** rewrite the plan's keys with the canonical form, +/// because those keys are surfaced downstream (e.g. as `MoveChange.path` in the +/// dry-run JSON) and should preserve the caller-supplied shape. Use this helper +/// only for transient comparisons via [`find_matching_plan_key`]. +fn canonical_plan_key(path: &Path) -> PathBuf { + path.canonicalize() + .or_else(|_| resolve_path(path)) + .unwrap_or_else(|_| path.to_path_buf()) +} + +/// Find the existing `ReplacementPlan` key that refers to the same filesystem +/// entry as `target`, comparing via [`canonical_plan_key`]. Returns a cloned +/// `PathBuf` so callers can pass it to `HashMap::remove` / `contains_key` +/// without fighting the borrow checker. +fn find_matching_plan_key(plan: &ReplacementPlan, target: &Path) -> Option { + let target_key = canonical_plan_key(target); + plan.keys() + .find(|key| canonical_plan_key(key) == target_key) + .cloned() +} + /// Execute a fallible closure within a transaction context. /// If the closure returns an error, the transaction is rolled back automatically. fn execute_with_rollback(transaction: &MoveTransaction, operation: F) -> Result<()> @@ -503,12 +534,16 @@ fn move_source_replacements_to_destination( source: &Path, destination: &Path, ) { - if let Some(source_replacements) = replacements_by_file.remove(source) { - let destination_replacements = replacements_by_file - .entry(destination.to_path_buf()) - .or_default(); - extend_unique_replacements(destination_replacements, source_replacements); - } + let Some(existing_source_key) = find_matching_plan_key(replacements_by_file, source) else { + return; + }; + let Some(source_replacements) = replacements_by_file.remove(&existing_source_key) else { + return; + }; + let destination_key = find_matching_plan_key(replacements_by_file, destination) + .unwrap_or_else(|| destination.to_path_buf()); + let destination_replacements = replacements_by_file.entry(destination_key).or_default(); + extend_unique_replacements(destination_replacements, source_replacements); } fn add_destination_replacements( @@ -520,9 +555,9 @@ fn add_destination_replacements( return; } - let destination_replacements = replacements_by_file - .entry(destination.to_path_buf()) - .or_default(); + let destination_key = find_matching_plan_key(replacements_by_file, destination) + .unwrap_or_else(|| destination.to_path_buf()); + let destination_replacements = replacements_by_file.entry(destination_key).or_default(); extend_unique_replacements(destination_replacements, replacements); } @@ -625,7 +660,9 @@ fn preview_regular_file_move( progress.set_message("Scanning references..."); let references = find_references(source, root, progress)?; let mut replacements_by_file = plan_external_replacements(&references, &resolved_dest)?; - replacements_by_file.remove(source); + if let Some(source_key) = find_matching_plan_key(&replacements_by_file, source) { + replacements_by_file.remove(&source_key); + } let internal_replacements = plan_internal_replacements(source, source, &resolved_dest)?; add_destination_replacements( &mut replacements_by_file, @@ -718,7 +755,9 @@ fn mv_regular_file( progress.set_message("Scanning references..."); let references = find_references(source, root, progress)?; let mut replacements_by_file = plan_external_replacements(&references, &resolved_dest)?; - replacements_by_file.remove(source); + if let Some(source_key) = find_matching_plan_key(&replacements_by_file, source) { + replacements_by_file.remove(&source_key); + } let internal_replacements = plan_internal_replacements(source, source, &resolved_dest)?; if dry_run { @@ -740,7 +779,9 @@ fn mv_regular_file( transaction.snapshot_file(file_path)?; } - if !internal_replacements.is_empty() && !replacements_by_file.contains_key(source) { + if !internal_replacements.is_empty() + && find_matching_plan_key(&replacements_by_file, source).is_none() + { transaction.snapshot_file(source)?; } @@ -821,12 +862,13 @@ fn mv_case_only_file( } let mut transaction = MoveTransaction::new(source.to_path_buf(), resolved_dest.to_path_buf()); - if replacements_by_file.contains_key(resolved_dest) { + let dest_key_in_plan = find_matching_plan_key(&replacements_by_file, resolved_dest); + if dest_key_in_plan.is_some() { transaction.snapshot_file(source)?; } for file_path in replacements_by_file .keys() - .filter(|path| *path != resolved_dest) + .filter(|path| Some(*path) != dest_key_in_plan.as_ref()) { transaction.snapshot_file(file_path)?; } diff --git a/tests/lib_mv_tests.rs b/tests/lib_mv_tests.rs index 576519e..7cddf87 100644 --- a/tests/lib_mv_tests.rs +++ b/tests/lib_mv_tests.rs @@ -2785,3 +2785,74 @@ fn test_mv_directory_dry_run_with_images() { "Dry-run should not modify references" ); } + +// ============= Regression tests for #6: relative source + self-reference ============= + +/// Regression test for #6. +/// +/// When the user invokes `mv` from a shell cwd with a **relative** source path +/// (no leading `./`) and `root="."`, the self-reference inside the source file +/// must still be recognised and rewritten — previously the `HashMap` key +/// comparison missed the self-reference entry because `WalkBuilder` produced +/// paths with a `./` prefix while the user supplied a bare relative path, +/// causing `apply_replacements` to read the stale pre-move path and fail. +#[test] +#[allow(clippy::unwrap_used)] +fn test_mv_relative_source_with_self_reference_succeeds() { + let temp_dir = TempDir::new().unwrap(); + write_file(temp_dir.path().join("page.md"), "[Self](page.md)\n"); + + with_current_dir(temp_dir.path(), || { + let result = mv("page.md", "./moved.md", ".", false, &NoopProgress); + assert!( + result.is_ok(), + "mv with bare relative source should succeed, got: {:?}", + result.err() + ); + }); + + let source = temp_dir.path().join("page.md"); + let target = temp_dir.path().join("moved.md"); + assert!(!source.exists(), "source should have been removed"); + assert!(target.exists(), "target should have been created"); + + let content = fs::read_to_string(&target).unwrap(); + assert!( + content.contains("moved.md"), + "self-reference should be rewritten to new filename, got: {content}" + ); + assert!( + !content.contains("page.md"), + "old self-reference must not survive, got: {content}" + ); +} + +/// Regression test for #6. +/// +/// A cross-reference from another file must still be rewritten when the user +/// supplies `source` as a bare relative path and `root="."`. Before the fix +/// the external-reference path happened to work (the `WalkBuilder`-produced +/// path was consistently used on both sides), but we lock in the end-to-end +/// guarantee here so future refactors can't regress it. +#[test] +#[allow(clippy::unwrap_used)] +fn test_mv_relative_source_rewrites_external_reference() { + let temp_dir = TempDir::new().unwrap(); + write_file(temp_dir.path().join("page.md"), "# Page\n"); + write_file(temp_dir.path().join("ref.md"), "[Link](page.md)\n"); + + with_current_dir(temp_dir.path(), || { + let result = mv("page.md", "./moved.md", ".", false, &NoopProgress); + assert!(result.is_ok(), "mv should succeed, got: {:?}", result.err()); + }); + + let ref_content = fs::read_to_string(temp_dir.path().join("ref.md")).unwrap(); + assert!( + ref_content.contains("moved.md"), + "external reference should point to the new filename, got: {ref_content}" + ); + assert!( + !ref_content.contains("page.md"), + "old external reference must not survive, got: {ref_content}" + ); +}