On Mon, Sep 23, 2019 at 10:00:37AM -0400, Derrick Stolee wrote:
> > builtin/pack-objects.c
> > 7c59828b 2694) (reuse_packfile_bitmap &&
> > 7c59828b 2695) bitmap_walk_contains(bitmap_git, reuse_packfile_bitmap,
> > oid));
>
> This change to obj_is_packed(oid) is a small change in a
> really big commit. Here is the change:
I'd suggest not looking too closely at this one yet. This was a
preliminary split by Christian of some older code that I had written. I
think it needs to be split up more, and probably does need more test
coverage (bitmaps are only enabled in a few specific scripts).
> @@ -2571,7 +2706,9 @@ static void ll_find_deltas(struct object_entry **list,
> unsigned list_size,
>
> static int obj_is_packed(const struct object_id *oid)
> {
> - return !!packlist_find(&to_pack, oid, NULL);
> + return packlist_find(&to_pack, oid, NULL) ||
> + (reuse_packfile_bitmap &&
> + bitmap_walk_contains(bitmap_git, reuse_packfile_bitmap,
> oid));
> }
>
> So, every time this is called, the || is short-circuited because
> packlist_find() returns true.
That does surprise me, though. I'm not sure if it's insufficient
testing, or if there's a bug.
> Does this change the meaning of this method? obj_is_packed() would
> only return true if we found the object specifically in the to_pack
> list. Now, we would also return true if the object is in the bitmap
> walk.
>
> If this is only a performance improvement, and the bitmap_walk_contains()
> method would return the same as packlist_find(), then should the order
> be swapped? Or rather, should reuse_packfile_bitmap indicate which to use
> as the full result?
No, there should be cases where the walk mentions some objects that
aren't added to the to_pack list (that's the point of the reuse code; it
fast-tracks a big chunk of objects directly to the output).
-Peff
