Do not read a sibling's shared stem sidecar for a non-owner photo (#1523)

crates/engine/src/cmd/query.rs

@@ -130,7 +130,7 @@ pub fn specs() -> Vec<CommandSpec> {
                 .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<Value> = 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())

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<PathBuf> {
-    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<PathBuf> {
+    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<SidecarLookup> {
+        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<Option<(Op, PathBuf, Vec<crate::import::ImportWarning>)>> {
         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 {

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<Photo>, SidecarNaming)>,
+    photos: Vec<(Arc<Photo>, 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<Photo>, SidecarNaming)], labels: &Catalog, progress: &SyncProgress) -> Vec<Check> {
+fn check_photos(photos: &[(Arc<Photo>, SidecarLookup)], labels: &Catalog, progress: &SyncProgress) -> Vec<Check> {
     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<FolderMoved>,
     missing: Vec<FolderPhoto>,
     /// (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::<Vec<_>>());
+            let namings = s.sidecar_lookups(&metadata.iter().map(|m| PhotoId(m.id)).collect::<Vec<_>>());
             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<crate::sidecar::SidecarData, String> {
-    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<crate::sidecar::SidecarData, String> {
+    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()));

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);
+}