Issue #93: Export: never overwrite an original; atomic writes
- lightcraft_catalog::safe_file: one tested helper for crash-safe writes (temp file in the same folder, sync, rename, folder sync; temp removed on failure), write_new/write_new_unique that never replace a file and read the result back, same_file, and a per-thread fault injection for tests. - engine::originals::OriginalGuard: refuses a write whose target is a catalogued photo's original or XMP sidecar (by file identity), whatever the conflict policy or exact path. Used by run_batch (export, Export with Previous, control/MCP exact paths), ui.render/ui.screenshot paths, merge previewPath, MCP render_photo/screenshot paths (new query command export.checkTarget) and lightcraft-cli render. - The conflict policy now covers an export's sidecars together with the file (Unique gives both the same free name, Skip skips when either is taken). - export::write_file replaces the plain std::fs::write in the desktop app, CLI, MCP and engine writers; XMP sidecar writes use the shared helper. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Brandon Thomas
parent
e99aa61fc4
commit
46732cb0fe
|
|
@ -533,11 +533,17 @@ fn render(args: &[String]) -> Result<(), String> {
|
|||
.ok_or_else(|| format!("{output}: unknown extension (use .jpg .png .tif .webp .avif .dng or --opt format=…)"))?;
|
||||
}
|
||||
let e = export_photo(&mut s, lightcraft_engine::catalog::PhotoId(id), &o, 1)?;
|
||||
std::fs::write(&output, &e.bytes).map_err(|err| format!("{output}: {err}"))?;
|
||||
for (ext, bytes) in &e.sidecars {
|
||||
let sc = Path::new(&output).with_extension(ext);
|
||||
std::fs::write(&sc, bytes).map_err(|err| format!("{}: {err}", sc.display()))?;
|
||||
eprintln!("lightcraft-cli: wrote {}", sc.display());
|
||||
// never over the input (or its sidecar), however it is spelled: `render IMG.jpg -o IMG.jpg`
|
||||
let sidecars: Vec<(String, &Vec<u8>)> =
|
||||
e.sidecars.iter().map(|(ext, bytes)| (Path::new(&output).with_extension(ext).to_string_lossy().to_string(), bytes)).collect();
|
||||
let guard = s.original_guard();
|
||||
for p in std::iter::once(&output).chain(sidecars.iter().map(|(p, _)| p)) {
|
||||
guard.check(Path::new(p)).map_err(|err| format!("render: {err}"))?;
|
||||
}
|
||||
lightcraft_engine::export::write_file(&output, &e.bytes)?;
|
||||
for (sc, bytes) in &sidecars {
|
||||
lightcraft_engine::export::write_file(sc, bytes)?;
|
||||
eprintln!("lightcraft-cli: wrote {sc}");
|
||||
}
|
||||
eprintln!("lightcraft-cli: wrote {output} ({}×{})", e.width, e.height);
|
||||
Ok(())
|
||||
|
|
@ -593,18 +599,8 @@ fn snapshot(args: &[String]) -> Result<(), String> {
|
|||
eprintln!("lightcraft-cli snapshot: imported {n} files in {:.0} ms", ti.elapsed().as_secs_f64() * 1e3);
|
||||
}
|
||||
let services = lightcraft_ui_egui::Services {
|
||||
write_shared: Some(std::sync::Arc::new(|p: &str, b: &[u8]| {
|
||||
if let Some(dir) = Path::new(p).parent().filter(|d| !d.as_os_str().is_empty()) {
|
||||
std::fs::create_dir_all(dir).map_err(|e| e.to_string())?;
|
||||
}
|
||||
std::fs::write(p, b).map_err(|e| format!("{p}: {e}"))
|
||||
})),
|
||||
write: Some(Box::new(|p: &str, b: &[u8]| {
|
||||
if let Some(dir) = Path::new(p).parent().filter(|d| !d.as_os_str().is_empty()) {
|
||||
std::fs::create_dir_all(dir).map_err(|e| e.to_string())?;
|
||||
}
|
||||
std::fs::write(p, b).map_err(|e| format!("{p}: {e}"))
|
||||
})),
|
||||
write_shared: Some(std::sync::Arc::new(lightcraft_engine::export::write_file)),
|
||||
write: Some(Box::new(lightcraft_engine::export::write_file)),
|
||||
png: Some(Box::new(|img: &lightcraft_raster::Rgba8| {
|
||||
lightcraft_codecs::encode_png(&lightcraft_codecs::EncodeImage::rgba8(img), &lightcraft_codecs::EncodeMeta::default()).unwrap_or_default()
|
||||
})),
|
||||
|
|
|
|||
|
|
@ -86,6 +86,24 @@ fn render_subcommand() {
|
|||
assert!(String::from_utf8_lossy(&o.stderr).contains("unknown control"));
|
||||
}
|
||||
|
||||
/// Issue #93: `render IMG -o IMG` replaced its own input with the render.
|
||||
#[test]
|
||||
fn render_refuses_to_overwrite_its_input() {
|
||||
let input = tmp("self-in.png");
|
||||
gradient_png(&input);
|
||||
let before = std::fs::read(&input).unwrap();
|
||||
let dir = input.parent().unwrap();
|
||||
std::fs::create_dir_all(dir.join("sub")).unwrap();
|
||||
let other_spelling = dir.join("sub/../self-in.png");
|
||||
for out in [&input, &other_spelling] {
|
||||
let o =
|
||||
Command::new(BIN).args(["render", input.to_str().unwrap(), "-o", out.to_str().unwrap(), "--set", "light.exposure=1"]).output().unwrap();
|
||||
assert!(!o.status.success());
|
||||
assert!(String::from_utf8_lossy(&o.stderr).contains("never writes over an original"), "{}", String::from_utf8_lossy(&o.stderr));
|
||||
}
|
||||
assert_eq!(std::fs::read(&input).unwrap(), before, "the input is untouched");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn render_export_options() {
|
||||
let input = tmp("o-in.png");
|
||||
|
|
|
|||
|
|
@ -209,18 +209,9 @@ fn services() -> Services {
|
|||
.save_file()
|
||||
.map(|p| p.to_string_lossy().to_string())
|
||||
})),
|
||||
write_shared: Some(std::sync::Arc::new(|p: &str, b: &[u8]| {
|
||||
if let Some(dir) = std::path::Path::new(p).parent().filter(|d| !d.as_os_str().is_empty()) {
|
||||
std::fs::create_dir_all(dir).map_err(|e| e.to_string())?;
|
||||
}
|
||||
std::fs::write(p, b).map_err(|e| e.to_string())
|
||||
})),
|
||||
write: Some(Box::new(|p: &str, b: &[u8]| {
|
||||
if let Some(dir) = std::path::Path::new(p).parent().filter(|d| !d.as_os_str().is_empty()) {
|
||||
std::fs::create_dir_all(dir).map_err(|e| e.to_string())?;
|
||||
}
|
||||
std::fs::write(p, b).map_err(|e| e.to_string())
|
||||
})),
|
||||
// atomic (temp file + sync + rename): a failed write never leaves a truncated file
|
||||
write_shared: Some(std::sync::Arc::new(lightcraft_engine::export::write_file)),
|
||||
write: Some(Box::new(lightcraft_engine::export::write_file)),
|
||||
png: Some(Box::new(|img: &lightcraft_raster::Rgba8| {
|
||||
lightcraft_codecs::encode_png(&lightcraft_codecs::EncodeImage::rgba8(img), &lightcraft_codecs::EncodeMeta::default()).unwrap_or_default()
|
||||
})),
|
||||
|
|
|
|||
|
|
@ -15,6 +15,7 @@ pub mod local;
|
|||
pub mod model;
|
||||
pub mod query;
|
||||
pub mod rules;
|
||||
pub mod safe_file;
|
||||
pub mod stacks;
|
||||
pub mod store;
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,317 @@
|
|||
//! Crash-safe writes of user-visible files: exports, renders, DNG conversions, smart previews and
|
||||
//! XMP sidecars all go through here (the catalog's own files use [`crate::store::FsStore`]).
|
||||
//!
|
||||
//! - [`write_atomic`] replaces a file so that after a crash, a full disk or an unplugged drive
|
||||
//! either the old or the complete new content is there — never a truncated mix. The content
|
||||
//! goes to a new temp file in the same folder (unique name, `create_new`), is synced to disk and
|
||||
//! renamed over the target; then the folder is synced (POSIX). Any failure removes the temp
|
||||
//! file and leaves the target as it was.
|
||||
//! - [`write_new`] / [`write_new_unique`] write a file that must not exist yet: the same temp file
|
||||
//! and sync, plus a read-back check, and the file then appears under its final name **without
|
||||
//! replacing anything** (a hard link, which fails on a taken name; a check-then-rename where the
|
||||
//! volume has no hard links).
|
||||
//! - [`same_file`] tells whether two paths name the same file on disk (device + inode on Unix,
|
||||
//! resolved paths elsewhere), for "is this target an original?" checks.
|
||||
|
||||
use std::cell::Cell;
|
||||
use std::fs::{self, File, OpenOptions};
|
||||
use std::io::{self, Write};
|
||||
use std::path::{Path, PathBuf};
|
||||
use std::sync::atomic::{AtomicU64, Ordering};
|
||||
|
||||
thread_local! {
|
||||
static FAIL_AFTER: Cell<Option<u64>> = const { Cell::new(None) };
|
||||
}
|
||||
|
||||
/// Fault injection for tests (in any crate): while the returned guard lives, every write made by
|
||||
/// this module **on the calling thread** fails once `bytes` bytes of a file were written — as a
|
||||
/// full disk or an unplugged drive would.
|
||||
#[doc(hidden)]
|
||||
pub fn fail_writes_after(bytes: u64) -> FaultGuard {
|
||||
FAIL_AFTER.with(|c| c.set(Some(bytes)));
|
||||
FaultGuard(())
|
||||
}
|
||||
|
||||
/// Ends a [`fail_writes_after`] when dropped.
|
||||
#[doc(hidden)]
|
||||
pub struct FaultGuard(());
|
||||
|
||||
impl Drop for FaultGuard {
|
||||
fn drop(&mut self) {
|
||||
FAIL_AFTER.with(|c| c.set(None));
|
||||
}
|
||||
}
|
||||
|
||||
/// Counts what is written; fails past an injected limit.
|
||||
struct Limited<W> {
|
||||
inner: W,
|
||||
n: u64,
|
||||
limit: Option<u64>,
|
||||
}
|
||||
|
||||
impl<W: Write> Write for Limited<W> {
|
||||
fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
|
||||
let buf = match self.limit {
|
||||
Some(l) => {
|
||||
let room = usize::try_from(l.saturating_sub(self.n)).unwrap_or(usize::MAX);
|
||||
if room == 0 && !buf.is_empty() {
|
||||
return Err(io::Error::other("write failed (injected fault)"));
|
||||
}
|
||||
buf.get(..room.min(buf.len())).unwrap_or(buf)
|
||||
}
|
||||
None => buf,
|
||||
};
|
||||
let k = self.inner.write(buf)?;
|
||||
self.n += k as u64;
|
||||
Ok(k)
|
||||
}
|
||||
fn flush(&mut self) -> io::Result<()> {
|
||||
self.inner.flush()
|
||||
}
|
||||
}
|
||||
|
||||
fn parent_of(path: &Path) -> &Path {
|
||||
path.parent().filter(|d| !d.as_os_str().is_empty()).unwrap_or(Path::new("."))
|
||||
}
|
||||
|
||||
/// Make a rename or a new name in `dir` durable (POSIX). Opening a directory fails on Windows,
|
||||
/// where NTFS journals the rename itself, so errors are ignored.
|
||||
pub fn sync_dir(dir: &Path) {
|
||||
if let Ok(d) = File::open(dir) {
|
||||
let _ = d.sync_all();
|
||||
}
|
||||
}
|
||||
|
||||
/// A new, empty temp file next to `path` (hidden, unique in this process and folder).
|
||||
fn create_temp(path: &Path) -> io::Result<(PathBuf, File)> {
|
||||
static N: AtomicU64 = AtomicU64::new(0);
|
||||
if path.file_name().is_none() {
|
||||
return Err(io::Error::new(io::ErrorKind::InvalidInput, format!("{} is not a file path", path.display())));
|
||||
}
|
||||
let dir = parent_of(path);
|
||||
for _ in 0..1000 {
|
||||
let tmp = dir.join(format!(".lightcraft-{}-{}.tmp", std::process::id(), N.fetch_add(1, Ordering::Relaxed)));
|
||||
match OpenOptions::new().write(true).create_new(true).open(&tmp) {
|
||||
Ok(f) => return Ok((tmp, f)),
|
||||
Err(e) if e.kind() == io::ErrorKind::AlreadyExists => continue,
|
||||
Err(e) => return Err(e),
|
||||
}
|
||||
}
|
||||
Err(io::Error::new(io::ErrorKind::AlreadyExists, format!("no free temp file name in {}", dir.display())))
|
||||
}
|
||||
|
||||
/// Write a temp file next to `path` with what `fill` writes, synced to disk. On failure the temp
|
||||
/// file is removed. Returns its path and length.
|
||||
fn write_temp(path: &Path, fill: &mut dyn FnMut(&mut dyn Write) -> io::Result<()>) -> io::Result<(PathBuf, u64)> {
|
||||
let (tmp, file) = create_temp(path)?;
|
||||
let r = (|| {
|
||||
let limit = FAIL_AFTER.with(Cell::get);
|
||||
let mut w = Limited { inner: io::BufWriter::with_capacity(1 << 20, file), n: 0, limit };
|
||||
fill(&mut w)?;
|
||||
w.flush()?;
|
||||
let n = w.n;
|
||||
let f = w.inner.into_inner().map_err(|e| e.into_error())?;
|
||||
f.sync_all()?;
|
||||
Ok(n)
|
||||
})();
|
||||
match r {
|
||||
Ok(n) => Ok((tmp, n)),
|
||||
Err(e) => {
|
||||
let _ = fs::remove_file(&tmp);
|
||||
Err(e)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Replace (or create) `path` with `data` atomically and durably; see the module docs. The
|
||||
/// folder must exist.
|
||||
pub fn write_atomic(path: &Path, data: &[u8]) -> io::Result<()> {
|
||||
write_atomic_with(path, &mut |w| w.write_all(data)).map(|_| ())
|
||||
}
|
||||
|
||||
/// [`write_atomic`] with streamed content: `fill` writes it (an error aborts and leaves the old
|
||||
/// file). Returns the length written.
|
||||
pub fn write_atomic_with(path: &Path, fill: &mut dyn FnMut(&mut dyn Write) -> io::Result<()>) -> io::Result<u64> {
|
||||
let (tmp, n) = write_temp(path, fill)?;
|
||||
if let Err(e) = fs::rename(&tmp, path) {
|
||||
let _ = fs::remove_file(&tmp);
|
||||
return Err(e);
|
||||
}
|
||||
sync_dir(parent_of(path));
|
||||
Ok(n)
|
||||
}
|
||||
|
||||
/// Write `data` as the new file `path`: never replaces an existing file (`AlreadyExists`), and
|
||||
/// the file only appears once it is complete, synced and read back identical.
|
||||
pub fn write_new(path: &Path, data: &[u8]) -> io::Result<()> {
|
||||
write_new_unique(&mut std::iter::once(path.to_path_buf()), data).map(|_| ())
|
||||
}
|
||||
|
||||
/// [`write_new`] under the first free name of `candidates` (all in the same folder, e.g.
|
||||
/// `IMG_1.dng`, `IMG_1-2.dng`, …): the file is written once and then given the first name that
|
||||
/// is not taken. Returns that name; `AlreadyExists` when every candidate is taken.
|
||||
pub fn write_new_unique(candidates: &mut dyn Iterator<Item = PathBuf>, data: &[u8]) -> io::Result<PathBuf> {
|
||||
let mut candidates = candidates.peekable();
|
||||
let Some(first) = candidates.peek().cloned() else {
|
||||
return Err(io::Error::new(io::ErrorKind::InvalidInput, "no file name to write"));
|
||||
};
|
||||
let (tmp, _) = write_temp(&first, &mut |w| w.write_all(data))?;
|
||||
let r = (|| {
|
||||
if !same_content(&tmp, data)? {
|
||||
return Err(io::Error::other("the written file reads back different (failing drive or connection?)"));
|
||||
}
|
||||
for path in candidates.by_ref().take(100_000) {
|
||||
match publish_new(&tmp, &path) {
|
||||
Ok(()) => {
|
||||
sync_dir(parent_of(&path));
|
||||
return Ok(path);
|
||||
}
|
||||
Err(e) if e.kind() == io::ErrorKind::AlreadyExists => continue,
|
||||
Err(e) => return Err(e),
|
||||
}
|
||||
}
|
||||
Err(io::Error::new(io::ErrorKind::AlreadyExists, format!("{}: no free file name", first.display())))
|
||||
})();
|
||||
// after a hard link the temp name is a second name of the new file; after a rename it's gone
|
||||
let _ = fs::remove_file(&tmp);
|
||||
r
|
||||
}
|
||||
|
||||
/// Give the complete temp file `tmp` the name `path`, unless that is taken.
|
||||
fn publish_new(tmp: &Path, path: &Path) -> io::Result<()> {
|
||||
match fs::hard_link(tmp, path) {
|
||||
Ok(()) => Ok(()),
|
||||
Err(e) if e.kind() == io::ErrorKind::AlreadyExists => Err(e),
|
||||
// no hard links on this volume (FAT/exFAT, some shares): check, then rename
|
||||
Err(_) => {
|
||||
if fs::symlink_metadata(path).is_ok() {
|
||||
return Err(io::Error::new(io::ErrorKind::AlreadyExists, format!("{} exists", path.display())));
|
||||
}
|
||||
fs::rename(tmp, path)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Does the file at `path` hold exactly `data`?
|
||||
fn same_content(path: &Path, data: &[u8]) -> io::Result<bool> {
|
||||
use std::io::Read;
|
||||
let mut f = File::open(path)?;
|
||||
if f.metadata()?.len() != data.len() as u64 {
|
||||
return Ok(false);
|
||||
}
|
||||
let mut buf = vec![0u8; 1 << 20];
|
||||
let mut at = 0usize;
|
||||
loop {
|
||||
let k = match f.read(&mut buf) {
|
||||
Ok(0) => return Ok(at == data.len()),
|
||||
Ok(k) => k,
|
||||
Err(e) if e.kind() == io::ErrorKind::Interrupted => continue,
|
||||
Err(e) => return Err(e),
|
||||
};
|
||||
if data.get(at..at + k) != buf.get(..k) {
|
||||
return Ok(false);
|
||||
}
|
||||
at += k;
|
||||
}
|
||||
}
|
||||
|
||||
/// Do `a` and `b` name the same existing file (through links, `..`, case-insensitive names)?
|
||||
pub fn same_file(a: &Path, b: &Path) -> bool {
|
||||
let (Ok(ma), Ok(mb)) = (fs::metadata(a), fs::metadata(b)) else { return false };
|
||||
#[cfg(unix)]
|
||||
{
|
||||
use std::os::unix::fs::MetadataExt;
|
||||
ma.dev() == mb.dev() && ma.ino() == mb.ino()
|
||||
}
|
||||
#[cfg(not(unix))]
|
||||
{
|
||||
if ma.len() != mb.len() {
|
||||
return false;
|
||||
}
|
||||
match (fs::canonicalize(a), fs::canonicalize(b)) {
|
||||
(Ok(x), Ok(y)) => x == y || x.to_string_lossy().to_lowercase() == y.to_string_lossy().to_lowercase(),
|
||||
_ => false,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
fn temp(tag: &str) -> PathBuf {
|
||||
let d = std::env::temp_dir().join(format!("lc-safe-file-{tag}-{}", std::process::id()));
|
||||
let _ = fs::remove_dir_all(&d);
|
||||
fs::create_dir_all(&d).unwrap();
|
||||
d
|
||||
}
|
||||
|
||||
fn names(dir: &Path) -> Vec<String> {
|
||||
let mut v: Vec<String> = fs::read_dir(dir).unwrap().flatten().map(|e| e.file_name().to_string_lossy().to_string()).collect();
|
||||
v.sort();
|
||||
v
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn atomic_write_replaces_and_leaves_no_temp() {
|
||||
let d = temp("replace");
|
||||
let p = d.join("out.jpg");
|
||||
write_atomic(&p, b"one").unwrap();
|
||||
write_atomic(&p, b"two two").unwrap();
|
||||
assert_eq!(fs::read(&p).unwrap(), b"two two");
|
||||
assert_eq!(names(&d), vec!["out.jpg"]);
|
||||
let _ = fs::remove_dir_all(&d);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_failure_mid_write_keeps_the_old_file() {
|
||||
let d = temp("midwrite");
|
||||
let p = d.join("photo.jpg");
|
||||
fs::write(&p, b"the original bytes").unwrap();
|
||||
{
|
||||
let _f = fail_writes_after(4);
|
||||
let e = write_atomic(&p, &[7u8; 10_000]).unwrap_err();
|
||||
assert!(e.to_string().contains("injected"), "{e}");
|
||||
assert!(write_new(&d.join("new.dng"), &[1u8; 100]).is_err());
|
||||
}
|
||||
assert_eq!(fs::read(&p).unwrap(), b"the original bytes", "the target is untouched");
|
||||
assert_eq!(names(&d), vec!["photo.jpg"], "no temp file and no partial new file");
|
||||
// a fill error aborts the same way
|
||||
let r = write_atomic_with(&p, &mut |w| {
|
||||
w.write_all(b"half")?;
|
||||
Err(io::Error::other("encoder failed"))
|
||||
});
|
||||
assert!(r.is_err());
|
||||
assert_eq!(fs::read(&p).unwrap(), b"the original bytes");
|
||||
assert_eq!(names(&d), vec!["photo.jpg"]);
|
||||
let _ = fs::remove_dir_all(&d);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn new_files_never_replace_anything() {
|
||||
let d = temp("new");
|
||||
let p = d.join("a.dng");
|
||||
fs::write(&p, b"keep").unwrap();
|
||||
assert_eq!(write_new(&p, b"other").unwrap_err().kind(), io::ErrorKind::AlreadyExists);
|
||||
assert_eq!(fs::read(&p).unwrap(), b"keep");
|
||||
let mut c = (1..).map(|k| if k == 1 { d.join("a.dng") } else { d.join(format!("a-{k}.dng")) });
|
||||
let got = write_new_unique(&mut c, b"fresh").unwrap();
|
||||
assert_eq!(got, d.join("a-2.dng"));
|
||||
assert_eq!(fs::read(&got).unwrap(), b"fresh");
|
||||
assert_eq!(names(&d), vec!["a-2.dng", "a.dng"], "the temp file is gone");
|
||||
let _ = fs::remove_dir_all(&d);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn same_file_sees_through_other_spellings() {
|
||||
let d = temp("same");
|
||||
let p = d.join("x.jpg");
|
||||
fs::write(&p, b"x").unwrap();
|
||||
fs::write(d.join("y.jpg"), b"x").unwrap();
|
||||
fs::create_dir_all(d.join("sub")).unwrap();
|
||||
assert!(same_file(&p, &d.join("sub/../x.jpg")));
|
||||
assert!(!same_file(&p, &d.join("y.jpg")));
|
||||
assert!(!same_file(&p, &d.join("missing.jpg")));
|
||||
let _ = fs::remove_dir_all(&d);
|
||||
}
|
||||
}
|
||||
|
|
@ -133,7 +133,7 @@ fn edit_external(s: &mut Session, p: &Value) -> Result<Value> {
|
|||
};
|
||||
let mut written = Vec::new();
|
||||
let mut write = |path: &str, bytes: &[u8]| -> std::result::Result<(), String> {
|
||||
std::fs::write(path, bytes).map_err(|e| format!("{path}: {e}"))?;
|
||||
crate::export::write_file(path, bytes)?;
|
||||
written.push(path.to_string());
|
||||
Ok(())
|
||||
};
|
||||
|
|
|
|||
|
|
@ -96,5 +96,18 @@ pub fn specs() -> Vec<CommandSpec> {
|
|||
save
|
||||
),
|
||||
cmd!("export.deletePreset", "Delete Export Preset", [], None, "{name} — removes a user preset → presets", always, delete),
|
||||
cmd!(
|
||||
query "export.checkTarget",
|
||||
"Check Output Path",
|
||||
[],
|
||||
None,
|
||||
"{path} — fails when writing `path` would replace a photo's original (or its XMP sidecar) in the library; front ends call it before saving a render or screenshot to a user-given path → {path}",
|
||||
always,
|
||||
|s, p| {
|
||||
let path = p.get("path").and_then(Value::as_str).ok_or_else(|| bad("export.checkTarget", "missing `path`"))?;
|
||||
s.check_write_target(path).map_err(|e| bad("export.checkTarget", e))?;
|
||||
Ok(json!({"path": path}))
|
||||
}
|
||||
),
|
||||
]
|
||||
}
|
||||
|
|
|
|||
|
|
@ -30,7 +30,8 @@ fn run(s: &mut Session, id: &str, p: &Value) -> Result<Value> {
|
|||
if let (Some(path), Some(img)) = (str_param(p, "previewPath"), &out.preview) {
|
||||
let png = lightcraft_codecs::encode_png(&lightcraft_codecs::EncodeImage::rgba8(img), &lightcraft_codecs::EncodeMeta::default())
|
||||
.map_err(|e| bad(id, e.to_string()))?;
|
||||
std::fs::write(path, png).map_err(|e| bad(id, format!("{path}: {e}")))?;
|
||||
s.check_write_target(path).map_err(|e| bad(id, e))?;
|
||||
crate::export::write_file(path, &png).map_err(|e| bad(id, e))?;
|
||||
info["previewPath"] = json!(path);
|
||||
}
|
||||
if let Some(img) = &out.preview {
|
||||
|
|
|
|||
|
|
@ -930,6 +930,8 @@ pub struct PreparedExport {
|
|||
pub photo: lightcraft_catalog::PhotoId,
|
||||
pub file_name: String,
|
||||
work: Work,
|
||||
/// The library's originals, which [`run_batch`] never writes over (shared by a batch).
|
||||
guard: std::sync::Arc<crate::originals::OriginalGuard>,
|
||||
}
|
||||
|
||||
struct RenderWork {
|
||||
|
|
@ -952,12 +954,30 @@ enum Work {
|
|||
},
|
||||
}
|
||||
|
||||
/// Set up the export of photo `id` at 1-based position `seq` of a batch.
|
||||
/// Set up the export of photo `id` at 1-based position `seq` of a batch. (For a whole batch use
|
||||
/// [`prepare_batch`]: it looks at the library's originals once.)
|
||||
pub fn prepare_export(
|
||||
session: &mut crate::Session,
|
||||
id: lightcraft_catalog::PhotoId,
|
||||
o: &ExportOptions,
|
||||
seq: usize,
|
||||
) -> Result<PreparedExport, String> {
|
||||
let guard = std::sync::Arc::new(session.original_guard());
|
||||
prepare_guarded(session, id, o, seq, guard)
|
||||
}
|
||||
|
||||
/// [`prepare_export`] for each of `ids` in order (`{seq}` = position, from 1).
|
||||
pub fn prepare_batch(session: &mut crate::Session, ids: &[lightcraft_catalog::PhotoId], o: &ExportOptions) -> Result<Vec<PreparedExport>, String> {
|
||||
let guard = std::sync::Arc::new(session.original_guard());
|
||||
ids.iter().enumerate().map(|(i, id)| prepare_guarded(session, *id, o, i + 1, guard.clone())).collect()
|
||||
}
|
||||
|
||||
fn prepare_guarded(
|
||||
session: &mut crate::Session,
|
||||
id: lightcraft_catalog::PhotoId,
|
||||
o: &ExportOptions,
|
||||
seq: usize,
|
||||
guard: std::sync::Arc<crate::originals::OriginalGuard>,
|
||||
) -> Result<PreparedExport, String> {
|
||||
let p = session.catalog.photo(id).ok_or("no such photo")?;
|
||||
let file_name = o.file_name_for(p, seq);
|
||||
|
|
@ -979,7 +999,7 @@ pub fn prepare_export(
|
|||
size: (p.width as usize, p.height as usize),
|
||||
}
|
||||
};
|
||||
Ok(PreparedExport { photo: id, file_name, work })
|
||||
Ok(PreparedExport { photo: id, file_name, work, guard })
|
||||
}
|
||||
|
||||
impl PreparedExport {
|
||||
|
|
@ -1031,14 +1051,16 @@ pub enum DngCompression {
|
|||
}
|
||||
|
||||
/// The destination of a batch: a folder (with `ExportOptions::subfolder` and the conflict policy
|
||||
/// applied), or one exact file path (single photo; overwritten).
|
||||
/// applied to each file together with its sidecars), or one exact file path (single photo; an
|
||||
/// ordinary file already there is replaced). Either way a catalogued original (or its sidecar)
|
||||
/// is never written over: see [`crate::originals`].
|
||||
#[derive(Clone, Debug, Default)]
|
||||
pub struct Destination {
|
||||
pub dir: String,
|
||||
pub exact: Option<String>,
|
||||
}
|
||||
|
||||
/// Export `ids` in order ([`prepare_export`] + [`run_batch`]), stopping at the first error.
|
||||
/// Export `ids` in order ([`prepare_batch`] + [`run_batch`]), stopping at the first error.
|
||||
pub fn export_batch(
|
||||
session: &mut crate::Session,
|
||||
ids: &[lightcraft_catalog::PhotoId],
|
||||
|
|
@ -1047,15 +1069,34 @@ pub fn export_batch(
|
|||
write: &mut dyn FnMut(&str, &[u8]) -> Result<(), String>,
|
||||
exists: &dyn Fn(&str) -> bool,
|
||||
) -> Result<Vec<serde_json::Value>, String> {
|
||||
let items = ids.iter().enumerate().map(|(i, id)| prepare_export(session, *id, o, i + 1)).collect::<Result<Vec<_>, _>>()?;
|
||||
let items = prepare_batch(session, ids, o)?;
|
||||
run_batch(items, o, to, write, exists, true, &mut |_, _| true)
|
||||
}
|
||||
|
||||
/// Write an exported or rendered file on disk: its folder is created if needed, and the file is
|
||||
/// replaced atomically ([`lightcraft_catalog::safe_file::write_atomic`]: a temp file, synced, then
|
||||
/// renamed), so a failure part-way leaves any previous file intact and no truncated one. The
|
||||
/// native writer behind exports, renders and screenshots (app, CLI, MCP).
|
||||
pub fn write_file(path: &str, bytes: &[u8]) -> Result<(), String> {
|
||||
let p = std::path::Path::new(path);
|
||||
if let Some(dir) = p.parent().filter(|d| !d.as_os_str().is_empty()) {
|
||||
std::fs::create_dir_all(dir).map_err(|e| format!("{}: {e}", dir.display()))?;
|
||||
}
|
||||
lightcraft_catalog::safe_file::write_atomic(p, bytes).map_err(|e| format!("{path}: {e}"))
|
||||
}
|
||||
|
||||
/// The path of the sidecar with extension `ext` of the exported file `main`.
|
||||
fn sidecar_path(main: &str, ext: &str) -> String {
|
||||
std::path::Path::new(main).with_extension(ext).to_string_lossy().to_string()
|
||||
}
|
||||
|
||||
/// Run prepared exports in order: pick each one's path, and hand the bytes (and sidecars) to
|
||||
/// `write`. `exists` tells whether a path is taken (conflict policy). `progress(done, next file)`
|
||||
/// is called before each photo; returning false cancels the rest. Returns one JSON object per
|
||||
/// photo: `{path, width, height, bytes, sidecars}`, `{skipped: path}` or (unless
|
||||
/// `stop_on_error`) `{photo, file, error}`.
|
||||
/// `write`. `exists` tells whether a path is taken. The conflict policy applies to a file and its
|
||||
/// sidecars as one: with Unique both get the same free name, with Skip the photo is skipped when
|
||||
/// either is taken. A path that is a catalogued original (or its sidecar) is refused whatever the
|
||||
/// policy. `progress(done, next file)` is called before each photo; returning false cancels the
|
||||
/// rest. Returns one JSON object per photo: `{path, width, height, bytes, sidecars}`,
|
||||
/// `{skipped: path}` or (unless `stop_on_error`) `{photo, file, error}`.
|
||||
pub fn run_batch(
|
||||
items: Vec<PreparedExport>,
|
||||
o: &ExportOptions,
|
||||
|
|
@ -1075,7 +1116,7 @@ pub fn run_batch(
|
|||
if !progress(i, &item.file_name) {
|
||||
break;
|
||||
}
|
||||
let (photo, name) = (item.photo, item.file_name.clone());
|
||||
let (photo, name, guard) = (item.photo, item.file_name.clone(), item.guard.clone());
|
||||
let e = match item.run() {
|
||||
Ok(e) => e,
|
||||
Err(err) if stop_on_error => return Err(err),
|
||||
|
|
@ -1084,14 +1125,18 @@ pub fn run_batch(
|
|||
continue;
|
||||
}
|
||||
};
|
||||
// the exported file and its sidecars
|
||||
let group = |main: &str| std::iter::once(main.to_string()).chain(e.sidecars.iter().map(|(x, _)| sidecar_path(main, x))).collect::<Vec<_>>();
|
||||
let path = match to.exact.as_deref().filter(|_| single) {
|
||||
Some(p) => p.to_string(),
|
||||
Some(p) => Ok(p.to_string()),
|
||||
None => {
|
||||
let mut path = join(&dir, &e.file_name);
|
||||
let busy = |p: &str, taken: &std::collections::HashSet<String>| taken.contains(p) || exists(p);
|
||||
if busy(&path, &taken) {
|
||||
let path = join(&dir, &e.file_name);
|
||||
let busy = |main: &str| group(main).iter().any(|p| taken.contains(p) || exists(p));
|
||||
if !busy(&path) {
|
||||
Ok(path)
|
||||
} else {
|
||||
match o.conflict {
|
||||
Conflict::Overwrite => {}
|
||||
Conflict::Overwrite => Ok(path),
|
||||
Conflict::Skip => {
|
||||
out.push(json!({"skipped": path}));
|
||||
continue;
|
||||
|
|
@ -1099,29 +1144,34 @@ pub fn run_batch(
|
|||
Conflict::Unique => {
|
||||
let (stem, ext) = e.file_name.rsplit_once('.').map_or((e.file_name.as_str(), None), |(a, b)| (a, Some(b)));
|
||||
let name = |n: usize| join(&dir, &ext.map_or(format!("{stem}-{n}"), |x| format!("{stem}-{n}.{x}")));
|
||||
path = (2..).map(name).find(|p| !busy(p, &taken)).expect("a free name");
|
||||
(2..1_000_000).map(name).find(|p| !busy(p)).ok_or_else(|| format!("{path}: no free file name"))
|
||||
}
|
||||
}
|
||||
}
|
||||
path
|
||||
}
|
||||
};
|
||||
let written = write(&path, &e.bytes).and_then(|()| {
|
||||
let mut sidecars = Vec::new();
|
||||
for (ext, bytes) in &e.sidecars {
|
||||
let sc = std::path::Path::new(&path).with_extension(ext).to_string_lossy().to_string();
|
||||
write(&sc, bytes)?;
|
||||
sidecars.push(sc);
|
||||
let file = path.clone().unwrap_or_else(|_| name.clone());
|
||||
let written = path.and_then(|path| {
|
||||
let files = group(&path);
|
||||
// never over an original, whatever the conflict policy or the exact path said
|
||||
for f in &files {
|
||||
guard.check(std::path::Path::new(f))?;
|
||||
}
|
||||
Ok(sidecars)
|
||||
write(&path, &e.bytes)?;
|
||||
let mut sidecars = Vec::new();
|
||||
for ((_, bytes), sc) in e.sidecars.iter().zip(files.iter().skip(1)) {
|
||||
write(sc, bytes)?;
|
||||
sidecars.push(sc.clone());
|
||||
}
|
||||
Ok((path, files, sidecars))
|
||||
});
|
||||
match written {
|
||||
Ok(sidecars) => {
|
||||
taken.insert(path.clone());
|
||||
Ok((path, files, sidecars)) => {
|
||||
taken.extend(files);
|
||||
out.push(json!({"path": path, "width": e.width, "height": e.height, "bytes": e.bytes.len(), "sidecars": sidecars}));
|
||||
}
|
||||
Err(err) if stop_on_error => return Err(err),
|
||||
Err(err) => out.push(json!({"photo": photo.0, "file": path, "error": err})),
|
||||
Err(err) => out.push(json!({"photo": photo.0, "file": file, "error": err})),
|
||||
}
|
||||
}
|
||||
Ok(out)
|
||||
|
|
|
|||
|
|
@ -24,6 +24,7 @@ pub mod library;
|
|||
pub mod media;
|
||||
pub mod memory;
|
||||
pub mod merge;
|
||||
pub mod originals;
|
||||
pub mod preset_import;
|
||||
pub mod preset_luminar;
|
||||
pub mod presets;
|
||||
|
|
|
|||
|
|
@ -0,0 +1,102 @@
|
|||
//! Never write over an original: exports, renders and other files LightCraft writes for the user
|
||||
//! must not land on a catalogued photo's own file (or its XMP sidecar), whatever the conflict
|
||||
//! setting or exact path asked for. [`OriginalGuard`] is a snapshot of the library's originals
|
||||
//! that answers "would writing here replace one?" by file identity (the same file under another
|
||||
//! spelling, through `..`, a link or a case-insensitive volume counts), not by comparing strings.
|
||||
|
||||
use std::collections::HashMap;
|
||||
use std::path::{Path, PathBuf};
|
||||
|
||||
use lightcraft_catalog::safe_file::same_file;
|
||||
use lightcraft_catalog::{Catalog, Source};
|
||||
|
||||
/// The library's originals and their sidecars, looked up by file name.
|
||||
#[derive(Debug, Default)]
|
||||
pub struct OriginalGuard {
|
||||
/// lower-case file name → (path on disk, what it is: "the original of IMG_1.CR3", …)
|
||||
by_name: HashMap<String, Vec<(PathBuf, String)>>,
|
||||
}
|
||||
|
||||
impl OriginalGuard {
|
||||
/// The originals of every photo in `catalog`.
|
||||
pub fn new(catalog: &Catalog) -> Self {
|
||||
let mut g = Self::default();
|
||||
for p in catalog.photos() {
|
||||
if let Source::File { path } = &p.source {
|
||||
g.add(Path::new(path), &p.file_name);
|
||||
}
|
||||
}
|
||||
g
|
||||
}
|
||||
|
||||
/// Protect `path` (an original named `label`) and its sidecars (`IMG_1.xmp`, `IMG_1.CR3.xmp`).
|
||||
pub fn add(&mut self, path: &Path, label: &str) {
|
||||
let Some(name) = path.file_name().map(|n| n.to_string_lossy().to_string()) else { return };
|
||||
let mut put = |file: String, p: PathBuf, what: String| self.by_name.entry(file.to_lowercase()).or_default().push((p, what));
|
||||
put(name.clone(), path.to_path_buf(), format!("the original of {label}"));
|
||||
let stem = path.file_stem().map(|s| s.to_string_lossy().to_string()).unwrap_or_default();
|
||||
put(format!("{stem}.xmp"), path.with_extension("xmp"), format!("the XMP sidecar of {label}"));
|
||||
put(format!("{name}.xmp"), PathBuf::from(format!("{}.xmp", path.to_string_lossy())), format!("the XMP sidecar of {label}"));
|
||||
}
|
||||
|
||||
/// `Err` (a message for the user) when writing `target` would replace a protected file.
|
||||
pub fn check(&self, target: &Path) -> Result<(), String> {
|
||||
if std::fs::symlink_metadata(target).is_err() {
|
||||
return Ok(()); // nothing there to replace
|
||||
}
|
||||
let mut names: Vec<String> = target.file_name().map(|n| n.to_string_lossy().to_lowercase()).into_iter().collect();
|
||||
if let Some(n) = std::fs::canonicalize(target).ok().and_then(|c| c.file_name().map(|n| n.to_string_lossy().to_lowercase())) {
|
||||
names.push(n);
|
||||
}
|
||||
for n in &names {
|
||||
for (p, what) in self.by_name.get(n).into_iter().flatten() {
|
||||
if same_file(p, target) {
|
||||
return Err(format!(
|
||||
"{} is {what} in the library: LightCraft never writes over an original (choose another folder or file name)",
|
||||
target.display()
|
||||
));
|
||||
}
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
impl crate::Session {
|
||||
/// A guard over this library's originals (see [`OriginalGuard`]).
|
||||
pub fn original_guard(&self) -> OriginalGuard {
|
||||
OriginalGuard::new(&self.catalog)
|
||||
}
|
||||
|
||||
/// `Err` when writing `path` would replace a catalogued photo's original or sidecar.
|
||||
pub fn check_write_target(&self, path: &str) -> Result<(), String> {
|
||||
self.original_guard().check(Path::new(path))
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use lightcraft_catalog::{Op, Photo, PhotoId};
|
||||
|
||||
#[test]
|
||||
fn originals_and_their_sidecars_are_protected_by_identity() {
|
||||
let d = std::env::temp_dir().join(format!("lc-originals-{}", std::process::id()));
|
||||
let _ = std::fs::remove_dir_all(&d);
|
||||
std::fs::create_dir_all(d.join("sub")).unwrap();
|
||||
let orig = d.join("IMG_1.jpg");
|
||||
std::fs::write(&orig, b"original").unwrap();
|
||||
std::fs::write(d.join("IMG_1.xmp"), b"<x/>").unwrap();
|
||||
std::fs::write(d.join("other.jpg"), b"other").unwrap();
|
||||
let mut c = Catalog::default();
|
||||
let p = Photo::new(PhotoId(1), Source::File { path: orig.to_string_lossy().to_string() }, "IMG_1.jpg", "JPEG", 10, 10, "2026-01-01");
|
||||
c.apply(Op::AddPhoto { photo: Box::new(p) }).unwrap();
|
||||
let g = OriginalGuard::new(&c);
|
||||
assert!(g.check(&orig).unwrap_err().contains("original of IMG_1.jpg"));
|
||||
assert!(g.check(&d.join("sub/../IMG_1.jpg")).is_err(), "another spelling of the same file");
|
||||
assert!(g.check(&d.join("IMG_1.xmp")).unwrap_err().contains("sidecar"));
|
||||
assert!(g.check(&d.join("other.jpg")).is_ok(), "an ordinary existing file");
|
||||
assert!(g.check(&d.join("new.jpg")).is_ok(), "a free name");
|
||||
let _ = std::fs::remove_dir_all(&d);
|
||||
}
|
||||
}
|
||||
|
|
@ -258,13 +258,9 @@ pub fn sidecar_packet(p: &Photo, cat: &lightcraft_catalog::Catalog) -> String {
|
|||
lightcraft_meta::write_xmp_lc(&meta, &lc)
|
||||
}
|
||||
|
||||
/// Write `data` to `path` atomically (temp file + rename).
|
||||
/// Write `data` to `path` atomically (temp file + sync + rename).
|
||||
fn write_atomic(path: &Path, data: &[u8]) -> std::io::Result<()> {
|
||||
let tmp = path.with_extension(format!("xmp.tmp-{}", std::process::id()));
|
||||
std::fs::write(&tmp, data)?;
|
||||
std::fs::rename(&tmp, path).inspect_err(|_| {
|
||||
let _ = std::fs::remove_file(&tmp);
|
||||
})
|
||||
lightcraft_catalog::safe_file::write_atomic(path, data)
|
||||
}
|
||||
|
||||
fn file_path(p: &Photo) -> Option<&str> {
|
||||
|
|
|
|||
|
|
@ -207,3 +207,119 @@ fn export_falls_back_to_the_cpu_when_gpu_work_is_lost() {
|
|||
assert!(m > 0.02, "not black: mean {m}");
|
||||
assert!((m - healthy).abs() < 0.01, "GPU {healthy} vs CPU {m}");
|
||||
}
|
||||
|
||||
/// A JPEG original `dir/IMG_1.jpg` imported into a library on disk.
|
||||
fn library_with_jpeg(tag: &str) -> (Session, lightcraft_catalog::PhotoId, std::path::PathBuf, std::path::PathBuf, Vec<u8>) {
|
||||
let dir = temp_dir(tag);
|
||||
let src = dir.join("IMG_1.jpg");
|
||||
let img = lightcraft_raster::Rgba8::from_fn(48, 32, |x, y| [(x * 5) as u8, (y * 7) as u8, 120, 255]);
|
||||
let bytes = crate::export::encode_image(&img, &ExportOptions::default()).unwrap();
|
||||
std::fs::write(&src, &bytes).unwrap();
|
||||
let mut s = Session::new().with_fs();
|
||||
s.execute("library.import", &json!({"paths": [src.to_string_lossy()]})).unwrap();
|
||||
let id = s.catalog.photos().next().unwrap().id;
|
||||
s.execute("develop.set", &json!({"ids": [id.0], "control": "light.exposure", "value": 1.0})).unwrap();
|
||||
(s, id, dir, src, bytes)
|
||||
}
|
||||
|
||||
fn disk_batch(
|
||||
s: &mut Session,
|
||||
id: lightcraft_catalog::PhotoId,
|
||||
o: &ExportOptions,
|
||||
to: &crate::export::Destination,
|
||||
) -> Result<Vec<serde_json::Value>, String> {
|
||||
crate::export::export_batch(s, &[id], o, to, &mut crate::export::write_file, &|p| std::path::Path::new(p).exists())
|
||||
}
|
||||
|
||||
/// Issue #93: exporting into the photo's own folder as `{name}` with "Overwrite" replaced the
|
||||
/// original with the re-encoded render. It is refused now, whatever the policy or exact path.
|
||||
#[test]
|
||||
fn export_never_overwrites_an_original() {
|
||||
use crate::export::{Conflict, Destination};
|
||||
let (mut s, id, dir, src, original) = library_with_jpeg("export-guard");
|
||||
let folder = Destination { dir: dir.to_string_lossy().to_string(), exact: None };
|
||||
let o = ExportOptions { naming: "{name}".into(), conflict: Conflict::Overwrite, ..Default::default() };
|
||||
let err = disk_batch(&mut s, id, &o, &folder).unwrap_err();
|
||||
assert!(err.contains("original of IMG_1.jpg"), "{err}");
|
||||
assert_eq!(std::fs::read(&src).unwrap(), original, "the original is byte-identical");
|
||||
|
||||
// the exact path of the control channel / MCP, also spelled another way
|
||||
std::fs::create_dir_all(dir.join("sub")).unwrap();
|
||||
for exact in [src.to_string_lossy().to_string(), dir.join("sub/../IMG_1.jpg").to_string_lossy().to_string()] {
|
||||
let to = Destination { dir: String::new(), exact: Some(exact) };
|
||||
assert!(disk_batch(&mut s, id, &o, &to).unwrap_err().contains("never writes over an original"));
|
||||
}
|
||||
// the "Original" format re-writing the file onto itself (and its sidecar)
|
||||
let orig = ExportOptions { format: ExportFormat::Original, ..o.clone() };
|
||||
assert!(disk_batch(&mut s, id, &orig, &folder).is_err());
|
||||
assert_eq!(std::fs::read(&src).unwrap(), original);
|
||||
assert!(s.execute("export.checkTarget", &json!({"path": src.to_string_lossy()})).is_err());
|
||||
assert!(s.execute("export.checkTarget", &json!({"path": dir.join("free.jpg").to_string_lossy()})).is_ok());
|
||||
|
||||
// Unique moves past it; Overwrite still replaces an ordinary earlier export
|
||||
let unique = ExportOptions { conflict: Conflict::Unique, ..o.clone() };
|
||||
let files = disk_batch(&mut s, id, &unique, &folder).unwrap();
|
||||
assert!(files[0]["path"].as_str().unwrap().ends_with("IMG_1-2.jpg"), "{files:?}");
|
||||
let earlier = dir.join("IMG_1-2.jpg");
|
||||
std::fs::write(&earlier, b"an earlier export").unwrap();
|
||||
let named = ExportOptions { naming: "{name}-2".into(), ..o };
|
||||
disk_batch(&mut s, id, &named, &folder).unwrap();
|
||||
assert_ne!(std::fs::read(&earlier).unwrap(), b"an earlier export", "an ordinary file is overwritten");
|
||||
assert_eq!(std::fs::read(&src).unwrap(), original);
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
}
|
||||
|
||||
/// Issue #93: the XMP sidecar of an "Original" export ignored the conflict policy.
|
||||
#[test]
|
||||
fn export_sidecars_follow_the_conflict_policy() {
|
||||
use crate::export::{Conflict, Destination};
|
||||
let (mut s, id, dir, _src, original) = library_with_jpeg("export-sidecar");
|
||||
let out = dir.join("out");
|
||||
std::fs::create_dir_all(&out).unwrap();
|
||||
let theirs = out.join("IMG_1.xmp");
|
||||
std::fs::write(&theirs, b"someone else's sidecar").unwrap();
|
||||
let to = Destination { dir: out.to_string_lossy().to_string(), exact: None };
|
||||
let o = ExportOptions { format: ExportFormat::Original, naming: "{name}".into(), ..Default::default() };
|
||||
|
||||
// Unique: the photo and its sidecar both move to the next free name
|
||||
let files = disk_batch(&mut s, id, &ExportOptions { conflict: Conflict::Unique, ..o.clone() }, &to).unwrap();
|
||||
assert!(files[0]["path"].as_str().unwrap().ends_with("IMG_1-2.jpg"), "{files:?}");
|
||||
assert!(files[0]["sidecars"][0].as_str().unwrap().ends_with("IMG_1-2.xmp"), "{files:?}");
|
||||
assert_eq!(std::fs::read(&theirs).unwrap(), b"someone else's sidecar");
|
||||
assert_eq!(std::fs::read(out.join("IMG_1-2.jpg")).unwrap(), original);
|
||||
// Skip: a taken sidecar name skips the photo
|
||||
std::fs::remove_file(out.join("IMG_1-2.jpg")).unwrap();
|
||||
std::fs::remove_file(out.join("IMG_1-2.xmp")).unwrap();
|
||||
let files = disk_batch(&mut s, id, &ExportOptions { conflict: Conflict::Skip, ..o.clone() }, &to).unwrap();
|
||||
assert!(files[0]["skipped"].as_str().is_some(), "{files:?}");
|
||||
assert!(!out.join("IMG_1.jpg").exists());
|
||||
assert_eq!(std::fs::read(&theirs).unwrap(), b"someone else's sidecar");
|
||||
// Overwrite: replaced, as asked
|
||||
disk_batch(&mut s, id, &ExportOptions { conflict: Conflict::Overwrite, ..o }, &to).unwrap();
|
||||
assert!(std::fs::read_to_string(&theirs).unwrap().contains("xmpmeta"));
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
}
|
||||
|
||||
/// Issue #93: the native writer truncated the target first; a failure mid-write (full disk,
|
||||
/// unplugged drive) now leaves the previous file intact and no partial one.
|
||||
#[test]
|
||||
fn a_failed_export_write_keeps_the_previous_file() {
|
||||
use crate::export::{Conflict, Destination};
|
||||
let (mut s, id, dir, _src, _) = library_with_jpeg("export-midwrite");
|
||||
let out = dir.join("out");
|
||||
std::fs::create_dir_all(&out).unwrap();
|
||||
let prev = out.join("IMG_1.jpg");
|
||||
std::fs::write(&prev, b"last week's export").unwrap();
|
||||
let to = Destination { dir: out.to_string_lossy().to_string(), exact: None };
|
||||
let o = ExportOptions { conflict: Conflict::Overwrite, ..Default::default() };
|
||||
{
|
||||
let _fault = lightcraft_catalog::safe_file::fail_writes_after(100);
|
||||
let err = disk_batch(&mut s, id, &o, &to).unwrap_err();
|
||||
assert!(err.contains("injected"), "{err}");
|
||||
}
|
||||
assert_eq!(std::fs::read(&prev).unwrap(), b"last week's export");
|
||||
assert_eq!(std::fs::read_dir(&out).unwrap().count(), 1, "no temp file left behind");
|
||||
disk_batch(&mut s, id, &o, &to).unwrap();
|
||||
assert_ne!(std::fs::read(&prev).unwrap(), b"last week's export");
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -78,12 +78,7 @@ impl Headless {
|
|||
None => vec![self.photo_or_active(p)?],
|
||||
};
|
||||
let dir = p.get("dir").and_then(Value::as_str).unwrap_or("");
|
||||
let write = &mut |path: &str, bytes: &[u8]| {
|
||||
if let Some(parent) = Path::new(path).parent().filter(|d| !d.as_os_str().is_empty()) {
|
||||
std::fs::create_dir_all(parent).map_err(|err| err.to_string())?;
|
||||
}
|
||||
std::fs::write(path, bytes).map_err(|err| format!("{path}: {err}"))
|
||||
};
|
||||
let write = &mut lightcraft_engine::export::write_file;
|
||||
let files =
|
||||
export_batch(&mut self.session, &ids, &opts, &Destination { dir: dir.to_string(), exact: exact.map(str::to_string) }, write, &|path| {
|
||||
Path::new(path).exists()
|
||||
|
|
@ -118,6 +113,7 @@ impl Backend for Headless {
|
|||
let img = self.render(&p, 1600)?;
|
||||
match p.get("path").and_then(Value::as_str) {
|
||||
Some(path) => {
|
||||
self.session.check_write_target(path)?;
|
||||
write_image(Path::new(path), &img, 92)?;
|
||||
Ok(json!({"path": path, "width": img.width, "height": img.height}))
|
||||
}
|
||||
|
|
@ -183,7 +179,7 @@ pub fn encode_image(ext: &str, img: &Rgba8, quality: u8) -> Result<Vec<u8>, Stri
|
|||
pub fn write_image(path: &Path, img: &Rgba8, quality: u8) -> Result<(), String> {
|
||||
let ext = path.extension().map(|e| e.to_string_lossy().to_string()).unwrap_or_default();
|
||||
let bytes = encode_image(&ext, img, quality)?;
|
||||
std::fs::write(path, bytes).map_err(|e| format!("{}: {e}", path.display()))
|
||||
lightcraft_engine::export::write_file(&path.to_string_lossy(), &bytes)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
|
|
|
|||
|
|
@ -1,6 +1,6 @@
|
|||
use serde_json::{Value, json};
|
||||
|
||||
use crate::{Headless, PROTOCOL_VERSION, Server, call_tool, command_tool_name};
|
||||
use crate::{Backend, Headless, PROTOCOL_VERSION, Server, call_tool, command_tool_name};
|
||||
|
||||
fn server() -> Server {
|
||||
Server::new(Box::new(Headless::demo()))
|
||||
|
|
@ -92,6 +92,34 @@ fn import_tool_copies_with_a_folder_template() {
|
|||
let _ = std::fs::remove_dir_all(&base);
|
||||
}
|
||||
|
||||
/// Issue #93: `render_photo`, `ui.render` and `app.export` with an exact `path` must not replace
|
||||
/// a photo's original.
|
||||
#[test]
|
||||
fn path_writes_never_replace_an_original() {
|
||||
let base = std::env::temp_dir().join(format!("lc-mcp-guard-{}", std::process::id()));
|
||||
let _ = std::fs::remove_dir_all(&base);
|
||||
std::fs::create_dir_all(&base).unwrap();
|
||||
let img = lightcraft_raster::Rgba8 { width: 8, height: 8, data: vec![[90, 30, 9, 255]; 64] };
|
||||
let png = lightcraft_codecs::encode_png(&lightcraft_codecs::EncodeImage::rgba8(&img), &lightcraft_codecs::EncodeMeta::default()).unwrap();
|
||||
let orig = base.join("a.png");
|
||||
std::fs::write(&orig, &png).unwrap();
|
||||
let mut b = Headless::demo();
|
||||
let r = call_tool(&mut b, "import", &json!({"paths": [orig.to_string_lossy()]}));
|
||||
assert!(!r.is_error, "{r:?}");
|
||||
let id = b.session.catalog.photos().find(|p| p.file_name == "a.png").unwrap().id.0;
|
||||
let path = orig.to_string_lossy().to_string();
|
||||
let r = call_tool(&mut b, "render_photo", &json!({"id": id, "path": path}));
|
||||
assert!(r.is_error, "{r:?}");
|
||||
assert!(b.call("ui.render", json!({"id": id, "path": path})).unwrap_err().contains("original"));
|
||||
let e = b.call("app.export", json!({"ids": [id], "path": path})).unwrap_err();
|
||||
assert!(e.contains("never writes over an original"), "{e}");
|
||||
assert_eq!(std::fs::read(&orig).unwrap(), png, "the original is untouched");
|
||||
// another path still works
|
||||
let r = call_tool(&mut b, "render_photo", &json!({"id": id, "path": base.join("render.png").to_string_lossy()}));
|
||||
assert!(!r.is_error, "{r:?}");
|
||||
let _ = std::fs::remove_dir_all(&base);
|
||||
}
|
||||
|
||||
/// The `import` helper moves: renamed into the folder template, the source removed.
|
||||
#[test]
|
||||
fn import_tool_moves_with_a_folder_template() {
|
||||
|
|
|
|||
|
|
@ -352,9 +352,9 @@ fn image_result(file: &std::path::Path, max: Option<u32>, format: &str, save_to:
|
|||
(bytes, w, h)
|
||||
};
|
||||
if let Some(p) = save_to
|
||||
&& let Err(e) = std::fs::write(p, &bytes)
|
||||
&& let Err(e) = lightcraft_engine::export::write_file(p, &bytes)
|
||||
{
|
||||
return ToolResult::error(format!("{p}: {e}"));
|
||||
return ToolResult::error(e);
|
||||
}
|
||||
let mime = if jpeg { "image/jpeg" } else { "image/png" };
|
||||
let mut info = meta;
|
||||
|
|
@ -372,7 +372,18 @@ fn image_result(file: &std::path::Path, max: Option<u32>, format: &str, save_to:
|
|||
}
|
||||
}
|
||||
|
||||
/// A user-given `path` to save to must not be a photo's original (or its sidecar).
|
||||
fn check_save_path(b: &mut dyn Backend, args: &Value) -> Result<(), String> {
|
||||
match args.get("path").and_then(Value::as_str) {
|
||||
Some(p) => exec(b, "export.checkTarget", json!({"path": p})).map(|_| ()),
|
||||
None => Ok(()),
|
||||
}
|
||||
}
|
||||
|
||||
fn render_photo(b: &mut dyn Backend, args: &Value) -> ToolResult {
|
||||
if let Err(e) = check_save_path(b, args) {
|
||||
return ToolResult::error(e);
|
||||
}
|
||||
let size = args.get("size").and_then(Value::as_u64).unwrap_or(1024).clamp(16, 4096);
|
||||
let id = match args.get("id").and_then(Value::as_u64) {
|
||||
Some(id) => Some(id),
|
||||
|
|
@ -394,6 +405,9 @@ fn render_photo(b: &mut dyn Backend, args: &Value) -> ToolResult {
|
|||
}
|
||||
|
||||
fn screenshot(b: &mut dyn Backend, args: &Value) -> ToolResult {
|
||||
if let Err(e) = check_save_path(b, args) {
|
||||
return ToolResult::error(e);
|
||||
}
|
||||
let file = temp_path("screenshot");
|
||||
let path = file.to_string_lossy().to_string();
|
||||
match b.call("ui.screenshot", json!({"path": path})) {
|
||||
|
|
|
|||
|
|
@ -290,12 +290,19 @@ pub fn handle(app: &mut LightcraftApp, ctx: &egui::Context, req: &ControlRequest
|
|||
ok(Value::Null)
|
||||
}
|
||||
"ui.screenshot" => {
|
||||
// never over a photo's original
|
||||
if let Some(Err(e)) = s("path").map(|path| app.session.check_write_target(path)) {
|
||||
return err(e);
|
||||
}
|
||||
ctx.request_repaint();
|
||||
Outcome::Screenshot { path: s("path").map(str::to_string), headless: p.get("headless").and_then(Value::as_bool).unwrap_or(false) }
|
||||
}
|
||||
"ui.render" => {
|
||||
let id = p.get("id").and_then(Value::as_u64).map(lightcraft_catalog::PhotoId).or(app.session.active());
|
||||
let Some(id) = id else { return err("no photo") };
|
||||
if let Some(Err(e)) = s("path").map(|path| app.session.check_write_target(path)) {
|
||||
return err(e);
|
||||
}
|
||||
let size = p.get("size").and_then(Value::as_u64).unwrap_or(1600) as usize;
|
||||
match app.session.render_now(id, size, size) {
|
||||
Ok(r) => match (s("path"), app.services.png.as_ref()) {
|
||||
|
|
@ -356,11 +363,7 @@ pub fn export_active(app: &mut LightcraftApp, p: &Value) -> Result<Value, String
|
|||
let to = Destination { dir: dir.clone(), exact: p.get("path").and_then(Value::as_str).map(str::to_string) };
|
||||
let background = p.get("background").and_then(Value::as_bool).unwrap_or(false) && app.services.write_shared.is_some();
|
||||
let out = if background {
|
||||
let items = ids
|
||||
.iter()
|
||||
.enumerate()
|
||||
.map(|(i, id)| lightcraft_engine::export::prepare_export(&mut app.session, *id, &opts, i + 1))
|
||||
.collect::<Result<Vec<_>, _>>()?;
|
||||
let items = lightcraft_engine::export::prepare_batch(&mut app.session, &ids, &opts)?;
|
||||
crate::export_task::start(app, items, opts, to)?
|
||||
} else {
|
||||
let w = app.services.write.as_mut().ok_or("no writer")?;
|
||||
|
|
|
|||
|
|
@ -436,10 +436,10 @@ under the UI command that wraps it (a no-op error); the UI command now wins.
|
|||
| LR-EXP-METADATA | Metadata policy | P1 | ✅ | `cmd:app.export` (`metadata`, `removeLocation`) | |
|
||||
| LR-EXP-WATERMARK | Watermark | P1 | ✅ | `cmd:app.export` (`watermark`: text or `image` + `imageWidth`), `crates/engine/src/export.rs` (`Watermark`) | text (size, colour, shadow) or a graphic with transparency (width as % of the photo, converted to the output colour space); position, inset, opacity |
|
||||
| LR-EXP-NAMING | File naming | P1 | ✅ | `cmd:app.export` (`naming`, `startNumber`), `crates/engine/src/rename.rs` (`expand_tokens`) | one template language for export, Rename Photos and import renaming: `{name}`, `{num}` (number at the end of the name), `{seq}` / `{seq:N}`, `{date}` / `{date:%Y-%m-%d}`, `{folder}`, `{camera}`, `{lens}`, `{iso}`, `{rating}`, `{title}`, `{creator}`, `{ext}`; free-form template rather than a list of named schemes |
|
||||
| LR-EXP-LOCATION | Destination folder | P0 | ✅ | `cmd:app.export` (`dir`, `subfolder`, `conflict`: unique / overwrite / skip), `crates/ui-egui/src/panels/dialogs.rs` (Choose… folder picker) | folder picker on the desktop; existing files get `-2`, `-3`… by default |
|
||||
| LR-EXP-LOCATION | Destination folder | P0 | ✅ | `cmd:app.export` (`dir`, `subfolder`, `conflict`: unique / overwrite / skip), `cmd:export.checkTarget`, `crates/ui-egui/src/panels/dialogs.rs` (Choose… folder picker), `crates/engine/src/originals.rs`, `crates/catalog/src/safe_file.rs` | folder picker on the desktop; existing files get `-2`, `-3`… by default; the policy covers a file and its sidecars together; a catalogued original (or its sidecar) is never written over, whatever the policy or an exact `path` (export, `ui.render`, MCP `render_photo`, merge `previewPath`, `lightcraft-cli render`); files are written to a temp file, synced and renamed into place (issue #93) |
|
||||
| LR-EXP-PREVIOUS | Export with previous settings | P0 | ✅ | `cmd:app.exportPrevious`, `cmd:dialog.export` | last options persist in prefs.json; dialog prefilled; no named export presets yet |
|
||||
| LR-EXP-DNGOPT | DNG options | P2 | 🟡 | `cmd:app.export` (`dngCompression`), `crates/engine/src/export.rs` | compression: lossless JPEG (default), ZIP or none; no embedded JPEG preview size, no lossy DNG output |
|
||||
| LR-EXP-ORIGINAL | Original + XMP | P1 | ✅ | `cmd:app.export` (`format: original`) | file copied byte for byte, sidecar named after the output |
|
||||
| LR-EXP-ORIGINAL | Original + XMP | P1 | ✅ | `cmd:app.export` (`format: original`), `crates/engine/src/tests_export.rs` | file copied byte for byte, sidecar named after the output and subject to the conflict policy |
|
||||
| LR-EXP-PHOTOS | Export to the system photo library | P2 | ⬜ | | |
|
||||
| LR-EXP-PSD | Round trip to an external editor | P2 | ✅ | `cmd:photo.editExternal`, `cmd:photo.editInExternal`, `crates/engine/src/cmd/convert.rs` | a 16-bit TIFF `-Edit` copy with the edits (Adobe RGB / ProPhoto / P3 / sRGB) next to the original, added stacked on top of it and opened in the editor set in Settings ▸ General (or the system default) |
|
||||
|
||||
|
|
|
|||
|
|
@ -10,6 +10,13 @@
|
|||
- 23 new built-in presets: Portrait, Landscape, Urban, Food, Seasons, Vintage and B&W toners.
|
||||
|
||||
### Reliability
|
||||
- Exports and renders never write over a photo's original (issue #93): exporting into the photo's own folder with
|
||||
the same name and "Overwrite" (or Export with Previous repeating it), an exact output path from the control
|
||||
channel or MCP, a merge preview path or `lightcraft-cli render IMG.jpg -o IMG.jpg` is refused with a clear
|
||||
message, and the original is left byte for byte. Ordinary earlier exports are still overwritten when asked.
|
||||
Exported files are written to a temp file, synced and then renamed into place, so a full disk or an unplugged
|
||||
drive never leaves a truncated file; the XMP sidecar of an "Original" export follows the "If file exists"
|
||||
choice too.
|
||||
- Canon CR2 photos from the EOS 7D, 50D, 60D, 550D, 600D, 1200D, 1300D, 5D Mark II and 1D Mark IV (and other
|
||||
models whose sensor starts on a green-blue row) no longer come out magenta (issue #85): the colour-filter
|
||||
layout is read from each file instead of assumed.
|
||||
|
|
|
|||
Loading…
Reference in New Issue