Re: [PATCH v1 2/3] pack-bitmap.c: add "break" statement in "open_pack_bitmap()"

From: Taylor Blau <me@ttaylorr.com>
Date: Mar 24, 2022, 19:03 UTC
Message-ID: <YjzAbSJfuu06smnD@nand.local>
In-Reply-To: <70500b63434f6e454631b3a8fde07c62b5adfef0.1648119652.git.dyroneteng@gmail.com>

On Thu, Mar 24, 2022 at 07:44:00PM +0800, Teng Long wrote:

Show 21 quoted lines

> There will be only one operant ".bitmap" file in repo, so let's
> add "break" statement in "open_pack_bitmap()" when looping all
> the packs in repo.
>
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>  pack-bitmap.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 931219adf0..b1357137bf 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -484,8 +484,10 @@ static int open_pack_bitmap(struct repository *r,
>  	assert(!bitmap_git->map);
>
>  	for (p = get_all_packs(r); p; p = p->next) {
> -		if (open_pack_bitmap_1(bitmap_git, p) == 0)
> +		if (open_pack_bitmap_1(bitmap_git, p) == 0) {
>  			ret = 0;
> +			break;

The lack of a break here is intentional, I think, since having more than one bitmap of the same kind in a repository is an error.

(This is behavior we inherited from the pre-MIDX bitmap days, when having more than one pack bitmap caused Git to signal an error, since it could only use the results from a single bitmap).

You can see in pack-bitmap.c::open_pack_bitmap_1() that we have a condition saying:

    if (bitmap_git->pack || bitmap_git->midx) {
        /* ignore extra bitmap file; we can only handle one */
        warning("...")
        close(fd;)
        return -1;
    }

We do want to call that open_pack_bitmap_1() function on every pack we know about to make sure that one and only one of them corresponds to a .bitmap.

Thanks, Taylor