collective
Browse / github.com/storytold/lightcraft / patch
LR-MERGE-HDRPANO: a repeated identical HDR merge reuses the verified output
[Bug] Repeating an identical HDR merge writes an orphan DNG and fails to import it **Environment** - Tested revision: `b1883231bdf0af341541de1ace336c6bf530571f` (verified latest upstream main immediately before filing). - macOS on Apple Silicon; local native `lightcraft-cli` debug build with Rust 1.99. - Fresh isolated filesystem library; three independently generated uncompressed linear RGB16 DNGs. Alignment, Auto Settings and stacking disabled; deghost `none`. **Steps to reproduce** 1. In a fresh directory, generate three 128×96 DNGs with constant RGB samples `(8192,16384,24576)`, `(16384,32768,49152)` and `(21845,43690,65535)`. This standalone generator creates the exa
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- emmertex (imported from pull request #3009, open)
- Base
387bed199fca44d76af5aa1c24dd7cc3de4c8db2- Size
- +125 −21 in 2 files
- Review
- pending
- Shared
- 2026-10-28
- Id
4033f20a5ccb6eb3048e69b0
How it went
No other operator has reported using it yet.
Diff
raw · crates/engine/src/merge.rs crates/engine/src/tests_merge.rs
@@ -430,29 +430,72 @@ impl Session { /// apply Auto Settings / the auto crop. Returns `{id, path, …info}`. pub fn finish_merge(&mut self, job: &MergeJob, out: MergeOutput) -> Result<Value> { let first = &job.sources.first().ok_or_else(|| EngineError::Other("nothing merged".into()))?.1;- // a new file under a free name (never replacing one), complete and synced before it appears- let path = lightcraft_catalog::safe_file::write_new_unique(&mut output_names(first, job.kind.suffix()), &out.dng)- .map_err(|e| EngineError::Other(format!("could not write the merged DNG next to {first}: {e}")))?- .to_string_lossy()- .to_string();- let report = crate::import::import(self, std::slice::from_ref(&path), crate::import::ImportMode::Add)?;- let id = report- .imported- .first()- .copied()- .map(PhotoId)- .ok_or_else(|| EngineError::Other(format!("the merged file {path} could not be imported: {:?}", report.failed)))?;- if job.finish.stack {+ let output_hash = lightcraft_preview::hash_bytes(&out.dng).to_string();+ // Import deduplicates by content. Reuse an existing merge only while its live source has+ // matching bytes; stale, missing or deleted rows fail before another file is published.+ let candidates: Vec<(PhotoId, String, bool)> = self+ .catalog+ .photos()+ .filter_map(|p| {+ if p.content_hash.as_deref() != Some(output_hash.as_str()) {+ return None;+ }+ let Source::File { path } = &p.source else { return None };+ Some((p.id, path.clone(), p.deleted))+ })+ .collect();+ let valid_existing = candidates+ .iter()+ .find(|(_, path, deleted)| {+ if *deleted {+ return false;+ }+ let bytes = match &self.media.file_bytes {+ Some(read) => read(path).ok(),+ None => std::fs::read(path).ok(),+ };+ bytes.is_some_and(|bytes| lightcraft_preview::hash_bytes(&bytes).to_string() == output_hash)+ })+ .cloned();+ let (id, path, reused) = if let Some((id, path, _)) = valid_existing {+ (id, path, true)+ } else if let Some((id, path, deleted)) = candidates.first() {+ let state = if *deleted { "deleted" } else { "missing or changed" };+ return Err(EngineError::Other(format!(+ "existing merge result {id:?} has a {state} source at {path}; relink or remove it before repeating the merge"+ )));+ } else {+ // A new file under a free name (never replacing one), complete and synced before it appears.+ let path = lightcraft_catalog::safe_file::write_new_unique(&mut output_names(first, job.kind.suffix()), &out.dng)+ .map_err(|e| EngineError::Other(format!("could not write the merged DNG next to {first}: {e}")))?+ .to_string_lossy()+ .to_string();+ let report = match crate::import::import(self, std::slice::from_ref(&path), crate::import::ImportMode::Add) {+ Ok(report) => report,+ Err(e) => {+ let _ = std::fs::remove_file(&path);+ return Err(e);+ }+ };+ if let Some(id) = report.imported.first().copied().map(PhotoId) {+ (id, path, false)+ } else {+ let _ = std::fs::remove_file(&path);+ return Err(EngineError::Other(format!("the merged file {path} could not be imported: {:?}", report.failed)));+ }+ };+ if !reused && job.finish.stack { let sources: Vec<PhotoId> = job.sources.iter().map(|(p, _)| *p).collect(); if let Some(op) = self.catalog.stack_with_ops(id, &sources) { self.commit("Stack with Sources", op)?; } } self.selection = crate::Selection::single(id);- if job.finish.auto_settings {+ if !reused && job.finish.auto_settings { self.execute("develop.auto", &json!({}))?; }- if let Some(c) = out.crop+ if !reused+ && let Some(c) = out.crop && let Some(d) = self.develop_of(id) { let mut d = (*d).clone();@@ -463,6 +506,7 @@ impl Session { info["id"] = json!(id.0); info["path"] = json!(path); info["bytes"] = json!(out.dng.len());+ info["reused"] = json!(reused); Ok(info) } @@ -40,6 +40,7 @@ fn hdr_merge_command_creates_and_imports_a_dng() { let r = s.execute("merge.hdr", &json!({"ids": ids, "autoSettings": true, "stack": true})).unwrap(); let path = r["path"].as_str().unwrap(); assert!(path.ends_with("IMG_0-HDR.dng"), "{path}");+ assert_eq!(r["reused"].as_bool(), Some(false)); let id = lightcraft_catalog::PhotoId(r["id"].as_u64().unwrap()); let p = s.catalog.photo(id).unwrap().clone(); assert_eq!((p.width, p.height), (480, 320));@@ -55,15 +56,25 @@ fn hdr_merge_command_creates_and_imports_a_dng() { assert_eq!(st.photos[1..].iter().map(|p| p.0).collect::<Vec<_>>(), ids); assert!(st.collapsed); assert_eq!(s.visible_cloned(), vec![id]);- // the second merge doesn't overwrite the first- let r2 = s.execute("merge.hdr", &json!({"ids": ids, "autoSettings": false})).unwrap();- assert!(r2["path"].as_str().unwrap().ends_with("IMG_0-HDR-2.dng"));- let id2 = lightcraft_catalog::PhotoId(r2["id"].as_u64().unwrap());- assert!(s.catalog.stack_of(id2).is_none(), "no stack without the option");+ // A repeated deterministic merge reuses the existing content instead of publishing an+ // unimported uniquely named orphan.+ s.execute("develop.set", &json!({"control": "light.exposure", "value": 0.73})).unwrap();+ let edited = s.develop_of(id).unwrap().as_ref().clone();+ let stack_before = s.catalog.stack_of(id).unwrap().clone();+ let undo_before = s.undo.len();+ let r2 = s.execute("merge.hdr", &json!({"ids": ids, "autoSettings": true, "stack": true})).unwrap();+ assert_eq!(r2["id"].as_u64(), Some(id.0));+ assert_eq!(r2["reused"].as_bool(), Some(true));+ assert_eq!(r2["path"].as_str(), Some(path));+ assert!(!dir.join("IMG_0-HDR-2.dng").exists(), "duplicate merge left an orphan");+ assert_eq!(s.catalog.photos().count(), 4);+ assert_eq!(s.undo.len(), undo_before, "reusing merge content added an undo step");+ assert_eq!(s.develop_of(id).unwrap().as_ref(), &edited, "reusing merge content changed edits");+ assert_eq!(s.catalog.stack_of(id), Some(&stack_before), "reusing merge content changed stack"); // the type filter finds merge results (and only them) s.execute("library.filter", &json!({"merged": "hdr"})).unwrap(); let found: std::collections::HashSet<_> = s.catalog.query(&s.filter, &Default::default()).into_iter().collect();- assert_eq!(found, [id, id2].into_iter().collect());+ assert_eq!(found, [id].into_iter().collect()); let _ = std::fs::remove_dir_all(&dir); } @@ -143,6 +154,55 @@ fn a_merge_of_raws_without_colour_matrices_keeps_their_colour() { let _ = std::fs::remove_dir_all(&dir); } +#[test]+fn hdr_merge_refuses_stale_result_before_writing() {+ let dir = temp_dir("hdr-stale");+ let mut paths = Vec::new();+ for (i, b) in lightcraft_merge::synth::bracket_dngs(240, 160, &[-2.0, 0.0, 2.0]).unwrap().into_iter().enumerate() {+ let p = dir.join(format!("IMG_{i}.dng"));+ std::fs::write(&p, b).unwrap();+ paths.push(p.to_string_lossy().to_string());+ }+ let mut s = Session::new().with_fs();+ let ids = import(&mut s, paths.clone());+ let first = s.execute("merge.hdr", &json!({"ids": ids, "autoSettings": false})).unwrap();+ let id = lightcraft_catalog::PhotoId(first["id"].as_u64().unwrap());+ let path = first["path"].as_str().unwrap().to_string();+ s.execute("develop.set", &json!({"control": "light.exposure", "value": 0.73})).unwrap();+ let edited = s.develop_of(id).unwrap().as_ref().clone();+ let stack_before = s.catalog.stack_of(id).cloned();++ std::fs::remove_file(&path).unwrap();+ let undo_before = s.undo.len();+ let missing = s.execute("merge.hdr", &json!({"ids": ids, "autoSettings": true}));+ assert!(missing.as_ref().unwrap_err().to_string().contains("missing or changed"));+ assert!(!std::path::Path::new(&path).exists());+ assert!(!dir.join("IMG_0-HDR-2.dng").exists());+ assert_eq!(s.undo.len(), undo_before);+ assert_eq!(s.develop_of(id).unwrap().as_ref(), &edited, "repair changed edits");+ assert_eq!(s.catalog.stack_of(id).cloned(), stack_before);++ std::fs::write(&path, std::fs::read(&paths[0]).unwrap()).unwrap();+ let changed_bytes = std::fs::read(&path).unwrap();+ let changed = s.execute("merge.hdr", &json!({"ids": ids, "autoSettings": true}));+ assert!(changed.as_ref().unwrap_err().to_string().contains("missing or changed"));+ assert_eq!(std::fs::read(&path).unwrap(), changed_bytes);+ assert!(!dir.join("IMG_0-HDR-2.dng").exists());+ assert_eq!(s.undo.len(), undo_before);+ assert_eq!(s.develop_of(id).unwrap().as_ref(), &edited, "changed-source repair changed edits");+ assert_eq!(s.catalog.stack_of(id).cloned(), stack_before);++ s.execute("photo.delete", &json!({"ids": [id.0]})).unwrap();+ let deleted_bytes = std::fs::read(&path).unwrap();+ let undo_after_delete = s.undo.len();+ let failed = s.execute("merge.hdr", &json!({"ids": ids, "autoSettings": false}));+ assert!(failed.as_ref().unwrap_err().to_string().contains("deleted"));+ assert_eq!(std::fs::read(&path).unwrap(), deleted_bytes);+ assert!(!dir.join("IMG_0-HDR-3.dng").exists(), "failed import left an orphan");+ assert_eq!(s.undo.len(), undo_after_delete);+ let _ = std::fs::remove_dir_all(&dir);+}+ #[test] fn panorama_command_with_auto_crop() { let dir = temp_dir("pano");