From 55e7c335fd9170302e89f86bdccbe84884aec9a7 Mon Sep 17 00:00:00 2001 From: MarekSpodBiedry <73122968+MarekSpodBiedry@users.noreply.github.com> Date: Tue, 13 Oct 2026 16:36:05 +0200 Subject: [PATCH] Do not read a sibling's shared stem sidecar for a non-owner photo (#1523) --- crates/engine/src/cmd/query.rs | 2 +- crates/engine/src/sidecar.rs | 50 ++++++++++++++++++++++++++++++---- crates/engine/src/sync.rs | 24 ++++++++-------- crates/engine/src/tests_xmp.rs | 30 ++++++++++++++++++++ 4 files changed, 88 insertions(+), 18 deletions(-) diff --git a/crates/engine/src/cmd/query.rs b/crates/engine/src/cmd/query.rs index 2e0b6f1579..5e02f0967d 100644 --- a/crates/engine/src/cmd/query.rs +++ b/crates/engine/src/cmd/query.rs @@ -130,7 +130,7 @@ pub fn specs() -> Vec { .map(|r| json!({"group": r.group, "tag": r.tag, "name": r.name, "value": r.value})) .collect(); // XMP: the sidecar if there is one, else the file's own packet - let packet = crate::sidecar::read_packet(path, ph.kind, s.sidecar_naming(ph.id)).map(|(x, _)| x).or_else(|| lightcraft_meta::embedded(&bytes).xmp); + let packet = crate::sidecar::read_packet(path, ph.kind, s.sidecar_lookup(ph.id)).map(|(x, _)| x).or_else(|| lightcraft_meta::embedded(&bytes).xmp); let xmp: Vec = packet .and_then(|x| lightcraft_meta::parse_xmp(&x).ok()) .map(|d| d.properties.into_iter().filter(|(k, _)| !k.starts_with("lc:")).map(|(k, v)| json!({"name": k, "value": v.join("; ")})).collect()) diff --git a/crates/engine/src/sidecar.rs b/crates/engine/src/sidecar.rs index 9aaf7e937a..9ae8122e74 100644 --- a/crates/engine/src/sidecar.rs +++ b/crates/engine/src/sidecar.rs @@ -62,12 +62,27 @@ pub fn sidecar_path(original: &str, naming: SidecarNaming) -> PathBuf { } } +/// Where to look for a catalogued photo's sidecar. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct SidecarLookup { + pub naming: SidecarNaming, + /// Whether a shared stem sidecar (`IMG_0001.xmp`) may be read for this photo. False when + /// another catalogued file with the same stem owns it (see [`Session::sidecar_naming`]). + pub stem_ok: bool, +} + /// An existing sidecar for `original`: the preferred naming first, then the other one (also /// accepting an upper-case `.XMP`). pub fn find_sidecar(original: &str, naming: SidecarNaming) -> Option { - let other = if naming == SidecarNaming::Stem { SidecarNaming::Full } else { SidecarNaming::Stem }; - [naming, other] + find_sidecar_in(original, SidecarLookup { naming, stem_ok: true }) +} + +/// [`find_sidecar`] for a catalogued photo: never a stem sidecar that a sibling owns. +pub fn find_sidecar_in(original: &str, lookup: SidecarLookup) -> Option { + let other = if lookup.naming == SidecarNaming::Stem { SidecarNaming::Full } else { SidecarNaming::Stem }; + [lookup.naming, other] .into_iter() + .filter(|n| *n != SidecarNaming::Stem || lookup.stem_ok) .flat_map(|n| { let p = sidecar_path(original, n); let upper = p.with_extension("XMP"); @@ -458,8 +473,8 @@ fn file_path(p: &Photo) -> Option<&str> { } /// The XMP packet that describes a photo on disk: its sidecar, else (raw/DNG) the embedded XMP. -pub fn read_packet(original: &str, kind: MediaKind, naming: SidecarNaming) -> Option<(String, PathBuf)> { - if let Some(sc) = find_sidecar(original, naming) +pub fn read_packet(original: &str, kind: MediaKind, lookup: SidecarLookup) -> Option<(String, PathBuf)> { + if let Some(sc) = find_sidecar_in(original, lookup) && let Ok(s) = std::fs::read_to_string(&sc) { return Some((s, sc)); @@ -512,6 +527,12 @@ impl StemOwners { _ => naming, } } + + /// [`StemOwners::naming`], plus whether the stem sidecar may be read for `p`. + fn lookup(&self, p: &Photo, naming: SidecarNaming) -> SidecarLookup { + let stem_ok = file_path(p).is_none_or(|path| self.0.get(&stem_key(path)).is_none_or(|owner| *owner == p.id)); + SidecarLookup { naming: self.naming(p, naming), stem_ok } + } } impl Session { @@ -531,6 +552,25 @@ impl Session { .collect() } + /// [`Session::sidecar_naming`] and the stem-sidecar ownership, for reading `ids`' sidecars. + pub fn sidecar_lookups(&self, ids: &[PhotoId]) -> Vec { + let owners = StemOwners::of(&self.catalog); + ids.iter() + .map(|id| match self.catalog.photo(*id) { + Some(p) => owners.lookup(p, self.xmp.naming), + None => SidecarLookup { naming: self.xmp.naming, stem_ok: true }, + }) + .collect() + } + + pub fn sidecar_lookup(&self, id: PhotoId) -> SidecarLookup { + let owners = StemOwners::of(&self.catalog); + match self.catalog.photo(id) { + Some(p) => owners.lookup(p, self.xmp.naming), + None => SidecarLookup { naming: self.xmp.naming, stem_ok: true }, + } + } + pub fn sidecar_naming(&self, id: PhotoId) -> SidecarNaming { match self.catalog.photo(id) { Some(p) if self.xmp.naming == SidecarNaming::Stem => StemOwners::of(&self.catalog).naming(p, self.xmp.naming), @@ -590,7 +630,7 @@ impl Session { pub fn read_sidecar_op_with_warnings(&self, id: PhotoId) -> Result)>> { let p = self.catalog.photo(id).ok_or(lightcraft_catalog::CatalogError::NoPhoto(id))?; let Some(orig) = file_path(p) else { return Ok(None) }; - let Some((packet, from)) = read_packet(orig, p.kind, self.sidecar_naming(id)) else { return Ok(None) }; + let Some((packet, from)) = read_packet(orig, p.kind, self.sidecar_lookup(id)) else { return Ok(None) }; let sc = parse_sidecar(&packet, crate::crs::Target::for_photo(p)).map_err(|e| EngineError::Other(format!("{}: {e}", from.display())))?; let mut warnings = sc.warnings.clone(); for warning in &mut warnings { diff --git a/crates/engine/src/sync.rs b/crates/engine/src/sync.rs index 0a00a59abd..3d091d5e2f 100644 --- a/crates/engine/src/sync.rs +++ b/crates/engine/src/sync.rs @@ -46,7 +46,7 @@ use serde::Serialize; use crate::import::{ImportCandidate, ImportMode, ImportOptions, ScanInput, ScanProgress}; use crate::media::ProbeInfo; -use crate::sidecar::SidecarNaming; +use crate::sidecar::SidecarLookup; use crate::{EngineError, Result, Session}; /// The largest XMP sidecar read when looking for metadata updates; a bigger one is skipped. @@ -75,7 +75,7 @@ pub struct SyncInput { folder: String, scan: ScanInput, /// The library's photos in the folder, each with how its sidecar is named. - photos: Vec<(Arc, SidecarNaming)>, + photos: Vec<(Arc, SidecarLookup)>, /// For the colour label names a sidecar may use. labels: Catalog, } @@ -176,7 +176,7 @@ impl SyncInput { if !disk && (lightcraft_catalog::folders::is_disk_root(path) || crate::cmd::library::covers_other_disks(s, path, &ids)) { return Err(invalid(format!("{path} is a whole disk or holds disks: pass `disk: true` to synchronize it"))); } - let namings = s.sidecar_namings(&ids); + let namings = s.sidecar_lookups(&ids); let photos = ids.iter().zip(namings).filter_map(|(id, naming)| s.catalog.photo(*id).map(|p| (Arc::clone(p), naming))).collect(); let (scan, _) = ScanInput::new(s, &[path.to_string()]); Ok(SyncInput { folder: path.to_string(), scan, photos, labels: s.catalog.clone() }) @@ -327,7 +327,7 @@ enum Check { News, } -fn check_photo(p: &Photo, naming: SidecarNaming, labels: &Catalog) -> Check { +fn check_photo(p: &Photo, naming: SidecarLookup, labels: &Catalog) -> Check { let Some(file) = crate::cmd::missing::checked_path(p) else { return Check::Nothing }; if !Path::new(file).exists() { Check::Missing @@ -340,11 +340,11 @@ fn check_photo(p: &Photo, naming: SidecarNaming, labels: &Catalog) -> Check { /// [`check_photo`] for each photo, in order, on several threads (each file is a wait on the /// disk, not work for the CPU). Photos not reached before a cancel count as nothing found. -fn check_photos(photos: &[(Arc, SidecarNaming)], labels: &Catalog, progress: &SyncProgress) -> Vec { +fn check_photos(photos: &[(Arc, SidecarLookup)], labels: &Catalog, progress: &SyncProgress) -> Vec { use std::sync::atomic::Ordering::Relaxed; progress.to_check.store(photos.len(), Relaxed); let cancel = &progress.files.cancel; - let check = |p: &Photo, naming: SidecarNaming| { + let check = |p: &Photo, naming: SidecarLookup| { let c = check_photo(p, naming, labels); progress.checked.fetch_add(1, Relaxed); c @@ -413,9 +413,9 @@ impl Session { /// Whether `p`'s XMP sidecar file was saved after the library last had its say about the photo /// (imported, or edited here) and says something the library doesn't. -fn sidecar_has_news(labels: &Catalog, p: &Photo, naming: SidecarNaming) -> bool { +fn sidecar_has_news(labels: &Catalog, p: &Photo, naming: SidecarLookup) -> bool { let Source::File { path } = &p.source else { return false }; - let Some(file) = crate::sidecar::find_sidecar(path, naming) else { return false }; + let Some(file) = crate::sidecar::find_sidecar_in(path, naming) else { return false }; let Ok(meta) = std::fs::metadata(&file) else { return false }; if meta.len() > MAX_SIDECAR_BYTES { return false; @@ -464,7 +464,7 @@ pub struct SyncWork { moved: Vec, missing: Vec, /// (photo, its file, how its sidecar is named) - sidecars: Vec<(PhotoId, String, SidecarNaming, crate::crs::Target)>, + sidecars: Vec<(PhotoId, String, SidecarLookup, crate::crs::Target)>, } /// The catalog half of a synchronize (see [`SyncJob`]). @@ -512,7 +512,7 @@ impl SyncJob { }; let now = import.as_ref().map_or_else(|| (s.clock)(), |(j, _)| j.now().to_string()); let sidecars = if choice.read_metadata { - let namings = s.sidecar_namings(&metadata.iter().map(|m| PhotoId(m.id)).collect::>()); + let namings = s.sidecar_lookups(&metadata.iter().map(|m| PhotoId(m.id)).collect::>()); metadata .into_iter() .zip(namings) @@ -604,8 +604,8 @@ impl SyncWork { } /// Read and parse photo file `path`'s XMP sidecar file. -fn read_sidecar(path: &str, naming: SidecarNaming, target: crate::crs::Target) -> std::result::Result { - let file = crate::sidecar::find_sidecar(path, naming).ok_or_else(|| "no XMP sidecar".to_string())?; +fn read_sidecar(path: &str, naming: SidecarLookup, target: crate::crs::Target) -> std::result::Result { + let file = crate::sidecar::find_sidecar_in(path, naming).ok_or_else(|| "no XMP sidecar".to_string())?; let len = std::fs::metadata(&file).map_err(|e| format!("{}: {e}", file.display()))?.len(); if len > MAX_SIDECAR_BYTES { return Err(format!("{}: too large for an XMP sidecar", file.display())); diff --git a/crates/engine/src/tests_xmp.rs b/crates/engine/src/tests_xmp.rs index 58d0d7a149..d801676389 100644 --- a/crates/engine/src/tests_xmp.rs +++ b/crates/engine/src/tests_xmp.rs @@ -1209,3 +1209,33 @@ fn files_sharing_a_stem_get_their_own_sidecars() { let _ = std::fs::remove_dir_all(&src); let _ = std::fs::remove_dir_all(&lib); } + +/// A file that shares its stem with another catalogued file never reads the stem sidecar the +/// other one owns, whichever naming the library uses. +#[test] +fn a_non_owner_does_not_read_its_siblings_stem_sidecar() { + let src = temp_dir("stem-read"); + let lib = temp_dir("stem-read-lib"); + write_png(&src.join("IMG_1.png"), 1); + let img = lightcraft_raster::Rgba8::from_fn(40, 24, |x, y| [(x * 6) as u8, (y * 9) as u8, 7, 255]); + let jpg = + crate::export::encode_image(&img, &crate::export::ExportOptions { format: crate::export::ExportFormat::Jpeg, ..Default::default() }).unwrap(); + std::fs::write(src.join("IMG_1.jpg"), jpg).unwrap(); + let mut s = Session::new().with_fs(); + s.open_library(&lib, false).unwrap(); + s.execute("library.import", &json!({"paths": [src.to_string_lossy()]})).unwrap(); + let id = |s: &Session, name: &str| s.catalog.photos().find(|p| p.file_name == name).unwrap().id; + let (j, p) = (id(&s, "IMG_1.jpg"), id(&s, "IMG_1.png")); + s.execute("photo.rate", &json!({"ids": [j.0], "rating": 2})).unwrap(); + s.execute("photo.saveMetadataToFile", &json!({"ids": [j.0]})).unwrap(); + assert!(src.join("IMG_1.xmp").is_file()); + assert!(!src.join("IMG_1.png.xmp").exists()); + // the owner reads its packet; the other file has none of its own + assert!(s.read_sidecar_op(j).unwrap().is_some()); + assert!(s.read_sidecar_op(p).unwrap().is_none()); + s.xmp.naming = crate::sidecar::SidecarNaming::Full; + assert!(s.read_sidecar_op(j).unwrap().is_some()); + assert!(s.read_sidecar_op(p).unwrap().is_none()); + let _ = std::fs::remove_dir_all(&src); + let _ = std::fs::remove_dir_all(&lib); +}