From c305e23ab4f184bc331486b24d8881cd48442184 Mon Sep 17 00:00:00 2001 From: Denis Gomes <6731020+denisg0mes@users.noreply.github.com> Date: Sat, 24 Oct 2026 23:36:26 -0400 Subject: [PATCH 1/3] M7: don't save over a file whose images past the budget weren't loaded A native file whose images together pass IMAGE_BUDGET (4 GiB, or 20,000 images) opens without the images past it. They are intact in the file, but saving it again wrote the archive from the images in memory, so every save, autosave or `convert` onto itself deleted them for good. A 7.6 GiB `.fig` converted to a 7.3 GiB `.figcraft` reopened with 1,312 of its 2,337 images: one save would have dropped the other 1,025. Loading now tells images left out for the budget apart from damaged ones (`Loaded::images_past_budget`). A tab opened or reverted from such a file remembers the file, and saving over it is refused with a message that says how many images would be lost and to save under another name; saving a copy elsewhere works, and the original keeps its images. Auto-save skips the tab instead of retrying and failing every minute. `figcraft-cli convert` and `run --in X --out X` refuse to write over their input the same way. The same file is matched by its resolved path, so `./a.figcraft` and `a.figcraft` count as one (`file_access::same_file`). The note for each image left out for the budget now says it stays in the file, which can't be saved over without losing it. Damaged and missing images keep the old wording: they can't be kept, and saving drops them as before. The doc comment on report_lost_images said saving always drops the images it reports, which is no longer true for budget ones, so it went. This stops the data loss; it doesn't make such files editable in place. Loading images on demand would, and is discussed in the issue this links. Tests: the count and the note for both budgets; saving over the file refused with the file unchanged, Save As then working, and saving back over the original refused again; a snapshot encoded in the background refused too; a tab without images left out saving as before. Checked by hand on the 7.3 GiB file with file.save, file.saveAs, convert and run --out. Co-Authored-By: Claude Opus 5.5 --- apps/figcraft-cli/src/files.rs | 16 ++++- crates/engine/src/cmd/file.rs | 28 +++++++- crates/engine/src/file_access.rs | 5 ++ .../engine/src/images_left_in_file_tests.rs | 68 +++++++++++++++++++ crates/engine/src/lib.rs | 13 ++++ crates/format/src/image_budget_tests.rs | 13 ++++ crates/format/src/lib.rs | 37 ++++++---- crates/ui-egui/src/lib.rs | 6 +- 8 files changed, 167 insertions(+), 19 deletions(-) create mode 100644 crates/engine/src/images_left_in_file_tests.rs diff --git a/apps/figcraft-cli/src/files.rs b/apps/figcraft-cli/src/files.rs index 3d36a7f2..263e1ac9 100644 --- a/apps/figcraft-cli/src/files.rs +++ b/apps/figcraft-cli/src/files.rs @@ -292,8 +292,19 @@ pub fn export(file: &str, out: &str, t: &Target) -> CliResult { Ok(ExitCode::SUCCESS) } +fn refuse_dropping_images(input: &str, out: &str, images_left_out: usize) -> CliResult<()> { + if images_left_out > 0 && figcraft_engine::file_access::same_file(input, out) { + return Err(CliError::Failed(format!( + "{out} wasn't written: {images_left_out} of its images weren't loaded (together they pass what FigCraft reads from one file), and writing over it would delete them. Write to another path; {input} keeps them." + ))); + } + Ok(()) +} + pub fn convert(input: &str, out: &str) -> CliResult { - let doc = load(input)?; + let (loaded, _) = figcraft_engine::cmd::file::load_path(input, None).map_err(|e| CliError::Failed(format!("can't open {input}: {e}")))?; + refuse_dropping_images(input, out, loaded.images_past_budget)?; + let doc = loaded.doc; let page = doc .current_page .filter(|p| doc.pages.contains(p)) @@ -389,6 +400,9 @@ pub fn run_script(script: &str, input: Option<&str>, out: Option<&str>) -> CliRe } if let Some(out) = out { let ds = s.ds().map_err(|e| CliError::Failed(e.to_string()))?; + if let (Some(input), Some(left_out)) = (input, ds.images_lost_saving_to(out)) { + refuse_dropping_images(input, out, left_out)?; + } if ext(out) == "figcraft" { // Through the engine, so the version history is kept. write(out, &figcraft_engine::cmd::file::file_bytes(ds).map_err(|e| CliError::Failed(e.to_string()))?)?; diff --git a/crates/engine/src/cmd/file.rs b/crates/engine/src/cmd/file.rs index 4bafa56d..76042486 100644 --- a/crates/engine/src/cmd/file.rs +++ b/crates/engine/src/cmd/file.rs @@ -156,7 +156,10 @@ pub fn specs() -> Vec { let i = s.active; let uid = s.ds()?.uid; if let Some(slot) = s.docs.get_mut(i) { - *slot = DocState::new(loaded.doc, Some(path)).with_versions(loaded.versions).with_branches(loaded.branches); + *slot = DocState::new(loaded.doc, Some(path.clone())) + .with_versions(loaded.versions) + .with_branches(loaded.branches) + .with_images_left_in_file(&path, loaded.images_past_budget); } // Edits on its branch tabs are reverted too. crate::branches::close_tabs(s, uid, None); @@ -731,7 +734,13 @@ fn open(s: &mut Session, p: &Value) -> Result { }); } let summary = import_summary(&loaded.doc); - s.docs.push(DocState::new(loaded.doc, native.then_some(path)).with_versions(loaded.versions).with_branches(loaded.branches)); + let images_left_out = if native { loaded.images_past_budget } else { 0 }; + s.docs.push( + DocState::new(loaded.doc, native.then(|| path.clone())) + .with_versions(loaded.versions) + .with_branches(loaded.branches) + .with_images_left_in_file(&path, images_left_out), + ); s.active = s.docs.len() - 1; let mut out = json!({"index": s.active, "summary": summary}); out[if warnings { "warnings" } else { "repairs" }] = json!(loaded.issues); @@ -819,7 +828,9 @@ impl FileSnapshot { let Some(ds) = s.docs.iter_mut().find(|d| d.uid == self.uid && d.path == self.path && Arc::ptr_eq(&d.saved_doc, &self.saved)) else { return Ok(false); }; - write(self.path.as_deref().ok_or(EngineError::NoDocument)?, bytes)?; + let path = self.path.as_deref().ok_or(EngineError::NoDocument)?; + refuse_dropping_images(ds, path)?; + write(path, bytes)?; // The history or branches may have changed since: then they still count as unsaved. if ds.revision == self.revision { ds.versions_changed = false; @@ -831,6 +842,16 @@ impl FileSnapshot { } } +fn refuse_dropping_images(ds: &DocState, path: &str) -> Result<()> { + match ds.images_lost_saving_to(path) { + Some(count) => Err(EngineError::Other(format!( + "{} wasn't saved: {count} of its images weren't loaded (together they pass what FigCraft reads from one file), and saving over it would delete them. Save it under another name; this file keeps them.", + file_stem(path) + ))), + None => Ok(()), + } +} + /// The branch tabs of the file in tab `uid` were saved with it. fn branch_tabs_saved(s: &mut Session, uid: u64) { for tab in s.docs.iter_mut().filter(|d| d.branch_of.as_ref().is_some_and(|(m, _)| *m == uid)) { @@ -866,6 +887,7 @@ fn save_to(s: &mut Session, path: String) -> Result { } let i = crate::branches::file_tab(s); let ds = s.docs.get_mut(i).ok_or(EngineError::NoDocument)?; + refuse_dropping_images(ds, &path)?; let mut snapshot = FileSnapshot::of(ds); let follows_path = ds.path.as_deref().map_or(ds.doc.name == "Untitled", |old| ds.doc.name == file_stem(old)); if follows_path && ds.path.as_deref() != Some(path.as_str()) { diff --git a/crates/engine/src/file_access.rs b/crates/engine/src/file_access.rs index 709b34a7..0fda5e92 100644 --- a/crates/engine/src/file_access.rs +++ b/crates/engine/src/file_access.rs @@ -115,6 +115,11 @@ pub(crate) fn within_app_folder(path: &Path, folder: &Path) -> bool { matches!((std::fs::canonicalize(path), std::fs::canonicalize(folder)), (Ok(p), Ok(f)) if p.starts_with(&f)) } +pub fn same_file(a: impl AsRef, b: impl AsRef) -> bool { + let (a, b) = (a.as_ref(), b.as_ref()); + a == b || matches!((std::fs::canonicalize(a), std::fs::canonicalize(b)), (Ok(x), Ok(y)) if x == y) +} + /// `p` without Windows' `\\?\` prefix, for messages. fn shown(p: &Path) -> String { let s = p.to_string_lossy(); diff --git a/crates/engine/src/images_left_in_file_tests.rs b/crates/engine/src/images_left_in_file_tests.rs new file mode 100644 index 00000000..44456ced --- /dev/null +++ b/crates/engine/src/images_left_in_file_tests.rs @@ -0,0 +1,68 @@ +use serde_json::json; + +use crate::Session; +use crate::cmd::file::FileSnapshot; + +fn rect(s: &mut Session, x: f64) { + s.execute("shape.rectangle", &json!({"x": x, "y": 0, "width": 10, "height": 10})).unwrap(); +} + +fn opened_with_images_left_in_file(name: &str) -> (std::path::PathBuf, String, Session) { + let dir = std::env::temp_dir().join(format!("figcraft-images-left-{name}-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("big.figcraft").to_string_lossy().to_string(); + let mut s = Session::new(); + rect(&mut s, 0.0); + s.execute("file.save", &json!({"path": path})).unwrap(); + let mut t = Session::empty(); + t.execute("file.open", &json!({"path": path})).unwrap(); + let i = t.active; + let tab = t.docs.remove(i).with_images_left_in_file(&path, 3); + t.docs.insert(i, tab); + (dir, path, t) +} + +#[test] +fn a_file_with_images_left_out_is_not_saved_over() { + let (dir, path, mut s) = opened_with_images_left_in_file("save"); + let before = std::fs::read(&path).unwrap(); + rect(&mut s, 20.0); + let err = s.execute("file.save", &json!({})).unwrap_err().to_string(); + assert!(err.contains("3 of its images") && err.contains("another name"), "{err}"); + assert_eq!(std::fs::read(&path).unwrap(), before); + assert!(s.ds().unwrap().is_dirty()); + + let copy = dir.join("copy.figcraft").to_string_lossy().to_string(); + s.execute("file.saveAs", &json!({"path": copy})).unwrap(); + assert!(!s.ds().unwrap().is_dirty()); + rect(&mut s, 40.0); + s.execute("file.save", &json!({})).unwrap(); + assert!(s.execute("file.saveAs", &json!({"path": path})).is_err()); + assert_eq!(std::fs::read(&path).unwrap(), before); + let _ = std::fs::remove_dir_all(&dir); +} + +#[test] +fn a_snapshot_encoded_in_the_background_does_not_save_over_it() { + let (dir, path, mut s) = opened_with_images_left_in_file("snapshot"); + let before = std::fs::read(&path).unwrap(); + rect(&mut s, 20.0); + let file = FileSnapshot::of(s.ds().unwrap()); + let bytes = file.encode().unwrap(); + assert!(file.save(&mut s, &bytes).is_err()); + assert_eq!(std::fs::read(&path).unwrap(), before); + assert!(s.ds().unwrap().is_dirty()); + let _ = std::fs::remove_dir_all(&dir); +} + +#[test] +fn a_file_without_images_left_out_saves_as_before() { + let (dir, path, mut s) = opened_with_images_left_in_file("none"); + let i = s.active; + let tab = s.docs.remove(i).with_images_left_in_file(&path, 0); + s.docs.insert(i, tab); + rect(&mut s, 20.0); + s.execute("file.save", &json!({})).unwrap(); + assert!(!s.ds().unwrap().is_dirty()); + let _ = std::fs::remove_dir_all(&dir); +} diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 60ae4aa9..71bd5241 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -398,6 +398,7 @@ pub struct DocState { /// The last duplicate while its copies are the selection: Duplicate repeats its offset (see /// [`cmd::edit::Duplicated`]; not saved). pub(crate) last_duplicate: Option, + images_left_in_file: Option<(String, usize)>, } /// A version shown on the canvas in place of the document: read-only until it's restored or the @@ -459,11 +460,21 @@ impl DocState { rotation_origin: None, view_from_prefs: false, last_duplicate: None, + images_left_in_file: None, }; st.zoom_to_fit(); st } + pub fn with_images_left_in_file(mut self, path: &str, count: usize) -> Self { + self.images_left_in_file = (count > 0).then(|| (path.to_string(), count)); + self + } + + pub fn images_lost_saving_to(&self, path: &str) -> Option { + self.images_left_in_file.as_ref().filter(|(file, _)| file_access::same_file(file, path)).map(|(_, count)| *count) + } + /// The selected ruler guide (its owner and index): it counts while no layer is selected and it /// is still there on this page. pub fn selected_guide(&self) -> Option<(NodeId, usize)> { @@ -1749,6 +1760,8 @@ mod huge_stroke_tests; #[cfg(test)] mod id_params_behaviour_tests; #[cfg(test)] +mod images_left_in_file_tests; +#[cfg(test)] mod import_export_tests; #[cfg(test)] mod import_journal_tests; diff --git a/crates/format/src/image_budget_tests.rs b/crates/format/src/image_budget_tests.rs index 58bb4da1..8e10cd71 100644 --- a/crates/format/src/image_budget_tests.rs +++ b/crates/format/src/image_budget_tests.rs @@ -66,6 +66,19 @@ fn images_past_the_byte_budget_are_left_out_and_reported() { assert!(loaded.issues.iter().any(|i| i.contains(lost[0].as_str()) && i.contains("together")), "{:?}", loaded.issues); } +#[test] +fn images_past_the_budget_are_counted_and_stay_in_the_file() { + let (bytes, _, total) = file_with_images(3); + let loaded = load_within(&bytes, ImageBudget { images: 100, bytes: total - 1 }).unwrap(); + assert_eq!(loaded.images_past_budget, 1); + assert!(loaded.issues.iter().any(|i| i.contains("stays in the file")), "{:?}", loaded.issues); + assert!(!loaded.issues.iter().any(|i| i.contains("drops it")), "{:?}", loaded.issues); + let counted = load_within(&bytes, ImageBudget { images: 2, bytes: u64::MAX }).unwrap(); + assert_eq!(counted.images_past_budget, 1); + let within = load_within(&bytes, ImageBudget { images: 3, bytes: total }).unwrap(); + assert_eq!(within.images_past_budget, 0); +} + #[test] fn images_past_the_count_budget_are_left_out_and_reported() { let (bytes, _, _) = file_with_images(4); diff --git a/crates/format/src/lib.rs b/crates/format/src/lib.rs index 36975f56..9b94ccec 100644 --- a/crates/format/src/lib.rs +++ b/crates/format/src/lib.rs @@ -190,11 +190,12 @@ pub struct Loaded { pub issues: Vec, pub versions: Vec, pub branches: Vec, + pub images_past_budget: usize, } impl From for Loaded { fn from(doc: Document) -> Self { - Self { doc, issues: Vec::new(), versions: Vec::new(), branches: Vec::new() } + Self { doc, issues: Vec::new(), versions: Vec::new(), branches: Vec::new(), images_past_budget: 0 } } } @@ -494,8 +495,9 @@ pub(crate) fn load_within(bytes: &[u8], budget: ImageBudget) -> Result doc.published = lib.map(Arc::new), Err(e) => issues.push(format!("the published library was skipped: {e}")), @@ -513,17 +515,13 @@ pub(crate) fn load_within(bytes: &[u8], budget: ImageBudget) -> Result( - z: &mut zip::ZipArchive, - own: Option<&BTreeSet>, - budget: ImageBudget, -) -> (BTreeMap, Vec<(String, FormatError)>) { +fn read_images(z: &mut zip::ZipArchive, own: Option<&BTreeSet>, budget: ImageBudget) -> ImagesRead { let mut hashes: Vec = z.file_names().filter_map(|n| n.strip_prefix("images/")).filter(|h| !h.is_empty()).map(str::to_string).collect(); hashes.sort_by_key(|h| !own.is_some_and(|o| o.contains(h))); let mut images = BTreeMap::new(); @@ -537,14 +535,17 @@ fn read_images( ok }); let mut left = budget.bytes; + let mut past_budget = BTreeSet::new(); for (i, hash) in hashes.into_iter().enumerate() { let name = format!("images/{hash}"); if i >= budget.images { + past_budget.insert(hash.clone()); unread.push((hash, FormatError::Corrupt(format!("the file has more than {} images", budget.images)))); continue; } if z.by_name(&name).is_ok_and(|f| f.size() > left) { let mb = budget.bytes / (1024 * 1024); + past_budget.insert(hash.clone()); unread.push((hash, FormatError::Corrupt(format!("the file's images together pass {mb} MB")))); continue; } @@ -556,18 +557,22 @@ fn read_images( Err(e) => unread.push((hash, e)), } } - (images, unread) + ImagesRead { images, unread, past_budget } +} + +struct ImagesRead { + images: BTreeMap, + unread: Vec<(String, FormatError)>, + past_budget: BTreeSet, } /// Most lost images reported one by one (the rest as a count). const MAX_IMAGE_ISSUES: usize = 20; -/// Report the images a file lost on loading: members that couldn't be read (damaged, too large) -/// and images its document lists but the archive doesn't hold. They are left out, so saving the -/// file again would drop them for good. fn report_lost_images( issues: &mut Vec, unread: &[(String, FormatError)], + past_budget: &BTreeSet, listed: Option<&BTreeSet>, images: &BTreeMap, ) { @@ -578,7 +583,11 @@ fn report_lost_images( FormatError::Corrupt(m) | FormatError::Io(m) => m.clone(), FormatError::Newer(v) => format!("format version {v}"), }; - format!("image {hash} couldn't be read ({why}): it is left out, and saving this file again drops it") + if past_budget.contains(hash) { + format!("image {hash} couldn't be read ({why}): it is left out and stays in the file, which can't be saved over without losing it") + } else { + format!("image {hash} couldn't be read ({why}): it is left out, and saving this file again drops it") + } }) .collect(); let unread_hashes: BTreeSet<&str> = unread.iter().map(|(h, _)| h.as_str()).collect(); diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 91cd5614..b64e4127 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -1739,7 +1739,11 @@ impl FigApp { let on = !cfg!(target_arch = "wasm32") && self.session.prefs.auto_save; let saving = &self.ui.auto_saving; let due = |d: &&figcraft_engine::DocState| { - d.path.is_some() && d.branch_of.is_none() && d.preview.is_none() && d.is_dirty() && !saving.iter().any(|(uid, _)| *uid == d.uid) + d.path.as_deref().is_some_and(|p| d.images_lost_saving_to(p).is_none()) + && d.branch_of.is_none() + && d.preview.is_none() + && d.is_dirty() + && !saving.iter().any(|(uid, _)| *uid == d.uid) }; let changes = if on { self.session.docs.iter().filter(due).map(|d| d.revision).reduce(u64::wrapping_add) } else { None }; match changes { From 03f4e6e07081dffa6229f9a98d2040573fe70b5d Mon Sep 17 00:00:00 2001 From: Denis Gomes <6731020+denisg0mes@users.noreply.github.com> Date: Sun, 25 Oct 2026 18:41:58 -0400 Subject: [PATCH 2/3] M7: images-past-budget guard: hard links, and script output over its input Two gaps CodeRabbit found in the guard: - `same_file` compared paths, plain and resolved. A second hard link to the file is a different path to the same data, so `convert`, `run --out` and Save As to the link passed the guard and truncated the file. It now asks the file system (same-file: device and inode on Unix, volume and file index on Windows). same-file was already in the lock through walkdir. - `run --in X --out X` checked the tab the script left active. A script that ends on `file.duplicate` or `file.new` leaves a tab with no count of images left in X, so X was written over. The count is now taken when X is opened and checked against `--out` whatever tab is active; the active tab's own file is still checked as before. The script check is a separate function so it can be tested without a file past the 4 GiB budget. --- Cargo.lock | 1 + Cargo.toml | 1 + apps/figcraft-cli/src/files.rs | 41 ++++++++++++++++--- crates/engine/Cargo.toml | 1 + crates/engine/src/file_access.rs | 2 +- .../engine/src/images_left_in_file_tests.rs | 13 ++++++ 6 files changed, 52 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index eb904655..76da20b7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1624,6 +1624,7 @@ dependencies = [ "image", "log", "regex", + "same-file", "serde", "serde_json", "thiserror 2.0.21", diff --git a/Cargo.toml b/Cargo.toml index 64a8c8a2..e39d2e46 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -49,6 +49,7 @@ serde = { version = "1", features = ["derive", "rc"] } serde_json = { version = "1", features = ["float_roundtrip"] } regex = "1.13" log = "0.4" +same-file = "1.0.6" # All date/time conversion goes through chrono (no hand-rolled calendar code beyond the 400-year range extension; AGENTS.md). chrono = { version = "0.4.45", default-features = false, features = ["std"] } proptest = "1" diff --git a/apps/figcraft-cli/src/files.rs b/apps/figcraft-cli/src/files.rs index 263e1ac9..1a4f27b1 100644 --- a/apps/figcraft-cli/src/files.rs +++ b/apps/figcraft-cli/src/files.rs @@ -301,6 +301,16 @@ fn refuse_dropping_images(input: &str, out: &str, images_left_out: usize) -> Cli Ok(()) } +fn refuse_dropping_images_from_script(input: Option<(&str, usize)>, active: &figcraft_engine::DocState, out: &str) -> CliResult<()> { + if let Some((input, images_left_out)) = input { + refuse_dropping_images(input, out, images_left_out)?; + } + if let Some(images_left_out) = active.images_lost_saving_to(out) { + refuse_dropping_images(out, out, images_left_out)?; + } + Ok(()) +} + pub fn convert(input: &str, out: &str) -> CliResult { let (loaded, _) = figcraft_engine::cmd::file::load_path(input, None).map_err(|e| CliError::Failed(format!("can't open {input}: {e}")))?; refuse_dropping_images(input, out, loaded.images_past_budget)?; @@ -363,13 +373,14 @@ pub fn run_script(script: &str, input: Option<&str>, out: Option<&str>) -> CliRe let bytes = figcraft_engine::file_access::read(script).map_err(|e| CliError::Failed(e.to_string()))?; let text = String::from_utf8(bytes).map_err(|_| CliError::Failed(format!("{script}: not UTF-8")))?; let steps = parse_script(&text).map_err(|e| CliError::Failed(format!("{script}: {e}")))?; - let mut s = match input { + let (mut s, input_images_left_out) = match input { Some(path) => { let mut s = Session::empty(); s.execute("file.open", &json!({"path": path})).map_err(|e| CliError::Failed(format!("can't open {path}: {e}")))?; - s + let left_out = s.ds().map_err(|e| CliError::Failed(e.to_string()))?.images_lost_saving_to(path).unwrap_or(0); + (s, left_out) } - None => Session::new(), + None => (Session::new(), 0), }; // One undo step and all-or-nothing, the way the same commands behave through `command.batch` // (MCP, the control channel, plugins): `{"$step": n}` resolves against the steps before it, and @@ -400,9 +411,7 @@ pub fn run_script(script: &str, input: Option<&str>, out: Option<&str>) -> CliRe } if let Some(out) = out { let ds = s.ds().map_err(|e| CliError::Failed(e.to_string()))?; - if let (Some(input), Some(left_out)) = (input, ds.images_lost_saving_to(out)) { - refuse_dropping_images(input, out, left_out)?; - } + refuse_dropping_images_from_script(input.map(|input| (input, input_images_left_out)), ds, out)?; if ext(out) == "figcraft" { // Through the engine, so the version history is kept. write(out, &figcraft_engine::cmd::file::file_bytes(ds).map_err(|e| CliError::Failed(e.to_string()))?)?; @@ -544,4 +553,24 @@ mod tests { assert!(!out.exists(), "a script that failed writes nothing"); std::fs::remove_dir_all(dir).unwrap(); } + + #[test] + fn a_script_output_over_its_input_is_refused_whichever_tab_the_script_left_active() { + let dir = temp_dir("script-images-left-out"); + let input = dir.join("big.figcraft"); + std::fs::write(&input, b"kept").unwrap(); + let link = dir.join("link.figcraft"); + std::fs::hard_link(&input, &link).unwrap(); + let (input, link, other) = (input.to_str().unwrap(), link.to_str().unwrap(), dir.join("other.figcraft")); + let duplicate = Session::new(); + let active = duplicate.ds().unwrap(); + + for out in [input, link] { + let refused = refuse_dropping_images_from_script(Some((input, 3)), active, out); + assert!(matches!(&refused, Err(CliError::Failed(m)) if m.contains("3 of its images")), "{out}"); + } + assert!(refuse_dropping_images_from_script(Some((input, 3)), active, other.to_str().unwrap()).is_ok()); + assert!(refuse_dropping_images_from_script(Some((input, 0)), active, input).is_ok()); + std::fs::remove_dir_all(dir).unwrap(); + } } diff --git a/crates/engine/Cargo.toml b/crates/engine/Cargo.toml index 545d872c..1ac9f0e1 100644 --- a/crates/engine/Cargo.toml +++ b/crates/engine/Cargo.toml @@ -28,6 +28,7 @@ thiserror = { workspace = true } log = { workspace = true } base64 = { workspace = true } regex = { workspace = true } +same-file = { workspace = true } web-time = "1.1.0" [dev-dependencies] diff --git a/crates/engine/src/file_access.rs b/crates/engine/src/file_access.rs index 0fda5e92..7a8ab8c9 100644 --- a/crates/engine/src/file_access.rs +++ b/crates/engine/src/file_access.rs @@ -117,7 +117,7 @@ pub(crate) fn within_app_folder(path: &Path, folder: &Path) -> bool { pub fn same_file(a: impl AsRef, b: impl AsRef) -> bool { let (a, b) = (a.as_ref(), b.as_ref()); - a == b || matches!((std::fs::canonicalize(a), std::fs::canonicalize(b)), (Ok(x), Ok(y)) if x == y) + a == b || same_file::is_same_file(a, b).unwrap_or(false) } /// `p` without Windows' `\\?\` prefix, for messages. diff --git a/crates/engine/src/images_left_in_file_tests.rs b/crates/engine/src/images_left_in_file_tests.rs index 44456ced..98e1cd8b 100644 --- a/crates/engine/src/images_left_in_file_tests.rs +++ b/crates/engine/src/images_left_in_file_tests.rs @@ -66,3 +66,16 @@ fn a_file_without_images_left_out_saves_as_before() { assert!(!s.ds().unwrap().is_dirty()); let _ = std::fs::remove_dir_all(&dir); } + +#[test] +fn a_hard_link_to_the_file_is_not_saved_over() { + let (dir, path, mut s) = opened_with_images_left_in_file("hard-link"); + let link = dir.join("link.figcraft"); + let _ = std::fs::remove_file(&link); + std::fs::hard_link(&path, &link).unwrap(); + let before = std::fs::read(&path).unwrap(); + rect(&mut s, 20.0); + assert!(s.execute("file.saveAs", &json!({"path": link.to_string_lossy()})).is_err()); + assert_eq!(std::fs::read(&path).unwrap(), before); + let _ = std::fs::remove_dir_all(&dir); +} From df8edc6b8ce8c39b00d5a27424ca5bca9360795d Mon Sep 17 00:00:00 2001 From: bflatastic Date: Mon, 26 Oct 2026 09:12:21 +0700 Subject: [PATCH 3/3] #1314: refuse saving over the file only when save_to has a path (main's download has none) --- crates/engine/src/cmd/file.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/cmd/file.rs b/crates/engine/src/cmd/file.rs index 039a33ba..d75ede3e 100644 --- a/crates/engine/src/cmd/file.rs +++ b/crates/engine/src/cmd/file.rs @@ -900,7 +900,10 @@ fn save_to(s: &mut Session, path: Option) -> Result { } let i = crate::branches::file_tab(s); let ds = s.docs.get_mut(i).ok_or(EngineError::NoDocument)?; - refuse_dropping_images(ds, &path)?; + // A download (no path) writes over no file. + if let Some(path) = path.as_deref() { + refuse_dropping_images(ds, path)?; + } let mut snapshot = FileSnapshot::of(ds); let follows_path = ds.path.as_deref().map_or(ds.doc.name == "Untitled", |old| ds.doc.name == file_stem(old)); if let Some(path) = path.as_deref()