On Mon, Dec 9, 2019 at 8:06 AM Jeff King <peff@xxxxxxxx> wrote:
>
> On Fri, Nov 15, 2019 at 03:15:37PM +0100, Christian Couder wrote:
> > +int bitmap_walk_contains(struct bitmap_index *bitmap_git,
> > + struct bitmap *bitmap, const struct object_id *oid)
> > +{
> > + int idx;
> > +
> > + if (!bitmap)
> > + return 0;
> > +
> > + idx = bitmap_position(bitmap_git, oid);
> > + return idx >= 0 && bitmap_get(bitmap, idx);
> > +}
>
> This is really a factoring out of code in
> bitmap_has_oid_in_uninteresting(). So I think you could simplify that
> like:
>
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index cbfc544411..f5749d0ab3 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -1194,16 +1194,6 @@ void free_bitmap_index(struct bitmap_index *b)
> int bitmap_has_oid_in_uninteresting(struct bitmap_index *bitmap_git,
> const struct object_id *oid)
> {
> - int pos;
> -
> - if (!bitmap_git)
> - return 0; /* no bitmap loaded */
> - if (!bitmap_git->haves)
> - return 0; /* walk had no "haves" */
> -
> - pos = bitmap_position_packfile(bitmap_git, oid);
> - if (pos < 0)
> - return 0;
> -
> - return bitmap_get(bitmap_git->haves, pos);
> + return bitmap_git &&
> + bitmap_walk_contains(bitmap_git, bitmap_git->haves, oid);
> }
Yeah, nice simplification. I added a patch doing that.
> One curiosity is that bitmap_has_oid_in_uninteresting() only uses
> bitmap_position_packfile(), not bitmap_position(). So it wouldn't find
> objects which weren't in the bitmapped packfile (i.e., ones where we
> extended the bitmap to handle loose objects, or objects in other packs).
>
> That seems like a bug in the current code to me. I suspect nobody
> noticed because the only effect would be that sometimes we fail to
> notice that we could reuse a delta against such an object (which isn't
> incorrect, just suboptimal). I don't think p5311 would show this,
> though, because it simulates a server that is fully packed.
>
> I think it's probably still worth doing this as a preparatory patch,
> though:
>
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index e07c798879..6df22e7291 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -1125,7 +1125,7 @@ int bitmap_has_oid_in_uninteresting(struct bitmap_index *bitmap_git,
> if (!bitmap_git->haves)
> return 0; /* walk had no "haves" */
>
> - pos = bitmap_position_packfile(bitmap_git, oid);
> + pos = bitmap_position(bitmap_git, oid);
> if (pos < 0)
> return 0;
Yeah, I agree that it's a good idea to do it in a preparatory patch,
so I added a patch doing that before the one doing the simplification
you suggest above.
