Re: [PATCH 2/4] t5329: test 'git gc --cruft' without '--prune=now'

From: Taylor Blau <me@ttaylorr.com>
Date: Jun 14, 2022, 23:38 UTC
Message-ID: <YqkcEhQX4qm7fSer@nand.local>
In-Reply-To: <f59a0c326a01ab5f4e415a7ed03ff046b84b337b.1655242070.git.gitgitgadget@gmail.com>

On Tue, Jun 14, 2022 at 09:27:48PM +0000, Derrick Stolee via GitGitGadget wrote:

Show 22 quoted lines

> From: Derrick Stolee <derrickstolee@github.com>
>
> Replace a 'git repack --cruft -d' with the wrapper 'git gc --cruft' to
> exercise some logic in builtin/gc.c that adds the '--cruft' option to
> the underlying 'git repack' command.
>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
>  t/t5329-pack-objects-cruft.sh | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh
> index b481224b93d..55bb9c0e7b5 100755
> --- a/t/t5329-pack-objects-cruft.sh
> +++ b/t/t5329-pack-objects-cruft.sh
> @@ -451,11 +451,13 @@ test_expect_success 'expiring cruft objects with git gc' '
>  		sort <reachable.raw >reachable &&
>  		comm -13 reachable objects >unreachable &&
>
> -		git repack --cruft -d &&
> +		# Write a cruft pack instead of deleting files.
> +		git gc --cruft &&

These ("git repack --cruft -d" and "git gc --cruft") do the same thing, so this transformation makes sense.

It may be slightly clearer to refer to "objects" instead of "files", perhaps like:

    # Write a cruft pack containing all unreachable objects

and then replace:

> +		# Ignore the cruft pack and delete every unreachable object.
>  		git gc --cruft --prune=now &&

with:

    # Prune all unreachable objects from the cruft pack

But I don't think the current wording is a problem, either, so feel free to take or leave these suggestions.

Quoting from your original coverage report, this should take care of:

Taylor Blau 5b92477f builtin/gc.c: conditionally avoid pruning objects via loose builtin/gc.c 5b92477f 337) strvec_push(&repack, "--cruft"); 5b92477f 338) if (prune_expire) 5b92477f 339) strvec_pushf(&repack, "--cruft-expiration=%s", prune_expire);

so this patch looks good to me.

Thanks, Taylor