Re: [PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure
- Date: Tue, 20 Nov 2018 12:25:38 +0100
- From: SZEDER Gábor <szeder.dev@xxxxxxxxx>
- Subject: Re: [PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure
On Mon, Nov 19, 2018 at 02:49:20PM -0500, Jeff King wrote:
> 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.
> 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).
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:
but then went travelling, and then the whole thing got forgotten.