diff --git a/Cargo.lock b/Cargo.lock index 0e1d3ec..387caec 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1643,6 +1643,7 @@ dependencies = [ "clap", "regex", "tar", + "tempfile", "thiserror", "uucore", ] diff --git a/benches/bench_tar.rs b/benches/bench_tar.rs index cb11a1c..0d2677e 100644 --- a/benches/bench_tar.rs +++ b/benches/bench_tar.rs @@ -4,7 +4,7 @@ // at various scales to track performance over time. use std::fs::{self, File}; -use std::io::Write; +use std::io::{self, Write}; use std::path::{Path, PathBuf}; use tar::operations; use tempfile::TempDir; @@ -39,7 +39,9 @@ fn collect_files(dir: &Path) -> Vec { fn build_archive(archive_path: &Path, source_dir: &Path) { let files = collect_files(source_dir); let refs: Vec<&Path> = files.iter().map(|p| p.as_path()).collect(); - operations::create::create_archive(archive_path, &refs, false, false).unwrap(); + let output = File::create(archive_path).unwrap(); + let status_output = io::sink(); + operations::create::create_archive(output, status_output, &refs, true, false).unwrap(); } // --------------------------------------------------------------------------- @@ -57,7 +59,9 @@ fn create_archive_10_files(bencher: divan::Bencher) { bencher.bench_local(|| { let refs: Vec<&Path> = files.iter().map(|p| p.as_path()).collect(); - operations::create::create_archive(&archive_path, &refs, false, false).unwrap(); + let output = File::create(&archive_path).unwrap(); + let status_output = io::sink(); + operations::create::create_archive(output, status_output, &refs, true, false).unwrap(); }); } @@ -72,7 +76,9 @@ fn create_archive_100_files(bencher: divan::Bencher) { bencher.bench_local(|| { let refs: Vec<&Path> = files.iter().map(|p| p.as_path()).collect(); - operations::create::create_archive(&archive_path, &refs, false, false).unwrap(); + let output = File::create(&archive_path).unwrap(); + let status_output = io::sink(); + operations::create::create_archive(output, status_output, &refs, true, false).unwrap(); }); } @@ -87,7 +93,10 @@ fn create_archive_directory(bencher: divan::Bencher) { let archive_path = out.path().join("bench.tar"); bencher.bench_local(|| { - operations::create::create_archive(&archive_path, &[sub.as_path()], false, false).unwrap(); + let output = File::create(&archive_path).unwrap(); + let status_output = io::sink(); + operations::create::create_archive(output, status_output, &[sub.as_path()], true, false) + .unwrap(); }); } @@ -104,7 +113,8 @@ fn list_archive_50_files(bencher: divan::Bencher) { build_archive(&archive_path, source.path()); bencher.bench_local(|| { - operations::list::list_archive(&archive_path, false).unwrap(); + let input = File::open(&archive_path).unwrap(); + operations::list::list_archive(input, false).unwrap(); }); } @@ -117,7 +127,8 @@ fn list_archive_verbose_50_files(bencher: divan::Bencher) { build_archive(&archive_path, source.path()); bencher.bench_local(|| { - operations::list::list_archive(&archive_path, true).unwrap(); + let input = File::open(&archive_path).unwrap(); + operations::list::list_archive(input, true).unwrap(); }); } @@ -138,7 +149,8 @@ fn extract_archive_20_files(bencher: divan::Bencher) { .with_inputs(|| TempDir::new().unwrap()) .bench_local_values(|extract_dir| { std::env::set_current_dir(extract_dir.path()).unwrap(); - operations::extract::extract_archive(&archive_path, false).unwrap(); + let input = File::open(&archive_path).unwrap(); + operations::extract::extract_archive(input, &archive_path, false).unwrap(); }); std::env::set_current_dir(original_dir).unwrap(); } diff --git a/src/uu/tar/Cargo.toml b/src/uu/tar/Cargo.toml index 3b341c7..c9e4375 100644 --- a/src/uu/tar/Cargo.toml +++ b/src/uu/tar/Cargo.toml @@ -26,3 +26,6 @@ path = "src/tar.rs" [[bin]] name = "tar" path = "src/main.rs" + +[dev-dependencies] +tempfile = { workspace = true } diff --git a/src/uu/tar/src/errors.rs b/src/uu/tar/src/errors.rs index 5630a95..de098bf 100644 --- a/src/uu/tar/src/errors.rs +++ b/src/uu/tar/src/errors.rs @@ -54,6 +54,10 @@ pub enum TarError { /// Cannot finalize the archive #[error("tar: Cannot finalize archive: {0}")] CannotFinalizeArchive(io::Error), + + /// Refusing to write archive contents to terminal + #[error("tar: Refusing to write archive contents to terminal (missing -f option?)")] + RefuseWriteArchiveToTerminal, } impl TarError { diff --git a/src/uu/tar/src/operations/create.rs b/src/uu/tar/src/operations/create.rs index 39b6d63..c79b144 100644 --- a/src/uu/tar/src/operations/create.rs +++ b/src/uu/tar/src/operations/create.rs @@ -5,8 +5,8 @@ use crate::errors::TarError; use std::collections::VecDeque; -use std::fs::{self, File}; -use std::io::{self, BufWriter, Write}; +use std::fs; +use std::io::{BufWriter, Write}; use std::path::Component::{self, ParentDir, Prefix, RootDir}; use std::path::{self, Path, PathBuf}; use tar::Builder; @@ -28,23 +28,19 @@ use uucore::error::UResult; /// - Any input file cannot be read /// - Files cannot be added due to I/O or permission errors pub fn create_archive( - archive_path: &Path, + output: impl Write, + status_output: impl Write, files: &[&Path], allow_absolute: bool, verbose: bool, ) -> UResult<()> { - // Create the output file - let file = File::create(archive_path).map_err(|e| TarError::CannotCreateArchive { - path: archive_path.to_path_buf(), - source: e, - })?; + let mut output = BufWriter::new(output); + let mut status_output = BufWriter::new(status_output); // Create Builder instance - let mut builder = Builder::new(file); + let mut builder = Builder::new(&mut output); builder.preserve_absolute(allow_absolute); - let mut out = BufWriter::new(io::stdout().lock()); - // Add each file or directory to the archive for &path in files { // Check if path exists @@ -68,7 +64,7 @@ pub fn create_archive( }) .collect::>() .join("\n"); - writeln!(out, "{to_print}").map_err(TarError::Io)?; + writeln!(status_output, "{to_print}").map_err(TarError::Io)?; } // Normalize path if needed (so far, handles only absolute paths) @@ -81,8 +77,8 @@ pub fn create_archive( .iter() .collect(); writeln!( - out, - "Removing leading `{}' from member names", + std::io::stderr(), + "tar: Removing leading `{}' from member names", removed.display() ) .map_err(TarError::Io)?; @@ -112,9 +108,11 @@ pub fn create_archive( } } - // Finish writing the archive - out.flush().map_err(TarError::Io)?; builder.finish().map_err(TarError::CannotFinalizeArchive)?; + drop(builder); + + status_output.flush().map_err(TarError::Io)?; + output.flush().map_err(TarError::Io)?; Ok(()) } @@ -148,3 +146,32 @@ fn normalize_path(path: &Path, allow_absolute: bool) -> Option { None } } +#[cfg(test)] +mod tests { + use super::*; + use std::io::{self, Write}; + use tempfile::TempDir; + + struct FailFlushWriter; + impl Write for FailFlushWriter { + fn write(&mut self, buf: &[u8]) -> io::Result { + Ok(buf.len()) + } + fn flush(&mut self) -> io::Result<()> { + Err(io::Error::other("flush failed")) + } + } + + #[test] + fn test_create_archive_flush_failed() { + let dir = TempDir::new().unwrap(); + let file_path = dir.path().join("test.txt"); + fs::write(&file_path, "hello").unwrap(); + + let output = FailFlushWriter; + let status_output = io::sink(); + + let res = create_archive(output, status_output, &[file_path.as_path()], false, false); + assert!(res.is_err()); + } +} diff --git a/src/uu/tar/src/operations/extract.rs b/src/uu/tar/src/operations/extract.rs index 135b66f..47cd1dc 100644 --- a/src/uu/tar/src/operations/extract.rs +++ b/src/uu/tar/src/operations/extract.rs @@ -4,8 +4,7 @@ // file that was distributed with this source code. use crate::errors::TarError; -use std::fs::File; -use std::io::{self, BufWriter, Write}; +use std::io::{self, BufReader, BufWriter, Read, Write}; use std::path::Path; use tar::Archive; use uucore::error::UResult; @@ -23,12 +22,9 @@ use uucore::error::UResult; /// - The archive file cannot be opened /// - The archive format is invalid /// - Files cannot be extracted due to I/O or permission errors -pub fn extract_archive(archive_path: &Path, verbose: bool) -> UResult<()> { - // Open the archive file - let file = File::open(archive_path).map_err(|e| TarError::from_io_error(e, archive_path))?; - +pub fn extract_archive(input: impl Read, archive_path: &Path, verbose: bool) -> UResult<()> { // Create Archive instance - let mut archive = Archive::new(file); + let mut archive = Archive::new(BufReader::new(input)); let mut out = BufWriter::new(io::stdout().lock()); // Extract to current directory diff --git a/src/uu/tar/src/operations/list.rs b/src/uu/tar/src/operations/list.rs index c424ca8..0bd98e2 100644 --- a/src/uu/tar/src/operations/list.rs +++ b/src/uu/tar/src/operations/list.rs @@ -5,18 +5,14 @@ use crate::errors::TarError; use chrono::{TimeZone, Utc}; -use std::fs::File; -use std::io::{self, BufWriter, Write}; -use std::path::Path; +use std::io::{self, BufReader, BufWriter, Read, Write}; use tar::Archive; use uucore::error::UResult; use uucore::fs::display_permissions_unix; /// List the contents of a tar archive, printing one entry per line. -pub fn list_archive(archive_path: &Path, verbose: bool) -> UResult<()> { - let file: File = - File::open(archive_path).map_err(|e| TarError::from_io_error(e, archive_path))?; - let mut archive = Archive::new(file); +pub fn list_archive(input: impl Read, verbose: bool) -> UResult<()> { + let mut archive = Archive::new(BufReader::new(input)); let mut out = BufWriter::new(io::stdout().lock()); for entry_result in archive.entries().map_err(TarError::CannotReadEntries)? { diff --git a/src/uu/tar/src/tar.rs b/src/uu/tar/src/tar.rs index ff6800f..61113f6 100644 --- a/src/uu/tar/src/tar.rs +++ b/src/uu/tar/src/tar.rs @@ -6,7 +6,10 @@ pub mod errors; pub mod operations; +use crate::errors::TarError; use clap::{arg, crate_version, ArgAction, Command}; +use std::fs::File; +use std::io::{self, IsTerminal}; use std::path::{Path, PathBuf}; use uucore::error::UResult; use uucore::format_usage; @@ -139,7 +142,13 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { uucore::error::USimpleError::new(64, "option requires an argument -- 'f'") })?; - return operations::extract::extract_archive(archive_path, verbose); + return if archive_path == Path::new("-") { + operations::extract::extract_archive(io::stdin(), archive_path, verbose) + } else { + let file = + File::open(archive_path).map_err(|e| TarError::from_io_error(e, archive_path))?; + operations::extract::extract_archive(file, archive_path, verbose) + }; } // Handle create operation @@ -160,7 +169,35 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { )); } - return operations::create::create_archive(archive_path, &files, allow_absolute, verbose); + let output_is_stdout = archive_path == Path::new("-"); + return if output_is_stdout { + if io::stdout().is_terminal() { + Err(TarError::RefuseWriteArchiveToTerminal.into()) + } else { + let output = io::stdout().lock(); + let status_output = io::stderr(); + operations::create::create_archive( + output, + status_output, + &files, + allow_absolute, + verbose, + ) + } + } else { + let output = File::create(archive_path).map_err(|e| TarError::CannotCreateArchive { + path: archive_path.clone(), + source: e, + })?; + let status_output = io::stdout().lock(); + operations::create::create_archive( + output, + status_output, + &files, + allow_absolute, + verbose, + ) + }; } // Handle list operation @@ -169,7 +206,13 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { uucore::error::USimpleError::new(64, "option requires an argument -- 'f'") })?; - return operations::list::list_archive(archive_path, verbose); + return if archive_path == Path::new("-") { + operations::list::list_archive(io::stdin(), verbose) + } else { + let file = + File::open(archive_path).map_err(|e| TarError::from_io_error(e, archive_path))?; + operations::list::list_archive(file, verbose) + }; } // If no operation specified, show error diff --git a/tests/by-util/test_tar.rs b/tests/by-util/test_tar.rs index 3fb603f..a426e1b 100644 --- a/tests/by-util/test_tar.rs +++ b/tests/by-util/test_tar.rs @@ -145,6 +145,17 @@ fn test_create_verbose() { assert!(at.file_exists("archive.tar")); } +#[test] +fn test_create_verbose_to_stdout_uses_stderr() { + let (at, mut ucmd) = at_and_ucmd!(); + + at.write("file.txt", "hello"); + + ucmd.args(&["-cvf", "-", "file.txt"]) + .succeeds() + .stderr_contains("file.txt"); +} + #[test] fn test_create_empty_archive_fails() { new_ucmd!() @@ -179,7 +190,7 @@ fn test_create_absolute_path() { &file_abs_path.display().to_string(), ]) .succeeds() - .stdout_contains("Removing leading"); + .stderr_contains("tar: Removing leading"); assert!(at.file_exists("archive-trimed.tar")); @@ -814,3 +825,115 @@ fn test_list_conflicts_with_extract() { .code_is(2) .stderr_contains("cannot be used with"); } + +// stdin/stdout (-f -) tests + +#[test] +fn test_create_to_stdout() { + let (at, mut ucmd) = at_and_ucmd!(); + at.write("file.txt", "hello"); + + let result = ucmd.args(&["-cf", "-", "file.txt"]).succeeds(); + let bytes = result.stdout(); + // A tar archive is at least two blocks: one header + two end-of-archive blocks + assert!( + bytes.len() >= TAR_BLOCK_SIZE, + "stdout should contain tar data (got {} bytes)", + bytes.len() + ); + // The first 100 bytes of a tar header are the file name + let header_prefix = std::str::from_utf8(&bytes[..100]).unwrap_or(""); + assert!( + header_prefix.contains("file.txt"), + "tar header should contain filename" + ); +} + +#[test] +fn test_create_verbose_to_stdout_keeps_stdout_as_tar_data() { + let (at, mut ucmd) = at_and_ucmd!(); + at.write("file.txt", "hello"); + + let result = ucmd.args(&["-cvf", "-", "file.txt"]).succeeds(); + let bytes = result.stdout(); + let header_prefix = std::str::from_utf8(&bytes[..100]).unwrap_or(""); + + assert!( + bytes.len() >= TAR_BLOCK_SIZE, + "stdout should contain tar data (got {} bytes)", + bytes.len() + ); + assert!( + header_prefix.contains("file.txt"), + "tar header should contain filename" + ); + assert_eq!(&bytes[..8], b"file.txt"); + assert_eq!( + bytes[8], 0, + "stdout should not contain verbose output prefix" + ); +} + +#[test] +fn test_create_absolute_path_to_stdout_uses_stderr_for_normalization_notice() { + let (at, mut ucmd) = at_and_ucmd!(); + + let mut file_abs_path = PathBuf::from(at.root_dir_resolved()); + file_abs_path.push("file1.txt"); + + at.write(&file_abs_path.display().to_string(), "content1"); + + let result = ucmd + .args(&["-cf", "-", &file_abs_path.display().to_string()]) + .succeeds(); + result.stderr_contains("Removing leading"); + let bytes = result.stdout(); + let header_prefix = std::str::from_utf8(&bytes[..100]).unwrap_or(""); + + assert!( + bytes.len() >= TAR_BLOCK_SIZE, + "stdout should contain tar data (got {} bytes)", + bytes.len() + ); + assert!( + !header_prefix.contains("Removing leading"), + "stdout should not contain normalization notices" + ); +} + +#[test] +fn test_list_from_stdin() { + let (at, mut ucmd) = at_and_ucmd!(); + at.write("file.txt", "hello"); + + // Create archive to a file in the same sandbox + new_ucmd!() + .args(&["-cf", &at.plus_as_string("archive.tar"), "file.txt"]) + .current_dir(at.as_string()) + .succeeds(); + + let archive_bytes = at.read_bytes("archive.tar"); + ucmd.args(&["-tf", "-"]) + .pipe_in(archive_bytes) + .succeeds() + .stdout_contains("file.txt"); +} + +#[test] +fn test_extract_from_stdin() { + let (at, mut ucmd) = at_and_ucmd!(); + at.write("file.txt", "hello world"); + + // Create archive to a file in the same sandbox + new_ucmd!() + .args(&["-cf", &at.plus_as_string("archive.tar"), "file.txt"]) + .current_dir(at.as_string()) + .succeeds(); + + at.remove("file.txt"); + + let archive_bytes = at.read_bytes("archive.tar"); + ucmd.args(&["-xf", "-"]).pipe_in(archive_bytes).succeeds(); + + assert_eq!(at.read("file.txt"), "hello world"); +}