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
