Re: [PATCH v2 08/15] t5310: factor out bitmap traversal comparison
- From
Jeff King <peff@peff.net>
- Date
- Feb 15, 2020, 07:00 UTC
- Message-ID
- <20200215070054.GD1633703@coredump.intra.peff.net>
- In-Reply-To
- <20200215021446.GA16515@syl.local>
On Fri, Feb 14, 2020 at 06:14:46PM -0800, Taylor Blau wrote:
Show 8 quoted lines
> > 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.
Show 10 quoted lines
> > +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