Re: [PATCH] pack-objects: Use reachability bitmap index when generating non-stdout pack too

On Mon, Jul 25, 2016 at 02:40:25PM -0400, Jeff King wrote:

quoted

@@ -1052,6 +1053,9 @@ static int add_object_entry_from_bitmap(const unsigned char *sha1,
 {
 	uint32_t index_pos;
 
+	if (local && has_loose_object_nonlocal(sha1))
+		return 0;
+
 	if (have_duplicate_entry(sha1, 0, &index_pos))
 		return 0;

Hrm. Adding entries from the bitmap should ideally be very fast, but
here we're introducing extra lookups in the object database. I guess it
only kicks in when --local is given, though, which most bitmap-using
paths would not do.

But is this check enough? The non-bitmap code path calls
want_object_in_pack, which checks not only loose objects, but also
non-local packs, and .keep.

Those don't kick in for your use case. I wonder if we should simply have
something like:

  if (local || ignore_packed_keep)
	use_bitmap_index = 0;

and just skip bitmaps for those cases. That's easy to reason about, and
I don't think anybody would care (your use case does not, and the repack
use case is already not going to use bitmaps).

BTW, I thought we had more optimizations in this area, but I realized
that I had never sent them to the list. I just did, and you may want to
take a peek at:

  http://thread.gmane.org/gmane.comp.version-control.git/300218

I doubt it will speed up your case much (unless you really do have tons
of packs in your extraction). And I think it is still worth doing
disabling I showed above, even with the optimizations, just because it's
easier to reason about.

So I _think_ those optimizations are orthogonal to what we're discussing
here, but I wanted to point you at them just in case.

-Peff