Re: Git Test Coverage Report (Sept 19)

From: Jeff King <peff@peff.net>
Date: Sep 24, 2019, 06:58 UTC
Message-ID: <20190924065806.GB30419@sigill.intra.peff.net>
In-Reply-To: <1ed86989-9ba2-0cd7-b6f7-654d1943b1d7@gmail.com>

On Mon, Sep 23, 2019 at 10:00:37AM -0400, Derrick Stolee wrote:

Show 6 quoted lines

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

Show 12 quoted lines

> @@ -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.

Show 9 quoted lines

> 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