Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ use crate::{
extension::{Extension, build_archive_file_suggestion},
info_accessible,
utils::{
NoQuotePathFmt, PathFmt, append_ascii_suffix_to_os_str, pretty_format_list_of_paths, try_infer_format,
user_wants_to_continue,
NoQuotePathFmt, PathFmt, append_ascii_suffix_to_os_str, is_path_stdin, pretty_format_list_of_paths,
try_infer_format, user_wants_to_continue,
},
warning,
};
Expand Down Expand Up @@ -205,6 +205,15 @@ 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(|| {
// "-" streams to stdout, which has no extension to detect a format from
if is_path_stdin(output_path) {
return FinalError::with_title("Cannot compress to \"-\" (stdout)")
.detail("The compression format cannot be detected because stdout has no file extension")
.hint("Specify the format with the '--format' flag:")
.hint(" ouch compress <FILES>... - --format tar.gz")
.hint(" ouch compress <FILES>... - --format zip")
.into();
}
FinalError::with_title(format!("Cannot compress to {}", PathFmt(output_path)))
.detail("You must supply the compression format")
.hint("Try adding supported extensions (see --help):")
Expand Down
3 changes: 2 additions & 1 deletion src/cli/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ pub enum Subcommand {
#[arg(required = true, value_hint = ValueHint::FilePath)]
files: Vec<PathBuf>,

/// The resulting file. Its extensions can be used to specify the compression formats
/// The resulting file, or "-" to write the archive to stdout (requires --format).
/// Its extensions can be used to specify the compression formats
#[arg(required = true, value_hint = ValueHint::FilePath)]
output: PathBuf,

Expand Down
6 changes: 5 additions & 1 deletion src/cli/completion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,11 @@ mod tests {
descriptions,
[
Some("Files to be compressed".to_owned()),
Some("The resulting file. Its extensions can be used to specify the compression formats".to_owned()),
Some(
"The resulting file, or \"-\" to write the archive to stdout (requires --format). Its extensions \
can be used to specify the compression formats"
.to_owned(),
),
]
);
}
Expand Down
101 changes: 97 additions & 4 deletions src/commands/compress.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,23 +26,25 @@ use crate::{
/// # Arguments:
/// - `files`: is the list of paths to be compressed: ["dir/file1.txt", "dir/file2.txt"]
/// - `extensions`: is a list of compression formats for compressing, example: [Tar, Gz] (in compression order)
/// - `output_file` is the resulting compressed file name, example: "archive.tar.gz"
/// - `output_writer` is where the compressed data is written to, usually the
/// output file created beforehand; pass `io::stdout()` to stream the archive
/// to standard output (when `output_path` is "-")
///
/// # Return value
/// - Returns `Ok(true)` if compressed all files normally.
/// - Returns `Ok(false)` if user opted to abort compression mid-way.
pub fn compress_files(
files: Vec<PathBuf>,
extensions: Vec<Extension>,
output_file: fs::File,
output_writer: impl Write + Send + 'static,
output_path: &Path,
follow_symlinks: bool,
question_policy: QuestionPolicy,
file_visibility_policy: FileVisibilityPolicy,
level: Option<i16>,
) -> Result<bool> {
// If the input files contain a directory, then the total size will be underestimated
let file_writer = BufWriter::with_capacity(BUFFER_CAPACITY, output_file);
let file_writer = BufWriter::with_capacity(BUFFER_CAPACITY, output_writer);

let mut writer: Box<dyn Send + Write> = Box::new(file_writer);

Expand Down Expand Up @@ -153,7 +155,6 @@ pub fn compress_files(
file_visibility_policy,
follow_symlinks,
)?;
writer.flush()?;
}
Zip => {
if !formats.is_empty() {
Expand Down Expand Up @@ -203,5 +204,97 @@ pub fn compress_files(
}
}

// Push everything left in the encoder chain and the BufWriter out to the
// target, so a failure on the final write (like a closed pipe when
// streaming to stdout) is reported instead of being lost on Drop
writer.flush()?;

Ok(true)
}

#[cfg(test)]
mod tests {
use std::sync::{Arc, Mutex};

use super::*;

/// Writer that stores the stream in a shared buffer and can be made to
/// fail on `flush`, emulating a reader that closed the pipe before the
/// archive stream was fully written
struct TestSink {
buffer: Arc<Mutex<Vec<u8>>>,
fail_on_flush: bool,
}

impl TestSink {
fn new() -> (Self, Arc<Mutex<Vec<u8>>>) {
let buffer = Arc::new(Mutex::new(Vec::new()));
let sink = Self {
buffer: buffer.clone(),
fail_on_flush: false,
};
(sink, buffer)
}

fn failing_on_flush() -> Self {
Self {
buffer: Arc::new(Mutex::new(Vec::new())),
fail_on_flush: true,
}
}
}

impl Write for TestSink {
fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
self.buffer.lock().unwrap().extend_from_slice(buf);
Ok(buf.len())
}

fn flush(&mut self) -> io::Result<()> {
if self.fail_on_flush {
Err(io::Error::new(io::ErrorKind::BrokenPipe, "reader went away"))
} else {
Ok(())
}
}
}

// A single-file format is used because archive formats like tar change
// the working directory, which is unsafe in parallel unit tests
fn compress_to(sink: TestSink) -> Result<bool> {
let mut input = tempfile::NamedTempFile::new().unwrap();
input.write_all(b"contents").unwrap();

compress_files(
vec![input.path().to_path_buf()],
vec![Extension::from_format(Bzip)],
sink,
Path::new("output.bz2"),
false,
QuestionPolicy::AlwaysYes,
FileVisibilityPolicy::new(),
None,
)
}

/// The final flush must propagate write errors, so a reader closing the
/// stream early (e.g. `| head`) cannot make compression report success
#[test]
fn flush_error_is_propagated() {
assert!(compress_to(TestSink::failing_on_flush()).is_err());
}

/// A healthy sink receives the whole stream, including the bytes flushed
/// at the end and the trailer written when the encoder chain is dropped
#[test]
fn stream_reaches_the_sink() {
let (sink, buffer) = TestSink::new();

assert!(compress_to(sink).unwrap());

let buffer = buffer.lock().unwrap();
// A bzip2 stream starts with the magic bytes "BZh"
assert!(buffer.starts_with(b"BZh"));
assert!(buffer.len() > 20);
}
}
83 changes: 61 additions & 22 deletions src/commands/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@ mod compress;
mod decompress;
mod list;

use std::path::PathBuf;
use std::{
io::{self, IsTerminal},
path::PathBuf,
};

use bstr::ByteSlice;
use decompress::{DecompressOptions, PreparedTarget, prepare_decompress_target};
Expand Down Expand Up @@ -101,13 +104,30 @@ pub fn run(args: CliArgs, question_policy: QuestionPolicy, file_visibility_polic
)?;
check::check_archive_formats_position(&formats, &output_path)?;

let (output_file, output_path) = match utils::create_file_or_prompt_on_conflict(
&output_path,
question_policy,
QuestionAction::Compression,
)? {
Some(writer) => writer,
None => return Ok(()),
// "-" streams the archive to stdout instead of creating an output
// file, so there is no path to create or conflict to resolve
let output_to_stdout = is_path_stdin(&output_path);
// Archive bytes on a terminal would flood the screen with garbage;
// only allow stdout as a target when it was redirected
if output_to_stdout && io::stdout().is_terminal() {
return Err(FinalError::with_title("Cannot compress to \"-\" (stdout)")
.detail("Refusing to write binary archive data to a terminal")
.hint("Redirect standard output to a file or pipe it to another command:")
.hint(" ouch compress <FILES>... - --format tar.gz > archive.tar.gz")
.hint("Or pass an output file name instead of \"-\"")
.into());
}
let (output_file, output_path) = if output_to_stdout {
(None, output_path)
} else {
match utils::create_file_or_prompt_on_conflict(
&output_path,
question_policy,
QuestionAction::Compression,
)? {
Some((file, path)) => (Some(file), path),
None => return Ok(()),
}
};

// Read on inputs. The output FD is held already so no write-path grant is needed, and
Expand All @@ -129,26 +149,45 @@ pub fn run(args: CliArgs, question_policy: QuestionPolicy, file_visibility_polic
level
};

let compress_result = compress_files(
files,
formats,
output_file,
&output_path,
follow_symlinks,
question_policy,
file_visibility_policy,
level,
);
let compress_result = match output_file {
Some(output_file) => compress_files(
files,
formats,
output_file,
&output_path,
follow_symlinks,
question_policy,
file_visibility_policy,
level,
),
// All diagnostics are emitted on stderr, so the archive bytes
// can be streamed to stdout without interleaving with logs
None => compress_files(
files,
formats,
io::stdout(),
&output_path,
follow_symlinks,
question_policy,
file_visibility_policy,
level,
),
};

if let Ok(true) = compress_result {
info_accessible!("Output file size: {}", BytesFmt(file_size(&output_path)?));
info_accessible!("Successfully compressed to {}", PathFmt(&output_path));
if output_to_stdout {
info_accessible!("Successfully compressed to stdout");
} else {
info_accessible!("Output file size: {}", BytesFmt(file_size(&output_path)?));
info_accessible!("Successfully compressed to {}", PathFmt(&output_path));
}
} else if let Ok(false) = compress_result {
// user cancelled; remove the partial output where the sandbox permits it
if !sandbox_active {
// (there is no partial file to remove when streaming to stdout)
if !output_to_stdout && !sandbox_active {
let _ = utils::remove_file_or_dir(&output_path);
}
} else if compress_result.is_err() {
} else if compress_result.is_err() && !output_to_stdout {
let deleted = !sandbox_active && utils::remove_file_or_dir(&output_path).is_ok();
if !deleted {
if sandbox_active {
Expand Down
61 changes: 61 additions & 0 deletions tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,67 @@ fn single_file_stdin(
assert_same_directory(before, after, false);
}

/// Compress files to standard output by passing "-" as the output path.
///
/// "-" must stream the archive bytes to stdout instead of creating a file
/// literally named "-", and "--format" is required because the compression
/// format cannot be detected from stdout.
#[test]
fn compress_to_stdout() {
let (_tempdir, dir) = testdir().unwrap();
let before = &dir.join("before");
fs::create_dir(before).unwrap();
let before_file = &before.join("file");
write_random_content(
&mut fs::File::create(before_file).unwrap(),
&mut SmallRng::from_os_rng(),
);

// Without "--format" there is no extension to detect the format from
let stderr = crate::utils::cargo_bin()
.args(["-A", "-y", "compress"])
.arg(before_file)
.arg("-")
.current_dir(dir)
.assert()
.failure()
.get_output()
.stderr
.clone();
assert!(
String::from_utf8_lossy(&stderr).contains("--format"),
"error should suggest the --format flag: {}",
String::from_utf8_lossy(&stderr)
);

let output = crate::utils::cargo_bin()
.args(["-A", "-y", "compress"])
.arg(before_file)
.args(["-", "--format", "tar"])
.current_dir(dir)
.assert()
.success()
.get_output()
.clone();

// A file literally named "-" must not be created
assert!(!dir.join("-").exists());
// A tar archive with one entry holds at least two 512-byte blocks
assert!(output.stdout.len() >= 1024);

// Round-trip: stream the archive back through `decompress -` (stdin)
let after = &dir.join("after");
crate::utils::cargo_bin()
.args(["-A", "-y", "decompress", "-", "-d"])
.arg(after)
.args(["--format", "tar"])
.write_stdin(output.stdout)
.assert()
.success();

assert_same_directory(before, after, false);
}

/// Compress and decompress a directory with random content generated with `create_random_files`
#[proptest(cases = 25)]
fn multiple_files(
Expand Down
Loading