From 6a2bb03ebf3c674823db9df63e5e8dbb18e4bf18 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 17 Oct 2026 08:54:02 -0700 Subject: [PATCH] PDF presets: a rename that can't be written keeps the preset under its old name Renaming a saved PDF preset (pdf.preset.save with newName) removed the old file from the PDF Presets folder before it wrote the new one. When that write failed, for example on a full disk, the folder held no file for the preset, and the preset was gone at the next start. The store now writes the new file first and removes the old one only when that write succeeded. A failed write leaves the preset in the folder under its old name. A rename that changes only the case of the name writes the old file and then renames it, and the old file gets its content back when the rename fails. The command still succeeds when the folder can't be written, and the failure is logged, as before. --- crates/engine/src/store.rs | 42 ++++++++++++++---- crates/engine/src/tests_store.rs | 76 ++++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+), 9 deletions(-) diff --git a/crates/engine/src/store.rs b/crates/engine/src/store.rs index f99e63036..7e3a860a3 100644 --- a/crates/engine/src/store.rs +++ b/crates/engine/src/store.rs @@ -133,22 +133,46 @@ impl Session { } /// Write the saved preset `preset` to the store, which held it as `old_name` when that is set - /// (a rename). Without a store, nothing happens. + /// (a rename). A rename writes the new file first and removes the old one only when that write + /// succeeded: after a failed write the store still holds the preset under its old name. When + /// the two names differ only in case, the old file is written and then renamed to the new name; + /// when that rename fails, the old file gets its content back. Without a store, nothing happens. pub(crate) fn store_pdf_preset(&self, old_name: Option<&str>, preset: &PdfPreset) { let Some(dir) = &self.pdf_preset_dir else { return }; let mut clean = preset.clone(); clean.settings.clear_passwords(); let file = format!("{}.{PRESET_FORMAT}", file_stem(&clean.name)); - if let Some(old) = old_name.map(|o| format!("{}.{PRESET_FORMAT}", file_stem(o))) - && old != file - { - take(dir, &old); - } - match serde_json::to_vec_pretty(&PresetFile::new(vec![clean])) { - Ok(bytes) => { + let bytes = match serde_json::to_vec_pretty(&PresetFile::new(vec![clean])) { + Ok(bytes) => bytes, + Err(e) => { + log::warn!("can't save the PDF preset `{}`: {e}", preset.name); + return; + } + }; + match old_name.map(|o| format!("{}.{PRESET_FORMAT}", file_stem(o))).filter(|old| *old != file) { + // Names equal in any case get file names that differ only in case. A file system that + // ignores case holds them as one file, which removing the old name would delete. + Some(old) if old.to_lowercase() == file.to_lowercase() => match read_capped(&dir.join(&old), MAX_PRESET_FILE) { + Ok(before) => { + if put(dir, &old, &bytes) + && let Err(e) = std::fs::rename(dir.join(&old), dir.join(&file)) + { + log::warn!("can't rename {} to {file}: {e}", dir.join(&old).display()); + put(dir, &old, &before); + } + } + Err(_) => { + put(dir, &file, &bytes); + } + }, + Some(old) => { + if put(dir, &file, &bytes) { + take(dir, &old); + } + } + None => { put(dir, &file, &bytes); } - Err(e) => log::warn!("can't save the PDF preset `{}`: {e}", preset.name), } } diff --git a/crates/engine/src/tests_store.rs b/crates/engine/src/tests_store.rs index f630d78eb..37eafe6e4 100644 --- a/crates/engine/src/tests_store.rs +++ b/crates/engine/src/tests_store.rs @@ -40,6 +40,82 @@ fn saved_presets_come_back_in_the_next_session_and_renames_and_deletes_follow() assert!(!dir.join(format!("{}.vcpdfpresets", file_stem("Old"))).exists()); } +#[test] +fn a_rename_that_cannot_be_written_keeps_the_preset_under_its_old_name() { + let dir = fresh_dir("rename-fails").join("PDF Presets"); + let mut a = Session::new(); + a.attach_pdf_preset_store(dir.clone()); + a.execute("pdf.preset.save", &json!({"name": "Web", "description": "for screens"})).unwrap(); + // The test puts a folder where the new name's file goes, so the write fails as it would on a full disk. + std::fs::create_dir(dir.join(format!("{}.vcpdfpresets", file_stem("Screen")))).unwrap(); + a.execute("pdf.preset.save", &json!({"name": "Web", "newName": "Screen"})).unwrap(); + + let mut b = Session::new(); + b.attach_pdf_preset_store(dir.clone()); + assert_eq!(saved_names(&mut b), ["Web"]); + assert_eq!(b.prefs.pdf_presets[0].description, "for screens"); + assert!(dir.join(format!("{}.vcpdfpresets", file_stem("Web"))).is_file()); +} + +#[test] +fn a_rename_that_only_changes_case_keeps_one_file() { + let dir = fresh_dir("rename-case").join("PDF Presets"); + let mut a = Session::new(); + a.attach_pdf_preset_store(dir.clone()); + a.execute("pdf.preset.save", &json!({"name": "Web"})).unwrap(); + a.execute("pdf.preset.save", &json!({"name": "Web", "newName": "WEB", "description": "renamed"})).unwrap(); + + let mut b = Session::new(); + b.attach_pdf_preset_store(dir.clone()); + assert_eq!(saved_names(&mut b), ["WEB"]); + assert_eq!(b.prefs.pdf_presets[0].description, "renamed"); + assert_eq!(file_names(&dir), [format!("{}.vcpdfpresets", file_stem("WEB"))]); +} + +fn file_names(dir: &std::path::Path) -> Vec { + std::fs::read_dir(dir).unwrap().map(|e| e.unwrap().file_name().to_string_lossy().into_owned()).collect() +} + +/// Windows does not allow replacing a read-only file or writing into it, so the write fails. +#[cfg(windows)] +#[test] +fn a_rename_that_only_changes_case_and_cannot_be_written_keeps_the_old_file() { + let dir = fresh_dir("rename-case-fails").join("PDF Presets"); + let mut a = Session::new(); + a.attach_pdf_preset_store(dir.clone()); + a.execute("pdf.preset.save", &json!({"name": "Web", "description": "for screens"})).unwrap(); + let old = dir.join(format!("{}.vcpdfpresets", file_stem("Web"))); + let writable = std::fs::metadata(&old).unwrap().permissions(); + let mut read_only = writable.clone(); + read_only.set_readonly(true); + std::fs::set_permissions(&old, read_only).unwrap(); + a.execute("pdf.preset.save", &json!({"name": "Web", "newName": "WEB"})).unwrap(); + std::fs::set_permissions(&old, writable).unwrap(); + + let mut b = Session::new(); + b.attach_pdf_preset_store(dir.clone()); + assert_eq!(saved_names(&mut b), ["Web"]); + assert_eq!(b.prefs.pdf_presets[0].description, "for screens"); + assert_eq!(file_names(&dir), [format!("{}.vcpdfpresets", file_stem("Web"))]); +} + +/// Where file names keep case apart, a folder named like the new file makes the rename fail. +#[cfg(not(any(windows, target_vendor = "apple")))] +#[test] +fn a_rename_that_only_changes_case_and_cannot_be_moved_keeps_the_old_preset() { + let dir = fresh_dir("rename-case-blocked").join("PDF Presets"); + let mut a = Session::new(); + a.attach_pdf_preset_store(dir.clone()); + a.execute("pdf.preset.save", &json!({"name": "Web", "description": "for screens"})).unwrap(); + std::fs::create_dir(dir.join(format!("{}.vcpdfpresets", file_stem("WEB")))).unwrap(); + a.execute("pdf.preset.save", &json!({"name": "Web", "newName": "WEB", "description": "renamed"})).unwrap(); + + let mut b = Session::new(); + b.attach_pdf_preset_store(dir.clone()); + assert_eq!(saved_names(&mut b), ["Web"]); + assert_eq!(b.prefs.pdf_presets[0].description, "for screens"); +} + #[test] fn a_folder_that_does_not_exist_yet_takes_the_presets_the_preferences_held() { let dir = fresh_dir("seed").join("PDF Presets");