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

From: Taylor Blau <me@ttaylorr.com>
Date: Jul 19, 2021, 21:31 UTC
Message-ID: <YPXvQXOcdKdCzcFb@nand.local>
In-Reply-To: <patch-2.2-c7315f2b378-20210709T101015Z-avarab@gmail.com>

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.

Show 5 quoted lines

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

Show 6 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).

Thanks, Taylor