From: Jeff King <peff@peff.net>
Date: Tue, 18 Apr 2023 10:56:22 GMT
Subject: Re: [PATCH 08/10] t/t6501-freshen-objects.sh: prepare for `gc --cruft` by default
Message-ID: <20230418105622.GA516715@coredump.intra.peff.net>
In-Reply-To: <4ccc525c39dd23db1136681bc0ffd572db0ed2f2.1681764848.git.me@ttaylorr.com>

On Mon, Apr 17, 2023 at 04:54:36PM -0400, Taylor Blau wrote:

> We could run this test twice, once with `--cruft` and once with
> `--no-cruft`, but doing so is unnecessary, since the object rescuing and
> freshening behavior is already extensively tested via t5329.

That's doubtless true for the general case of freshening (after all,
that's the point of cruft packs). I do wonder about these "broken links"
cases:

> @@ -151,7 +151,7 @@ test_expect_success 'do not complain about existing broken links (commit)' '
>  	some message
>  	EOF
>  	commit=$(git hash-object -t commit -w broken-commit) &&
> -	git gc -q 2>stderr &&
> +	git gc --no-cruft -q 2>stderr &&
>  	verbose git cat-file -e $commit &&
>  	test_must_be_empty stderr
>  '

The idea is that we don't complain when repacking unreachable-but-broken
segments of history. Which could perhaps behave differently for objects
that are going into a cruft pack versus being turned loose. So maybe
it's worth covering for the --cruft case, too. I dunno.

Certainly your patch is not making the test coverage worse, but it might
be highlighting an existing blind-spot (and one that will become the
default behavior in the next patch).

-Peff