Re: [PATCH v2 2/2] pack-objects: fix segfault in --stdin-packs option

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 20, 2021, 11:55 UTC
Message-ID: <87zguhum6y.fsf@evledraar.gmail.com>
In-Reply-To: <YPXvQXOcdKdCzcFb@nand.local>

On Mon, Jul 19 2021, Taylor Blau wrote:

Show 16 quoted lines

> On Fri, Jul 09, 2021 at 12:13:48PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> Thanks for the update, and sorry that it took me so long to get to. I
> see that this still hasn't quite made its way to 'next', so I'll just
> add one comment.
>
>> +test_expect_success 'pack-object <stdin parsing: --stdin-packs handles garbage' '
>> +	cat >in <<-EOF &&
>> +	$(git -C pack-object-stdin rev-parse one)
>> +	$(git -C pack-object-stdin rev-parse two)
>> +	EOF
>
> I see that you left my suggestion to inline this here-doc with the
> actual 'pack-objects' invocation below alone, which is fine. I think
> that it does help the readability, too, since it separates the input
> from the command its being fed to.

Yeah, per CL:
    
    I didn't end up moving away from the "<in" pattern. I prefer it
    because it makes manual inspection easier, and the tests above this
    one used it consistently, so I left it in place.

Show 21 quoted lines

>> +	# That we get "two" and not "one" has to do with OID
>> +	# ordering. It happens to be the same here under SHA-1 and
>> +	# SHA-256. See commentary in pack-objects.c
>> +	cat >err.expect <<-EOF &&
>> +	fatal: could not find pack '"'"'$(git -C pack-object-stdin rev-parse two)'"'"'
>> +	EOF
>
> On the other hand, crafting this err.expect with one of the object's
> full OID still sits funny with me. I appreciate you checking that this
> is the correct object to test with in SHA-1 and SHA-256 mode, but isn't
> the point that we shouldn't be relying on which object comes out?
>
> I think that dropping this down to just something like:
>
>     grep 'could not find' err.actual
>
> would be an improvement since it avoids the finicky shell quoting,
> hardens this test in the event of a future change in hashing algorithm,
> and brings the test more in line with the spirit of the patch itself
> (which is to report some of its input, not necessarily the first one
> given).

If we've got another hash transition (unlikely, at least near-ish term) we can just look at this test again.

More plausibly it's a common pattern in our test suite that greps like that elide actual problems, e.g. a loop printing the error N times, that seems more likely in this case.

Do you mind if it's just left as it is?