From a22bd4576ede80c32ab0dda164c9e17c29fb5ccb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20M=2E=20Bezerra?= Date: Sun, 17 Sep 2023 18:55:39 -0300 Subject: [PATCH 1/3] Add tests to the extension module --- src/extension.rs | 41 +++++++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/src/extension.rs b/src/extension.rs index d602ec7f0..0f7170b81 100644 --- a/src/extension.rs +++ b/src/extension.rs @@ -33,7 +33,8 @@ pub const PRETTY_SUPPORTED_EXTENSIONS: &str = "tar, zip, bz, bz2, gz, lz4, xz, l pub const PRETTY_SUPPORTED_ALIASES: &str = "tgz, tbz, tlz4, txz, tzlma, tsz, tzst"; /// A wrapper around `CompressionFormat` that allows combinations like `tgz` -#[derive(Debug, Clone, Eq)] +#[derive(Debug, Clone)] +#[cfg_attr(test, derive(PartialEq))] #[non_exhaustive] pub struct Extension { /// One extension like "tgz" can be made of multiple CompressionFormats ([Tar, Gz]) @@ -42,13 +43,6 @@ pub struct Extension { display_text: String, } -// The display_text should be ignored when comparing extensions -impl PartialEq for Extension { - fn eq(&self, other: &Self) -> bool { - self.compression_formats == other.compression_formats - } -} - impl Extension { /// # Panics: /// Will panic if `formats` is empty @@ -262,6 +256,37 @@ mod tests { assert_eq!(formats, vec![Tar, Gzip]); } + #[test] + fn test_separate_known_extensions_from_name() { + assert_eq!( + separate_known_extensions_from_name("file".as_ref()), + ("file".as_ref(), vec![]) + ); + assert_eq!( + separate_known_extensions_from_name("tar".as_ref()), + ("tar".as_ref(), vec![]) + ); + assert_eq!( + separate_known_extensions_from_name(".tar".as_ref()), + (".tar".as_ref(), vec![]) + ); + assert_eq!( + separate_known_extensions_from_name("file.tar".as_ref()), + ("file".as_ref(), vec![Extension::new(&[Tar], "tar")]) + ); + assert_eq!( + separate_known_extensions_from_name("file.tar.gz".as_ref()), + ( + "file".as_ref(), + vec![Extension::new(&[Tar], "tar"), Extension::new(&[Gzip], "gz")] + ) + ); + assert_eq!( + separate_known_extensions_from_name(".tar.gz".as_ref()), + (".tar".as_ref(), vec![Extension::new(&[Gzip], "gz")]) + ); + } + #[test] fn builds_suggestion_correctly() { assert_eq!(build_archive_file_suggestion(Path::new("linux.png"), ".tar"), None); From 795c2543910a4852e96cd5c38e6bbb443b3a550a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20M=2E=20Bezerra?= Date: Mon, 18 Sep 2023 00:07:07 -0300 Subject: [PATCH 2/3] Fix `--format` parsing extensions with dots Also improve error reporting for `--format` with malformed or unsupported extensions This commit is very messy, as it also does an refac in the project, which should ideally be in a separated commit --- src/archive/rar.rs | 2 +- src/archive/zip.rs | 2 +- src/check.rs | 8 +- src/cli/mod.rs | 1 - src/commands/mod.rs | 12 +-- src/error.rs | 96 ++++++++++++------- src/extension.rs | 70 +++++++++++--- src/list.rs | 1 - src/utils/formatting.rs | 22 +++-- src/utils/mod.rs | 22 +++-- src/utils/question.rs | 7 +- .../ui__ui_test_err_format_flag-2.snap | 14 +++ .../ui__ui_test_err_format_flag-3.snap | 15 +++ .../ui__ui_test_err_format_flag.snap | 14 +++ .../ui__ui_test_ok_format_flag-2.snap | 7 ++ .../snapshots/ui__ui_test_ok_format_flag.snap | 7 ++ tests/ui.rs | 25 ++++- tests/utils.rs | 1 + 18 files changed, 240 insertions(+), 86 deletions(-) create mode 100644 tests/snapshots/ui__ui_test_err_format_flag-2.snap create mode 100644 tests/snapshots/ui__ui_test_err_format_flag-3.snap create mode 100644 tests/snapshots/ui__ui_test_err_format_flag.snap create mode 100644 tests/snapshots/ui__ui_test_ok_format_flag-2.snap create mode 100644 tests/snapshots/ui__ui_test_ok_format_flag.snap diff --git a/src/archive/rar.rs b/src/archive/rar.rs index 225ec1f19..dc9f11a77 100644 --- a/src/archive/rar.rs +++ b/src/archive/rar.rs @@ -2,7 +2,7 @@ use std::path::Path; -use unrar::{self, Archive}; +use unrar::Archive; use crate::{error::Error, info, list::FileInArchive}; diff --git a/src/archive/zip.rs b/src/archive/zip.rs index e9bde15d6..e78adff02 100644 --- a/src/archive/zip.rs +++ b/src/archive/zip.rs @@ -14,7 +14,7 @@ use filetime_creation::{set_file_mtime, FileTime}; use fs_err as fs; use same_file::Handle; use time::OffsetDateTime; -use zip::{self, read::ZipFile, DateTime, ZipArchive}; +use zip::{read::ZipFile, DateTime, ZipArchive}; use crate::{ error::FinalError, diff --git a/src/check.rs b/src/check.rs index 9e1bdac37..afb3c3b01 100644 --- a/src/check.rs +++ b/src/check.rs @@ -10,7 +10,7 @@ use std::{ use crate::{ error::FinalError, - extension::{build_archive_file_suggestion, Extension, PRETTY_SUPPORTED_ALIASES, PRETTY_SUPPORTED_EXTENSIONS}, + extension::{build_archive_file_suggestion, Extension}, info, utils::{pretty_format_list_of_paths, try_infer_extension, user_wants_to_continue, EscapedPathDisplay}, warning, QuestionAction, QuestionPolicy, Result, @@ -159,10 +159,8 @@ pub fn check_missing_formats_when_decompressing(files: &[PathBuf], formats: &[Ve )); } - error = error - .detail("Decompression formats are detected automatically from file extension") - .hint(format!("Supported extensions are: {}", PRETTY_SUPPORTED_EXTENSIONS)) - .hint(format!("Supported aliases are: {}", PRETTY_SUPPORTED_ALIASES)); + error = error.detail("Decompression formats are detected automatically from file extension"); + error = error.hint_all_supported_formats(); // If there's exactly one file, give a suggestion to use `--format` if let &[path] = files_with_broken_extension.as_slice() { diff --git a/src/cli/mod.rs b/src/cli/mod.rs index cfcd67d0e..69bf2139b 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -5,7 +5,6 @@ mod args; use std::{ io, path::{Path, PathBuf}, - vec::Vec, }; use clap::Parser; diff --git a/src/commands/mod.rs b/src/commands/mod.rs index b9763d1fc..0d85a1576 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -14,10 +14,10 @@ use crate::{ cli::Subcommand, commands::{compress::compress_files, decompress::decompress_file, list::list_archive_contents}, error::{Error, FinalError}, - extension::{self, parse_format}, + extension::{self, parse_format_flag}, info, list::ListOptions, - utils::{self, to_utf, EscapedPathDisplay, FileVisibilityPolicy}, + utils::{self, path_to_str, EscapedPathDisplay, FileVisibilityPolicy}, warning, CliArgs, QuestionPolicy, }; @@ -66,7 +66,7 @@ pub fn run( // Formats from path extension, like "file.tar.gz.xz" -> vec![Tar, Gzip, Lzma] let (formats_from_flag, formats) = match args.format { Some(formats) => { - let parsed_formats = parse_format(&formats)?; + let parsed_formats = parse_format_flag(&formats)?; (Some(formats), parsed_formats) } None => (None, extension::extensions_from_path(&output_path)), @@ -109,7 +109,7 @@ pub fn run( // having a final status message is important especially in an accessibility context // as screen readers may not read a commands exit code, making it hard to reason // about whether the command succeeded without such a message - info!(accessible, "Successfully compressed '{}'.", to_utf(&output_path)); + info!(accessible, "Successfully compressed '{}'.", path_to_str(&output_path)); } else { // If Ok(false) or Err() occurred, delete incomplete file at `output_path` // @@ -137,7 +137,7 @@ pub fn run( let mut formats = vec![]; if let Some(format) = args.format { - let format = parse_format(&format)?; + let format = parse_format_flag(&format)?; for path in files.iter() { let file_name = path.file_name().ok_or_else(|| Error::NotFound { error_title: format!("{} does not have a file name", EscapedPathDisplay::new(path)), @@ -189,7 +189,7 @@ pub fn run( let mut formats = vec![]; if let Some(format) = args.format { - let format = parse_format(&format)?; + let format = parse_format_flag(&format)?; for _ in 0..files.len() { formats.push(format.clone()); } diff --git a/src/error.rs b/src/error.rs index a54d0a2cc..1cc580243 100644 --- a/src/error.rs +++ b/src/error.rs @@ -4,15 +4,21 @@ use std::{ borrow::Cow, + ffi::OsString, fmt::{self, Display}, + io, }; -use crate::{accessible::is_running_in_accessible_mode, utils::colors::*}; +use crate::{ + accessible::is_running_in_accessible_mode, + extension::{PRETTY_SUPPORTED_ALIASES, PRETTY_SUPPORTED_EXTENSIONS}, + utils::os_str_to_str, +}; /// All errors that can be generated by `ouch` -#[derive(Debug)] +#[derive(Debug, Clone)] pub enum Error { - /// Not every IoError, some of them get filtered by `From` into other variants + /// An IoError that doesn't have a dedicated error variant IoError { reason: String }, /// From lzzzz::lz4f::Error Lz4Error { reason: String }, @@ -33,9 +39,9 @@ pub enum Error { /// Custom and unique errors are reported in this variant Custom { reason: FinalError }, /// Invalid format passed to `--format` - InvalidFormat { reason: String }, + InvalidFormatFlag { text: OsString, reason: String }, /// From sevenz_rust::Error - SevenzipError(sevenz_rust::Error), + SevenzipError { reason: String }, /// Recognised but unsupported format // currently only RAR when built without the `unrar` feature UnsupportedFormat { reason: String }, @@ -60,6 +66,8 @@ pub struct FinalError { impl Display for FinalError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + use crate::utils::colors::*; + // Title // // When in ACCESSIBLE mode, the square brackets are suppressed @@ -120,55 +128,71 @@ impl FinalError { self.hints.push(hint.into()); self } + + /// Adds all supported formats as hints. + /// + /// This is what it looks like: + /// ``` + /// hint: Supported extensions are: tar, zip, bz, bz2, gz, lz4, xz, lzma, sz, zst + /// hint: Supported aliases are: tgz, tbz, tlz4, txz, tzlma, tsz, tzst + /// ``` + pub fn hint_all_supported_formats(self) -> Self { + self.hint(format!("Supported extensions are: {}", PRETTY_SUPPORTED_EXTENSIONS)) + .hint(format!("Supported aliases are: {}", PRETTY_SUPPORTED_ALIASES)) + } } -impl fmt::Display for Error { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - let err = match self { - Error::WalkdirError { reason } => FinalError::with_title(reason.to_string()), - Error::NotFound { error_title } => FinalError::with_title(error_title.to_string()).detail("File not found"), +impl From for FinalError { + fn from(err: Error) -> Self { + match err { + Error::WalkdirError { reason } => FinalError::with_title(reason), + Error::NotFound { error_title } => FinalError::with_title(error_title).detail("File not found"), Error::CompressingRootFolder => { FinalError::with_title("It seems you're trying to compress the root folder.") .detail("This is unadvisable since ouch does compressions in-memory.") .hint("Use a more appropriate tool for this, such as rsync.") } - Error::IoError { reason } => FinalError::with_title(reason.to_string()), - Error::Lz4Error { reason } => FinalError::with_title(reason.to_string()), - Error::AlreadyExists { error_title } => { - FinalError::with_title(error_title.to_string()).detail("File already exists") - } - Error::InvalidZipArchive(reason) => FinalError::with_title("Invalid zip archive").detail(*reason), - Error::PermissionDenied { error_title } => { - FinalError::with_title(error_title.to_string()).detail("Permission denied") + Error::IoError { reason } => FinalError::with_title(reason), + Error::Lz4Error { reason } => FinalError::with_title(reason), + Error::AlreadyExists { error_title } => FinalError::with_title(error_title).detail("File already exists"), + Error::InvalidZipArchive(reason) => FinalError::with_title("Invalid zip archive").detail(reason), + Error::PermissionDenied { error_title } => FinalError::with_title(error_title).detail("Permission denied"), + Error::UnsupportedZipArchive(reason) => FinalError::with_title("Unsupported zip archive").detail(reason), + Error::InvalidFormatFlag { reason, text } => { + FinalError::with_title(format!("Failed to parse `--format {}`", os_str_to_str(&text))) + .detail(reason) + .hint_all_supported_formats() + .hint("") + .hint("Examples:") + .hint(" --format tar") + .hint(" --format gz") + .hint(" --format tar.gz") } - Error::UnsupportedZipArchive(reason) => FinalError::with_title("Unsupported zip archive").detail(*reason), - Error::InvalidFormat { reason } => FinalError::with_title("Invalid archive format").detail(reason.clone()), Error::Custom { reason } => reason.clone(), - Error::SevenzipError(reason) => FinalError::with_title("7z error").detail(reason.to_string()), + Error::SevenzipError { reason } => FinalError::with_title("7z error").detail(reason), Error::UnsupportedFormat { reason } => { FinalError::with_title("Recognised but unsupported format").detail(reason.clone()) } - }; + } + } +} +impl fmt::Display for Error { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let err = FinalError::from(self.clone()); write!(f, "{err}") } } impl From for Error { fn from(err: std::io::Error) -> Self { + let error_title = err.to_string(); + match err.kind() { - std::io::ErrorKind::NotFound => Self::NotFound { - error_title: err.to_string(), - }, - std::io::ErrorKind::PermissionDenied => Self::PermissionDenied { - error_title: err.to_string(), - }, - std::io::ErrorKind::AlreadyExists => Self::AlreadyExists { - error_title: err.to_string(), - }, - _other => Self::IoError { - reason: err.to_string(), - }, + io::ErrorKind::NotFound => Self::NotFound { error_title }, + io::ErrorKind::PermissionDenied => Self::PermissionDenied { error_title }, + io::ErrorKind::AlreadyExists => Self::AlreadyExists { error_title }, + _other => Self::IoError { reason: error_title }, } } } @@ -198,7 +222,9 @@ impl From for Error { impl From for Error { fn from(err: sevenz_rust::Error) -> Self { - Self::SevenzipError(err) + Self::SevenzipError { + reason: err.to_string(), + } } } diff --git a/src/extension.rs b/src/extension.rs index 0f7170b81..bbcc4d95b 100644 --- a/src/extension.rs +++ b/src/extension.rs @@ -3,8 +3,8 @@ use std::{ffi::OsStr, fmt, path::Path}; use bstr::ByteSlice; +use CompressionFormat::*; -use self::CompressionFormat::*; use crate::{error::Error, warning}; pub const SUPPORTED_EXTENSIONS: &[&str] = &[ @@ -34,7 +34,10 @@ pub const PRETTY_SUPPORTED_ALIASES: &str = "tgz, tbz, tlz4, txz, tzlma, tsz, tzs /// A wrapper around `CompressionFormat` that allows combinations like `tgz` #[derive(Debug, Clone)] +// Keep `PartialEq` only for testing because two formats are the same even if +// their `display_text` does not match (beware of aliases) #[cfg_attr(test, derive(PartialEq))] +// Should only be built with constructors #[non_exhaustive] pub struct Extension { /// One extension like "tgz" can be made of multiple CompressionFormats ([Tar, Gz]) @@ -144,17 +147,30 @@ fn split_extension(name: &mut &[u8]) -> Option { Some(ext) } -pub fn parse_format(fmt: &OsStr) -> crate::Result> { - let fmt = <[u8] as ByteSlice>::from_os_str(fmt).ok_or_else(|| Error::InvalidFormat { - reason: "Invalid UTF-8".into(), +pub fn parse_format_flag(input: &OsStr) -> crate::Result> { + let format = input.as_encoded_bytes(); + + let format = std::str::from_utf8(format).map_err(|_| Error::InvalidFormatFlag { + text: input.to_owned(), + reason: "Invalid UTF-8.".to_string(), })?; - let mut extensions = Vec::new(); - for extension in fmt.split_str(b".") { - let extension = to_extension(extension).ok_or_else(|| Error::InvalidFormat { - reason: format!("Unsupported extension: {}", extension.to_str_lossy()), - })?; - extensions.push(extension); + let extensions: Vec = format + .split('.') + .filter(|extension| !extension.is_empty()) + .map(|extension| { + to_extension(extension.as_bytes()).ok_or_else(|| Error::InvalidFormatFlag { + text: input.to_owned(), + reason: format!("Unsupported extension '{}'", extension), + }) + }) + .collect::>()?; + + if extensions.is_empty() { + return Err(Error::InvalidFormatFlag { + text: input.to_owned(), + reason: "Parsing got an empty list of extensions.".to_string(), + }); } Ok(extensions) @@ -242,8 +258,6 @@ pub fn build_archive_file_suggestion(path: &Path, suggested_extension: &str) -> #[cfg(test)] mod tests { - use std::path::Path; - use super::*; #[test] @@ -257,6 +271,7 @@ mod tests { } #[test] + /// Test extension parsing for input/output files fn test_separate_known_extensions_from_name() { assert_eq!( separate_known_extensions_from_name("file".as_ref()), @@ -287,6 +302,37 @@ mod tests { ); } + #[test] + /// Test extension parsing of `--format FORMAT` + fn test_parse_of_format_flag() { + assert_eq!( + parse_format_flag(OsStr::new("tar")).unwrap(), + vec![Extension::new(&[Tar], "tar")] + ); + assert_eq!( + parse_format_flag(OsStr::new(".tar")).unwrap(), + vec![Extension::new(&[Tar], "tar")] + ); + assert_eq!( + parse_format_flag(OsStr::new("tar.gz")).unwrap(), + vec![Extension::new(&[Tar], "tar"), Extension::new(&[Gzip], "gz")] + ); + assert_eq!( + parse_format_flag(OsStr::new(".tar.gz")).unwrap(), + vec![Extension::new(&[Tar], "tar"), Extension::new(&[Gzip], "gz")] + ); + assert_eq!( + parse_format_flag(OsStr::new("..tar..gz.....")).unwrap(), + vec![Extension::new(&[Tar], "tar"), Extension::new(&[Gzip], "gz")] + ); + + assert!(parse_format_flag(OsStr::new("../tar.gz")).is_err()); + assert!(parse_format_flag(OsStr::new("targz")).is_err()); + assert!(parse_format_flag(OsStr::new("tar.gz.unknown")).is_err()); + assert!(parse_format_flag(OsStr::new(".tar.gz.unknown")).is_err()); + assert!(parse_format_flag(OsStr::new(".tar.!@#.gz")).is_err()); + } + #[test] fn builds_suggestion_correctly() { assert_eq!(build_archive_file_suggestion(Path::new("linux.png"), ".tar"), None); diff --git a/src/list.rs b/src/list.rs index 4d4a94524..6b861494c 100644 --- a/src/list.rs +++ b/src/list.rs @@ -78,7 +78,6 @@ mod tree { use std::{ ffi::{OsStr, OsString}, io::Write, - iter::FromIterator, path, }; diff --git a/src/utils/formatting.rs b/src/utils/formatting.rs index fac5c1126..9ebef9698 100644 --- a/src/utils/formatting.rs +++ b/src/utils/formatting.rs @@ -1,4 +1,4 @@ -use std::{borrow::Cow, cmp, fmt::Display, path::Path}; +use std::{borrow::Cow, cmp, ffi::OsStr, fmt::Display, path::Path}; use crate::CURRENT_DIRECTORY; @@ -45,7 +45,11 @@ impl Display for EscapedPathDisplay<'_> { /// This is different from [`Path::display`]. /// /// See for a comparison. -pub fn to_utf(os_str: &Path) -> Cow { +pub fn path_to_str(path: &Path) -> Cow { + os_str_to_str(path.as_ref()) +} + +pub fn os_str_to_str(os_str: &OsStr) -> Cow { let format = || { let text = format!("{os_str:?}"); Cow::Owned(text.trim_matches('"').to_string()) @@ -65,15 +69,15 @@ pub fn strip_cur_dir(source_path: &Path) -> &Path { /// Converts a slice of `AsRef` to comma separated String /// /// Panics if the slice is empty. -pub fn pretty_format_list_of_paths(os_strs: &[impl AsRef]) -> String { - let mut iter = os_strs.iter().map(AsRef::as_ref); +pub fn pretty_format_list_of_paths(paths: &[impl AsRef]) -> String { + let mut iter = paths.iter().map(AsRef::as_ref); - let first_element = iter.next().unwrap(); - let mut string = to_utf(first_element).into_owned(); + let first_path = iter.next().unwrap(); + let mut string = path_to_str(first_path).into_owned(); - for os_str in iter { + for path in iter { string += ", "; - string += &to_utf(os_str); + string += &path_to_str(path); } string } @@ -83,7 +87,7 @@ pub fn nice_directory_display(path: &Path) -> Cow { if path == Path::new(".") { Cow::Borrowed("current directory") } else { - to_utf(path) + path_to_str(path) } } diff --git a/src/utils/mod.rs b/src/utils/mod.rs index b6ea08e50..a27d6295d 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -9,17 +9,19 @@ mod formatting; mod fs; mod question; -pub use file_visibility::FileVisibilityPolicy; -pub use formatting::{ - nice_directory_display, pretty_format_list_of_paths, strip_cur_dir, to_utf, Bytes, EscapedPathDisplay, +pub use self::{ + file_visibility::FileVisibilityPolicy, + formatting::{ + nice_directory_display, os_str_to_str, path_to_str, pretty_format_list_of_paths, strip_cur_dir, Bytes, + EscapedPathDisplay, + }, + fs::{ + cd_into_same_dir_as, clear_path, create_dir_if_non_existent, is_symlink, remove_file_or_dir, + try_infer_extension, + }, + question::{ask_to_create_file, user_wants_to_continue, user_wants_to_overwrite, QuestionAction, QuestionPolicy}, + utf8::{get_invalid_utf8_paths, is_invalid_utf8}, }; -pub use fs::{ - cd_into_same_dir_as, clear_path, create_dir_if_non_existent, is_symlink, remove_file_or_dir, try_infer_extension, -}; -pub use question::{ - ask_to_create_file, user_wants_to_continue, user_wants_to_overwrite, QuestionAction, QuestionPolicy, -}; -pub use utf8::{get_invalid_utf8_paths, is_invalid_utf8}; mod utf8 { use std::{ffi::OsStr, path::PathBuf}; diff --git a/src/utils/question.rs b/src/utils/question.rs index 713931e39..92e599f65 100644 --- a/src/utils/question.rs +++ b/src/utils/question.rs @@ -11,11 +11,10 @@ use std::{ use fs_err as fs; -use super::{strip_cur_dir, to_utf}; use crate::{ accessible::is_running_in_accessible_mode, error::{Error, FinalError, Result}, - utils::{self, colors}, + utils::{self, colors, formatting::path_to_str, strip_cur_dir}, }; #[derive(Debug, PartialEq, Eq, Clone, Copy)] @@ -44,7 +43,7 @@ pub fn user_wants_to_overwrite(path: &Path, question_policy: QuestionPolicy) -> QuestionPolicy::AlwaysYes => Ok(true), QuestionPolicy::AlwaysNo => Ok(false), QuestionPolicy::Ask => { - let path = to_utf(strip_cur_dir(path)); + let path = path_to_str(strip_cur_dir(path)); let path = Some(&*path); let placeholder = Some("FILE"); Confirmation::new("Do you want to overwrite 'FILE'?", placeholder).ask(path) @@ -83,7 +82,7 @@ pub fn user_wants_to_continue( QuestionAction::Compression => "compress", QuestionAction::Decompression => "decompress", }; - let path = to_utf(strip_cur_dir(path)); + let path = path_to_str(strip_cur_dir(path)); let path = Some(&*path); let placeholder = Some("FILE"); Confirmation::new(&format!("Do you want to {action} 'FILE'?"), placeholder).ask(path) diff --git a/tests/snapshots/ui__ui_test_err_format_flag-2.snap b/tests/snapshots/ui__ui_test_err_format_flag-2.snap new file mode 100644 index 000000000..d8a14c962 --- /dev/null +++ b/tests/snapshots/ui__ui_test_err_format_flag-2.snap @@ -0,0 +1,14 @@ +--- +source: tests/ui.rs +expression: "run_ouch(\"ouch compress input output --format targz\", dir)" +--- +[ERROR] Failed to parse `--format targz` + - Unsupported extension 'targz' + +hint: Supported extensions are: tar, zip, bz, bz2, gz, lz4, xz, lzma, sz, zst, rar, 7z +hint: Supported aliases are: tgz, tbz, tlz4, txz, tzlma, tsz, tzst +hint: +hint: Examples: +hint: --format tar +hint: --format gz +hint: --format tar.gz diff --git a/tests/snapshots/ui__ui_test_err_format_flag-3.snap b/tests/snapshots/ui__ui_test_err_format_flag-3.snap new file mode 100644 index 000000000..940f93a92 --- /dev/null +++ b/tests/snapshots/ui__ui_test_err_format_flag-3.snap @@ -0,0 +1,15 @@ +--- +source: tests/ui.rs +expression: "run_ouch(\"ouch compress input output --format .tar.$#!@.rest\", dir)" +--- +[ERROR] Failed to parse `--format .tar.$#!@.rest` + - Unsupported extension '$#!@' + +hint: Supported extensions are: tar, zip, bz, bz2, gz, lz4, xz, lzma, sz, zst +hint: Supported aliases are: tgz, tbz, tlz4, txz, tzlma, tsz, tzst +hint: +hint: Examples: +hint: --format tar +hint: --format gz +hint: --format tar.gz + diff --git a/tests/snapshots/ui__ui_test_err_format_flag.snap b/tests/snapshots/ui__ui_test_err_format_flag.snap new file mode 100644 index 000000000..ca5e05cdc --- /dev/null +++ b/tests/snapshots/ui__ui_test_err_format_flag.snap @@ -0,0 +1,14 @@ +--- +source: tests/ui.rs +expression: "run_ouch(\"ouch compress input output --format tar.gz.unknown\", dir)" +--- +[ERROR] Failed to parse `--format tar.gz.unknown` + - Unsupported extension 'unknown' + +hint: Supported extensions are: tar, zip, bz, bz2, gz, lz4, xz, lzma, sz, zst, rar, 7z +hint: Supported aliases are: tgz, tbz, tlz4, txz, tzlma, tsz, tzst +hint: +hint: Examples: +hint: --format tar +hint: --format gz +hint: --format tar.gz diff --git a/tests/snapshots/ui__ui_test_ok_format_flag-2.snap b/tests/snapshots/ui__ui_test_ok_format_flag-2.snap new file mode 100644 index 000000000..ac57ca5eb --- /dev/null +++ b/tests/snapshots/ui__ui_test_ok_format_flag-2.snap @@ -0,0 +1,7 @@ +--- +source: tests/ui.rs +expression: "run_ouch(\"ouch compress input output2 --format .tar.gz\", dir)" +--- +[INFO] Compressing 'input'. +[INFO] Successfully compressed 'output2'. + diff --git a/tests/snapshots/ui__ui_test_ok_format_flag.snap b/tests/snapshots/ui__ui_test_ok_format_flag.snap new file mode 100644 index 000000000..f4f38c59d --- /dev/null +++ b/tests/snapshots/ui__ui_test_ok_format_flag.snap @@ -0,0 +1,7 @@ +--- +source: tests/ui.rs +expression: "run_ouch(\"ouch compress input output1 --format tar.gz\", dir)" +--- +[INFO] Compressing 'input'. +[INFO] Successfully compressed 'output1'. + diff --git a/tests/ui.rs b/tests/ui.rs index 980078089..51d4dd310 100644 --- a/tests/ui.rs +++ b/tests/ui.rs @@ -8,7 +8,7 @@ mod utils; use std::{ffi::OsStr, io, path::Path, process::Output}; -use insta::assert_display_snapshot as ui; +use insta::assert_snapshot as ui; use regex::Regex; use crate::utils::create_files_in; @@ -87,6 +87,29 @@ fn ui_test_err_missing_files() { ui!(run_ouch("ouch list a b", dir)); } +#[test] +fn ui_test_err_format_flag() { + let (_dropper, dir) = testdir().unwrap(); + + // prepare + create_files_in(dir, &["input"]); + + ui!(run_ouch("ouch compress input output --format tar.gz.unknown", dir)); + ui!(run_ouch("ouch compress input output --format targz", dir)); + ui!(run_ouch("ouch compress input output --format .tar.$#!@.rest", dir)); +} + +#[test] +fn ui_test_ok_format_flag() { + let (_dropper, dir) = testdir().unwrap(); + + // prepare + create_files_in(dir, &["input"]); + + ui!(run_ouch("ouch compress input output1 --format tar.gz", dir)); + ui!(run_ouch("ouch compress input output2 --format .tar.gz", dir)); +} + #[test] fn ui_test_ok_compress() { let (_dropper, dir) = testdir().unwrap(); diff --git a/tests/utils.rs b/tests/utils.rs index bd5fc346a..0cda620b2 100644 --- a/tests/utils.rs +++ b/tests/utils.rs @@ -52,6 +52,7 @@ pub fn create_files_in(dir: &Path, files: &[&str]) { /// Write random content to a file pub fn write_random_content(file: &mut impl Write, rng: &mut impl RngCore) { let mut data = vec![0; rng.gen_range(0..4096)]; + rng.fill_bytes(&mut data); file.write_all(&data).unwrap(); } From 3d9b205f46bf4ba57ffcfdd3e79981eec3e58135 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20M=2E=20Bezerra?= Date: Mon, 18 Sep 2023 01:07:20 -0300 Subject: [PATCH 3/3] Use Path::display() directly where applicable --- src/archive/sevenz.rs | 4 +- src/archive/tar.rs | 4 +- src/archive/zip.rs | 4 +- src/check.rs | 42 ++++++++--------- src/commands/mod.rs | 9 ++-- src/extension.rs | 4 +- src/list.rs | 10 ++--- src/utils/formatting.rs | 45 ++----------------- src/utils/fs.rs | 4 +- src/utils/mod.rs | 1 - src/utils/question.rs | 2 +- .../ui__ui_test_err_format_flag-3.snap | 3 +- 12 files changed, 40 insertions(+), 92 deletions(-) diff --git a/src/archive/sevenz.rs b/src/archive/sevenz.rs index 0847f4ca5..b76b0e267 100644 --- a/src/archive/sevenz.rs +++ b/src/archive/sevenz.rs @@ -12,7 +12,7 @@ use same_file::Handle; use crate::{ error::FinalError, info, - utils::{self, cd_into_same_dir_as, Bytes, EscapedPathDisplay, FileVisibilityPolicy}, + utils::{self, cd_into_same_dir_as, Bytes, FileVisibilityPolicy}, warning, }; @@ -56,7 +56,7 @@ where // spoken text for users using screen readers, braille displays // and so on if !quiet { - info!(inaccessible, "Compressing '{}'.", EscapedPathDisplay::new(path)); + info!(inaccessible, "Compressing '{}'.", path.display()); } let metadata = match path.metadata() { diff --git a/src/archive/tar.rs b/src/archive/tar.rs index 822212283..b4545fc86 100644 --- a/src/archive/tar.rs +++ b/src/archive/tar.rs @@ -15,7 +15,7 @@ use crate::{ error::FinalError, info, list::FileInArchive, - utils::{self, Bytes, EscapedPathDisplay, FileVisibilityPolicy}, + utils::{self, Bytes, FileVisibilityPolicy}, warning, }; @@ -120,7 +120,7 @@ where // spoken text for users using screen readers, braille displays // and so on if !quiet { - info!(inaccessible, "Compressing '{}'.", EscapedPathDisplay::new(path)); + info!(inaccessible, "Compressing '{}'.", path.display()); } if path.is_dir() { diff --git a/src/archive/zip.rs b/src/archive/zip.rs index e78adff02..c42a870c0 100644 --- a/src/archive/zip.rs +++ b/src/archive/zip.rs @@ -22,7 +22,7 @@ use crate::{ list::FileInArchive, utils::{ self, cd_into_same_dir_as, get_invalid_utf8_paths, pretty_format_list_of_paths, strip_cur_dir, Bytes, - EscapedPathDisplay, FileVisibilityPolicy, + FileVisibilityPolicy, }, warning, }; @@ -191,7 +191,7 @@ where // spoken text for users using screen readers, braille displays // and so on if !quiet { - info!(inaccessible, "Compressing '{}'.", EscapedPathDisplay::new(path)); + info!(inaccessible, "Compressing '{}'.", path.display()); } let metadata = match path.metadata() { diff --git a/src/check.rs b/src/check.rs index afb3c3b01..74ffe957c 100644 --- a/src/check.rs +++ b/src/check.rs @@ -12,7 +12,7 @@ use crate::{ error::FinalError, extension::{build_archive_file_suggestion, Extension}, info, - utils::{pretty_format_list_of_paths, try_infer_extension, user_wants_to_continue, EscapedPathDisplay}, + utils::{pretty_format_list_of_paths, try_infer_extension, user_wants_to_continue}, warning, QuestionAction, QuestionPolicy, Result, }; @@ -103,22 +103,19 @@ pub fn check_for_non_archive_formats(files: &[PathBuf], formats: &[Vec Result<()> { if let Some(format) = formats.iter().skip(1).find(|format| format.is_archive()) { - let error = FinalError::with_title(format!( - "Cannot compress to '{}'.", - EscapedPathDisplay::new(output_path) - )) - .detail(format!("Found the format '{format}' in an incorrect position.")) - .detail(format!( - "'{format}' can only be used at the start of the file extension." - )) - .hint(format!( - "If you wish to compress multiple files, start the extension with '{format}'." - )) - .hint(format!( - "Otherwise, remove the last '{}' from '{}'.", - format, - EscapedPathDisplay::new(output_path) - )); + let error = FinalError::with_title(format!("Cannot compress to '{}'.", output_path.display())) + .detail(format!("Found the format '{format}' in an incorrect position.")) + .detail(format!( + "'{format}' can only be used at the start of the file extension." + )) + .hint(format!( + "If you wish to compress multiple files, start the extension with '{format}'." + )) + .hint(format!( + "Otherwise, remove the last '{}' from '{}'.", + format, + output_path.display() + )); return Err(error.into()); } @@ -167,10 +164,7 @@ pub fn check_missing_formats_when_decompressing(files: &[PathBuf], formats: &[Ve error = error .hint("") .hint("Alternatively, you can pass an extension to the '--format' flag:") - .hint(format!( - " ouch decompress {} --format tar.gz", - EscapedPathDisplay::new(path), - )); + .hint(format!(" ouch decompress {} --format tar.gz", path.display(),)); } Err(error.into()) @@ -179,7 +173,7 @@ pub fn check_missing_formats_when_decompressing(files: &[PathBuf], formats: &[Ve /// Check if there is a first format when compressing, and returns it. pub fn check_first_format_when_compressing<'a>(formats: &'a [Extension], output_path: &Path) -> Result<&'a Extension> { formats.first().ok_or_else(|| { - let output_path = EscapedPathDisplay::new(output_path); + let output_path = output_path.display(); FinalError::with_title(format!("Cannot compress to '{output_path}'.")) .detail("You shall supply the compression format") .hint("Try adding supported extensions (see --help):") @@ -233,11 +227,11 @@ pub fn check_invalid_compression_with_non_archive_format( .expect("output path should contain a compression format"); ( - format!("From: {}", EscapedPathDisplay::new(output_path)), + format!("From: {}", output_path.display()), format!("To: {suggested_output_path}"), ) }; - let output_path = EscapedPathDisplay::new(output_path); + let output_path = output_path.display(); let error = FinalError::with_title(format!("Cannot compress to '{output_path}'.")) .detail(first_detail_message) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 0d85a1576..3cb493845 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -17,7 +17,7 @@ use crate::{ extension::{self, parse_format_flag}, info, list::ListOptions, - utils::{self, path_to_str, EscapedPathDisplay, FileVisibilityPolicy}, + utils::{self, path_to_str, FileVisibilityPolicy}, warning, CliArgs, QuestionPolicy, }; @@ -117,10 +117,7 @@ pub fn run( // out that we left a possibly CORRUPTED file at `output_path` if utils::remove_file_or_dir(&output_path).is_err() { eprintln!("{red}FATAL ERROR:\n", red = *colors::RED); - eprintln!( - " Ouch failed to delete the file '{}'.", - EscapedPathDisplay::new(&output_path) - ); + eprintln!(" Ouch failed to delete the file '{}'.", &output_path.display()); eprintln!(" Please delete it manually."); eprintln!(" This file is corrupted if compression didn't finished."); @@ -140,7 +137,7 @@ pub fn run( let format = parse_format_flag(&format)?; for path in files.iter() { let file_name = path.file_name().ok_or_else(|| Error::NotFound { - error_title: format!("{} does not have a file name", EscapedPathDisplay::new(path)), + error_title: format!("{} does not have a file name", path.display()), })?; output_paths.push(file_name.as_ref()); formats.push(format.clone()); diff --git a/src/extension.rs b/src/extension.rs index bbcc4d95b..8d2f185d4 100644 --- a/src/extension.rs +++ b/src/extension.rs @@ -148,9 +148,7 @@ fn split_extension(name: &mut &[u8]) -> Option { } pub fn parse_format_flag(input: &OsStr) -> crate::Result> { - let format = input.as_encoded_bytes(); - - let format = std::str::from_utf8(format).map_err(|_| Error::InvalidFormatFlag { + let format = input.to_str().ok_or_else(|| Error::InvalidFormatFlag { text: input.to_owned(), reason: "Invalid UTF-8.".to_string(), })?; diff --git a/src/list.rs b/src/list.rs index 6b861494c..b3b350873 100644 --- a/src/list.rs +++ b/src/list.rs @@ -6,7 +6,7 @@ use std::{ }; use self::tree::Tree; -use crate::{accessible::is_running_in_accessible_mode, utils::EscapedPathDisplay}; +use crate::accessible::is_running_in_accessible_mode; /// Options controlling how archive contents should be listed #[derive(Debug, Clone, Copy)] @@ -33,7 +33,7 @@ pub fn list_files( list_options: ListOptions, ) -> crate::Result<()> { let out = &mut stdout().lock(); - let _ = writeln!(out, "Archive: {}", EscapedPathDisplay::new(archive)); + let _ = writeln!(out, "Archive: {}", archive.display()); if list_options.tree { let tree = files.into_iter().collect::>()?; @@ -41,7 +41,7 @@ pub fn list_files( } else { for file in files { let FileInArchive { path, is_dir } = file?; - print_entry(out, EscapedPathDisplay::new(&path), is_dir); + print_entry(out, &path.display(), is_dir); } } Ok(()) @@ -85,7 +85,7 @@ mod tree { use linked_hash_map::LinkedHashMap; use super::FileInArchive; - use crate::{utils::EscapedPathDisplay, warning}; + use crate::warning; /// Directory tree #[derive(Debug, Default)] @@ -121,7 +121,7 @@ mod tree { Some(file) => { warning!( "multiple files with the same name in a single directory ({})", - EscapedPathDisplay::new(&file.path), + &file.path.display(), ); } } diff --git a/src/utils/formatting.rs b/src/utils/formatting.rs index 9ebef9698..06d69c643 100644 --- a/src/utils/formatting.rs +++ b/src/utils/formatting.rs @@ -1,50 +1,11 @@ -use std::{borrow::Cow, cmp, ffi::OsStr, fmt::Display, path::Path}; +use std::{borrow::Cow, cmp, ffi::OsStr, path::Path}; use crate::CURRENT_DIRECTORY; -/// Converts invalid UTF-8 bytes to the Unicode replacement codepoint (�) in its Display implementation. -pub struct EscapedPathDisplay<'a> { - path: &'a Path, -} - -impl<'a> EscapedPathDisplay<'a> { - pub fn new(path: &'a Path) -> Self { - Self { path } - } -} - -#[cfg(unix)] -impl Display for EscapedPathDisplay<'_> { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - use std::os::unix::prelude::OsStrExt; - - let bstr = bstr::BStr::new(self.path.as_os_str().as_bytes()); - - write!(f, "{bstr}") - } -} - -#[cfg(windows)] -impl Display for EscapedPathDisplay<'_> { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - use std::{char, fmt::Write, os::windows::prelude::OsStrExt}; - - let utf16 = self.path.as_os_str().encode_wide(); - let chars = char::decode_utf16(utf16).map(|decoded| decoded.unwrap_or(char::REPLACEMENT_CHARACTER)); - - for char in chars { - f.write_char(char)?; - } - - Ok(()) - } -} - /// Converts an OsStr to utf8 with custom formatting. /// -/// This is different from [`Path::display`]. -/// -/// See for a comparison. +/// This is different from [`Path::display`], see +/// for a comparison. pub fn path_to_str(path: &Path) -> Cow { os_str_to_str(path.as_ref()) } diff --git a/src/utils/fs.rs b/src/utils/fs.rs index dbd422e0e..506560111 100644 --- a/src/utils/fs.rs +++ b/src/utils/fs.rs @@ -9,7 +9,7 @@ use std::{ use fs_err as fs; use super::user_wants_to_overwrite; -use crate::{extension::Extension, info, utils::EscapedPathDisplay, QuestionPolicy}; +use crate::{extension::Extension, info, QuestionPolicy}; /// Remove `path` asking the user to overwrite if necessary. /// @@ -41,7 +41,7 @@ pub fn create_dir_if_non_existent(path: &Path) -> crate::Result<()> { fs::create_dir_all(path)?; // creating a directory is an important change to the file system we // should always inform the user about - info!(accessible, "directory {} created.", EscapedPathDisplay::new(path)); + info!(accessible, "directory {} created.", path.display()); } Ok(()) } diff --git a/src/utils/mod.rs b/src/utils/mod.rs index a27d6295d..398a70a02 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -13,7 +13,6 @@ pub use self::{ file_visibility::FileVisibilityPolicy, formatting::{ nice_directory_display, os_str_to_str, path_to_str, pretty_format_list_of_paths, strip_cur_dir, Bytes, - EscapedPathDisplay, }, fs::{ cd_into_same_dir_as, clear_path, create_dir_if_non_existent, is_symlink, remove_file_or_dir, diff --git a/src/utils/question.rs b/src/utils/question.rs index 92e599f65..29d0af273 100644 --- a/src/utils/question.rs +++ b/src/utils/question.rs @@ -14,7 +14,7 @@ use fs_err as fs; use crate::{ accessible::is_running_in_accessible_mode, error::{Error, FinalError, Result}, - utils::{self, colors, formatting::path_to_str, strip_cur_dir}, + utils::{self, colors, path_to_str, strip_cur_dir}, }; #[derive(Debug, PartialEq, Eq, Clone, Copy)] diff --git a/tests/snapshots/ui__ui_test_err_format_flag-3.snap b/tests/snapshots/ui__ui_test_err_format_flag-3.snap index 940f93a92..a1c8aabd0 100644 --- a/tests/snapshots/ui__ui_test_err_format_flag-3.snap +++ b/tests/snapshots/ui__ui_test_err_format_flag-3.snap @@ -5,11 +5,10 @@ expression: "run_ouch(\"ouch compress input output --format .tar.$#!@.rest\", di [ERROR] Failed to parse `--format .tar.$#!@.rest` - Unsupported extension '$#!@' -hint: Supported extensions are: tar, zip, bz, bz2, gz, lz4, xz, lzma, sz, zst +hint: Supported extensions are: tar, zip, bz, bz2, gz, lz4, xz, lzma, sz, zst, rar, 7z hint: Supported aliases are: tgz, tbz, tlz4, txz, tzlma, tsz, tzst hint: hint: Examples: hint: --format tar hint: --format gz hint: --format tar.gz -