Re: with reuse-delta patches, fetching with bitmaps segfaults due to possibly incomplete bitmap traverse

[Jeff King <peff@peff.net>]

On Fri, Mar 21, 2014 at 07:58:55PM -0700, Siddharth Agarwal wrote:

> At Facebook we've found that fetch speed is a bottleneck for our Git repos,
> so we've been looking to deploy bitmaps to speed up fetches. We've been
> trying out git-next with the top two patches from
> https://github.com/peff/git/commits/jk/bitmap-reuse-delta, but the following
> is reproducible with tip of that branch, currently 81cdec2.

Is it also reproducible just with the tip of "next"? Note that the
patches in jk/bitmap-reuse-delta have not been widely deployed (in
particular, we are not yet using them at GitHub, and we track segfaults
on our servers closely and have not seen any related to this).

Those patches allocate extra "fake" entries in the entry->delta fields,
which are not accounted for in to_pack.nr_objects. It's entirely
possible that those entries are related to the bug you are seeing.

> I dug into this a bit and it looks like at this point:
> 
> https://github.com/peff/git/blob/81cdec28fa24fdc613ab7c3406c1c67975dbf22f/builtin/pack-objects.c#L700
> 
> at some object that add_family_to_write_order is called for, wo_end exceeds
> to_pack.nr_objects by over 1000 objects. More precisely, at the point it
> crashes, wo_end is 218081 while to_pack.nr_objects is 201614. (This means
> wo_end overshot to_pack.nr_objects some time ago.)

Hmm, yeah, that confirms my suspicion. In the earlier loops, we call
add_to_write_order, which only adds the object in question, and can
never exceed to_pack.nr_objects. In this final loop, we call
add_family_to_write_order, which is going to add any deltas that were
not already included.

The patch below may fix your problem, but I have a feeling it is not the
right thing to do. The point of 81cdec28 is to try to point to a delta
entry as if it were a "preferred base" (i.e., something we know that the
other side has already). We perhaps want to add these entries to the
actual packing list, and skip them as we do with normal preferred_base
objects.

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 9fc5321..ca1b0f7 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1437,6 +1437,7 @@ static void check_object(struct object_entry *entry)
 			entry->delta = xcalloc(1, sizeof(*entry->delta));
 			hashcpy(entry->delta->idx.sha1, base_ref);
 			entry->delta->preferred_base = 1;
+			entry->delta->filled = 1;
 			unuse_pack(&w_curs);
 			return;
 		}

-Peff