From 00afd1f18af771dd84580e82b6a1c63de141f653 Mon Sep 17 00:00:00 2001 From: StudentWeis Date: Sat, 25 Apr 2026 19:59:43 +0800 Subject: [PATCH] refactor(core): normalize WalkBuilder paths at source to eliminate ./ prefix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/core/mv.rs | 74 +++++++++-------------------------------- src/core/util.rs | 9 +++++ tests/lib_find_tests.rs | 39 ++++++++++++++++++++++ 3 files changed, 64 insertions(+), 58 deletions(-) diff --git a/src/core/mv.rs b/src/core/mv.rs index 3866b27..36ba8ff 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, resolve_path, - strip_utf8_bom_prefix, url_decode_link, + collect_markdown_files, is_external_url, relative_path, strip_utf8_bom_prefix, + url_decode_link, }, }; use crate::{LinkType, MdrefError, Reference, Result, core::pathdiff::diff_paths, find_links}; @@ -29,37 +29,6 @@ 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<()> @@ -534,16 +503,12 @@ fn move_source_replacements_to_destination( source: &Path, destination: &Path, ) { - 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); + 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); + } } fn add_destination_replacements( @@ -555,9 +520,9 @@ fn add_destination_replacements( 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(); + let destination_replacements = replacements_by_file + .entry(destination.to_path_buf()) + .or_default(); extend_unique_replacements(destination_replacements, replacements); } @@ -660,9 +625,7 @@ 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)?; - if let Some(source_key) = find_matching_plan_key(&replacements_by_file, source) { - replacements_by_file.remove(&source_key); - } + replacements_by_file.remove(source); let internal_replacements = plan_internal_replacements(source, source, &resolved_dest)?; add_destination_replacements( &mut replacements_by_file, @@ -755,9 +718,7 @@ 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)?; - if let Some(source_key) = find_matching_plan_key(&replacements_by_file, source) { - replacements_by_file.remove(&source_key); - } + replacements_by_file.remove(source); let internal_replacements = plan_internal_replacements(source, source, &resolved_dest)?; if dry_run { @@ -779,9 +740,7 @@ fn mv_regular_file( transaction.snapshot_file(file_path)?; } - if !internal_replacements.is_empty() - && find_matching_plan_key(&replacements_by_file, source).is_none() - { + if !internal_replacements.is_empty() && !replacements_by_file.contains_key(source) { transaction.snapshot_file(source)?; } @@ -862,13 +821,12 @@ fn mv_case_only_file( } let mut transaction = MoveTransaction::new(source.to_path_buf(), resolved_dest.to_path_buf()); - let dest_key_in_plan = find_matching_plan_key(&replacements_by_file, resolved_dest); - if dest_key_in_plan.is_some() { + if replacements_by_file.contains_key(resolved_dest) { transaction.snapshot_file(source)?; } for file_path in replacements_by_file .keys() - .filter(|path| Some(*path) != dest_key_in_plan.as_ref()) + .filter(|path| *path != resolved_dest) { transaction.snapshot_file(file_path)?; } diff --git a/src/core/util.rs b/src/core/util.rs index 78325e0..ca078e0 100644 --- a/src/core/util.rs +++ b/src/core/util.rs @@ -5,6 +5,10 @@ use ignore::WalkBuilder; use crate::{MdrefError, Result, core::pathdiff::diff_paths}; /// Collect markdown files while respecting ignore files such as `.gitignore`. +/// +/// Returned paths are normalised: a leading `./` prefix (produced by +/// `WalkBuilder` when `root` is `"."`) is stripped so that path shapes +/// match user-supplied relative paths. pub fn collect_markdown_files(root: &Path) -> Vec { let mut builder = WalkBuilder::new(root); builder.standard_filters(true).require_git(false); @@ -18,6 +22,11 @@ pub fn collect_markdown_files(root: &Path) -> Vec { .is_some_and(|file_type| file_type.is_file()) }) .map(|entry| entry.into_path()) + .map(|path| { + path.strip_prefix("./") + .map(|stripped| stripped.to_path_buf()) + .unwrap_or(path) + }) .filter(|path| path.extension().and_then(|ext| ext.to_str()) == Some("md")) .collect() } diff --git a/tests/lib_find_tests.rs b/tests/lib_find_tests.rs index beaa674..a44d859 100644 --- a/tests/lib_find_tests.rs +++ b/tests/lib_find_tests.rs @@ -563,3 +563,42 @@ fn test_find_references_handles_unicode_paths( assert_eq!(result.len(), 1, "Should find one Unicode reference"); assert_eq!(result[0].link_text, expected_link_text); } + +// ============= Path normalization tests ============= + +/// Regression test for #8. +/// +/// When `root` is `"."`, `find_references` should return `Reference.path` values +/// without a `./` prefix — they should match the shape a user would type on the +/// command line (e.g. `sub/ref.md`, not `./sub/ref.md`). +#[test] +#[allow(clippy::unwrap_used)] +fn test_find_references_paths_have_no_dot_slash_prefix() { + let temp_dir = TempDir::new().unwrap(); + let target = temp_dir.path().join("target.md"); + write_file(&target, "# Target"); + + let sub_dir = temp_dir.path().join("sub"); + fs::create_dir_all(&sub_dir).unwrap(); + let ref_file = sub_dir.join("ref.md"); + write_file(&ref_file, "[Link](../target.md)"); + + // Use "." as root — this is the shape that triggers WalkBuilder's ./ prefix. + let original_dir = std::env::current_dir().unwrap(); + std::env::set_current_dir(temp_dir.path()).unwrap(); + + let result = find_references("target.md", ".", &NoopProgress).unwrap(); + + std::env::set_current_dir(&original_dir).unwrap(); + + assert_eq!(result.len(), 1, "Should find exactly one reference"); + let ref_path = result[0].path.to_string_lossy(); + assert!( + !ref_path.starts_with("./"), + "Reference path should not have ./ prefix, got: {ref_path}" + ); + assert_eq!( + ref_path, "sub/ref.md", + "Reference path should be a clean relative path" + ); +}