From f4df914731f0d77bcfe265f424e515ff413375a8 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Mon, 5 Oct 2026 17:12:25 -0400 Subject: [PATCH] Issue #92: Rename Photos copies a stem sidecar another file still shares Renaming IMG_1.CR3 alone took the shared IMG_1.xmp along and left IMG_1.JPG without it. A Stem-named sidecar that a sibling photo file still uses is now copied (independently, never linked) instead of moved; the last file renamed takes it. A rollback removes the copy. Stacked on fix/issue-105-rename-rollback. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/import_move.rs | 2 +- crates/engine/src/rename.rs | 94 +++++++++++++++++++++++++++++--- docs/parity.md | 2 +- docs/whats-new.md | 3 +- 4 files changed, 91 insertions(+), 10 deletions(-) diff --git a/crates/engine/src/import_move.rs b/crates/engine/src/import_move.rs index 3c949e79..14697c0d 100644 --- a/crates/engine/src/import_move.rs +++ b/crates/engine/src/import_move.rs @@ -213,7 +213,7 @@ pub(crate) fn finish(p: &Placed) -> Result, String> { } /// A photo (by extension) still beside a stem-named sidecar, sharing its stem. -fn sibling_photo(sidecar: &Path) -> Option { +pub(crate) fn sibling_photo(sidecar: &Path) -> Option { let dir = sidecar.parent()?; let stem = sidecar.file_stem()?.to_string_lossy().to_string(); fs::read_dir(dir).ok()?.flatten().map(|e| e.path()).find_map(|f| { diff --git a/crates/engine/src/rename.rs b/crates/engine/src/rename.rs index 6e363ed3..511aa8b4 100644 --- a/crates/engine/src/rename.rs +++ b/crates/engine/src/rename.rs @@ -313,6 +313,12 @@ pub(crate) trait MoveFs { fn same_file(&self, a: &Path, b: &Path) -> bool; /// Rename `a` to `b`, failing (never replacing) when `b` exists. fn rename_no_replace(&self, a: &Path, b: &Path) -> std::io::Result<()>; + /// Copy `a` to a new file `b` (an independent copy, never a link), failing when `b` exists. + fn copy_no_replace(&self, a: &Path, b: &Path) -> std::io::Result<()>; + fn remove_file(&self, p: &Path) -> std::io::Result<()>; + /// Another photo file beside the stem-named sidecar `sidecar` that shares its stem (e.g. + /// `IMG_1.JPG` for `IMG_1.xmp`), which still uses that sidecar. + fn stem_sibling(&self, sidecar: &Path) -> Option; } /// The real file system. @@ -367,6 +373,23 @@ impl MoveFs for RealFs { Err(_) => std::fs::rename(a, b), } } + + fn copy_no_replace(&self, a: &Path, b: &Path) -> std::io::Result<()> { + use std::io::Write; + let bytes = std::fs::read(a)?; + let mut f = std::fs::OpenOptions::new().write(true).create_new(true).open(b)?; + f.write_all(&bytes).and_then(|_| f.sync_all()).inspect_err(|_| { + let _ = std::fs::remove_file(b); + }) + } + + fn remove_file(&self, p: &Path) -> std::io::Result<()> { + std::fs::remove_file(p) + } + + fn stem_sibling(&self, sidecar: &Path) -> Option { + crate::import_move::sibling_photo(sidecar) + } } /// `from` → `to` differs only in letter case and `to` is that very file (a case-insensitive @@ -427,7 +450,8 @@ pub(crate) fn move_file_with(fs: &dyn MoveFs, from: &str, to: &str) -> std::resu return Err(fail(format!("{to} already exists"))); } move_one(fs, f, t).map_err(|e| fail(format!("rename {from}: {e}")))?; - let mut done: Vec<(PathBuf, PathBuf)> = Vec::new(); // sidecars moved + // 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; @@ -436,28 +460,35 @@ pub(crate) fn move_file_with(fs: &dyn MoveFs, from: &str, to: &str) -> std::resu if upper { dst = dst.with_extension("XMP"); } - if fs.exists(&dst) && !is_respelling(fs, &sc, &dst) { + // 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)) { continue; // never overwrite: the old sidecar stays where it was } - if let Err(e) = move_one(fs, &sc, &dst) { + 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 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(|(_, b, _)| b.display().to_string()).collect(); message.push_str(&format!(" with its sidecar {}", names.join(", "))); } return Err(MoveError { message, moved: true }); } - for (a, b) in done.iter().rev() { - if let Err(e) = move_one(fs, b, a) { + 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())); } } return Err(fail(message)); } - done.push((sc, dst)); + done.push((sc, dst, shared)); } Ok(()) } @@ -747,6 +778,27 @@ mod tests { self.files.borrow_mut()[i].0 = b.to_string_lossy().to_string(); Ok(()) } + fn copy_no_replace(&self, a: &Path, b: &Path) -> std::io::Result<()> { + if self.exists(b) { + return Err(std::io::Error::new(std::io::ErrorKind::AlreadyExists, "exists")); + } + let i = self.idx(a).ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, "no such file"))?; + let c = self.files.borrow()[i].1.clone(); + self.files.borrow_mut().push((b.to_string_lossy().to_string(), c)); + Ok(()) + } + fn remove_file(&self, p: &Path) -> std::io::Result<()> { + let i = self.idx(p).ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, "no such file"))?; + self.files.borrow_mut().remove(i); + Ok(()) + } + fn stem_sibling(&self, sidecar: &Path) -> Option { + let stem = sidecar.with_extension("").to_string_lossy().to_string(); + self.files.borrow().iter().map(|(f, _)| f.clone()).find(|f| { + let p = Path::new(f); + p.with_extension("").to_string_lossy() == stem && !p.extension().is_some_and(|e| e.eq_ignore_ascii_case("xmp")) + }) + } } fn file_photo(id: u64, path: &str) -> Photo { @@ -849,6 +901,34 @@ mod tests { assert!(fs.listing().iter().all(|(p, _)| p.starts_with("/p/d."))); } + /// 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] + fn shared_stem_sidecar_is_copied_not_taken() { + let fs = FakeFs::new(false, &[("/p/IMG_1.CR3", "raw"), ("/p/IMG_1.JPG", "jpg"), ("/p/IMG_1.xmp", "shared"), ("/p/IMG_1.CR3.xmp", "own")]); + move_file_with(&fs, "/p/IMG_1.CR3", "/p/Trip_1.CR3").unwrap(); + assert_eq!( + fs.listing(), + vec![ + ("/p/IMG_1.JPG".into(), "jpg".into()), + ("/p/IMG_1.xmp".into(), "shared".into()), + ("/p/Trip_1.CR3".into(), "raw".into()), + ("/p/Trip_1.CR3.xmp".into(), "own".into()), + ("/p/Trip_1.xmp".into(), "shared".into()), + ] + ); + // the JPEG is the last one using it: it moves + move_file_with(&fs, "/p/IMG_1.JPG", "/p/Trip_2.JPG").unwrap(); + assert!(fs.listing().contains(&("/p/Trip_2.xmp".into(), "shared".into()))); + assert!(!fs.exists(Path::new("/p/IMG_1.xmp"))); + // a failure after the copy removes the copy and keeps the original + let fs = FakeFs::new(false, &[("/p/A.CR3", "raw"), ("/p/A.JPG", "jpg"), ("/p/A.xmp", "shared"), ("/p/A.CR3.xmp", "own")]); + fs.fail.borrow_mut().push("A.CR3.xmp".into()); + assert!(move_file_with(&fs, "/p/A.CR3", "/p/B.CR3").is_err()); + assert_eq!(fs.listing().len(), 4); + assert!(fs.listing().contains(&("/p/A.xmp".into(), "shared".into())) && !fs.exists(Path::new("/p/B.xmp"))); + } + /// Issue #95 on the real disk, whichever kind of volume the temp folder is on. #[test] fn case_only_rename_on_disk() { diff --git a/docs/parity.md b/docs/parity.md index 2b864225..d0029603 100644 --- a/docs/parity.md +++ b/docs/parity.md @@ -153,7 +153,7 @@ under the UI command that wraps it (a no-op error); the UI command now wins. | LR-LIB-DELETE | Delete / Recently Deleted | P0 | ✅ | `cmd:photo.delete`, `cmd:photo.restore`, `cmd:photo.deletePermanently` | no confirmation dialog, no auto-purge after N days, no "Empty" | | LR-LIB-REMOVE-ALBUM | Remove from album | P0 | ✅ | `cmd:album.removePhotos` | | | LR-LIB-DUPLICATE | Duplicate a photo | P2 | ✅ | `cmd:photo.duplicate` | Photo ▸ Duplicate: a real `-copy` file with the same settings, metadata and albums (virtual copies share the file) | -| LR-LIB-RENAME | Batch rename | P1 | ✅ | `cmd:photo.rename`, `cmd:photo.renamePreview`, `cmd:photo.renameTokens`, `cmd:dialog.rename`, `crates/engine/src/rename.rs` (`TOKENS`) | template tokens {name} {num} {seq:N} {date:%Y%m%d} {folder} {camera} {lens} {iso} {rating} {title} {creator} {ext} (shared with export naming and import rename); a Tags picker beside every template field (Rename Photos, import Rename, export File name) lists each tag with meaning + live example, the date directives and the rules, and inserts the clicked tag at the text cursor; unknown tags are flagged; preview; renames files on disk with their XMP sidecars, never overwriting (-1, -2… suffixes; a change of letter case only is checked by file identity, so on case-sensitive volumes `img_1.jpg` next to `IMG_1.JPG` is a collision), rolls back on failure (a file that can't be moved back is reported old → new and the library follows it as an undoable partial rename; the same for undo/redo); undo/redo move the files; virtual copies follow | +| LR-LIB-RENAME | Batch rename | P1 | ✅ | `cmd:photo.rename`, `cmd:photo.renamePreview`, `cmd:photo.renameTokens`, `cmd:dialog.rename`, `crates/engine/src/rename.rs` (`TOKENS`) | template tokens {name} {num} {seq:N} {date:%Y%m%d} {folder} {camera} {lens} {iso} {rating} {title} {creator} {ext} (shared with export naming and import rename); a Tags picker beside every template field (Rename Photos, import Rename, export File name) lists each tag with meaning + live example, the date directives and the rules, and inserts the clicked tag at the text cursor; unknown tags are flagged; preview; renames files on disk with their XMP sidecars (a stem sidecar another file still shares, e.g. raw + JPEG, is copied rather than taken), never overwriting (-1, -2… suffixes; a change of letter case only is checked by file identity, so on case-sensitive volumes `img_1.jpg` next to `IMG_1.JPG` is a collision), rolls back on failure (a file that can't be moved back is reported old → new and the library follows it as an undoable partial rename; the same for undo/redo); undo/redo move the files; virtual copies follow | | LR-LIB-CAPTURETIME | Edit capture time | P1 | ✅ | `cmd:photo.setCaptureTime`, `cmd:dialog.captureTime`, `crates/catalog/src/dates.rs` | set (the other selected photos shift by the same amount, or `each`), shift by days/hours/minutes, time-zone shift; one undo step, journaled; Info panel button; no “revert to original capture time” | | LR-LIB-SHOWFINDER | Reveal original in file manager | P0 | ✅ | `cmd:app.showInFinder` | ⌘R (see MENU-FILE-SHOWFINDER) | | LR-LIB-COVER | Album cover | P2 | ✅ | `cmd:album.setCover` | | diff --git a/docs/whats-new.md b/docs/whats-new.md index 524f77ae..d94b6dd4 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -26,7 +26,8 @@ `img_1-1.JPG`; on case-insensitive volumes the case change still goes through. - Rename Photos reports files it could not move back after a failure (issue #105), e.g. when a network share drops mid-batch: the error lists them (old → new) and the library points at their new names (an undoable partial rename), - so none shows as missing. Find Missing Photos also finds renamed files by their content, prefers a content match + so none shows as missing. Renaming one of a raw + JPEG pair copies their shared `IMG_0001.xmp` instead of taking + it away from the other (issue #92). Find Missing Photos also finds renamed files by their content, prefers a content match over a same-name same-size look-alike, and skips (and reports) photos it can't tell apart instead of guessing. - Smart albums with a rule editor: match all / any / none, nested groups, 26 fields. - Quick Collection and target album (B in the grid), keyword sets (⌥1–⌥9), colour-label sets.