Add --in/--in0 options to read input paths from files or stdin - #3462
Add --in/--in0 options to read input paths from files or stdin#3462ltrzesniewski wants to merge 1 commit into
--in/--in0 options to read input paths from files or stdin#3462Conversation
|
Actually I think I prefer the paths to be relative to the input file, as this makes more sense to me. I pushed a second commit which changes the behavior. Feel free to drop it if you'd rather have the |
BurntSushi
left a comment
There was a problem hiding this comment.
Nice work for an initial pass!
| .sp | ||
| When this flag is used multiple times or in combination with the \flag{in0} | ||
| flag, then all provided files are searched in addition to the paths in the | ||
| positional arguments. |
There was a problem hiding this comment.
I think this should be a little more specific. Something like:
When this flag is used, it behaves as if the file paths in the file provided
were passed as positional arguments in the same order as they are
in the file and relative to other positional arguments.
| flag, then all provided files are searched in addition to the paths in the | ||
| positional arguments. | ||
| .sp | ||
| Empty lines are ignored, and newlines (both LF and CRLF) are not counted as |
There was a problem hiding this comment.
find does not ignore empty lines and instead errors. I think we should do the same here. The one exception is that we should treat this file as line (or NUL) terminated, not delimited. That is, if the file has a trailing newline or NUL, then that's fine and doesn't indicate the start of another file path if nothing follows it.
(I note that empty lines are not ignored with ripgrep's -f/--file flag. They are treated as an empty string, which creates a regex that matches at all positions.)
For this flag, I think an empty line is just something we should error on. It's also the conservative choice. We can change an error into something else that isn't an error later, but we can't do the reverse.
| Empty lines are ignored, and newlines (both LF and CRLF) are not counted as | ||
| part of the path. The rest of the path name is decoded as UTF-8 on Windows | ||
| and taken as-is on Unix systems. | ||
| Use the \flag{in0} flag to use \fBNUL\fP as separator. |
There was a problem hiding this comment.
I would use the word "terminator" instead.
| Empty entries are ignored. The path names are decoded as UTF-8 on Windows | ||
| and taken as-is on Unix systems. | ||
| .sp | ||
| When \fIINPUTFILE\fP is \fB-\fP, then \fBstdin\fP will be read for the files. |
| InputSource::TextFile(PathBuf::from("baz")), | ||
| ], | ||
| args.inputs | ||
| ); |
There was a problem hiding this comment.
Can you add tests that mingle --in, --in0 and other positional paths?
| if state.stdin_consumed && path == Path::new("-") { | ||
| anyhow::bail!( | ||
| "error: attempted to read patterns from stdin \ | ||
| "error: attempted to read patterns or inputs from stdin \ |
| let mut paths = Vec::with_capacity(low.positional.len()); | ||
| for input in low.inputs.drain(..) { | ||
| Self::read_from_input(state, &input, &mut paths)? | ||
| } |
There was a problem hiding this comment.
I think we want users to be able to mingle --in and --in0 with positional paths and have them processed in the same order as given. This means, I think, that low.positional needs to become an InputSource and InputSource needs to grow a third single-file-path variant. To do this, I think this needs to be tweaked:
| reader.for_byte_line(|line| { | ||
| if line.contains(&0u8) { | ||
| return Err(std::io::Error::other(format!( | ||
| "--in {}: file contains a NUL byte, \ |
There was a problem hiding this comment.
Error messages should just start with a file path. The --in can come after the colon.
There was a problem hiding this comment.
Also, anyhow::bail! should be used here. And anywhere else you're building an io::Error like this.
There was a problem hiding this comment.
The issue I have with this is that for_byte_line/for_byte_record expect a function which returns io::Result<bool>. Maybe I'm stupid, but I don't see a way to use them with anyhow...
There was a problem hiding this comment.
Oh whoops, that's my bad. You aren't stupid. Thanks for pushing back. :-)
| /// A text file with newline-separated paths. Comes from the `--in` flag. | ||
| TextFile(PathBuf), | ||
| /// A text file with NUL-separated paths. Comes from the `--in0` flag. | ||
| BinaryFile(PathBuf), |
There was a problem hiding this comment.
These should be LineTerminated and NulTerminated.
| cmd.arg("--file").arg("-").arg("--in0").arg("-").arg("match"); | ||
| cmd.assert_exit_code(2); | ||
| } | ||
| ); |
There was a problem hiding this comment.
There are enough tests here that this warrants its own module.
Can you include tests that mingle --in, --in0 and positional file paths?
| "--no-ignore-files[don't respect --ignore-file flags]" | ||
| $no'--ignore-files[respect --ignore-file files]' | ||
|
|
||
| + input # Read input paths from file options |
There was a problem hiding this comment.
you can get rid of the group comment, i've removed them all now
| $no'--ignore-files[respect --ignore-file files]' | ||
|
|
||
| + input # Read input paths from file options | ||
| '*--in=[text file containing paths to search]: :_files' |
There was a problem hiding this comment.
these should both start with 'specify ...'
db18217 to
bf683fc
Compare
|
Thank you for your reviews @BurntSushi and @okdana! I took all your comments into account and rebased the PR. This version is basically a major rewrite, but it feels much better and more idiomatic to me thanks to your insights. 🙂 |
bf683fc to
3314671
Compare
|
I just rebased again because I noticed there were conflicts, and a new test needed an update. |
Adds `--in` and `--in0` command-line options, which read files or directories to be searched from their arguments, as if they were specified on the command line. The value `-` means the list of files should be read from stdin. e.g. having a file `files.txt` with the following content: ``` foo.txt bar.txt ``` Then running `rg pattern --in files.txt` is *equivalent* to running `rg pattern foo.txt bar.txt`. `--in` reads from a text file (supporting LF and CRLF line terminators), while `--in0` reads from a null-delimited file. Both flags can be used multiple times. These features are designed for reusing input files and ripgrep calls chaining. Fixes BurntSushi#3459 and BurntSushi#273
3314671 to
11df15d
Compare
This PR adds
--inand--in0command-line options, which read files or directories to be searched from their arguments, similar to as if they were specified on the command line.If the file contains relative paths, their paths are resolved relatively to the input file.The value-means the list of files should be read from stdin.e.g. having a file
files.txtwith the following content:Then running
rg pattern --in files.txtis equivalent to runningrg pattern foo.txt bar.txt.--inreads from a text file (supporting LF and CRLF line terminators), while--in0reads from a null-delimited file. Both flags can be used multiple times.Empty entries are ignored.These features are designed for reusing input files and ripgrep calls chaining, such as:
These commands will find all the files that contain
foo,barandbaz, and will highlight all of thebazmatches, with headings and line numbers. The second one is equivalent to:Implementation notes:
Currently, the paths read from the input file are treated exactly as if they were specified on the command line, which means relative paths are relative to the current working directory, not the directory of the input file. This may be surprising behavior, but is consistent with the wayxargsworks. Please tell me if you'd prefer for the paths to be relative to the input file instead (or the CWD when stdin is used). I really don't know which one is better TBH, so I defer to your opinion.Decision: interpret everything as if it were a command-lime argument.
--file, but I was reluctant to create agrep_cli::inputmodule similar togrep_cli::patternfor this kind of functionality, so I put everything inrg::flags::hiargs::Paths::read_from_input. This feels a bit messy/unidiomatic to me TBH, don't hesitate to suggest some other approach if you think it would be better.std::io::Resultandanyhow::Resultinread_from_inputdue to the signaturesbstr::io::BufReadExtexpects. I'm not sure if that's ok or not.--incontains a NUL byte, it's almost certainly an error, and is reported as such.Related issues:
--files-fromoption #273