From 928c2c974fbdbb129e30f0039447a2000d27f6fa Mon Sep 17 00:00:00 2001 From: radialmonster Date: Wed, 7 Oct 2026 08:35:01 -0400 Subject: [PATCH] Windows: treat separator-only path differences as the same file Catalog paths may mix `/` and `\` on Windows, so raw string comparison saw `N:/photos/IMG.jpg` and `N:/photos\IMG.jpg` as different files: - Rename: planning, the move list, move_file_with and file_moves now compare as paths. A separator-only "rename" is a no-op: no file move, no catalog change and no undo entry, alone or batched with a genuine rename. Previously it attempted a plain rename onto the existing file, failed, and rolled back the rest of the batch. Case-only renames still work (#95). - Import: the already-in-library index is keyed by PathBuf. - Tests build expected paths with Path::join, and the CLI test escapes the device path with serde_json instead of string formatting. Split from a327b98 (T0 thumbnail work), with the follow-up rename fix that the committed version needed. Co-authored-by: Codex Co-Authored-By: Claude Opus 5.5 --- apps/lightcraft-cli/tests/cli.rs | 2 +- crates/engine/src/import.rs | 8 ++-- crates/engine/src/rename.rs | 73 ++++++++++++++++++++++++++----- crates/engine/src/tests_import.rs | 10 +++-- crates/ui-egui/src/headless.rs | 8 +++- 5 files changed, 80 insertions(+), 21 deletions(-) diff --git a/apps/lightcraft-cli/tests/cli.rs b/apps/lightcraft-cli/tests/cli.rs index 37799f4a..6e015af3 100644 --- a/apps/lightcraft-cli/tests/cli.rs +++ b/apps/lightcraft-cli/tests/cli.rs @@ -292,7 +292,7 @@ fn devices_are_listed_and_imported_from() { assert_eq!(dev["name"], "CARD", "{line}"); // the import review of its DCIM folder finds the photo let dcim = dev["path"].as_str().unwrap().to_string(); - let o = Command::new(BIN).args(["run", "library.importPreview", &format!("paths=[\"{dcim}\"]")]).output().unwrap(); + let o = Command::new(BIN).args(["run", "library.importPreview", &format!("paths={}", serde_json::json!([dcim]))]).output().unwrap(); let line: Value = serde_json::from_slice(o.stdout.split(|b| *b == b'\n').next().unwrap()).unwrap(); assert_eq!(line["result"]["candidates"].as_array().map(Vec::len), Some(1), "{line}"); } diff --git a/crates/engine/src/import.rs b/crates/engine/src/import.rs index 6f35b670..80e5eb43 100644 --- a/crates/engine/src/import.rs +++ b/crates/engine/src/import.rs @@ -393,7 +393,7 @@ pub struct ScanInput { probe: Option, skip: Option, /// path → (photo, its summary) for files already in the library. - by_path: HashMap, + by_path: HashMap, by_hash: HashMap, cache: HashMap, } @@ -432,7 +432,7 @@ impl ScanInput { existing: Some(p.id.0), ..Default::default() }; - by_path.insert(path.clone(), (p.id, c)); + by_path.insert(PathBuf::from(path), (p.id, c)); } if let Some(h) = &p.content_hash { by_hash.insert(h.clone(), p.id); @@ -458,7 +458,7 @@ pub fn scan(s: &mut Session, paths: &[String]) -> Vec { pub fn scan_with(mut input: ScanInput, paths: &[String], progress: &ScanProgress) -> ScanOutput { use std::sync::atomic::Ordering::Relaxed; let files = expand(paths, input.skip.as_deref()); - let todo: Vec = files.iter().filter(|f| !input.by_path.contains_key(f.as_str())).cloned().collect(); + let todo: Vec = files.iter().filter(|f| !input.by_path.contains_key(Path::new(f))).cloned().collect(); progress.total.store(todo.len(), Relaxed); // probes from a preceding `scan` are reused when the file is unchanged (same size) let cached: Vec> = todo @@ -480,7 +480,7 @@ pub fn scan_with(mut input: ScanInput, paths: &[String], progress: &ScanProgress let mut out = Vec::with_capacity(files.len()); let mut kept = HashMap::new(); for f in files { - if let Some((_, c)) = input.by_path.get(&f) { + if let Some((_, c)) = input.by_path.get(Path::new(&f)) { let mut c = c.clone(); c.name = Path::new(&f).file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_else(|| f.clone()); out.push(c); diff --git a/crates/engine/src/rename.rs b/crates/engine/src/rename.rs index 27fc5fb5..31e8e02c 100644 --- a/crates/engine/src/rename.rs +++ b/crates/engine/src/rename.rs @@ -395,7 +395,12 @@ impl MoveFs for RealFs { /// `from` → `to` differs only in letter case and `to` is that very file (a case-insensitive /// volume): the rename must go through a temporary name. fn is_respelling(fs: &dyn MoveFs, from: &Path, to: &Path) -> bool { - from != to && from.to_string_lossy().to_lowercase() == to.to_string_lossy().to_lowercase() && fs.exists(to) && fs.same_file(from, to) + from != to && folded_path(&from.to_string_lossy()) == folded_path(&to.to_string_lossy()) && fs.exists(to) && fs.same_file(from, to) +} + +/// Windows accepts both separators; spelling differences must not hide file identity. +fn folded_path(path: &str) -> String { + if cfg!(windows) { path.replace('\\', "/").to_lowercase() } else { path.to_lowercase() } } /// Rename `a` to `b` without ever replacing a file; a change of letter case only goes through a @@ -439,7 +444,7 @@ pub fn move_file(from: &str, to: &str) -> std::result::Result<(), MoveError> { /// [`move_file`] on the file system `fs`. pub(crate) fn move_file_with(fs: &dyn MoveFs, from: &str, to: &str) -> std::result::Result<(), MoveError> { - if from == to { + if Path::new(from) == Path::new(to) { return Ok(()); } let fail = |message: String| MoveError { message, moved: false }; @@ -590,11 +595,11 @@ fn plan_rename_core( loop { let name = candidate(k); let tp = dir.join(&name).to_string_lossy().to_string(); - let key = tp.to_lowercase(); + let key = folded_path(&tp); // this very file, maybe spelled differently (case-insensitive volume)? By // identity: on a case-sensitive volume `img_1.jpg` may be another photo let (from, to) = (Path::new(path), Path::new(&tp)); - let same = tp == *path || (from != to && key == path.to_lowercase() && exists(to) && same_file(from, to)); + let same = from == to || (from != to && key == folded_path(path) && exists(to) && same_file(from, to)); // free: not claimed in this batch and not on disk (unless it is this very file). // A file this batch moves away still counts as taken: simple and safe. if !taken.contains(&key) && (same || !exists(to)) { @@ -656,6 +661,7 @@ impl Session { }; let Some(plan) = plan else { continue }; let source = match &plan.to_path { + Some(t) if matches!(&p.source, Source::File { path } if Path::new(path) == Path::new(t)) => p.source.clone(), Some(t) => Source::File { path: t.clone() }, None => p.source.clone(), }; @@ -668,7 +674,7 @@ impl Session { let moves: Vec<(String, String)> = plans .iter() .filter_map(|pl| match (&pl.from_path, &pl.to_path) { - (Some(a), Some(b)) if a != b => Some((a.clone(), b.clone())), + (Some(a), Some(b)) if Path::new(a) != Path::new(b) => Some((a.clone(), b.clone())), _ => None, }) .collect(); @@ -732,7 +738,7 @@ impl Session { Op::Batch { ops } => ops.iter().for_each(|o| rec(s, o, v)), Op::SetFile { id, source: Source::File { path: to }, .. } => { if let Some(Source::File { path: from }) = s.catalog.photo(*id).map(|p| &p.source) - && from != to + && Path::new(from) != Path::new(to) && !v.iter().any(|(f, _)| f == from) { v.push((from.clone(), to.clone())); @@ -770,11 +776,14 @@ mod tests { } fn idx(&self, p: &Path) -> Option { let p = p.to_string_lossy(); - self.files.borrow().iter().position(|(f, _)| if self.ci { f.to_lowercase() == p.to_lowercase() } else { *f == p }) + self.files + .borrow() + .iter() + .position(|(f, _)| if self.ci { folded_path(f) == folded_path(&p) } else { Path::new(f) == Path::new(p.as_ref()) }) } /// The listing: (exact path, contents), sorted. pub fn listing(&self) -> Vec<(String, String)> { - let mut v: Vec<_> = self.files.borrow().iter().map(|(p, c)| (p.clone(), String::from_utf8_lossy(c).to_string())).collect(); + let mut v: Vec<_> = self.files.borrow().iter().map(|(p, c)| (p.replace('\\', "/"), String::from_utf8_lossy(c).to_string())).collect(); v.sort(); v } @@ -829,6 +838,48 @@ mod tests { Photo::new(PhotoId(id), Source::File { path: path.into() }, &name, "JPG", 1, 1, "2026-01-01T00:00:00") } + #[cfg(windows)] + #[test] + fn mixed_separator_noop_rename_preserves_files_catalog_and_undo() { + let fs = FakeFs::new(true, &[("N:/photos/IMG.jpg", "image"), ("N:/photos/IMG.xmp", "edits"), ("N:/photos/other.jpg", "other")]); + // Any attempt to move the unchanged photo (including rollback) must fail. + fs.fail.borrow_mut().push("IMG".into()); + let mut s = Session::new(); + for (id, path) in [(1, "N:/photos/IMG.jpg"), (2, "N:/photos/other.jpg")] { + s.catalog.apply(Op::AddPhoto { photo: Box::new(file_photo(id, path)) }).unwrap(); + } + let before = fs.listing(); + let plans = s.plan_rename_with(&fs, &[PhotoId(1)], "{name}", 1); + assert_ne!(plans[0].from_path, plans[0].to_path, "fixture exercises different separator spelling"); + assert_eq!(s.apply_rename_with(&fs, &plans).unwrap(), 0); + assert_eq!(fs.listing(), before); + assert_eq!(s.catalog.photo(PhotoId(1)).unwrap().source, Source::File { path: "N:/photos/IMG.jpg".into() }); + assert!(s.undo.is_empty()); + move_file_with(&fs, "N:/photos/IMG.jpg", "N:/photos\\IMG.jpg").unwrap(); + let separator_only = Op::SetFile { id: PhotoId(1), file_name: "IMG.jpg".into(), source: Source::File { path: "N:/photos\\IMG.jpg".into() } }; + assert!(s.file_moves(&separator_only).is_empty()); + + // A batch may combine that no-op with a genuine rename, without touching IMG or its sidecar. + let mut batch = plans; + batch.extend(s.plan_rename_with(&fs, &[PhotoId(2)], "renamed", 1)); + assert_eq!(s.apply_rename_with(&fs, &batch).unwrap(), 1); + assert_eq!( + fs.listing(), + vec![ + ("N:/photos/IMG.jpg".into(), "image".into()), + ("N:/photos/IMG.xmp".into(), "edits".into()), + ("N:/photos/renamed.jpg".into(), "other".into()) + ] + ); + let undo = &s.undo.last().unwrap().op; + let moves = s.file_moves(undo); + assert_eq!(moves.len(), 1); + assert_eq!(Path::new(&moves[0].1), Path::new("N:/photos/other.jpg")); + move_all(&fs, &moves).unwrap(); + s.catalog.apply(undo.clone()).unwrap(); + assert_eq!(fs.listing(), before); + } + /// Issue #95: a case-only rename is a real change on a case-insensitive volume and never /// replaces a different file that differs only by case on a case-sensitive one. #[test] @@ -884,7 +935,7 @@ mod tests { let plans = s.plan_rename_with(&fs, &ids, "Trip-{seq}", 1); // c can't be renamed (the share went away), and neither can Trip-1 be moved back fs.fail.borrow_mut().extend(["c.jpg".to_string(), "Trip-1".to_string()]); - let err = s.apply_rename_with(&fs, &plans).unwrap_err().to_string(); + let err = s.apply_rename_with(&fs, &plans).unwrap_err().to_string().replace('\\', "/"); assert!(err.contains("rename /p/c.jpg"), "{err}"); assert!(err.contains("/p/Trip-1.jpg could not be moved back"), "{err}"); assert!(err.contains("/p/a.jpg → /p/Trip-1.jpg"), "lists what stayed renamed: {err}"); @@ -899,7 +950,7 @@ mod tests { ); // the catalog matches the disk let path = |s: &Session, id: u64| match &s.catalog.photo(PhotoId(id)).unwrap().source { - Source::File { path } => path.clone(), + Source::File { path } => path.replace('\\', "/"), Source::Demo { .. } => String::new(), }; assert_eq!(path(&s, 1), "/p/Trip-1.jpg"); @@ -913,7 +964,7 @@ mod tests { fs.fail.borrow_mut().extend(["d.jpg.xmp".to_string(), "/p/e.jpg".to_string()]); let e = move_file_with(&fs, "/p/d.jpg", "/p/e.jpg").unwrap_err(); assert!(e.moved, "{e}"); - assert!(e.message.contains("it is now /p/e.jpg with its sidecar /p/e.xmp"), "{e}"); + assert!(e.message.replace('\\', "/").contains("it is now /p/e.jpg with its sidecar /p/e.xmp"), "{e}"); assert_eq!(fs.listing(), vec![("/p/d.jpg.xmp".into(), "Df".into()), ("/p/e.jpg".into(), "D".into()), ("/p/e.xmp".into(), "Ds".into())]); // … and when the file does go back, its sidecars follow it let fs = FakeFs::new(false, &[("/p/d.jpg", "D"), ("/p/d.xmp", "Ds"), ("/p/d.jpg.xmp", "Df")]); diff --git a/crates/engine/src/tests_import.rs b/crates/engine/src/tests_import.rs index dd3f7ca1..af106765 100644 --- a/crates/engine/src/tests_import.rs +++ b/crates/engine/src/tests_import.rs @@ -181,8 +181,12 @@ fn find_missing_matches_renamed_files_by_content() { let to = |name: &str| { found.iter().find(|f| f["from"].as_str().unwrap().ends_with(name)).map(|f| (f["to"].as_str().unwrap().to_string(), f["by"].clone())) }; - assert_eq!(to("a.png"), Some((dir.join("moved/Trip-001.png").to_string_lossy().to_string(), json!("content")))); - assert_eq!(to("b.png"), Some((dir.join("moved/real/Trip-002.png").to_string_lossy().to_string(), json!("content"))), "not the impostor"); + assert_eq!(to("a.png"), Some((dir.join("moved").join("Trip-001.png").to_string_lossy().to_string(), json!("content")))); + assert_eq!( + to("b.png"), + Some((dir.join("moved").join("real").join("Trip-002.png").to_string_lossy().to_string(), json!("content"))), + "not the impostor" + ); assert_eq!(r["missing"], 1); assert_eq!(r["ambiguous"][0]["candidates"].as_array().map(Vec::len), Some(2), "{r}"); let _ = std::fs::remove_dir_all(&dir); @@ -582,7 +586,7 @@ fn folders_rename_and_move_with_their_photos() { assert!(s.execute("folder.move", &json!({"path": b.to_string_lossy(), "into": b.join("deeper").to_string_lossy()})).is_err(), "not into itself"); let r = s.execute("folder.move", &json!({"path": b.to_string_lossy(), "into": root.join("Archive").to_string_lossy()})).unwrap(); assert_eq!(r["relinked"], 1); - assert_eq!(path(&s), root.join("Archive/Italy 2026/one.png").to_string_lossy()); + assert_eq!(path(&s), root.join("Archive").join("Italy 2026").join("one.png").to_string_lossy()); let _ = std::fs::remove_dir_all(&root); } diff --git a/crates/ui-egui/src/headless.rs b/crates/ui-egui/src/headless.rs index a6b8fc43..25d54167 100644 --- a/crates/ui-egui/src/headless.rs +++ b/crates/ui-egui/src/headless.rs @@ -857,8 +857,12 @@ mod tests { std::fs::create_dir_all(base.join(d)).unwrap(); } let s = |p: std::path::PathBuf| p.to_string_lossy().to_string(); - let (photos, day1, day2, other) = - (s(base.join("Photos")), s(base.join("Photos/2026/20260101")), s(base.join("Photos/2026/20260114")), s(base.join("Other"))); + let (photos, day1, day2, other) = ( + s(base.join("Photos")), + s(base.join("Photos").join("2026").join("20260101")), + s(base.join("Photos").join("2026").join("20260114")), + s(base.join("Other")), + ); let exec = |h: &mut Headless, c: &str, p: Value| h.request("engine.execute", json!({"command": c, "params": p}), t); let rects = |h: &mut Headless| -> std::collections::HashMap { let w = h.request("ui.widgets", json!({"filter": "lc-ui-roots-"}), t);