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

On Tue, Jul 20, 2021 at 01:55:31PM +0200, Ævar Arnfjörð Bjarmason wrote:

On Mon, Jul 19 2021, Taylor Blau wrote:

quoted

On Fri, Jul 09, 2021 at 12:13:48PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

+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.

Ah, my apologies for reading right past it. Thanks!

quoted

quoted

+	# 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.

I understand the reasoning, but I'm not quite sure that I buy that that
is a likely defect here. Of course, that doesn't mean that stricter
tests aren't good for nothing, but it's a tradeoff.

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

No, I don't mind, but I do think that changing the test to be looser
would be an improvement. Of course, there are lots of unlikely/rare
things that could cause this test to break (like picking a different
hashing algorithm), but I think fundamentally this test disagrees in
spirit with the code.

I.e., it seems odd to me that the code says "well, we have to sort this
list before we can produce an error, so the error won't necessarily
correspond to your first line of input" but the test says "we should
check that exactly such-and-such an object is included in the error".

Obviously that is far from the worst outcome in the world, but it does
seem a little odd to me. In any case, I don't feel particularly strongly
about it, and (as I said much earlier in the thread) this probably is
all academic anyway, since I would expect the vast majority of uses of
'--stdin-packs' are from 'git repack --geometric' anyway.

Thanks,
Taylor