Re: [PATCH v1 1/3] pack-bitmap.c: use "ret" in "open_midx_bitmap()"
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Mar 24, 2022, 19:11 UTC
- Message-ID
- <YjzCTLLDCby+kJrZ@nand.local>
- In-Reply-To
- <3048b4dd2982932fa11ba8393895fa33a00a5b58.1648119652.git.dyroneteng@gmail.com>
On Thu, Mar 24, 2022 at 07:43:59PM +0800, Teng Long wrote:
Show 23 quoted lines
> 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