Repository navigation
cmp: honor the byte limit in quiet size checks - #296
SichenLiang wants to merge 2 commits into
Conversation
| if params.quiet | ||
| && a_meta.is_file() | ||
| && b_meta.is_file() | ||
| && params.skip_a.unwrap_or(0) == 0 |
There was a problem hiding this comment.
why not subtract the skips from the sizes instead of disabling the shortcut?
| #[cfg(unix)] | ||
| { | ||
| let stdin = io::stdin().lock(); | ||
| // SAFETY: stdin's fd 0 must stay open and valid while locked. |
There was a problem hiding this comment.
do we really need unsafe here?
io::stdin().as_fd().try_clone_to_owned() would be safe, and failing on an fd limit of 4 seems acceptable to me
|
|
||
| #[test] | ||
| #[cfg(unix)] | ||
| fn cmp_zero_bytes_stdin_directory() -> Result<(), Box<dyn std::error::Error>> { |
There was a problem hiding this comment.
this is mostly the same as cmp_zero_bytes_stdin_directory_fd_limit and cmp_zero_bytes_directory_operand.
could you please factor the cmd + assert part into a helper? 250 lines of tests is a lot for this
| let mut cmd = std::process::Command::new("sh"); | ||
| cmd.env("LC_ALL", "C") | ||
| .current_dir(tmp_dir.path()) | ||
| .args(["-c", "ulimit -n 4 || exit; exec \"$@\"", "sh"]) |
There was a problem hiding this comment.
the ulimit -n 4 test seems overkill, could we drop it?
|
|
||
| #[test] | ||
| #[cfg(not(windows))] | ||
| fn cmp_quiet_size_shortcut_variants() -> Result<(), Box<dyn std::error::Error>> { |
There was a problem hiding this comment.
almost the same as cmp_fast_paths, could this be merged into it?
3b39012 to
906e801
Compare
|
Thanks for the review. I’ve addressed all five comments.
I also excluded |
|
|
||
| pub fn cmp(params: &Params) -> Result<Cmp, String> { | ||
| // With -n 0 a directory operand must still be read, so that it is reported. | ||
| let reads_directory = match params.max_bytes { |
There was a problem hiding this comment.
this adds a lot of code just for -n 0 with a directory.
could we check the limit right after the two fill_buf calls (before the EOF checks) instead?
both operands would still be read once, so the directory error comes for free, and is_directory / stop_at_limit could go away
There was a problem hiding this comment.
Done. The limit is now checked after the two fill_buf() calls, and is_directory / stop_at_limit are gone still kept one small check at the top of the loop: once N bytes have been compared, waiting for another fill_buf() can block if the pipe is still open. For example, { printf abcde; sleep 3; } | cmp -n 5 - five waited three seconds without it
In quiet mode, when both operands are regular files and neither is `-`, subtract the skipped bytes from each size, stopping at zero, and then apply the `-n` limit. If the resulting lengths differ, report a difference without reading; otherwise compare the requested data. Do not use the shortcut for `-`, because metadata for that path would describe a file literally named `-`, not standard input. This fixes byte-limited comparisons and gives GNU-compatible status in the tested directory and /dev/zero cases. One difference remains: `cmp -s a dir` now returns 2, but quiet mode still omits GNU's "Is a directory" diagnostic. New tests cover byte limits, skipped bytes, devices, directories, and `-i 0`.
Check the limit after both operands have been read and before the EOF checks. This keeps -n 0 reporting a directory operand while allowing is_directory and stop_at_limit to be removed. Keep a check at the top of the loop after at least one byte has been compared. Without it, reaching the limit can trigger another read and block on a pipe that is still open. Add a regression test for that case.
906e801 to
eac3123
Compare
In quiet mode, when both operands are regular files and neither is
-, subtract the skipped bytes from each size, stopping at zero, and then apply the-nlimit. If the resulting lengths differ, report a difference without reading; otherwise compare the requested data.Do not use the shortcut for
-, because metadata for that path would describe a file literally named-, not standard input.This fixes byte-limited comparisons and gives GNU-compatible status in the tested directory and
/dev/zerocases. One difference remains:cmp -s a dirnow returns 2, but quiet mode still omits GNU'sIs a directorydiagnostic.New tests cover byte limits, skipped bytes, devices, directories, and
-i 0.Fixes #295