Re: [PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure

[Jeff King <peff@peff.net>]

On Mon, Nov 19, 2018 at 02:28:18PM +0100, SZEDER Gábor wrote:

> The 'test_cmp_rev' helper is merely a wrapper around 'test_cmp'
> checking the output of two 'git rev-parse' commands, which means that
> its output on failure is not particularly informative, as it's
> basically two OIDs with a bit of extra clutter of the diff header, but
> without any indication of which two revisions have caused the failure:
> 
>   --- expect.rev  2018-11-17 14:02:11.569747033 +0000
>   +++ actual.rev  2018-11-17 14:02:11.569747033 +0000
>   @@ -1 +1 @@
>   -d79ce1670bdcb76e6d1da2ae095e890ccb326ae9
>   +139b20d8e6c5b496de61f033f642d0e3dbff528d
> 
> It also pollutes the test repo with these two intermediate files,
> though that doesn't seem to cause any complications in our current
> tests (meaning that I couldn't find any tests that have to work around
> the presence of these files by explicitly removing or ignoring them).
> 
> Enhance 'test_cmp_rev' to provide a more useful output on failure with
> less clutter:
> 
>   error: two revisions point to different objects:
>     'HEAD^': d79ce1670bdcb76e6d1da2ae095e890ccb326ae9
>     'extra': 139b20d8e6c5b496de61f033f642d0e3dbff528d
> 
> Doing so is more convenient when storing the OIDs outputted by 'git
> rev-parse' in a local variable each, which, as a bonus, won't pollute
> the repository with intermediate files.
> 
> While at it, also ensure that 'test_cmp_rev' is invoked with the right
> number of parameters, namely two.

This is an improvement, in my opinion (and I agree that using your new
BUG for this last part would be better still). It also saves a process
in the common case.

One question:

> +	else
> +		local r1 r2
> +		r1=$(git rev-parse --verify "$1") &&
> +		r2=$(git rev-parse --verify "$2") &&
> +		if test "$r1" != "$r2"
> +		then
> +			cat >&4 <<-EOF
> +			error: two revisions point to different objects:
> +			  '$1': $r1
> +			  '$2': $r2
> +			EOF
> +			return 1
> +		fi

Why does this cat go to descriptor 4? I get why you'd want it to (it's
meant for the user's eyes, and that's where 4 goes), but we do not
usually bother to do so for our helper functions (like test_cmp).

I don't think it matters usually in practice, because nobody tries to
capture the stderr of test_cmp, etc. I don't think it would ever hurt,
though. Should we be doing that for all the others, too?

-Peff