Re: [PATCH v2 08/15] t5310: factor out bitmap traversal comparison

From: Jeff King <peff@peff.net>
To: Taylor Blau <me@ttaylorr.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v2 08/15] t5310: factor out bitmap traversal comparison
Date: Sat, 15 Feb 2020 02:00:54 -0500	[thread overview]
Message-ID: <20200215070054.GD1633703@coredump.intra.peff.net> (raw)
In-Reply-To: <20200215021446.GA16515@syl.local>

On Fri, Feb 14, 2020 at 06:14:46PM -0800, Taylor Blau wrote:

> > 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).
> 
> Thanks for pointing out the rationale behind this.

I didn't write it this way at first, but the need became quite apparent
when I ran the test from the next patch. :)

I waffled on whether the extra option made the helper too convoluted to
be worthwhile.  Another way to do it would be two separate functions:

  test_for_bitmap_output expect actual &&
  test_bitmap_output_matches expect actual

but as you can see I couldn't come up with good names.

I doubt it matters all that much either way, as long as the docstring is
clear.

> > +test_bitmap_traversal () {
> > +	if test "$1" = "--no-confirm-bitmaps"
> > +	then
> > +		shift
> > +	elif cmp "$1" "$2"
> 
> Any reason to prefer 'cmp' here and then use 'test_cmp' below? I can't
> think of a good reason one way or another, especially in places where
> GIT_TEST_CMP is just 'cmp', but I think the two usages should be
> consistent with one another.

On most platforms GIT_TEST_CMP is "diff -u" (it's only on platforms with
a crappy system diff tool that we replace it with "cmp"). So I thought
it would be confusing for it to dump a diff in the expected case (since
unlike a normal test_cmp, we want the outputs to differ).  "cmp" does
still write a short message to stderr; I thought about redirecting it to
/dev/null, but worried that would make debugging verbose output harder).

> If there *is* a favorable argument for 'test_cmp' (instead of a bare
> 'cmp'), I think that it'd be that the original code a couple of hunks up
> uses it.

Right, but it uses to check that two things are equal (which we also use
it for here).

-Peff