On Tue, Aug 14, 2018 at 01:47:21PM +0200, SZEDER Gábor wrote:
> The test 'pack-objects to file can use bitmap' added in 645c432d61
> (pack-objects: use reachability bitmap index when generating
> non-stdout pack, 2016-09-10) is silently buggy and doesn't check what
> it's supposed to.
>
> In 't5310-pack-bitmaps.sh', the 'list_packed_objects' helper function
> does what its name implies by running:
>
> git show-index <"$1" | cut -d' ' -f2
>
> The test in question invokes this function like this:
>
> list_packed_objects <packa-$packasha1.idx >packa.objects &&
> list_packed_objects <packb-$packbsha1.idx >packb.objects &&
> test_cmp packa.objects packb.objects
>
> Note how these two callsites don't specify the name of the pack index
> file as the function's parameter, but redirect the function's standard
> input from it. This triggers an error message from the shell, as it
> has no filename to redirect from in the function, but this error is
> ignored, because it happens upstream of a pipe. Consequently, both
> invocations produce empty 'pack{a,b}.objects' files, and the
> subsequent 'test_cmp' happily finds those two empty files identical.
>
> Fix these two 'list_packed_objects' invocations by specifying the pack
> index files as parameters. Furthermore, eliminate the pipe in that
> function by replacing it with an &&-chained pair of commands using an
> intermediate file, so a failure of 'git show-index' or the shell
> redirection will fail the test.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@xxxxxxxxx>
> ---
> t/t5310-pack-bitmaps.sh | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> index 6ee4d3f2d9..557bd0d0c0 100755
> --- a/t/t5310-pack-bitmaps.sh
> +++ b/t/t5310-pack-bitmaps.sh
> @@ -9,7 +9,8 @@ objpath () {
>
> # show objects present in pack ($1 should be associated *.idx)
> list_packed_objects () {
> - git show-index <"$1" | cut -d' ' -f2
> + git show-index <"$1" >object-list &&
> + cut -d' ' -f2 object-list
> }
>
> # has_any pattern-file content-file
> @@ -204,8 +205,8 @@ test_expect_success 'pack-objects to file can use bitmap' '
> # verify equivalent packs are generated with/without using bitmap index
> packasha1=$(git pack-objects --no-use-bitmap-index --all packa </dev/null) &&
> packbsha1=$(git pack-objects --use-bitmap-index --all packb </dev/null) &&
> - list_packed_objects <packa-$packasha1.idx >packa.objects &&
> - list_packed_objects <packb-$packbsha1.idx >packb.objects &&
> + list_packed_objects packa-$packasha1.idx >packa.objects &&
> + list_packed_objects packb-$packbsha1.idx >packb.objects &&
> test_cmp packa.objects packb.objects
> '
Thanks for catching and correcting this.
A minor comment from outside observer: running tests under something
like
-e and -o pipefail
would automatically catch the mistake in the first place. Maybe `-o
pipefail` is bashism (I had not checked), but `git grep " | " t/` shows
there are a lot of pipelines being used, and thus similar errors might be
silently resting there. Something like -o pipefail would catch all such
problems automatically.
Kirill
