diff --git a/crates/engine/src/cmd/browse.rs b/crates/engine/src/cmd/browse.rs index 814e1d63..a8a192f9 100644 --- a/crates/engine/src/cmd/browse.rs +++ b/crates/engine/src/cmd/browse.rs @@ -287,6 +287,9 @@ pub(crate) fn move_folder(s: &mut Session, from: &str, to: &str) -> std::result: if s.library.as_ref().is_some_and(|l| l.dir.starts_with(src) || src.starts_with(&l.dir)) { return Err("the library's own folders can't be moved here".into()); } + // Repairs retain their exact old paths, including those on surviving history entries. + // Complete them before the directory moves; an unavailable repair leaves it in place. + s.repair_sidecars_with(&crate::rename::RealFs).map_err(|e| e.to_string())?; rename_folder_on_disk(from, to)?; let mut ops: Vec = s .catalog diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 1d2ffa2a..95171979 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -110,6 +110,10 @@ pub struct UndoEntry { /// A folder to rename on disk (`from` → `to`) before `op` is applied: Rename / Move Folder /// (whose `op` relinks the photos inside). Never overwrites; refused when `to` exists. pub folder: Option, + /// Inverses of logged path corrections from a partially failed history step. + pub(crate) retry_inverse: Vec, + /// Sidecars still separated from their photo after a failed move or rollback. + pub(crate) retry_sidecars: Vec, } /// The one photo an undo step changes, when it changes exactly one through its edit, rating, flag, @@ -182,6 +186,8 @@ pub struct Session { /// its length can't tell how many steps a command just made; see [`Session::commits`]). commits: u64, pub redo: Vec, + /// Exact filesystem repairs from discarded history; catalog inverses stay discarded. + pub(crate) pending_sidecar_repairs: Vec, pub interaction: Option, /// Set by a command whose change must not rewrite the photo's XMP sidecar even with auto-write on /// (a catalog-only edit of data the sidecar writer does not emit); consumed when the command ends. @@ -315,6 +321,7 @@ impl Session { undo: Vec::new(), commits: 0, redo: Vec::new(), + pending_sidecar_repairs: Vec::new(), interaction: None, skip_auto_write: false, denoise: Default::default(), @@ -420,7 +427,7 @@ impl Session { if self.depth == 0 { let skip = std::mem::take(&mut self.skip_auto_write); if r.is_ok() && !skip && self.xmp.auto_write && self.interaction.is_none() && self.pending_log.len() > log_start { - self.auto_write_sidecars(&self.pending_log[log_start..]); + self.auto_write_sidecars_since(log_start); } } if self.depth == 0 && self.library.is_some() { @@ -449,17 +456,34 @@ impl Session { /// Apply an op as one undoable step. pub fn commit(&mut self, label: &str, op: Op) -> Result<()> { let fwd = op.clone(); - let inv = self.catalog.apply(op)?; + let inv = self.apply_catalog(op)?; self.pending_log.push(fwd); - self.undo.push(UndoEntry { label: label.to_string(), op: inv, folder: None }); + self.undo.push(UndoEntry { label: label.to_string(), op: inv, folder: None, retry_inverse: Vec::new(), retry_sidecars: Vec::new() }); self.commits = self.commits.wrapping_add(1); if self.undo.len() > 1000 { - self.undo.remove(0); + let mut dropped = self.undo.remove(0); + self.pending_sidecar_repairs.append(&mut dropped.retry_sidecars); + } + for mut dropped in self.redo.drain(..) { + self.pending_sidecar_repairs.append(&mut dropped.retry_sidecars); } - self.redo.clear(); Ok(()) } + fn apply_catalog(&mut self, op: Op) -> Result { + let inv = self.catalog.apply(op.clone())?; + let mut pending = vec![&op]; + while let Some(op) = pending.pop() { + match op { + Op::SetFile { id, .. } | Op::Relink { id, .. } | Op::SetContent { id, .. } | Op::RemovePhoto { id } => self.media.forget(*id), + Op::AddPhoto { photo } => self.media.forget(photo.id), + Op::Batch { ops } => pending.extend(ops), + _ => {} + } + } + Ok(inv) + } + /// [`Session::commit`] for an op that goes with a folder already renamed on disk; the undo /// step renames it back (`folder` is the undo direction: current place → old place). pub(crate) fn commit_with_folder(&mut self, label: &str, op: Op, folder: FolderMove) -> Result<()> { @@ -527,13 +551,20 @@ impl Session { if n < 2 || n > self.undo.len() { return; } - // a step that moves a folder on disk stays on its own - if self.undo[self.undo.len() - n..].iter().any(|e| e.folder.is_some()) { + let Some(entries) = self.undo.get(self.undo.len() - n..) else { return }; + // Folder moves and partially applied file steps stay on their own. + if entries.iter().any(|e| e.folder.is_some() || !e.retry_inverse.is_empty() || !e.retry_sidecars.is_empty()) { return; } let tail = self.undo.split_off(self.undo.len() - n); let ops = tail.into_iter().rev().map(|e| e.op).collect(); - self.undo.push(UndoEntry { label: label.to_string(), op: Op::Batch { ops }, folder: None }); + self.undo.push(UndoEntry { + label: label.to_string(), + op: Op::Batch { ops }, + folder: None, + retry_inverse: Vec::new(), + retry_sidecars: Vec::new(), + }); } /// Apply without recording undo (interactive previews). @@ -543,8 +574,12 @@ impl Session { } pub fn undo_step(&mut self) -> Result { - let e = self.undo.pop().ok_or_else(|| EngineError::Other("nothing to undo".into()))?; - let redo = match self.apply_with_files(&e.op, e.folder.as_ref()) { + self.undo_step_with(&rename::RealFs) + } + + pub(crate) fn undo_step_with(&mut self, fs: &dyn rename::MoveFs) -> Result { + let mut e = self.undo.pop().ok_or_else(|| EngineError::Other("nothing to undo".into()))?; + let redo = match self.apply_with_files(&mut e, fs) { Ok(r) => r, Err(err) => { self.undo.push(e); @@ -553,13 +588,23 @@ impl Session { }; self.show_undone(&e.op); self.pending_log.push(e.op); - self.redo.push(UndoEntry { label: e.label.clone(), op: redo, folder: e.folder.as_ref().map(FolderMove::reversed) }); + self.redo.push(UndoEntry { + label: e.label.clone(), + op: redo, + folder: e.folder.as_ref().map(FolderMove::reversed), + retry_inverse: Vec::new(), + retry_sidecars: Vec::new(), + }); Ok(e.label) } pub fn redo_step(&mut self) -> Result { - let e = self.redo.pop().ok_or_else(|| EngineError::Other("nothing to redo".into()))?; - let undo = match self.apply_with_files(&e.op, e.folder.as_ref()) { + self.redo_step_with(&rename::RealFs) + } + + pub(crate) fn redo_step_with(&mut self, fs: &dyn rename::MoveFs) -> Result { + let mut e = self.redo.pop().ok_or_else(|| EngineError::Other("nothing to redo".into()))?; + let undo = match self.apply_with_files(&mut e, fs) { Ok(r) => r, Err(err) => { self.redo.push(e); @@ -568,7 +613,13 @@ impl Session { }; self.show_undone(&e.op); self.pending_log.push(e.op); - self.undo.push(UndoEntry { label: e.label.clone(), op: undo, folder: e.folder.as_ref().map(FolderMove::reversed) }); + self.undo.push(UndoEntry { + label: e.label.clone(), + op: undo, + folder: e.folder.as_ref().map(FolderMove::reversed), + retry_inverse: Vec::new(), + retry_sidecars: Vec::new(), + }); Ok(e.label) } @@ -594,8 +645,18 @@ impl Session { /// 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; the /// folder is only moved back when no file was left behind at its new path. - fn apply_with_files(&mut self, op: &Op, folder: Option<&FolderMove>) -> Result { - let fs = rename::RealFs; + fn apply_with_files(&mut self, entry: &mut UndoEntry, fs: &dyn rename::MoveFs) -> Result { + // The entry is detached from self.undo/self.redo: include it before any global + // repair can retire, then retry revalidates it before moving its own packets. + rename::validate_sidecars(fs, &entry.retry_sidecars).map_err(|e| EngineError::Other(format!("can't move the files back: {e}")))?; + // Repair discarded file steps before another path change can stale their exact paths. + // Unrelated metadata history must remain usable while a sidecar is still unwritable. + if entry.folder.is_some() || !entry.retry_sidecars.is_empty() || !self.file_moves(&entry.op).is_empty() { + self.repair_sidecars_with(fs).map_err(|e| EngineError::Other(format!("can't move the files back: {e}")))?; + } + rename::retry_sidecars(fs, &mut entry.retry_sidecars).map_err(|e| EngineError::Other(format!("can't move the files back: {e}")))?; + let op = &entry.op; + let folder = entry.folder.as_ref(); if let Some(f) = folder { cmd::browse::rename_folder_on_disk(&f.from, &f.to).map_err(|e| EngineError::Other(format!("can't move the folder back: {e}")))?; } @@ -605,26 +666,42 @@ impl Session { } }; let moves = self.file_moves(op); - if let Err(e) = rename::move_all(&fs, &moves) { + if let Err(mut e) = rename::move_all(fs, &moves) { + entry.retry_sidecars.append(&mut e.pending_sidecars); if e.stuck.is_empty() { undo_folder(); } - return Err(EngineError::Other(format!("can't move the files back: {}", self.follow_stuck(op, e, false)))); + let (message, mut inverses) = self.follow_stuck(op, e, false); + // A later successful retry observes the corrected paths. Retain their old paths + // now, so the opposite history direction still restores the entire original step. + inverses.append(&mut entry.retry_inverse); + entry.retry_inverse = inverses; + return Err(EngineError::Other(format!("can't move the files back: {message}"))); } - match self.catalog.apply(op.clone()) { + match self.apply_catalog(op.clone()) { Ok(inv) => { if let Some(f) = folder { cmd::browse::follow_folder(self, &f.from, &f.to); } - Ok(inv) + if entry.retry_inverse.is_empty() { + Ok(inv) + } else { + let mut ops = match inv { + Op::Batch { ops } => ops, + op => vec![op], + }; + ops.append(&mut entry.retry_inverse); + Ok(Op::Batch { ops }) + } } Err(e) => { let back: Vec<(String, String)> = moves.iter().rev().map(|(a, b)| (b.clone(), a.clone())).collect(); - if let Err(be) = rename::move_all(&fs, &back) { + if let Err(mut be) = rename::move_all(fs, &back) { + entry.retry_sidecars.append(&mut be.pending_sidecars); return Err(EngineError::Other(format!("{e}; the files could not all be moved back: {}", be.message))); } undo_folder(); - Err(e.into()) + Err(e) } } } @@ -893,6 +970,8 @@ mod tests_face_models; #[cfg(not(target_arch = "wasm32"))] mod tests_face_recognize; #[cfg(test)] +mod tests_filesystem_history; +#[cfg(test)] mod tests_folders; #[cfg(test)] mod tests_forget_local; diff --git a/crates/engine/src/library.rs b/crates/engine/src/library.rs index e0110b35..85c988ea 100644 --- a/crates/engine/src/library.rs +++ b/crates/engine/src/library.rs @@ -320,6 +320,9 @@ impl Session { catalog.revision = LOADS.fetch_add(1, std::sync::atomic::Ordering::Relaxed).wrapping_add(1) << 32; self.catalog = catalog; self.library_generation = crate::LIBRARY_GEN.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + for entry in self.undo.iter_mut().chain(self.redo.iter_mut()) { + self.pending_sidecar_repairs.append(&mut entry.retry_sidecars); + } self.undo.clear(); self.redo.clear(); self.interaction = None; diff --git a/crates/engine/src/rename.rs b/crates/engine/src/rename.rs index 259d51fc..7b6c825a 100644 --- a/crates/engine/src/rename.rs +++ b/crates/engine/src/rename.rs @@ -319,6 +319,27 @@ pub(crate) trait MoveFs { /// Another photo file beside the stem-named sidecar `sidecar` that shares its stem (e.g. /// `IMG_1.JPG` for `IMG_1.xmp`), which still uses that sidecar. fn stem_sibling(&self, sidecar: &Path) -> Option; + /// Content observed in a packet, independent of its pathname or inode. Model volumes + /// override this; disk implementations stream a bounded buffer without retaining the XML. + fn packet_version(&self, path: &Path) -> std::io::Result { + use std::io::Read; + let mut file = std::fs::File::open(path)?; + let mut hash = lightcraft_preview::Hasher128::new(); + let mut len = 0u64; + let mut buffer = [0u8; 16 * 1024]; + loop { + let n = match file.read(&mut buffer) { + Err(e) if e.kind() == std::io::ErrorKind::Interrupted => continue, + result => result?, + }; + if n == 0 { + return Ok(PacketVersion { len, hash: hash.finish() }); + } + let bytes = buffer.get(..n).ok_or_else(|| std::io::Error::other("invalid sidecar read length"))?; + len = len.checked_add(n as u64).ok_or_else(|| std::io::Error::other("sidecar is too large"))?; + hash.update(bytes); + } + } } /// The real file system. @@ -420,6 +441,178 @@ fn move_one(fs: &dyn MoveFs, a: &Path, b: &Path) -> std::io::Result<()> { }) } +/// An unfinished sidecar operation. Keep its exact paths and any recorded copy requirement: the photo may +/// already have followed its new path, so a retry cannot infer this work from the catalog. +#[derive(Clone, Debug, PartialEq)] +pub(crate) struct SidecarMove { + pub from: PathBuf, + pub to: PathBuf, + copied: bool, + /// Observed packets in the partial move, including the separated source. Absent planned + /// destinations are never owned; a completed repair advances these observations. + companions: Vec, +} + +/// A content version, not an assertion that a pathname still names the same packet. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) struct PacketVersion { + len: u64, + hash: lightcraft_preview::Hash128, +} + +impl PacketVersion { + fn of(bytes: &[u8]) -> Self { + Self { len: bytes.len() as u64, hash: lightcraft_preview::hash_bytes(bytes) } + } +} + +#[derive(Clone, Debug, PartialEq)] +struct RetainedPacket { + path: PathBuf, + // An unreadable observation remains untrusted even if a later read succeeds. + version: Option, +} + +impl RetainedPacket { + fn validate(&self, fs: &dyn MoveFs) -> std::result::Result<(), String> { + if self.version.is_none() { + return Err(format!("can't verify retained XMP {}: its content was unreadable when the partial move was recorded", self.path.display())); + } + let current = fs.packet_version(&self.path).map_err(|e| format!("can't verify retained XMP {}: {e}", self.path.display()))?; + if self.version != Some(current) { + return Err(format!( + "retained XMP {} changed since the partial move; packet ownership must be resolved before retrying", + self.path.display() + )); + } + Ok(()) + } +} + +/// Advance only packets whose operation actually succeeded. A copy retains its original; +/// a move retires vanished source aliases and adds the observed destination. +fn advance_packets(packets: &mut Vec, aliases: &[PathBuf], to: &Path, copied: bool, fs: &dyn MoveFs) { + let Some(mut packet) = packets.iter().find(|p| aliases.contains(&p.path)).cloned() else { return }; + if !copied { + packets.retain(|p| !aliases.contains(&p.path) || fs.is_file(&p.path)); + } + packet.path = to.to_path_buf(); + if !packets.iter().any(|p| p.path == packet.path) { + packets.push(packet); + } +} + +fn packet_aliases(packets: &[RetainedPacket], fs: &dyn MoveFs, from: &Path) -> Vec { + packets.iter().filter(|p| p.path == from || fs.same_file(&p.path, from)).map(|p| p.path.clone()).collect() +} + +impl SidecarMove { + fn validate_packets(&self, fs: &dyn MoveFs) -> std::result::Result<(), String> { + for packet in &self.companions { + packet.validate(fs)?; + } + Ok(()) + } + + /// Validate a queued retry and return the copy decision for this physical operation. + fn validate_repair(&self, fs: &dyn MoveFs) -> std::result::Result { + self.validate_packets(fs)?; + let respelling = is_respelling(fs, &self.from, &self.to); + // Batch rollback may have restored another photo using this source packet. + let copied = self.copied || (!respelling && fs.stem_sibling(&self.from).is_some()); + if fs.exists(&self.to) && (copied || !respelling) { + return Err(format!("{} already exists while repairing {} → {}", self.to.display(), self.from.display(), self.to.display())); + } + Ok(copied) + } + + fn needed(&self, fs: &dyn MoveFs) -> bool { + fs.is_file(&self.from) && !(fs.exists(&self.to) && (self.copied || !is_respelling(fs, &self.from, &self.to))) + } + + fn touches(&self, fs: &dyn MoveFs, path: &Path) -> bool { + let same_path = |endpoint: &Path| { + endpoint == path + || fs.same_file(endpoint, path) + || (endpoint.file_name() == path.file_name() + && endpoint.parent().zip(path.parent()).is_some_and(|(a, b)| a == b || fs.same_file(a, b))) + }; + same_path(&self.from) || same_path(&self.to) + } + + fn apply(&self, fs: &dyn MoveFs) -> std::io::Result<()> { + if self.copied { fs.copy_no_replace(&self.from, &self.to) } else { move_one(fs, &self.from, &self.to) } + } +} + +/// Complete known unfinished sidecars before moving any more photos. Completed operations +/// leave the queue; a failure retains that operation and the rest, without replacing a file. +pub(crate) fn retry_sidecars(fs: &dyn MoveFs, pending: &mut Vec) -> std::result::Result<(), String> { + validate_sidecars(fs, pending)?; + while !pending.is_empty() { + apply_repair(fs, pending, 0)?; + } + Ok(()) +} + +/// Preflight a detached history entry before global queues are allowed to retire. The +/// caller can validate without mutating it; retry revalidates before each physical operation. +pub(crate) fn validate_sidecars(fs: &dyn MoveFs, pending: &[SidecarMove]) -> std::result::Result<(), String> { + for repair in pending { + repair.validate_repair(fs)?; + } + Ok(()) +} + +/// Finish one operation and transfer its observed content to the remaining repairs. Do not +/// infer ownership by reading an occupied destination or by retaining a planned pathname. +fn apply_repair(fs: &dyn MoveFs, pending: &mut Vec, i: usize) -> std::result::Result<(), String> { + let Some(mut repair) = pending.get(i).cloned() else { return Ok(()) }; + repair.copied = repair.validate_repair(fs)?; + let aliases = packet_aliases(&repair.companions, fs, &repair.from); + repair.apply(fs).map_err(|e| format!("rename {} → {}: {e}", repair.from.display(), repair.to.display()))?; + for remaining in pending.iter_mut() { + advance_packets(&mut remaining.companions, &aliases, &repair.to, repair.copied, fs); + } + pending.remove(i); + Ok(()) +} + +/// Repair only operations that affect a sidecar being saved. Unrelated unavailable originals +/// must not block healthy metadata saves. Completed repairs retire; failures stay queued. +pub(crate) fn retry_sidecars_for_paths(fs: &dyn MoveFs, pending: &mut Vec, paths: &[PathBuf]) -> std::result::Result<(), String> { + for repair in pending.iter().filter(|m| paths.iter().any(|path| m.touches(fs, path))) { + repair.validate_repair(fs)?; + } + let mut i = 0; + while let Some(m) = pending.get(i) { + if paths.iter().any(|path| m.touches(fs, path)) { + // Path/alias equality only locates an observation; validation above proves its + // content. A newly occupied, previously absent peer destination is a collision. + if let Some(path) = paths.iter().find(|path| { + fs.exists(path) + && !fs.same_file(path, &m.from) + && !fs.same_file(path, &m.to) + && !m.companions.iter().any(|known| fs.same_file(path, &known.path)) + }) { + return Err(format!("{} already exists while repairing {} → {}", path.display(), m.from.display(), m.to.display())); + } + apply_repair(fs, pending, i)?; + } else { + i += 1; + } + } + Ok(()) +} + +/// Every record came from an actual packet observed before this move. Successful physical +/// operations have already advanced its path; missing planned destinations are not recorded. +fn note_companions(pending: &mut [SidecarMove], packets: Vec) { + for repair in pending { + repair.companions.clone_from(&packets); + } +} + /// Why [`move_file`] failed, and what it left behind. #[derive(Clone, Debug, PartialEq)] pub struct MoveError { @@ -428,6 +621,7 @@ pub struct MoveError { /// 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, + pub(crate) pending_sidecars: Vec, } impl std::fmt::Display for MoveError { @@ -447,53 +641,82 @@ pub(crate) fn move_file_with(fs: &dyn MoveFs, from: &str, to: &str) -> std::resu if Path::new(from) == Path::new(to) { return Ok(()); } - let fail = |message: String| MoveError { message, moved: false }; + let fail = |message: String| MoveError { message, moved: false, pending_sidecars: Vec::new() }; 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(fail(format!("{to} already exists"))); } + let mut packets: Vec = sidecars(from) + .into_iter() + .map(|(p, _, _)| p) + .filter(|p| fs.is_file(p)) + .map(|path| RetainedPacket { version: fs.packet_version(&path).ok(), path }) + .collect(); move_one(fs, f, t).map_err(|e| fail(format!("rename {from}: {e}")))?; - // sidecars moved (or copied: `true`) - let mut done: Vec<(PathBuf, PathBuf, bool)> = Vec::new(); - for (sc, naming, upper) in sidecars(from) { - if !fs.is_file(&sc) { - continue; - } + let mut done: Vec = Vec::new(); + let mut remaining = sidecars(from).into_iter().filter(|(sc, _, _)| fs.is_file(sc)).map(|(sc, naming, upper)| { let mut dst = crate::sidecar::sidecar_path(to, naming); if upper { dst = dst.with_extension("XMP"); } - // a stem sidecar another file still uses (`IMG_1.xmp` of `IMG_1.CR3` and `IMG_1.JPG`) - // is copied, not taken away from it (issue #92) - let shared = naming == SidecarNaming::Stem && fs.stem_sibling(&sc).is_some(); - if fs.exists(&dst) && (shared || !is_respelling(fs, &sc, &dst)) { + // A stem sidecar another photo still uses is copied, not taken away (issue #92). + let copied = naming == SidecarNaming::Stem && fs.stem_sibling(&sc).is_some(); + SidecarMove { from: sc, to: dst, copied, companions: Vec::new() } + }); + while let Some(m) = remaining.next() { + if !m.needed(fs) { continue; // never overwrite: the old sidecar stays where it was } - let r = if shared { fs.copy_no_replace(&sc, &dst) } else { move_one(fs, &sc, &dst) }; - if let Err(e) = r { - let mut message = format!("rename {}: {e}", sc.display()); - // the file first: if it can't go back, the sidecars that moved stay with it + let aliases = packet_aliases(&packets, fs, &m.from); + if let Err(e) = m.apply(fs) { + let mut message = format!("rename {}: {e}", m.from.display()); + // The file first: if it can't go back, 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(); + let names: Vec = done.iter().map(|m| m.to.display().to_string()).collect(); message.push_str(&format!(" with its sidecar {}", names.join(", "))); } - return Err(MoveError { message, moved: true }); + let mut pending_sidecars = vec![m]; + for m in remaining.filter(|m| m.needed(fs)) { + // The two case spellings can name one file. Keep one operation for + // those aliases, while retaining distinct sidecars on sensitive volumes. + let alias = pending_sidecars.iter().any(|p| { + folded_path(&p.from.to_string_lossy()) == folded_path(&m.from.to_string_lossy()) + && folded_path(&p.to.to_string_lossy()) == folded_path(&m.to.to_string_lossy()) + && fs.same_file(&p.from, &m.from) + }); + if !alias { + pending_sidecars.push(m); + } + } + note_companions(&mut pending_sidecars, packets); + return Err(MoveError { message, moved: true, pending_sidecars }); } - for (a, b, copied) in done.iter().rev() { - if *copied { - // the original is still there: the copy just goes (a leftover copy loses nothing) - let _ = fs.remove_file(b); - } else 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())); + let mut pending_sidecars = Vec::new(); + for m in done.iter().rev() { + if m.copied { + // The original is still there: a leftover copy loses nothing. + if fs.remove_file(&m.to).is_ok() { + packets.retain(|p| p.path != m.to); + } + } else { + let aliases = packet_aliases(&packets, fs, &m.to); + if let Err(e) = move_one(fs, &m.to, &m.from) { + message.push_str(&format!("; sidecar {} could not be moved back ({e}): it is now {}", m.from.display(), m.to.display())); + pending_sidecars.push(SidecarMove { from: m.to.clone(), to: m.from.clone(), copied: false, companions: Vec::new() }); + } else { + advance_packets(&mut packets, &aliases, &m.from, false, fs); + } } } - return Err(fail(message)); + note_companions(&mut pending_sidecars, packets); + return Err(MoveError { message, moved: false, pending_sidecars }); } - done.push((sc, dst, shared)); + advance_packets(&mut packets, &aliases, &m.to, m.copied, fs); + done.push(m); } Ok(()) } @@ -505,6 +728,8 @@ pub(crate) struct BatchMoveError { pub message: String, /// Moves that stayed done, (from, to): those files are at `to` now. pub stuck: Vec<(String, String)>, + /// Sidecars still separated from the path where their photo ended up, in either direction. + pub pending_sidecars: Vec, } /// Move the files `moves` names, all or nothing: after a failure the earlier moves are undone. @@ -517,30 +742,41 @@ pub(crate) fn move_all(fs: &dyn MoveFs, moves: &[(String, String)]) -> std::resu continue; }; let mut message = e.message; + let mut pending_sidecars = e.pending_sidecars; 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) { + if let Err(mut 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()); } + pending_sidecars.append(&mut e.pending_sidecars); } } - return Err(BatchMoveError { message, stuck }); + return Err(BatchMoveError { message, stuck, pending_sidecars }); } Ok(()) } -/// The `SetFile` ops in `op` (nested batches included) that point a photo at one of `to`. +/// Final `SetFile` per photo (nested batches included), filtered by destination only afterward. 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()), - _ => {} + let mut pending = vec![op]; + let mut order = Vec::new(); + let mut final_ops = HashMap::new(); + while let Some(op) = pending.pop() { + match op { + Op::Batch { ops } => pending.extend(ops.iter().rev()), + Op::SetFile { id, .. } if final_ops.insert(*id, op).is_none() => order.push(*id), + _ => {} + } } + out.extend(order.into_iter().filter_map(|id| { + let op = final_ops.get(&id).copied()?; + matches!(op, Op::SetFile { source: Source::File { path }, .. } if to.contains(path.as_str())).then(|| op.clone()) + })); } /// The photos among `ids` a rename handles, in order: photos sharing one file (virtual copies) @@ -650,8 +886,62 @@ impl Session { self.apply_rename_with(&RealFs, plans) } + /// Complete retained repairs before a file or folder change can stale their exact paths. + pub(crate) fn repair_sidecars_with(&mut self, fs: &dyn MoveFs) -> Result<()> { + for repair in self.pending_sidecar_repairs.iter().chain(self.undo.iter().chain(&self.redo).flat_map(|e| &e.retry_sidecars)) { + repair.validate_repair(fs).map_err(EngineError::Other)?; + } + retry_sidecars(fs, &mut self.pending_sidecar_repairs).map_err(EngineError::Other)?; + for entry in self.undo.iter_mut().chain(self.redo.iter_mut()) { + retry_sidecars(fs, &mut entry.retry_sidecars).map_err(EngineError::Other)?; + } + Ok(()) + } + + pub(crate) fn repair_sidecars_for_paths(&mut self, paths: &[PathBuf], fs: &dyn MoveFs) -> Result<()> { + self.validate_sidecar_packets_for_paths(paths, fs)?; + retry_sidecars_for_paths(fs, &mut self.pending_sidecar_repairs, paths).map_err(EngineError::Other)?; + for entry in self.undo.iter_mut().chain(self.redo.iter_mut()) { + retry_sidecars_for_paths(fs, &mut entry.retry_sidecars, paths).map_err(EngineError::Other)?; + } + Ok(()) + } + + /// A healthy shared packet may be saved while another photo's repair is unavailable. + /// Check its retained content too, without forcing that unrelated physical operation. + pub(crate) fn validate_sidecar_packets_for_paths(&self, paths: &[PathBuf], fs: &dyn MoveFs) -> Result<()> { + for repair in self.pending_sidecar_repairs.iter().chain(self.undo.iter().chain(&self.redo).flat_map(|e| &e.retry_sidecars)) { + if paths.iter().any(|path| repair.touches(fs, path)) { + repair.validate_repair(fs).map_err(EngineError::Other)?; + } else { + for packet in &repair.companions { + if paths.iter().any(|path| fs.same_file(path, &packet.path)) { + packet.validate(fs).map_err(EngineError::Other)?; + } + } + } + } + Ok(()) + } + + /// Only a successful application write updates a retained content version. Atomic writes + /// replace inodes; aliases that now resolve to the written path observe the new content. + pub(crate) fn note_sidecar_write(&mut self, path: &Path, bytes: &[u8]) { + let version = PacketVersion::of(bytes); + for repair in self.pending_sidecar_repairs.iter_mut().chain(self.undo.iter_mut().chain(&mut self.redo).flat_map(|e| &mut e.retry_sidecars)) { + for packet in &mut repair.companions { + if RealFs.same_file(path, &packet.path) { + packet.version = Some(version); + } + } + } + } + /// [`Session::apply_rename`] on the file system `fs`. pub(crate) fn apply_rename_with(&mut self, fs: &dyn MoveFs, plans: &[RenamePlan]) -> Result { + // A partly completed rename can already have the requested photo name. Repair + // its sidecars before treating that empty move list as a successful retry. + self.repair_sidecars_with(fs)?; 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() { @@ -679,7 +969,7 @@ impl Session { }) .collect(); if let Err(e) = move_all(fs, &moves) { - return Err(EngineError::Other(self.follow_stuck(&op, e, true))); + return Err(EngineError::Other(self.follow_stuck(&op, e, true).0)); } if n > 0 && let Err(err) = self.commit(&format!("Rename {n} Photo{}", if n == 1 { "" } else { "s" }), op) @@ -695,11 +985,13 @@ impl Session { /// 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 { + /// change), so the library matches the disk. Returns the message and, for a history retry, + /// the correction's inverse file ops (so a retry doesn't lose the original paths). + pub(crate) fn follow_stuck(&mut self, op: &Op, e: BatchMoveError, undoable: bool) -> (String, Vec) { let mut msg = e.message; - if e.stuck.is_empty() { - return msg; + let pending_sidecars = e.pending_sidecars; + if e.stuck.is_empty() && (!undoable || pending_sidecars.is_empty()) { + return (msg, Vec::new()); } let to: HashSet<&str> = e.stuck.iter().map(|m| m.1.as_str()).collect(); let mut keep = Vec::new(); @@ -708,53 +1000,69 @@ impl Session { let n = e.stuck.len(); let s = if n == 1 { "" } else { "s" }; let fix = Op::Batch { ops: keep }; + let mut inverses = Vec::new(); let applied = if undoable { - self.commit(&format!("Rename {n} Photo{s} (partly)"), fix) + let label = if n == 0 { "Repair Rename Sidecars".to_string() } else { format!("Rename {n} Photo{s} (partly)") }; + self.commit(&label, fix).map(|()| { + if let Some(entry) = self.undo.last_mut() { + entry.retry_sidecars.extend(pending_sidecars); + } + }) } else { - match self.catalog.apply(fix.clone()) { - Ok(_) => { + match self.apply_catalog(fix.clone()) { + Ok(inv) => { + inverses = match inv { + Op::Batch { ops } => ops, + op => vec![op], + }; self.pending_log.push(fix); Ok(()) } - Err(e) => Err(e.into()), + Err(e) => Err(e), } }; match applied { - Ok(()) => msg + Ok(()) if n > 0 => msg .push_str(&format!(". {n} file{s} could not be moved back and kept the new name; the library now points there: {}", list.join(", "))), + Ok(()) => {} 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 + (msg, inverses) } - /// File moves an undo/redo op implies: `SetFile` ops whose source path differs from the photo's - /// current one. + /// Physical moves to the final `SetFile` destination of each photo. A merged history + /// batch can rename the same photo repeatedly; intermediate catalog paths need no file. pub(crate) fn file_moves(&self, op: &Op) -> Vec<(String, String)> { - fn rec(s: &Session, op: &Op, v: &mut Vec<(String, String)>) { + let mut pending = vec![op]; + let mut order = Vec::new(); + let mut targets = HashMap::new(); + while let Some(op) = pending.pop() { match op { - Op::Batch { ops } => ops.iter().for_each(|o| rec(s, o, v)), - Op::SetFile { id, source: Source::File { path: to }, .. } => { - if let Some(Source::File { path: from }) = s.catalog.photo(*id).map(|p| &p.source) - && Path::new(from) != Path::new(to) - && !v.iter().any(|(f, _)| f == from) - { - v.push((from.clone(), to.clone())); - } + Op::Batch { ops } => pending.extend(ops.iter().rev()), + Op::SetFile { id, source, .. } if targets.insert(*id, source).is_none() => { + order.push(*id); } _ => {} } } - let mut v = Vec::new(); - rec(self, op, &mut v); - v + // Virtual copies share one physical move, keeping the original batch order. + let mut seen = HashSet::new(); + order + .into_iter() + .filter_map(|id| { + let Source::File { path: to } = targets.get(&id).copied()? else { return None }; + let Source::File { path: from } = &self.catalog.photo(id)?.source else { return None }; + (Path::new(from) != Path::new(to) && seen.insert(from)).then(|| (from.clone(), to.clone())) + }) + .collect() } } #[cfg(test)] -mod tests { +pub(crate) mod tests { use super::*; use std::cell::RefCell; @@ -791,6 +1099,10 @@ mod tests { } impl MoveFs for FakeFs { + fn packet_version(&self, path: &Path) -> std::io::Result { + let i = self.idx(path).ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, "no such packet"))?; + self.files.borrow().get(i).map(|(_, bytes)| PacketVersion::of(bytes)).ok_or_else(|| std::io::Error::other("packet disappeared")) + } fn exists(&self, p: &Path) -> bool { self.idx(p).is_some() } @@ -812,6 +1124,9 @@ mod tests { Ok(()) } fn copy_no_replace(&self, a: &Path, b: &Path) -> std::io::Result<()> { + if self.fail.borrow().iter().any(|f| Self::norm(a).contains(f.as_str())) { + return Err(std::io::Error::other("injected failure")); + } if self.exists(b) { return Err(std::io::Error::new(std::io::ErrorKind::AlreadyExists, "exists")); } @@ -841,6 +1156,101 @@ mod tests { Photo::new(PhotoId(id), Source::File { path }, &name, "JPG", 1, 1, "2026-01-01T00:00:00") } + /// Compare the unfinished operation and every observed packet after partial progress. + /// Companion order is incidental; paths and recorded content versions are exact. + pub(crate) fn assert_repair_packets(actual: &SidecarMove, unfinished: &SidecarMove, packets: &[(PathBuf, &[u8])]) { + let mut expected = unfinished.clone(); + expected.companions = + packets.iter().map(|(path, bytes)| RetainedPacket { path: path.clone(), version: Some(PacketVersion::of(bytes)) }).collect(); + let mut actual = actual.clone(); + actual.companions.sort_by(|a, b| a.path.cmp(&b.path)); + expected.companions.sort_by(|a, b| a.path.cmp(&b.path)); + assert_eq!(actual, expected); + } + + #[test] + fn folder_moves_repair_retained_sidecars_before_changing_paths() { + for command in ["folder.rename", "folder.move"] { + for queue in ["pending", "undo", "redo"] { + for (naming, copied) in [(SidecarNaming::Stem, false), (SidecarNaming::Full, false), (SidecarNaming::Stem, true)] { + let root = crate::tests_xmp::temp_dir("folder-sidecar-repair"); + let from = root.join("Trip"); + std::fs::create_dir_all(&from).unwrap(); + let photo = from.join("b.jpg"); + std::fs::write(&photo, b"Photo bytes").unwrap(); + let old_sidecar = crate::sidecar::sidecar_path(&from.join("a.jpg").to_string_lossy(), naming); + let sidecar = crate::sidecar::sidecar_path(&photo.to_string_lossy(), naming); + std::fs::write(&old_sidecar, b"Retained edits").unwrap(); + // An ordinary occupied destination must block repair without replacing its bytes. + std::fs::write(&sidecar, b"Other edits").unwrap(); + let mut s = Session::new(); + s.catalog.apply(Op::AddPhoto { photo: Box::new(file_photo(1, &photo.to_string_lossy())) }).unwrap(); + if copied { + let sibling = from.join("a.raw"); + std::fs::write(&sibling, b"Shared original").unwrap(); + s.catalog.apply(Op::AddPhoto { photo: Box::new(file_photo(2, &sibling.to_string_lossy())) }).unwrap(); + } + s.commit("Rating", Op::SetRating { id: PhotoId(1), rating: 3 }).unwrap(); + if queue == "redo" { + s.undo_step().unwrap(); + } + let repair = SidecarMove { + from: old_sidecar.clone(), + to: sidecar.clone(), + copied, + companions: vec![RetainedPacket { path: old_sidecar.clone(), version: Some(RealFs.packet_version(&old_sidecar).unwrap()) }], + }; + match queue { + "undo" => s.undo.last_mut().unwrap().retry_sidecars.push(repair), + "redo" => s.redo.last_mut().unwrap().retry_sidecars.push(repair), + _ => s.pending_sidecar_repairs.push(repair), + } + let params = if command == "folder.rename" { + serde_json::json!({"path": from, "name": "Holiday"}) + } else { + serde_json::json!({"path": from, "into": root.join("Archive")}) + }; + let to = if command == "folder.rename" { root.join("Holiday") } else { root.join("Archive/Trip") }; + let before = s.catalog.to_snapshot(); + let history = (s.undo.len(), s.redo.len()); + assert!(s.execute(command, ¶ms).is_err()); + assert!(from.is_dir() && !to.exists()); + assert_eq!(s.catalog.to_snapshot(), before); + assert_eq!((s.undo.len(), s.redo.len()), history); + assert_eq!(std::fs::read(&old_sidecar).unwrap(), b"Retained edits"); + assert_eq!(std::fs::read(&sidecar).unwrap(), b"Other edits"); + std::fs::remove_file(&sidecar).unwrap(); + s.execute(command, ¶ms).unwrap(); + assert!(s.pending_sidecar_repairs.is_empty()); + assert!(s.undo.iter().chain(&s.redo).all(|e| e.retry_sidecars.is_empty())); + let moved_sidecar = to.join(sidecar.file_name().unwrap()); + assert_eq!(std::fs::read(&moved_sidecar).unwrap(), b"Retained edits"); + assert_eq!(to.join(old_sidecar.file_name().unwrap()).exists(), copied); + for (step, current) in [("edit.undo", &from), ("edit.redo", &to)] { + s.execute(step, &serde_json::json!({})).unwrap(); + assert_eq!( + s.catalog.photo(PhotoId(1)).unwrap().source, + Source::File { path: current.join("b.jpg").to_string_lossy().into_owned() } + ); + assert_eq!(std::fs::read(current.join(sidecar.file_name().unwrap())).unwrap(), b"Retained edits"); + assert_eq!(current.join(old_sidecar.file_name().unwrap()).exists(), copied); + } + // Explicit IDs choose targets after the command's selection gate. + s.execute("library.select", &serde_json::json!({"ids": [1], "active": 1})).unwrap(); + s.execute("photo.rename", &serde_json::json!({"ids": [1], "template": "Latest"})).unwrap(); + s.execute("edit.undo", &serde_json::json!({})).unwrap(); + assert!(to.join("b.jpg").is_file()); + assert_eq!(std::fs::read(&moved_sidecar).unwrap(), b"Retained edits"); + s.execute("edit.redo", &serde_json::json!({})).unwrap(); + assert!(to.join("Latest.jpg").is_file()); + let final_sidecar = crate::sidecar::sidecar_path(&to.join("Latest.jpg").to_string_lossy(), naming); + assert_eq!(std::fs::read(final_sidecar).unwrap(), b"Retained edits"); + std::fs::remove_dir_all(root).unwrap(); + } + } + } + } + #[cfg(windows)] #[test] fn mixed_separator_noop_rename_preserves_files_catalog_and_undo() { @@ -981,6 +1391,33 @@ mod tests { assert!(fs.listing().iter().all(|(p, _)| p.starts_with("/p/d."))); } + #[test] + fn pending_sidecars_preserve_shared_copies_and_do_not_repeat_case_aliases() { + // These are synthetic volume semantics; no host driver or OS runtime is involved. + for ci in [false, true] { + let fs = FakeFs::new(ci, &[("/p/a.CR3", "Raw"), ("/p/a.JPG", "Jpeg"), ("/p/a.XMP", "Shared edits"), ("/p/a.CR3.XMP", "Raw edits")]); + fs.fail.borrow_mut().extend(["/p/a.XMP".into(), "/p/b.CR3".into()]); + let mut error = move_file_with(&fs, "/p/a.CR3", "/p/b.CR3").unwrap_err(); + assert!(error.moved); + assert_eq!(error.pending_sidecars.len(), 2); + assert!(error.pending_sidecars[0].copied); + assert!(!error.pending_sidecars[1].copied); + fs.fail.borrow_mut().clear(); + retry_sidecars(&fs, &mut error.pending_sidecars).unwrap(); + assert!(error.pending_sidecars.is_empty()); + assert_eq!( + fs.listing(), + vec![ + ("/p/a.JPG".into(), "Jpeg".into()), + ("/p/a.XMP".into(), "Shared edits".into()), + ("/p/b.CR3".into(), "Raw".into()), + ("/p/b.CR3.XMP".into(), "Raw edits".into()), + ("/p/b.XMP".into(), "Shared edits".into()), + ] + ); + } + } + /// Issue #92 (comment): renaming one of `IMG_1.CR3` + `IMG_1.JPG` copies their shared /// `IMG_1.xmp` instead of taking it away from the other; the last one renamed takes it. #[test] diff --git a/crates/engine/src/sidecar.rs b/crates/engine/src/sidecar.rs index 06eba0b9..5257a018 100644 --- a/crates/engine/src/sidecar.rs +++ b/crates/engine/src/sidecar.rs @@ -478,20 +478,40 @@ impl Session { } /// Write the photo's XMP sidecar, merged into an existing one (see the module docs). - pub fn save_sidecar(&self, id: PhotoId) -> Result { + pub fn save_sidecar(&mut self, id: PhotoId) -> Result { self.save_sidecar_with(id, &StemOwners::of(&self.catalog)) } - pub(crate) fn save_sidecar_with(&self, id: PhotoId, owners: &StemOwners) -> Result { + pub(crate) fn save_sidecar_with(&mut self, id: PhotoId, owners: &StemOwners) -> Result { let p = self.catalog.photo(id).ok_or(lightcraft_catalog::CatalogError::NoPhoto(id))?; if p.copy_of.is_some() { return Err(EngineError::Other(format!("{} is a virtual copy: its settings live only in the library", p.file_name))); } let orig = file_path(p).ok_or_else(|| EngineError::Other(format!("{} is not a file on disk", p.file_name)))?; - let path = sidecar_path(orig, owners.naming(p, self.xmp.naming)); + let naming = owners.naming(p, self.xmp.naming); + let preferred = sidecar_path(orig, naming); + let other = if naming == SidecarNaming::Stem { SidecarNaming::Full } else { SidecarNaming::Stem }; + let candidates: Vec = [naming, other] + .into_iter() + .filter(|n| *n != SidecarNaming::Stem || owners.naming(p, SidecarNaming::Stem) == SidecarNaming::Stem) + .flat_map(|n| { + let path = sidecar_path(orig, n); + [path.clone(), path.with_extension("XMP")] + }) + .collect(); + // A separated packet is still the photo's metadata. Repair it before a fresh write + // occupies its destination; a failure preserves the queue and both existing packets. + self.repair_sidecars_for_paths(&candidates, &crate::rename::RealFs) + .map_err(|e| EngineError::Other(format!("{}: unfinished XMP repair: {e}", preferred.display())))?; + // Keep an existing packet's spelling/style so foreign properties survive this save and + // subsequent Undo/Redo writes. Shared stems owned by another photo were excluded above. + let path = candidates.into_iter().find(|path| path.is_file()).unwrap_or(preferred); + self.validate_sidecar_packets_for_paths(std::slice::from_ref(&path), &crate::rename::RealFs)?; + let p = self.catalog.photo(id).ok_or(lightcraft_catalog::CatalogError::NoPhoto(id))?; let err = |e: std::io::Error| EngineError::Other(format!("{}: {e}", path.display())); let (data, merged, backup) = sidecar_contents(&path, sidecar_packet(p, &self.catalog), &(self.clock)()).map_err(err)?; write_atomic(&path, data.as_bytes()).map_err(err)?; + self.note_sidecar_write(&path, data.as_bytes()); Ok(SidecarSaved { path, merged, backup }) } @@ -527,9 +547,22 @@ impl Session { } /// Auto-write: sidecars for photos changed by `ops` (errors are logged, not returned). - pub(crate) fn auto_write_sidecars(&self, ops: &[Op]) { + pub(crate) fn auto_write_sidecars(&mut self, ops: &[Op]) { let mut ids = Vec::new(); ops.iter().for_each(|o| op_photos(o, &mut ids)); + self.auto_write_sidecar_ids(ids); + } + + /// Gather changed IDs before the saves mutate retained physical repair queues. + pub(crate) fn auto_write_sidecars_since(&mut self, log_start: usize) { + let mut ids = Vec::new(); + if let Some(ops) = self.pending_log.get(log_start..) { + ops.iter().for_each(|o| op_photos(o, &mut ids)); + } + self.auto_write_sidecar_ids(ids); + } + + fn auto_write_sidecar_ids(&mut self, ids: Vec) { if ids.is_empty() { return; } @@ -643,3 +676,15 @@ mod tests { assert_eq!(v, vec![PhotoId(1), PhotoId(3)]); } } + +#[cfg(test)] +#[path = "sidecar/tests_repair_saves.rs"] +mod tests_repair_saves; + +#[cfg(test)] +#[path = "sidecar/tests_repair_companions.rs"] +mod tests_repair_companions; + +#[cfg(test)] +#[path = "sidecar/tests_repair_provenance.rs"] +mod tests_repair_provenance; diff --git a/crates/engine/src/sidecar/tests_repair_companions.rs b/crates/engine/src/sidecar/tests_repair_companions.rs new file mode 100644 index 00000000..cefec46e --- /dev/null +++ b/crates/engine/src/sidecar/tests_repair_companions.rs @@ -0,0 +1,287 @@ +//! Real Session partial moves, sidecar serialization/merging, AutoWrite and history. +use super::*; +use crate::rename::{MoveFs, RealFs}; +use lightcraft_catalog::{Op, Photo, Source}; +use serde_json::json; + +struct Faults(Vec); +impl MoveFs for Faults { + fn exists(&self, p: &Path) -> bool { + RealFs.exists(p) + } + fn is_file(&self, p: &Path) -> bool { + RealFs.is_file(p) + } + fn same_file(&self, a: &Path, b: &Path) -> bool { + RealFs.same_file(a, b) + } + fn rename_no_replace(&self, a: &Path, b: &Path) -> std::io::Result<()> { + if self.0.iter().any(|p| p == a) { + Err(std::io::Error::new(std::io::ErrorKind::PermissionDenied, "injected partial move")) + } else { + RealFs.rename_no_replace(a, b) + } + } + fn copy_no_replace(&self, a: &Path, b: &Path) -> std::io::Result<()> { + RealFs.copy_no_replace(a, b) + } + fn remove_file(&self, p: &Path) -> std::io::Result<()> { + RealFs.remove_file(p) + } + fn stem_sibling(&self, p: &Path) -> Option { + RealFs.stem_sibling(p) + } +} +fn packet(marker: &str) -> String { + format!( + r#""# + ) +} +fn queues(s: &Session) -> Vec { + s.pending_sidecar_repairs.iter().chain(s.undo.iter().chain(&s.redo).flat_map(|e| &e.retry_sidecars)).cloned().collect() +} +fn session(path: &Path) -> Session { + let mut s = Session::new(); + let photo = Photo::new( + PhotoId(1), + Source::File { path: path.to_string_lossy().into_owned() }, + path.file_name().unwrap().to_str().unwrap(), + "JPEG", + 4, + 3, + "", + ); + s.catalog.apply(Op::AddPhoto { photo: Box::new(photo) }).unwrap(); + s.selection = crate::Selection::single(PhotoId(1)); + s +} +fn save(s: &mut Session) -> serde_json::Value { + s.execute("photo.saveMetadataToFile", &json!({})).unwrap() +} +fn assert_fresh(path: &Path) { + let xml = std::fs::read_to_string(path).unwrap(); + assert!(xml.contains("fixture:Marker="), "foreign property must survive real serialization/merge"); + let data = parse_sidecar(&xml, crate::crs::Target::Rendered).unwrap(); + assert_eq!(data.title.as_deref(), Some("Fresh")); + assert!(data.keywords.unwrap().contains(&"café".to_string())); +} + +#[test] +fn partial_move_companions_allow_save_and_autowrite_without_weakening_collisions() { + for upper_stem in [false, true] { + for upper_full in [false, true] { + for naming in [SidecarNaming::Stem, SidecarNaming::Full] { + for auto in [false, true] { + let dir = crate::tests_xmp::temp_dir(&format!("r3-companions-{upper_stem}-{upper_full}-{naming:?}-{auto}")); + let a = dir.join("a.jpg"); + let b = dir.join("Trip.jpg"); + std::fs::write(&a, b"physical fixture; Session rename does not decode").unwrap(); + let path = |photo: &Path, style, upper| { + let path = sidecar_path(&photo.to_string_lossy(), style); + if upper { path.with_extension("XMP") } else { path } + }; + let stem_a = path(&a, SidecarNaming::Stem, upper_stem); + let full_a = path(&a, SidecarNaming::Full, upper_full); + let stem_b = path(&b, SidecarNaming::Stem, upper_stem); + let full_b = path(&b, SidecarNaming::Full, upper_full); + let stem_bytes = packet("stem-keep"); + let full_bytes = packet("full-keep"); + std::fs::write(&stem_a, &stem_bytes).unwrap(); + std::fs::write(&full_a, &full_bytes).unwrap(); + let mut s = session(&a); + s.xmp.naming = naming; + s.xmp.auto_write = auto; + let fs = Faults(vec![full_a.clone(), b.clone()]); + let plans = s.plan_rename_with(&fs, &[PhotoId(1)], "Trip", 1); + assert!(s.execute_fn("photo.rename", |s| Ok(json!({"renamed": s.apply_rename_with(&fs, &plans)?}))).is_err()); + assert_eq!(s.catalog.photo(PhotoId(1)).unwrap().source, Source::File { path: b.to_string_lossy().into_owned() }); + let pending = queues(&s); + assert_eq!(pending.len(), 1); + assert_eq!(std::fs::read_to_string(&stem_b).unwrap(), stem_bytes); + + // Missing packet and an unrelated packet at either case spelling remain protected. + let held = dir.join("held.xmp"); + std::fs::rename(&full_a, &held).unwrap(); + s.execute("photo.setMeta", &json!({"title": "Fresh", "addKeywords": ["café"]})).unwrap(); + assert_eq!(save(&mut s)["failed"].as_array().unwrap().len(), 1); + assert_eq!(queues(&s), pending); + assert_eq!(std::fs::read_to_string(&stem_b).unwrap(), stem_bytes); + std::fs::rename(&held, &full_a).unwrap(); + for collision in [full_b.clone(), full_b.with_extension(if upper_full { "xmp" } else { "XMP" })] { + std::fs::write(&collision, packet("unrelated-packet")).unwrap(); + let occupied = std::fs::read(&collision).unwrap(); + assert_eq!(save(&mut s)["failed"].as_array().unwrap().len(), 1); + assert_eq!(queues(&s), pending); + assert_eq!(std::fs::read(&collision).unwrap(), occupied); + assert_eq!(std::fs::read_to_string(&full_a).unwrap(), full_bytes); + assert_eq!(std::fs::read_to_string(&stem_b).unwrap(), stem_bytes); + std::fs::remove_file(collision).unwrap(); + } + // AutoWrite is the first successful repair when enabled; explicit save otherwise. + s.execute("photo.rate", &json!({"rating": 4})).unwrap(); + if !auto { + assert_eq!(save(&mut s)["failed"].as_array().unwrap().len(), 0); + } + assert!(queues(&s).is_empty()); + assert!(!full_a.exists()); + let (written, untouched, expected) = + if naming == SidecarNaming::Stem { (&stem_b, &full_b, &full_bytes) } else { (&full_b, &stem_b, &stem_bytes) }; + assert_fresh(written); + assert_eq!(std::fs::read_to_string(untouched).unwrap(), *expected); + assert_eq!(parse_sidecar(&std::fs::read_to_string(written).unwrap(), crate::crs::Target::Rendered).unwrap().rating, Some(4)); + for (command, rating) in [("edit.undo", 0), ("edit.redo", 4)] { + s.execute(command, &json!({})).unwrap(); + if !auto { + save(&mut s); + } + assert_fresh(written); + assert_eq!( + parse_sidecar(&std::fs::read_to_string(written).unwrap(), crate::crs::Target::Rendered).unwrap().rating, + Some(rating) + ); + assert_eq!(std::fs::read_to_string(untouched).unwrap(), *expected); + } + assert_eq!(s.xmp.naming, naming); + assert_eq!(s.xmp.auto_write, auto); + drop(s); + std::fs::remove_dir_all(dir).unwrap(); + } + } + } + } +} + +#[test] +fn rollback_repair_and_shared_copy_keep_known_companions() { + for rollback in [false, true] { + for naming in [SidecarNaming::Stem, SidecarNaming::Full] { + let dir = crate::tests_xmp::temp_dir(&format!("r3-companion-rollback-{rollback}-{naming:?}")); + let a = dir.join("a.jpg"); + let b = dir.join("Trip.jpg"); + std::fs::write(&a, b"physical fixture").unwrap(); + let stem_a = sidecar_path(&a.to_string_lossy(), SidecarNaming::Stem); + let full_a = sidecar_path(&a.to_string_lossy(), SidecarNaming::Full); + let stem_b = sidecar_path(&b.to_string_lossy(), SidecarNaming::Stem); + std::fs::write(&stem_a, packet("stem-keep")).unwrap(); + std::fs::write(&full_a, packet("full-keep")).unwrap(); + // When the photo cannot roll back, the stem is copied for the physical sibling. + if !rollback { + std::fs::write(dir.join("a.CR3"), b"physical stem sibling").unwrap(); + } + let mut s = session(&a); + let fs = Faults(vec![full_a.clone(), if rollback { stem_b.clone() } else { b.clone() }]); + let plans = s.plan_rename_with(&fs, &[PhotoId(1)], "Trip", 1); + assert!(s.execute_fn("photo.rename", |s| Ok(json!({"renamed": s.apply_rename_with(&fs, &plans)?}))).is_err()); + assert_eq!(queues(&s).len(), 1); + let actual = if rollback { &a } else { &b }; + assert_eq!(s.catalog.photo(PhotoId(1)).unwrap().source, Source::File { path: actual.to_string_lossy().into_owned() }); + s.xmp.naming = naming; + s.execute("photo.setMeta", &json!({"title": "Fresh", "addKeywords": ["café"]})).unwrap(); + let result = save(&mut s); + assert!(result["failed"].as_array().unwrap().is_empty(), "{result}"); + assert!(queues(&s).is_empty()); + assert_fresh(&sidecar_path(&actual.to_string_lossy(), naming)); + if !rollback { + assert_eq!(std::fs::read_to_string(&stem_a).unwrap(), packet("stem-keep")); + } + drop(s); + std::fs::remove_dir_all(dir).unwrap(); + } + } +} + +#[test] +fn a_later_save_recognizes_companions_after_an_earlier_repair_retires() { + let dir = crate::tests_xmp::temp_dir("r3-successive-companions"); + let a = dir.join("a.jpg"); + let b = dir.join("Trip.jpg"); + let stem_a = sidecar_path(&a.to_string_lossy(), SidecarNaming::Stem); + let full_a = sidecar_path(&a.to_string_lossy(), SidecarNaming::Full); + let upper_a = full_a.with_extension("XMP"); + std::fs::write(&a, b"physical fixture").unwrap(); + std::fs::write(&stem_a, packet("stem-keep")).unwrap(); + std::fs::write(&full_a, packet("lower-keep")).unwrap(); + std::fs::write(&upper_a, packet("upper-keep")).unwrap(); + if RealFs.same_file(&full_a, &upper_a) { + std::fs::remove_dir_all(dir).unwrap(); + return; // This case requires two physically distinct full packets. + } + let mut s = session(&a); + s.xmp.naming = SidecarNaming::Full; + let fs = Faults(vec![full_a.clone(), upper_a.clone(), b.clone()]); + let plans = s.plan_rename_with(&fs, &[PhotoId(1)], "Trip", 1); + assert!(s.execute_fn("photo.rename", |s| Ok(json!({"renamed": s.apply_rename_with(&fs, &plans)?}))).is_err()); + assert_eq!(queues(&s).len(), 2); + assert!(s.repair_sidecars_with(&Faults(vec![full_a.clone()])).is_err()); + assert_eq!(queues(&s).len(), 1); + let upper_b = sidecar_path(&b.to_string_lossy(), SidecarNaming::Full).with_extension("XMP"); + assert_eq!(std::fs::read_to_string(&upper_b).unwrap(), packet("upper-keep")); + s.execute("photo.setMeta", &json!({"title": "Fresh", "addKeywords": ["café"]})).unwrap(); + assert!(save(&mut s)["failed"].as_array().unwrap().is_empty()); + assert!(queues(&s).is_empty()); + assert_fresh(&sidecar_path(&b.to_string_lossy(), SidecarNaming::Full)); + assert_eq!(std::fs::read_to_string(&upper_b).unwrap(), packet("upper-keep")); + assert_eq!(std::fs::read_to_string(sidecar_path(&b.to_string_lossy(), SidecarNaming::Stem)).unwrap(), packet("stem-keep")); + drop(s); + std::fs::remove_dir_all(dir).unwrap(); +} + +#[test] +fn queued_stem_retry_preserves_shared_packet_after_batch_rollback() { + let dir = crate::tests_xmp::temp_dir("queued-stem-batch-rollback"); + let a = dir.join("a.jpg"); + let b = dir.join("a.png"); + let first = dir.join("Trip_1.jpg"); + let second = dir.join("Trip_2.png"); + let shared = dir.join("a.xmp"); + let first_packet = dir.join("Trip_1.xmp"); + let second_packet = dir.join("Trip_2.xmp"); + let jpeg = b"first physical photo; rename does not decode".to_vec(); + let png = b"second physical photo; rename does not decode".to_vec(); + let bytes = packet("shared-after-batch-rollback").into_bytes(); + std::fs::write(&a, &jpeg).unwrap(); + std::fs::write(&b, &png).unwrap(); + std::fs::write(&shared, &bytes).unwrap(); + let mut s = session(&a); + let photo = Photo::new(PhotoId(2), Source::File { path: b.to_string_lossy().into_owned() }, "a.png", "PNG", 4, 3, ""); + s.catalog.apply(Op::AddPhoto { photo: Box::new(photo) }).unwrap(); + s.xmp.auto_write = false; + + // Existing Faults refuses renames only: the first shared-packet copy must succeed. + let faults = Faults(vec![shared.clone(), second.clone()]); + let plans = s.plan_rename_with(&faults, &[PhotoId(1), PhotoId(2)], "Trip_{seq}", 1); + assert!(s.execute_fn("photo.rename", |s| Ok(json!({"renamed": s.apply_rename_with(&faults, &plans)?}))).is_err()); + assert_eq!(s.catalog.photo(PhotoId(1)).unwrap().source, Source::File { path: a.to_string_lossy().into_owned() }); + assert_eq!(s.catalog.photo(PhotoId(2)).unwrap().source, Source::File { path: second.to_string_lossy().into_owned() }); + assert_eq!(std::fs::read(&a).unwrap(), jpeg); + assert_eq!(std::fs::read(&second).unwrap(), png); + assert!(!b.exists() && !first.exists()); + assert_eq!(std::fs::read(&shared).unwrap(), bytes); + assert_eq!(std::fs::read(&first_packet).unwrap(), bytes); + assert!(!second_packet.exists()); + let pending = queues(&s); + assert_eq!(pending.len(), 1); + assert_eq!(pending[0].from, shared); + assert_eq!(pending[0].to, second_packet); + let catalog_before = s.catalog.to_snapshot(); + let history_before = (s.undo.len(), s.redo.len(), s.commits()); + + // Public command, real filesystem: its no-op path drains the naturally retained repair. + let retry = s.execute("photo.rename", &json!({"ids": [2], "template": "{name}"})); + let catalog_after = s.catalog.to_snapshot(); + let history_after = (s.undo.len(), s.redo.len(), s.commits()); + let repaired = queues(&s).is_empty(); + let old_paths_absent = !b.exists() && !first.exists(); + let files = [&a, &second, &first_packet, &second_packet].map(|p| std::fs::read(p).map_err(|e| e.kind())); + let retained = std::fs::read(&shared).map_err(|e| e.kind()); + drop(s); + std::fs::remove_dir_all(&dir).unwrap(); + assert!(!dir.exists(), "fixture cleanup must precede the qualifying assertion"); + + assert_eq!(retry.unwrap()["renamed"], 0); + assert_eq!(catalog_after, catalog_before, "repair must preserve both catalog sources"); + assert_eq!(history_after, history_before); + assert!(repaired && old_paths_absent); + assert_eq!(files, [Ok(jpeg), Ok(png), Ok(bytes.clone()), Ok(bytes.clone())]); + assert_eq!(retained, Ok(bytes), "queued repair took shared XMP from the photo restored by batch rollback"); +} diff --git a/crates/engine/src/sidecar/tests_repair_provenance.rs b/crates/engine/src/sidecar/tests_repair_provenance.rs new file mode 100644 index 00000000..6039e45b --- /dev/null +++ b/crates/engine/src/sidecar/tests_repair_provenance.rs @@ -0,0 +1,461 @@ +//! Native files and genuine Session entry points: packet ownership survives partial renames. +use super::*; +use crate::rename::{MoveFs, RealFs}; +use lightcraft_catalog::{Op, Photo, Source}; +use serde_json::json; + +struct Faults(Vec); +impl MoveFs for Faults { + fn exists(&self, p: &Path) -> bool { + RealFs.exists(p) + } + fn is_file(&self, p: &Path) -> bool { + RealFs.is_file(p) + } + fn same_file(&self, a: &Path, b: &Path) -> bool { + RealFs.same_file(a, b) + } + fn rename_no_replace(&self, a: &Path, b: &Path) -> std::io::Result<()> { + if self.0.iter().any(|p| p == a) { + Err(std::io::Error::new(std::io::ErrorKind::PermissionDenied, "injected provenance partial move")) + } else { + RealFs.rename_no_replace(a, b) + } + } + fn copy_no_replace(&self, a: &Path, b: &Path) -> std::io::Result<()> { + RealFs.copy_no_replace(a, b) + } + fn remove_file(&self, p: &Path) -> std::io::Result<()> { + RealFs.remove_file(p) + } + fn stem_sibling(&self, p: &Path) -> Option { + RealFs.stem_sibling(p) + } +} +fn packet(marker: &str) -> String { + format!( + r#""# + ) +} +fn path(photo: &Path, style: SidecarNaming, upper: bool) -> PathBuf { + let p = sidecar_path(&photo.to_string_lossy(), style); + if upper { p.with_extension("XMP") } else { p } +} +fn queued(s: &Session) -> Vec { + s.pending_sidecar_repairs.iter().chain(s.undo.iter().chain(&s.redo).flat_map(|e| &e.retry_sidecars)).cloned().collect() +} +fn session(photo: &Path) -> Session { + let mut s = Session::new(); + let p = Photo::new(PhotoId(1), Source::File { path: photo.to_string_lossy().into_owned() }, "a.jpg", "JPEG", 4, 3, ""); + s.catalog.apply(Op::AddPhoto { photo: Box::new(p) }).unwrap(); + s.selection = crate::Selection::single(PhotoId(1)); + s +} +fn save(s: &mut Session) -> serde_json::Value { + s.execute("photo.saveMetadataToFile", &json!({})).unwrap() +} +struct Partial { + s: Session, + dir: PathBuf, + a: PathBuf, + b: PathBuf, + companion: PathBuf, + source: PathBuf, + target: PathBuf, +} +fn partial(style: SidecarNaming, upper_stem: bool, upper_full: bool, shared: bool) -> Partial { + let dir = crate::tests_xmp::temp_dir("sidecar-provenance"); + let a = dir.join("a.jpg"); + let b = dir.join("Trip.jpg"); + std::fs::write(&a, b"tiny physical photo; rename does not decode").unwrap(); + let stem_a = path(&a, SidecarNaming::Stem, upper_stem); + let full_a = path(&a, SidecarNaming::Full, upper_full); + let stem_b = path(&b, SidecarNaming::Stem, upper_stem); + let full_b = path(&b, SidecarNaming::Full, upper_full); + std::fs::write(&stem_a, packet("observed-stem")).unwrap(); + std::fs::write(&full_a, packet("observed-full")).unwrap(); + if shared { + std::fs::write(dir.join("a.CR3"), b"shared physical sibling").unwrap(); + } + let mut s = session(&a); + s.xmp.naming = style; + let rollback = style == SidecarNaming::Full; + let faults = Faults(vec![full_a.clone(), if rollback { stem_b.clone() } else { b.clone() }]); + let plans = s.plan_rename_with(&faults, &[PhotoId(1)], "Trip", 1); + assert!(s.execute_fn("photo.rename", |s| Ok(json!({"renamed": s.apply_rename_with(&faults, &plans)?}))).is_err()); + assert_eq!(queued(&s).len(), 1); + let (companion, source, target) = if rollback { (full_a, stem_b, stem_a) } else { (stem_b, full_a, full_b) }; + Partial { s, dir, a, b, companion, source, target } +} +fn replace(p: &Path, held: &Path, in_place: bool) -> (Vec, Vec) { + let original = std::fs::read(p).unwrap(); + #[cfg(unix)] + let before = { + use std::os::unix::fs::MetadataExt; + let m = std::fs::metadata(p).unwrap(); + (m.dev(), m.ino()) + }; + if in_place { + std::fs::write(held, &original).unwrap(); + } else { + std::fs::rename(p, held).unwrap(); + } + let foreign = packet(&"f".repeat(original.len() - packet("").len())).into_bytes(); + assert_eq!(original.len(), foreign.len(), "content changes must be detected even at the same length"); + assert_ne!(original, foreign); + assert!(parse_sidecar(std::str::from_utf8(&foreign).unwrap(), crate::crs::Target::Rendered).is_ok()); + std::fs::write(p, &foreign).unwrap(); + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt; + let m = std::fs::metadata(p).unwrap(); + assert_eq!(before == (m.dev(), m.ino()), in_place, "exercise replacement and unchanged-inode edits separately"); + } + (original, foreign) +} + +#[test] +fn replaced_and_in_place_companions_block_save_autowrite_retry_and_file_history() { + for style in [SidecarNaming::Stem, SidecarNaming::Full] { + for upper_stem in [false, true] { + for upper_full in [false, true] { + for in_place in [false, true] { + let mut f = partial(style, upper_stem, upper_full, false); + let held = f.dir.join("original-held.xmp"); + let (original, foreign) = replace(&f.companion, &held, in_place); + let queue = queued(&f.s); + let source = std::fs::read(&f.source).unwrap(); + let before = f.s.catalog.photo(PhotoId(1)).unwrap().source.clone(); + f.s.xmp.auto_write = true; + f.s.execute("photo.setMeta", &json!({"title": "Fresh", "addKeywords": ["café"]})).unwrap(); + assert_eq!(queued(&f.s), queue, "AutoWrite must retain the exact repair"); + assert_eq!(std::fs::read(&f.companion).unwrap(), foreign); + let saved = save(&mut f.s); + assert!(saved["written"].as_array().unwrap().is_empty()); + assert!(saved["failed"][0]["error"].as_str().unwrap().contains("changed since")); + assert_eq!(queued(&f.s), queue); + assert_eq!(std::fs::read(&f.source).unwrap(), source); + assert!(!f.target.exists()); + assert!(f.s.repair_sidecars_with(&RealFs).is_err()); + let plans = f.s.plan_rename(&[PhotoId(1)], if style == SidecarNaming::Stem { "Trip" } else { "a" }, 1); + assert!(f.s.apply_rename(&plans).is_err()); + // Metadata history remains available while physical ownership is unresolved. + f.s.execute("edit.undo", &json!({})).unwrap(); + assert_eq!(std::fs::read(&f.companion).unwrap(), foreign); + assert!(f.s.undo_step().is_err(), "the real rename Undo must reject the foreign companion"); + assert_eq!(queued(&f.s), queue); + assert_eq!(f.s.catalog.photo(PhotoId(1)).unwrap().source, before); + assert_eq!(std::fs::read(&f.companion).unwrap(), foreign); + assert_eq!(std::fs::read(&held).unwrap(), original); + // Restoring the observed content resolves ownership and keeps the positive path. + std::fs::write(&f.companion, &original).unwrap(); + assert!(save(&mut f.s)["failed"].as_array().unwrap().is_empty()); + assert!(queued(&f.s).is_empty()); + assert_eq!(std::fs::read(&f.target).unwrap(), source); + drop(f.s); + std::fs::remove_dir_all(f.dir).unwrap(); + } + } + } + } +} + +#[test] +fn failed_redo_and_discarded_or_pruned_history_keep_companion_provenance() { + for owner in ["redo", "discarded", "pruned"] { + let mut f = partial(SidecarNaming::Stem, false, false, false); + if owner == "pruned" { + for _ in 0..1000 { + f.s.commit("Unrelated retention step", Op::Batch { ops: Vec::new() }).unwrap(); + } + assert_eq!(f.s.pending_sidecar_repairs.len(), 1); + } else { + f.s.undo_step().unwrap(); + let full_a = path(&f.a, SidecarNaming::Full, false); + assert!(f.s.redo_step_with(&Faults(vec![full_a, f.b.clone()])).is_err()); + assert_eq!(f.s.redo.iter().map(|e| e.retry_sidecars.len()).sum::(), 1); + if owner == "discarded" { + f.s.commit("Discard failed redo", Op::SetRating { id: PhotoId(1), rating: 3 }).unwrap(); + assert_eq!(f.s.pending_sidecar_repairs.len(), 1); + assert!(f.s.redo.is_empty()); + } + } + let held = f.dir.join("original-held.xmp"); + let (original, foreign) = replace(&f.companion, &held, false); + let queue = queued(&f.s); + assert!(!save(&mut f.s)["failed"].as_array().unwrap().is_empty()); + assert!(f.s.repair_sidecars_with(&RealFs).is_err()); + if owner == "redo" { + assert!(f.s.redo_step().is_err()); + } + assert_eq!(queued(&f.s), queue); + assert_eq!(std::fs::read(&f.companion).unwrap(), foreign); + std::fs::write(&f.companion, &original).unwrap(); + assert!(save(&mut f.s)["failed"].as_array().unwrap().is_empty()); + assert!(queued(&f.s).is_empty()); + drop(f.s); + std::fs::remove_dir_all(f.dir).unwrap(); + } +} + +fn two_repairs() -> Option { + let mut f = partial(SidecarNaming::Stem, false, false, false); + // Resolve the initial fixture, then create a new genuine partial rename with two packets. + f.s.undo_step().unwrap(); + let upper_a = path(&f.a, SidecarNaming::Full, true); + std::fs::write(&upper_a, packet("observed-upper-full")).unwrap(); + let full_a = path(&f.a, SidecarNaming::Full, false); + if RealFs.same_file(&upper_a, &full_a) { + drop(f.s); + std::fs::remove_dir_all(f.dir).unwrap(); + return None; // This case requires two physically distinct case spellings. + } + let fs = Faults(vec![upper_a, full_a.clone(), f.b.clone()]); + let plans = f.s.plan_rename_with(&fs, &[PhotoId(1)], "Trip", 1); + assert!(f.s.apply_rename_with(&fs, &plans).is_err()); + assert_eq!(queued(&f.s).len(), 2); + f.source = full_a; + Some(f) +} + +#[test] +fn absent_peer_destinations_are_not_owned_and_retirement_records_only_completed_packets() { + let Some(mut f) = two_repairs() else { return }; + let future = path(&f.b, SidecarNaming::Full, false); + let upper = path(&f.b, SidecarNaming::Full, true); + let foreign = packet("foreign-at-never-created-destination").into_bytes(); + std::fs::write(&future, &foreign).unwrap(); + let queue = queued(&f.s); + let original_companion = std::fs::read(&f.companion).unwrap(); + assert!(!save(&mut f.s)["failed"].as_array().unwrap().is_empty()); + assert!(f.s.repair_sidecars_with(&RealFs).is_err()); + assert_eq!(queued(&f.s), queue); + assert!(!upper.exists(), "a foreign peer must be rejected before the first repair retires"); + assert_eq!(std::fs::read(&future).unwrap(), foreign); + assert_eq!(std::fs::read(&f.companion).unwrap(), original_companion); + std::fs::remove_file(&future).unwrap(); + assert!(f.s.repair_sidecars_with(&Faults(vec![f.source.clone()])).is_err()); + assert_eq!(queued(&f.s).len(), 1); + let held = f.dir.join("retired-original-held.xmp"); + let (observed_upper, replaced) = replace(&upper, &held, false); + let queue = queued(&f.s); + assert!(!save(&mut f.s)["failed"].as_array().unwrap().is_empty()); + assert!(f.s.repair_sidecars_with(&RealFs).is_err()); + assert_eq!(queued(&f.s), queue); + assert_eq!(std::fs::read(&upper).unwrap(), replaced); + assert!(!future.exists()); + std::fs::write(&upper, &observed_upper).unwrap(); + assert!(save(&mut f.s)["failed"].as_array().unwrap().is_empty()); + assert!(queued(&f.s).is_empty()); + assert_eq!(std::fs::read(&upper).unwrap(), observed_upper); + drop(f.s); + std::fs::remove_dir_all(f.dir).unwrap(); +} + +#[test] +fn own_atomic_shared_packet_writes_advance_retained_versions_and_failures_do_not() { + let mut f = partial(SidecarNaming::Stem, false, false, true); + let shared = path(&f.a, SidecarNaming::Stem, false); + let shared_before = std::fs::read(&shared).unwrap(); + let copied_before = std::fs::read(&f.companion).unwrap(); + let raw = f.dir.join("a.CR3"); + let mut photo = Photo::new(PhotoId(2), Source::File { path: raw.to_string_lossy().into_owned() }, "a.CR3", "CR3", 4, 3, ""); + photo.kind = MediaKind::Raw; + photo.meta.title = "Legitimate shared packet write".into(); + f.s.catalog.apply(Op::AddPhoto { photo: Box::new(photo) }).unwrap(); + // A missing repair for the moved JPEG must not block a healthy save by its raw sibling. + let held = f.dir.join("unavailable-source.xmp"); + std::fs::rename(&f.source, &held).unwrap(); + let before = queued(&f.s); + { + let _fault = lightcraft_catalog::safe_file::fail_writes_after(0); + assert!(f.s.save_sidecar(PhotoId(2)).is_err()); + } + assert_eq!(queued(&f.s), before, "failed atomic writes must not advance provenance"); + assert_eq!(std::fs::read(&shared).unwrap(), shared_before); + f.s.save_sidecar(PhotoId(2)).unwrap(); + assert_ne!(std::fs::read(&shared).unwrap(), shared_before); + assert_eq!(std::fs::read(&f.companion).unwrap(), copied_before); + assert_eq!(queued(&f.s).len(), 1); + // External edits are still refused after the application's own atomic replacement. + let shared_written = std::fs::read(&shared).unwrap(); + std::fs::write(&shared, packet("foreign-after-our-write")).unwrap(); + assert!(f.s.save_sidecar(PhotoId(2)).is_err()); + assert_eq!(std::fs::read_to_string(&shared).unwrap(), packet("foreign-after-our-write")); + std::fs::write(&shared, &shared_written).unwrap(); + f.s.save_sidecar(PhotoId(2)).unwrap(); + std::fs::rename(&held, &f.source).unwrap(); + assert!(save(&mut f.s)["failed"].as_array().unwrap().is_empty()); + assert!(queued(&f.s).is_empty(), "own shared writes must not stale the retained repair"); + assert_eq!(std::fs::read(&shared).unwrap(), shared_written); + drop(f.s); + std::fs::remove_dir_all(f.dir).unwrap(); +} + +#[test] +fn replaced_copy_keeps_its_shared_original_and_repair_source_byte_identical() { + let mut f = partial(SidecarNaming::Stem, true, true, true); + let shared = path(&f.a, SidecarNaming::Stem, true); + let shared_before = std::fs::read(&shared).unwrap(); + let source = std::fs::read(&f.source).unwrap(); + let held = f.dir.join("copied-original-held.xmp"); + let (original, foreign) = replace(&f.companion, &held, false); + let queue = queued(&f.s); + f.s.xmp.auto_write = true; + f.s.execute("photo.setMeta", &json!({"title": "Fresh"})).unwrap(); + assert!(!save(&mut f.s)["failed"].as_array().unwrap().is_empty()); + assert!(f.s.repair_sidecars_with(&RealFs).is_err()); + assert_eq!(queued(&f.s), queue); + assert_eq!(std::fs::read(&f.companion).unwrap(), foreign); + assert_eq!(std::fs::read(&shared).unwrap(), shared_before); + assert_eq!(std::fs::read(&f.source).unwrap(), source); + std::fs::write(&f.companion, &original).unwrap(); + assert!(save(&mut f.s)["failed"].as_array().unwrap().is_empty()); + assert!(queued(&f.s).is_empty()); + assert_eq!(std::fs::read(&shared).unwrap(), shared_before); + drop(f.s); + std::fs::remove_dir_all(f.dir).unwrap(); +} + +#[test] +fn changed_separated_sources_are_not_moved_or_retired() { + for style in [SidecarNaming::Stem, SidecarNaming::Full] { + let mut f = partial(style, false, true, false); + let held = f.dir.join("source-original-held.xmp"); + let (original, foreign) = replace(&f.source, &held, true); + let queue = queued(&f.s); + let companion = std::fs::read(&f.companion).unwrap(); + assert!(!save(&mut f.s)["failed"].as_array().unwrap().is_empty()); + assert!(f.s.repair_sidecars_with(&RealFs).is_err()); + assert_eq!(queued(&f.s), queue); + assert_eq!(std::fs::read(&f.source).unwrap(), foreign); + assert_eq!(std::fs::read(&f.companion).unwrap(), companion); + assert!(!f.target.exists()); + std::fs::write(&f.source, &original).unwrap(); + assert!(save(&mut f.s)["failed"].as_array().unwrap().is_empty()); + assert!(queued(&f.s).is_empty()); + drop(f.s); + std::fs::remove_dir_all(f.dir).unwrap(); + } +} + +#[test] +fn detached_history_entry_is_checked_before_unrelated_global_retirement() { + // A private-method contract, not a UI reproduction: application reachability of two + // simultaneous repair owners is unproven. Native move failures and follow_stuck seed + // both records; the second owner's normal apply_rename preflight is deliberately bypassed. + struct NativePartial { + id: PhotoId, + original: PathBuf, + moved: PathBuf, + stem: PathBuf, + full: PathBuf, + companion: PathBuf, + target: PathBuf, + } + let root = crate::tests_xmp::temp_dir("detached-history-preflight-contract"); + let image = lightcraft_raster::Rgba8::from_fn(4, 3, |_, _| [40, 80, 120, 255]); + let png = lightcraft_codecs::encode_png(&lightcraft_codecs::EncodeImage::rgba8(&image), &Default::default()).unwrap(); + let mut s = Session::new(); + s.xmp.naming = SidecarNaming::Stem; + let mut native = Vec::new(); + for (i, name) in ["first", "second"].into_iter().enumerate() { + let dir = root.join(name); + std::fs::create_dir_all(&dir).unwrap(); + let original = dir.join("a.png"); + let moved = dir.join("Trip.png"); + std::fs::write(&original, &png).unwrap(); + let stem = path(&original, SidecarNaming::Stem, false); + let full = path(&original, SidecarNaming::Full, true); + for (p, marker) in [(&stem, format!("{name}-stem")), (&full, format!("{name}-full"))] { + std::fs::write(p, packet(&marker)).unwrap(); + assert_eq!(parse_sidecar(&std::fs::read_to_string(p).unwrap(), crate::crs::Target::Rendered).unwrap().rating, Some(5)); + } + let id = PhotoId(i as u64 + 1); + let photo = Photo::new(id, Source::File { path: original.to_string_lossy().into_owned() }, "a.png", "PNG", 4, 3, ""); + s.catalog.apply(Op::AddPhoto { photo: Box::new(photo) }).unwrap(); + let fs = Faults(vec![full.clone(), moved.clone()]); + if i == 0 { + let plans = s.plan_rename_with(&fs, &[id], "Trip", 1); + assert!(s.apply_rename_with(&fs, &plans).is_err()); + } else { + // Register the genuine second partial move without draining the first repair. + let moves = vec![(original.to_string_lossy().into_owned(), moved.to_string_lossy().into_owned())]; + let err = crate::rename::move_all(&fs, &moves).unwrap_err(); + assert_eq!(err.stuck, moves); + assert_eq!(err.pending_sidecars.len(), 1); + let op = Op::SetFile { id, source: Source::File { path: moved.to_string_lossy().into_owned() }, file_name: "Trip.png".into() }; + s.follow_stuck(&op, err, true); + } + assert_eq!(s.undo.len(), i + 1); + assert_eq!(queued(&s).len(), i + 1); + assert_eq!(s.catalog.photo(id).unwrap().source, Source::File { path: moved.to_string_lossy().into_owned() }); + let decoded = lightcraft_codecs::decode(&std::fs::read(&moved).unwrap(), Default::default()).unwrap(); + assert_eq!((decoded.width, decoded.height), (4, 3)); + native.push(NativePartial { + id, + original, + companion: path(&moved, SidecarNaming::Stem, false), + target: path(&moved, SidecarNaming::Full, true), + moved, + stem, + full, + }); + } + let first = &native[0]; + let second = &native[1]; + let first_stem = std::fs::read(&first.companion).unwrap(); + let full_packets: Vec<_> = native.iter().map(|f| std::fs::read(&f.full).unwrap()).collect(); + let held = root.join("observed-second-companion.xmp"); + let (observed, foreign) = replace(&second.companion, &held, false); + let mut entry = s.undo.pop().unwrap(); // Undo/Redo call this method with their entry detached. + let retained = queued(&s); + let detached = entry.retry_sidecars.clone(); + assert_eq!((retained.len(), detached.len()), (1, 1)); + let sources: Vec<_> = native.iter().map(|f| s.catalog.photo(f.id).unwrap().source.clone()).collect(); + let log_len = s.pending_log.len(); + let mut packets: Vec<_> = native.iter().flat_map(|f| [&f.moved, &f.full, &f.companion]).map(|p| (p.clone(), std::fs::read(p).unwrap())).collect(); + packets.push((held.clone(), std::fs::read(&held).unwrap())); + assert!(!first.target.exists() && !second.target.exists()); + + let err = s.apply_with_files(&mut entry, &RealFs).unwrap_err(); + assert!(err.to_string().contains("changed since"), "{err}"); + assert_eq!(std::fs::read(&second.companion).unwrap(), foreign); + println!( + "detached history preflight contract: retained_before={}, retained_after={}, detached_before={}, detached_after={}, first_target_exists={}", + retained.len(), + queued(&s).len(), + detached.len(), + entry.retry_sidecars.len(), + first.target.exists() + ); + assert_eq!(queued(&s), retained, "reject the detached owner before any unrelated global repair retires"); + assert_eq!(entry.retry_sidecars, detached); + assert_eq!(s.pending_log.len(), log_len); + for (f, source) in native.iter().zip(&sources) { + assert_eq!(&s.catalog.photo(f.id).unwrap().source, source); + assert!(!f.original.exists() && !f.target.exists()); + } + for (p, bytes) in &packets { + assert_eq!(std::fs::read(p).unwrap(), *bytes, "rejection must preserve {} byte for byte", p.display()); + } + + // Resolving the changed content permits the same real method to repair and undo the move. + std::fs::write(&second.companion, &observed).unwrap(); + let inverse = s.apply_with_files(&mut entry, &RealFs).unwrap(); + assert!(queued(&s).is_empty() && entry.retry_sidecars.is_empty()); + assert_eq!(s.catalog.photo(first.id).unwrap().source, sources[0]); + assert_eq!(s.catalog.photo(second.id).unwrap().source, Source::File { path: second.original.to_string_lossy().into_owned() }); + assert_eq!(s.file_moves(&inverse), vec![(second.original.to_string_lossy().into_owned(), second.moved.to_string_lossy().into_owned())]); + assert_eq!(std::fs::read(&first.moved).unwrap(), png); + assert_eq!(std::fs::read(&second.original).unwrap(), png); + assert_eq!(std::fs::read(&first.companion).unwrap(), first_stem); + assert_eq!(std::fs::read(&first.target).unwrap(), full_packets[0]); + assert_eq!(std::fs::read(&second.stem).unwrap(), observed); + assert_eq!(std::fs::read(&second.full).unwrap(), full_packets[1]); + assert_eq!(std::fs::read(&held).unwrap(), observed); + assert!(!first.original.exists() && !first.full.exists()); + assert!(!second.moved.exists() && !second.companion.exists() && !second.target.exists()); + drop(s); + std::fs::remove_dir_all(root).unwrap(); +} diff --git a/crates/engine/src/sidecar/tests_repair_saves.rs b/crates/engine/src/sidecar/tests_repair_saves.rs new file mode 100644 index 00000000..6f9fa4aa --- /dev/null +++ b/crates/engine/src/sidecar/tests_repair_saves.rs @@ -0,0 +1,210 @@ +//! Real Session/serializer regressions for saves that overlap retained physical repairs. +use super::*; +use crate::rename::{MoveFs, RealFs}; +use lightcraft_catalog::{Op, Photo, Source}; +use serde_json::json; + +struct PartialRename { + sidecar: PathBuf, + photo: PathBuf, +} +impl MoveFs for PartialRename { + fn exists(&self, path: &Path) -> bool { + RealFs.exists(path) + } + fn is_file(&self, path: &Path) -> bool { + RealFs.is_file(path) + } + fn same_file(&self, a: &Path, b: &Path) -> bool { + RealFs.same_file(a, b) + } + fn rename_no_replace(&self, a: &Path, b: &Path) -> std::io::Result<()> { + if a == self.sidecar || a == self.photo { + Err(std::io::Error::new(std::io::ErrorKind::PermissionDenied, "transient rename fault")) + } else { + RealFs.rename_no_replace(a, b) + } + } + fn copy_no_replace(&self, a: &Path, b: &Path) -> std::io::Result<()> { + if a == self.sidecar { + Err(std::io::Error::new(std::io::ErrorKind::PermissionDenied, "transient copy fault")) + } else { + RealFs.copy_no_replace(a, b) + } + } + fn remove_file(&self, path: &Path) -> std::io::Result<()> { + RealFs.remove_file(path) + } + fn stem_sibling(&self, path: &Path) -> Option { + RealFs.stem_sibling(path) + } +} + +const FOREIGN: &[u8] = br#""#; + +fn png(path: &Path) { + let image = lightcraft_raster::Rgba8::from_fn(4, 3, |_, _| [40, 80, 120, 255]); + let bytes = lightcraft_codecs::encode_png(&lightcraft_codecs::EncodeImage::rgba8(&image), &Default::default()).unwrap(); + std::fs::write(path, bytes).unwrap(); +} +fn queued(s: &Session) -> Vec { + s.pending_sidecar_repairs.iter().chain(s.undo.iter().chain(&s.redo).flat_map(|entry| entry.retry_sidecars.iter())).cloned().collect() +} + +#[test] +fn autowrite_repairs_partial_rename_before_merging_foreign_xmp_across_library_switch() { + for naming in [SidecarNaming::Stem, SidecarNaming::Full] { + for save_naming in [SidecarNaming::Stem, SidecarNaming::Full] { + for upper in [false, true] { + for switch in [false, true] { + let root = crate::tests_xmp::temp_dir(&format!("repair-save-{naming:?}-{save_naming:?}-{upper}-{switch}")); + let a = root.join("a.png"); + let b = root.join("Trip.png"); + png(&a); + let mut old = sidecar_path(&a.to_string_lossy(), naming); + if upper { + old = old.with_extension("XMP"); + } + std::fs::write(&old, FOREIGN).unwrap(); + let mut s = Session::new().with_fs(); + s.open_library(root.join("library-a"), false).unwrap(); + s.execute("library.import", &json!({"paths": [a]})).unwrap(); + let mut id = s.catalog.photos().next().unwrap().id; + s.selection = crate::Selection::single(id); + s.undo.clear(); + let fs = PartialRename { sidecar: old.clone(), photo: b.clone() }; + let plans = s.plan_rename_with(&fs, &[id], "Trip", 1); + assert!(s.execute_fn("photo.rename", |s| Ok(json!({"renamed": s.apply_rename_with(&fs, &plans)?}))).is_err()); + assert_eq!(queued(&s).len(), 1); + assert_eq!(s.catalog.photo(id).unwrap().source, Source::File { path: b.to_string_lossy().into_owned() }); + let pending = queued(&s); + let held = root.join("temporarily-unavailable.xmp"); + std::fs::rename(&old, &held).unwrap(); + if switch { + s.close_library().unwrap(); + s.open_library(root.join("library-b"), false).unwrap(); + assert_eq!(s.pending_sidecar_repairs, pending); + s.execute("library.import", &json!({"paths": [b]})).unwrap(); + id = s.catalog.photos().next().unwrap().id; + s.selection = crate::Selection::single(id); + } + s.xmp.naming = save_naming; + s.xmp.auto_write = true; + s.execute("photo.rate", &json!({"rating": 4})).unwrap(); + let preferred = sidecar_path(&b.to_string_lossy(), save_naming); + assert!(!preferred.exists() && !preferred.with_extension("XMP").exists()); + assert!(!pending[0].to.exists()); + assert_eq!(queued(&s), pending, "unavailable repair stays queued after AutoWrite"); + let saved = s.execute("photo.saveMetadataToFile", &json!({})).unwrap(); + assert!(saved["written"].as_array().unwrap().is_empty()); + assert!(saved["failed"][0]["error"].as_str().unwrap().contains("unfinished XMP repair")); + assert_eq!(std::fs::read(&held).unwrap(), FOREIGN); + assert_eq!(queued(&s), pending); + + // A missing sidecar for another photo does not prevent this healthy save. + let healthy = root.join("healthy.png"); + png(&healthy); + let healthy_id = PhotoId(999); + s.commit( + "Healthy Photo", + Op::AddPhoto { + photo: Box::new(Photo::new( + healthy_id, + Source::File { path: healthy.to_string_lossy().into_owned() }, + "healthy.png", + "PNG", + 4, + 3, + "", + )), + }, + ) + .unwrap(); + s.execute("photo.setMeta", &json!({"ids": [healthy_id.0], "title": "Healthy save"})).unwrap(); + let packet = std::fs::read_to_string(sidecar_path(&healthy.to_string_lossy(), save_naming)).unwrap(); + assert_eq!(parse_sidecar(&packet, crate::crs::Target::Rendered).unwrap().title.as_deref(), Some("Healthy save")); + assert_eq!(queued(&s), pending); + + // An existing destination is never overwritten and the original packet remains. + std::fs::rename(&held, &old).unwrap(); + let destination = pending[0].to.clone(); + let collision = String::from_utf8(FOREIGN.to_vec()).unwrap().replace("Rating=\"2\"", "Rating=\"5\"").into_bytes(); + let collision_path = if upper { preferred.clone() } else { destination.clone() }; + std::fs::write(&collision_path, &collision).unwrap(); + s.execute("photo.setMeta", &json!({"ids": [id.0], "title": "Fresh title", "addKeywords": ["café"]})).unwrap(); + assert_eq!(std::fs::read(&old).unwrap(), FOREIGN); + assert_eq!(std::fs::read(&collision_path).unwrap(), collision); + assert_eq!(queued(&s), pending); + s.selection = crate::Selection::single(id); + assert_eq!(s.execute("photo.saveMetadataToFile", &json!({})).unwrap()["failed"].as_array().unwrap().len(), 1); + assert_eq!(std::fs::read(&collision_path).unwrap(), collision); + std::fs::remove_file(&collision_path).unwrap(); // remove only this test's collision + + // The next successful AutoWrite runs the real repair and production XMP merge. + s.execute("photo.setMeta", &json!({"title": "Final title"})).unwrap(); + assert!(queued(&s).is_empty()); + assert!(!old.exists()); + let packet = std::fs::read_to_string(&destination).unwrap(); + assert!(packet.contains("fixture:Marker=\"keep-me\"")); + let parsed = parse_sidecar(&packet, crate::crs::Target::Rendered).unwrap(); + assert_eq!(parsed.rating, Some(4)); + assert_eq!(parsed.title.as_deref(), Some("Final title")); + assert!(parsed.keywords.as_ref().unwrap().contains(&"café".to_string())); + if upper { + assert!(!preferred.exists(), "merge uses the repaired uppercase packet"); + } + for (step, title) in [("edit.undo", "Fresh title"), ("edit.redo", "Final title")] { + s.execute(step, &json!({})).unwrap(); + let packet = std::fs::read_to_string(&destination).unwrap(); + assert!(packet.contains("fixture:Marker=\"keep-me\"")); + assert_eq!(parse_sidecar(&packet, crate::crs::Target::Rendered).unwrap().title.as_deref(), Some(title)); + } + drop(s); + std::fs::remove_dir_all(root).unwrap(); + } + } + } + } +} + +#[test] +fn unowned_shared_stem_repair_does_not_block_healthy_jpeg_full_sidecar_save() { + let root = crate::tests_xmp::temp_dir("repair-save-shared-stem"); + let raw = root.join("old.CR3"); + let moved = root.join("Trip.CR3"); + let jpeg = root.join("Trip.jpg"); + std::fs::write(&raw, b"physical fixture; catalogued without decoding").unwrap(); + png(&jpeg); + let old = sidecar_path(&raw.to_string_lossy(), SidecarNaming::Stem); + std::fs::write(&old, FOREIGN).unwrap(); + let mut s = Session::new(); + let raw_id = PhotoId(1); + let jpeg_id = PhotoId(2); + let mut photo = Photo::new(raw_id, Source::File { path: raw.to_string_lossy().into_owned() }, "old.CR3", "CR3", 4, 3, ""); + photo.kind = MediaKind::Raw; + s.catalog.apply(Op::AddPhoto { photo: Box::new(photo) }).unwrap(); + s.catalog + .apply(Op::AddPhoto { + photo: Box::new(Photo::new(jpeg_id, Source::File { path: jpeg.to_string_lossy().into_owned() }, "Trip.jpg", "JPEG", 4, 3, "")), + }) + .unwrap(); + let fs = PartialRename { sidecar: old.clone(), photo: moved }; + let plans = s.plan_rename_with(&fs, &[raw_id], "Trip", 1); + assert!(s.execute_fn("photo.rename", |s| Ok(json!({"renamed": s.apply_rename_with(&fs, &plans)?}))).is_err()); + let pending = queued(&s); + assert_eq!(pending.len(), 1); + let held = root.join("temporarily-unavailable.xmp"); + std::fs::rename(&old, &held).unwrap(); + assert_eq!(s.sidecar_naming(jpeg_id), SidecarNaming::Full); + s.selection = crate::Selection::single(jpeg_id); + s.xmp.auto_write = true; + s.execute("photo.setMeta", &json!({"title": "Healthy JPEG"})).unwrap(); + let full = sidecar_path(&jpeg.to_string_lossy(), SidecarNaming::Full); + let packet = std::fs::read_to_string(&full).unwrap(); + assert_eq!(parse_sidecar(&packet, crate::crs::Target::Rendered).unwrap().title.as_deref(), Some("Healthy JPEG")); + assert!(!pending[0].to.exists()); + assert_eq!(queued(&s), pending); + assert_eq!(std::fs::read(&held).unwrap(), FOREIGN); + drop(s); + std::fs::remove_dir_all(root).unwrap(); +} diff --git a/crates/engine/src/tests_filesystem_history.rs b/crates/engine/src/tests_filesystem_history.rs new file mode 100644 index 00000000..c071d995 --- /dev/null +++ b/crates/engine/src/tests_filesystem_history.rs @@ -0,0 +1,537 @@ +//! Filesystem history and retained XMP repair regressions. + +use lightcraft_catalog::{Op, Photo, PhotoId, Source}; +use serde_json::json; + +use crate::{Selection, Session}; + +fn select(s: &mut Session, ids: &[u64], active: u64) { + s.execute("library.select", &json!({"ids": ids, "active": active})).unwrap(); +} + +fn memory_library(store: &lightcraft_catalog::MemStore) -> Session { + let mut s = Session::new(); + s.open_library_in( + crate::library::LibraryStores { + dir: "history-regression".into(), + catalog: Box::new(store.clone()), + files: Box::new(lightcraft_catalog::MemStore::new()), + on_disk: false, + }, + false, + ) + .unwrap(); + s +} + +fn file_history_step(s: &mut Session, fs: &dyn crate::rename::MoveFs, redo: bool) -> crate::Result { + s.execute_fn(if redo { "edit.redo" } else { "edit.undo" }, |s| { + let label = if redo { s.redo_step_with(fs)? } else { s.undo_step_with(fs)? }; + Ok(json!({"label": label})) + }) +} + +fn assert_file_catalog_and_journal(s: &Session, fs: &crate::rename::tests::FakeFs, store: &lightcraft_catalog::MemStore) { + let files = fs.listing(); + for photo in s.catalog.photos() { + let Source::File { path } = &photo.source else { panic!("synthetic file fixture") }; + assert!(files.iter().any(|(file, _)| file == path), "photo {} points to missing {path}: {files:?}", photo.id.0); + assert_eq!(std::path::Path::new(path).file_name().unwrap().to_string_lossy(), photo.file_name.as_str()); + } + let replayed = memory_library(store); + assert_eq!(replayed.catalog.to_snapshot(), s.catalog.to_snapshot()); +} + +fn sidecar_history_fixture(redo: bool) -> (Session, crate::rename::tests::FakeFs, lightcraft_catalog::MemStore) { + let store = lightcraft_catalog::MemStore::new(); + let mut s = memory_library(&store); + let fs = crate::rename::tests::FakeFs::new( + false, + &[ + ("/p/a.jpg", "Photo"), + ("/p/a.XMP", "Upper stem edits"), + ("/p/a.xmp", "Lower stem edits"), + ("/p/a.jpg.XMP", "Upper full-name edits"), + ("/p/a.jpg.xmp", "Lower full-name edits"), + ("/p/other.jpg", "Other"), + ], + ); + for (i, path) in ["/p/a.jpg", "/p/other.jpg"].iter().enumerate() { + let name = std::path::Path::new(path).file_name().unwrap().to_string_lossy(); + let photo = Photo::new(PhotoId(i as u64 + 1), Source::File { path: (*path).into() }, &name, "JPEG", 16, 16, ""); + s.commit("Synthetic File", Op::AddPhoto { photo: Box::new(photo) }).unwrap(); + } + s.persist().unwrap(); + s.undo.clear(); + select(&mut s, &[1], 1); + let plans = s.plan_rename_with(&fs, &[PhotoId(1)], "Trip", 1); + s.execute_fn("photo.rename", |s| Ok(json!({"renamed": s.apply_rename_with(&fs, &plans)?}))).unwrap(); + if redo { + file_history_step(&mut s, &fs, false).unwrap(); + } + select(&mut s, &[2], 2); + (s, fs, store) +} + +fn sidecar_history_entry(s: &Session, redo: bool) -> &crate::UndoEntry { + if redo { s.redo.last().unwrap() } else { s.undo.last().unwrap() } +} + +#[test] +fn merged_renames_coalesce_physical_moves_and_keep_retry_inverses_and_journal() { + use crate::rename::tests::FakeFs; + use crate::sidecar::{SidecarNaming, sidecar_path}; + + let store = lightcraft_catalog::MemStore::new(); + let mut s = memory_library(&store); + let fs = FakeFs::new(false, &[("/p/a.jpg", "Photo"), ("/p/a.xmp", "Edits"), ("/p/other.jpg", "Other")]); + for (id, path) in [(1, "/p/a.jpg"), (2, "/p/other.jpg")] { + let name = std::path::Path::new(path).file_name().unwrap().to_string_lossy(); + let photo = Photo::new(PhotoId(id), Source::File { path: path.into() }, &name, "JPEG", 16, 16, ""); + s.commit("Synthetic File", Op::AddPhoto { photo: Box::new(photo) }).unwrap(); + } + select(&mut s, &[1], 1); + let copy = s.execute("photo.virtualCopy", &json!({})).unwrap()["ids"][0].as_u64().unwrap(); + s.undo.clear(); + let before = fs.listing(); + for name in ["middle", "final"] { + let plans = s.plan_rename_with(&fs, &[PhotoId(1)], name, 1); + s.execute_fn("photo.rename", |s| Ok(json!({"renamed": s.apply_rename_with(&fs, &plans)?}))).unwrap(); + assert_file_catalog_and_journal(&s, &fs, &store); + } + let renamed = fs.listing(); + s.merge_undo(2, "Rename Twice"); + assert_eq!(s.undo.len(), 1); + assert_eq!(s.file_moves(&s.undo.last().unwrap().op), vec![("/p/final.jpg".into(), "/p/a.jpg".into())]); + // First exercise the ordinary merged step, before any corrective paths exist. + for redo in [false, true] { + file_history_step(&mut s, &fs, redo).unwrap(); + assert_eq!(fs.listing(), if redo { renamed.clone() } else { before.clone() }); + assert_eq!(s.catalog.photo(PhotoId(1)).unwrap().source, s.catalog.photo(PhotoId(copy)).unwrap().source); + assert_file_catalog_and_journal(&s, &fs, &store); + } + for redo in [false, true, false] { + select(&mut s, &[2], 2); + let entry = if redo { s.redo.last().unwrap() } else { s.undo.last().unwrap() }; + let moves = s.file_moves(&entry.op); + assert_eq!(moves.len(), 1, "virtual copies must share the final physical move"); + let (from, to) = &moves[0]; + let sidecar = sidecar_path(from, SidecarNaming::Stem).to_string_lossy().into_owned(); + *fs.fail.borrow_mut() = vec![sidecar, to.clone()]; + let history = (s.undo.len(), s.redo.len()); + assert!(file_history_step(&mut s, &fs, redo).is_err()); + assert_eq!((s.undo.len(), s.redo.len()), history); + assert_eq!(s.selection, Selection::single(PhotoId(2))); + assert_file_catalog_and_journal(&s, &fs, &store); + fs.fail.borrow_mut().clear(); + file_history_step(&mut s, &fs, redo).unwrap(); + assert_eq!(fs.listing(), if redo { renamed.clone() } else { before.clone() }); + assert_eq!(s.catalog.photo(PhotoId(1)).unwrap().source, s.catalog.photo(PhotoId(copy)).unwrap().source); + assert_file_catalog_and_journal(&s, &fs, &store); + } +} + +#[test] +fn partial_file_history_failures_retry_without_losing_either_inverse_or_journal() { + use crate::rename::tests::FakeFs; + + for redo in [false, true] { + let store = lightcraft_catalog::MemStore::new(); + let mut s = memory_library(&store); + let fs = FakeFs::new( + false, + &[ + ("/p/a.jpg", "A"), + ("/p/a.xmp", "A edits"), + ("/p/b.jpg", "B"), + ("/p/b.xmp", "B edits"), + ("/p/c.jpg", "C"), + ("/p/c.xmp", "C edits"), + ("/p/other.jpg", "Other"), + ], + ); + let ops = ["/p/a.jpg", "/p/b.jpg", "/p/c.jpg", "/p/other.jpg"] + .iter() + .enumerate() + .map(|(i, path)| { + let name = std::path::Path::new(path).file_name().unwrap().to_string_lossy(); + let photo = Photo::new(PhotoId(i as u64 + 1), Source::File { path: (*path).into() }, &name, "JPEG", 16, 16, ""); + Op::AddPhoto { photo: Box::new(photo) } + }) + .collect(); + s.commit("Synthetic Files", Op::Batch { ops }).unwrap(); + // A virtual copy shares B's physical file and must follow every partial correction too. + let mut copy = (**s.catalog.photo(PhotoId(2)).unwrap()).clone(); + copy.id = PhotoId(5); + copy.copy_of = Some(PhotoId(2)); + s.commit("Synthetic Copy", Op::AddPhoto { photo: Box::new(copy) }).unwrap(); + s.persist().unwrap(); + s.undo.clear(); + select(&mut s, &[1, 2, 3], 1); + let before = fs.listing(); + let plans = s.plan_rename_with(&fs, &[PhotoId(1), PhotoId(2), PhotoId(3)], "Trip-{seq}", 1); + s.execute_fn("photo.rename", |s| Ok(json!({"renamed": s.apply_rename_with(&fs, &plans)?}))).unwrap(); + let renamed = fs.listing(); + if redo { + file_history_step(&mut s, &fs, false).unwrap(); + } + select(&mut s, &[4], 4); + // Two bounded failures leave a different file moved on each attempt. A subsequent + // failure with no moves must keep those corrections; a final retry round-trips them. + for attempt in 0..2 { + let entry = if redo { s.redo.last().unwrap() } else { s.undo.last().unwrap() }; + let moves = s.file_moves(&entry.op); + assert_eq!(moves.len(), 3 - attempt); + *fs.fail.borrow_mut() = vec![moves[1].0.clone(), moves[0].1.clone()]; + let history = (s.undo.len(), s.redo.len()); + let error = file_history_step(&mut s, &fs, redo).unwrap_err().to_string(); + assert!(error.contains("could not be moved back"), "{error}"); + assert_eq!((s.undo.len(), s.redo.len()), history); + assert_eq!(s.selection, Selection::single(PhotoId(4))); + assert_file_catalog_and_journal(&s, &fs, &store); + } + let entry = if redo { s.redo.last().unwrap() } else { s.undo.last().unwrap() }; + let moves = s.file_moves(&entry.op); + assert_eq!(moves.len(), 1); + *fs.fail.borrow_mut() = vec![moves[0].0.clone()]; + let unchanged = fs.listing(); + let history = (s.undo.len(), s.redo.len()); + assert!(file_history_step(&mut s, &fs, redo).is_err()); + assert_eq!(fs.listing(), unchanged); + assert_eq!((s.undo.len(), s.redo.len()), history); + assert_eq!(s.selection, Selection::single(PhotoId(4))); + assert_file_catalog_and_journal(&s, &fs, &store); + fs.fail.borrow_mut().clear(); + file_history_step(&mut s, &fs, redo).unwrap(); + assert_eq!(fs.listing(), if redo { renamed.clone() } else { before.clone() }); + assert_file_catalog_and_journal(&s, &fs, &store); + file_history_step(&mut s, &fs, !redo).unwrap(); + assert_eq!(fs.listing(), if redo { before.clone() } else { renamed.clone() }); + assert_file_catalog_and_journal(&s, &fs, &store); + file_history_step(&mut s, &fs, redo).unwrap(); + assert_eq!(fs.listing(), if redo { renamed } else { before }); + assert_file_catalog_and_journal(&s, &fs, &store); + } +} + +#[test] +fn history_retries_both_sidecar_styles_after_following_a_photo_with_failed_rollback() { + use crate::rename::MoveFs; + use crate::sidecar::{SidecarNaming, sidecar_path}; + + for redo in [false, true] { + for naming in [SidecarNaming::Stem, SidecarNaming::Full] { + let (mut s, fs, store) = sidecar_history_fixture(redo); + let moves = s.file_moves(&sidecar_history_entry(&s, redo).op); + assert_eq!(moves.len(), 1); + let (from, to) = &moves[0]; + let from = from.clone(); + let to = to.clone(); + let before = fs.listing(); + let failing = sidecar_path(&from, naming).with_extension("XMP"); + *fs.fail.borrow_mut() = vec![failing.to_string_lossy().into_owned(), to.clone()]; + let history = (s.undo.len(), s.redo.len()); + let error = file_history_step(&mut s, &fs, redo).unwrap_err().to_string(); + assert!(error.contains("could not be moved back"), "{error}"); + assert_eq!(s.catalog.photo(PhotoId(1)).unwrap().source, Source::File { path: to.clone() }); + assert!(s.file_moves(&sidecar_history_entry(&s, redo).op).is_empty(), "the photo already followed its destination"); + let expected_pending = if naming == SidecarNaming::Stem { 4 } else { 2 }; + assert_eq!(sidecar_history_entry(&s, redo).retry_sidecars.len(), expected_pending); + assert_file_catalog_and_journal(&s, &fs, &store); + + // Repeating the same XMP failure must not turn the empty photo move list into success. + *fs.fail.borrow_mut() = vec![failing.to_string_lossy().into_owned()]; + let partial = fs.listing(); + let error = file_history_step(&mut s, &fs, redo).unwrap_err().to_string(); + assert!(error.contains("injected failure"), "{error}"); + assert_eq!(fs.listing(), partial); + assert_eq!((s.undo.len(), s.redo.len()), history); + assert_eq!(s.selection, Selection::single(PhotoId(2))); + assert_eq!(sidecar_history_entry(&s, redo).retry_sidecars.len(), expected_pending); + assert_file_catalog_and_journal(&s, &fs, &store); + + // Make progress, then fail at the other full-name spelling. Completed sidecars + // must not be retried (their old names no longer exist). + let last = sidecar_history_entry(&s, redo).retry_sidecars.last().unwrap().clone(); + *fs.fail.borrow_mut() = vec![last.from.to_string_lossy().into_owned()]; + assert!(file_history_step(&mut s, &fs, redo).is_err()); + assert_eq!(sidecar_history_entry(&s, redo).retry_sidecars.len(), 1); + assert_eq!((s.undo.len(), s.redo.len()), history); + assert_eq!(s.selection, Selection::single(PhotoId(2))); + assert_file_catalog_and_journal(&s, &fs, &store); + + // A new destination collision stays an error and never replaces unrelated bytes. + fs.fail.borrow_mut().clear(); + fs.files.borrow_mut().push((last.to.to_string_lossy().into_owned(), b"Unrelated edits".to_vec())); + let occupied = fs.listing(); + assert!(file_history_step(&mut s, &fs, redo).is_err()); + assert_eq!(fs.listing(), occupied); + assert_eq!((s.undo.len(), s.redo.len()), history); + assert_eq!(sidecar_history_entry(&s, redo).retry_sidecars.len(), 1); + assert_file_catalog_and_journal(&s, &fs, &store); + fs.remove_file(&last.to).unwrap(); + + file_history_step(&mut s, &fs, redo).unwrap(); + let mut after = before + .iter() + .map(|(path, bytes)| (path.replace(from.trim_end_matches(".jpg"), to.trim_end_matches(".jpg")), bytes.clone())) + .collect::>(); + after.sort(); + assert_eq!(fs.listing(), after); + assert_file_catalog_and_journal(&s, &fs, &store); + file_history_step(&mut s, &fs, !redo).unwrap(); + assert_eq!(fs.listing(), before); + assert_file_catalog_and_journal(&s, &fs, &store); + file_history_step(&mut s, &fs, redo).unwrap(); + assert_eq!(fs.listing(), after); + assert_file_catalog_and_journal(&s, &fs, &store); + } + } +} + +#[test] +fn history_repairs_a_sidecar_whose_rollback_failed_before_retrying_the_photo() { + use crate::sidecar::{SidecarNaming, sidecar_path}; + + for redo in [false, true] { + let (mut s, fs, store) = sidecar_history_fixture(redo); + let moves = s.file_moves(&sidecar_history_entry(&s, redo).op); + let (from, to) = &moves[0]; + let from = from.clone(); + let to = to.clone(); + let before = fs.listing(); + // The stem sidecars move first; full-name failure rolls the photo back, but the + // first stem sidecar cannot return. The catalog has not changed this time. + let failed_full = sidecar_path(&from, SidecarNaming::Full).with_extension("XMP"); + let stranded = sidecar_path(&to, SidecarNaming::Stem).with_extension("XMP"); + *fs.fail.borrow_mut() = vec![failed_full.to_string_lossy().into_owned(), stranded.to_string_lossy().into_owned()]; + let history = (s.undo.len(), s.redo.len()); + let error = file_history_step(&mut s, &fs, redo).unwrap_err().to_string(); + assert!(error.contains("sidecar") && error.contains("could not be moved back"), "{error}"); + let entry = sidecar_history_entry(&s, redo); + assert_eq!(entry.retry_sidecars.len(), 1); + assert!(entry.retry_inverse.is_empty()); + assert_eq!(s.catalog.photo(PhotoId(1)).unwrap().source, Source::File { path: from }); + assert_file_catalog_and_journal(&s, &fs, &store); + let partial = fs.listing(); + *fs.fail.borrow_mut() = vec![stranded.to_string_lossy().into_owned()]; + assert!(file_history_step(&mut s, &fs, redo).is_err()); + assert_eq!(fs.listing(), partial); + assert_eq!((s.undo.len(), s.redo.len()), history); + assert_eq!(s.selection, Selection::single(PhotoId(2))); + fs.fail.borrow_mut().clear(); + file_history_step(&mut s, &fs, redo).unwrap(); + assert_file_catalog_and_journal(&s, &fs, &store); + file_history_step(&mut s, &fs, !redo).unwrap(); + assert_eq!(fs.listing(), before); + assert_file_catalog_and_journal(&s, &fs, &store); + } +} + +#[test] +fn initial_partial_rename_keeps_sidecar_repairs_for_noop_retry_and_history() { + use crate::rename::tests::FakeFs; + let store = lightcraft_catalog::MemStore::new(); + let mut s = memory_library(&store); + let fs = FakeFs::new(false, &[("/p/a.jpg", "Photo"), ("/p/a.xmp", "Stem edits"), ("/p/a.jpg.xmp", "Full edits")]); + let photo = Photo::new(PhotoId(1), Source::File { path: "/p/a.jpg".into() }, "a.jpg", "JPEG", 16, 16, ""); + s.commit("Synthetic File", Op::AddPhoto { photo: Box::new(photo) }).unwrap(); + s.persist().unwrap(); + s.undo.clear(); + select(&mut s, &[1], 1); + let before = fs.listing(); + let plans = s.plan_rename_with(&fs, &[PhotoId(1)], "Trip", 1); + *fs.fail.borrow_mut() = vec!["/p/a.xmp".into(), "/p/Trip.jpg".into()]; + assert!(s.execute_fn("photo.rename", |s| Ok(json!({"renamed": s.apply_rename_with(&fs, &plans)?}))).is_err()); + assert_eq!(s.catalog.photo(PhotoId(1)).unwrap().source, Source::File { path: "/p/Trip.jpg".into() }); + assert_eq!(s.undo.len(), 1); + assert_eq!(s.undo.last().unwrap().retry_sidecars.len(), 2); + assert_file_catalog_and_journal(&s, &fs, &store); + let retry = s.plan_rename_with(&fs, &[PhotoId(1)], "Trip", 1); + assert_eq!(retry[0].from_path, retry[0].to_path, "the physical photo already has the requested name"); + *fs.fail.borrow_mut() = vec!["/p/a.xmp".into()]; + let partial = fs.listing(); + assert!(s.execute_fn("photo.rename", |s| Ok(json!({"renamed": s.apply_rename_with(&fs, &retry)?}))).is_err()); + assert_eq!(fs.listing(), partial); + assert_eq!(s.undo.last().unwrap().retry_sidecars.len(), 2, "a failed no-op retry must keep its repair queue"); + fs.fail.borrow_mut().clear(); + assert_eq!(s.execute_fn("photo.rename", |s| Ok(json!({"renamed": s.apply_rename_with(&fs, &retry)?}))).unwrap()["renamed"], 0); + assert!(s.undo.last().unwrap().retry_sidecars.is_empty()); + assert_eq!(s.undo.len(), 1, "a repaired no-op retry adds no empty rename step"); + let after = fs.listing(); + assert_eq!( + after, + vec![("/p/Trip.jpg".into(), "Photo".into()), ("/p/Trip.jpg.xmp".into(), "Full edits".into()), ("/p/Trip.xmp".into(), "Stem edits".into())] + ); + assert_file_catalog_and_journal(&s, &fs, &store); + file_history_step(&mut s, &fs, false).unwrap(); + assert_eq!(fs.listing(), before); + file_history_step(&mut s, &fs, true).unwrap(); + assert_eq!(fs.listing(), after); + assert_file_catalog_and_journal(&s, &fs, &store); +} + +#[test] +fn initial_sidecar_rollback_failure_keeps_a_repair_only_history_step() { + use crate::rename::tests::FakeFs; + let store = lightcraft_catalog::MemStore::new(); + let mut s = memory_library(&store); + let fs = FakeFs::new(false, &[("/p/a.jpg", "Photo"), ("/p/a.XMP", "Stem edits"), ("/p/a.jpg.XMP", "Full edits")]); + let photo = Photo::new(PhotoId(1), Source::File { path: "/p/a.jpg".into() }, "a.jpg", "JPEG", 16, 16, ""); + s.commit("Synthetic File", Op::AddPhoto { photo: Box::new(photo) }).unwrap(); + s.persist().unwrap(); + s.undo.clear(); + select(&mut s, &[1], 1); + let before = fs.listing(); + let plans = s.plan_rename_with(&fs, &[PhotoId(1)], "Trip", 1); + *fs.fail.borrow_mut() = vec!["/p/a.jpg.XMP".into(), "/p/Trip.XMP".into()]; + assert!(s.execute_fn("photo.rename", |s| Ok(json!({"renamed": s.apply_rename_with(&fs, &plans)?}))).is_err()); + assert_eq!(s.catalog.photo(PhotoId(1)).unwrap().source, Source::File { path: "/p/a.jpg".into() }); + assert_eq!(s.undo.len(), 1); + assert_eq!(s.undo.last().unwrap().retry_sidecars.len(), 1); + assert_file_catalog_and_journal(&s, &fs, &store); + let partial = fs.listing(); + *fs.fail.borrow_mut() = vec!["/p/Trip.XMP".into()]; + assert!(file_history_step(&mut s, &fs, false).is_err()); + assert_eq!(fs.listing(), partial); + assert_eq!(s.undo.last().unwrap().retry_sidecars.len(), 1); + fs.fail.borrow_mut().clear(); + file_history_step(&mut s, &fs, false).unwrap(); + assert_eq!(fs.listing(), before); + assert_file_catalog_and_journal(&s, &fs, &store); + file_history_step(&mut s, &fs, true).unwrap(); + assert_eq!(fs.listing(), before, "redo of the repair-only step must not rename healthy files"); + assert_file_catalog_and_journal(&s, &fs, &store); +} + +#[test] +fn discarded_file_history_keeps_sidecar_repairs_without_stale_catalog_inverses() { + use crate::rename::MoveFs; + use crate::sidecar::{SidecarNaming, sidecar_path}; + + for redo in [false, true] { + for naming in [SidecarNaming::Stem, SidecarNaming::Full] { + let (mut s, fs, store) = sidecar_history_fixture(redo); + let moves = s.file_moves(&sidecar_history_entry(&s, redo).op); + let (from, to) = moves[0].clone(); + let failing = sidecar_path(&from, naming).with_extension("XMP"); + *fs.fail.borrow_mut() = vec![failing.to_string_lossy().into_owned(), to.clone()]; + assert!(file_history_step(&mut s, &fs, redo).is_err()); + let pending = sidecar_history_entry(&s, redo).retry_sidecars.clone(); + assert!(!sidecar_history_entry(&s, redo).retry_inverse.is_empty()); + // A new edit discards Redo. Enough edits also evict an unresolved Undo entry. + for i in 0..if redo { 1 } else { 1000 } { + s.execute("photo.rate", &json!({"ids": [2], "rating": i % 5 + 1})).unwrap(); + } + assert!(s.redo.is_empty()); + assert_eq!(s.pending_sidecar_repairs, pending); + assert!(s.undo.iter().all(|e| e.retry_inverse.is_empty() && e.retry_sidecars.is_empty())); + let partial = fs.listing(); + // Metadata history remains usable without write access to the separated XMP. + file_history_step(&mut s, &fs, false).unwrap(); + file_history_step(&mut s, &fs, true).unwrap(); + assert_eq!(s.pending_sidecar_repairs, pending); + assert_eq!(fs.listing(), partial); + assert_eq!(s.catalog.photo(PhotoId(1)).unwrap().source, Source::File { path: to.clone() }); + assert_file_catalog_and_journal(&s, &fs, &store); + + let plans = s.plan_rename_with(&fs, &[PhotoId(1)], "Next", 1); + let history = (s.undo.len(), s.redo.len()); + assert!(s.apply_rename_with(&fs, &plans).is_err()); + assert_eq!(fs.listing(), partial); + assert_eq!((s.undo.len(), s.redo.len()), history); + assert_eq!(s.pending_sidecar_repairs, pending); + // Retain only unfinished work after partial progress; never overwrite a collision. + let last = pending.last().unwrap().clone(); + *fs.fail.borrow_mut() = vec![last.from.to_string_lossy().into_owned()]; + assert!(s.apply_rename_with(&fs, &plans).is_err()); + assert_eq!(s.pending_sidecar_repairs.len(), 1); + crate::rename::tests::assert_repair_packets( + &s.pending_sidecar_repairs[0], + &last, + &[ + (sidecar_path(&to, SidecarNaming::Stem).with_extension("XMP"), b"Upper stem edits"), + (sidecar_path(&to, SidecarNaming::Stem), b"Lower stem edits"), + (sidecar_path(&to, SidecarNaming::Full).with_extension("XMP"), b"Upper full-name edits"), + (last.from.clone(), b"Lower full-name edits"), + ], + ); + // Successful repairs advance companion paths while retaining their content versions. + let last = s.pending_sidecar_repairs[0].clone(); + fs.fail.borrow_mut().clear(); + fs.files.borrow_mut().push((last.to.to_string_lossy().into_owned(), b"Other edits".to_vec())); + let occupied = fs.listing(); + assert!(s.apply_rename_with(&fs, &plans).is_err()); + assert_eq!(fs.listing(), occupied); + assert_eq!(s.pending_sidecar_repairs, vec![last.clone()]); + fs.remove_file(&last.to).unwrap(); + + // A no-op Rename still repairs the detached sidecars, without a new undo step. + let noop = s.plan_rename_with(&fs, &[PhotoId(1)], "{name}", 1); + assert_eq!(s.apply_rename_with(&fs, &noop).unwrap(), 0); + assert!(s.pending_sidecar_repairs.is_empty()); + assert_eq!((s.undo.len(), s.redo.len()), history); + let repaired = fs.listing(); + assert_file_catalog_and_journal(&s, &fs, &store); + s.execute_fn("photo.rename", |s| Ok(json!({"renamed": s.apply_rename_with(&fs, &plans)?}))).unwrap(); + let renamed = fs.listing(); + file_history_step(&mut s, &fs, false).unwrap(); + assert_eq!(fs.listing(), repaired, "new history restores the current name, not the discarded inverse"); + file_history_step(&mut s, &fs, true).unwrap(); + assert_eq!(fs.listing(), renamed); + assert_file_catalog_and_journal(&s, &fs, &store); + } + } +} + +#[test] +fn merged_rename_failure_keeps_rolled_back_photo_identity() { + use crate::rename::tests::FakeFs; + + let store = lightcraft_catalog::MemStore::new(); + let mut s = memory_library(&store); + let fs = FakeFs::new(false, &[("/p/a.jpg", "Photo 1"), ("/p/a.xmp", "Photo 1 edits"), ("/p/b.jpg", "Photo 2")]); + for (id, path) in [(1, "/p/a.jpg"), (2, "/p/b.jpg")] { + let name = std::path::Path::new(path).file_name().unwrap().to_string_lossy(); + let photo = Photo::new(PhotoId(id), Source::File { path: path.into() }, &name, "JPEG", 16, 16, ""); + s.commit("Synthetic File", Op::AddPhoto { photo: Box::new(photo) }).unwrap(); + } + s.persist().unwrap(); + s.undo.clear(); + for (id, name) in [(1, "c"), (2, "a"), (2, "d")] { + select(&mut s, &[id], id); + let plans = s.plan_rename_with(&fs, &[PhotoId(id)], name, 1); + s.execute_fn("photo.rename", |s| Ok(json!({"renamed": s.apply_rename_with(&fs, &plans)?}))).unwrap(); + } + assert_eq!( + fs.listing(), + vec![ + ("/p/c.jpg".to_string(), "Photo 1".to_string()), + ("/p/c.xmp".to_string(), "Photo 1 edits".to_string()), + ("/p/d.jpg".to_string(), "Photo 2".to_string()), + ] + ); + s.merge_undo(3, "Rename Three Times"); + assert_eq!((s.undo.len(), s.redo.len()), (1, 0)); + assert_eq!(s.file_moves(&s.undo.last().unwrap().op), vec![("/p/d.jpg".into(), "/p/b.jpg".into()), ("/p/c.jpg".into(), "/p/a.jpg".into())]); + // The second photo move sticks; the preceding d -> b move must roll back to d. + *fs.fail.borrow_mut() = vec!["/p/c.xmp".into(), "/p/a.jpg".into()]; + let error = file_history_step(&mut s, &fs, false).unwrap_err().to_string(); + assert!(error.contains("rename /p/c.xmp: injected failure"), "{error}"); + assert!(error.contains("/p/c.jpg could not be moved back"), "{error}"); + let files = fs.listing(); + assert_eq!( + files, + vec![ + ("/p/a.jpg".to_string(), "Photo 1".to_string()), + ("/p/c.xmp".to_string(), "Photo 1 edits".to_string()), + ("/p/d.jpg".to_string(), "Photo 2".to_string()), + ] + ); + assert_eq!((s.undo.len(), s.redo.len()), (1, 0)); + assert_eq!(s.catalog.photo(PhotoId(1)).unwrap().source, Source::File { path: "/p/a.jpg".into() }); + let photo2 = s.catalog.photo(PhotoId(2)).unwrap(); + assert_eq!(photo2.source, Source::File { path: "/p/d.jpg".into() }, "photo 2 rolled back physically and must still identify d.jpg"); + let Source::File { path } = &photo2.source else { panic!("file fixture") }; + assert_eq!(files.iter().find(|(file, _)| file == path).map(|(_, bytes)| bytes.as_str()), Some("Photo 2")); +} diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index b87f1fa6..7aa623af 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -1,6 +1,6 @@ # Lightroom parity checklist -> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-11 · **Change:** minor (rapid slider entry; LR-PROF-CAMERACOLOR: tone curve from black-and-white camera JPEGs; LR-VIEW-ZOOM, LR-VIEW-COMPARE: Compare and Reference zoom windows; LR-IMP-FORMATS, LR-IMP-CAMERA-COVERAGE: EOS R5 Mark II CR3 decodes) · **Target:** Adobe Lightroom Classic 15.5 / Lightroom 9.6 +> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-11 · **Change:** minor (rapid slider entry; LR-PROF-CAMERACOLOR: tone curve from black-and-white camera JPEGs; LR-VIEW-ZOOM, LR-VIEW-COMPARE: Compare and Reference zoom windows; LR-IMP-FORMATS, LR-IMP-CAMERA-COVERAGE: EOS R5 Mark II CR3 decodes; renamed from `parity.md`; Top gaps moved to gaps.md, shortcut notes to ui-parity.md; filesystem history retry/Undo/Redo evidence) · **Target:** Adobe Lightroom Classic 15.5 / Lightroom 9.6 The measured, row-by-row checklist: one row per Lightroom feature, menu item and shortcut, what we have and what is missing. It answers *does the feature exist?* (feature breadth). Whether it is good @@ -448,7 +448,7 @@ differences and missing bindings are in [ui-parity.md](ui-parity.md#shortcuts). | Id | Feature | Tier | Status | Evidence | Missing / notes | |---|---|---|---|---|---| | LR-BEHAV-AUTOSAVE | Instant autosave | P0 | ✅ | `crates/catalog/src/journal.rs`, `crates/engine/src/library.rs` | every change fsynced before the command returns; a failed write fails the command ("saved in memory but not written to disk"), keeps the change queued and retries; top-bar warning until saved; a write that fails part-way is cut back to the last whole record so the retry never reads back as damage (logs from older builds with such a fragment load in full); quitting saves queued changes in the closing snapshot even when the log can't be appended to; versioned catalog format (`journal::VERSION`): older libraries are upgraded on open, a library from a newer LightCraft is refused untouched ("written by a newer version") | -| LR-BEHAV-UNDO | Global undo | P0 | ✅ | `cmd:edit.undo`, `crates/engine/src/tests.rs` (`rating_flag_undo_redo`) | covers ratings, albums, deletes, edits | +| LR-BEHAV-UNDO | Global undo | P0 | ✅ | `cmd:edit.undo`, `crates/engine/src/tests.rs` (`rating_flag_undo_redo`), `crates/engine/src/tests_filesystem_history.rs` | covers ratings, albums, deletes, edits; partial file/XMP moves keep unfinished repairs across retries and discarded history; merged renames move directly to their final catalog path | | LR-BEHAV-MULTISELECT | Multi-selection | P0 | ✅ | `cmd:library.select` (`replace`/`add`/`toggle`/`range`), `cmd:library.selectAll` | | | LR-BEHAV-BATCH | Batch apply to selection | P0 | ✅ | `cmd:photo.rate`, `cmd:develop.paste`, `cmd:preset.apply`, `cmd:app.export` | | | LR-BEHAV-PREVIEW-HOVER | Hover previews | P1 | ✅ | `crates/ui-egui/src/panels/presets.rs`, `crates/ui-egui/src/panels/profiles.rs`, `crates/ui-egui/src/panels/right.rs` (`versions`), `crates/ui-egui/src/panels/detail.rs` | presets, profile menu, profile browser and versions preview in the loupe | @@ -784,5 +784,6 @@ Latin stays upright. Advanced Japanese composition remains open. Regression cove | 2026-10-11 | minor | LR-PROF-CAMERACOLOR: raws with a black-and-white camera JPEG take its tone curve (issue #577) | | 2026-10-11 | minor | LR-VIEW-ZOOM and LR-VIEW-COMPARE: Compare and Reference are sharp at 1:1 (zoom windows per pane, two originals kept decoded while they are open); statuses unchanged | | 2026-10-11 | minor | LR-IMP-FORMATS and LR-IMP-CAMERA-COVERAGE: EOS R5 Mark II CR3 decodes from sensor data (57% of its C-RAW files opened as a preview: QP-map escape); status unchanged (🟡) | +| 2026-10-10 | patch | LR-BEHAV-UNDO: file/XMP repair retries and merged rename Undo/Redo evidence; status and summary counts unchanged. | | 2026-10-10 | minor | Renamed from `parity.md` (progress-docs standard); Top gaps moved to gaps.md (its duplicated items 1–2 from a bad merge resolved there), shortcut notes moved to ui-parity.md. Rows unchanged; a recount of the rows gives 81.1% of 517 in-scope rows (P0 98.0%, P1 95.1%, P2 46.9%), so the generated summary above is due a `cargo xtask parity --write` | | 2026-09-30 – 2026-10-10 | — | Rows added and updated with each feature (see `git log -- docs/parity.md docs/parity-checklist.md`) |