diff --git a/apps/lightcraft/src/editors.rs b/apps/lightcraft/src/editors.rs index 9a8cc22d2..8b99e284d 100644 --- a/apps/lightcraft/src/editors.rs +++ b/apps/lightcraft/src/editors.rs @@ -172,8 +172,10 @@ fn desktop_exec(entry: &str) -> Option { Some(rest) => rest.split('"').next()?, None => exec.split_whitespace().next()?, }; - // a bare name (`photocraft %F`) was already looked for on the PATH - Some(PathBuf::from(program)).filter(|p| p.is_absolute()) + // a bare name (`photocraft %F`) was already looked for on the PATH. Desktop entries are a + // freedesktop (POSIX) format: absolute means a leading `/` on every platform the parser runs on + // (`Path::is_absolute` wants a drive letter on Windows, where the tests run too) + program.starts_with('/').then(|| PathBuf::from(program)) } #[cfg(test)] diff --git a/crates/catalog/src/lock.rs b/crates/catalog/src/lock.rs index 8a2b496d6..d699da049 100644 --- a/crates/catalog/src/lock.rs +++ b/crates/catalog/src/lock.rs @@ -135,8 +135,9 @@ impl LibraryLock { } /// Another descriptor on the locked file: what a process spawned by another thread holds - /// between fork and exec (a `dup` refers to the same open file description). Tests only. - #[cfg(test)] + /// between fork and exec (a `dup` refers to the same open file description). Tests only (the + /// test that uses it is Unix-only: `flock` semantics). + #[cfg(all(test, unix))] pub(crate) fn descriptor_copy(&self) -> Option { self.file.as_ref().and_then(|f| f.try_clone().ok()) } diff --git a/crates/engine/src/rename.rs b/crates/engine/src/rename.rs index c5ff9a0c4..e49355c08 100644 --- a/crates/engine/src/rename.rs +++ b/crates/engine/src/rename.rs @@ -319,6 +319,17 @@ pub(crate) trait MoveFs { /// Another photo file beside the stem-named sidecar `sidecar` that shares its stem (e.g. /// `IMG_1.JPG` for `IMG_1.xmp`), which still uses that sidecar. fn stem_sibling(&self, sidecar: &Path) -> Option; + /// The folder lists an entry spelled exactly like `p` (letter case included). On a + /// case-insensitive volume `a.XMP` "exists" when only `a.xmp` is there; this tells the two + /// apart. When the folder can't be listed: `true`, so nothing is skipped. + fn listed(&self, p: &Path) -> bool { + let (Some(dir), Some(name)) = (p.parent(), p.file_name()) else { return true }; + let dir = if dir.as_os_str().is_empty() { Path::new(".") } else { dir }; + match std::fs::read_dir(dir) { + Ok(entries) => entries.flatten().any(|e| e.file_name() == name), + Err(_) => true, + } + } /// Content observed in a packet, independent of its pathname or inode. Model volumes /// override this; disk implementations stream a bounded buffer without retaining the XML. fn packet_version(&self, path: &Path) -> std::io::Result { @@ -366,15 +377,23 @@ impl MoveFs for RealFs { } #[cfg(not(unix))] { - // no stable file id in std here: `b` is `a` under another spelling when they look - // alike and the folder has no entry spelled exactly like `b` (on a case-sensitive - // folder holding both `IMG.JPG` and `img.jpg`, both entries are listed) - let (Some(dir), Some(name)) = (b.parent(), b.file_name()) else { return false }; - if a.parent() != Some(dir) || ma.len() != mb.len() || ma.modified().ok() != mb.modified().ok() { + // no stable file id in std here: `a` and `b` are one file under two spellings when + // they look alike and the folder doesn't list both (on a case-sensitive folder + // holding both `IMG.JPG` and `img.jpg`, both entries are listed). Either may be the + // listed one: the answer must not depend on the argument order. + let (Some(dir), Some(name_a), Some(name_b)) = (b.parent(), a.file_name(), b.file_name()) else { return false }; + let respelled = name_a.to_string_lossy().to_lowercase() == name_b.to_string_lossy().to_lowercase(); + if !respelled || a.parent() != Some(dir) || ma.len() != mb.len() || ma.modified().ok() != mb.modified().ok() { return false; } let Ok(entries) = std::fs::read_dir(dir) else { return false }; - !entries.flatten().any(|e| e.file_name() == name) + let (mut listed_a, mut listed_b) = (false, false); + for e in entries.flatten() { + let name = e.file_name(); + listed_a |= name == name_a; + listed_b |= name == name_b; + } + !(listed_a && listed_b) } } @@ -699,15 +718,14 @@ pub(crate) fn move_file_with(fs: &dyn MoveFs, from: &str, to: &str) -> std::resu if fs.exists(t) && !is_respelling(fs, f, t) { return Err(fail(format!("{to} already exists"))); } - let mut packets: Vec = sidecars(from) - .into_iter() - .map(|(p, _, _)| p) - .filter(|p| fs.is_file(p)) - .map(|path| RetainedPacket { version: fs.packet_version(&path).ok(), path }) - .collect(); + // An upper-case `.XMP` spelling that only answers through a case-insensitive volume is the + // lower-case sidecar: track and move it under its own name, so `a.xmp` doesn't become `b.XMP`. + let present = |(sc, _, upper): &(PathBuf, SidecarNaming, bool)| fs.is_file(sc) && (!*upper || fs.listed(sc)); + let mut packets: Vec = + sidecars(from).into_iter().filter(present).map(|(path, _, _)| RetainedPacket { version: fs.packet_version(&path).ok(), path }).collect(); move_one(fs, f, t).map_err(|e| fail(format!("rename {from}: {e}")))?; let mut done: Vec = Vec::new(); - let mut remaining = sidecars(from).into_iter().filter(|(sc, _, _)| fs.is_file(sc)).map(|(sc, naming, upper)| { + let mut remaining = sidecars(from).into_iter().filter(present).map(|(sc, naming, upper)| { let mut dst = crate::sidecar::sidecar_path(to, naming); if upper { dst = dst.with_extension("XMP"); @@ -1124,6 +1142,24 @@ pub(crate) mod tests { use super::*; use std::cell::RefCell; + /// Windows has no file id in std: two spellings of one file are the same file whichever is + /// asked about first (a save after a partial rename saw `Trip.XMP` as a collision with the + /// known `Trip.xmp`), and only the spelling on disk is listed. + #[cfg(windows)] + #[test] + fn same_file_spellings_are_symmetric_on_windows() { + let dir = std::env::temp_dir().join(format!("lc-same-file-spelling-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let (real, alias, other) = (dir.join("Trip.xmp"), dir.join("Trip.XMP"), dir.join("other.xmp")); + std::fs::write(&real, b"packet").unwrap(); + std::fs::write(&other, b"packet").unwrap(); + assert!(RealFs.same_file(&real, &alias) && RealFs.same_file(&alias, &real)); + assert!(!RealFs.same_file(&real, &other) && !RealFs.same_file(&other, &alias)); + assert!(RealFs.listed(&real) && !RealFs.listed(&alias)); + std::fs::remove_dir_all(&dir).unwrap(); + } + /// The cross-volume move's check compares files chunk by chunk: equal files across several /// chunks, a difference in the last chunk, different lengths, a missing file. #[test] @@ -1171,7 +1207,10 @@ pub(crate) mod tests { } fn idx(&self, p: &Path) -> Option { let p = Self::norm(p); - self.files.borrow().iter().position(|(f, _)| if self.ci { folded_path(f) == folded_path(&p) } else { *f == p }) + self.files.borrow().iter().position(|(f, _)| { + let f = f.replace('\\', "/"); + if self.ci { folded_path(&f) == folded_path(&p) } else { f == p } + }) } /// The listing: (exact path, contents), sorted. pub fn listing(&self) -> Vec<(String, String)> { @@ -1196,7 +1235,7 @@ pub(crate) mod tests { self.idx(a).is_some() && self.idx(a) == self.idx(b) } fn rename_no_replace(&self, a: &Path, b: &Path) -> std::io::Result<()> { - if self.fail.borrow().iter().any(|f| Self::norm(a).contains(f.as_str())) { + if self.fail.borrow().iter().any(|f| Self::norm(a).contains(&f.replace('\\', "/"))) { return Err(std::io::Error::other("injected failure")); } if self.exists(b) { @@ -1207,7 +1246,7 @@ pub(crate) mod tests { Ok(()) } fn copy_no_replace(&self, a: &Path, b: &Path) -> std::io::Result<()> { - if self.fail.borrow().iter().any(|f| Self::norm(a).contains(f.as_str())) { + if self.fail.borrow().iter().any(|f| Self::norm(a).contains(&f.replace('\\', "/"))) { return Err(std::io::Error::other("injected failure")); } if self.exists(b) { @@ -1223,6 +1262,10 @@ pub(crate) mod tests { self.files.borrow_mut().remove(i); Ok(()) } + fn listed(&self, p: &Path) -> bool { + let p = Self::norm(p); + self.files.borrow().iter().any(|(f, _)| f.replace('\\', "/") == p) + } fn stem_sibling(&self, sidecar: &Path) -> Option { let stem = Self::norm(&sidecar.with_extension("")); self.files.borrow().iter().map(|(f, _)| f.clone()).find(|f| { @@ -1293,7 +1336,7 @@ pub(crate) mod tests { } else { serde_json::json!({"path": from, "into": root.join("Archive")}) }; - let to = if command == "folder.rename" { root.join("Holiday") } else { root.join("Archive/Trip") }; + let to = if command == "folder.rename" { root.join("Holiday") } else { root.join("Archive").join("Trip") }; let before = s.catalog.to_snapshot(); let history = (s.undo.len(), s.redo.len()); assert!(s.execute(command, ¶ms).is_err()); @@ -1421,6 +1464,34 @@ pub(crate) mod tests { assert!(move_file_with(&fs, "/p/A.JPG", "/p/B.JPG").is_err()); } + /// On a case-insensitive volume (Windows, macOS) `a.XMP` answers for `a.xmp`: a renamed + /// photo's sidecars keep the spelling they have on disk, and a failure on the real spelling + /// is the one reported and queued. + #[test] + fn case_insensitive_renames_keep_each_sidecars_spelling() { + let fs = FakeFs::new(true, &[("/p/a.jpg", "A"), ("/p/a.xmp", "stem"), ("/p/a.jpg.XMP", "full"), ("/p/b.jpg", "B"), ("/p/b.Xmp", "mixed")]); + move_file_with(&fs, "/p/a.jpg", "/p/c.jpg").unwrap(); + move_file_with(&fs, "/p/b.jpg", "/p/d.jpg").unwrap(); + assert_eq!( + fs.listing(), + vec![ + ("/p/c.jpg".into(), "A".into()), + ("/p/c.jpg.XMP".into(), "full".into()), + ("/p/c.xmp".into(), "stem".into()), + ("/p/d.jpg".into(), "B".into()), + ("/p/d.xmp".into(), "mixed".into()) + ] + ); + // the move of the sidecar's real spelling is the one attempted (and here fails): the photo + // goes back and nothing changed + let before = fs.listing(); + *fs.fail.borrow_mut() = vec!["/p/c.xmp".into()]; + let e = move_file_with(&fs, "/p/c.jpg", "/p/a.jpg").unwrap_err(); + assert!(e.message.replace('\\', "/").contains("rename /p/c.xmp: injected failure"), "{e}"); + assert!(!e.moved && e.pending_sidecars.is_empty()); + assert_eq!(fs.listing(), before); + } + /// Issue #105: a rollback that fails is reported, and the library follows the files that kept /// their new names (an undoable partial rename), so nothing goes missing. #[test] diff --git a/crates/engine/src/sidecar.rs b/crates/engine/src/sidecar.rs index 4f44780ec..9aaf7e937 100644 --- a/crates/engine/src/sidecar.rs +++ b/crates/engine/src/sidecar.rs @@ -566,7 +566,12 @@ impl Session { .map_err(|e| EngineError::Other(format!("{}: unfinished XMP repair: {e}", preferred.display())))?; // Keep an existing packet's spelling/style so foreign properties survive this save and // subsequent Undo/Redo writes. Shared stems owned by another photo were excluded above. - let path = candidates.into_iter().find(|path| path.is_file()).unwrap_or(preferred); + // On a case-insensitive volume `a.xmp` also answers for `a.XMP`: the spelling the folder + // lists comes first, so the atomic write doesn't rename the packet. + use crate::rename::MoveFs; + let existing: Vec = candidates.into_iter().filter(|path| path.is_file()).collect(); + let listed = existing.iter().find(|path| crate::rename::RealFs.listed(path)).cloned(); + let path = listed.or_else(|| existing.into_iter().next()).unwrap_or(preferred); self.validate_sidecar_packets_for_paths(std::slice::from_ref(&path), &crate::rename::RealFs)?; let p = self.catalog.photo(id).ok_or(lightcraft_catalog::CatalogError::NoPhoto(id))?; let err = |e: std::io::Error| EngineError::Other(format!("{}: {e}", path.display())); diff --git a/crates/engine/src/sidecar/tests_repair_saves.rs b/crates/engine/src/sidecar/tests_repair_saves.rs index 6fd3c2e9f..4b622e657 100644 --- a/crates/engine/src/sidecar/tests_repair_saves.rs +++ b/crates/engine/src/sidecar/tests_repair_saves.rs @@ -1,7 +1,7 @@ //! Real Session/serializer regressions for saves that overlap retained physical repairs. use super::*; use crate::rename::{MoveFs, RealFs}; -use crate::tests_xmp::{case_insensitive, same_path_or_file}; +use crate::tests_xmp::same_path_or_file; use lightcraft_catalog::{Op, Photo, Source}; use serde_json::json; @@ -151,9 +151,10 @@ fn autowrite_repairs_partial_rename_before_merging_foreign_xmp_across_library_sw assert_eq!(parsed.rating, Some(4)); assert_eq!(parsed.title.as_deref(), Some("Final title")); assert!(parsed.keywords.as_ref().unwrap().contains(&"cafĂ©".to_string())); - // (on a case-insensitive volume the two spellings are one file) - if upper && !case_insensitive(&root) { - assert!(!preferred.exists(), "merge uses the repaired uppercase packet"); + if upper { + // by listing: on a case-insensitive volume `preferred` answers for the upper-case packet + let listed = std::fs::read_dir(&root).unwrap().any(|e| Some(e.unwrap().file_name().as_os_str()) == preferred.file_name()); + assert!(!listed, "merge uses the repaired uppercase packet"); } for (step, title) in [("edit.undo", "Fresh title"), ("edit.redo", "Final title")] { s.execute(step, &json!({})).unwrap(); diff --git a/crates/engine/src/tests_filesystem_history.rs b/crates/engine/src/tests_filesystem_history.rs index c071d995b..e4e150c25 100644 --- a/crates/engine/src/tests_filesystem_history.rs +++ b/crates/engine/src/tests_filesystem_history.rs @@ -31,11 +31,21 @@ fn file_history_step(s: &mut Session, fs: &dyn crate::rename::MoveFs, redo: bool }) } +/// The model volume spells paths with `/`; the code under test joins them with the host's separator. +fn slash(path: &str) -> String { + path.replace('\\', "/") +} + +fn file_source(s: &Session, id: u64) -> String { + let Source::File { path } = &s.catalog.photo(PhotoId(id)).unwrap().source else { panic!("synthetic file fixture") }; + slash(path) +} + fn assert_file_catalog_and_journal(s: &Session, fs: &crate::rename::tests::FakeFs, store: &lightcraft_catalog::MemStore) { let files = fs.listing(); for photo in s.catalog.photos() { let Source::File { path } = &photo.source else { panic!("synthetic file fixture") }; - assert!(files.iter().any(|(file, _)| file == path), "photo {} points to missing {path}: {files:?}", photo.id.0); + assert!(files.iter().any(|(file, _)| *file == slash(path)), "photo {} points to missing {path}: {files:?}", photo.id.0); assert_eq!(std::path::Path::new(path).file_name().unwrap().to_string_lossy(), photo.file_name.as_str()); } let replayed = memory_library(store); @@ -102,7 +112,8 @@ fn merged_renames_coalesce_physical_moves_and_keep_retry_inverses_and_journal() let renamed = fs.listing(); s.merge_undo(2, "Rename Twice"); assert_eq!(s.undo.len(), 1); - assert_eq!(s.file_moves(&s.undo.last().unwrap().op), vec![("/p/final.jpg".into(), "/p/a.jpg".into())]); + let moves: Vec<_> = s.file_moves(&s.undo.last().unwrap().op).iter().map(|(from, to)| (slash(from), slash(to))).collect(); + assert_eq!(moves, vec![("/p/final.jpg".to_string(), "/p/a.jpg".to_string())]); // First exercise the ordinary merged step, before any corrective paths exist. for redo in [false, true] { file_history_step(&mut s, &fs, redo).unwrap(); @@ -274,7 +285,7 @@ fn history_retries_both_sidecar_styles_after_following_a_photo_with_failed_rollb file_history_step(&mut s, &fs, redo).unwrap(); let mut after = before .iter() - .map(|(path, bytes)| (path.replace(from.trim_end_matches(".jpg"), to.trim_end_matches(".jpg")), bytes.clone())) + .map(|(path, bytes)| (path.replace(slash(&from).trim_end_matches(".jpg"), slash(&to).trim_end_matches(".jpg")), bytes.clone())) .collect::>(); after.sort(); assert_eq!(fs.listing(), after); @@ -343,7 +354,7 @@ fn initial_partial_rename_keeps_sidecar_repairs_for_noop_retry_and_history() { let plans = s.plan_rename_with(&fs, &[PhotoId(1)], "Trip", 1); *fs.fail.borrow_mut() = vec!["/p/a.xmp".into(), "/p/Trip.jpg".into()]; assert!(s.execute_fn("photo.rename", |s| Ok(json!({"renamed": s.apply_rename_with(&fs, &plans)?}))).is_err()); - assert_eq!(s.catalog.photo(PhotoId(1)).unwrap().source, Source::File { path: "/p/Trip.jpg".into() }); + assert_eq!(file_source(&s, 1), "/p/Trip.jpg"); assert_eq!(s.undo.len(), 1); assert_eq!(s.undo.last().unwrap().retry_sidecars.len(), 2); assert_file_catalog_and_journal(&s, &fs, &store); @@ -513,10 +524,11 @@ fn merged_rename_failure_keeps_rolled_back_photo_identity() { ); s.merge_undo(3, "Rename Three Times"); assert_eq!((s.undo.len(), s.redo.len()), (1, 0)); - assert_eq!(s.file_moves(&s.undo.last().unwrap().op), vec![("/p/d.jpg".into(), "/p/b.jpg".into()), ("/p/c.jpg".into(), "/p/a.jpg".into())]); + let moves: Vec<_> = s.file_moves(&s.undo.last().unwrap().op).iter().map(|(from, to)| (slash(from), slash(to))).collect(); + assert_eq!(moves, vec![("/p/d.jpg".to_string(), "/p/b.jpg".to_string()), ("/p/c.jpg".to_string(), "/p/a.jpg".to_string())]); // The second photo move sticks; the preceding d -> b move must roll back to d. *fs.fail.borrow_mut() = vec!["/p/c.xmp".into(), "/p/a.jpg".into()]; - let error = file_history_step(&mut s, &fs, false).unwrap_err().to_string(); + let error = slash(&file_history_step(&mut s, &fs, false).unwrap_err().to_string()); assert!(error.contains("rename /p/c.xmp: injected failure"), "{error}"); assert!(error.contains("/p/c.jpg could not be moved back"), "{error}"); let files = fs.listing(); @@ -529,9 +541,8 @@ fn merged_rename_failure_keeps_rolled_back_photo_identity() { ] ); assert_eq!((s.undo.len(), s.redo.len()), (1, 0)); - assert_eq!(s.catalog.photo(PhotoId(1)).unwrap().source, Source::File { path: "/p/a.jpg".into() }); - let photo2 = s.catalog.photo(PhotoId(2)).unwrap(); - assert_eq!(photo2.source, Source::File { path: "/p/d.jpg".into() }, "photo 2 rolled back physically and must still identify d.jpg"); - let Source::File { path } = &photo2.source else { panic!("file fixture") }; - assert_eq!(files.iter().find(|(file, _)| file == path).map(|(_, bytes)| bytes.as_str()), Some("Photo 2")); + assert_eq!(file_source(&s, 1), "/p/a.jpg"); + let path = file_source(&s, 2); + assert_eq!(path, "/p/d.jpg", "photo 2 rolled back physically and must still identify d.jpg"); + assert_eq!(files.iter().find(|(file, _)| *file == path).map(|(_, bytes)| bytes.as_str()), Some("Photo 2")); } diff --git a/crates/engine/src/tests_folders.rs b/crates/engine/src/tests_folders.rs index b55e88c8a..697c98087 100644 --- a/crates/engine/src/tests_folders.rs +++ b/crates/engine/src/tests_folders.rs @@ -40,7 +40,8 @@ impl Scratch { Scratch(dir) } fn path(&self, rel: &str) -> String { - self.0.join(rel).to_string_lossy().to_string() + // the host's separator throughout, as the code under test spells joined paths + self.0.join(rel).components().collect::().to_string_lossy().to_string() } }