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

[Taylor Blau <me@ttaylorr.com>]

On Fri, Feb 14, 2020 at 01:22:25PM -0500, Jeff King wrote:
> We check the results of "rev-list --use-bitmap-index" by comparing it to
> the same traversal without the bitmap option. However, this is a little
> tricky to do because of some output differences (see the included
> comment for details). Let's pull this out into a helper function, since
> we'll be adding some similar tests.
>
> 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).

Thanks for pointing out the rationale behind this.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/t5310-pack-bitmaps.sh | 10 +++-------
>  t/test-lib-functions.sh | 27 +++++++++++++++++++++++++++
>  2 files changed, 30 insertions(+), 7 deletions(-)
>
> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> index 7ba7d294a5..b8645ae070 100755
> --- a/t/t5310-pack-bitmaps.sh
> +++ b/t/t5310-pack-bitmaps.sh
> @@ -81,13 +81,9 @@ rev_list_tests() {
>  	'
>
>  	test_expect_success "enumerate --objects ($state)" '
> -		git rev-list --objects --use-bitmap-index HEAD >tmp &&
> -		cut -d" " -f1 <tmp >tmp2 &&
> -		sort <tmp2 >actual &&
> -		git rev-list --objects HEAD >tmp &&
> -		cut -d" " -f1 <tmp >tmp2 &&
> -		sort <tmp2 >expect &&
> -		test_cmp expect actual
> +		git rev-list --objects --use-bitmap-index HEAD >actual &&
> +		git rev-list --objects HEAD >expect &&
> +		test_bitmap_traversal expect actual
>  	'
>
>  	test_expect_success "bitmap --objects handles non-commit objects ($state)" '
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 284c52d076..352c213d52 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1516,3 +1516,30 @@ test_set_port () {
>  	port=$(($port + ${GIT_TEST_STRESS_JOB_NR:-0}))
>  	eval $var=$port
>  }
> +
> +# Compare a file containing rev-list bitmap traversal output to its non-bitmap
> +# counterpart. You can't just use test_cmp for this, because the two produce
> +# subtly different output:
> +#
> +#   - regular output is in traversal order, whereas bitmap is split by type,
> +#     with non-packed objects at the end
> +#
> +#   - regular output has a space and the pathname appended to non-commit
> +#     objects; bitmap output omits this
> +#
> +# This function normalizes and compares the two. The second file should
> +# always be the bitmap output.
> +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.

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.

> +	then
> +		echo >&2 "identical raw outputs; are you sure bitmaps were used?"
> +		return 1
> +	fi &&
> +	cut -d' ' -f1 "$1" | sort >"$1.normalized" &&
> +	sort "$2" >"$2.normalized" &&
> +	test_cmp "$1.normalized" "$2.normalized" &&
> +	rm -f "$1.normalized" "$2.normalized"

This implementation looks good to me.

> +}
> --
> 2.25.0.796.gcc29325708

Thanks,
Taylor