From 0fd0ffb783fa7a9fce86ee9028b614ad8766167f Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Mon, 5 Oct 2026 16:23:32 -0400 Subject: [PATCH 1/8] Issue #98: thumbnail cache only lists, counts and deletes its own files scan() now accepts only <2 hex>/<32 hex>.jpg (matching shard) and the cache's own <32 hex>.tmp temp files, so prune/clear never delete and size() never counts foreign files in a shared thumbs/ folder. Co-Authored-By: Claude Opus 5.5 --- crates/preview/src/disk.rs | 42 ++++++++++++++++++++++++++++++++++++- crates/preview/src/tests.rs | 38 +++++++++++++++++++++++++++++++++ docs/whats-new.md | 2 ++ 3 files changed, 81 insertions(+), 1 deletion(-) diff --git a/crates/preview/src/disk.rs b/crates/preview/src/disk.rs index 7dbdda1e..7ec2dcfe 100644 --- a/crates/preview/src/disk.rs +++ b/crates/preview/src/disk.rs @@ -4,6 +4,11 @@ //! files) but not fsynced; unreadable files are deleted and re-rendered. When the total size //! exceeds the budget, the least recently used files (by modification time, refreshed on read) //! are removed down to 80 % of the budget. +//! +//! The cache only ever lists, counts or deletes files of its own name shape +//! (`<2 hex>/<32 hex>.jpg` and the `<32 hex>.tmp…` temp files of [`DiskCache::put`], see +//! [`is_cache_file`]): the directory may be shared with other files (e.g. a library opened on a +//! photo folder that already has a `thumbs/` folder), and those are never touched (issue #98). use std::path::{Path, PathBuf}; use std::sync::Mutex; @@ -96,13 +101,25 @@ impl DiskCache { } } - /// All cache files: (path, size, modified). + /// All cache files (only names of the cache's own shape, see [`is_cache_file`]): + /// (path, size, modified). fn scan(&self) -> Vec<(PathBuf, u64, std::time::SystemTime)> { let mut out = Vec::new(); let Ok(rd) = std::fs::read_dir(&self.dir) else { return out }; for sub in rd.flatten() { + let sub_name = sub.file_name(); + let Some(sub_name) = sub_name.to_str() else { continue }; + if !is_shard_name(sub_name) { + continue; + } let Ok(files) = std::fs::read_dir(sub.path()) else { continue }; for f in files.flatten() { + let name = f.file_name(); + let Some(name) = name.to_str() else { continue }; + if !is_cache_file(sub_name, name) { + continue; + } + // `DirEntry::metadata` does not follow symlinks: a link is never a cache file if let Ok(m) = f.metadata() && m.is_file() { @@ -144,6 +161,29 @@ impl DiskCache { } } +fn is_lower_hex(s: &str) -> bool { + s.bytes().all(|b| b.is_ascii_digit() || (b'a'..=b'f').contains(&b)) +} + +/// A shard directory name: two lowercase hex digits. +fn is_shard_name(s: &str) -> bool { + s.len() == 2 && is_lower_hex(s) +} + +/// Whether `name` inside shard directory `shard` is a file this cache wrote: `<32 hex>.jpg` +/// (whose first two digits are the shard) or a `<32 hex>.tmp` temp file of +/// [`DiskCache::put`]. Anything else in the cache directory belongs to someone else. +pub fn is_cache_file(shard: &str, name: &str) -> bool { + if !is_shard_name(shard) { + return false; + } + let Some((stem, ext)) = name.split_once('.') else { return false }; + if stem.len() != 32 || !is_lower_hex(stem) || !stem.starts_with(shard) { + return false; + } + ext == "jpg" || ext.strip_prefix("tmp").is_some_and(|t| !t.is_empty() && t.bytes().all(|b| b.is_ascii_alphanumeric())) +} + /// Encode a thumbnail as the cache stores it (JPEG, RGB, 4:2:0). pub fn encode_jpeg(img: &Rgba8) -> Option> { if img.width == 0 || img.height == 0 || img.width > u16::MAX as usize || img.height > u16::MAX as usize { diff --git a/crates/preview/src/tests.rs b/crates/preview/src/tests.rs index 038f935a..ee5bd36f 100644 --- a/crates/preview/src/tests.rs +++ b/crates/preview/src/tests.rs @@ -154,3 +154,41 @@ fn pool_runs_inline() { assert_eq!(p.run_inline(10), 1); assert_eq!(p.try_recv().map(|d| d.result), Some(1)); } + +#[test] +fn disk_cache_never_touches_foreign_files() { + // issue #98: a library opened on a folder that already has a `thumbs/` folder + let dir = temp_dir("foreign"); + let foreign = [ + dir.join("readme.txt"), + dir.join("ab").join("holiday.jpg"), + dir.join("ab").join("0123456789abcdef0123456789abcdef.jpg"), // wrong shard + dir.join("small").join("0123456789abcdef0123456789abcdef.jpg"), + dir.join("01").join("0123456789ABCDEF0123456789ABCDEF.jpg"), // upper case + dir.join("01").join("0123456789abcdef0123456789abcdef.jpeg"), + dir.join("01").join("0123456789abcdef0123456789abcdef.tmp"), + ]; + for p in &foreign { + std::fs::create_dir_all(p.parent().unwrap()).unwrap(); + std::fs::write(p, vec![7u8; 50_000]).unwrap(); + } + // a stale temp file of our own is cleaned up + let own_tmp = dir.join("01").join("0123456789abcdef0123456789abcdef.tmpThreadId9"); + std::fs::write(&own_tmp, b"partial").unwrap(); + let d = DiskCache::new(&dir, 40_000); + assert_eq!(d.size(), 7, "only the cache's own temp file counts"); + // fill past the budget (forces prunes), then clear + for i in 0..40u8 { + d.put(hash_bytes(&[i, 2]), &gradient(160, 100, i)); + } + assert!(d.size() <= 40_000); + d.clear(); + assert_eq!(d.size(), 0); + assert!(!own_tmp.exists()); + for p in &foreign { + assert!(p.exists(), "foreign file deleted: {}", p.display()); + } + assert!(disk::is_cache_file("01", "0123456789abcdef0123456789abcdef.jpg")); + assert!(!disk::is_cache_file("01", "0123456789abcdef0123456789abcdef.jpg.bak")); + let _ = std::fs::remove_dir_all(&dir); +} diff --git a/docs/whats-new.md b/docs/whats-new.md index dbbadf90..b606889d 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -19,6 +19,8 @@ in short pieces so slow integrated GPUs aren't reset by their watchdog. `ui.inspect` → `perf` (`gpuReason`, `gpuFallback`), Help ▸ System Info and Settings ▸ Performance say why the GPU isn't used (e.g. a skipped software adapter such as llvmpipe) and why the last render fell back. +- The thumbnail cache only ever counts and deletes its own files (issue #98): a library opened on a folder that + already has a `thumbs/` folder of other pictures no longer loses them when the cache is trimmed or cleared. ### Library - Smart albums with a rule editor: match all / any / none, nested groups, 26 fields. From dfbb1ad935a6ab0af5a3c347cb2c5e6944bd6da9 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Mon, 5 Oct 2026 16:36:35 -0400 Subject: [PATCH 2/8] Issue #95: Rename Photos: case-only renames never overwrite another file Whether an existing target is the photo itself is decided by file identity (dev+inode on Unix; an exact directory entry check elsewhere), not by a lower-case name comparison, so on case-sensitive volumes img_1.JPG next to IMG_1.JPG is a collision (-1 suffix). Moves use a no-replace rename (hard link + unlink where supported) and case changes go through a unique temp name. Tests model case-sensitive and case-insensitive volumes and check the real disk. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/rename.rs | 267 ++++++++++++++++++++++++++++++++---- docs/parity.md | 2 +- docs/whats-new.md | 3 + 3 files changed, 248 insertions(+), 24 deletions(-) diff --git a/crates/engine/src/rename.rs b/crates/engine/src/rename.rs index 4b956c84..266f76e0 100644 --- a/crates/engine/src/rename.rs +++ b/crates/engine/src/rename.rs @@ -11,7 +11,11 @@ //! //! Safety: //! - a target that exists on disk, or that another photo in the batch gets, receives a `-1`, `-2`… -//! suffix — no file is ever overwritten; +//! suffix — no file is ever overwritten. Whether an existing target is the photo itself (a +//! change of letter case on a case-insensitive volume) is decided by file identity, not by +//! comparing names, so on a case-sensitive volume `img_1.jpg` next to `IMG_1.JPG` is a collision +//! (issue #95); moves never replace a file even if one appears after the check (hard link + +//! unlink where the volume supports it); //! - the moves happen before the catalog changes; if one fails, the ones already done are moved //! back and nothing is committed; //! - the catalog op ([`Op::SetFile`]) is undoable: undo/redo move the files back and forth (see @@ -297,47 +301,130 @@ fn sidecars(path: &str) -> Vec<(PathBuf, SidecarNaming, bool)> { v } +/// The file-system primitives renaming uses. [`RealFs`] is the disk; tests substitute a model of +/// a case-insensitive or case-sensitive volume (and inject failures). +pub(crate) trait MoveFs { + /// Something (file, folder, link) answers to `p`. + fn exists(&self, p: &Path) -> bool; + fn is_file(&self, p: &Path) -> bool; + /// `a` and `b` name the same file (e.g. `IMG.JPG` and `img.jpg` on a case-insensitive + /// volume). Decided by file identity, never by comparing names: on a case-sensitive volume + /// they are two different photos. When in doubt: `false`. + fn same_file(&self, a: &Path, b: &Path) -> bool; + /// Rename `a` to `b`, failing (never replacing) when `b` exists. + fn rename_no_replace(&self, a: &Path, b: &Path) -> std::io::Result<()>; +} + +/// The real file system. +pub(crate) struct RealFs; + +impl MoveFs for RealFs { + fn exists(&self, p: &Path) -> bool { + std::fs::symlink_metadata(p).is_ok() + } + + fn is_file(&self, p: &Path) -> bool { + p.is_file() + } + + fn same_file(&self, a: &Path, b: &Path) -> bool { + if a == b { + return true; + } + let (Ok(ma), Ok(mb)) = (std::fs::metadata(a), std::fs::metadata(b)) else { return false }; + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt; + ma.dev() == mb.dev() && ma.ino() == mb.ino() + } + #[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() { + return false; + } + let Ok(entries) = std::fs::read_dir(dir) else { return false }; + !entries.flatten().any(|e| e.file_name() == name) + } + } + + fn rename_no_replace(&self, a: &Path, b: &Path) -> std::io::Result<()> { + if self.exists(b) { + return Err(std::io::Error::new(std::io::ErrorKind::AlreadyExists, format!("{} already exists", b.display()))); + } + // A hard link fails when `b` exists — even if it appeared since the check above — so + // nothing is ever replaced. Volumes without hard links (FAT, some network shares) fall back + // to a plain rename after the check. + match std::fs::hard_link(a, b) { + Ok(()) => std::fs::remove_file(a).inspect_err(|_| { + // both names are the same file: dropping the new one loses nothing + let _ = std::fs::remove_file(b); + }), + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => Err(e), + Err(_) => std::fs::rename(a, b), + } + } +} + +/// `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) +} + +/// Rename `a` to `b` without ever replacing a file; a change of letter case only goes through a +/// unique temporary name so case-insensitive volumes see a real change. +fn move_one(fs: &dyn MoveFs, a: &Path, b: &Path) -> std::io::Result<()> { + if !is_respelling(fs, a, b) { + return fs.rename_no_replace(a, b); + } + let tmp = (0..1000u32) + .map(|k| a.with_file_name(format!(".lc-rename-{}-{k}", std::process::id()))) + .find(|t| !fs.exists(t)) + .ok_or_else(|| std::io::Error::other("no free temporary name"))?; + fs.rename_no_replace(a, &tmp)?; + fs.rename_no_replace(&tmp, b).inspect_err(|_| { + let _ = fs.rename_no_replace(&tmp, a); + }) +} + /// Move `from` to `to` (never over an existing file) together with its sidecars. On error nothing /// is left moved. pub fn move_file(from: &str, to: &str) -> std::result::Result<(), String> { + move_file_with(&RealFs, from, to) +} + +/// [`move_file`] on the file system `fs`. +pub(crate) fn move_file_with(fs: &dyn MoveFs, from: &str, to: &str) -> std::result::Result<(), String> { if from == to { return Ok(()); } let (f, t) = (Path::new(from), Path::new(to)); - // a case-only change on a case-insensitive file system "exists" already: that is this file - let case_only = from.to_lowercase() == to.to_lowercase(); - if t.exists() && !case_only { + // `to` may be this very file spelled differently (case-insensitive volume); any other file + // there — e.g. `img.jpg` next to `IMG.JPG` on a case-sensitive volume — is never replaced + if fs.exists(t) && !is_respelling(fs, f, t) { return Err(format!("{to} already exists")); } let mut done: Vec<(PathBuf, PathBuf)> = Vec::new(); - let mv = |a: &Path, b: &Path| -> std::io::Result<()> { - if case_only { - // via a temporary name, so case-insensitive file systems see a real change - let tmp = a.with_file_name(format!(".lc-rename-{}", std::process::id())); - std::fs::rename(a, &tmp)?; - std::fs::rename(&tmp, b).inspect_err(|_| { - let _ = std::fs::rename(&tmp, a); - }) - } else { - std::fs::rename(a, b) - } - }; - mv(f, t).map_err(|e| format!("rename {from}: {e}"))?; + move_one(fs, f, t).map_err(|e| format!("rename {from}: {e}"))?; done.push((f.to_path_buf(), t.to_path_buf())); for (sc, naming, upper) in sidecars(from) { - if !sc.is_file() { + if !fs.is_file(&sc) { continue; } let mut dst = crate::sidecar::sidecar_path(to, naming); if upper { dst = dst.with_extension("XMP"); } - if dst.exists() && !case_only { + if fs.exists(&dst) && !is_respelling(fs, &sc, &dst) { continue; // never overwrite: the old sidecar stays where it was } - if let Err(e) = mv(&sc, &dst) { + if let Err(e) = move_one(fs, &sc, &dst) { for (a, b) in done.iter().rev() { - let _ = std::fs::rename(b, a); + let _ = move_one(fs, b, a); } return Err(format!("rename {}: {e}", sc.display())); } @@ -349,6 +436,11 @@ pub fn move_file(from: &str, to: &str) -> std::result::Result<(), String> { impl Session { /// Plan renaming `ids` with `template` (sequence numbers from `start`), resolving collisions. pub fn plan_rename(&self, ids: &[PhotoId], template: &str, start: usize) -> Vec { + self.plan_rename_with(&RealFs, ids, template, start) + } + + /// [`Session::plan_rename`] on the file system `fs`. + pub(crate) fn plan_rename_with(&self, fs: &dyn MoveFs, ids: &[PhotoId], template: &str, start: usize) -> Vec { // photos sharing one file (virtual copies) rename once let mut seen_paths: HashSet = HashSet::new(); let mut taken: HashSet = HashSet::new(); // lower-case target paths/names claimed in this batch @@ -382,10 +474,12 @@ impl Session { let name = candidate(k); let tp = dir.join(&name).to_string_lossy().to_string(); let key = tp.to_lowercase(); - let same = key == path.to_lowercase(); + // this very file, maybe spelled differently (case-insensitive volume)? By + // identity: on a case-sensitive volume `img_1.jpg` may be another photo + let same = tp == *path || is_respelling(fs, Path::new(path), Path::new(&tp)); // 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. - let on_disk = Path::new(&tp).exists() && !same; + let on_disk = fs.exists(Path::new(&tp)) && !same; if !taken.contains(&key) && !on_disk { taken.insert(key); break (name, Some(tp)); @@ -499,6 +593,133 @@ impl Session { #[cfg(test)] mod tests { use super::*; + use std::cell::RefCell; + + /// A model volume: files by path with contents; case-insensitive (default APFS/NTFS) or + /// case-sensitive (Linux, case-sensitive APFS). `fail` makes renames from a matching source fail. + pub(crate) struct FakeFs { + pub ci: bool, + pub files: RefCell)>>, + pub fail: RefCell>, + } + + impl FakeFs { + pub fn new(ci: bool, files: &[(&str, &str)]) -> FakeFs { + FakeFs { + ci, + files: RefCell::new(files.iter().map(|(p, c)| (p.to_string(), c.as_bytes().to_vec())).collect()), + fail: RefCell::new(Vec::new()), + } + } + 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 }) + } + /// 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(); + v.sort(); + v + } + } + + impl MoveFs for FakeFs { + fn exists(&self, p: &Path) -> bool { + self.idx(p).is_some() + } + fn is_file(&self, p: &Path) -> bool { + self.exists(p) + } + fn same_file(&self, a: &Path, b: &Path) -> bool { + 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| a.to_string_lossy().contains(f.as_str())) { + return Err(std::io::Error::other("injected failure")); + } + if self.exists(b) { + return Err(std::io::Error::new(std::io::ErrorKind::AlreadyExists, "exists")); + } + let i = self.idx(a).ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, "no such file"))?; + self.files.borrow_mut()[i].0 = b.to_string_lossy().to_string(); + Ok(()) + } + } + + fn file_photo(id: u64, path: &str) -> Photo { + let name = Path::new(path).file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_default(); + Photo::new(PhotoId(id), Source::File { path: path.into() }, &name, "JPG", 1, 1, "2026-01-01T00:00:00") + } + + /// 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] + fn case_only_renames_never_replace_another_file() { + // case-sensitive: IMG_1.JPG and img_1.JPG are two photos + let fs = FakeFs::new(false, &[("/p/IMG_1.JPG", "first"), ("/p/img_1.JPG", "second"), ("/p/IMG_1.xmp", "sc1"), ("/p/img_1.xmp", "sc2")]); + assert!(move_file_with(&fs, "/p/IMG_1.JPG", "/p/img_1.JPG").unwrap_err().contains("already exists")); + assert_eq!(fs.listing().len(), 4); + assert!(fs.listing().contains(&("/p/img_1.JPG".into(), "second".into()))); + // the planner picks a free name instead + let mut s = Session::new(); + s.catalog.apply(Op::AddPhoto { photo: Box::new(file_photo(1, "/p/IMG_1.JPG")) }).unwrap(); + let plans = s.plan_rename_with(&fs, &[PhotoId(1)], "img_1", 1); + assert_eq!(plans[0].to, "img_1-1.JPG"); + move_file_with(&fs, "/p/IMG_1.JPG", plans[0].to_path.as_deref().unwrap()).unwrap(); + assert_eq!( + fs.listing(), + vec![ + ("/p/img_1-1.JPG".into(), "first".into()), + ("/p/img_1-1.xmp".into(), "sc1".into()), + ("/p/img_1.JPG".into(), "second".into()), + ("/p/img_1.xmp".into(), "sc2".into()) + ] + ); + // case-sensitive, target free: a plain rename + let fs = FakeFs::new(false, &[("/p/IMG_2.JPG", "x")]); + move_file_with(&fs, "/p/IMG_2.JPG", "/p/img_2.JPG").unwrap(); + assert_eq!(fs.listing(), vec![("/p/img_2.JPG".into(), "x".into())]); + + // case-insensitive: the "existing" target is the photo itself → renamed via a temp name + let fs = FakeFs::new(true, &[("/p/IMG_1.JPG", "first"), ("/p/IMG_1.xmp", "sc1")]); + let mut s = Session::new(); + s.catalog.apply(Op::AddPhoto { photo: Box::new(file_photo(1, "/p/IMG_1.JPG")) }).unwrap(); + let plans = s.plan_rename_with(&fs, &[PhotoId(1)], "img_1", 1); + assert_eq!(plans[0].to, "img_1.JPG", "not a collision with itself"); + move_file_with(&fs, "/p/IMG_1.JPG", "/p/img_1.JPG").unwrap(); + assert_eq!(fs.listing(), vec![("/p/img_1.JPG".into(), "first".into()), ("/p/img_1.xmp".into(), "sc1".into())]); + // case-insensitive, another file under another name: still a collision + let fs = FakeFs::new(true, &[("/p/A.JPG", "a"), ("/p/b.JPG", "b")]); + assert!(move_file_with(&fs, "/p/A.JPG", "/p/B.JPG").is_err()); + } + + /// Issue #95 on the real disk, whichever kind of volume the temp folder is on. + #[test] + fn case_only_rename_on_disk() { + let dir = std::env::temp_dir().join(format!("lc-rename-case-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let (upper, lower) = (dir.join("IMG_1.JPG"), dir.join("img_1.JPG")); + std::fs::write(&upper, b"first").unwrap(); + let case_sensitive = !lower.exists(); + let p = |q: &Path| q.to_string_lossy().to_string(); + if case_sensitive { + std::fs::write(&lower, b"second").unwrap(); + assert!(move_file(&p(&upper), &p(&lower)).is_err()); + assert_eq!(std::fs::read(&lower).unwrap(), b"second"); + assert_eq!(std::fs::read(&upper).unwrap(), b"first"); + } else { + move_file(&p(&upper), &p(&lower)).unwrap(); + let names: Vec = std::fs::read_dir(&dir).unwrap().flatten().map(|e| e.file_name().to_string_lossy().to_string()).collect(); + assert_eq!(names, vec!["img_1.JPG".to_string()]); + assert_eq!(std::fs::read(&lower).unwrap(), b"first"); + } + // a plain rename never replaces + std::fs::write(dir.join("other.JPG"), b"other").unwrap(); + assert!(move_file(&p(&dir.join("other.JPG")), &p(&lower)).is_err()); + assert_eq!(std::fs::read(dir.join("other.JPG")).unwrap(), b"other"); + let _ = std::fs::remove_dir_all(&dir); + } #[test] fn folder_templates_stay_inside_the_destination() { diff --git a/docs/parity.md b/docs/parity.md index b8f0e30c..ced24174 100644 --- a/docs/parity.md +++ b/docs/parity.md @@ -153,7 +153,7 @@ under the UI command that wraps it (a no-op error); the UI command now wins. | LR-LIB-DELETE | Delete / Recently Deleted | P0 | ✅ | `cmd:photo.delete`, `cmd:photo.restore`, `cmd:photo.deletePermanently` | no confirmation dialog, no auto-purge after N days, no "Empty" | | LR-LIB-REMOVE-ALBUM | Remove from album | P0 | ✅ | `cmd:album.removePhotos` | | | LR-LIB-DUPLICATE | Duplicate a photo | P2 | ✅ | `cmd:photo.duplicate` | Photo ▸ Duplicate: a real `-copy` file with the same settings, metadata and albums (virtual copies share the file) | -| LR-LIB-RENAME | Batch rename | P1 | ✅ | `cmd:photo.rename`, `cmd:photo.renamePreview`, `cmd:photo.renameTokens`, `cmd:dialog.rename`, `crates/engine/src/rename.rs` (`TOKENS`) | template tokens {name} {num} {seq:N} {date:%Y%m%d} {folder} {camera} {lens} {iso} {rating} {title} {creator} {ext} (shared with export naming and import rename); a Tags picker beside every template field (Rename Photos, import Rename, export File name) lists each tag with meaning + live example, the date directives and the rules, and inserts the clicked tag at the text cursor; unknown tags are flagged; preview; renames files on disk with their XMP sidecars, never overwriting (-1, -2… suffixes), rolls back on failure; undo/redo move the files; virtual copies follow | +| LR-LIB-RENAME | Batch rename | P1 | ✅ | `cmd:photo.rename`, `cmd:photo.renamePreview`, `cmd:photo.renameTokens`, `cmd:dialog.rename`, `crates/engine/src/rename.rs` (`TOKENS`) | template tokens {name} {num} {seq:N} {date:%Y%m%d} {folder} {camera} {lens} {iso} {rating} {title} {creator} {ext} (shared with export naming and import rename); a Tags picker beside every template field (Rename Photos, import Rename, export File name) lists each tag with meaning + live example, the date directives and the rules, and inserts the clicked tag at the text cursor; unknown tags are flagged; preview; renames files on disk with their XMP sidecars, never overwriting (-1, -2… suffixes; a change of letter case only is checked by file identity, so on case-sensitive volumes `img_1.jpg` next to `IMG_1.JPG` is a collision), rolls back on failure; undo/redo move the files; virtual copies follow | | LR-LIB-CAPTURETIME | Edit capture time | P1 | ✅ | `cmd:photo.setCaptureTime`, `cmd:dialog.captureTime`, `crates/catalog/src/dates.rs` | set (the other selected photos shift by the same amount, or `each`), shift by days/hours/minutes, time-zone shift; one undo step, journaled; Info panel button; no “revert to original capture time” | | LR-LIB-SHOWFINDER | Reveal original in file manager | P0 | ✅ | `cmd:app.showInFinder` | ⌘R (see MENU-FILE-SHOWFINDER) | | LR-LIB-COVER | Album cover | P2 | ✅ | `cmd:album.setCover` | | diff --git a/docs/whats-new.md b/docs/whats-new.md index dbbadf90..a976c8ef 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -21,6 +21,9 @@ used (e.g. a skipped software adapter such as llvmpipe) and why the last render fell back. ### Library +- Rename Photos never overwrites another photo when only the letter case changes (issue #95): on case-sensitive + volumes (Linux, case-sensitive APFS) `img_1.JPG` next to `IMG_1.JPG` is a different photo and the renamed one gets + `img_1-1.JPG`; on case-insensitive volumes the case change still goes through. - Smart albums with a rule editor: match all / any / none, nested groups, 26 fields. - Quick Collection and target album (B in the grid), keyword sets (⌥1–⌥9), colour-label sets. - Colour-label filter with several labels at once; expandable folder tree in Local. From fb5b18843fe5ff897cdf0024d305bd7b0ddbf809 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Mon, 5 Oct 2026 16:47:50 -0400 Subject: [PATCH 3/8] Issue #105: Rename Photos reports failed rollbacks; Find Missing matches by content Rollbacks in move_file, apply_rename and undo/redo no longer ignore errors: every move that can't be undone is reported (old -> new), and the catalog follows the files that kept their new names (an undoable partial rename; a logged change for undo/redo), so nothing shows as missing. A file that can't go back keeps the sidecars that moved with it. library.findMissing: same name + size confirmed by the stored content hash first, then renamed files by size + content hash, then a unique name + size match; several look-alikes are skipped and reported in 'ambiguous' instead of guessed. Files already used by a photo are skipped. Stacked on fix/issue-95-case-only-rename (same functions). Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/missing.rs | 103 +++++++++--- crates/engine/src/lib.rs | 13 +- crates/engine/src/rename.rs | 250 ++++++++++++++++++++++++------ crates/engine/src/tests_import.rs | 39 +++++ crates/ui-egui/src/menus.rs | 7 +- docs/parity.md | 4 +- docs/whats-new.md | 4 + 7 files changed, 349 insertions(+), 71 deletions(-) diff --git a/crates/engine/src/cmd/missing.rs b/crates/engine/src/cmd/missing.rs index 05bea65f..bf35f22e 100644 --- a/crates/engine/src/cmd/missing.rs +++ b/crates/engine/src/cmd/missing.rs @@ -1,8 +1,9 @@ //! Missing originals: photos whose file is no longer where the library expects it (moved, //! renamed, on an unplugged drive), and relinking them — one at a time (`photo.relink`) or by -//! searching a folder for files with the same name and size (`library.findMissing`). +//! searching a folder for files with the same name and size, or the same content +//! (`library.findMissing`). -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::path::Path; use lightcraft_catalog::{Catalog, Op, Photo, PhotoId, Source}; @@ -83,8 +84,29 @@ fn relink(s: &mut Session, p: &Value) -> Result { Ok(json!({"id": id.0, "path": abs})) } -/// Search `folder` (recursively) for each missing photo's file name — and its size when the -/// library knows it — and relink every match in one undoable step. +/// The stored content hash of a photo, without the `:dupN` suffix duplicates get. +fn stored_hash(p: &Photo) -> Option<&str> { + p.content_hash.as_deref().map(|h| h.split(':').next().unwrap_or(h)).filter(|h| !h.is_empty()) +} + +/// The content hash of the file at `path` (as import computes it), if it can be read. +fn file_hash(path: &str) -> Option { + std::fs::read(path).ok().map(|b| lightcraft_preview::hash_bytes(&b).to_string()) +} + +/// Search `folder` (recursively) for each missing photo's file and relink every match in one +/// undoable step. +/// +/// Matching, per photo: +/// 1. a file with the same name (any letter case), the same size and the same content hash; +/// 2. renamed: a file of the same size with the same content hash (only same-size files are +/// read, so this stays cheap); +/// 3. without a hash to confirm (or the file was edited in place): the one file with the same +/// name and size. Several such files are ambiguous: the photo is skipped and reported in +/// `ambiguous`, never guessed (two cameras' `IMG_0001.CR3` of one size are different photos). +/// +/// Files that are already some photo's original, or that another photo was just matched with, +/// are not used. fn find_missing(s: &mut Session, p: &Value) -> Result { const C: &str = "library.findMissing"; let folder = str_param(p, "folder").ok_or_else(|| bad(C, "missing `folder`"))?; @@ -93,27 +115,72 @@ fn find_missing(s: &mut Session, p: &Value) -> Result { } let lost = missing(s); if lost.is_empty() { - return Ok(json!({"found": [], "missing": 0})); + return Ok(json!({"found": [], "missing": 0, "ambiguous": []})); } - // file name (lower case) → candidate paths + let in_use: HashSet = s.catalog.photos().filter_map(|p| checked_path(p).map(str::to_string)).collect(); + // file name (lower case) → candidate paths; size → paths (filled lazily) let mut by_name: HashMap> = HashMap::new(); - for f in crate::import::expand(&[folder.to_string()], None) { - if let Some(n) = Path::new(&f).file_name() { - by_name.entry(n.to_string_lossy().to_lowercase()).or_default().push(f); + let files: Vec = crate::import::expand(&[folder.to_string()], None).into_iter().filter(|f| !in_use.contains(f)).collect(); + for f in &files { + if let Some(n) = Path::new(f).file_name() { + by_name.entry(n.to_string_lossy().to_lowercase()).or_default().push(f.clone()); } } + let mut by_size: Option>> = None; + let mut hashes: HashMap> = HashMap::new(); + let mut hash_of = |f: &str| hashes.entry(f.to_string()).or_insert_with(|| file_hash(f)).clone(); + let size_of = |f: &str| std::fs::metadata(f).map(|m| m.len()).ok(); + let mut used: HashSet = HashSet::new(); let mut ops = Vec::new(); let mut found = Vec::new(); + let mut ambiguous = Vec::new(); for (id, old) in &lost { + let Some(ph) = s.catalog.photo(*id) else { continue }; let name = Path::new(old).file_name().map(|n| n.to_string_lossy().to_lowercase()).unwrap_or_default(); - let size = s.catalog.photo(*id).map(|p| p.file_size).unwrap_or(0); - let hit = by_name.get(&name).and_then(|c| { - c.iter().find(|f| size == 0 || std::fs::metadata(f).is_ok_and(|m| m.len() == size)).or_else(|| (c.len() == 1 && size == 0).then(|| &c[0])) - }); - if let Some(path) = hit { - ops.push(relink_op(*id, path)); - found.push(json!({"id": id.0, "from": old, "to": path})); + let size = ph.file_size; + let hash = stored_hash(ph).map(str::to_string); + let named: Vec<&String> = by_name + .get(&name) + .map(|c| c.iter().filter(|f| !used.contains(*f) && (size == 0 || size_of(f) == Some(size))).collect()) + .unwrap_or_default(); + // 1. same name and size, confirmed by the content hash + let mut hit: Option<(String, &str)> = + hash.as_deref().and_then(|h| named.iter().find(|f| hash_of(f).as_deref() == Some(h))).map(|f| (f.to_string(), "name")); + // 2. renamed: same size and content + if hit.is_none() + && let Some(h) = &hash + && size > 0 + { + let sizes = by_size.get_or_insert_with(|| { + let mut m: HashMap> = HashMap::new(); + for f in &files { + if let Some(n) = size_of(f) { + m.entry(n).or_default().push(f.clone()); + } + } + m + }); + hit = sizes + .get(&size) + .and_then(|c| c.iter().find(|f| !used.contains(*f) && hash_of(f).as_deref() == Some(h.as_str()))) + .map(|f| (f.clone(), "content")); } + // 3. same name and size only (no hash, or the file was edited in place): one candidate + // is taken, several are ambiguous and skipped rather than guessed + if hit.is_none() { + match named.len() { + 1 => hit = Some((named[0].clone(), "name")), + 0 => {} + _ => { + ambiguous.push(json!({"id": id.0, "from": old, "candidates": named})); + continue; + } + } + } + let Some((path, by)) = hit else { continue }; + ops.push(relink_op(*id, &path)); + found.push(json!({"id": id.0, "from": old, "to": path, "by": by})); + used.insert(path); } let still = lost.len() - found.len(); if !ops.is_empty() { @@ -124,7 +191,7 @@ fn find_missing(s: &mut Session, p: &Value) -> Result { } } } - Ok(json!({"found": found, "missing": still})) + Ok(json!({"found": found, "missing": still, "ambiguous": ambiguous})) } pub fn specs() -> Vec { @@ -146,7 +213,7 @@ pub fn specs() -> Vec { "Find Missing Photos", [], None, - "{folder} — relink every missing photo whose file (same name and size) is somewhere in the folder → {found: [{id, from, to}], missing}", + "{folder} — relink every missing photo whose file is somewhere in the folder: same name and size (and content, when the library knows its hash), or — renamed — same size and content → {found: [{id, from, to, by: name|content}], missing, ambiguous: [{id, from, candidates}] (several same-name candidates and no hash to tell them apart: skipped)}", always, find_missing ), diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index dcbfd9ed..8f128bc9 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -378,15 +378,22 @@ impl Session { Ok(e.label) } - /// Apply an undo/redo op, first moving the files its renames imply (all or nothing). + /// Apply an undo/redo op, first moving the files its renames imply (all or nothing). Files + /// that can't be moved back after a failure are reported and the library follows them (a + /// logged change outside the undo history), so none goes missing. fn apply_with_files(&mut self, op: &Op) -> Result { + let fs = rename::RealFs; let moves = self.file_moves(op); - Session::move_files(&moves)?; + if let Err(e) = rename::move_all(&fs, &moves) { + return Err(EngineError::Other(format!("can't move the files back: {}", self.follow_stuck(op, e, false)))); + } match self.catalog.apply(op.clone()) { Ok(inv) => Ok(inv), Err(e) => { let back: Vec<(String, String)> = moves.iter().rev().map(|(a, b)| (b.clone(), a.clone())).collect(); - let _ = Session::move_files(&back); + if let Err(be) = rename::move_all(&fs, &back) { + return Err(EngineError::Other(format!("{e}; the files could not all be moved back: {}", be.message))); + } Err(e.into()) } } diff --git a/crates/engine/src/rename.rs b/crates/engine/src/rename.rs index 266f76e0..6e363ed3 100644 --- a/crates/engine/src/rename.rs +++ b/crates/engine/src/rename.rs @@ -386,31 +386,48 @@ fn move_one(fs: &dyn MoveFs, a: &Path, b: &Path) -> std::io::Result<()> { .find(|t| !fs.exists(t)) .ok_or_else(|| std::io::Error::other("no free temporary name"))?; fs.rename_no_replace(a, &tmp)?; - fs.rename_no_replace(&tmp, b).inspect_err(|_| { - let _ = fs.rename_no_replace(&tmp, a); + fs.rename_no_replace(&tmp, b).map_err(|e| match fs.rename_no_replace(&tmp, a) { + Ok(()) => e, + Err(e2) => std::io::Error::other(format!("{e}; it could not be given its old name back either ({e2}): it is now {}", tmp.display())), }) } -/// Move `from` to `to` (never over an existing file) together with its sidecars. On error nothing -/// is left moved. -pub fn move_file(from: &str, to: &str) -> std::result::Result<(), String> { +/// Why [`move_file`] failed, and what it left behind. +#[derive(Clone, Debug, PartialEq)] +pub struct MoveError { + /// What went wrong, including every step that could not be undone and where that file is now. + pub message: String, + /// The file itself ended up at the new path: it moved, a sidecar didn't, and moving the file + /// back failed too. Its sidecars that did move stay with it. + pub moved: bool, +} + +impl std::fmt::Display for MoveError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(&self.message) + } +} + +/// Move `from` to `to` (never over an existing file) together with its sidecars. On error the +/// moves are undone; whatever can't be undone is named in the error (see [`MoveError`]). +pub fn move_file(from: &str, to: &str) -> std::result::Result<(), MoveError> { move_file_with(&RealFs, from, to) } /// [`move_file`] on the file system `fs`. -pub(crate) fn move_file_with(fs: &dyn MoveFs, from: &str, to: &str) -> std::result::Result<(), String> { +pub(crate) fn move_file_with(fs: &dyn MoveFs, from: &str, to: &str) -> std::result::Result<(), MoveError> { if from == to { return Ok(()); } + let fail = |message: String| MoveError { message, moved: false }; let (f, t) = (Path::new(from), Path::new(to)); // `to` may be this very file spelled differently (case-insensitive volume); any other file // there — e.g. `img.jpg` next to `IMG.JPG` on a case-sensitive volume — is never replaced if fs.exists(t) && !is_respelling(fs, f, t) { - return Err(format!("{to} already exists")); + return Err(fail(format!("{to} already exists"))); } - let mut done: Vec<(PathBuf, PathBuf)> = Vec::new(); - move_one(fs, f, t).map_err(|e| format!("rename {from}: {e}"))?; - done.push((f.to_path_buf(), t.to_path_buf())); + move_one(fs, f, t).map_err(|e| fail(format!("rename {from}: {e}")))?; + let mut done: Vec<(PathBuf, PathBuf)> = Vec::new(); // sidecars moved for (sc, naming, upper) in sidecars(from) { if !fs.is_file(&sc) { continue; @@ -423,16 +440,73 @@ pub(crate) fn move_file_with(fs: &dyn MoveFs, from: &str, to: &str) -> std::resu continue; // never overwrite: the old sidecar stays where it was } if let Err(e) = move_one(fs, &sc, &dst) { - for (a, b) in done.iter().rev() { - let _ = move_one(fs, b, a); + let mut message = format!("rename {}: {e}", sc.display()); + // the file first: if it can't go back, the sidecars that moved stay with it + if let Err(e) = move_one(fs, t, f) { + message.push_str(&format!("; {from} could not be moved back ({e}): it is now {to}")); + if !done.is_empty() { + let names: Vec = done.iter().map(|(_, b)| b.display().to_string()).collect(); + message.push_str(&format!(" with its sidecar {}", names.join(", "))); + } + return Err(MoveError { message, moved: true }); } - return Err(format!("rename {}: {e}", sc.display())); + for (a, b) in done.iter().rev() { + if let Err(e) = move_one(fs, b, a) { + message.push_str(&format!("; sidecar {} could not be moved back ({e}): it is now {}", a.display(), b.display())); + } + } + return Err(fail(message)); } done.push((sc, dst)); } Ok(()) } +/// What a failed batch of moves ([`move_all`]) left behind. +#[derive(Debug)] +pub(crate) struct BatchMoveError { + /// The first failure and every move that could not be undone. + pub message: String, + /// Moves that stayed done, (from, to): those files are at `to` now. + pub stuck: Vec<(String, String)>, +} + +/// Move the files `moves` names, all or nothing: after a failure the earlier moves are undone. +/// Moves that can't be undone (e.g. the network share went away) are reported, not ignored. +pub(crate) fn move_all(fs: &dyn MoveFs, moves: &[(String, String)]) -> std::result::Result<(), BatchMoveError> { + let mut done: Vec<&(String, String)> = Vec::new(); + for m in moves { + let Err(e) = move_file_with(fs, &m.0, &m.1) else { + done.push(m); + continue; + }; + let mut message = e.message; + let mut stuck = Vec::new(); + if e.moved { + stuck.push(m.clone()); + } + for d in done.iter().rev() { + if let Err(e) = move_file_with(fs, &d.1, &d.0) { + message.push_str(&format!("; {} could not be moved back to {}: {}", d.1, d.0, e.message)); + if !e.moved { + stuck.push((*d).clone()); + } + } + } + return Err(BatchMoveError { message, stuck }); + } + Ok(()) +} + +/// The `SetFile` ops in `op` (nested batches included) that point a photo at one of `to`. +fn set_file_ops_to(op: &Op, to: &HashSet<&str>, out: &mut Vec) { + match op { + Op::Batch { ops } => ops.iter().for_each(|o| set_file_ops_to(o, to, out)), + Op::SetFile { source: Source::File { path }, .. } if to.contains(path.as_str()) => out.push(op.clone()), + _ => {} + } +} + impl Session { /// Plan renaming `ids` with `template` (sequence numbers from `start`), resolving collisions. pub fn plan_rename(&self, ids: &[PhotoId], template: &str, start: usize) -> Vec { @@ -509,21 +583,16 @@ impl Session { /// Carry out `plans`: move the files (rolled back on failure), then commit one undoable op /// that updates the photos (and their virtual copies). + /// + /// When a failure can't be fully rolled back, the files that kept their new names are + /// committed as a smaller undoable rename, so the library points at them (nothing goes + /// missing), and the error lists them (old → new). pub fn apply_rename(&mut self, plans: &[RenamePlan]) -> Result { - let mut moved: Vec<(String, String)> = Vec::new(); - for pl in plans { - if let (Some(a), Some(b)) = (&pl.from_path, &pl.to_path) - && a != b - { - if let Err(e) = move_file(a, b) { - for (a, b) in moved.iter().rev() { - let _ = move_file(b, a); - } - return Err(EngineError::Other(e)); - } - moved.push((a.clone(), b.clone())); - } - } + self.apply_rename_with(&RealFs, plans) + } + + /// [`Session::apply_rename`] on the file system `fs`. + pub(crate) fn apply_rename_with(&mut self, fs: &dyn MoveFs, plans: &[RenamePlan]) -> Result { let mut ops = Vec::new(); let by_path: HashMap<&str, &RenamePlan> = plans.iter().filter_map(|p| p.from_path.as_deref().map(|f| (f, p))).collect(); for p in self.catalog.photos() { @@ -541,17 +610,66 @@ impl Session { } } let n = ops.len(); + let op = Op::Batch { ops }; + 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())), + _ => None, + }) + .collect(); + if let Err(e) = move_all(fs, &moves) { + return Err(EngineError::Other(self.follow_stuck(&op, e, true))); + } if n > 0 - && let Err(e) = self.commit(&format!("Rename {n} Photo{}", if n == 1 { "" } else { "s" }), Op::Batch { ops }) + && let Err(err) = self.commit(&format!("Rename {n} Photo{}", if n == 1 { "" } else { "s" }), op) { - for (a, b) in moved.iter().rev() { - let _ = move_file(b, a); + let back: Vec<(String, String)> = moves.iter().rev().map(|(a, b)| (b.clone(), a.clone())).collect(); + if let Err(e) = move_all(fs, &back) { + return Err(EngineError::Other(format!("{err}; the renamed files could not all be moved back: {}", e.message))); } - return Err(e); + return Err(err); } Ok(n) } + /// After a failed batch of moves for `op`: point the photos whose files stayed moved at + /// their new paths (an undoable "partly" rename when `undoable`, otherwise a plain logged + /// change), so the library matches the disk. Returns the message for the user. + pub(crate) fn follow_stuck(&mut self, op: &Op, e: BatchMoveError, undoable: bool) -> String { + let mut msg = e.message; + if e.stuck.is_empty() { + return msg; + } + let to: HashSet<&str> = e.stuck.iter().map(|m| m.1.as_str()).collect(); + let mut keep = Vec::new(); + set_file_ops_to(op, &to, &mut keep); + let list: Vec = e.stuck.iter().map(|(a, b)| format!("{a} → {b}")).collect(); + let n = e.stuck.len(); + let s = if n == 1 { "" } else { "s" }; + let fix = Op::Batch { ops: keep }; + let applied = if undoable { + self.commit(&format!("Rename {n} Photo{s} (partly)"), fix) + } else { + match self.catalog.apply(fix.clone()) { + Ok(_) => { + self.pending_log.push(fix); + Ok(()) + } + Err(e) => Err(e.into()), + } + }; + match applied { + Ok(()) => msg + .push_str(&format!(". {n} file{s} could not be moved back and kept the new name; the library now points there: {}", list.join(", "))), + Err(err) => msg.push_str(&format!( + ". {n} file{s} could not be moved back and the library could not follow ({err}); relink with Find Missing Photos: {}", + list.join(", ") + )), + } + msg + } + /// File moves an undo/redo op implies: `SetFile` ops whose source path differs from the photo's /// current one. pub(crate) fn file_moves(&self, op: &Op) -> Vec<(String, String)> { @@ -573,21 +691,6 @@ impl Session { rec(self, op, &mut v); v } - - /// Move the files `moves` names; all or nothing. - pub(crate) fn move_files(moves: &[(String, String)]) -> Result<()> { - let mut done: Vec<&(String, String)> = Vec::new(); - for m in moves { - if let Err(e) = move_file(&m.0, &m.1) { - for (a, b) in done.iter().rev().map(|m| (&m.0, &m.1)) { - let _ = move_file(b, a); - } - return Err(EngineError::Other(format!("can't undo the rename: {e}"))); - } - done.push(m); - } - Ok(()) - } } #[cfg(test)] @@ -657,7 +760,7 @@ mod tests { fn case_only_renames_never_replace_another_file() { // case-sensitive: IMG_1.JPG and img_1.JPG are two photos let fs = FakeFs::new(false, &[("/p/IMG_1.JPG", "first"), ("/p/img_1.JPG", "second"), ("/p/IMG_1.xmp", "sc1"), ("/p/img_1.xmp", "sc2")]); - assert!(move_file_with(&fs, "/p/IMG_1.JPG", "/p/img_1.JPG").unwrap_err().contains("already exists")); + assert!(move_file_with(&fs, "/p/IMG_1.JPG", "/p/img_1.JPG").unwrap_err().message.contains("already exists")); assert_eq!(fs.listing().len(), 4); assert!(fs.listing().contains(&("/p/img_1.JPG".into(), "second".into()))); // the planner picks a free name instead @@ -693,6 +796,59 @@ mod tests { assert!(move_file_with(&fs, "/p/A.JPG", "/p/B.JPG").is_err()); } + /// 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] + fn failed_rollbacks_are_reported_and_followed() { + let fs = FakeFs::new(false, &[("/p/a.jpg", "A"), ("/p/a.xmp", "As"), ("/p/b.jpg", "B"), ("/p/c.jpg", "C")]); + let mut s = Session::new(); + for (i, f) in ["/p/a.jpg", "/p/b.jpg", "/p/c.jpg"].iter().enumerate() { + s.catalog.apply(Op::AddPhoto { photo: Box::new(file_photo(i as u64 + 1, f)) }).unwrap(); + } + let ids = [PhotoId(1), PhotoId(2), PhotoId(3)]; + 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(); + 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}"); + assert_eq!( + fs.listing(), + vec![ + ("/p/Trip-1.jpg".into(), "A".into()), + ("/p/Trip-1.xmp".into(), "As".into()), + ("/p/b.jpg".into(), "B".into()), + ("/p/c.jpg".into(), "C".into()) + ] + ); + // 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::Demo { .. } => String::new(), + }; + assert_eq!(path(&s, 1), "/p/Trip-1.jpg"); + assert_eq!(s.catalog.photo(PhotoId(1)).unwrap().file_name, "Trip-1.jpg"); + assert_eq!((path(&s, 2), path(&s, 3)), ("/p/b.jpg".into(), "/p/c.jpg".into())); + assert_eq!(s.undo.last().map(|e| e.label.as_str()), Some("Rename 1 Photo (partly)")); + + // a sidecar that won't move, and a file that won't go back: the file keeps its new name + // with the sidecars that did move, and says so + let fs = FakeFs::new(false, &[("/p/d.jpg", "D"), ("/p/d.xmp", "Ds"), ("/p/d.jpg.xmp", "Df")]); + 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_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")]); + fs.fail.borrow_mut().push("d.jpg.xmp".to_string()); + let e = move_file_with(&fs, "/p/d.jpg", "/p/e.jpg").unwrap_err(); + assert!(!e.moved); + assert_eq!(fs.listing().len(), 3); + assert!(fs.listing().iter().all(|(p, _)| p.starts_with("/p/d."))); + } + /// Issue #95 on the real disk, whichever kind of volume the temp folder is on. #[test] fn case_only_rename_on_disk() { diff --git a/crates/engine/src/tests_import.rs b/crates/engine/src/tests_import.rs index 6be7ce9a..cd156395 100644 --- a/crates/engine/src/tests_import.rs +++ b/crates/engine/src/tests_import.rs @@ -144,6 +144,45 @@ fn browsing_a_folder_lists_its_photos_without_adding_them() { let _ = std::fs::remove_dir_all(&dir); } +/// Issue #105: Find Missing finds renamed files by content, prefers the content match over a +/// same-name same-size impostor, and skips (reports) photos it can't tell apart. +#[test] +fn find_missing_matches_renamed_files_by_content() { + let dir = temp_dir("missing-content"); + std::fs::create_dir_all(dir.join("old")).unwrap(); + std::fs::create_dir_all(dir.join("moved/real")).unwrap(); + for (n, seed) in [("a.png", 1), ("b.png", 2), ("c.png", 3)] { + write_png(&dir.join("old").join(n), seed); + } + let mut s = Session::new().with_fs(); + s.execute("library.import", &json!({"paths": [dir.join("old").to_string_lossy()]})).unwrap(); + // an impostor: same name and size, other bytes + let impostor = |src: &Path, dst: &Path| { + let mut b = std::fs::read(src).unwrap(); + let k = b.len() - 20; + b[k] ^= 0xff; + std::fs::write(dst, b).unwrap(); + }; + // a: renamed; b: renamed, with an impostor under its old name; c: gone, two impostors + std::fs::rename(dir.join("old/a.png"), dir.join("moved/Trip-001.png")).unwrap(); + impostor(&dir.join("old/b.png"), &dir.join("moved/b.png")); + std::fs::rename(dir.join("old/b.png"), dir.join("moved/real/Trip-002.png")).unwrap(); + impostor(&dir.join("old/c.png"), &dir.join("moved/c.png")); + impostor(&dir.join("old/c.png"), &dir.join("moved/real/c.png")); + std::fs::remove_file(dir.join("old/c.png")).unwrap(); + let r = s.execute("library.findMissing", &json!({"folder": dir.join("moved").to_string_lossy()})).unwrap(); + let found = r["found"].as_array().unwrap(); + assert_eq!(found.len(), 2, "{r}"); + 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!(r["missing"], 1); + assert_eq!(r["ambiguous"][0]["candidates"].as_array().map(Vec::len), Some(2), "{r}"); + let _ = std::fs::remove_dir_all(&dir); +} + #[test] fn missing_files_are_found_and_relinked() { let dir = std::env::temp_dir().join(format!("lc-missing-{}", std::process::id())); diff --git a/crates/ui-egui/src/menus.rs b/crates/ui-egui/src/menus.rs index 5f2ed2e0..2d0bcff2 100644 --- a/crates/ui-egui/src/menus.rs +++ b/crates/ui-egui/src/menus.rs @@ -926,7 +926,12 @@ pub fn run_ui_command(app: &mut LightcraftApp, id: &str, p: &Value) -> Option 0 { format!(" ({unsure} with several look-alike files: use Locate)") } else { String::new() }; + app.toast( + &egui::Context::default(), + format!("Found {n} missing photo{}; {left} still missing{unsure}", if n == 1 { "" } else { "s" }), + ); } r } diff --git a/docs/parity.md b/docs/parity.md index ced24174..2b864225 100644 --- a/docs/parity.md +++ b/docs/parity.md @@ -153,7 +153,7 @@ under the UI command that wraps it (a no-op error); the UI command now wins. | LR-LIB-DELETE | Delete / Recently Deleted | P0 | ✅ | `cmd:photo.delete`, `cmd:photo.restore`, `cmd:photo.deletePermanently` | no confirmation dialog, no auto-purge after N days, no "Empty" | | LR-LIB-REMOVE-ALBUM | Remove from album | P0 | ✅ | `cmd:album.removePhotos` | | | LR-LIB-DUPLICATE | Duplicate a photo | P2 | ✅ | `cmd:photo.duplicate` | Photo ▸ Duplicate: a real `-copy` file with the same settings, metadata and albums (virtual copies share the file) | -| LR-LIB-RENAME | Batch rename | P1 | ✅ | `cmd:photo.rename`, `cmd:photo.renamePreview`, `cmd:photo.renameTokens`, `cmd:dialog.rename`, `crates/engine/src/rename.rs` (`TOKENS`) | template tokens {name} {num} {seq:N} {date:%Y%m%d} {folder} {camera} {lens} {iso} {rating} {title} {creator} {ext} (shared with export naming and import rename); a Tags picker beside every template field (Rename Photos, import Rename, export File name) lists each tag with meaning + live example, the date directives and the rules, and inserts the clicked tag at the text cursor; unknown tags are flagged; preview; renames files on disk with their XMP sidecars, never overwriting (-1, -2… suffixes; a change of letter case only is checked by file identity, so on case-sensitive volumes `img_1.jpg` next to `IMG_1.JPG` is a collision), rolls back on failure; undo/redo move the files; virtual copies follow | +| LR-LIB-RENAME | Batch rename | P1 | ✅ | `cmd:photo.rename`, `cmd:photo.renamePreview`, `cmd:photo.renameTokens`, `cmd:dialog.rename`, `crates/engine/src/rename.rs` (`TOKENS`) | template tokens {name} {num} {seq:N} {date:%Y%m%d} {folder} {camera} {lens} {iso} {rating} {title} {creator} {ext} (shared with export naming and import rename); a Tags picker beside every template field (Rename Photos, import Rename, export File name) lists each tag with meaning + live example, the date directives and the rules, and inserts the clicked tag at the text cursor; unknown tags are flagged; preview; renames files on disk with their XMP sidecars, never overwriting (-1, -2… suffixes; a change of letter case only is checked by file identity, so on case-sensitive volumes `img_1.jpg` next to `IMG_1.JPG` is a collision), rolls back on failure (a file that can't be moved back is reported old → new and the library follows it as an undoable partial rename; the same for undo/redo); undo/redo move the files; virtual copies follow | | LR-LIB-CAPTURETIME | Edit capture time | P1 | ✅ | `cmd:photo.setCaptureTime`, `cmd:dialog.captureTime`, `crates/catalog/src/dates.rs` | set (the other selected photos shift by the same amount, or `each`), shift by days/hours/minutes, time-zone shift; one undo step, journaled; Info panel button; no “revert to original capture time” | | LR-LIB-SHOWFINDER | Reveal original in file manager | P0 | ✅ | `cmd:app.showInFinder` | ⌘R (see MENU-FILE-SHOWFINDER) | | LR-LIB-COVER | Album cover | P2 | ✅ | `cmd:album.setCover` | | @@ -727,7 +727,7 @@ From `08-lightroom-classic-extras.md` (Classic-only features) and part 2 of `06- | LRC-LIB-PEOPLE | People view | P2 | ⬜ | | | | LRC-LIB-COMMENTS | Comments panel | P2 | ⬜ | | | | LRC-LIB-VISUALSEARCH | Find similar photos | P2 | ✅ | `cmd:library.findSimilar`, `crates/pipeline/src/cull.rs` (`signature`) | photo menu / Photo ▸ Find Similar Photos: look-alike photos (composition and tones), most similar first, as a filter (`only`) | -| LRC-LIB-MISSING | Missing files & relink | P1 | ✅ | `cmd:library.missing`, `cmd:photo.relink`, `cmd:library.findMissing`, `cmd:file.findMissing`, `cmd:photo.locate`, `crates/engine/src/cmd/missing.rs` (`checked_path`) | File → Find Missing Photos… (same name and size anywhere in a folder, one undo step), photo menu → Locate Missing File…; unreadable files show "!" in the grid and a reason in the loupe; undo never moves files. Missing Photos in the sidebar (counted on a worker thread); it covers library photos only — Local browse records are never checked, for the count, the view, `library.missing` and Find Missing alike | +| LRC-LIB-MISSING | Missing files & relink | P1 | ✅ | `cmd:library.missing`, `cmd:photo.relink`, `cmd:library.findMissing`, `cmd:file.findMissing`, `cmd:photo.locate`, `crates/engine/src/cmd/missing.rs` (`checked_path`) | File → Find Missing Photos… (anywhere in a folder: same name, size and content hash; renamed files by size + content hash; same name and size alone only when unambiguous — look-alikes are skipped and reported; one undo step), photo menu → Locate Missing File…; unreadable files show "!" in the grid and a reason in the loupe; undo never moves files. Missing Photos in the sidebar (counted on a worker thread); it covers library photos only — Local browse records are never checked, for the count, the view, `library.missing` and Find Missing alike | | LRC-LIB-CONVERT | Convert to DNG | P2 | ✅ | `cmd:photo.convertToDng`, `crates/engine/src/cmd/convert.rs` | Photo ▸ Convert to DNG: lossless DNG next to the raw with the settings embedded, photo (and its virtual copies) relinked; originals kept; undoable | | LRC-LIB-PREVIEWS | Build / discard previews | P1 | ✅ | `cmd:library.buildPreviews`, `cmd:library.previewProgress`, `cmd:library.cancelPreviews`, `cmd:library.clearPreviews`, `crates/engine/src/cmd/previews.rs`, `crates/preview/src/lib.rs` | File ▸ Previews: build standard-sized (Settings → Performance size) or 1:1 previews of the selected / visible photos in the background (progress toasts, stop), discard the cache; disk thumbnail + view cache; no smart previews (offline proxies) | | LRC-LIB-SLIDESHOW-IMPROMPTU | Impromptu slideshow | P2 | ✅ | `cmd:view.slideshow` | see LR-VIEW-SLIDESHOW | diff --git a/docs/whats-new.md b/docs/whats-new.md index a976c8ef..524f77ae 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -24,6 +24,10 @@ - Rename Photos never overwrites another photo when only the letter case changes (issue #95): on case-sensitive volumes (Linux, case-sensitive APFS) `img_1.JPG` next to `IMG_1.JPG` is a different photo and the renamed one gets `img_1-1.JPG`; on case-insensitive volumes the case change still goes through. +- Rename Photos reports files it could not move back after a failure (issue #105), e.g. when a network share drops + mid-batch: the error lists them (old → new) and the library points at their new names (an undoable partial rename), + so none shows as missing. Find Missing Photos also finds renamed files by their content, prefers a content match + over a same-name same-size look-alike, and skips (and reports) photos it can't tell apart instead of guessing. - Smart albums with a rule editor: match all / any / none, nested groups, 26 fields. - Quick Collection and target album (B in the grid), keyword sets (⌥1–⌥9), colour-label sets. - Colour-label filter with several labels at once; expandable folder tree in Local. From 46732cb0febc5b96bb80d0ded42af3cdc7907f2c Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Mon, 5 Oct 2026 17:03:07 -0400 Subject: [PATCH 4/8] Issue #93: Export: never overwrite an original; atomic writes - lightcraft_catalog::safe_file: one tested helper for crash-safe writes (temp file in the same folder, sync, rename, folder sync; temp removed on failure), write_new/write_new_unique that never replace a file and read the result back, same_file, and a per-thread fault injection for tests. - engine::originals::OriginalGuard: refuses a write whose target is a catalogued photo's original or XMP sidecar (by file identity), whatever the conflict policy or exact path. Used by run_batch (export, Export with Previous, control/MCP exact paths), ui.render/ui.screenshot paths, merge previewPath, MCP render_photo/screenshot paths (new query command export.checkTarget) and lightcraft-cli render. - The conflict policy now covers an export's sidecars together with the file (Unique gives both the same free name, Skip skips when either is taken). - export::write_file replaces the plain std::fs::write in the desktop app, CLI, MCP and engine writers; XMP sidecar writes use the shared helper. Co-Authored-By: Claude Opus 5.5 --- apps/lightcraft-cli/src/main.rs | 30 ++- apps/lightcraft-cli/tests/cli.rs | 18 ++ apps/lightcraft/src/main.rs | 15 +- crates/catalog/src/lib.rs | 1 + crates/catalog/src/safe_file.rs | 317 ++++++++++++++++++++++++++++++ crates/engine/src/cmd/convert.rs | 2 +- crates/engine/src/cmd/export.rs | 13 ++ crates/engine/src/cmd/merge.rs | 3 +- crates/engine/src/export.rs | 104 +++++++--- crates/engine/src/lib.rs | 1 + crates/engine/src/originals.rs | 102 ++++++++++ crates/engine/src/sidecar.rs | 8 +- crates/engine/src/tests_export.rs | 116 +++++++++++ crates/mcp/src/headless.rs | 10 +- crates/mcp/src/tests.rs | 30 ++- crates/mcp/src/tools.rs | 18 +- crates/ui-egui/src/control.rs | 13 +- docs/parity.md | 4 +- docs/whats-new.md | 7 + 19 files changed, 731 insertions(+), 81 deletions(-) create mode 100644 crates/catalog/src/safe_file.rs create mode 100644 crates/engine/src/originals.rs diff --git a/apps/lightcraft-cli/src/main.rs b/apps/lightcraft-cli/src/main.rs index 4f68ec88..00c236c2 100644 --- a/apps/lightcraft-cli/src/main.rs +++ b/apps/lightcraft-cli/src/main.rs @@ -533,11 +533,17 @@ fn render(args: &[String]) -> Result<(), String> { .ok_or_else(|| format!("{output}: unknown extension (use .jpg .png .tif .webp .avif .dng or --opt format=…)"))?; } let e = export_photo(&mut s, lightcraft_engine::catalog::PhotoId(id), &o, 1)?; - std::fs::write(&output, &e.bytes).map_err(|err| format!("{output}: {err}"))?; - for (ext, bytes) in &e.sidecars { - let sc = Path::new(&output).with_extension(ext); - std::fs::write(&sc, bytes).map_err(|err| format!("{}: {err}", sc.display()))?; - eprintln!("lightcraft-cli: wrote {}", sc.display()); + // never over the input (or its sidecar), however it is spelled: `render IMG.jpg -o IMG.jpg` + let sidecars: Vec<(String, &Vec)> = + e.sidecars.iter().map(|(ext, bytes)| (Path::new(&output).with_extension(ext).to_string_lossy().to_string(), bytes)).collect(); + let guard = s.original_guard(); + for p in std::iter::once(&output).chain(sidecars.iter().map(|(p, _)| p)) { + guard.check(Path::new(p)).map_err(|err| format!("render: {err}"))?; + } + lightcraft_engine::export::write_file(&output, &e.bytes)?; + for (sc, bytes) in &sidecars { + lightcraft_engine::export::write_file(sc, bytes)?; + eprintln!("lightcraft-cli: wrote {sc}"); } eprintln!("lightcraft-cli: wrote {output} ({}×{})", e.width, e.height); Ok(()) @@ -593,18 +599,8 @@ fn snapshot(args: &[String]) -> Result<(), String> { eprintln!("lightcraft-cli snapshot: imported {n} files in {:.0} ms", ti.elapsed().as_secs_f64() * 1e3); } let services = lightcraft_ui_egui::Services { - write_shared: Some(std::sync::Arc::new(|p: &str, b: &[u8]| { - if let Some(dir) = Path::new(p).parent().filter(|d| !d.as_os_str().is_empty()) { - std::fs::create_dir_all(dir).map_err(|e| e.to_string())?; - } - std::fs::write(p, b).map_err(|e| format!("{p}: {e}")) - })), - write: Some(Box::new(|p: &str, b: &[u8]| { - if let Some(dir) = Path::new(p).parent().filter(|d| !d.as_os_str().is_empty()) { - std::fs::create_dir_all(dir).map_err(|e| e.to_string())?; - } - std::fs::write(p, b).map_err(|e| format!("{p}: {e}")) - })), + write_shared: Some(std::sync::Arc::new(lightcraft_engine::export::write_file)), + write: Some(Box::new(lightcraft_engine::export::write_file)), png: Some(Box::new(|img: &lightcraft_raster::Rgba8| { lightcraft_codecs::encode_png(&lightcraft_codecs::EncodeImage::rgba8(img), &lightcraft_codecs::EncodeMeta::default()).unwrap_or_default() })), diff --git a/apps/lightcraft-cli/tests/cli.rs b/apps/lightcraft-cli/tests/cli.rs index 913200d2..b1f816b4 100644 --- a/apps/lightcraft-cli/tests/cli.rs +++ b/apps/lightcraft-cli/tests/cli.rs @@ -86,6 +86,24 @@ fn render_subcommand() { assert!(String::from_utf8_lossy(&o.stderr).contains("unknown control")); } +/// Issue #93: `render IMG -o IMG` replaced its own input with the render. +#[test] +fn render_refuses_to_overwrite_its_input() { + let input = tmp("self-in.png"); + gradient_png(&input); + let before = std::fs::read(&input).unwrap(); + let dir = input.parent().unwrap(); + std::fs::create_dir_all(dir.join("sub")).unwrap(); + let other_spelling = dir.join("sub/../self-in.png"); + for out in [&input, &other_spelling] { + let o = + Command::new(BIN).args(["render", input.to_str().unwrap(), "-o", out.to_str().unwrap(), "--set", "light.exposure=1"]).output().unwrap(); + assert!(!o.status.success()); + assert!(String::from_utf8_lossy(&o.stderr).contains("never writes over an original"), "{}", String::from_utf8_lossy(&o.stderr)); + } + assert_eq!(std::fs::read(&input).unwrap(), before, "the input is untouched"); +} + #[test] fn render_export_options() { let input = tmp("o-in.png"); diff --git a/apps/lightcraft/src/main.rs b/apps/lightcraft/src/main.rs index 981867c8..427c97cd 100644 --- a/apps/lightcraft/src/main.rs +++ b/apps/lightcraft/src/main.rs @@ -209,18 +209,9 @@ fn services() -> Services { .save_file() .map(|p| p.to_string_lossy().to_string()) })), - write_shared: Some(std::sync::Arc::new(|p: &str, b: &[u8]| { - if let Some(dir) = std::path::Path::new(p).parent().filter(|d| !d.as_os_str().is_empty()) { - std::fs::create_dir_all(dir).map_err(|e| e.to_string())?; - } - std::fs::write(p, b).map_err(|e| e.to_string()) - })), - write: Some(Box::new(|p: &str, b: &[u8]| { - if let Some(dir) = std::path::Path::new(p).parent().filter(|d| !d.as_os_str().is_empty()) { - std::fs::create_dir_all(dir).map_err(|e| e.to_string())?; - } - std::fs::write(p, b).map_err(|e| e.to_string()) - })), + // atomic (temp file + sync + rename): a failed write never leaves a truncated file + write_shared: Some(std::sync::Arc::new(lightcraft_engine::export::write_file)), + write: Some(Box::new(lightcraft_engine::export::write_file)), png: Some(Box::new(|img: &lightcraft_raster::Rgba8| { lightcraft_codecs::encode_png(&lightcraft_codecs::EncodeImage::rgba8(img), &lightcraft_codecs::EncodeMeta::default()).unwrap_or_default() })), diff --git a/crates/catalog/src/lib.rs b/crates/catalog/src/lib.rs index 4b9bdcdc..5bea87e8 100644 --- a/crates/catalog/src/lib.rs +++ b/crates/catalog/src/lib.rs @@ -15,6 +15,7 @@ pub mod local; pub mod model; pub mod query; pub mod rules; +pub mod safe_file; pub mod stacks; pub mod store; diff --git a/crates/catalog/src/safe_file.rs b/crates/catalog/src/safe_file.rs new file mode 100644 index 00000000..1e482b81 --- /dev/null +++ b/crates/catalog/src/safe_file.rs @@ -0,0 +1,317 @@ +//! Crash-safe writes of user-visible files: exports, renders, DNG conversions, smart previews and +//! XMP sidecars all go through here (the catalog's own files use [`crate::store::FsStore`]). +//! +//! - [`write_atomic`] replaces a file so that after a crash, a full disk or an unplugged drive +//! either the old or the complete new content is there — never a truncated mix. The content +//! goes to a new temp file in the same folder (unique name, `create_new`), is synced to disk and +//! renamed over the target; then the folder is synced (POSIX). Any failure removes the temp +//! file and leaves the target as it was. +//! - [`write_new`] / [`write_new_unique`] write a file that must not exist yet: the same temp file +//! and sync, plus a read-back check, and the file then appears under its final name **without +//! replacing anything** (a hard link, which fails on a taken name; a check-then-rename where the +//! volume has no hard links). +//! - [`same_file`] tells whether two paths name the same file on disk (device + inode on Unix, +//! resolved paths elsewhere), for "is this target an original?" checks. + +use std::cell::Cell; +use std::fs::{self, File, OpenOptions}; +use std::io::{self, Write}; +use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicU64, Ordering}; + +thread_local! { + static FAIL_AFTER: Cell> = const { Cell::new(None) }; +} + +/// Fault injection for tests (in any crate): while the returned guard lives, every write made by +/// this module **on the calling thread** fails once `bytes` bytes of a file were written — as a +/// full disk or an unplugged drive would. +#[doc(hidden)] +pub fn fail_writes_after(bytes: u64) -> FaultGuard { + FAIL_AFTER.with(|c| c.set(Some(bytes))); + FaultGuard(()) +} + +/// Ends a [`fail_writes_after`] when dropped. +#[doc(hidden)] +pub struct FaultGuard(()); + +impl Drop for FaultGuard { + fn drop(&mut self) { + FAIL_AFTER.with(|c| c.set(None)); + } +} + +/// Counts what is written; fails past an injected limit. +struct Limited { + inner: W, + n: u64, + limit: Option, +} + +impl Write for Limited { + fn write(&mut self, buf: &[u8]) -> io::Result { + let buf = match self.limit { + Some(l) => { + let room = usize::try_from(l.saturating_sub(self.n)).unwrap_or(usize::MAX); + if room == 0 && !buf.is_empty() { + return Err(io::Error::other("write failed (injected fault)")); + } + buf.get(..room.min(buf.len())).unwrap_or(buf) + } + None => buf, + }; + let k = self.inner.write(buf)?; + self.n += k as u64; + Ok(k) + } + fn flush(&mut self) -> io::Result<()> { + self.inner.flush() + } +} + +fn parent_of(path: &Path) -> &Path { + path.parent().filter(|d| !d.as_os_str().is_empty()).unwrap_or(Path::new(".")) +} + +/// Make a rename or a new name in `dir` durable (POSIX). Opening a directory fails on Windows, +/// where NTFS journals the rename itself, so errors are ignored. +pub fn sync_dir(dir: &Path) { + if let Ok(d) = File::open(dir) { + let _ = d.sync_all(); + } +} + +/// A new, empty temp file next to `path` (hidden, unique in this process and folder). +fn create_temp(path: &Path) -> io::Result<(PathBuf, File)> { + static N: AtomicU64 = AtomicU64::new(0); + if path.file_name().is_none() { + return Err(io::Error::new(io::ErrorKind::InvalidInput, format!("{} is not a file path", path.display()))); + } + let dir = parent_of(path); + for _ in 0..1000 { + let tmp = dir.join(format!(".lightcraft-{}-{}.tmp", std::process::id(), N.fetch_add(1, Ordering::Relaxed))); + match OpenOptions::new().write(true).create_new(true).open(&tmp) { + Ok(f) => return Ok((tmp, f)), + Err(e) if e.kind() == io::ErrorKind::AlreadyExists => continue, + Err(e) => return Err(e), + } + } + Err(io::Error::new(io::ErrorKind::AlreadyExists, format!("no free temp file name in {}", dir.display()))) +} + +/// Write a temp file next to `path` with what `fill` writes, synced to disk. On failure the temp +/// file is removed. Returns its path and length. +fn write_temp(path: &Path, fill: &mut dyn FnMut(&mut dyn Write) -> io::Result<()>) -> io::Result<(PathBuf, u64)> { + let (tmp, file) = create_temp(path)?; + let r = (|| { + let limit = FAIL_AFTER.with(Cell::get); + let mut w = Limited { inner: io::BufWriter::with_capacity(1 << 20, file), n: 0, limit }; + fill(&mut w)?; + w.flush()?; + let n = w.n; + let f = w.inner.into_inner().map_err(|e| e.into_error())?; + f.sync_all()?; + Ok(n) + })(); + match r { + Ok(n) => Ok((tmp, n)), + Err(e) => { + let _ = fs::remove_file(&tmp); + Err(e) + } + } +} + +/// Replace (or create) `path` with `data` atomically and durably; see the module docs. The +/// folder must exist. +pub fn write_atomic(path: &Path, data: &[u8]) -> io::Result<()> { + write_atomic_with(path, &mut |w| w.write_all(data)).map(|_| ()) +} + +/// [`write_atomic`] with streamed content: `fill` writes it (an error aborts and leaves the old +/// file). Returns the length written. +pub fn write_atomic_with(path: &Path, fill: &mut dyn FnMut(&mut dyn Write) -> io::Result<()>) -> io::Result { + let (tmp, n) = write_temp(path, fill)?; + if let Err(e) = fs::rename(&tmp, path) { + let _ = fs::remove_file(&tmp); + return Err(e); + } + sync_dir(parent_of(path)); + Ok(n) +} + +/// Write `data` as the new file `path`: never replaces an existing file (`AlreadyExists`), and +/// the file only appears once it is complete, synced and read back identical. +pub fn write_new(path: &Path, data: &[u8]) -> io::Result<()> { + write_new_unique(&mut std::iter::once(path.to_path_buf()), data).map(|_| ()) +} + +/// [`write_new`] under the first free name of `candidates` (all in the same folder, e.g. +/// `IMG_1.dng`, `IMG_1-2.dng`, …): the file is written once and then given the first name that +/// is not taken. Returns that name; `AlreadyExists` when every candidate is taken. +pub fn write_new_unique(candidates: &mut dyn Iterator, data: &[u8]) -> io::Result { + let mut candidates = candidates.peekable(); + let Some(first) = candidates.peek().cloned() else { + return Err(io::Error::new(io::ErrorKind::InvalidInput, "no file name to write")); + }; + let (tmp, _) = write_temp(&first, &mut |w| w.write_all(data))?; + let r = (|| { + if !same_content(&tmp, data)? { + return Err(io::Error::other("the written file reads back different (failing drive or connection?)")); + } + for path in candidates.by_ref().take(100_000) { + match publish_new(&tmp, &path) { + Ok(()) => { + sync_dir(parent_of(&path)); + return Ok(path); + } + Err(e) if e.kind() == io::ErrorKind::AlreadyExists => continue, + Err(e) => return Err(e), + } + } + Err(io::Error::new(io::ErrorKind::AlreadyExists, format!("{}: no free file name", first.display()))) + })(); + // after a hard link the temp name is a second name of the new file; after a rename it's gone + let _ = fs::remove_file(&tmp); + r +} + +/// Give the complete temp file `tmp` the name `path`, unless that is taken. +fn publish_new(tmp: &Path, path: &Path) -> io::Result<()> { + match fs::hard_link(tmp, path) { + Ok(()) => Ok(()), + Err(e) if e.kind() == io::ErrorKind::AlreadyExists => Err(e), + // no hard links on this volume (FAT/exFAT, some shares): check, then rename + Err(_) => { + if fs::symlink_metadata(path).is_ok() { + return Err(io::Error::new(io::ErrorKind::AlreadyExists, format!("{} exists", path.display()))); + } + fs::rename(tmp, path) + } + } +} + +/// Does the file at `path` hold exactly `data`? +fn same_content(path: &Path, data: &[u8]) -> io::Result { + use std::io::Read; + let mut f = File::open(path)?; + if f.metadata()?.len() != data.len() as u64 { + return Ok(false); + } + let mut buf = vec![0u8; 1 << 20]; + let mut at = 0usize; + loop { + let k = match f.read(&mut buf) { + Ok(0) => return Ok(at == data.len()), + Ok(k) => k, + Err(e) if e.kind() == io::ErrorKind::Interrupted => continue, + Err(e) => return Err(e), + }; + if data.get(at..at + k) != buf.get(..k) { + return Ok(false); + } + at += k; + } +} + +/// Do `a` and `b` name the same existing file (through links, `..`, case-insensitive names)? +pub fn same_file(a: &Path, b: &Path) -> bool { + let (Ok(ma), Ok(mb)) = (fs::metadata(a), fs::metadata(b)) else { return false }; + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt; + ma.dev() == mb.dev() && ma.ino() == mb.ino() + } + #[cfg(not(unix))] + { + if ma.len() != mb.len() { + return false; + } + match (fs::canonicalize(a), fs::canonicalize(b)) { + (Ok(x), Ok(y)) => x == y || x.to_string_lossy().to_lowercase() == y.to_string_lossy().to_lowercase(), + _ => false, + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn temp(tag: &str) -> PathBuf { + let d = std::env::temp_dir().join(format!("lc-safe-file-{tag}-{}", std::process::id())); + let _ = fs::remove_dir_all(&d); + fs::create_dir_all(&d).unwrap(); + d + } + + fn names(dir: &Path) -> Vec { + let mut v: Vec = fs::read_dir(dir).unwrap().flatten().map(|e| e.file_name().to_string_lossy().to_string()).collect(); + v.sort(); + v + } + + #[test] + fn atomic_write_replaces_and_leaves_no_temp() { + let d = temp("replace"); + let p = d.join("out.jpg"); + write_atomic(&p, b"one").unwrap(); + write_atomic(&p, b"two two").unwrap(); + assert_eq!(fs::read(&p).unwrap(), b"two two"); + assert_eq!(names(&d), vec!["out.jpg"]); + let _ = fs::remove_dir_all(&d); + } + + #[test] + fn a_failure_mid_write_keeps_the_old_file() { + let d = temp("midwrite"); + let p = d.join("photo.jpg"); + fs::write(&p, b"the original bytes").unwrap(); + { + let _f = fail_writes_after(4); + let e = write_atomic(&p, &[7u8; 10_000]).unwrap_err(); + assert!(e.to_string().contains("injected"), "{e}"); + assert!(write_new(&d.join("new.dng"), &[1u8; 100]).is_err()); + } + assert_eq!(fs::read(&p).unwrap(), b"the original bytes", "the target is untouched"); + assert_eq!(names(&d), vec!["photo.jpg"], "no temp file and no partial new file"); + // a fill error aborts the same way + let r = write_atomic_with(&p, &mut |w| { + w.write_all(b"half")?; + Err(io::Error::other("encoder failed")) + }); + assert!(r.is_err()); + assert_eq!(fs::read(&p).unwrap(), b"the original bytes"); + assert_eq!(names(&d), vec!["photo.jpg"]); + let _ = fs::remove_dir_all(&d); + } + + #[test] + fn new_files_never_replace_anything() { + let d = temp("new"); + let p = d.join("a.dng"); + fs::write(&p, b"keep").unwrap(); + assert_eq!(write_new(&p, b"other").unwrap_err().kind(), io::ErrorKind::AlreadyExists); + assert_eq!(fs::read(&p).unwrap(), b"keep"); + let mut c = (1..).map(|k| if k == 1 { d.join("a.dng") } else { d.join(format!("a-{k}.dng")) }); + let got = write_new_unique(&mut c, b"fresh").unwrap(); + assert_eq!(got, d.join("a-2.dng")); + assert_eq!(fs::read(&got).unwrap(), b"fresh"); + assert_eq!(names(&d), vec!["a-2.dng", "a.dng"], "the temp file is gone"); + let _ = fs::remove_dir_all(&d); + } + + #[test] + fn same_file_sees_through_other_spellings() { + let d = temp("same"); + let p = d.join("x.jpg"); + fs::write(&p, b"x").unwrap(); + fs::write(d.join("y.jpg"), b"x").unwrap(); + fs::create_dir_all(d.join("sub")).unwrap(); + assert!(same_file(&p, &d.join("sub/../x.jpg"))); + assert!(!same_file(&p, &d.join("y.jpg"))); + assert!(!same_file(&p, &d.join("missing.jpg"))); + let _ = fs::remove_dir_all(&d); + } +} diff --git a/crates/engine/src/cmd/convert.rs b/crates/engine/src/cmd/convert.rs index ddb2fdac..26725e70 100644 --- a/crates/engine/src/cmd/convert.rs +++ b/crates/engine/src/cmd/convert.rs @@ -133,7 +133,7 @@ fn edit_external(s: &mut Session, p: &Value) -> Result { }; let mut written = Vec::new(); let mut write = |path: &str, bytes: &[u8]| -> std::result::Result<(), String> { - std::fs::write(path, bytes).map_err(|e| format!("{path}: {e}"))?; + crate::export::write_file(path, bytes)?; written.push(path.to_string()); Ok(()) }; diff --git a/crates/engine/src/cmd/export.rs b/crates/engine/src/cmd/export.rs index 75173c84..11a11f6c 100644 --- a/crates/engine/src/cmd/export.rs +++ b/crates/engine/src/cmd/export.rs @@ -96,5 +96,18 @@ pub fn specs() -> Vec { save ), cmd!("export.deletePreset", "Delete Export Preset", [], None, "{name} — removes a user preset → presets", always, delete), + cmd!( + query "export.checkTarget", + "Check Output Path", + [], + None, + "{path} — fails when writing `path` would replace a photo's original (or its XMP sidecar) in the library; front ends call it before saving a render or screenshot to a user-given path → {path}", + always, + |s, p| { + let path = p.get("path").and_then(Value::as_str).ok_or_else(|| bad("export.checkTarget", "missing `path`"))?; + s.check_write_target(path).map_err(|e| bad("export.checkTarget", e))?; + Ok(json!({"path": path})) + } + ), ] } diff --git a/crates/engine/src/cmd/merge.rs b/crates/engine/src/cmd/merge.rs index 102ff0d3..a8869153 100644 --- a/crates/engine/src/cmd/merge.rs +++ b/crates/engine/src/cmd/merge.rs @@ -30,7 +30,8 @@ fn run(s: &mut Session, id: &str, p: &Value) -> Result { if let (Some(path), Some(img)) = (str_param(p, "previewPath"), &out.preview) { let png = lightcraft_codecs::encode_png(&lightcraft_codecs::EncodeImage::rgba8(img), &lightcraft_codecs::EncodeMeta::default()) .map_err(|e| bad(id, e.to_string()))?; - std::fs::write(path, png).map_err(|e| bad(id, format!("{path}: {e}")))?; + s.check_write_target(path).map_err(|e| bad(id, e))?; + crate::export::write_file(path, &png).map_err(|e| bad(id, e))?; info["previewPath"] = json!(path); } if let Some(img) = &out.preview { diff --git a/crates/engine/src/export.rs b/crates/engine/src/export.rs index 70f89a7a..f0e6bde8 100644 --- a/crates/engine/src/export.rs +++ b/crates/engine/src/export.rs @@ -930,6 +930,8 @@ pub struct PreparedExport { pub photo: lightcraft_catalog::PhotoId, pub file_name: String, work: Work, + /// The library's originals, which [`run_batch`] never writes over (shared by a batch). + guard: std::sync::Arc, } struct RenderWork { @@ -952,12 +954,30 @@ enum Work { }, } -/// Set up the export of photo `id` at 1-based position `seq` of a batch. +/// Set up the export of photo `id` at 1-based position `seq` of a batch. (For a whole batch use +/// [`prepare_batch`]: it looks at the library's originals once.) pub fn prepare_export( session: &mut crate::Session, id: lightcraft_catalog::PhotoId, o: &ExportOptions, seq: usize, +) -> Result { + let guard = std::sync::Arc::new(session.original_guard()); + prepare_guarded(session, id, o, seq, guard) +} + +/// [`prepare_export`] for each of `ids` in order (`{seq}` = position, from 1). +pub fn prepare_batch(session: &mut crate::Session, ids: &[lightcraft_catalog::PhotoId], o: &ExportOptions) -> Result, String> { + let guard = std::sync::Arc::new(session.original_guard()); + ids.iter().enumerate().map(|(i, id)| prepare_guarded(session, *id, o, i + 1, guard.clone())).collect() +} + +fn prepare_guarded( + session: &mut crate::Session, + id: lightcraft_catalog::PhotoId, + o: &ExportOptions, + seq: usize, + guard: std::sync::Arc, ) -> Result { let p = session.catalog.photo(id).ok_or("no such photo")?; let file_name = o.file_name_for(p, seq); @@ -979,7 +999,7 @@ pub fn prepare_export( size: (p.width as usize, p.height as usize), } }; - Ok(PreparedExport { photo: id, file_name, work }) + Ok(PreparedExport { photo: id, file_name, work, guard }) } impl PreparedExport { @@ -1031,14 +1051,16 @@ pub enum DngCompression { } /// The destination of a batch: a folder (with `ExportOptions::subfolder` and the conflict policy -/// applied), or one exact file path (single photo; overwritten). +/// applied to each file together with its sidecars), or one exact file path (single photo; an +/// ordinary file already there is replaced). Either way a catalogued original (or its sidecar) +/// is never written over: see [`crate::originals`]. #[derive(Clone, Debug, Default)] pub struct Destination { pub dir: String, pub exact: Option, } -/// Export `ids` in order ([`prepare_export`] + [`run_batch`]), stopping at the first error. +/// Export `ids` in order ([`prepare_batch`] + [`run_batch`]), stopping at the first error. pub fn export_batch( session: &mut crate::Session, ids: &[lightcraft_catalog::PhotoId], @@ -1047,15 +1069,34 @@ pub fn export_batch( write: &mut dyn FnMut(&str, &[u8]) -> Result<(), String>, exists: &dyn Fn(&str) -> bool, ) -> Result, String> { - let items = ids.iter().enumerate().map(|(i, id)| prepare_export(session, *id, o, i + 1)).collect::, _>>()?; + let items = prepare_batch(session, ids, o)?; run_batch(items, o, to, write, exists, true, &mut |_, _| true) } +/// Write an exported or rendered file on disk: its folder is created if needed, and the file is +/// replaced atomically ([`lightcraft_catalog::safe_file::write_atomic`]: a temp file, synced, then +/// renamed), so a failure part-way leaves any previous file intact and no truncated one. The +/// native writer behind exports, renders and screenshots (app, CLI, MCP). +pub fn write_file(path: &str, bytes: &[u8]) -> Result<(), String> { + let p = std::path::Path::new(path); + if let Some(dir) = p.parent().filter(|d| !d.as_os_str().is_empty()) { + std::fs::create_dir_all(dir).map_err(|e| format!("{}: {e}", dir.display()))?; + } + lightcraft_catalog::safe_file::write_atomic(p, bytes).map_err(|e| format!("{path}: {e}")) +} + +/// The path of the sidecar with extension `ext` of the exported file `main`. +fn sidecar_path(main: &str, ext: &str) -> String { + std::path::Path::new(main).with_extension(ext).to_string_lossy().to_string() +} + /// Run prepared exports in order: pick each one's path, and hand the bytes (and sidecars) to -/// `write`. `exists` tells whether a path is taken (conflict policy). `progress(done, next file)` -/// is called before each photo; returning false cancels the rest. Returns one JSON object per -/// photo: `{path, width, height, bytes, sidecars}`, `{skipped: path}` or (unless -/// `stop_on_error`) `{photo, file, error}`. +/// `write`. `exists` tells whether a path is taken. The conflict policy applies to a file and its +/// sidecars as one: with Unique both get the same free name, with Skip the photo is skipped when +/// either is taken. A path that is a catalogued original (or its sidecar) is refused whatever the +/// policy. `progress(done, next file)` is called before each photo; returning false cancels the +/// rest. Returns one JSON object per photo: `{path, width, height, bytes, sidecars}`, +/// `{skipped: path}` or (unless `stop_on_error`) `{photo, file, error}`. pub fn run_batch( items: Vec, o: &ExportOptions, @@ -1075,7 +1116,7 @@ pub fn run_batch( if !progress(i, &item.file_name) { break; } - let (photo, name) = (item.photo, item.file_name.clone()); + let (photo, name, guard) = (item.photo, item.file_name.clone(), item.guard.clone()); let e = match item.run() { Ok(e) => e, Err(err) if stop_on_error => return Err(err), @@ -1084,14 +1125,18 @@ pub fn run_batch( continue; } }; + // the exported file and its sidecars + let group = |main: &str| std::iter::once(main.to_string()).chain(e.sidecars.iter().map(|(x, _)| sidecar_path(main, x))).collect::>(); let path = match to.exact.as_deref().filter(|_| single) { - Some(p) => p.to_string(), + Some(p) => Ok(p.to_string()), None => { - let mut path = join(&dir, &e.file_name); - let busy = |p: &str, taken: &std::collections::HashSet| taken.contains(p) || exists(p); - if busy(&path, &taken) { + let path = join(&dir, &e.file_name); + let busy = |main: &str| group(main).iter().any(|p| taken.contains(p) || exists(p)); + if !busy(&path) { + Ok(path) + } else { match o.conflict { - Conflict::Overwrite => {} + Conflict::Overwrite => Ok(path), Conflict::Skip => { out.push(json!({"skipped": path})); continue; @@ -1099,29 +1144,34 @@ pub fn run_batch( Conflict::Unique => { let (stem, ext) = e.file_name.rsplit_once('.').map_or((e.file_name.as_str(), None), |(a, b)| (a, Some(b))); let name = |n: usize| join(&dir, &ext.map_or(format!("{stem}-{n}"), |x| format!("{stem}-{n}.{x}"))); - path = (2..).map(name).find(|p| !busy(p, &taken)).expect("a free name"); + (2..1_000_000).map(name).find(|p| !busy(p)).ok_or_else(|| format!("{path}: no free file name")) } } } - path } }; - let written = write(&path, &e.bytes).and_then(|()| { - let mut sidecars = Vec::new(); - for (ext, bytes) in &e.sidecars { - let sc = std::path::Path::new(&path).with_extension(ext).to_string_lossy().to_string(); - write(&sc, bytes)?; - sidecars.push(sc); + let file = path.clone().unwrap_or_else(|_| name.clone()); + let written = path.and_then(|path| { + let files = group(&path); + // never over an original, whatever the conflict policy or the exact path said + for f in &files { + guard.check(std::path::Path::new(f))?; } - Ok(sidecars) + write(&path, &e.bytes)?; + let mut sidecars = Vec::new(); + for ((_, bytes), sc) in e.sidecars.iter().zip(files.iter().skip(1)) { + write(sc, bytes)?; + sidecars.push(sc.clone()); + } + Ok((path, files, sidecars)) }); match written { - Ok(sidecars) => { - taken.insert(path.clone()); + Ok((path, files, sidecars)) => { + taken.extend(files); out.push(json!({"path": path, "width": e.width, "height": e.height, "bytes": e.bytes.len(), "sidecars": sidecars})); } Err(err) if stop_on_error => return Err(err), - Err(err) => out.push(json!({"photo": photo.0, "file": path, "error": err})), + Err(err) => out.push(json!({"photo": photo.0, "file": file, "error": err})), } } Ok(out) diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index dcbfd9ed..b02e3d54 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -24,6 +24,7 @@ pub mod library; pub mod media; pub mod memory; pub mod merge; +pub mod originals; pub mod preset_import; pub mod preset_luminar; pub mod presets; diff --git a/crates/engine/src/originals.rs b/crates/engine/src/originals.rs new file mode 100644 index 00000000..59a5f6a2 --- /dev/null +++ b/crates/engine/src/originals.rs @@ -0,0 +1,102 @@ +//! Never write over an original: exports, renders and other files LightCraft writes for the user +//! must not land on a catalogued photo's own file (or its XMP sidecar), whatever the conflict +//! setting or exact path asked for. [`OriginalGuard`] is a snapshot of the library's originals +//! that answers "would writing here replace one?" by file identity (the same file under another +//! spelling, through `..`, a link or a case-insensitive volume counts), not by comparing strings. + +use std::collections::HashMap; +use std::path::{Path, PathBuf}; + +use lightcraft_catalog::safe_file::same_file; +use lightcraft_catalog::{Catalog, Source}; + +/// The library's originals and their sidecars, looked up by file name. +#[derive(Debug, Default)] +pub struct OriginalGuard { + /// lower-case file name → (path on disk, what it is: "the original of IMG_1.CR3", …) + by_name: HashMap>, +} + +impl OriginalGuard { + /// The originals of every photo in `catalog`. + pub fn new(catalog: &Catalog) -> Self { + let mut g = Self::default(); + for p in catalog.photos() { + if let Source::File { path } = &p.source { + g.add(Path::new(path), &p.file_name); + } + } + g + } + + /// Protect `path` (an original named `label`) and its sidecars (`IMG_1.xmp`, `IMG_1.CR3.xmp`). + pub fn add(&mut self, path: &Path, label: &str) { + let Some(name) = path.file_name().map(|n| n.to_string_lossy().to_string()) else { return }; + let mut put = |file: String, p: PathBuf, what: String| self.by_name.entry(file.to_lowercase()).or_default().push((p, what)); + put(name.clone(), path.to_path_buf(), format!("the original of {label}")); + let stem = path.file_stem().map(|s| s.to_string_lossy().to_string()).unwrap_or_default(); + put(format!("{stem}.xmp"), path.with_extension("xmp"), format!("the XMP sidecar of {label}")); + put(format!("{name}.xmp"), PathBuf::from(format!("{}.xmp", path.to_string_lossy())), format!("the XMP sidecar of {label}")); + } + + /// `Err` (a message for the user) when writing `target` would replace a protected file. + pub fn check(&self, target: &Path) -> Result<(), String> { + if std::fs::symlink_metadata(target).is_err() { + return Ok(()); // nothing there to replace + } + let mut names: Vec = target.file_name().map(|n| n.to_string_lossy().to_lowercase()).into_iter().collect(); + if let Some(n) = std::fs::canonicalize(target).ok().and_then(|c| c.file_name().map(|n| n.to_string_lossy().to_lowercase())) { + names.push(n); + } + for n in &names { + for (p, what) in self.by_name.get(n).into_iter().flatten() { + if same_file(p, target) { + return Err(format!( + "{} is {what} in the library: LightCraft never writes over an original (choose another folder or file name)", + target.display() + )); + } + } + } + Ok(()) + } +} + +impl crate::Session { + /// A guard over this library's originals (see [`OriginalGuard`]). + pub fn original_guard(&self) -> OriginalGuard { + OriginalGuard::new(&self.catalog) + } + + /// `Err` when writing `path` would replace a catalogued photo's original or sidecar. + pub fn check_write_target(&self, path: &str) -> Result<(), String> { + self.original_guard().check(Path::new(path)) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use lightcraft_catalog::{Op, Photo, PhotoId}; + + #[test] + fn originals_and_their_sidecars_are_protected_by_identity() { + let d = std::env::temp_dir().join(format!("lc-originals-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&d); + std::fs::create_dir_all(d.join("sub")).unwrap(); + let orig = d.join("IMG_1.jpg"); + std::fs::write(&orig, b"original").unwrap(); + std::fs::write(d.join("IMG_1.xmp"), b"").unwrap(); + std::fs::write(d.join("other.jpg"), b"other").unwrap(); + let mut c = Catalog::default(); + let p = Photo::new(PhotoId(1), Source::File { path: orig.to_string_lossy().to_string() }, "IMG_1.jpg", "JPEG", 10, 10, "2026-01-01"); + c.apply(Op::AddPhoto { photo: Box::new(p) }).unwrap(); + let g = OriginalGuard::new(&c); + assert!(g.check(&orig).unwrap_err().contains("original of IMG_1.jpg")); + assert!(g.check(&d.join("sub/../IMG_1.jpg")).is_err(), "another spelling of the same file"); + assert!(g.check(&d.join("IMG_1.xmp")).unwrap_err().contains("sidecar")); + assert!(g.check(&d.join("other.jpg")).is_ok(), "an ordinary existing file"); + assert!(g.check(&d.join("new.jpg")).is_ok(), "a free name"); + let _ = std::fs::remove_dir_all(&d); + } +} diff --git a/crates/engine/src/sidecar.rs b/crates/engine/src/sidecar.rs index a49e5fee..fb25871c 100644 --- a/crates/engine/src/sidecar.rs +++ b/crates/engine/src/sidecar.rs @@ -258,13 +258,9 @@ pub fn sidecar_packet(p: &Photo, cat: &lightcraft_catalog::Catalog) -> String { lightcraft_meta::write_xmp_lc(&meta, &lc) } -/// Write `data` to `path` atomically (temp file + rename). +/// Write `data` to `path` atomically (temp file + sync + rename). fn write_atomic(path: &Path, data: &[u8]) -> std::io::Result<()> { - let tmp = path.with_extension(format!("xmp.tmp-{}", std::process::id())); - std::fs::write(&tmp, data)?; - std::fs::rename(&tmp, path).inspect_err(|_| { - let _ = std::fs::remove_file(&tmp); - }) + lightcraft_catalog::safe_file::write_atomic(path, data) } fn file_path(p: &Photo) -> Option<&str> { diff --git a/crates/engine/src/tests_export.rs b/crates/engine/src/tests_export.rs index 13c28213..dcd74cb4 100644 --- a/crates/engine/src/tests_export.rs +++ b/crates/engine/src/tests_export.rs @@ -207,3 +207,119 @@ fn export_falls_back_to_the_cpu_when_gpu_work_is_lost() { assert!(m > 0.02, "not black: mean {m}"); assert!((m - healthy).abs() < 0.01, "GPU {healthy} vs CPU {m}"); } + +/// A JPEG original `dir/IMG_1.jpg` imported into a library on disk. +fn library_with_jpeg(tag: &str) -> (Session, lightcraft_catalog::PhotoId, std::path::PathBuf, std::path::PathBuf, Vec) { + let dir = temp_dir(tag); + let src = dir.join("IMG_1.jpg"); + let img = lightcraft_raster::Rgba8::from_fn(48, 32, |x, y| [(x * 5) as u8, (y * 7) as u8, 120, 255]); + let bytes = crate::export::encode_image(&img, &ExportOptions::default()).unwrap(); + std::fs::write(&src, &bytes).unwrap(); + let mut s = Session::new().with_fs(); + s.execute("library.import", &json!({"paths": [src.to_string_lossy()]})).unwrap(); + let id = s.catalog.photos().next().unwrap().id; + s.execute("develop.set", &json!({"ids": [id.0], "control": "light.exposure", "value": 1.0})).unwrap(); + (s, id, dir, src, bytes) +} + +fn disk_batch( + s: &mut Session, + id: lightcraft_catalog::PhotoId, + o: &ExportOptions, + to: &crate::export::Destination, +) -> Result, String> { + crate::export::export_batch(s, &[id], o, to, &mut crate::export::write_file, &|p| std::path::Path::new(p).exists()) +} + +/// Issue #93: exporting into the photo's own folder as `{name}` with "Overwrite" replaced the +/// original with the re-encoded render. It is refused now, whatever the policy or exact path. +#[test] +fn export_never_overwrites_an_original() { + use crate::export::{Conflict, Destination}; + let (mut s, id, dir, src, original) = library_with_jpeg("export-guard"); + let folder = Destination { dir: dir.to_string_lossy().to_string(), exact: None }; + let o = ExportOptions { naming: "{name}".into(), conflict: Conflict::Overwrite, ..Default::default() }; + let err = disk_batch(&mut s, id, &o, &folder).unwrap_err(); + assert!(err.contains("original of IMG_1.jpg"), "{err}"); + assert_eq!(std::fs::read(&src).unwrap(), original, "the original is byte-identical"); + + // the exact path of the control channel / MCP, also spelled another way + std::fs::create_dir_all(dir.join("sub")).unwrap(); + for exact in [src.to_string_lossy().to_string(), dir.join("sub/../IMG_1.jpg").to_string_lossy().to_string()] { + let to = Destination { dir: String::new(), exact: Some(exact) }; + assert!(disk_batch(&mut s, id, &o, &to).unwrap_err().contains("never writes over an original")); + } + // the "Original" format re-writing the file onto itself (and its sidecar) + let orig = ExportOptions { format: ExportFormat::Original, ..o.clone() }; + assert!(disk_batch(&mut s, id, &orig, &folder).is_err()); + assert_eq!(std::fs::read(&src).unwrap(), original); + assert!(s.execute("export.checkTarget", &json!({"path": src.to_string_lossy()})).is_err()); + assert!(s.execute("export.checkTarget", &json!({"path": dir.join("free.jpg").to_string_lossy()})).is_ok()); + + // Unique moves past it; Overwrite still replaces an ordinary earlier export + let unique = ExportOptions { conflict: Conflict::Unique, ..o.clone() }; + let files = disk_batch(&mut s, id, &unique, &folder).unwrap(); + assert!(files[0]["path"].as_str().unwrap().ends_with("IMG_1-2.jpg"), "{files:?}"); + let earlier = dir.join("IMG_1-2.jpg"); + std::fs::write(&earlier, b"an earlier export").unwrap(); + let named = ExportOptions { naming: "{name}-2".into(), ..o }; + disk_batch(&mut s, id, &named, &folder).unwrap(); + assert_ne!(std::fs::read(&earlier).unwrap(), b"an earlier export", "an ordinary file is overwritten"); + assert_eq!(std::fs::read(&src).unwrap(), original); + let _ = std::fs::remove_dir_all(&dir); +} + +/// Issue #93: the XMP sidecar of an "Original" export ignored the conflict policy. +#[test] +fn export_sidecars_follow_the_conflict_policy() { + use crate::export::{Conflict, Destination}; + let (mut s, id, dir, _src, original) = library_with_jpeg("export-sidecar"); + let out = dir.join("out"); + std::fs::create_dir_all(&out).unwrap(); + let theirs = out.join("IMG_1.xmp"); + std::fs::write(&theirs, b"someone else's sidecar").unwrap(); + let to = Destination { dir: out.to_string_lossy().to_string(), exact: None }; + let o = ExportOptions { format: ExportFormat::Original, naming: "{name}".into(), ..Default::default() }; + + // Unique: the photo and its sidecar both move to the next free name + let files = disk_batch(&mut s, id, &ExportOptions { conflict: Conflict::Unique, ..o.clone() }, &to).unwrap(); + assert!(files[0]["path"].as_str().unwrap().ends_with("IMG_1-2.jpg"), "{files:?}"); + assert!(files[0]["sidecars"][0].as_str().unwrap().ends_with("IMG_1-2.xmp"), "{files:?}"); + assert_eq!(std::fs::read(&theirs).unwrap(), b"someone else's sidecar"); + assert_eq!(std::fs::read(out.join("IMG_1-2.jpg")).unwrap(), original); + // Skip: a taken sidecar name skips the photo + std::fs::remove_file(out.join("IMG_1-2.jpg")).unwrap(); + std::fs::remove_file(out.join("IMG_1-2.xmp")).unwrap(); + let files = disk_batch(&mut s, id, &ExportOptions { conflict: Conflict::Skip, ..o.clone() }, &to).unwrap(); + assert!(files[0]["skipped"].as_str().is_some(), "{files:?}"); + assert!(!out.join("IMG_1.jpg").exists()); + assert_eq!(std::fs::read(&theirs).unwrap(), b"someone else's sidecar"); + // Overwrite: replaced, as asked + disk_batch(&mut s, id, &ExportOptions { conflict: Conflict::Overwrite, ..o }, &to).unwrap(); + assert!(std::fs::read_to_string(&theirs).unwrap().contains("xmpmeta")); + let _ = std::fs::remove_dir_all(&dir); +} + +/// Issue #93: the native writer truncated the target first; a failure mid-write (full disk, +/// unplugged drive) now leaves the previous file intact and no partial one. +#[test] +fn a_failed_export_write_keeps_the_previous_file() { + use crate::export::{Conflict, Destination}; + let (mut s, id, dir, _src, _) = library_with_jpeg("export-midwrite"); + let out = dir.join("out"); + std::fs::create_dir_all(&out).unwrap(); + let prev = out.join("IMG_1.jpg"); + std::fs::write(&prev, b"last week's export").unwrap(); + let to = Destination { dir: out.to_string_lossy().to_string(), exact: None }; + let o = ExportOptions { conflict: Conflict::Overwrite, ..Default::default() }; + { + let _fault = lightcraft_catalog::safe_file::fail_writes_after(100); + let err = disk_batch(&mut s, id, &o, &to).unwrap_err(); + assert!(err.contains("injected"), "{err}"); + } + assert_eq!(std::fs::read(&prev).unwrap(), b"last week's export"); + assert_eq!(std::fs::read_dir(&out).unwrap().count(), 1, "no temp file left behind"); + disk_batch(&mut s, id, &o, &to).unwrap(); + assert_ne!(std::fs::read(&prev).unwrap(), b"last week's export"); + let _ = std::fs::remove_dir_all(&dir); +} diff --git a/crates/mcp/src/headless.rs b/crates/mcp/src/headless.rs index c5154ca4..2ae21b25 100644 --- a/crates/mcp/src/headless.rs +++ b/crates/mcp/src/headless.rs @@ -78,12 +78,7 @@ impl Headless { None => vec![self.photo_or_active(p)?], }; let dir = p.get("dir").and_then(Value::as_str).unwrap_or(""); - let write = &mut |path: &str, bytes: &[u8]| { - if let Some(parent) = Path::new(path).parent().filter(|d| !d.as_os_str().is_empty()) { - std::fs::create_dir_all(parent).map_err(|err| err.to_string())?; - } - std::fs::write(path, bytes).map_err(|err| format!("{path}: {err}")) - }; + let write = &mut lightcraft_engine::export::write_file; let files = export_batch(&mut self.session, &ids, &opts, &Destination { dir: dir.to_string(), exact: exact.map(str::to_string) }, write, &|path| { Path::new(path).exists() @@ -118,6 +113,7 @@ impl Backend for Headless { let img = self.render(&p, 1600)?; match p.get("path").and_then(Value::as_str) { Some(path) => { + self.session.check_write_target(path)?; write_image(Path::new(path), &img, 92)?; Ok(json!({"path": path, "width": img.width, "height": img.height})) } @@ -183,7 +179,7 @@ pub fn encode_image(ext: &str, img: &Rgba8, quality: u8) -> Result, Stri pub fn write_image(path: &Path, img: &Rgba8, quality: u8) -> Result<(), String> { let ext = path.extension().map(|e| e.to_string_lossy().to_string()).unwrap_or_default(); let bytes = encode_image(&ext, img, quality)?; - std::fs::write(path, bytes).map_err(|e| format!("{}: {e}", path.display())) + lightcraft_engine::export::write_file(&path.to_string_lossy(), &bytes) } #[cfg(test)] diff --git a/crates/mcp/src/tests.rs b/crates/mcp/src/tests.rs index 517326ed..c613bf37 100644 --- a/crates/mcp/src/tests.rs +++ b/crates/mcp/src/tests.rs @@ -1,6 +1,6 @@ use serde_json::{Value, json}; -use crate::{Headless, PROTOCOL_VERSION, Server, call_tool, command_tool_name}; +use crate::{Backend, Headless, PROTOCOL_VERSION, Server, call_tool, command_tool_name}; fn server() -> Server { Server::new(Box::new(Headless::demo())) @@ -92,6 +92,34 @@ fn import_tool_copies_with_a_folder_template() { let _ = std::fs::remove_dir_all(&base); } +/// Issue #93: `render_photo`, `ui.render` and `app.export` with an exact `path` must not replace +/// a photo's original. +#[test] +fn path_writes_never_replace_an_original() { + let base = std::env::temp_dir().join(format!("lc-mcp-guard-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&base); + std::fs::create_dir_all(&base).unwrap(); + let img = lightcraft_raster::Rgba8 { width: 8, height: 8, data: vec![[90, 30, 9, 255]; 64] }; + let png = lightcraft_codecs::encode_png(&lightcraft_codecs::EncodeImage::rgba8(&img), &lightcraft_codecs::EncodeMeta::default()).unwrap(); + let orig = base.join("a.png"); + std::fs::write(&orig, &png).unwrap(); + let mut b = Headless::demo(); + let r = call_tool(&mut b, "import", &json!({"paths": [orig.to_string_lossy()]})); + assert!(!r.is_error, "{r:?}"); + let id = b.session.catalog.photos().find(|p| p.file_name == "a.png").unwrap().id.0; + let path = orig.to_string_lossy().to_string(); + let r = call_tool(&mut b, "render_photo", &json!({"id": id, "path": path})); + assert!(r.is_error, "{r:?}"); + assert!(b.call("ui.render", json!({"id": id, "path": path})).unwrap_err().contains("original")); + let e = b.call("app.export", json!({"ids": [id], "path": path})).unwrap_err(); + assert!(e.contains("never writes over an original"), "{e}"); + assert_eq!(std::fs::read(&orig).unwrap(), png, "the original is untouched"); + // another path still works + let r = call_tool(&mut b, "render_photo", &json!({"id": id, "path": base.join("render.png").to_string_lossy()})); + assert!(!r.is_error, "{r:?}"); + let _ = std::fs::remove_dir_all(&base); +} + /// The `import` helper moves: renamed into the folder template, the source removed. #[test] fn import_tool_moves_with_a_folder_template() { diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index 7e2e212b..cc977556 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -352,9 +352,9 @@ fn image_result(file: &std::path::Path, max: Option, format: &str, save_to: (bytes, w, h) }; if let Some(p) = save_to - && let Err(e) = std::fs::write(p, &bytes) + && let Err(e) = lightcraft_engine::export::write_file(p, &bytes) { - return ToolResult::error(format!("{p}: {e}")); + return ToolResult::error(e); } let mime = if jpeg { "image/jpeg" } else { "image/png" }; let mut info = meta; @@ -372,7 +372,18 @@ fn image_result(file: &std::path::Path, max: Option, format: &str, save_to: } } +/// A user-given `path` to save to must not be a photo's original (or its sidecar). +fn check_save_path(b: &mut dyn Backend, args: &Value) -> Result<(), String> { + match args.get("path").and_then(Value::as_str) { + Some(p) => exec(b, "export.checkTarget", json!({"path": p})).map(|_| ()), + None => Ok(()), + } +} + fn render_photo(b: &mut dyn Backend, args: &Value) -> ToolResult { + if let Err(e) = check_save_path(b, args) { + return ToolResult::error(e); + } let size = args.get("size").and_then(Value::as_u64).unwrap_or(1024).clamp(16, 4096); let id = match args.get("id").and_then(Value::as_u64) { Some(id) => Some(id), @@ -394,6 +405,9 @@ fn render_photo(b: &mut dyn Backend, args: &Value) -> ToolResult { } fn screenshot(b: &mut dyn Backend, args: &Value) -> ToolResult { + if let Err(e) = check_save_path(b, args) { + return ToolResult::error(e); + } let file = temp_path("screenshot"); let path = file.to_string_lossy().to_string(); match b.call("ui.screenshot", json!({"path": path})) { diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index e71124de..26408bc4 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -290,12 +290,19 @@ pub fn handle(app: &mut LightcraftApp, ctx: &egui::Context, req: &ControlRequest ok(Value::Null) } "ui.screenshot" => { + // never over a photo's original + if let Some(Err(e)) = s("path").map(|path| app.session.check_write_target(path)) { + return err(e); + } ctx.request_repaint(); Outcome::Screenshot { path: s("path").map(str::to_string), headless: p.get("headless").and_then(Value::as_bool).unwrap_or(false) } } "ui.render" => { let id = p.get("id").and_then(Value::as_u64).map(lightcraft_catalog::PhotoId).or(app.session.active()); let Some(id) = id else { return err("no photo") }; + if let Some(Err(e)) = s("path").map(|path| app.session.check_write_target(path)) { + return err(e); + } let size = p.get("size").and_then(Value::as_u64).unwrap_or(1600) as usize; match app.session.render_now(id, size, size) { Ok(r) => match (s("path"), app.services.png.as_ref()) { @@ -356,11 +363,7 @@ pub fn export_active(app: &mut LightcraftApp, p: &Value) -> Result, _>>()?; + let items = lightcraft_engine::export::prepare_batch(&mut app.session, &ids, &opts)?; crate::export_task::start(app, items, opts, to)? } else { let w = app.services.write.as_mut().ok_or("no writer")?; diff --git a/docs/parity.md b/docs/parity.md index b8f0e30c..717096a8 100644 --- a/docs/parity.md +++ b/docs/parity.md @@ -436,10 +436,10 @@ under the UI command that wraps it (a no-op error); the UI command now wins. | LR-EXP-METADATA | Metadata policy | P1 | ✅ | `cmd:app.export` (`metadata`, `removeLocation`) | | | LR-EXP-WATERMARK | Watermark | P1 | ✅ | `cmd:app.export` (`watermark`: text or `image` + `imageWidth`), `crates/engine/src/export.rs` (`Watermark`) | text (size, colour, shadow) or a graphic with transparency (width as % of the photo, converted to the output colour space); position, inset, opacity | | LR-EXP-NAMING | File naming | P1 | ✅ | `cmd:app.export` (`naming`, `startNumber`), `crates/engine/src/rename.rs` (`expand_tokens`) | one template language for export, Rename Photos and import renaming: `{name}`, `{num}` (number at the end of the name), `{seq}` / `{seq:N}`, `{date}` / `{date:%Y-%m-%d}`, `{folder}`, `{camera}`, `{lens}`, `{iso}`, `{rating}`, `{title}`, `{creator}`, `{ext}`; free-form template rather than a list of named schemes | -| LR-EXP-LOCATION | Destination folder | P0 | ✅ | `cmd:app.export` (`dir`, `subfolder`, `conflict`: unique / overwrite / skip), `crates/ui-egui/src/panels/dialogs.rs` (Choose… folder picker) | folder picker on the desktop; existing files get `-2`, `-3`… by default | +| LR-EXP-LOCATION | Destination folder | P0 | ✅ | `cmd:app.export` (`dir`, `subfolder`, `conflict`: unique / overwrite / skip), `cmd:export.checkTarget`, `crates/ui-egui/src/panels/dialogs.rs` (Choose… folder picker), `crates/engine/src/originals.rs`, `crates/catalog/src/safe_file.rs` | folder picker on the desktop; existing files get `-2`, `-3`… by default; the policy covers a file and its sidecars together; a catalogued original (or its sidecar) is never written over, whatever the policy or an exact `path` (export, `ui.render`, MCP `render_photo`, merge `previewPath`, `lightcraft-cli render`); files are written to a temp file, synced and renamed into place (issue #93) | | LR-EXP-PREVIOUS | Export with previous settings | P0 | ✅ | `cmd:app.exportPrevious`, `cmd:dialog.export` | last options persist in prefs.json; dialog prefilled; no named export presets yet | | LR-EXP-DNGOPT | DNG options | P2 | 🟡 | `cmd:app.export` (`dngCompression`), `crates/engine/src/export.rs` | compression: lossless JPEG (default), ZIP or none; no embedded JPEG preview size, no lossy DNG output | -| LR-EXP-ORIGINAL | Original + XMP | P1 | ✅ | `cmd:app.export` (`format: original`) | file copied byte for byte, sidecar named after the output | +| LR-EXP-ORIGINAL | Original + XMP | P1 | ✅ | `cmd:app.export` (`format: original`), `crates/engine/src/tests_export.rs` | file copied byte for byte, sidecar named after the output and subject to the conflict policy | | LR-EXP-PHOTOS | Export to the system photo library | P2 | ⬜ | | | | LR-EXP-PSD | Round trip to an external editor | P2 | ✅ | `cmd:photo.editExternal`, `cmd:photo.editInExternal`, `crates/engine/src/cmd/convert.rs` | a 16-bit TIFF `-Edit` copy with the edits (Adobe RGB / ProPhoto / P3 / sRGB) next to the original, added stacked on top of it and opened in the editor set in Settings ▸ General (or the system default) | diff --git a/docs/whats-new.md b/docs/whats-new.md index dbbadf90..42baa1bf 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -10,6 +10,13 @@ - 23 new built-in presets: Portrait, Landscape, Urban, Food, Seasons, Vintage and B&W toners. ### Reliability +- Exports and renders never write over a photo's original (issue #93): exporting into the photo's own folder with + the same name and "Overwrite" (or Export with Previous repeating it), an exact output path from the control + channel or MCP, a merge preview path or `lightcraft-cli render IMG.jpg -o IMG.jpg` is refused with a clear + message, and the original is left byte for byte. Ordinary earlier exports are still overwritten when asked. + Exported files are written to a temp file, synced and then renamed into place, so a full disk or an unplugged + drive never leaves a truncated file; the XMP sidecar of an "Original" export follows the "If file exists" + choice too. - Canon CR2 photos from the EOS 7D, 50D, 60D, 550D, 600D, 1200D, 1300D, 5D Mark II and 1D Mark IV (and other models whose sensor starts on a green-blue row) no longer come out magenta (issue #85): the colour-filter layout is read from each file instead of assumed. From 882a059fac3772d06b22ddac65df6ff3a4a23e1d Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Mon, 5 Oct 2026 17:08:18 -0400 Subject: [PATCH 5/8] Issue #92: XMP: merge into an existing sidecar instead of replacing it Saving (Cmd+S or auto-write) now merges LightCraft's properties into an existing sidecar (lightcraft_meta::merge_xmp): owned properties (rating, label, dc/IPTC/rights fields, lc:*) are replaced or removed, capture time and GPS only when LightCraft has a value; everything else (crs:, xmpMM:History, unknown namespaces, packet wrapper) is kept byte for byte. The result is re-parsed and checked before it is written (temp + fsync + rename). A sidecar that isn't valid XMP is copied to .xmp.bak-