On Mon, Jul 19 2021, Taylor Blau wrote:
> 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.
>> + # 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?
