From ca90946d8d56935a0e2eacf76562e827bf764011 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Wed, 11 Nov 2026 21:46:54 -0800 Subject: [PATCH] Data Recovery: a folder error names the folder that is missing or is not a folder When a folder on the way to the Data Recovery folder was missing and couldn't be made, or was a file, the error named the area folder or the copy inside it, for example `/1794385145-1: Not a directory (os error 20)`. Errors on the store's folders and files now name the first folder on the way, from the top, that is not a folder (" is not a folder: ") or is missing ("couldn't make : " while the store makes it, " is missing" otherwise). When every folder on the way is there, the error names the folder or file it happened on. When reading a folder reports it missing, listing checks the folders on the way, so a file there is an error and not an empty list. --- crates/engine/src/cmd/recovery.rs | 65 +++++++++++++++++++---- crates/engine/src/tests/recovery.rs | 81 +++++++++++++++++++++++++++++ 2 files changed, 136 insertions(+), 10 deletions(-) diff --git a/crates/engine/src/cmd/recovery.rs b/crates/engine/src/cmd/recovery.rs index 3424da3dd..136ba51ce 100644 --- a/crates/engine/src/cmd/recovery.rs +++ b/crates/engine/src/cmd/recovery.rs @@ -274,7 +274,46 @@ impl FolderStore { builder.recursive(true); #[cfg(unix)] std::os::unix::fs::DirBuilderExt::mode(&mut builder, 0o700); - builder.create(dir).map_err(|e| format!("{}: {e}", dir.display())) + builder.create(dir).map_err(|e| Self::folder_error(dir, &e, true)) + } + + /// The first folder on the way to `dir` (`dir` included), from the top down, that is not a + /// folder (`true`) or is missing (`false`). None when every one of them is a folder or one of + /// them can't be checked. + fn in_the_way(dir: &std::path::Path) -> Option<(&std::path::Path, bool)> { + let mut way: Vec<&std::path::Path> = dir.ancestors().filter(|p| !p.as_os_str().is_empty()).collect(); + way.reverse(); + for p in way { + match std::fs::metadata(p) { + Ok(m) if m.is_dir() => {} + Ok(_) => return Some((p, true)), + Err(m) if m.kind() == std::io::ErrorKind::NotFound => return Some((p, false)), + Err(_) => return None, + } + } + None + } + + /// The message for error `e` on folder `dir`, which was being made (`making`) or read. It + /// names the first folder on the way to `dir` that is not a folder, or is missing (when + /// `making`, the one that couldn't be made), and otherwise `dir`. + fn folder_error(dir: &std::path::Path, e: &std::io::Error, making: bool) -> String { + match Self::in_the_way(dir) { + Some((p, true)) => format!("{} is not a folder: {e}", p.display()), + Some((p, false)) if making => format!("couldn't make {}: {e}", p.display()), + Some((p, false)) if p != dir => format!("{} is missing", p.display()), + _ => format!("{}: {e}", dir.display()), + } + } + + /// The message for error `e` on file `f`. It names the first folder on the way to `f` that is + /// not a folder or is missing, and otherwise `f`. + fn file_error(f: &std::path::Path, e: &std::io::Error) -> String { + match f.parent().and_then(Self::in_the_way) { + Some((p, true)) => format!("{} is not a folder: {e}", p.display()), + Some((p, false)) => format!("{} is missing", p.display()), + None => format!("{}: {e}", f.display()), + } } /// Refuse area folder `dir` unless it is a folder itself, not a link to one, and (on Unix) @@ -282,7 +321,7 @@ impl FolderStore { /// linked to a folder of its own in a shared recovery folder looked free, and claiming it /// wrote the copies through the link. fn own_area(dir: &std::path::Path) -> std::result::Result<(), String> { - let m = std::fs::symlink_metadata(dir).map_err(|e| format!("{}: {e}", dir.display()))?; + let m = std::fs::symlink_metadata(dir).map_err(|e| Self::folder_error(dir, &e, false))?; if !m.file_type().is_dir() { return Err(format!("{}: not a folder of its own", dir.display())); } @@ -294,11 +333,17 @@ impl FolderStore { } /// The plain names of the files (or, `dirs`, folders) in `dir`; none when it doesn't exist. + /// A file on the way to `dir` is an error: Windows reports such a folder as missing. fn names(dir: &std::path::Path, dirs: bool) -> std::result::Result, String> { let entries = match std::fs::read_dir(dir) { Ok(e) => e, - Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(vec![]), - Err(e) => return Err(format!("{}: {e}", dir.display())), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + return match Self::in_the_way(dir) { + Some((p, true)) => Err(format!("{} is not a folder: {e}", p.display())), + _ => Ok(vec![]), + }; + } + Err(e) => return Err(Self::folder_error(dir, &e, false)), }; Ok(entries .filter_map(|e| e.ok()) @@ -333,7 +378,7 @@ impl FolderStore { match e.kind() { std::io::ErrorKind::NotFound => Ok(Lock::Busy), std::io::ErrorKind::PermissionDenied if std::fs::symlink_metadata(path).is_ok() => Ok(Lock::Busy), - _ => Err(format!("{}: {e}", path.display())), + _ => Err(Self::file_error(path, &e)), } } @@ -345,7 +390,7 @@ impl FolderStore { Ok(()) | Err(std::fs::TryLockError::WouldBlock) => Ok(Lock::Busy), // The lock file was removed meanwhile, and locking it failed instead. Err(std::fs::TryLockError::Error(_)) if !Self::still_named(&file, path) => Ok(Lock::Busy), - Err(std::fs::TryLockError::Error(e)) => Err(format!("{}: {e}", path.display())), + Err(std::fs::TryLockError::Error(e)) => Err(Self::file_error(path, &e)), } } @@ -393,10 +438,10 @@ impl RecoveryStore for FolderStore { if let Some(dir) = f.parent() { Self::own_area(dir)?; } - if !std::fs::symlink_metadata(&f).map_err(|e| format!("{}: {e}", f.display()))?.file_type().is_file() { + if !std::fs::symlink_metadata(&f).map_err(|e| Self::file_error(&f, &e))?.file_type().is_file() { return Err(format!("{}: not a plain file", f.display())); } - std::fs::read(&f).map_err(|e| format!("{}: {e}", f.display())) + std::fs::read(&f).map_err(|e| Self::file_error(&f, &e)) } fn write(&self, name: &str, bytes: &[u8]) -> std::result::Result<(), String> { let f = self.file(name)?; @@ -404,12 +449,12 @@ impl RecoveryStore for FolderStore { Self::create_area(dir)?; Self::own_area(dir)?; } - vectorcraft_format::write_atomic(&f, bytes).map_err(|e| format!("{}: {e}", f.display())) + vectorcraft_format::write_atomic(&f, bytes).map_err(|e| Self::file_error(&f, &e)) } fn remove(&self, name: &str) -> std::result::Result<(), String> { let f = self.file(name)?; match std::fs::remove_file(&f) { - Err(e) if e.kind() != std::io::ErrorKind::NotFound => Err(format!("{}: {e}", f.display())), + Err(e) if e.kind() != std::io::ErrorKind::NotFound => Err(Self::file_error(&f, &e)), _ => Ok(()), } } diff --git a/crates/engine/src/tests/recovery.rs b/crates/engine/src/tests/recovery.rs index f0b6a0708..2b991e332 100644 --- a/crates/engine/src/tests/recovery.rs +++ b/crates/engine/src/tests/recovery.rs @@ -483,6 +483,87 @@ fn the_folder_store_reads_no_linked_entries() { let _ = std::fs::remove_dir_all(&base); } +/// A file where a folder of the store's path should be: the error names that file, not the area +/// folder or the copy below it, for a write, a lock, a read, a listing, a removal and a recovery +/// save. +#[cfg(unix)] +#[test] +fn folder_errors_name_a_file_in_the_way() { + let base = std::env::temp_dir().join(format!("vc-recovery-file-in-way-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&base); + std::fs::create_dir_all(&base).unwrap(); + let file = base.join("file"); + std::fs::write(&file, b"").unwrap(); + let folder = file.join("recovery"); + let store = FolderStore::new(&folder); + let mut s = Session::new(); + s.prefs.recovery_folder = folder.to_string_lossy().to_string(); + s.execute("file.new", &json!({"width": 50, "height": 50})).unwrap(); + s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 5, "height": 5})).unwrap(); + let errors = [ + ("write", store.write("1-1/Untitled-1-1.vectorcraft", b"copy").err()), + ("lock", store.lock("1-2").err()), + ("read", store.read("1-1/Untitled-1-1.vectorcraft").err()), + ("list", store.list().err()), + ("remove", store.remove("1-1/Untitled-1-1.vectorcraft").err()), + ("save", s.execute("file.recovery.save", &json!({})).err().map(|e| e.to_string())), + ]; + let _ = std::fs::remove_dir_all(&base); + let named = format!("{} is not a folder: ", file.display()); + let wrong: Vec<_> = errors.iter().filter(|(_, e)| !e.as_deref().is_some_and(|e| e.contains(&named))).collect(); + assert!(wrong.is_empty(), "{wrong:?}"); +} + +/// A folder of the store's path that is missing and can't be made: a write or a lock names it as +/// the folder it couldn't make, and a read names it as missing. An area folder that can't be made +/// in a store folder that is there is named the same way. +#[cfg(unix)] +#[test] +fn folder_errors_name_a_missing_folder_that_cannot_be_made() { + use std::os::unix::fs::PermissionsExt as _; + let base = std::env::temp_dir().join(format!("vc-recovery-missing-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&base); + let closed = base.join("closed"); + std::fs::create_dir_all(&closed).unwrap(); + std::fs::set_permissions(&closed, std::fs::Permissions::from_mode(0o500)).unwrap(); + let gone = closed.join("gone"); + let store = FolderStore::new(gone.join("recovery")); + let unmade = format!("couldn't make {}: ", gone.display()); + let area = FolderStore::new(&closed).write("1-1/Untitled-1-1.vectorcraft", b"copy").err(); + let errors = [ + ("write", store.write("1-1/Untitled-1-1.vectorcraft", b"copy").err(), unmade.clone()), + ("lock", store.lock("1-2").err(), unmade), + ("read", store.read("1-1/Untitled-1-1.vectorcraft").err(), format!("{} is missing", gone.display())), + ("area", area, format!("couldn't make {}: ", closed.join("1-1").display())), + ]; + std::fs::set_permissions(&closed, std::fs::Permissions::from_mode(0o700)).unwrap(); + let _ = std::fs::remove_dir_all(&base); + // An account that can write anywhere (root) makes the folders, so the test has nothing to check. + if errors[0].1.is_none() { + return; + } + let wrong: Vec<_> = errors.iter().filter(|(_, e, named)| !e.as_deref().is_some_and(|e| e.starts_with(named.as_str()))).collect(); + assert!(wrong.is_empty(), "{wrong:?}"); +} + +/// When every folder on the way is there, an error names the folder it happened on. +#[cfg(unix)] +#[test] +fn folder_errors_name_the_folder_when_every_folder_is_there() { + use std::os::unix::fs::PermissionsExt as _; + let base = std::env::temp_dir().join(format!("vc-recovery-unreadable-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&base); + let unreadable = base.join("unreadable"); + std::fs::create_dir_all(&unreadable).unwrap(); + std::fs::set_permissions(&unreadable, std::fs::Permissions::from_mode(0o300)).unwrap(); + let listed = FolderStore::new(&unreadable).list(); + std::fs::set_permissions(&unreadable, std::fs::Permissions::from_mode(0o700)).unwrap(); + let _ = std::fs::remove_dir_all(&base); + // An account that can read any folder (root) lists it, so the test has nothing to check. + let Err(err) = listed else { return }; + assert!(err.starts_with(&format!("{}: ", unreadable.display())), "{err}"); +} + /// A store in memory that notes each removal of an area (its lock file and folder, then its empty /// folder) and whether the area was still held then. #[derive(Default)]