From a33b6d87b578990893257c2b063f8579a3f960c4 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Fri, 30 Jul 2021 20:48:07 +0200 Subject: [PATCH] sort: open the output file at the beginning This causes us to print an error message about an invalid output file right at the start of the invocation, but *after* verifying other arguments. --- src/uu/sort/src/ext_sort.rs | 18 ++++++++--- src/uu/sort/src/merge.rs | 6 ++-- src/uu/sort/src/sort.rs | 59 ++++++++++++++++++++++--------------- tests/by-util/test_sort.rs | 27 +++++++++++++++++ 4 files changed, 80 insertions(+), 30 deletions(-) diff --git a/src/uu/sort/src/ext_sort.rs b/src/uu/sort/src/ext_sort.rs index 201dda8bb..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 => { 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 3d7e890b9..6e4cd8a23 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()); - crash!(2, "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, @@ -1053,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); @@ -1099,9 +1106,11 @@ pub fn uumain(args: impl uucore::Args) -> i32 { ); } + 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> { @@ -1334,10 +1343,10 @@ pub fn uu_app() -> App<'static, 'static> { .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!(2, "only one file allowed with -c"); @@ -1346,7 +1355,7 @@ fn exec(files: &[String], settings: &GlobalSettings) -> i32 { } else { let mut lines = files.iter().map(open); - ext_sort(&mut lines, settings); + ext_sort(&mut lines, settings, output); } 0 } @@ -1618,8 +1627,12 @@ 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); } diff --git a/tests/by-util/test_sort.rs b/tests/by-util/test_sort.rs index 1add0bb54..b4b4ba41c 100644 --- a/tests/by-util/test_sort.rs +++ b/tests/by-util/test_sort.rs @@ -959,3 +959,30 @@ fn test_key_takes_one_arg() { .succeeds() .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_out_file_after_keys() { + new_ucmd!() + .args(&["-o", "nonexistent_dir/nonexistent_file", "-k", "0"]) + .fails() + .status_code(2) + .stderr_contains("failed to parse key"); +}