git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

From
Kirill Smelkov <kirr@nexedi.com>
Date
Aug 27, 2018, 10:22 UTC
Message-ID
<20180827102238.GA26738@deco.navytux.spb.ru>
In-Reply-To
<20180814114721.25577-1-szeder.dev@gmail.com>
On Tue, Aug 14, 2018 at 01:47:21PM +0200, SZEDER Gábor wrote:
Show 59 quoted lines
> 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@gmail.com>
> ---
>  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
Previous: Matthew DeVoreNext: Jeff King
Message 15 of 17 in “t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test”
  1. t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' testSZEDER Gábor, Aug 14, 2018
  2. Jeff KingAug 14, 2018
  3. Andrei RybakAug 16, 2018
  4. Junio C HamanoAug 16, 2018
  5. SZEDER GáborAug 17, 2018
  6. Andrei RybakAug 17, 2018
  7. Junio C HamanoAug 17, 2018
  8. Andrei RybakAug 19, 2018
  9. Jeff KingAug 19, 2018
  10. Andrei RybakAug 19, 2018
  11. Jeff KingAug 19, 2018
  12. Junio C HamanoAug 21, 2018
  13. SZEDER GáborAug 17, 2018
  14. Matthew DeVoreAug 22, 2018
  15. Kirill SmelkovAug 27, 2018
  16. Jeff KingAug 27, 2018
  17. Kirill SmelkovAug 28, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.