On Fri, Nov 15, 2019 at 03:15:37PM +0100, Christian Couder wrote:
> We will use this helper function in a following commit to
> tell us if an object is packed.
Yeah, makes sense. This is eventually used in have_duplicate_entry() in
pack-objects, to check whether an object is already mentioned in
reuse_packfile_bitmap. And that's the part that would fix the test
failures from the previous commit.
But of course we don't yet have reuse_packfile_bitmap; that comes later.
> +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);
}
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;
-Peff
