Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow streaming, line-buffered input and output #287

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
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
4 changes: 4 additions & 0 deletions src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ w - match full words only
*/
pub flags: Option<String>,

#[arg(short = 'l', long = "line-buffered", default_value_t = false)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#[arg(short = 'l', long = "line-buffered", default_value_t = false)]
#[arg(short, long, default_value_t)]

Nit: we can use clap's "magic values" for all of these

/// Buffered mode. Doesn't support multiline matching
pub line_buffered: bool,

/// The regexp or string (if using `-F`) to search for.
pub find: String,

Expand Down
30 changes: 29 additions & 1 deletion src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use clap::Parser;
use memmap2::MmapMut;
use std::{
fs,
io::{stdout, Write},
io::{stdout, BufRead, Write},
ops::DerefMut,
path::PathBuf,
process,
Expand Down Expand Up @@ -43,6 +43,34 @@ fn try_main() -> Result<()> {
Source::from_stdin()
};

if options.line_buffered && sources == Source::from_stdin() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently this silently ignores the --line-buffered flag when running with files which seems pretty problematic

let stdin = std::io::stdin();
let stdout = std::io::stdout();
let mut handle = stdout.lock();

let mut buffer = String::new();

loop {
let mut read_handle = stdin.lock();
let bytes_read = read_handle.read_line(&mut buffer)?;

// .lock()
// .read_line(&mut buffer)
// .expect("Error reading from standard input");
if bytes_read == 0 {
break;
}
let replaced = replacer.replace(buffer.as_bytes());
handle
.write_all(replaced.as_ref())
.expect("Error writing to standard output");
handle.flush().expect("Error flushing output");
Comment on lines +64 to +67
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
handle
.write_all(replaced.as_ref())
.expect("Error writing to standard output");
handle.flush().expect("Error flushing output");
handle.write_all(replaced.as_ref())?;
handle.flush()?;

Failing to write is a pretty normal operation that we handle with the normal failure paths currently

buffer.clear();
Copy link
Collaborator

@CosmicHorrorDev CosmicHorrorDev Dec 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer if this buffer.clear() was moved right before the .read_line(). Forgetting to clear a buffer that you're reusing is a super common mistake that's annoying to debug

}

return Ok(());
}

let mut mmaps = Vec::new();
for source in sources.iter() {
let mmap = match source {
Expand Down