diff --git a/src/uu/sort/src/check.rs b/src/uu/sort/src/check.rs index f1cd22686..44c71674e 100644 --- a/src/uu/sort/src/check.rs +++ b/src/uu/sort/src/check.rs @@ -56,7 +56,7 @@ pub fn check(path: &str, settings: &GlobalSettings) -> i32 { ) == Ordering::Greater { if !settings.check_silent { - println!("sort: {}:{}: disorder: {}", path, line_idx, new_first.line); + eprintln!("sort: {}:{}: disorder: {}", path, line_idx, new_first.line); } return 1; } @@ -68,7 +68,7 @@ pub fn check(path: &str, settings: &GlobalSettings) -> i32 { if compare_by(a, b, settings, chunk.line_data(), chunk.line_data()) == Ordering::Greater { if !settings.check_silent { - println!("sort: {}:{}: disorder: {}", path, line_idx, b.line); + eprintln!("sort: {}:{}: disorder: {}", path, line_idx, b.line); } return 1; } diff --git a/src/uu/sort/src/chunks.rs b/src/uu/sort/src/chunks.rs index 9e9d212c2..5ab98392d 100644 --- a/src/uu/sort/src/chunks.rs +++ b/src/uu/sort/src/chunks.rs @@ -175,7 +175,7 @@ pub fn read( // because it was only temporarily transmuted to a Vec> to make recycling possible. std::mem::transmute::>, Vec>>(lines) }; - let read = crash_if_err!(1, std::str::from_utf8(&buffer[..read])); + let read = crash_if_err!(2, std::str::from_utf8(&buffer[..read])); let mut line_data = LineData { selections, num_infos, @@ -313,7 +313,7 @@ fn read_to_buffer( Err(e) if e.kind() == ErrorKind::Interrupted => { // retry } - Err(e) => crash!(1, "{}", e), + Err(e) => crash!(2, "{}", e), } } } diff --git a/src/uu/sort/src/ext_sort.rs b/src/uu/sort/src/ext_sort.rs index e0814b7a2..ea53900e2 100644 --- a/src/uu/sort/src/ext_sort.rs +++ b/src/uu/sort/src/ext_sort.rs @@ -28,6 +28,7 @@ use crate::merge::ClosedTmpFile; use crate::merge::WriteableCompressedTmpFile; use crate::merge::WriteablePlainTmpFile; use crate::merge::WriteableTmpFile; +use crate::Output; use crate::{ chunks::{self, Chunk}, compare_by, merge, sort_by, GlobalSettings, @@ -38,7 +39,11 @@ use tempfile::TempDir; const START_BUFFER_SIZE: usize = 8_000; /// Sort files by using auxiliary files for storing intermediate chunks (if needed), and output the result. -pub fn ext_sort(files: &mut impl Iterator>, settings: &GlobalSettings) { +pub fn ext_sort( + files: &mut impl Iterator>, + settings: &GlobalSettings, + output: Output, +) { let (sorted_sender, sorted_receiver) = std::sync::mpsc::sync_channel(1); let (recycled_sender, recycled_receiver) = std::sync::mpsc::sync_channel(1); thread::spawn({ @@ -51,6 +56,7 @@ pub fn ext_sort(files: &mut impl Iterator>, settings settings, sorted_receiver, recycled_sender, + output, ); } else { reader_writer::<_, WriteablePlainTmpFile>( @@ -58,6 +64,7 @@ pub fn ext_sort(files: &mut impl Iterator>, settings settings, sorted_receiver, recycled_sender, + output, ); } } @@ -67,6 +74,7 @@ fn reader_writer>, Tmp: WriteableTmpFile settings: &GlobalSettings, receiver: Receiver, sender: SyncSender, + output: Output, ) { let separator = if settings.zero_terminated { b'\0' @@ -96,7 +104,7 @@ fn reader_writer>, Tmp: WriteableTmpFile settings, Some((tmp_dir, tmp_dir_size)), ); - merger.write_all(settings); + merger.write_all(settings, output); } ReadResult::SortedSingleChunk(chunk) => { if settings.unique { @@ -106,9 +114,10 @@ fn reader_writer>, Tmp: WriteableTmpFile == Ordering::Equal }), settings, + output, ); } else { - print_sorted(chunk.lines().iter(), settings); + print_sorted(chunk.lines().iter(), settings, output); } } ReadResult::SortedTwoChunks([a, b]) => { @@ -128,9 +137,10 @@ fn reader_writer>, Tmp: WriteableTmpFile }) .map(|(line, _)| line), settings, + output, ); } else { - print_sorted(merged_iter.map(|(line, _)| line), settings); + print_sorted(merged_iter.map(|(line, _)| line), settings, output); } } ReadResult::EmptyInput => { @@ -211,7 +221,7 @@ fn read_write_loop( } let tmp_dir = crash_if_err!( - 1, + 2, tempfile::Builder::new() .prefix("uutils_sort") .tempdir_in(tmp_dir_parent) diff --git a/src/uu/sort/src/merge.rs b/src/uu/sort/src/merge.rs index 12d7a9b9b..b8d69fb14 100644 --- a/src/uu/sort/src/merge.rs +++ b/src/uu/sort/src/merge.rs @@ -25,7 +25,7 @@ use tempfile::TempDir; use crate::{ chunks::{self, Chunk, RecycledChunk}, - compare_by, GlobalSettings, + compare_by, GlobalSettings, Output, }; /// Merge pre-sorted `Box`s. @@ -238,8 +238,8 @@ pub struct FileMerger<'a> { impl<'a> FileMerger<'a> { /// Write the merged contents to the output file. - pub fn write_all(&mut self, settings: &GlobalSettings) { - let mut out = settings.out_writer(); + pub fn write_all(&mut self, settings: &GlobalSettings, output: Output) { + let mut out = output.into_write(); self.write_all_to(settings, &mut out); } diff --git a/src/uu/sort/src/sort.rs b/src/uu/sort/src/sort.rs index f779bca38..4362863d5 100644 --- a/src/uu/sort/src/sort.rs +++ b/src/uu/sort/src/sort.rs @@ -37,7 +37,7 @@ use std::env; use std::ffi::OsStr; use std::fs::File; use std::hash::{Hash, Hasher}; -use std::io::{stdin, stdout, BufRead, BufReader, BufWriter, Read, Write}; +use std::io::{stdin, stdout, BufRead, BufReader, Read, Write}; use std::ops::Range; use std::path::Path; use std::path::PathBuf; @@ -146,6 +146,29 @@ impl SortMode { } } +pub struct Output { + file: Option, +} + +impl Output { + fn new(name: Option<&str>) -> Self { + Self { + file: name.map(|name| { + File::create(name).unwrap_or_else(|e| { + crash!(2, "open failed: {}: {}", name, strip_errno(&e.to_string())) + }) + }), + } + } + + fn into_write(self) -> Box { + match self.file { + Some(file) => Box::new(file), + None => Box::new(stdout()), + } + } +} + #[derive(Clone)] pub struct GlobalSettings { mode: SortMode, @@ -156,7 +179,6 @@ pub struct GlobalSettings { ignore_non_printing: bool, merge: bool, reverse: bool, - output_file: Option, stable: bool, unique: bool, check: bool, @@ -209,19 +231,6 @@ impl GlobalSettings { } } - fn out_writer(&self) -> BufWriter> { - match self.output_file { - Some(ref filename) => match File::create(Path::new(&filename)) { - Ok(f) => BufWriter::new(Box::new(f) as Box), - Err(e) => { - show_error!("{0}: {1}", filename, e.to_string()); - panic!("Could not open output file"); - } - }, - None => BufWriter::new(Box::new(stdout()) as Box), - } - } - /// Precompute some data needed for sorting. /// This function **must** be called before starting to sort, and `GlobalSettings` may not be altered /// afterwards. @@ -253,7 +262,6 @@ impl Default for GlobalSettings { ignore_non_printing: false, merge: false, reverse: false, - output_file: None, stable: false, unique: false, check: false, @@ -942,7 +950,9 @@ pub fn uumain(args: impl uucore::Args) -> i32 { let usage = get_usage(); let mut settings: GlobalSettings = Default::default(); - let matches = uu_app().usage(&usage[..]).get_matches_from(args); + let matches = uu_app().usage(&usage[..]).get_matches_from_safe(args); + + let matches = crash_if_err!(2, matches); settings.debug = matches.is_present(options::DEBUG); @@ -1051,7 +1061,6 @@ pub fn uumain(args: impl uucore::Args) -> i32 { settings.ignore_leading_blanks = matches.is_present(options::IGNORE_LEADING_BLANKS); - settings.output_file = matches.value_of(options::OUTPUT).map(String::from); settings.reverse = matches.is_present(options::REVERSE); settings.stable = matches.is_present(options::STABLE); settings.unique = matches.is_present(options::UNIQUE); @@ -1060,7 +1069,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { /* if no file, default to stdin */ files.push("-".to_owned()); } else if settings.check && files.len() != 1 { - crash!(1, "extra operand `{}' not allowed with -c", files[1]) + crash!(2, "extra operand `{}' not allowed with -c", files[1]) } if let Some(arg) = matches.args.get(options::SEPARATOR) { @@ -1070,7 +1079,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { separator = "\0"; } if separator.len() != 1 { - crash!(1, "separator must be exactly one character long"); + crash!(2, "separator must be exactly one character long"); } settings.separator = Some(separator.chars().next().unwrap()) } @@ -1100,9 +1109,19 @@ pub fn uumain(args: impl uucore::Args) -> i32 { ); } + // Verify that we can open all input files. + // It is the correct behavior to close all files afterwards, + // and to reopen them at a later point. This is different from how the output file is handled, + // probably to prevent running out of file descriptors. + for file in &files { + open(file); + } + + let output = Output::new(matches.value_of(options::OUTPUT)); + settings.init_precomputed(); - exec(&files, &settings) + exec(&files, &settings, output) } pub fn uu_app() -> App<'static, 'static> { @@ -1113,73 +1132,57 @@ pub fn uu_app() -> App<'static, 'static> { Arg::with_name(options::modes::SORT) .long(options::modes::SORT) .takes_value(true) - .possible_values( - &[ - "general-numeric", - "human-numeric", - "month", - "numeric", - "version", - "random", - ] - ) - .conflicts_with_all(&options::modes::ALL_SORT_MODES) - ) - .arg( - make_sort_mode_arg( - options::modes::HUMAN_NUMERIC, - "h", - "compare according to human readable sizes, eg 1M > 100k" - ), - ) - .arg( - make_sort_mode_arg( - options::modes::MONTH, - "M", - "compare according to month name abbreviation" - ), - ) - .arg( - make_sort_mode_arg( - options::modes::NUMERIC, - "n", - "compare according to string numerical value" - ), - ) - .arg( - make_sort_mode_arg( - options::modes::GENERAL_NUMERIC, - "g", - "compare according to string general numerical value" - ), - ) - .arg( - make_sort_mode_arg( - options::modes::VERSION, - "V", - "Sort by SemVer version number, eg 1.12.2 > 1.1.2", - ), - ) - .arg( - make_sort_mode_arg( - options::modes::RANDOM, - "R", - "shuffle in random order", - ), + .possible_values(&[ + "general-numeric", + "human-numeric", + "month", + "numeric", + "version", + "random", + ]) + .conflicts_with_all(&options::modes::ALL_SORT_MODES), ) + .arg(make_sort_mode_arg( + options::modes::HUMAN_NUMERIC, + "h", + "compare according to human readable sizes, eg 1M > 100k", + )) + .arg(make_sort_mode_arg( + options::modes::MONTH, + "M", + "compare according to month name abbreviation", + )) + .arg(make_sort_mode_arg( + options::modes::NUMERIC, + "n", + "compare according to string numerical value", + )) + .arg(make_sort_mode_arg( + options::modes::GENERAL_NUMERIC, + "g", + "compare according to string general numerical value", + )) + .arg(make_sort_mode_arg( + options::modes::VERSION, + "V", + "Sort by SemVer version number, eg 1.12.2 > 1.1.2", + )) + .arg(make_sort_mode_arg( + options::modes::RANDOM, + "R", + "shuffle in random order", + )) .arg( Arg::with_name(options::DICTIONARY_ORDER) .short("d") .long(options::DICTIONARY_ORDER) .help("consider only blanks and alphanumeric characters") - .conflicts_with_all( - &[ - options::modes::NUMERIC, - options::modes::GENERAL_NUMERIC, - options::modes::HUMAN_NUMERIC, - options::modes::MONTH, - ] - ), + .conflicts_with_all(&[ + options::modes::NUMERIC, + options::modes::GENERAL_NUMERIC, + options::modes::HUMAN_NUMERIC, + options::modes::MONTH, + ]), ) .arg( Arg::with_name(options::MERGE) @@ -1207,7 +1210,10 @@ pub fn uu_app() -> App<'static, 'static> { .short("C") .long(options::check::CHECK_SILENT) .conflicts_with(options::OUTPUT) - .help("exit successfully if the given file is already sorted, and exit with status 1 otherwise."), + .help( + "exit successfully if the given file is already sorted,\ + and exit with status 1 otherwise.", + ), ) .arg( Arg::with_name(options::IGNORE_CASE) @@ -1220,14 +1226,12 @@ pub fn uu_app() -> App<'static, 'static> { .short("i") .long(options::IGNORE_NONPRINTING) .help("ignore nonprinting characters") - .conflicts_with_all( - &[ - options::modes::NUMERIC, - options::modes::GENERAL_NUMERIC, - options::modes::HUMAN_NUMERIC, - options::modes::MONTH - ] - ), + .conflicts_with_all(&[ + options::modes::NUMERIC, + options::modes::GENERAL_NUMERIC, + options::modes::HUMAN_NUMERIC, + options::modes::MONTH, + ]), ) .arg( Arg::with_name(options::IGNORE_LEADING_BLANKS) @@ -1276,7 +1280,8 @@ pub fn uu_app() -> App<'static, 'static> { .short("t") .long(options::SEPARATOR) .help("custom separator for -k") - .takes_value(true)) + .takes_value(true), + ) .arg( Arg::with_name(options::ZERO_TERMINATED) .short("z") @@ -1311,13 +1316,13 @@ pub fn uu_app() -> App<'static, 'static> { .long(options::COMPRESS_PROG) .help("compress temporary files with PROG, decompress with PROG -d") .long_help("PROG has to take input from stdin and output to stdout") - .value_name("PROG") + .value_name("PROG"), ) .arg( Arg::with_name(options::BATCH_SIZE) .long(options::BATCH_SIZE) .help("Merge at most N_MERGE inputs at once.") - .value_name("N_MERGE") + .value_name("N_MERGE"), ) .arg( Arg::with_name(options::FILES0_FROM) @@ -1332,22 +1337,26 @@ pub fn uu_app() -> App<'static, 'static> { .long(options::DEBUG) .help("underline the parts of the line that are actually used for sorting"), ) - .arg(Arg::with_name(options::FILES).multiple(true).takes_value(true)) + .arg( + Arg::with_name(options::FILES) + .multiple(true) + .takes_value(true), + ) } -fn exec(files: &[String], settings: &GlobalSettings) -> i32 { +fn exec(files: &[String], settings: &GlobalSettings, output: Output) -> i32 { if settings.merge { let mut file_merger = merge::merge(files.iter().map(open), settings); - file_merger.write_all(settings); + file_merger.write_all(settings, output); } else if settings.check { if files.len() > 1 { - crash!(1, "only one file allowed with -c"); + crash!(2, "only one file allowed with -c"); } return check::check(files.first().unwrap(), settings); } else { let mut lines = files.iter().map(open); - ext_sort(&mut lines, settings); + ext_sort(&mut lines, settings, output); } 0 } @@ -1619,14 +1628,22 @@ fn month_compare(a: &str, b: &str) -> Ordering { } } -fn print_sorted<'a, T: Iterator>>(iter: T, settings: &GlobalSettings) { - let mut writer = settings.out_writer(); +fn print_sorted<'a, T: Iterator>>( + iter: T, + settings: &GlobalSettings, + output: Output, +) { + let mut writer = output.into_write(); for line in iter { line.print(&mut writer, settings); } } -// from cat.rs +/// Strips the trailing " (os error XX)" from io error strings. +fn strip_errno(err: &str) -> &str { + &err[..err.find(" (os error ").unwrap_or(err.len())] +} + fn open(path: impl AsRef) -> Box { let path = path.as_ref(); if path == "-" { @@ -1634,10 +1651,17 @@ fn open(path: impl AsRef) -> Box { return Box::new(stdin) as Box; } - match File::open(Path::new(path)) { + let path = Path::new(path); + + match File::open(path) { Ok(f) => Box::new(f) as Box, Err(e) => { - crash!(2, "cannot read: {0:?}: {1}", path, e); + crash!( + 2, + "cannot read: {0}: {1}", + path.to_string_lossy(), + strip_errno(&e.to_string()) + ); } } } diff --git a/tests/by-util/test_sort.rs b/tests/by-util/test_sort.rs index a9519e153..5f44ce35f 100644 --- a/tests/by-util/test_sort.rs +++ b/tests/by-util/test_sort.rs @@ -181,7 +181,7 @@ fn test_check_zero_terminated_failure() { .arg("-c") .arg("zero-terminated.txt") .fails() - .stdout_is("sort: zero-terminated.txt:2: disorder: ../../fixtures/du\n"); + .stderr_only("sort: zero-terminated.txt:2: disorder: ../../fixtures/du\n"); } #[test] @@ -775,13 +775,13 @@ fn test_check() { .arg(diagnose_arg) .arg("check_fail.txt") .fails() - .stdout_is("sort: check_fail.txt:6: disorder: 5\n"); + .stderr_only("sort: check_fail.txt:6: disorder: 5\n"); new_ucmd!() .arg(diagnose_arg) .arg("multiple_files.expected") .succeeds() - .stdout_is(""); + .stderr_is(""); } } @@ -839,9 +839,9 @@ fn test_nonexistent_file() { .status_code(2) .stderr_only( #[cfg(not(windows))] - "sort: cannot read: \"nonexistent.txt\": No such file or directory (os error 2)", + "sort: cannot read: nonexistent.txt: No such file or directory", #[cfg(windows)] - "sort: cannot read: \"nonexistent.txt\": The system cannot find the file specified. (os error 2)", + "sort: cannot read: nonexistent.txt: The system cannot find the file specified.", ); } @@ -960,6 +960,49 @@ fn test_key_takes_one_arg() { .stdout_is_fixture("keys_open_ended.expected"); } +#[test] +fn test_verifies_out_file() { + let inputs = ["" /* no input */, "some input"]; + for &input in &inputs { + new_ucmd!() + .args(&["-o", "nonexistent_dir/nonexistent_file"]) + .pipe_in(input) + .fails() + .status_code(2) + .stderr_only( + #[cfg(not(windows))] + "sort: open failed: nonexistent_dir/nonexistent_file: No such file or directory", + #[cfg(windows)] + "sort: open failed: nonexistent_dir/nonexistent_file: The system cannot find the path specified.", + ); + } +} + +#[test] +fn test_verifies_files_after_keys() { + new_ucmd!() + .args(&[ + "-o", + "nonexistent_dir/nonexistent_file", + "-k", + "0", + "nonexistent_dir/input_file", + ]) + .fails() + .status_code(2) + .stderr_contains("failed to parse key"); +} + +#[test] +#[cfg(unix)] +fn test_verifies_input_files() { + new_ucmd!() + .args(&["/dev/random", "nonexistent_file"]) + .fails() + .status_code(2) + .stderr_is("sort: cannot read: nonexistent_file: No such file or directory"); +} + #[test] fn test_separator_null() { new_ucmd!()