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
