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

From: Teng Long <dyroneteng@gmail.com>
Date: Mar 29, 2022, 02:49 UTC
Message-ID: <20220329024949.62091-1-dyroneteng@gmail.com>
In-Reply-To: <YjzAbSJfuu06smnD@nand.local>

On Thu, 24 Mar 2022 15:03:09 -0400, Taylor Blau wrote:

> 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.

Yeah. I quite sure that it's intentional now.

Thanks for your explanations.

Show 17 quoted lines

> (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.

Yeah, the condition and "warning" make it clear to me which is if already exists a bitmap of the pack or MIDX is ready, we will give warnings and just let it fail (return -1 means a return of NULL in "prepare_bitmap_git()", and will then die() in usage cases I found).

In addition of above, I had a question that why we need "bitmap_git->midx" in the condition? Because here in "open_pack_bitmap_1()" we intent to open the non-midx-bitmap and it's after we deal with "open_midx_bitmap()" in "open_bitmap()":

static int open_bitmap(struct repository *r,
              struct bitmap_index *bitmap_git)
{			      
	assert(!bitmap_git->map);

	if (!open_midx_bitmap(r, bitmap_git))
	   return 0;
	   return open_pack_bitmap(r, bitmap_git);
}

So if I understood correct, maybe we can made condition of "bitmap_git->midx" a little earlier so that we can avoid to open every packfile, maybe it's like:

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 9c666cdb8b..38f53b8f1c 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -483,11 +483,12 @@ 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)
-                       ret = 0;
+       if (!bitmap_git->midx) {
+               for (p = get_all_packs(r); p; p = p->next) {
+                       if (open_pack_bitmap_1(bitmap_git, p) == 0)
+                               ret = 0;
+               }
        }
-
        return ret;
 }


Thanks.