On Thu, Mar 24, 2022 at 07:43:59PM +0800, Teng Long wrote:
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 9c666cdb8b..931219adf0 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -494,15 +494,18 @@ static int open_pack_bitmap(struct repository *r,
> static int open_midx_bitmap(struct repository *r,
> struct bitmap_index *bitmap_git)
> {
> + int ret = -1;
> struct multi_pack_index *midx;
>
> assert(!bitmap_git->map);
>
> for (midx = get_multi_pack_index(r); midx; midx = midx->next) {
> - if (!open_midx_bitmap_1(bitmap_git, midx))
> - return 0;
> + if (!open_midx_bitmap_1(bitmap_git, midx)) {
> + ret = 0;
> + break;
> + }
> }
> - return -1;
> + return ret;
This all looks like good clean-up to me, and it indeed preserves the
behavior before and after this patch is applied.
But thinking about some of my comments on patch 2/3 here, I think that
we don't want to break out of that loop until we have visited both the
MIDX in our repository, as well as any alternates (along with _their_
alternates, recursively).
That _is_ a behavior change with respect to the existing implementation
on master, but I think that what's on master is wrong to stop after
looking at the first MIDX bitmap. At least, it's wrong in the same sense
of: "we will only load _one_ of these MIDX bitmaps, so if there is more
than one to choose from, the caller is mistaken".
I think instead we'd want to do something like this on top:
--- 8< ---
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 410020c4d3..0c6640b0f6 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -500,10 +500,8 @@ static int open_midx_bitmap(struct repository *r,
assert(!bitmap_git->map);
for (midx = get_multi_pack_index(r); midx; midx = midx->next) {
- if (!open_midx_bitmap_1(bitmap_git, midx)) {
+ if (!open_midx_bitmap_1(bitmap_git, midx))
ret = 0;
- break;
- }
}
return ret;
}
--- >8 ---
Thanks,
Taylor
