Re: [PATCH] t1002: stop using sum(1)

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

René Scharfe wrote:

> sum(1) is a command for calculating checksums of the contents of files.
> It was part of early editions of Unix ("Research Unix", 1972/1973, [1]).
> cksum(1) appeared in 4.4BSD (1993) as a replacement [2], and became part
> of POSIX.1-2008 [3].  OpenBSD 5.6 (2014) removed sum(1).
>
> We only use sum(1) in t1002 to check for changes in three files.  On
> MinGW we use md5sum(1) instead.  We could switch to the standard command
> cksum(1) for all platforms; MinGW comes with GNU coreutils now, which
> provides sum(1), cksum(1) and md5sum(1).  Use our standard method for
> checking for file changes instead: test_cmp.
>
> It's more convenient because it shows differences nicely, it's faster on
> MinGW because we have a special implementation there based only on
> shell-internal commands, it's simpler as it allows us to avoid stripping
> out unnecessary entries from the checksum file using grep(1), and it's
> more consistent with the rest of the test suite.
>
> We already compare changed files with their expected new contents using
> diff(1), so we don't need to check with "test_must_fail test_cmp" if
> they differ from their original state.  A later patch could convert the
> direct diff(1) calls to test_cmp as well.
>
> With all sum(1) calls gone, remove the MinGW-specific implementation
> from test-lib.sh as well.
>
> [1] http://minnie.tuhs.org/cgi-bin/utree.pl?file=V3/man/man1/sum.1
> [2] http://minnie.tuhs.org/cgi-bin/utree.pl?file=4.4BSD/usr/share/man/cat1/cksum.0
> [3] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/cksum.html
> ---
>  t/t1002-read-tree-m-u-2way.sh | 67 ++++++++++++++++++++++---------------------
>  t/test-lib.sh                 |  3 --
>  2 files changed, 35 insertions(+), 35 deletions(-)

Nicely analyzed.  May we forge your sign-off?

[...]
> --- a/t/t1002-read-tree-m-u-2way.sh
> +++ b/t/t1002-read-tree-m-u-2way.sh
[...]
> @@ -132,8 +138,8 @@ test_expect_success \
>       git ls-files --stage >7.out &&
>       test_cmp M.out 7.out &&
>       check_cache_at frotz dirty &&
> -     sum bozbar frotz nitfol >actual7.sum &&
> -     if cmp M.sum actual7.sum; then false; else :; fi &&
> +     test_cmp bozbar.M bozbar &&
> +     test_cmp nitfol.M nitfol &&

This one is strange.  What is that '! cmp' trying to check for?
Does the replacement capture the same thing?

E.g., does it need a '! test_cmp frotz.M frotz &&' line?

I haven't looked at the context closely --- another option could be a
note in the commit message about how that '! cmp' line was not testing
anything useful in the first place.

[...]
> @@ -209,11 +217,8 @@ test_expect_success \
>       git ls-files --stage >14.out &&
>       test_must_fail git diff -U0 --no-index M.out 14.out >14diff.out &&
>       compare_change 14diff.out expected &&
> -     sum bozbar frotz >actual14.sum &&
> -     grep -v nitfol M.sum > expected14.sum &&
> -     cmp expected14.sum actual14.sum &&
> -     sum bozbar frotz nitfol >actual14a.sum &&
> -     if cmp M.sum actual14a.sum; then false; else :; fi &&
> +     test_cmp bozbar.M bozbar &&
> +     test_cmp frotz.M frotz &&

Same question here: the preimage seems to be a stricter test than the
postimage.

[...]
> @@ -231,11 +236,8 @@ test_expect_success \
>       test_must_fail git diff -U0 --no-index M.out 15.out >15diff.out &&
>       compare_change 15diff.out expected &&
>       check_cache_at nitfol dirty &&
> -     sum bozbar frotz >actual15.sum &&
> -     grep -v nitfol M.sum > expected15.sum &&
> -     cmp expected15.sum actual15.sum &&
> -     sum bozbar frotz nitfol >actual15a.sum &&
> -     if cmp M.sum actual15a.sum; then false; else :; fi &&
> +     test_cmp bozbar.M bozbar &&
> +     test_cmp frotz.M frotz &&

Likewise.

[...]
> @@ -281,11 +285,8 @@ test_expect_success \
>       git ls-files --stage >19.out &&
>       test_cmp M.out 19.out &&
>       check_cache_at bozbar dirty &&
> -     sum frotz nitfol >actual19.sum &&
> -     grep -v bozbar  M.sum > expected19.sum &&
> -     cmp expected19.sum actual19.sum &&
> -     sum bozbar frotz nitfol >actual19a.sum &&
> -     if cmp M.sum actual19a.sum; then false; else :; fi &&
> +     test_cmp frotz.M frotz &&
> +     test_cmp nitfol.M nitfol &&

Likewise.

The rest looks good.

Thanks,
Jonathan