Merge PR #1052

apps/lightcraft/src/editors.rs

@@ -172,8 +172,10 @@ fn desktop_exec(entry: &str) -> Option<PathBuf> {
         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)]

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<File> {
         self.file.as_ref().and_then(|f| f.try_clone().ok())
     }

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<String>;
+    /// 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<PacketVersion> {
@@ -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<RetainedPacket> = 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<RetainedPacket> =
+        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<SidecarMove> = 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<usize> {
             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<String> {
             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, &params).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]

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<PathBuf> = 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()));

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();

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::<Vec<_>>();
             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"));
 }

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::<std::path::PathBuf>().to_string_lossy().to_string()
     }
 }