GeezerVison

Codecov Report

Attention: Patch coverage is 96.01050% with 76 lines in your changes missing coverage. Please review.

Project coverage is 84.95%. Comparing base (9103365) to head (fac8dab).
Report is 11 commits behind head on main.

@@            Coverage Diff             @@
##             main      #88      +/-   ##
==========================================
+ Coverage   81.01%   84.95%   +3.94%     
==========================================
  Files          10       12       +2     
  Lines        4245     5824    +1579     
  Branches      397      480      +83     
==========================================
+ Hits         3439     4948    +1509     
- Misses        806      856      +50     
- Partials        0       20      +20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

could you please run your benchmarks with hyperfine? time is too limited

could you please run your benchmarks with hyperfine? time is too limited

Will do! Have begun working on fuzzing support, fwiw, need to do some refactoring as I currently use process::exit() as a shortcut in a lot of places.

Added fuzz implementation to the first utility commit, ran it overnight with no issues spotted. Replaced my hand crafted benchmarks with hyperfine output (and made sure to have the system free of memory pressure / cpu interference).

I have tried to run the GNU tests, but the script fails here. I only changed it to also symlink diffutils to cmp and to run the cmp tests. Can anyone spot anything obvious before I start diving in to investigate?

kov@jabuticaba ~/P/diffutils (cmp)> ./tests/run-upstream-testsuite.sh
Fetching upstream test suite from https://git.savannah.gnu.org/git/diffutils.git
Running 31 tests
./tests/run-upstream-testsuite.sh: line 96: cd: gt-basic.*: No such file or directory
  basic                                    FAIL
./tests/run-upstream-testsuite.sh: line 96: cd: gt-bignum.*: No such file or directory
  bignum                                   FAIL
./tests/run-upstream-testsuite.sh: line 96: cd: gt-binary.*: No such file or directory
  binary                                   FAIL
./tests/run-upstream-testsuite.sh: line 96: cd: gt-brief-vs-stat-zero-kernel-lies.*: No such file or directory
  brief-vs-stat-zero-kernel-lies           FAIL
...

diff --git a/tests/run-upstream-testsuite.sh b/tests/run-upstream-testsuite.sh
index cb59834..f75b0b3 100755
--- a/tests/run-upstream-testsuite.sh
+++ b/tests/run-upstream-testsuite.sh
@@ -21,7 +21,7 @@
 # (e.g. 'dev' or 'test').
 # Unless overridden by the $TESTS environment variable, all tests in the test
 # suite will be run. Tests targeting a command that is not yet implemented
-# (e.g. cmp, diff3 or sdiff) are skipped.
+# (e.g. diff3 or sdiff) are skipped.
 
 scriptpath=$(dirname "$(readlink -f "$0")")
 rev=$(git rev-parse HEAD)
@@ -57,6 +57,7 @@ upstreamrev=$(git rev-parse HEAD)
 mkdir src
 cd src
 ln -s "$binary" diff
+ln -s "$binary" cmp
 cd ../tests
 
 if [[ -n "$TESTS" ]]
@@ -82,9 +83,9 @@ for test in $tests
 do
   result="FAIL"
   url="$urlroot$test?id=$upstreamrev"
-  # Run only the tests that invoke `diff`,
+  # Run only the tests that invoke `diff` or `cmp`,
   # because other binaries aren't implemented yet
-  if ! grep -E -s -q "(cmp|diff3|sdiff)" "$test"
+  if ! grep -E -s -q "(diff3|sdiff)" "$test"
   then
     sh "$test" 1> stdout.txt 2> stderr.txt && result="PASS" || exitcode=1
     json+="{\"test\":\"$test\",\"result\":\"$result\","

clippy is reporting 9 trivial errors, could you address them to get the CI results green?

I have tried to run the GNU tests, but the script fails here. I only changed it to also symlink diffutils to cmp and to run the cmp tests. Can anyone spot anything obvious before I start diving in to investigate?

This is a regression that also affects main, so not introduced by your changes. I've filed #90 and am investigating it.

I have tried to run the GNU tests, but the script fails here. I only changed it to also symlink diffutils to cmp and to run the cmp tests. Can anyone spot anything obvious before I start diving in to investigate?

This is a regression that also affects main, so not introduced by your changes. I've filed #90 and am investigating it.

With the fix in #91, I ran the test suite on your branch, and I'm seeing the following differences:

@@ -3,9 +3,9 @@
   basic                                    PASS
   bignum                                   PASS
   binary                                   FAIL
-  brief-vs-stat-zero-kernel-lies           SKIP
+  brief-vs-stat-zero-kernel-lies           FAIL
   bug-64316                                PASS
-  cmp                                      SKIP
+  cmp                                      FAIL
   colliding-file-names                     FAIL
   diff3                                    SKIP
   excess-slash                             FAIL
@@ -30,9 +30,9 @@
   strip-trailing-cr                        FAIL
   timezone                                 PASS
   colors                                   FAIL
-  y2038-vs-32bit                           SKIP
+  y2038-vs-32bit                           PASS
 
-Summary: TOTAL: 31 / PASS: 6 / FAIL: 20 / SKIP: 5
+Summary: TOTAL: 31 / PASS: 7 / FAIL: 22 / SKIP: 2
 
 Results written to /home/osomon/build/uutils/diffutils/tests/test-results.json

The failure in the cmp test needs to be investigated for sure, but this looks promising.

Fixed the clippy complaints and quite close to getting the cmp test to pass locally. I am a bit confused, though, as my system's GNU cmp also doesn't pass the tests. I think the test infrastructure may not be sanitizing locale environments properly or there is something else on my Fedora it is not liking.

I got the failures on our tool down to a minimum, but still hit these, which the GNU cmp built from git also hits. Wonder if there is a weird corner case in my system messing things up:

