On Tue, Jun 14, 2022 at 04:20:05PM -0400, Derrick Stolee wrote: > > 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. > > > 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
