Re: [PATCH] pack-objects: handle island check for "external" delta base

From: Martin Ågren <martin.agren@gmail.com>
Date: Sep 19, 2018, 18:34 UTC
Message-ID: <CAN0heSpMRfDd7=E_kYL6ucL9bL2VWMGLTAkT72BUrVwR6YTezQ@mail.gmail.com>
In-Reply-To: <20180919034907.GA7626@sigill.intra.peff.net>

On Wed, 19 Sep 2018 at 05:49, Jeff King <peff@peff.net> wrote:

Show 7 quoted lines

> This is tricky to do inside a single "if" statement. And
> after the merge in f3504ea3dd (Merge branch
> 'cc/delta-islands', 2018-09-17), that "if" condition is
> already getting pretty unwieldy. So this patch moves the
> logic into a helper function, where we can easily use
> multiple return paths. The result is a bit longer, but the
> logic should be much easier to follow.

Show 8 quoted lines

> +static int can_reuse_delta(const unsigned char *base_sha1,
> +                          struct object_entry *delta,
> +                          struct object_entry **base_out)
> +{
> +       struct object_entry *base;
> +
> +       if (!base_sha1)
> +               return 0;

So this corresponds to "if (base_ref &&".

> +       /*
> +        * First see if we're already sending the base (or it's explicitly in
> +        * our "excluded" list.
> +        */

Missing ')'.

> +       base = packlist_find(&to_pack, base_sha1, NULL);
> +       if (base) {
> +               if (!in_same_island(&delta->idx.oid, &base->idx.oid))
> +                       return 0;

This logic matches the removed code...

Show 10 quoted lines

> +               *base_out = base;
> +               return 1;
> +       }
> +
> +       /*
> +        * Otherwise, reachability bitmaps may tell us if the receiver has it,
> +        * even if it was buried too deep in history to make it into the
> +        * packing list.
> +        */
> +       if (thin && bitmap_has_sha1_in_uninteresting(bitmap_git, base_sha1)) {

This matches...

Show 5 quoted lines

> +               if (use_delta_islands) {
> +                       struct object_id base_oid;
> +                       hashcpy(base_oid.hash, base_sha1);
> +                       if (!in_same_island(&delta->idx.oid, &base_oid))
> +                               return 0;

This does some extra juggling to avoid using `base->idx.oid`, which would have been the moral equivalent of the original code, but which won't fly since `base` is NULL.

Show 20 quoted lines

> +               }
> +               *base_out = NULL;
> +               return 1;
> +       }
> +
> +       return 0;
> +}
> +
>  static void check_object(struct object_entry *entry)
>  {
>         unsigned long canonical_size;
> @@ -1556,22 +1607,7 @@ static void check_object(struct object_entry *entry)
>                         break;
>                 }
>
> -               if (base_ref && (
> -                   (base_entry = packlist_find(&to_pack, base_ref, NULL)) ||
> -                   (thin &&
> -                    bitmap_has_sha1_in_uninteresting(bitmap_git, base_ref))) &&
> -                   in_same_island(&entry->idx.oid, &base_entry->idx.oid)) {

Yeah, the new function looks much simpler than this. We have

  if (A && (B1 || B2) && C) {.

Knowing what to look for, it can be seen that we can -- under the right circumstances -- have A and B2, but not B1, and try to evalute C by dereferencing `base_entry` which will be NULL.

> +               if (can_reuse_delta(base_ref, entry, &base_entry)) {
>                         oe_set_type(entry, entry->in_pack_type);
>                         SET_SIZE(entry, in_pack_size); /* delta size */
>                         SET_DELTA_SIZE(entry, in_pack_size);

Without being at all familiar with this code, this looks sane to me. Just had a small nit about the missing closing ')'.

Martin