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@xxxxxxxx>
> ---
> 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
