Re: [PATCH 2/2] builtin/repack: fix `--keep-unreachable` when there are no packs

[Patrick Steinhardt <ps@pks.im>]

On Mon, Feb 03, 2025 at 10:01:57PM -0500, Jeff King wrote:
> On Mon, Feb 03, 2025 at 02:06:55PM +0100, Patrick Steinhardt wrote:
> > Fix the bug by splitting out the "--pack-loose-unreachable" and only
> > making it depend on the second and third condition. Like this, loose
> > unreachable objects will be packed regardless of any preexisting
> > packfiles.
> 
> Makes sense. My only question would be whether there are any gotchas
> inside pack-objects about using --pack-loose-unreachable without
> --keep-unreachable (since the two were up until now always used
> together).
> 
> It was added by e26a8c4721. And looking over that patch, I don't see
> anything that would let the options be used independently. So this seems
> like a good solution.

You probably meant "I don't see anything that would *not* let the
options be used independently." But yeah, they don't seem to require one
another.

> > diff --git a/builtin/repack.c b/builtin/repack.c
> > index 81d13630ea..8194344b04 100644
> > --- a/builtin/repack.c
> > +++ b/builtin/repack.c
> > @@ -1370,9 +1370,12 @@ int cmd_repack(int argc,
> >  					    "--unpack-unreachable");
> >  			} else if (keep_unreachable) {
> >  				strvec_push(&cmd.args, "--keep-unreachable");
> > -				strvec_push(&cmd.args, "--pack-loose-unreachable");
> >  			}
> >  		}
> > +
> > +		if (keep_unreachable && delete_redundant &&
> > +		    !(pack_everything & PACK_CRUFT))
> > +			strvec_push(&cmd.args, "--pack-loose-unreachable");
> 
> One funny thing here is that previously unpack_unreachable took
> precedence over keep_unreachable in the if-else chain. I wondered if we
> could end up invoking pack-objects with both --unpack-unreachable and
> --pack-loose-unreachable, which is nonsense.
> 
> But I think the answer is no, because we forbid --unpack-unreachable/-A
> and --keep-unreachable from both being passed up front.

Yup.

> > -test_expect_failure '--keep-unreachable packs unreachable loose object without existing packs' '
> > +test_expect_success '--keep-unreachable packs unreachable loose object without existing packs' '
> >  	test_when_finished "rm -rf repo" &&
> >  	git init repo &&
> >  	(
> 
> Your test from patch 1 looked reasonable to me. If you fold patch 1 into
> the existing tests in t7701, you might want to adjust it to match the
> techniques those tests use for checking the object (rather than the new
> helpers you added).

Will do.

Thanks!

Patrick