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

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:

quoted

quoted

+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 :)