Re: [PATCH v2 6/8] builtin/pack-objects.c: rewrite honor-pack-keep logic

[Taylor Blau <me@xxxxxxxxxxxx>]

On Wed, Feb 17, 2021 at 11:05:22AM -0500, Jeff King wrote:
> I think just bumping that:
>
>   if (local && !p->pack_local)
> 	return 0;

> above the new code would fix it. Or to lay out the logic more fully, the
> order of checks should be:

>   - does _this_ pack we found the object in disqualify it. If so, we can
>     cheaply return 0. And that applies to both keep and local rules.
>
>   - otherwise, check all packs via has_object_kept_pack(), which is
>     cheaper than continuing to iterate through all packs by returning
>     -1.
>
>   - once we know definitively about keep-packs, then check any shortcuts
>     related to local packs (like !have_non_local_packs)
>
>   - and then if no shortcuts, we return -1

I don't understand what you're suggesting. Is the (local &&
!p->pack_local) a disqualifying condition? Reading the comment, I think
it is, and so we could do something like:

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 36c2fa3aff..be3ba60bc2 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1205,14 +1205,21 @@ static int want_found_object(const struct object_id *oid, int exclude,
         * make sure no copy of this object appears in _any_ pack that makes us
         * to omit the object, so we need to check all the packs.
         *
-        * We can however first check whether these options can possible matter;
+        * We can however first check whether these options can possibly matter;
         * if they do not matter we know we want the object in generated pack.
         * Otherwise, we signal "-1" at the end to tell the caller that we do
         * not know either way, and it needs to check more packs.
         */

        /*
-        * Handle .keep first, as we have a fast(er) path there.
+        * Objects in packs borrowed from elsewhere are discarded regardless of
+        * if they appear in other packs that weren't borrowed.
+        */
+       if (local && !p->pack_local)
+               return 0;
+
+       /*
+        * Then handle .keep first, as we have a fast(er) path there.
         */
        if (ignore_packed_keep_on_disk || ignore_packed_keep_in_core) {
                /*
@@ -1242,11 +1249,8 @@ static int want_found_object(const struct object_id *oid, int exclude,
         * keep-packs, or the object is not in one. Keep checking other
         * conditions...
         */
-
        if (!local || !have_non_local_packs)
                return 1;
-       if (local && !p->pack_local)
-               return 0;

        /* we don't know yet; keep looking for more packs */
        return -1;

But your "check any shortcuts related to local packs" makes me think
that we should leave the code as-is.

Which are you suggesting?

Thanks,
Taylor