Re: [PATCH 07/12] test-lib-functions: move function to lib-bitmap.sh

From: "SZEDER Gábor" <szeder.dev@gmail.com>
To: Jeff King <peff@peff.net>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	git@vger.kernel.org, "Junio C Hamano" <gitster@pobox.com>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Jiang Xin" <zhiyou.jx@alibaba-inc.com>
Subject: Re: [PATCH 07/12] test-lib-functions: move function to lib-bitmap.sh
Date: Thu, 11 Feb 2021 20:38:20 +0100	[thread overview]
Message-ID: <20210211193820.GH1015009@szeder.dev> (raw)
In-Reply-To: <YCRL4lq5eQnPLwK1@coredump.intra.peff.net>

On Wed, Feb 10, 2021 at 04:10:58PM -0500, Jeff King wrote:
> On Wed, Feb 10, 2021 at 09:56:45PM +0100, SZEDER Gábor wrote:
> 
> > > +test_bitmap_traversal () {
> > > +	if test "$1" = "--no-confirm-bitmaps"
> > > +	then
> > > +		shift
> > > +	elif cmp "$1" "$2"
> > > +	then
> > > +		echo >&2 "identical raw outputs; are you sure bitmaps were used?"
> > > +		return 1
> > 
> > Since you converted two 'error's to BUG earlier in this series...
> > 
> > This helper function was added in ea047a8eb4 (t5310: factor out bitmap
> > traversal comparison, 2020-02-14), and so I Cc: its author and quote
> > part of its commit message:
> > 
> >     While we're at it, let's also try to confirm that the bitmap output did
> >     indeed use bitmaps. Since the code internally falls back to the
> >     non-bitmap path in some cases, the tests are at risk of becoming trivial
> >     noops.
> >     
> >     This is a bit fragile, as not all outputs will differ (e.g., looking at
> >     only the commits from a fully-bitmapped pack will end up exactly the
> >     same as the normal traversal order, since it also matches the pack
> >     order). So we'll provide an escape hatch by which tests can disable this
> >     check (which should only be used after manually confirming that bitmaps
> >     kicked in).
> > 
> > Shouldn't this be a BUG as well?  I think this should be a BUG.
> 
> I dunno. I guess you are thinking of the case where somebody adds a new
> caller but fails to invoke Git correctly. Which would be a bug in the
> test script.
> 
> But it could also easily be due to Git changing how it behaves in a
> certain circumstance. And maybe the solution is changing the test to
> adapt, or maybe we just found a bug in Git.

Well, I though that if the raw output is the same with and without
pack bitmaps, then the missing '--no-confirm-bitmaps' option is the
bug in the test script.

> TBH, I do not care that much either way. Either way, I think somebody
> would notice the problem.

Me neither, I just don't like test helper functions that simply return
in the middle, becauase that makes suppressing their trace output even
more awkward than it already is :)