Re: Git Test Coverage Report for v2.37.0-rc0

From: Taylor Blau <me@ttaylorr.com>
Date: Jun 14, 2022, 23:46 UTC
Message-ID: <Yqkd5GNYAKBnPQxz@nand.local>
In-Reply-To: <3d1c6dfd-1df6-3393-df5e-692719375772@github.com>

On Tue, Jun 14, 2022 at 04:20:05PM -0400, Derrick Stolee wrote:

Show 5 quoted lines

> > Taylor Blau	f9825d1c builtin/repack.c: support generating a cruft pack
> > builtin/repack.c
> > f9825d1c 680) strvec_pushf(&cmd.args, "--cruft-expiration=%s",
>
> The --cruft-expiration option is not tested.

Note that only the `--cruft-expiration` option of `git repack` is untested. This code is just passing down the expiration from `repack`'s perspective down to pack-objects, which has extensive coverage of the `--cruft-expiration` option in t5329.

It does feel a little weird to leave this uncovered, but I don't think it's substantially different from the uncovered lines in setting up `cruft_po_args` on lines 86-90 of the repack code.

> > f9825d1c 708) fprintf(in, "%s.pack\n", item->string);
>
> This is related to the existence of .keep packs. A corner case, but maybe
> worth exploring.

Probably worth testing. We're ensuring that `.keep` packs in addition to `--keep-pack`s are excluded from the cruft pack. That isn't tested and probably should be.

Show 8 quoted lines

>
> > pack-write.c
> > 5dfaf49a 330) unlink(mtimes_name);
> > 5dfaf49a 331) fd = xopen(mtimes_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
>
> This appears to be an interesting case for the write_mtimes_file() method,
> since its first parameter is 'mtimes_name' and all tested cases are
> passing NULL, it seems.

Ugh, yes :-). That's imitating a common pattern in the write_xyz_file() for xyz referring to auxiliary pack data. The cleanup should be pretty straightforward, I'll send a patch or two shortly...

Thanks, Taylor