cmp: honor the byte limit in quiet size checks - #296
SichenLiang wants to merge 1 commit 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?
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`.
3b39012 to
906e801
Compare
|
Thanks for the review. I’ve addressed all five comments.
I also excluded |
2 participants
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