Re: [PATCH v1 1/3] pack-bitmap.c: use "ret" in "open_midx_bitmap()"

[Taylor Blau <me@ttaylorr.com>]

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