On Wed, Sep 19, 2018 at 08:34:05PM +0200, Martin Ågren wrote:
> > + /*
> > + * First see if we're already sending the base (or it's explicitly
> > in
> > + * our "excluded" list.
> > + */
>
> Missing ')'.
Oops, yes.
> > + 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.
> > + 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
