diff --git a/crates/backend/src/local.rs b/crates/backend/src/local.rs index 29df4656..872877f7 100644 --- a/crates/backend/src/local.rs +++ b/crates/backend/src/local.rs @@ -315,7 +315,7 @@ impl ReadBackend for LocalBackend { }) .inspect(|r| { if let Err(err) = r { - error!("Error while listing files: {err:?}"); + error!("Error while listing files: {}", err.display_log()); } }) .filter_map(RusticResult::ok); @@ -557,7 +557,7 @@ impl WriteBackend for LocalBackend { if let Some(command) = &self.post_create_command { if let Err(err) = Self::call_command(tpe, id, &filename, command) { - warn!("post-create: {err}"); + warn!("post-create: {}", err.display_log()); } } Ok(()) @@ -587,7 +587,7 @@ impl WriteBackend for LocalBackend { )?; if let Some(command) = &self.post_delete_command { if let Err(err) = Self::call_command(tpe, id, &filename, command) { - warn!("post-delete: {err}"); + warn!("post-delete: {}", err.display_log()); } } Ok(()) diff --git a/crates/backend/src/opendal.rs b/crates/backend/src/opendal.rs index 0165d0e4..049e16b0 100644 --- a/crates/backend/src/opendal.rs +++ b/crates/backend/src/opendal.rs @@ -328,7 +328,7 @@ impl ReadBackend for OpenDALBackend { }) .inspect(|r| { if let Err(err) = r { - error!("Error while listing files: {err:?}"); + error!("Error while listing files: {}", err.display_log()); } }) .filter_map(RusticResult::ok) diff --git a/crates/core/src/archiver.rs b/crates/core/src/archiver.rs index e9ab4ae3..fa2f6db6 100644 --- a/crates/core/src/archiver.rs +++ b/crates/core/src/archiver.rs @@ -144,15 +144,15 @@ impl<'a, BE: DecryptFullBackend, I: ReadGlobalIndex> Archiver<'a, BE, I> { match src.size() { Ok(Some(size)) => p.set_length(size), Ok(None) => {} - Err(err) => warn!("error determining backup size: {err}"), + Err(err) => warn!("error determining backup size: {}", err.display_log()), } } }); // filter out errors and handle as_path let iter = src.entries().filter_map(|item| match item { - Err(e) => { - warn!("ignoring error {e}\n"); + Err(err) => { + warn!("ignoring error: {}", err.display_log()); None } Ok(ReadSourceEntry { path, node, open }) => { @@ -197,7 +197,7 @@ impl<'a, BE: DecryptFullBackend, I: ReadGlobalIndex> Archiver<'a, BE, I> { .filter_map(|item| match item { Ok(item) => Some(item), Err(err) => { - warn!("ignoring error: {err:?}"); + warn!("ignoring error: {}", err.display_log()); None } }) diff --git a/crates/core/src/archiver/parent.rs b/crates/core/src/archiver/parent.rs index f807ce9d..f1eb176f 100644 --- a/crates/core/src/archiver/parent.rs +++ b/crates/core/src/archiver/parent.rs @@ -98,7 +98,10 @@ impl Parent { let tree = tree_id.and_then(|tree_id| match Tree::from_backend(be, index, tree_id) { Ok(tree) => Some(tree), Err(err) => { - warn!("ignoring error when loading parent tree {tree_id}: {err}"); + warn!( + "ignoring error when loading parent tree {tree_id}: {}", + err.display_log() + ); None } }); @@ -202,7 +205,10 @@ impl Parent { |tree_id| match Tree::from_backend(be, index, tree_id) { Ok(tree) => Some(tree), Err(err) => { - warn!("ignoring error when loading parent tree {tree_id}: {err}"); + warn!( + "ignoring error when loading parent tree {tree_id}: {}", + err.display_log() + ); None } }, @@ -274,8 +280,9 @@ impl Parent { ParentResult::Matched(()) } else { warn!( - "missing blobs in index for unchanged file {path:?}; re-reading file", - ); + "missing blobs in index for unchanged file {}; re-reading file", + path.display() + ); ParentResult::NotFound } } diff --git a/crates/core/src/backend/cache.rs b/crates/core/src/backend/cache.rs index 8d82ecb0..d534cdfd 100644 --- a/crates/core/src/backend/cache.rs +++ b/crates/core/src/backend/cache.rs @@ -69,7 +69,10 @@ impl ReadBackend for CachedBackend { if tpe.is_cacheable() { if let Err(err) = self.cache.remove_not_in_list(tpe, &list) { - warn!("Error in cache backend removing files {tpe:?}: {err}"); + warn!( + "Error in cache backend removing files {tpe:?}: {}", + err.display_log() + ); } } @@ -95,12 +98,18 @@ impl ReadBackend for CachedBackend { match self.cache.read_full(tpe, id) { Ok(Some(data)) => return Ok(data), Ok(None) => {} - Err(err) => warn!("Error in cache backend reading {tpe:?},{id}: {err}"), + Err(err) => warn!( + "Error in cache backend reading {tpe:?},{id}: {}", + err.display_log() + ), } let res = self.be.read_full(tpe, id); if let Ok(data) = &res { if let Err(err) = self.cache.write_bytes(tpe, id, data) { - warn!("Error in cache backend writing {tpe:?},{id}: {err}"); + warn!( + "Error in cache backend writing {tpe:?},{id}: {}", + err.display_log() + ); } } res @@ -138,14 +147,20 @@ impl ReadBackend for CachedBackend { match self.cache.read_partial(tpe, id, offset, length) { Ok(Some(data)) => return Ok(data), Ok(None) => {} - Err(err) => warn!("Error in cache backend reading {tpe:?},{id}: {err}"), + Err(err) => warn!( + "Error in cache backend reading {tpe:?},{id}: {}", + err.display_log() + ), }; // read full file, save to cache and return partial content match self.be.read_full(tpe, id) { Ok(data) => { let range = offset as usize..(offset + length) as usize; if let Err(err) = self.cache.write_bytes(tpe, id, &data) { - warn!("Error in cache backend writing {tpe:?},{id}: {err}"); + warn!( + "Error in cache backend writing {tpe:?},{id}: {}", + err.display_log() + ); } Ok(Bytes::copy_from_slice(&data.slice(range))) } @@ -183,7 +198,10 @@ impl WriteBackend for CachedBackend { fn write_bytes(&self, tpe: FileType, id: &Id, cacheable: bool, buf: Bytes) -> RusticResult<()> { if cacheable || tpe.is_cacheable() { if let Err(err) = self.cache.write_bytes(tpe, id, &buf) { - warn!("Error in cache backend writing {tpe:?},{id}: {err}"); + warn!( + "Error in cache backend writing {tpe:?},{id}: {}", + err.display_log() + ); } } self.be.write_bytes(tpe, id, cacheable, buf) @@ -200,7 +218,10 @@ impl WriteBackend for CachedBackend { fn remove(&self, tpe: FileType, id: &Id, cacheable: bool) -> RusticResult<()> { if cacheable || tpe.is_cacheable() { if let Err(err) = self.cache.remove(tpe, id) { - warn!("Error in cache backend removing {tpe:?},{id}: {err}"); + warn!( + "Error in cache backend removing {tpe:?},{id}: {}", + err.display_log() + ); } } self.be.remove(tpe, id, cacheable) diff --git a/crates/core/src/backend/ignore.rs b/crates/core/src/backend/ignore.rs index a3c5cf2d..f33b49ce 100644 --- a/crates/core/src/backend/ignore.rs +++ b/crates/core/src/backend/ignore.rs @@ -358,10 +358,10 @@ impl ReadSource for LocalSource { fn size(&self) -> RusticResult> { let mut size = 0; for entry in self.builder.build() { - if let Err(e) = entry.and_then(|e| e.metadata()).map(|m| { + if let Err(err) = entry.and_then(|e| e.metadata()).map(|m| { size += if m.is_dir() { 0 } else { m.len() }; }) { - warn!("ignoring error {}", e); + warn!("ignoring error {err}"); } } Ok(Some(size)) @@ -656,8 +656,8 @@ fn map_entry( let links = if m.is_dir() { 0 } else { m.nlink() }; let extended_attributes = match list_extended_attributes(entry.path()) { - Err(e) => { - warn!("ignoring error: {e}\n"); + Err(err) => { + warn!("ignoring error: {err}"); vec![] } Ok(xattr_list) => xattr_list, diff --git a/crates/core/src/commands/backup.rs b/crates/core/src/commands/backup.rs index 0b7e6602..78327eba 100644 --- a/crates/core/src/commands/backup.rs +++ b/crates/core/src/commands/backup.rs @@ -267,7 +267,7 @@ pub(crate) fn backup( let (parent_id, parent) = opts.parent_opts.get_parent(repo, &snap, backup_stdin); match parent_id { Some(id) => { - info!("using parent {}", id); + info!("using parent {id}"); snap.parent = Some(id); } None => { @@ -276,7 +276,7 @@ pub(crate) fn backup( }; let be = DryRunBackend::new(repo.dbe().clone(), opts.dry_run); - info!("starting to backup {source}..."); + info!("starting to backup {source} ..."); let archiver = Archiver::new(be, index, repo.config(), parent, snap)?; let p = repo.pb.progress_bytes("backing up..."); diff --git a/crates/core/src/commands/check.rs b/crates/core/src/commands/check.rs index 7d0668ef..31a467a4 100644 --- a/crates/core/src/commands/check.rs +++ b/crates/core/src/commands/check.rs @@ -274,7 +274,10 @@ pub(crate) fn check_repository( .map(|(id, size)| (**id, *size)) .collect(); if let Err(err) = cache.remove_not_in_list(FileType::Pack, &ids) { - warn!("Error in cache backend removing pack files: {err}"); + warn!( + "Error in cache backend removing pack files: {}", + err.display_log() + ); } p.finish(); @@ -309,21 +312,13 @@ pub(crate) fn check_repository( let data = match be.read_full(FileType::Pack, &id) { Ok(data) => data, Err(err) => { - // FIXME: This needs different handling, now it prints a full display of RusticError - // Instead we should actually collect and return a list of errors on the happy path - // for `Check`, as this is a non-critical operation and we want to show all errors - // to the user. - error!("Error reading data for pack {id} : {err}"); + error!("Error reading data for pack {id} : {}", err.display_log()); return; } }; match check_pack(be, pack, data, &p) { Ok(()) => {} - // FIXME: This needs different handling, now it prints a full display of RusticError - // Instead we should actually collect and return a list of errors on the happy path - // for `Check`, as this is a non-critical operation and we want to show all errors - // to the user. - Err(err) => error!("Pack {id} is not valid: {err}",), + Err(err) => error!("Pack {id} is not valid: {}", err.display_log()), } }); p.finish(); @@ -362,7 +357,6 @@ fn check_hot_files( match files.remove(&id) { None => error!("hot file Type: {file_type:?}, Id: {id} does not exist in repo"), Some(size) if size != size_hot => { - // TODO: This should be an actual error not a log entry error!("Type: {file_type:?}, Id: {id}: hot size: {size_hot}, actual size: {size}"); } _ => {} //everything ok @@ -415,10 +409,16 @@ fn check_cache_files( be.read_full(file_type, &id), ) { (Err(err), _) => { - error!("Error reading cached file Type: {file_type:?}, Id: {id} : {err}"); + error!( + "Error reading cached file Type: {file_type:?}, Id: {id} : {}", + err.display_log() + ); } (_, Err(err)) => { - error!("Error reading file Type: {file_type:?}, Id: {id} : {err}"); + error!( + "Error reading file Type: {file_type:?}, Id: {id} : {}", + err.display_log() + ); } (Ok(Some(data_cached)), Ok(data)) if data_cached != data => { error!( diff --git a/crates/core/src/commands/repair/index.rs b/crates/core/src/commands/repair/index.rs index 4ff7dc2f..ba0ecfa9 100644 --- a/crates/core/src/commands/repair/index.rs +++ b/crates/core/src/commands/repair/index.rs @@ -90,7 +90,10 @@ pub(crate) fn repair_index( debug!("reading pack {id}..."); match PackHeader::from_file(be, id, size_hint, packsize) { Err(err) => { - warn!("error reading pack {id} (-> removing from index): {err}"); + warn!( + "error reading pack {id} (-> removing from index): {}", + err.display_log() + ); } Ok(header) => { let pack = IndexPack { diff --git a/crates/core/src/commands/repair/snapshots.rs b/crates/core/src/commands/repair/snapshots.rs index ebc43c87..1672e09b 100644 --- a/crates/core/src/commands/repair/snapshots.rs +++ b/crates/core/src/commands/repair/snapshots.rs @@ -213,8 +213,8 @@ pub(crate) fn repair_tree( } let (tree, mut changed) = Tree::from_backend(be, index, id).map_or_else( - |_err| { - warn!("tree {id} could not be loaded."); + |err| { + warn!("tree {id} could not be loaded: {}", err.display_log()); (Tree::new(), Changed::This) }, |tree| (tree, Changed::None), diff --git a/crates/core/src/error.rs b/crates/core/src/error.rs index 5d98035f..9d6b3933 100644 --- a/crates/core/src/error.rs +++ b/crates/core/src/error.rs @@ -19,7 +19,7 @@ //! `Result (==RusticResult)` or nested results like `RusticResult>`. //! So even if the visibility of that function is `fn` or `pub(crate) fn` it should return a `RusticResult` containing a `RusticError`. //! -//! If we `map_err` or `and_then` a `RusticError`, we don't want to create a new RusticError from it, but just attach some context +//! If we `map_err` or `and_then` a `RusticError`, we don't want to create a new `RusticError` from it, but just attach some context //! to it, e.g. `map_err(|e| e.attach_context("key", "value"))`, so we don't lose the original error. We can also change the error //! kind with `map_err(|e| e.overwrite_kind(ErrorKind::NewKind))`. If we want to pre- or append to the guidance, we can use //! `map_err(|e| e.append_guidance_line("new line"))` or `map_err(|e| e.prepend_guidance_line("new line"))`. @@ -48,11 +48,6 @@ //! All types that we want to attach to an error should implement `Display` and `Debug` to provide a good error message and a nice way //! to display the error. -// FIXME: Remove when 'displaydoc' has fixed/recommended further treatment upstream: https://github.com/yaahc/displaydoc/issues/48 -#![allow(clippy::doc_markdown)] -// use std::error::Error as StdError; -// use std::fmt; - use derive_more::derive::Display; use ecow::{EcoString, EcoVec}; use std::{ @@ -293,7 +288,7 @@ impl Display for RusticError { writeln!(f, "Caused by:")?; writeln!(f, "{cause}")?; if let Some(source) = cause.source() { - write!(f, " : (source: {source:?})")?; + write!(f, " : (source: {source})")?; } writeln!(f)?; } @@ -375,6 +370,53 @@ impl RusticError { ) -> Box { Self::with_source(kind, error.to_string(), error) } + + /// Returns a String representation for logging purposes. + /// + /// This is a more concise version of the error message. + pub fn display_log(&self) -> String { + use std::fmt::Write; + + let mut fmt = String::new(); + + _ = write!(fmt, "Error: "); + + if self.context.is_empty() { + _ = write!(fmt, "{}", self.guidance); + } else { + // If there is context, we want to iterate over it + // use the key to replace the placeholder in the guidance. + let mut guidance = self.guidance.to_string(); + + self.context + .iter() + // remove context which has been used in the guidance + .for_each(|(key, value)| { + let pattern = "{".to_owned() + key + "}"; + guidance = guidance.replace(&pattern, value); + }); + + _ = write!(fmt, "{guidance}"); + }; + + _ = write!(fmt, " (kind: related to {}", self.kind); + + if let Some(code) = &self.error_code { + _ = write!(fmt, ", code: {code}"); + } + + _ = write!(fmt, ")"); + + if let Some(cause) = &self.source { + _ = write!(fmt, ": caused by: {cause}"); + + if let Some(source) = cause.source() { + _ = write!(fmt, " : (source: {source})"); + } + } + + fmt + } } // Setters for anything we do want to expose publicly. diff --git a/crates/core/src/repofile/snapshotfile.rs b/crates/core/src/repofile/snapshotfile.rs index 66718986..90f17903 100644 --- a/crates/core/src/repofile/snapshotfile.rs +++ b/crates/core/src/repofile/snapshotfile.rs @@ -607,7 +607,7 @@ impl SnapshotFile { /// * If no id could be found. /// * If the id is not unique. pub(crate) fn from_id(be: &B, id: &str) -> RusticResult { - info!("getting snapshot..."); + info!("getting snapshot ..."); let id = be.find_id(FileType::Snapshot, id)?; Self::from_backend(be, &SnapshotId::from(id)) } diff --git a/crates/core/src/repository/warm_up.rs b/crates/core/src/repository/warm_up.rs index 22131a92..a1a74a6b 100644 --- a/crates/core/src/repository/warm_up.rs +++ b/crates/core/src/repository/warm_up.rs @@ -144,9 +144,9 @@ fn warm_up_repo( pool.in_place_scope(|scope| { for pack in packs { scope.spawn(move |_| { - if let Err(e) = backend.warm_up(FileType::Pack, &pack) { + if let Err(err) = backend.warm_up(FileType::Pack, &pack) { // FIXME: Use error handling - error!("warm-up failed for pack {pack:?}. {e}"); + error!("warm-up failed for pack {pack:?}. {}", err.display_log()); }; progress_bar_ref.inc(1); }); diff --git a/crates/core/tests/errors.rs b/crates/core/tests/errors.rs index 2171dd81..29842b99 100644 --- a/crates/core/tests/errors.rs +++ b/crates/core/tests/errors.rs @@ -20,12 +20,35 @@ fn error() -> Box { .ask_report() } +#[fixture] +fn minimal_error() -> Box { + RusticError::new( + ErrorKind::InputOutput, + "A file could not be read, make sure the file at `{path}` is existing and readable by the system.", + ) + .attach_context("path", "/path/to/file") +} + #[rstest] +#[allow(clippy::boxed_local)] fn test_error_display(error: Box) { insta::assert_snapshot!(error); } #[rstest] +#[allow(clippy::boxed_local)] fn test_error_debug(error: Box) { insta::assert_debug_snapshot!(error); } + +#[rstest] +#[allow(clippy::boxed_local)] +fn test_log_output_passes(error: Box) { + insta::assert_snapshot!(error.display_log()); +} + +#[rstest] +#[allow(clippy::boxed_local)] +fn test_log_output_minimal_passes(minimal_error: Box) { + insta::assert_snapshot!(minimal_error.display_log()); +} diff --git a/crates/core/tests/snapshots/errors__log_output_minimal_passes.snap b/crates/core/tests/snapshots/errors__log_output_minimal_passes.snap new file mode 100644 index 00000000..ca438435 --- /dev/null +++ b/crates/core/tests/snapshots/errors__log_output_minimal_passes.snap @@ -0,0 +1,5 @@ +--- +source: crates/core/tests/errors.rs +expression: minimal_error.to_log_output() +--- +Error: A file could not be read, make sure the file at `/path/to/file` is existing and readable by the system. (kind: related to input/output operations) diff --git a/crates/core/tests/snapshots/errors__log_output_passes.snap b/crates/core/tests/snapshots/errors__log_output_passes.snap new file mode 100644 index 00000000..2c9db8ed --- /dev/null +++ b/crates/core/tests/snapshots/errors__log_output_passes.snap @@ -0,0 +1,7 @@ +--- +source: crates/core/tests/errors.rs +expression: error.to_log_output() +--- +Error: Prepended guidance line +A file could not be read, make sure the file at `/path/to/file` is existing and readable by the system. +Appended guidance line (kind: related to input/output operations, code: C001): caused by: Networking Error