kov@jabuticaba ~/P/d/t/d/tests (master)> which cmp
/home/kov/.local/bin/cmp
kov@jabuticaba ~/P/d/t/d/tests (master)> ls -lh /home/kov/.local/bin/cmp
lrwxrwxrwx. 1 kov kov 51 set 26 22:11 /home/kov/.local/bin/cmp -> /home/kov/Projects/diffutils/target/debug/diffutils*
kov@jabuticaba ~/P/d/t/d/tests (master) [1]> cargo build; and env LANG=C ./cmp 
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.02s
LC_ALL=C cmp -b bad bug
bad bug differ: byte 2, line 1 is 141 a 165 u
cmp: invalid --ignore-initial value '99999999999999999999999999999999999999999999999999999999999'
cmp: invalid --bytes value '99999999999999999999999999999999999999999999999999999999999'
kov@jabuticaba ~/P/d/t/d/tests (master)> ln -fs ~/Projects/diffutils/t/cmp ~/.local/bin/cmp # this is GNU cmp built from the tree
kov@jabuticaba ~/P/d/t/d/tests (master)> env LC_ALL=C ./cmp
LC_ALL=C cmp -b bad bug
bad bug differ: byte 2, line 1 is 141 a 165 u
cmp: invalid --ignore-initial value '99999999999999999999999999999999999999999999999999999999999'
cmp: Try 'cmp --help' for more information.
cmp: invalid --ignore-initial value '1000'
cmp: Try 'cmp --help' for more information.

I checked with strace that the cmp being run was appropriate fwiw. The last error message from GNU cmp is a real wth xD

I checked with strace that the cmp being run was appropriate fwiw. The last error message from GNU cmp is a real wth xD

I ran LANG=C ./cmp from the tests directory in a local checkout of the upstream repository, and I'm not seeing the "invalid --ignore-initial value" error that you're observing. I do see it when using cmp built from your branch.

Note that the cmp test script prepends ../src to $PATH. Could it be that you had symlinked the wrong executable in there?
I'm seeing a different behaviour when I run that same test script against my system-wide /usr/bin/cmp installed from distro packages (using Ubuntu 24.04).

The following upstream commits seem relevant: 4ee8300 and 9c5fcbd (and they were added after the last upstream release).

Comparing further the output of the cmp test script, it looks like your implementation conforms to the released version of GNU cmp (version 3.10), i.e. it chokes on large numbers passed to -i/--ignore-initial.

We have two options here:

• align your implementation with the latest unreleased changes in the GNU diffutils repository
• change tests/run-upstream-testsuite.sh to checkout a released version of the test suite, not the latest HEAD

I think that both approaches are equally interesting and valid (one favours the bleeding edge, the other compatibility with released versions), so feel free to pick whichever you prefer (I can help with updating the test script if you decide to go that way).

Getting there, I went with updating the implementation to allow huge numbers - forward looking probably makes more sense =)

I just need to investigate this one now:

• brief-vs-stat-zero-kernel-lies SKIP

• brief-vs-stat-zero-kernel-lies FAIL

Will hopefully get to it this evening. Thanks for the help so far!

I just need to investigate this one now:

* brief-vs-stat-zero-kernel-lies           SKIP
* brief-vs-stat-zero-kernel-lies           FAIL

I looked into it, and it turns out it's an issue in the test suite runner, not in your code. I filed #92 to track it.

I created another fuzz target for parameter parsing and left it running overnight with no issues, do you think having this target is also useful? I think the hand-crafted argument parsing is more likely to go wrong, now that I think about it.

cat fuzz/fuzz_targets/fuzz_cmp_args.rs
#![no_main]
#[macro_use]
extern crate libfuzzer_sys;
use diffutilslib::cmp;

use std::ffi::OsString;

fuzz_target!(|x: Vec<OsString>| {
    let _ = cmp::parse_params(x.into_iter().peekable());
});

Any thoughts on how we could deny corpus entries to make it more useful? I thought about denying certain sizes if they do not have any -x or --whatever, or at least one of the known parameters. Like, if x.len() > 4 it needs to have some known parameters, but that excludes having over 4 positional parameters quite often, I suppose. Could be worth the tradeoff, still.

I created another fuzz target for parameter parsing and left it running overnight with no issues, do you think having this target is also useful? I think the hand-crafted argument parsing is more likely to go wrong, now that I think about it.

Yes, I think it is definitely useful. For the record, we implement argument parsing by hand because ready-made argument parsers like clap do not offer the flexibility we need to replicate the GNU diffutils arguments. We'd lose in compatibility what we would gain in code simplicity.

Any thoughts on how we could deny corpus entries to make it more useful? I thought about denying certain sizes if they do not have any -x or --whatever, or at least one of the known parameters. Like, if x.len() > 4 it needs to have some known parameters, but that excludes having over 4 positional parameters quite often, I suppose. Could be worth the tradeoff, still.

I don't have prior experience with this, but perhaps we could use a dictionary?

One additional thought: I see that you wrote integration tests for the new cmp command. Those are good and they provide a decent code coverage, but I would also suggest writing unit tests for parse_params(…) trying to bump the coverage as much as possible. With the combination of unit tests, integration tests and targeted fuzzing we should have our backs covered.

Added a dictionary, but ended up going with some Corpus rejection as well, as using the dictionary requires passing parameters to cargo fuzz, may be forgotten. The github action looks like it would need to be split per target if we need to pass dictionaries there - which makes me realize I never added the fuzz target there, will fix right now.

In addition to the param parsing I added yesterday, I also added a few more integration tests to cover more of the fast paths that had no coverage (some of them were covered by the GNU cmp tests, but good to have our own tests as well I suppose).

I may need to set up a build on a Windows VM xD