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

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> The "--keep-unreachable" flag is supposed to append any unreachable
> objects to the newly written pack. This flag is explicitly documented as
> appending both packed and loose unreachable objects to the new packfile.
> And while this works alright when repacking with preexisting packfiles,
> it stops working when the repository does not have any packfiles at all.

Chuckle.  That's a cute corner case the developers never considered,
it seems ;-).

> The root cause are the conditions used to decide whether or not we want
> to append "--pack-loose-unreachable" to git-pack-objects(1). There are
> a couple of conditions here:
>
>   - `has_existing_non_kept_packs()` checks whether there are existing
>     packfiles. This condition makes sense to guard "--keep-pack=",
>     "--unpack-unreachable" and "--keep-unreachable", because all of
>     these flags only make sense in combination with existing packfiles.
>     But it does not make sense to disable `--pack-loose-unreachable`
>     when there aren't any preexisting packfiles, as loose objects can be
>     packed into the new packfile regardless of that.
>
>   - `delete_redundant` checks whether we want to delete any objects or
>     packs that are about to become redundant. The documentation of
>     `--keep-unreachable` explicitly says that `git repack -ad` needs to
>     be executed for the flag to have an effect.
>
>     It is not immediately obvious why such redundant objects need to be
>     deleted in order for "--pack-unreachable-objects" to be effective.
>     But as things are working as documented this is nothing we'll change
>     for now.
>
>   - `pack_everything & PACK_CRUFT` checks that we're not creating a
>     cruft pack. This condition makes sense in the context of
>     "--pack-loose-unreachable", as unreachable objects would end up in
>     the cruft pack anyway.
>
> So while the second and third condition are sensible, it does not make
> any sense to condition `--pack-loose-unreachable` on the existence of
> packfiles.
>
> 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.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/repack.c  | 5 ++++-
>  t/t7700-repack.sh | 2 +-
>  2 files changed, 5 insertions(+), 2 deletions(-)

Nicely analized and described.  Thanks.