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

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Nov 20, 2018, 11:25 UTC
Message-ID: <20181120112538.GX30222@szeder.dev>
In-Reply-To: <20181119194920.GB7330@sigill.intra.peff.net>

On Mon, Nov 19, 2018 at 02:49:20PM -0500, Jeff King wrote:

Show 36 quoted lines

> 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.

But then it adds two subshells to the common case right away... I'm not sure which is worse on Windows, where it matters the most.

Show 18 quoted lines

> 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),

Exactly.

> but we do not
> usually bother to do so for our helper functions (like test_cmp).

test_i18ngrep() does since your 03aa3783f2 (t: send verbose test-helper output to fd 4, 2018-02-22).

> I don't think it matters usually in practice, because nobody tries to
> capture the stderr of test_cmp, etc.

See 54ce2e9be9 (t9400-git-cvsserver-server: don't rely on the output of 'test_cmp', 2018-03-08) for a fun example, I remember it caused a spike in WTF/minutes :)

> I don't think it would ever hurt,
> though. Should we be doing that for all the others, too?

I think we should, and you agreed:

https://public-inbox.org/git/20180303065648.GA17312@sigill.intra.peff.net/

but then went travelling, and then the whole thing got forgotten.