Re: [PATCH 07/12] test-lib-functions: move function to lib-bitmap.sh

From: "SZEDER Gábor" <szeder.dev@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Jiang Xin <zhiyou.jx@alibaba-inc.com>, Jeff King <peff@peff.net>
Subject: Re: [PATCH 07/12] test-lib-functions: move function to lib-bitmap.sh
Date: Wed, 10 Feb 2021 21:56:45 +0100	[thread overview]
Message-ID: <20210210205645.GE1015009@szeder.dev> (raw)
In-Reply-To: <20210209214159.22815-8-avarab@gmail.com>

On Tue, Feb 09, 2021 at 10:41:54PM +0100, Ævar Arnfjörð Bjarmason wrote:
> Move a function added to test-lib-functions.sh in ea047a8eb4f (t5310:
> factor out bitmap traversal comparison, 2020-02-14) into a new
> lib-bitmap.sh.
> 
> The test-lib-functions.sh file should be for functions that are widely
> used across the test suite, if something's only used by a few tests it
> makes more sense to have it in a lib-*.sh file.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/lib-bitmap.sh                    | 26 ++++++++++++++++++++++++++
>  t/t5310-pack-bitmaps.sh            |  2 ++
>  t/t6113-rev-list-bitmap-filters.sh |  1 +
>  t/test-lib-functions.sh            | 27 ---------------------------
>  4 files changed, 29 insertions(+), 27 deletions(-)
>  create mode 100644 t/lib-bitmap.sh
> 
> diff --git a/t/lib-bitmap.sh b/t/lib-bitmap.sh
> new file mode 100644
> index 00000000000..fe3f98be24f
> --- /dev/null
> +++ b/t/lib-bitmap.sh
> @@ -0,0 +1,26 @@
> +# 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"
> +	then
> +		echo >&2 "identical raw outputs; are you sure bitmaps were used?"
> +		return 1

Since you converted two 'error's to BUG earlier in this series...

This helper function was added in ea047a8eb4 (t5310: factor out bitmap
traversal comparison, 2020-02-14), and so I Cc: its author and quote
part of its commit message:

    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).

Shouldn't this be a BUG as well?  I think this should be a BUG.

> +	fi &&
> +	cut -d' ' -f1 "$1" | sort >"$1.normalized" &&
> +	sort "$2" >"$2.normalized" &&
> +	test_cmp "$1.normalized" "$2.normalized" &&
> +	rm -f "$1.normalized" "$2.normalized"
> +}
> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> index 5ba76031090..40b9f632441 100755
> --- a/t/t5310-pack-bitmaps.sh
> +++ b/t/t5310-pack-bitmaps.sh
> @@ -5,6 +5,8 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master
>  export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>  
>  . ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-bundle.sh
> +. "$TEST_DIRECTORY"/lib-bitmap.sh
>  
>  objpath () {
>  	echo ".git/objects/$(echo "$1" | sed -e 's|\(..\)|\1/|')"
> diff --git a/t/t6113-rev-list-bitmap-filters.sh b/t/t6113-rev-list-bitmap-filters.sh
> index 2b551e6fd0c..3f889949ca1 100755
> --- a/t/t6113-rev-list-bitmap-filters.sh
> +++ b/t/t6113-rev-list-bitmap-filters.sh
> @@ -2,6 +2,7 @@
>  
>  test_description='rev-list combining bitmaps and filters'
>  . ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-bitmap.sh
>  
>  test_expect_success 'set up bitmapped repo' '
>  	# one commit will have bitmaps, the other will not
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 55d58b4a6ac..f228647c2b8 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1602,33 +1602,6 @@ test_set_port () {
>  	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"
> -	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"
> -}
> -
>  # Tests for the hidden file attribute on Windows
>  test_path_is_hidden () {
>  	test_have_prereq MINGW ||
> -- 
> 2.30.0.284.gd98b1dd5eaa7
> 