From f541b90a9cc9cba13d5e5f63c7c715ab67099fa2 Mon Sep 17 00:00:00 2001 From: Jeff Bailey Date: Sat, 4 Apr 2026 23:47:25 +0100 Subject: [PATCH] tar: replace all string-based error variants with structured types (#149) * tar: replace all string-based error variants with structured types Every TarError variant now carries PathBuf and/or io::Error data instead of a pre-formatted String. Add thiserror derive macros so Display implementations are generated from #[error(...)] attributes in one central place. Eliminates the last generic TarOperation and InvalidArchive string variants. Add tar prefixes, update tests to match. --- Cargo.lock | 1 + Cargo.toml | 1 + src/uu/tar/Cargo.toml | 1 + src/uu/tar/src/errors.rs | 112 +++++++++++++-------------- src/uu/tar/src/operations/create.rs | 36 ++++----- src/uu/tar/src/operations/extract.rs | 15 ++-- src/uu/tar/src/operations/list.rs | 16 +--- src/uu/tar/tests/test_errors.rs | 99 +++++++++++++++++------ 8 files changed, 155 insertions(+), 126 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2bc1767..55fbe9b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1405,6 +1405,7 @@ dependencies = [ "clap", "regex", "tar", + "thiserror", "uucore", ] diff --git a/Cargo.toml b/Cargo.toml index 5a0c695..9a5f8a0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -43,6 +43,7 @@ phf = "0.13.0" phf_codegen = "0.13.0" rand = { version = "0.10.0" } regex = "1.10.4" +thiserror = "2.0.3" uucore = { git = "https://github.com/uutils/coreutils" } uutests = { git = "https://github.com/uutils/coreutils" } tar = "0.4" diff --git a/src/uu/tar/Cargo.toml b/src/uu/tar/Cargo.toml index 45ee8cc..3b341c7 100644 --- a/src/uu/tar/Cargo.toml +++ b/src/uu/tar/Cargo.toml @@ -18,6 +18,7 @@ clap = { workspace = true } regex = { workspace = true } tar = { workspace = true } chrono = { workspace = true } +thiserror = { workspace = true } [lib] path = "src/tar.rs" diff --git a/src/uu/tar/src/errors.rs b/src/uu/tar/src/errors.rs index 5c51ff4..5630a95 100644 --- a/src/uu/tar/src/errors.rs +++ b/src/uu/tar/src/errors.rs @@ -3,62 +3,70 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -use std::fmt; use std::io; +use std::path::PathBuf; +use thiserror::Error; use uucore::error::UError; /// Error types for tar operations -#[derive(Debug)] +#[derive(Debug, Error)] pub enum TarError { - /// I/O error occurred - IoError(io::Error), - /// Invalid archive format or corrupted archive - InvalidArchive(String), - /// File or directory not found - FileNotFound(String), - /// Permission denied - PermissionDenied(String), - /// General tar operation error - TarOperationError(String), -} + /// I/O error occurred while reading/writing + #[error("{0}")] + Io(#[from] io::Error), -/// Implements display formatting for TarError. -impl fmt::Display for TarError { - /// Formats the error for display to users - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match self { - TarError::IoError(err) => write!(f, "{err}"), - TarError::InvalidArchive(msg) => write!(f, "{msg}"), - TarError::FileNotFound(path) => { - write!(f, "{path}: Cannot open: No such file or directory") - } - TarError::PermissionDenied(path) => { - write!(f, "{path}: Cannot open: Permission denied") - } - TarError::TarOperationError(msg) => write!(f, "{msg}"), - } - } + /// Cannot read entries from archive + #[error("tar: Cannot read archive entries: {0}")] + CannotReadEntries(io::Error), + + /// Cannot read an individual archive entry + #[error("tar: Cannot read archive entry: {0}")] + CannotReadEntry(io::Error), + + /// Cannot read the path of an archive entry + #[error("tar: Cannot read entry path: {0}")] + CannotReadEntryPath(io::Error), + + /// File or directory not found + #[error("tar: {path}: Cannot open: No such file or directory")] + FileNotFound { path: PathBuf }, + + /// Permission denied when accessing file + #[error("tar: {path}: Cannot open: Permission denied")] + PermissionDenied { path: PathBuf }, + + /// Cannot create archive file + #[error("tar: Cannot create archive '{path}': {source}")] + CannotCreateArchive { path: PathBuf, source: io::Error }, + + /// Cannot add a directory to the archive + #[error("tar: Cannot add directory '{path}': {source}")] + CannotAddDirectory { path: PathBuf, source: io::Error }, + + /// Cannot add a file to the archive + #[error("tar: Cannot add file '{path}': {source}")] + CannotAddFile { path: PathBuf, source: io::Error }, + + /// Cannot extract an archive entry + #[error("tar: Cannot extract '{path}': {source}")] + CannotExtract { path: PathBuf, source: io::Error }, + + /// Cannot finalize the archive + #[error("tar: Cannot finalize archive: {0}")] + CannotFinalizeArchive(io::Error), } impl TarError { /// Create a TarError from an io::Error with file path context pub fn from_io_error(err: io::Error, path: &std::path::Path) -> Self { match err.kind() { - io::ErrorKind::NotFound => TarError::FileNotFound(path.display().to_string()), - io::ErrorKind::PermissionDenied => { - TarError::PermissionDenied(path.display().to_string()) - } - _ => TarError::IoError(err), - } - } -} - -impl std::error::Error for TarError { - /// Returns the underlying error cause, if any - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - match self { - TarError::IoError(err) => Some(err), - _ => None, + io::ErrorKind::NotFound => TarError::FileNotFound { + path: path.to_path_buf(), + }, + io::ErrorKind::PermissionDenied => TarError::PermissionDenied { + path: path.to_path_buf(), + }, + _ => TarError::Io(err), } } } @@ -66,20 +74,6 @@ impl std::error::Error for TarError { impl UError for TarError { /// Returns the exit code for this error type fn code(&self) -> i32 { - match self { - TarError::IoError(_) => 2, - TarError::InvalidArchive(_) => 2, - TarError::FileNotFound(_) => 2, - TarError::PermissionDenied(_) => 2, - TarError::TarOperationError(_) => 2, - } - } -} - -impl From for TarError { - /// Converts io::Error into the appropriate TarError variant - fn from(err: io::Error) -> Self { - // For generic io::Error without context, just wrap it - TarError::IoError(err) + 2 // TarError variants exit with code 2; argument/usage errors use code 64 (see tar.rs) } } diff --git a/src/uu/tar/src/operations/create.rs b/src/uu/tar/src/operations/create.rs index 2cde873..6e639dc 100644 --- a/src/uu/tar/src/operations/create.rs +++ b/src/uu/tar/src/operations/create.rs @@ -27,12 +27,9 @@ use uucore::error::UResult; /// - Files cannot be added due to I/O or permission errors pub fn create_archive(archive_path: &Path, files: &[&Path], verbose: bool) -> UResult<()> { // Create the output file - let file = File::create(archive_path).map_err(|e| { - TarError::TarOperationError(format!( - "Cannot create archive '{}': {}", - archive_path.display(), - e - )) + let file = File::create(archive_path).map_err(|e| TarError::CannotCreateArchive { + path: archive_path.to_path_buf(), + source: e, })?; // Create Builder instance @@ -42,7 +39,10 @@ pub fn create_archive(archive_path: &Path, files: &[&Path], verbose: bool) -> UR for &path in files { // Check if path exists if !path.exists() { - return Err(TarError::FileNotFound(path.display().to_string()).into()); + return Err(TarError::FileNotFound { + path: path.to_path_buf(), + } + .into()); } if verbose { @@ -81,30 +81,24 @@ pub fn create_archive(archive_path: &Path, files: &[&Path], verbose: bool) -> UR // If it's a directory, recursively add all contents if path.is_dir() { builder.append_dir_all(normalized_name, path).map_err(|e| { - TarError::TarOperationError(format!( - "Failed to add directory '{}': {}", - path.display(), - e - )) + TarError::CannotAddDirectory { + path: path.to_path_buf(), + source: e, + } })?; } else { // For files, add them directly builder .append_path_with_name(path, normalized_name) - .map_err(|e| { - TarError::TarOperationError(format!( - "Failed to add file '{}': {}", - path.display(), - e - )) + .map_err(|e| TarError::CannotAddFile { + path: path.to_path_buf(), + source: e, })?; } } // Finish writing the archive - builder - .finish() - .map_err(|e| TarError::TarOperationError(format!("Failed to finalize archive: {e}")))?; + builder.finish().map_err(TarError::CannotFinalizeArchive)?; Ok(()) } diff --git a/src/uu/tar/src/operations/extract.rs b/src/uu/tar/src/operations/extract.rs index 2668009..be14b11 100644 --- a/src/uu/tar/src/operations/extract.rs +++ b/src/uu/tar/src/operations/extract.rs @@ -35,17 +35,13 @@ pub fn extract_archive(archive_path: &Path, verbose: bool) -> UResult<()> { } // Iterate through entries for verbose output and error handling - for entry_result in archive - .entries() - .map_err(|e| TarError::InvalidArchive(format!("Failed to read archive entries: {e}")))? - { - let mut entry = entry_result - .map_err(|e| TarError::InvalidArchive(format!("Failed to read entry: {e}")))?; + for entry_result in archive.entries().map_err(TarError::CannotReadEntries)? { + let mut entry = entry_result.map_err(TarError::CannotReadEntry)?; // Get the path before unpacking (clone it so we can use it after borrowing entry mutably) let path = entry .path() - .map_err(|e| TarError::InvalidArchive(format!("Failed to read entry path: {e}")))? + .map_err(TarError::CannotReadEntryPath)? .to_path_buf(); if verbose { @@ -53,8 +49,9 @@ pub fn extract_archive(archive_path: &Path, verbose: bool) -> UResult<()> { } // Unpack the entry - entry.unpack_in(".").map_err(|e| { - TarError::TarOperationError(format!("Failed to extract '{}': {}", path.display(), e)) + entry.unpack_in(".").map_err(|e| TarError::CannotExtract { + path: path.clone(), + source: e, })?; } diff --git a/src/uu/tar/src/operations/list.rs b/src/uu/tar/src/operations/list.rs index 6826a50..98f1566 100644 --- a/src/uu/tar/src/operations/list.rs +++ b/src/uu/tar/src/operations/list.rs @@ -17,12 +17,8 @@ pub fn list_archive(archive_path: &Path, verbose: bool) -> UResult<()> { File::open(archive_path).map_err(|e| TarError::from_io_error(e, archive_path))?; let mut archive = Archive::new(file); - for entry_result in archive - .entries() - .map_err(|e| TarError::InvalidArchive(format!("Failed to read archive entries: {e}")))? - { - let entry = entry_result - .map_err(|e| TarError::InvalidArchive(format!("Failed to read entry: {e}")))?; + for entry_result in archive.entries().map_err(TarError::CannotReadEntries)? { + let entry = entry_result.map_err(TarError::CannotReadEntry)?; if verbose { // Collect all header fields into owned values before borrowing entry for the path, @@ -49,9 +45,7 @@ pub fn list_archive(archive_path: &Path, verbose: bool) -> UResult<()> { ) }; - let path = entry - .path() - .map_err(|e| TarError::InvalidArchive(format!("Failed to read entry path: {e}")))?; + let path = entry.path().map_err(TarError::CannotReadEntryPath)?; let type_char = match entry_type { tar::EntryType::Directory => 'd', @@ -79,9 +73,7 @@ pub fn list_archive(archive_path: &Path, verbose: bool) -> UResult<()> { path.display() ); } else { - let path = entry - .path() - .map_err(|e| TarError::InvalidArchive(format!("Failed to read entry path: {e}")))?; + let path = entry.path().map_err(TarError::CannotReadEntryPath)?; println!("{}", path.display()); } diff --git a/src/uu/tar/tests/test_errors.rs b/src/uu/tar/tests/test_errors.rs index 405d7df..98b7acb 100644 --- a/src/uu/tar/tests/test_errors.rs +++ b/src/uu/tar/tests/test_errors.rs @@ -4,37 +4,50 @@ // file that was distributed with this source code. use std::io; +use std::path::PathBuf; use uu_tar::errors::TarError; #[test] fn test_tar_error_display() { - let err = TarError::FileNotFound("test.txt".to_string()); + let err = TarError::FileNotFound { + path: PathBuf::from("test.txt"), + }; assert_eq!( err.to_string(), - "test.txt: Cannot open: No such file or directory" + "tar: test.txt: Cannot open: No such file or directory" ); - let err = TarError::InvalidArchive("corrupted header".to_string()); - assert_eq!(err.to_string(), "corrupted header"); - - let err = TarError::PermissionDenied("/root/file".to_string()); + let err = TarError::PermissionDenied { + path: PathBuf::from("/root/file"), + }; assert_eq!( err.to_string(), - "/root/file: Cannot open: Permission denied" + "tar: /root/file: Cannot open: Permission denied" ); - - let err = TarError::TarOperationError("failed to write".to_string()); - assert_eq!(err.to_string(), "failed to write"); } #[test] fn test_tar_error_code() { use uucore::error::UError; - assert_eq!(TarError::FileNotFound("test".to_string()).code(), 2); - assert_eq!(TarError::InvalidArchive("test".to_string()).code(), 2); - assert_eq!(TarError::PermissionDenied("test".to_string()).code(), 2); - assert_eq!(TarError::TarOperationError("test".to_string()).code(), 2); + assert_eq!( + TarError::FileNotFound { + path: PathBuf::from("test") + } + .code(), + 2 + ); + assert_eq!( + TarError::PermissionDenied { + path: PathBuf::from("test") + } + .code(), + 2 + ); + assert_eq!( + TarError::Io(io::Error::new(io::ErrorKind::Other, "test")).code(), + 2 + ); } #[test] @@ -43,8 +56,8 @@ fn test_io_error_conversion_not_found() { let tar_err = TarError::from(io_err); match tar_err { - TarError::IoError(e) => assert_eq!(e.kind(), io::ErrorKind::NotFound), - _ => panic!("Expected IoError variant"), + TarError::Io(e) => assert_eq!(e.kind(), io::ErrorKind::NotFound), + _ => panic!("Expected Io variant"), } } @@ -54,8 +67,8 @@ fn test_io_error_conversion_permission_denied() { let tar_err = TarError::from(io_err); match tar_err { - TarError::IoError(e) => assert_eq!(e.kind(), io::ErrorKind::PermissionDenied), - _ => panic!("Expected IoError variant"), + TarError::Io(e) => assert_eq!(e.kind(), io::ErrorKind::PermissionDenied), + _ => panic!("Expected Io variant"), } } @@ -65,28 +78,64 @@ fn test_io_error_conversion_other() { let tar_err = TarError::from(io_err); match tar_err { - TarError::IoError(e) => assert_eq!(e.kind(), io::ErrorKind::BrokenPipe), - _ => panic!("Expected IoError variant"), + TarError::Io(e) => assert_eq!(e.kind(), io::ErrorKind::BrokenPipe), + _ => panic!("Expected Io variant"), } } #[test] fn test_error_source() { let io_err = io::Error::other("some error"); - let tar_err = TarError::IoError(io_err); + let tar_err = TarError::Io(io_err); - // IoError should have a source + // Io should have a source assert!(std::error::Error::source(&tar_err).is_some()); // Other variants should not have a source - let tar_err = TarError::FileNotFound("test".to_string()); + let tar_err = TarError::FileNotFound { + path: PathBuf::from("test"), + }; assert!(std::error::Error::source(&tar_err).is_none()); } +#[test] +fn test_from_io_error_not_found() { + let io_err = io::Error::new(io::ErrorKind::NotFound, "not found"); + let tar_err = TarError::from_io_error(io_err, std::path::Path::new("myfile.txt")); + + assert!(matches!(tar_err, TarError::FileNotFound { .. })); + assert_eq!( + tar_err.to_string(), + "tar: myfile.txt: Cannot open: No such file or directory" + ); +} + +#[test] +fn test_from_io_error_permission_denied() { + let io_err = io::Error::new(io::ErrorKind::PermissionDenied, "denied"); + let tar_err = TarError::from_io_error(io_err, std::path::Path::new("/root/secret")); + + assert!(matches!(tar_err, TarError::PermissionDenied { .. })); + assert_eq!( + tar_err.to_string(), + "tar: /root/secret: Cannot open: Permission denied" + ); +} + +#[test] +fn test_from_io_error_other() { + let io_err = io::Error::new(io::ErrorKind::BrokenPipe, "broken"); + let tar_err = TarError::from_io_error(io_err, std::path::Path::new("file.txt")); + + assert!(matches!(tar_err, TarError::Io(_))); +} + #[test] fn test_tar_error_is_debug() { - let err = TarError::TarOperationError("test".to_string()); + let err = TarError::FileNotFound { + path: PathBuf::from("test"), + }; let debug_str = format!("{err:?}"); - assert!(debug_str.contains("TarOperationError")); + assert!(debug_str.contains("FileNotFound")); assert!(debug_str.contains("test")); }