From: Jeff King <peff@peff.net>
Date: Tue, 14 Aug 2018 21:49:42 GMT
Subject: Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test
Message-ID: <20180814214942.GB667@sigill.intra.peff.net>
In-Reply-To: <20180814114721.25577-1-szeder.dev@gmail.com>

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.

Good catch, and nicely explained. The patch itself looks obviously
correct.

-Peff