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

From: Jeff King <peff@peff.net>
Date: Sep 19, 2018, 18:43 UTC
Message-ID: <20180919184343.GA13014@sigill.intra.peff.net>
In-Reply-To: <CAN0heSpMRfDd7=E_kYL6ucL9bL2VWMGLTAkT72BUrVwR6YTezQ@mail.gmail.com>

On Wed, Sep 19, 2018 at 08:34:05PM +0200, Martin Ågren wrote:

Show 6 quoted lines

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

Oops, yes.

Show 9 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.

Yeah, this is the actual bug-fix.

I wasn't happy about having to write in_same_island() twice, but writing it the other way ended up pretty nasty, too. Something like:

  struct object_id ext_oid;
  struct object_id *base_oid;

  base = packlist_find(&to_pack, base_sha1, NULL);
  if (base) {
	base_oid = &base->idx.oid;
	*base_out = base;
  }
  else if (thin && bitmap_has_sha1_in_uninteresting(bitmap_git, base_sha1)) {
	hashcpy(ext_oid.hash, base_sha1);
	base_oid = &ext_oid;
	*base_out = NULL;
  } else {
	return 0;
  }

  if (use_island_marks && !in_same_island(&delta->idx.oid, base_oid))
	return 0;

  return 1;

That's less repetitive, but I feel like it's harder to follow which variables are valid when.

Show 7 quoted lines

> > +               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 ')'.

Thanks for the review!

-Peff