From 21ae26cb17ad0dfe815ce001be84f40078c5a4a7 Mon Sep 17 00:00:00 2001 From: Niklas Mohrin Date: Wed, 25 Nov 2020 00:24:04 +0100 Subject: [PATCH 1/3] Add cycle detection with clircle, now v0.2 --- Cargo.lock | 41 ++++++++++++++++++++++++++++++++++++----- Cargo.toml | 1 + src/controller.rs | 14 ++++++++++++++ src/input.rs | 14 ++++++++++++++ 4 files changed, 65 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5d7eda7e..060c41e0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -99,6 +99,7 @@ dependencies = [ "assert_cmd", "atty", "clap", + "clircle", "console", "content_inspector", "dirs", @@ -218,6 +219,12 @@ version = "0.1.10" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "4785bdd1c96b2a846b2bd7cc02e86b6b3dbf14e7e53446c4f54c92a361040822" +[[package]] +name = "cfg-if" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "baf1de4339761588bc0619e3cbc0120ee582ebb74b53b4efbf79117bd2da40fd" + [[package]] name = "chrono" version = "0.4.19" @@ -244,6 +251,18 @@ dependencies = [ "vec_map", ] +[[package]] +name = "clircle" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e27a01e782190a8314e65cc94274d9bbcd52e05a9d15b437fe2b31259b854b0d" +dependencies = [ + "cfg-if 1.0.0", + "nix", + "serde", + "winapi", +] + [[package]] name = "console" version = "0.14.0" @@ -280,7 +299,7 @@ version = "1.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ba125de2af0df55319f41944744ad91c71113bf74a4646efff39afe1f6842db1" dependencies = [ - "cfg-if", + "cfg-if 0.1.10", ] [[package]] @@ -290,7 +309,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c3c7c73a2d1e9fc0886a08b93e98eb643461230d5f1925e4036204d5f2e261a8" dependencies = [ "autocfg", - "cfg-if", + "cfg-if 0.1.10", "lazy_static", ] @@ -442,7 +461,7 @@ version = "1.0.18" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "da80be589a72651dcda34d8b35bcdc9b7254ad06325611074d9cc0fbb19f60ee" dependencies = [ - "cfg-if", + "cfg-if 0.1.10", "crc32fast", "libc", "miniz_oxide", @@ -484,7 +503,7 @@ version = "0.1.15" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "fc587bc0ec293155d5bfa6b9891ec18a1e330c234f896ea47fbada4cadbe47e6" dependencies = [ - "cfg-if", + "cfg-if 0.1.10", "libc", "wasi", ] @@ -635,7 +654,7 @@ version = "0.4.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "4fabed175da42fed1fa0746b0ea71f412aa9d35e76e95e59b192c64b9dc2bf8b" dependencies = [ - "cfg-if", + "cfg-if 0.1.10", ] [[package]] @@ -666,6 +685,18 @@ dependencies = [ "autocfg", ] +[[package]] +name = "nix" +version = "0.19.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b2ccba0cfe4fdf15982d1674c69b1fd80bad427d293849982668dfe454bd61f2" +dependencies = [ + "bitflags", + "cc", + "cfg-if 1.0.0", + "libc", +] + [[package]] name = "normalize-line-endings" version = "0.3.0" diff --git a/Cargo.toml b/Cargo.toml index 6afe7523..50d7df94 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -50,6 +50,7 @@ serde = { version = "1.0", features = ["derive"] } serde_yaml = "0.8" semver = "0.11" path_abs = { version = "0.5", default-features = false } +clircle = "0.2.0" [dependencies.git2] version = "0.13" diff --git a/src/controller.rs b/src/controller.rs index 8dd73986..a1e3f119 100644 --- a/src/controller.rs +++ b/src/controller.rs @@ -1,3 +1,4 @@ +use std::convert::TryFrom; use std::io::{self, Write}; use crate::assets::HighlightingAssets; @@ -66,6 +67,14 @@ impl<'b> Controller<'b> { } let attached_to_pager = output_type.is_pager(); + let rw_cycle_detected = !attached_to_pager && { + let identifiers: Vec<_> = inputs + .iter() + .flat_map(clircle::Identifier::try_from) + .collect(); + clircle::stdout_among_inputs(&identifiers) + }; + let writer = output_type.handle()?; let mut no_errors: bool = true; @@ -78,6 +87,11 @@ impl<'b> Controller<'b> { } }; + if rw_cycle_detected { + print_error(&"The output file is also an input!".into(), writer); + return Ok(false); + } + for (index, input) in inputs.into_iter().enumerate() { match input.open(io::stdin().lock()) { Err(error) => { diff --git a/src/input.rs b/src/input.rs index 5d8493c0..5b6a4e67 100644 --- a/src/input.rs +++ b/src/input.rs @@ -1,6 +1,8 @@ +use std::convert::TryFrom; use std::ffi::{OsStr, OsString}; use std::fs::File; use std::io::{self, BufRead, BufReader, Read}; +use std::path::Path; use content_inspector::{self, ContentType}; @@ -191,6 +193,18 @@ impl<'a> Input<'a> { } } +impl TryFrom<&'_ Input<'_>> for clircle::Identifier { + type Error = (); + + fn try_from(input: &Input) -> std::result::Result { + match input.kind { + InputKind::OrdinaryFile(ref path) => Self::try_from(Path::new(path)).map_err(|_| ()), + InputKind::StdIn => Self::try_from(clircle::Stdio::Stdin).map_err(|_| ()), + InputKind::CustomReader(_) => Err(()), + } + } +} + pub(crate) struct InputReader<'a> { inner: Box, pub(crate) first_line: Vec, From ace655e164a4ce383cc5b6664e8cf5e360ddae35 Mon Sep 17 00:00:00 2001 From: Niklas Mohrin Date: Tue, 5 Jan 2021 10:54:14 +0100 Subject: [PATCH 2/3] Add integration tests for clircle cycle detection --- tests/examples/cycle.txt | 0 tests/integration_tests.rs | 37 ++++++++++++++++++++++++++++++++++--- 2 files changed, 34 insertions(+), 3 deletions(-) create mode 100644 tests/examples/cycle.txt diff --git a/tests/examples/cycle.txt b/tests/examples/cycle.txt new file mode 100644 index 00000000..e69de29b diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index 3373aa7d..3f125aa1 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -1,11 +1,14 @@ -use assert_cmd::Command; +use assert_cmd::assert::OutputAssertExt; +use assert_cmd::cargo::CommandCargoExt; use predicates::{prelude::predicate, str::PredicateStrExt}; +use std::fs::File; use std::path::Path; +use std::process::{Command, Stdio}; use std::str::from_utf8; const EXAMPLES_DIR: &str = "tests/examples"; -fn bat_with_config() -> Command { +fn bat_raw_command() -> Command { let mut cmd = Command::cargo_bin("bat").unwrap(); cmd.current_dir("tests/examples"); cmd.env_remove("PAGER"); @@ -17,7 +20,11 @@ fn bat_with_config() -> Command { cmd } -fn bat() -> Command { +fn bat_with_config() -> assert_cmd::Command { + assert_cmd::Command::from_std(bat_raw_command()) +} + +fn bat() -> assert_cmd::Command { let mut cmd = bat_with_config(); cmd.arg("--no-config"); cmd @@ -189,6 +196,30 @@ fn line_range_multiple() { .stdout("line 1\nline 2\nline 4\n"); } +#[test] +fn basic_io_cycle() { + let file_out = Stdio::from(File::open("tests/examples/cycle.txt").unwrap()); + bat_raw_command() + .arg("test.txt") + .arg("cycle.txt") + .stdout(file_out) + .assert() + .failure(); +} + +#[test] +fn stdin_to_stdout_cycle() { + let file_out = Stdio::from(File::open("tests/examples/cycle.txt").unwrap()); + let file_in = Stdio::from(File::open("tests/examples/cycle.txt").unwrap()); + bat_raw_command() + .stdin(file_in) + .arg("test.txt") + .arg("-") + .stdout(file_out) + .assert() + .failure(); +} + #[test] fn tabs_numbers() { bat() From b600f62ab68e0a11a9bb69dcf45648632d3f52ee Mon Sep 17 00:00:00 2001 From: Niklas Mohrin Date: Tue, 5 Jan 2021 10:55:22 +0100 Subject: [PATCH 3/3] Add unix specific integration test about running bat without any arguments --- Cargo.lock | 1 + Cargo.toml | 1 + tests/integration_tests.rs | 53 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 060c41e0..9d34c3c4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -117,6 +117,7 @@ dependencies = [ "syntect", "tempdir", "unicode-width", + "wait-timeout", "wild", ] diff --git a/Cargo.toml b/Cargo.toml index 50d7df94..212db77a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -76,6 +76,7 @@ default-features = false tempdir = "0.3" assert_cmd = "1.0.2" predicates = "1.0.6" +wait-timeout = "0.2.0" [build-dependencies] clap = { version = "2.33", optional = true } diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index 3f125aa1..e16a8a91 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -5,8 +5,11 @@ use std::fs::File; use std::path::Path; use std::process::{Command, Stdio}; use std::str::from_utf8; +use std::time::Duration; const EXAMPLES_DIR: &str = "tests/examples"; +const SAFE_CHILD_PROCESS_CREATION_TIME: Duration = Duration::from_millis(100); +const CHILD_WAIT_TIMEOUT: Duration = Duration::from_secs(15); fn bat_raw_command() -> Command { let mut cmd = Command::cargo_bin("bat").unwrap(); @@ -220,6 +223,56 @@ fn stdin_to_stdout_cycle() { .failure(); } +#[cfg(unix)] +#[test] +fn no_args_doesnt_break() { + use std::io::Write; + use std::os::unix::io::FromRawFd; + use std::thread; + + use clircle::nix::pty::{openpty, OpenptyResult}; + use wait_timeout::ChildExt; + + // To simulate bat getting started from the shell, a process is created with stdin and stdout + // as the slave end of a pseudo terminal. Although both point to the same "file", bat should + // not exit, because in this case it is safe to read and write to the same fd, which is why + // this test exists. + let OpenptyResult { master, slave } = openpty(None, None).expect("Couldn't open pty."); + let mut master = unsafe { File::from_raw_fd(master) }; + let stdin = unsafe { Stdio::from_raw_fd(slave) }; + let stdout = unsafe { Stdio::from_raw_fd(slave) }; + + let mut child = bat_raw_command() + .stdin(stdin) + .stdout(stdout) + .spawn() + .expect("Failed to start."); + + // Some time for the child process to start and to make sure, that we can poll the exit status. + // Although this waiting period is not necessary, it is best to keep it in and be absolutely + // sure, that the try_wait does not error later. + thread::sleep(SAFE_CHILD_PROCESS_CREATION_TIME); + + // The child process should be running and waiting for input, + // therefore no exit status should be available. + let exit_status = child + .try_wait() + .expect("Error polling exit status, this should never happen."); + assert!(exit_status.is_none()); + + // Write Ctrl-D (end of transmission) to the pty. + master + .write_all(&[0x04]) + .expect("Couldn't write EOT character to master end."); + + let exit_status = child + .wait_timeout(CHILD_WAIT_TIMEOUT) + .expect("Error polling exit status, this should never happen.") + .expect("Exit status not set, but the child should have exited already."); + + assert!(exit_status.success()); +} + #[test] fn tabs_numbers() { bat()