[PATCH v1 0/3] trace2 output for bitmap decision path

[PATCH v1 0/3] trace2 output for bitmap decision path

[Teng Long]

A Git repo can be chosen to use the normal bitmap (before MIDX) and MIDX bitmap.

I recently tried to understand this part of the MIDX implementation because I found
a bug which has been discovered and repaired in community [1].

I am grateful to Taylor Blau for his help and for introducing me to the testing
method according to the `git rev-list --test-bitmap <rev>`.

In the process of understanding and troubleshooting by using this command, I found
when the execution is failed it will output a single line of
"fatal: failed to load bitmap indexes", sometimes will be more informations like
if the bitmap file is broken, the outputs maybe contain
"error: Corrupted bitmap index file (wrong header)".), but most of time are single
line output I mentioned above. So, this brings a little obstacle for debugging and
troubleshooting I think, because "failed to load bitmap indexes" can represent
to much informations (many causes like: midx config turn off, bitmap inexistence, etc.)

Therefore, as a git repo can be chosen to use the normal bitmap (before MIDX) or
MIDX bitmap, or they can both exist and let git make the decision. I think why not add
some extra informations based on TRACE2 to help for showing the bitmap decision path
clearer and more plentiful, so when the failure occurs the user can use it to debug
around bitmap in a quicker way.

Thanks.

Links:
	1. https://public-inbox.org/git/cover.1638991570.git.me@ttaylorr.com/)

Teng Long (3):
  pack-bitmap.c: use "ret" in "open_midx_bitmap()"
  pack-bitmap.c: add "break" statement in "open_pack_bitmap()"
  bitmap: add trace outputs during open "bitmap" file

 midx.c        |  2 ++
 pack-bitmap.c | 17 +++++++++++++----
 2 files changed, 15 insertions(+), 4 deletions(-)

Range-diff against v0:
-:  ---------- > 1:  3048b4dd29 pack-bitmap.c: use "ret" in "open_midx_bitmap()"
-:  ---------- > 2:  70500b6343 pack-bitmap.c: add "break" statement in "open_pack_bitmap()"
-:  ---------- > 3:  9912450fc1 bitmap: add trace outputs during open "bitmap" file
-- 
2.35.1.579.g70500b6343.dirty

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

[Teng Long]

Let's use "ret" value for "return" statement in "open_midx_bitmap()"
just as the same way as int "open_pack_bitmap()".

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

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;
 }
 
 static int open_bitmap(struct repository *r,
-- 
2.35.1.579.g70500b6343.dirty

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

[Taylor Blau]

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

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

[Teng Long]

On Thu, 24 Mar 2022 15:11:08 -0400, Taylor Blau wrote:


> 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'm a little wondering that what's the practial meaning for _ do not _
stop after looking at the first MIDX bitmap? 

Although all MIDX bitmap are scanned, only one of them will eventually work
, and there seems to be no guarantee that the last MIDX that works is the
most appropriate one? (with the same comfusion applies to non-MIDX
bitmap's behavour...)

Thanks.

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

[Taylor Blau]

On Mon, Mar 28, 2022 at 03:59:07PM +0800, Teng Long wrote:
> Although all MIDX bitmap are scanned, only one of them will eventually work
> , and there seems to be no guarantee that the last MIDX that works is the
> most appropriate one? (with the same comfusion applies to non-MIDX
> bitmap's behavour...)

I think your "which is the most appropriate one?" exactly highlights the
problem. Since Git can only handle one .bitmap file at a time, we should
tell the user when we found more than one candidate.

In this case, they may be expecting or depending on a particular choice
for performance reasons, and so may be rather surprised or upset if Git
chose a different available .bitmap.

The one exception to this is when we have both a single- and multi-pack
bitmap available, in which case it almost always makes more sense to
pick the multi-pack bitmap. So we don't complain in this case (i.e., if
open_midx_bitmap() succeeds, we immediately return success from
open_bitmap() and proceed to have prepare_bitmap_git() call
load_bitmap() on the previous result without even bothering to check for
pack bitmaps).

Thanks,
Taylor

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

[Teng Long]

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;
+		}
 	}
 
 	return ret;
-- 
2.35.1.579.g70500b6343.dirty

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

[Junio C Hamano]

Teng Long <dyroneteng@gmail.com> writes:

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

Googling "operant" only gives a psychology term X-<.

    : behavior (such as bar pressing by a rat to obtain food) that
    operates on the environment to produce rewarding and reinforcing
    effects.

I do not quite get this.  We expect that there may be more than one
.pack in the object store, and that is why we iterate over the packs
get_all_packs() gives us.  Then for each of these packs, we try to
open pack bitmap.

Ah, are you referring to the current limitation that we can have
only one .bitmap file, even when there are many packs in the object
store?

I wonder if that limitation is something we need to hardcode here.

IOW, when we ever extend the feature to allow more than one, we may
have to extend the "struct bitmap_index" so that we can have bitmap
info from more than one on-disk files, and the implementation of
open_pack_bitmap_1() (which is "read bitmap for this one pack" as
opposed to "read bitmpa for this repository" which is this function)
would certainly have to change, but would we need to revert this
change to the function?  If we do not omit this patch, i.e. even if
we know there can be only one pack that may have a corresponding
bitmap but if the code does not know it and has to try git_open()
only to see it fail, how much wasted effort are we talking about?

> 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;
> +		}
>  	}
>  
>  	return ret;

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

[Taylor Blau]

On Thu, Mar 24, 2022 at 11:40:02AM -0700, Junio C Hamano wrote:
> Teng Long <dyroneteng@gmail.com> writes:
>
> > 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.
>
> Googling "operant" only gives a psychology term X-<.
>
>     : behavior (such as bar pressing by a rat to obtain food) that
>     operates on the environment to produce rewarding and reinforcing
>     effects.

I suspect that the author meant "operating", which I would probably have
written as "Git will only use information from a single .bitmap file in
the repository" or similar.

But...

> I do not quite get this.  We expect that there may be more than one
> .pack in the object store, and that is why we iterate over the packs
> get_all_packs() gives us.  Then for each of these packs, we try to
> open pack bitmap.
>
> Ah, are you referring to the current limitation that we can have
> only one .bitmap file, even when there are many packs in the object
> store?
>
> I wonder if that limitation is something we need to hardcode here.

...you're exactly right: if we see more than one pack bitmap in the
repository, we can and should warn there, since we'll only ever *use*
information from a single bitmap at a time.

That's the behavior before this patch which is guaranteed by t5310.74.
This patch causes that test to break, and I would suggest that is
telling us to drop this part of the series.

Thanks,
Taylor

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

[Taylor Blau]

On Thu, Mar 24, 2022 at 07:44:00PM +0800, Teng Long wrote:
> 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

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

[Teng Long]

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.

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

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

[Taylor Blau]

On Tue, Mar 29, 2022 at 10:49:49AM +0800, Teng Long wrote:
> 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()":

You're right; open_pack_bitmap_1() doesn't need to care about whether or
not bitmap_git->midx is or isn't non-NULL, since:

  - if we did open a MIDX bitmap (which we will always attempt first
    before trying to open single-pack bitmaps), then we won't even
    bother to call open_pack_bitmap() at all.

  - if we _do_ end up within open_pack_bitmap_1(), then we _know_ that
    no MIDX bitmap could be found/opened, so there is no need to check
    in that case, either.

So I think we realistically could do something like:

--- 8< ---

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 97909d48da..6e7c89826d 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -387,3 +387,3 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git

-	if (bitmap_git->pack || bitmap_git->midx) {
+	if (bitmap_git->pack) {
 		/* ignore extra bitmap file; we can only handle one */

--- >8 ---

...but having the conditional there from the pre-image doesn't hurt,
either, and it makes the error clearer in case of an accidental
regression where we start looking for single-pack bitmaps after
successfully opening a multi-pack one.

> 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;
>  }

This shouldn't be necessary, since we don't bother calling
open_pack_bitmap() at all if open_midx_bitmap() returns success. In
other words, based on the way that open_bitmap() (which is the caller
for both of these functions) is written, we know that once we're in
open_pack_bitmap(), that `bitmap_git->midx` is definitely NULL, which
makes this change a noop.

Thanks,
Taylor

Teng Long

[Teng Long]

On Tue, 29 Mar 2022 22:55:39 -0400, Taylor Blau wrote:

> You're right; open_pack_bitmap_1() doesn't need to care about whether or
> not bitmap_git->midx is or isn't non-NULL, since:
> 
>   - if we did open a MIDX bitmap (which we will always attempt first
>     before trying to open single-pack bitmaps), then we won't even
>     bother to call open_pack_bitmap() at all.
> 
>   - if we _do_ end up within open_pack_bitmap_1(), then we _know_ that
>     no MIDX bitmap could be found/opened, so there is no need to check
>     in that case, either.
> 
> So I think we realistically could do something like:
> 
> --- 8< ---
> 
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 97909d48da..6e7c89826d 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -387,3 +387,3 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
> 
> -  if (bitmap_git->pack || bitmap_git->midx) {
> +  if (bitmap_git->pack) {
>       /* ignore extra bitmap file; we can only handle one */
> 
> --- >8 ---
> ...but having the conditional there from the pre-image doesn't hurt,
> either, and it makes the error clearer in case of an accidental
> regression where we start looking for single-pack bitmaps after
> successfully opening a multi-pack one.

I agree with the "accidental regression", it's a protection without
any disadvantages so far. So, if we don't remove the "bitmap_git->midx"
condition for some robust reason, then I think maybe we can let the
warning more detailed if the "accident" happens, like:

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 91a6be358d..e64a85bc59 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -333,7 +333,15 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
                struct strbuf buf = STRBUF_INIT;
                get_midx_filename(&buf, midx->object_dir);
                /* ignore extra bitmap file; we can only handle one */
-               warning("ignoring extra bitmap file: %s", buf.buf);
+               warning("%signoring extra bitmap file: %s",
+                       bitmap_git->pack ?
+                               xstrfmt("A non-MIDX bitmap has been opened: %s, ",
+                                       bitmap_git->pack->pack_name) :
+                       bitmap_git->midx ?
+                               xstrfmt("A MIDX bitmap has been opened : %s, ",
+                                       midx_bitmap_filename(bitmap_git->midx)) :
+                                     "",
+                       buf.buf);
                close(fd);
                strbuf_release(&buf);
                return -1;
@@ -398,9 +406,17 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
                return -1;
        }
 
-       if (bitmap_git->pack) {
+       if (bitmap_git->pack || bitmap_git->midx) {
                /* ignore extra bitmap file; we can only handle one */
-               warning("ignoring extra bitmap file: %s", packfile->pack_name);
+               warning("%signoring extra bitmap file: %s",
+                       bitmap_git->pack ?
+                               xstrfmt("A non-MIDX bitmap has been opened: %s, ",
+                                       bitmap_git->pack->pack_name) :
+                       bitmap_git->midx ?
+                               xstrfmt("A MIDX bitmap has been opened : %s, ",
+                                       midx_bitmap_filename(bitmap_git->midx)) :
+                                     "",
+                       packfile->pack_name);
                close(fd);
                return -1;
        }
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index f775fc1ce6..a962f6861f 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -421,6 +421,7 @@ test_expect_success 'complains about multiple pack bitmaps' '
                test_line_count = 2 bitmaps &&
 
                git rev-list --use-bitmap-index HEAD 2>err &&
+               grep "A non-MIDX bitmap has been opened" err &&
                grep "ignoring extra bitmap file" err
        )


Does that make sense to you? 

And also I look codes in "open_midx_bitmap_1()", it's really nice for me to read and
understand, but the "char *idx_name = ..." declaration made me a litte confusing at
the beginning, I think maybe it's better to name it as "bitmap_name" for it, I know
bitmap is also a kind of index structure for using, but when I saw "idx", it brings
me to the ".idx" or "multi-pack-index" like files naturally in git context, of course
this is not a problem and maybe it's just me.

Re: Teng Long

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 30 2022, Teng Long wrote:

> On Tue, 29 Mar 2022 22:55:39 -0400, Taylor Blau wrote:
>
>> You're right; open_pack_bitmap_1() doesn't need to care about whether or
>> not bitmap_git->midx is or isn't non-NULL, since:
>> 
>>   - if we did open a MIDX bitmap (which we will always attempt first
>>     before trying to open single-pack bitmaps), then we won't even
>>     bother to call open_pack_bitmap() at all.
>> 
>>   - if we _do_ end up within open_pack_bitmap_1(), then we _know_ that
>>     no MIDX bitmap could be found/opened, so there is no need to check
>>     in that case, either.
>> 
>> So I think we realistically could do something like:
>> 
>> --- 8< ---
>> 
>> diff --git a/pack-bitmap.c b/pack-bitmap.c
>> index 97909d48da..6e7c89826d 100644
>> --- a/pack-bitmap.c
>> +++ b/pack-bitmap.c
>> @@ -387,3 +387,3 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
>> 
>> -  if (bitmap_git->pack || bitmap_git->midx) {
>> +  if (bitmap_git->pack) {
>>       /* ignore extra bitmap file; we can only handle one */
>> 
>> --- >8 ---
>> ...but having the conditional there from the pre-image doesn't hurt,
>> either, and it makes the error clearer in case of an accidental
>> regression where we start looking for single-pack bitmaps after
>> successfully opening a multi-pack one.
>
> I agree with the "accidental regression", it's a protection without
> any disadvantages so far. So, if we don't remove the "bitmap_git->midx"
> condition for some robust reason, then I think maybe we can let the
> warning more detailed if the "accident" happens, like:
>
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 91a6be358d..e64a85bc59 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -333,7 +333,15 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
>                 struct strbuf buf = STRBUF_INIT;
>                 get_midx_filename(&buf, midx->object_dir);
>                 /* ignore extra bitmap file; we can only handle one */
> -               warning("ignoring extra bitmap file: %s", buf.buf);
> +               warning("%signoring extra bitmap file: %s",
> +                       bitmap_git->pack ?
> +                               xstrfmt("A non-MIDX bitmap has been opened: %s, ",
> +                                       bitmap_git->pack->pack_name) :
> +                       bitmap_git->midx ?
> +                               xstrfmt("A MIDX bitmap has been opened : %s, ",
> +                                       midx_bitmap_filename(bitmap_git->midx)) :
> +                                     "",
> +                       buf.buf);
>                 close(fd);

Aside from any bitmap-specific issues, let's not compose messages like
that via concat, it makes them harder to translate.

In this case the original message doesn't have _(), but looking at the
context that seems to be a simple omission.

It's more verbose, but it's better to split this out into 3x warning()
calls.

It also has the advantage of not leaking memory from the xstrfmt().

[PATCH v1 3/3] bitmap: add trace outputs during open "bitmap" file

[Teng Long]

It's supported for a Git repo to use bitmap in both normal bitmap way
or a multi-pack-index bitmap.

Sometimes the debug path is not obvious, for example, when executing:

	  $git rev-list  --test-bitmap  HEAD
	  fatal: failed to load bitmap indexes

If we see the output like this, we are not sure about what's happened,
because the cause should be :

	  1. neither normal nor midx bitmap exists
	  2. only midx bitmap exists but core.multipackIndex="false"
          3. core.multipackIndex="true" but midx bitmap file is currupt
	  4. core.multipackIndex="true" and no midx bitmap exists but
	     normal bitmap file is currupt
	 ....

These are some of the scenarios I briefly tested, but maybe there are
others (some scenarios is produced manually like "currupt bitmap file",
but it's not represent it's an existed bug.).

Therefore, we added some TRACE2 code so that when we read the bitmap
we can be more clear about the decision path, such as whether it is
working on midx bitmap or normal bitmap, or is it simply because the
related configuration is disabled. This may help with logging, user
troubleshooting, and development debugging.

Here are some of the outputs of

     "$GIT_TRACE2_PERF=1 git rev-list --test-bitmap HEAD"

based on this commit:

      1. core.multipackindex="true" and midx bitmap exists

	17:12:50.496112 common-main.c:49             | d0 | main                     | version      |     |           |           |              | 2.35.1.579.g70500b6343.dirty
	17:12:50.496141 common-main.c:50             | d0 | main                     | start        |     |  0.000306 |           |              | /opt/git/master/bin/git rev-list --test-bitmap HEAD
	17:12:50.496208 compat/linux/procinfo.c:170  | d0 | main                     | cmd_ancestry |     |           |           |              | ancestry:[bash sshd sshd sshd systemd]
	17:12:50.496333 git.c:460                    | d0 | main                     | cmd_name     |     |           |           |              | rev-list (rev-list)
	17:12:50.496536 midx.c:395                   | d0 | main                     | data         | r0  |  0.000706 |  0.000706 | midx         | core.multipackIndex:true
	17:12:50.496563 midx.c:185                   | d0 | main                     | data         | r0  |  0.000733 |  0.000733 | midx         | load/num_packs:1
	17:12:50.496571 midx.c:186                   | d0 | main                     | data         | r0  |  0.000741 |  0.000741 | midx         | load/num_objects:15
	17:12:50.496677 pack-revindex.c:315          | d0 | main                     | data         | r0  |  0.000847 |  0.000847 | load_midx_re | source:midx
	17:12:50.496685 pack-bitmap.c:512            | d0 | main                     | data         | r0  |  0.000856 |  0.000856 | midx         | open bitmap (midx):ok
	Bitmap v1 test (4 entries loaded)
	Found bitmap for 9a0a2928a280d91f5c8ded46e952add4c4296979. 64 bits / f15de25e checksum
	17:12:50.497016 progress.c:268               | d0 | main                     | region_enter | r0  |  0.001186 |           | progress     | label:Verifying bitmap entries
	Verifying bitmap entries: 100% (15/15), done.
	17:12:50.497179 progress.c:339               | d0 | main                     | data         | r0  |  0.001350 |  0.000164 | progress     | ..total_objects:15
	17:12:50.497188 progress.c:346               | d0 | main                     | region_leave | r0  |  0.001358 |  0.000172 | progress     | label:Verifying bitmap entries
	OK!
	17:12:50.497204 git.c:718                    | d0 | main                     | exit         |     |  0.001374 |           |              | code:0
	17:12:50.497213 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001384 |           |              | code:0

      2. core.multipackindex="false" and only midx bitmap exists

	17:18:38.423328 common-main.c:49             | d0 | main                     | version      |     |           |           |              | 2.35.1.579.g70500b6343.dirty
	17:18:38.423357 common-main.c:50             | d0 | main                     | start        |     |  0.000263 |           |              | /opt/git/master/bin/git rev-list --test-bitmap HEAD
	17:18:38.423443 compat/linux/procinfo.c:170  | d0 | main                     | cmd_ancestry |     |           |           |              | ancestry:[bash sshd sshd sshd systemd]
	17:18:38.423587 git.c:460                    | d0 | main                     | cmd_name     |     |           |           |              | rev-list (rev-list)
	17:18:38.423809 midx.c:395                   | d0 | main                     | data         | r0  |  0.000720 |  0.000720 | midx         | core.multipackIndex:false
	17:18:38.423933 pack-bitmap.c:512            | d0 | main                     | data         | r0  |  0.000845 |  0.000845 | midx         | open bitmap (midx):failed
	17:18:38.423947 pack-bitmap.c:493            | d0 | main                     | data         | r0  |  0.000858 |  0.000858 | bitmap       | open bitmap (non-midx):failed
	17:18:38.423956 usage.c:60                   | d0 | main                     | error        |     |           |           |              | failed to load bitmap indexes
	fatal: failed to load bitmap indexes
	17:18:38.423967 usage.c:74                   | d0 | main                     | exit         |     |  0.000879 |           |              | code:128
	17:18:38.423976 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.000888 |           |              | code:128

      3. core.multipackindex="false" and only normal bitmap exists

	17:21:25.519233 common-main.c:49             | d0 | main                     | version      |     |           |           |              | 2.35.1.579.g70500b6343.dirty
	17:21:25.519261 common-main.c:50             | d0 | main                     | start        |     |  0.000284 |           |              | /opt/git/master/bin/git rev-list --test-bitmap HEAD
	17:21:25.519335 compat/linux/procinfo.c:170  | d0 | main                     | cmd_ancestry |     |           |           |              | ancestry:[bash sshd sshd sshd systemd]
	17:21:25.519468 git.c:460                    | d0 | main                     | cmd_name     |     |           |           |              | rev-list (rev-list)
	17:21:25.519673 midx.c:395                   | d0 | main                     | data         | r0  |  0.000701 |  0.000701 | midx         | core.multipackIndex:false
	17:21:25.520051 pack-bitmap.c:512            | d0 | main                     | data         | r0  |  0.001080 |  0.001080 | midx         | open bitmap (midx):failed
	17:21:25.520070 pack-bitmap.c:493            | d0 | main                     | data         | r0  |  0.001099 |  0.001099 | bitmap       | open bitmap (non-midx):ok
	Bitmap v1 test (4 entries loaded)
	Found bitmap for 9a0a2928a280d91f5c8ded46e952add4c4296979. 64 bits / f15de25e checksum
	17:21:25.520395 progress.c:268               | d0 | main                     | region_enter | r0  |  0.001423 |           | progress     | label:Verifying bitmap entries
	Verifying bitmap entries: 100% (15/15), done.
	17:21:25.520526 progress.c:339               | d0 | main                     | data         | r0  |  0.001555 |  0.000132 | progress     | ..total_objects:15
	17:21:25.520533 progress.c:346               | d0 | main                     | region_leave | r0  |  0.001562 |  0.000139 | progress     | label:Verifying bitmap entries
	OK!
	17:21:25.520549 git.c:718                    | d0 | main                     | exit         |     |  0.001578 |           |              | code:0
	17:21:25.520557 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001586 |           |              | code:0

      4. core.multipackindex="false" and both midx and normal bitmaps do not exists

	17:22:27.680275 common-main.c:49             | d0 | main                     | version      |     |           |           |              | 2.35.1.579.g70500b6343.dirty
	17:22:27.680298 common-main.c:50             | d0 | main                     | start        |     |  0.000284 |           |              | /opt/git/master/bin/git rev-list --test-bitmap HEAD
	17:22:27.680375 compat/linux/procinfo.c:170  | d0 | main                     | cmd_ancestry |     |           |           |              | ancestry:[bash sshd sshd sshd systemd]
	17:22:27.680511 git.c:460                    | d0 | main                     | cmd_name     |     |           |           |              | rev-list (rev-list)
	17:22:27.680718 midx.c:395                   | d0 | main                     | data         | r0  |  0.000709 |  0.000709 | midx         | core.multipackIndex:false
	17:22:27.681085 pack-bitmap.c:512            | d0 | main                     | data         | r0  |  0.001075 |  0.001075 | midx         | open bitmap (midx):failed
	17:22:27.681102 pack-bitmap.c:493            | d0 | main                     | data         | r0  |  0.001093 |  0.001093 | bitmap       | open bitmap (non-midx):failed
	17:22:27.681110 usage.c:60                   | d0 | main                     | error        |     |           |           |              | failed to load bitmap indexes
	fatal: failed to load bitmap indexes
	17:22:27.681120 usage.c:74                   | d0 | main                     | exit         |     |  0.001112 |           |              | code:128
	17:22:27.681129 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001120 |           |              | code:128

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 midx.c        | 2 ++
 pack-bitmap.c | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/midx.c b/midx.c
index 865170bad0..fda9644028 100644
--- a/midx.c
+++ b/midx.c
@@ -392,6 +392,8 @@ int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, i
 	struct multi_pack_index *m_search;
 
 	prepare_repo_settings(r);
+	trace2_data_string("midx", r, "core.multipackIndex",
+					   r->settings.core_multi_pack_index ? "true" : "false");
 	if (!r->settings.core_multi_pack_index)
 		return 0;
 
diff --git a/pack-bitmap.c b/pack-bitmap.c
index b1357137bf..14cf8abebd 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -490,6 +490,8 @@ static int open_pack_bitmap(struct repository *r,
 		}
 	}
 
+	trace2_data_string("bitmap", the_repository, "open bitmap (non-midx)",
+					   ret ? "failed" : "ok");
 	return ret;
 }
 
@@ -507,6 +509,8 @@ static int open_midx_bitmap(struct repository *r,
 			break;
 		}
 	}
+	trace2_data_string("midx", the_repository, "open bitmap (midx)",
+					   ret ? "failed" : "ok");
 	return ret;
 }
 
-- 
2.35.1.579.g70500b6343.dirty

Re: [PATCH v1 3/3] bitmap: add trace outputs during open "bitmap" file

[Junio C Hamano]

Teng Long <dyroneteng@gmail.com> writes:

> It's supported for a Git repo to use bitmap in both normal bitmap way
> or a multi-pack-index bitmap.

This step out of 3 patches does match what was sold on the cover
letter, and it does not seem to even need the other two.

> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>  midx.c        | 2 ++
>  pack-bitmap.c | 4 ++++
>  2 files changed, 6 insertions(+)
>
> diff --git a/midx.c b/midx.c
> index 865170bad0..fda9644028 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -392,6 +392,8 @@ int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, i
>  	struct multi_pack_index *m_search;
>  
>  	prepare_repo_settings(r);
> +	trace2_data_string("midx", r, "core.multipackIndex",
> +					   r->settings.core_multi_pack_index ? "true" : "false");

That's an overly deep contination line.

>  	if (!r->settings.core_multi_pack_index)
>  		return 0;
>  
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index b1357137bf..14cf8abebd 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -490,6 +490,8 @@ static int open_pack_bitmap(struct repository *r,
>  		}
>  	}
>  
> +	trace2_data_string("bitmap", the_repository, "open bitmap (non-midx)",
> +					   ret ? "failed" : "ok");

Likewise.

>  	return ret;
>  }
>  
> @@ -507,6 +509,8 @@ static int open_midx_bitmap(struct repository *r,
>  			break;
>  		}
>  	}
> +	trace2_data_string("midx", the_repository, "open bitmap (midx)",
> +					   ret ? "failed" : "ok");

Likewise.

>  	return ret;
>  }

Re: [PATCH v1 0/3] trace2 output for bitmap decision path

[Ævar Arnfjörð Bjarmason]

On Thu, Mar 24 2022, Teng Long wrote:

> A Git repo can be chosen to use the normal bitmap (before MIDX) and MIDX bitmap.
>
> I recently tried to understand this part of the MIDX implementation because I found
> a bug which has been discovered and repaired in community [1].
>
> I am grateful to Taylor Blau for his help and for introducing me to the testing
> method according to the `git rev-list --test-bitmap <rev>`.
>
> In the process of understanding and troubleshooting by using this command, I found
> when the execution is failed it will output a single line of
> "fatal: failed to load bitmap indexes", sometimes will be more informations like
> if the bitmap file is broken, the outputs maybe contain
> "error: Corrupted bitmap index file (wrong header)".), but most of time are single
> line output I mentioned above. So, this brings a little obstacle for debugging and
> troubleshooting I think, because "failed to load bitmap indexes" can represent
> to much informations (many causes like: midx config turn off, bitmap inexistence, etc.)
>
> Therefore, as a git repo can be chosen to use the normal bitmap (before MIDX) or
> MIDX bitmap, or they can both exist and let git make the decision. I think why not add
> some extra informations based on TRACE2 to help for showing the bitmap decision path
> clearer and more plentiful, so when the failure occurs the user can use it to debug
> around bitmap in a quicker way.
>
> Thanks.
>
> Links:
> 	1. https://public-inbox.org/git/cover.1638991570.git.me@ttaylorr.com/)
>
> Teng Long (3):
>   pack-bitmap.c: use "ret" in "open_midx_bitmap()"
>   pack-bitmap.c: add "break" statement in "open_pack_bitmap()"
>   bitmap: add trace outputs during open "bitmap" file
>
>  midx.c        |  2 ++
>  pack-bitmap.c | 17 +++++++++++++----
>  2 files changed, 15 insertions(+), 4 deletions(-)
>
> Range-diff against v0:
> -:  ---------- > 1:  3048b4dd29 pack-bitmap.c: use "ret" in "open_midx_bitmap()"
> -:  ---------- > 2:  70500b6343 pack-bitmap.c: add "break" statement in "open_pack_bitmap()"
> -:  ---------- > 3:  9912450fc1 bitmap: add trace outputs during open "bitmap" file

Was there an on-list v0 (RFC?) or is this a range-diff against nothing?
Best not to include it until a v2 then.

Comments:

Sometimes it's better to split up patches, but I think these 3x should
really be squashed together. We make incremental progress to nowhere in
1/3 and 2/3, and it all comes together in 3/3. The 1-2/3 are trivial
enough that we can squash them in.

We then end up with this, with my comments added:
	
	 midx.c        |  2 ++
	 pack-bitmap.c | 17 +++++++++++++----
	 2 files changed, 15 insertions(+), 4 deletions(-)
	
	diff --git a/midx.c b/midx.c
	index 865170bad05..fda96440287 100644
	--- a/midx.c
	+++ b/midx.c
	@@ -392,6 +392,8 @@ int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, i
	 	struct multi_pack_index *m_search;
	 
	 	prepare_repo_settings(r);
	+	trace2_data_string("midx", r, "core.multipackIndex",
	+					   r->settings.core_multi_pack_index ? "true" : "false");

Weird indentation here.

Also, if we think it's a good idea to log these shouldn't it be in
repo_cfg_bool() in repo-settings.c, why is core.multipackIndex out of
all in r->settings special?

	 	if (!r->settings.core_multi_pack_index)
	 		return 0;
	 
	diff --git a/pack-bitmap.c b/pack-bitmap.c
	index 97909d48da3..cac8d4a978f 100644
	--- a/pack-bitmap.c
	+++ b/pack-bitmap.c
	@@ -484,25 +484,34 @@ 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) {

Aside: If we end up changing this line anyway, it's OK to just change it
to "if (!open...".


	 			ret = 0;
	+			break;
	+		}
	 	}
	 
	+	trace2_data_string("bitmap", the_repository, "open bitmap (non-midx)",
	+					   ret ? "failed" : "ok");
	 	return ret;
	 }
	 
	 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;
	+	trace2_data_string("midx", the_repository, "open bitmap (midx)",
	+					   ret ? "failed" : "ok");
	+	return ret;
	 }
	 
	 static int open_bitmap(struct repository *r,

It seems odd not to use trace2 regions for this, and to not add add this
data logging open_bitmap(). I came up with this on top of this when
testing this:
	
	diff --git a/pack-bitmap.c b/pack-bitmap.c
	index cac8d4a978f..ba71a7ea5cd 100644
	--- a/pack-bitmap.c
	+++ b/pack-bitmap.c
	@@ -318,11 +318,14 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
	 
	 	free(idx_name);
	 
	-	if (fd < 0)
	+	if (fd < 0) {
	+		/* TODO: Log trace2_data_string() here, do we care? */
	 		return -1;
	+	}
	 
	 	if (fstat(fd, &st)) {
	 		close(fd);
	+		/* TODO: Log trace2_data_string() here, do we care? */
	 		return -1;
	 	}
	 
	@@ -330,6 +333,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
	 		struct strbuf buf = STRBUF_INIT;
	 		get_midx_filename(&buf, midx->object_dir);
	 		/* ignore extra bitmap file; we can only handle one */
	+		/* NOTE: You'll already get a warning (well, "error") event due to this, and it'll be in your region */
	 		warning("ignoring extra bitmap file: %s", buf.buf);
	 		close(fd);
	 		strbuf_release(&buf);
	@@ -344,9 +348,11 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
	 	close(fd);
	 
	 	if (load_bitmap_header(bitmap_git) < 0)
	+		/* TODO: Add trace2_data_string() or warning/error here? */
	 		goto cleanup;
	 
	 	if (!hasheq(get_midx_checksum(bitmap_git->midx), bitmap_git->checksum))
	+		/* TODO: Add trace2_data_string() or warning/error here? */
	 		goto cleanup;
	 
	 	if (load_midx_revindex(bitmap_git->midx) < 0) {
	@@ -479,49 +485,44 @@ static int open_pack_bitmap(struct repository *r,
	 			    struct bitmap_index *bitmap_git)
	 {
	 	struct packed_git *p;
	-	int ret = -1;
	-
	-	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;
	-			break;
	+			return 0;
	 		}
	 	}
	-
	-	trace2_data_string("bitmap", the_repository, "open bitmap (non-midx)",
	-					   ret ? "failed" : "ok");
	-	return ret;
	+	return -1;
	 }
	 
	 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)) {
	-			ret = 0;
	-			break;
	+			return 0;
	 		}
	 	}
	-	trace2_data_string("midx", the_repository, "open bitmap (midx)",
	-					   ret ? "failed" : "ok");
	-	return ret;
	+	return -1;
	 }
	 
	 static int open_bitmap(struct repository *r,
	 		       struct bitmap_index *bitmap_git)
	 {
	+	int ret;
	+
	 	assert(!bitmap_git->map);
	 
	-	if (!open_midx_bitmap(r, bitmap_git))
	-		return 0;
	-	return open_pack_bitmap(r, bitmap_git);
	+	trace2_region_enter("pack-bitmap", "open_bitmap", r);
	+	if (!open_midx_bitmap(r, bitmap_git)) {
	+		ret = 0;
	+		goto done;
	+	}
	+	ret = open_pack_bitmap(r, bitmap_git);
	+done:
	+	trace2_region_leave("pack-bitmap", "open_bitmap", r);
	+	return ret;
	 }
	 
	 struct bitmap_index *prepare_bitmap_git(struct repository *r)

I.e. surely you just want to create a region, and if you care enough to
log failure shouldn't we log that in open_midx_bitmap_1() if we care,
and perhaps error() there instead of silently returning -1?

Teng Long

[Teng Long]

> Was there an on-list v0 (RFC?) or is this a range-diff against nothing?
> Best not to include it until a v2 then.

My careless, there's no RFC, actually this patch should begin with v0 and
without range-diff.


> Sometimes it's better to split up patches, but I think these 3x should
> really be squashed together. We make incremental progress to nowhere in
> 1/3 and 2/3, and it all comes together in 3/3. The 1-2/3 are trivial
> enough that we can squash them in.

Sure.

>	+	trace2_data_string("midx", r, "core.multipackIndex",
>	+						   r->settings.core_multi_pack_index ? "true" : "false");
>
> Weird indentation here.

Will fix.

> I.e. surely you just want to create a region, and if you care enough to
> log failure shouldn't we log that in open_midx_bitmap_1() if we care,

Actually, I just want to use "trace2_data_string()" at first because which informations I
want to append is not so many, or does "create a region" a preferred principle for using
TRACE2 APIs?

> and perhaps error() there instead of silently returning -1?

I think so, after I checked function "error_builtin()" and there is a "trace2_cmd_error_va()"
usage in it which is already support to print some debug infos using TRACE2 envs.

Thanks.

Re: Teng Long

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 29 2022, Teng Long wrote:

>> Was there an on-list v0 (RFC?) or is this a range-diff against nothing?
>> Best not to include it until a v2 then.
>
> My careless, there's no RFC, actually this patch should begin with v0 and
> without range-diff.

No worries.

>> Sometimes it's better to split up patches, but I think these 3x should
>> really be squashed together. We make incremental progress to nowhere in
>> 1/3 and 2/3, and it all comes together in 3/3. The 1-2/3 are trivial
>> enough that we can squash them in.
>
> Sure.
>
>>	+	trace2_data_string("midx", r, "core.multipackIndex",
>>	+						   r->settings.core_multi_pack_index ? "true" : "false");
>>
>> Weird indentation here.
>
> Will fix.

*Nod*

>> I.e. surely you just want to create a region, and if you care enough to
>> log failure shouldn't we log that in open_midx_bitmap_1() if we care,
>
> Actually, I just want to use "trace2_data_string()" at first because which informations I
> want to append is not so many, or does "create a region" a preferred principle for using
> TRACE2 APIs?

I think a begin/end region gives you everything you want here and more, i.e.:

 * We'll get start/end times on this (potentially expensive?) operation
   automatically.

 * You just log a "failed", but as the RFC-on-top shows we can just log
   the specific reason instead, either via error(), warning() or perhaps
   even trace2_data_string(). because it's within a region it'll be
   clear from the data what failed.

B.t.w. I noticed after sending that that this ad-hoc patch fails the
tests, it passed the one bitmap test I hacked it up against.

It was just there to make the point/demo that it looked easier to do
this one function call above where you were adding this.

>> and perhaps error() there instead of silently returning -1?
>
> I think so, after I checked function "error_builtin()" and there is a "trace2_cmd_error_va()"
> usage in it which is already support to print some debug infos using TRACE2 envs.

Exactly.

[PATCH v2 0/5] trace2 output for bitmap decision path

[Teng Long]

Sorry for the late patch. Main diffs from v1:

1. Add a new commit to rename "idx_name" and this may clear up
some misconceptions about the naming

2. Add the warnings more detailed when opening bitmap and also
let the texts support translation.

3. using error() instead of silently returning -1 when opening
bitmap.

4. Continue looping when first MIDX bitmap is found.

Thanks.

Teng Long (5):
  pack-bitmap.c: continue looping when first MIDX bitmap is found
  pack-bitmap.c: rename "idx_name" to "bitmap_name"
  pack-bitmap.c: make warnings more detailed when opening bitmap
  bitmap: add trace2 outputs during open "bitmap" file
  pack-bitmap.c: using error() instead of silently returning -1

 pack-bitmap.c           | 73 ++++++++++++++++++++++++++++-------------
 repo-settings.c         |  1 +
 t/t5310-pack-bitmaps.sh |  3 +-
 3 files changed, 54 insertions(+), 23 deletions(-)

Range-diff against v1:
1:  3048b4dd29 ! 1:  1bfd2fb6ab pack-bitmap.c: use "ret" in "open_midx_bitmap()"
    @@ Metadata
     Author: Teng Long <dyroneteng@gmail.com>
     
      ## Commit message ##
    -    pack-bitmap.c: use "ret" in "open_midx_bitmap()"
    +    pack-bitmap.c: continue looping when first MIDX bitmap is found
     
    -    Let's use "ret" value for "return" statement in "open_midx_bitmap()"
    -    just as the same way as int "open_pack_bitmap()".
    +    In "open_midx_bitmap()", we do a loop with the MIDX(es) in repo, when
    +    the first one has been found, then will break out by a "return"
    +    directly.
     
    +    But actually, it's better to don't stop the loop until we have visited
    +    both the MIDX in our repository, as well as any alternates (along with
    +    _their_ alternates, recursively).
    +
    +    The discussion link of community:
    +
    +      https://public-inbox.org/git/YjzCTLLDCby+kJrZ@nand.local/
    +
    +    Helped-by: Taylor Blau <me@ttaylorr.com>
         Signed-off-by: Teng Long <dyroneteng@gmail.com>
     
      ## pack-bitmap.c ##
    @@ pack-bitmap.c: static int open_pack_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))
     -			return 0;
    -+		if (!open_midx_bitmap_1(bitmap_git, midx)) {
     +			ret = 0;
    -+			break;
    -+		}
      	}
     -	return -1;
     +	return ret;
2:  70500b6343 < -:  ---------- pack-bitmap.c: add "break" statement in "open_pack_bitmap()"
3:  9912450fc1 < -:  ---------- bitmap: add trace outputs during open "bitmap" file
-:  ---------- > 2:  1fff3b3ca7 pack-bitmap.c: rename "idx_name" to "bitmap_name"
-:  ---------- > 3:  5a8f5afccf pack-bitmap.c: make warnings more detailed when opening bitmap
-:  ---------- > 4:  2016ef2e34 bitmap: add trace2 outputs during open "bitmap" file
-:  ---------- > 5:  1a169d7b5e pack-bitmap.c: using error() instead of silently returning -1
-- 
2.35.1.583.g30faa5f068

[PATCH v2 1/5] pack-bitmap.c: continue looping when first MIDX bitmap is found

[Teng Long]

In "open_midx_bitmap()", we do a loop with the MIDX(es) in repo, when
the first one has been found, then will break out by a "return"
directly.

But actually, it's better to don't stop the loop until we have visited
both the MIDX in our repository, as well as any alternates (along with
_their_ alternates, recursively).

The discussion link of community:

  https://public-inbox.org/git/YjzCTLLDCby+kJrZ@nand.local/

Helped-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 9c666cdb8b..112c2b12c6 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -494,15 +494,16 @@ 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;
+			ret = 0;
 	}
-	return -1;
+	return ret;
 }
 
 static int open_bitmap(struct repository *r,
-- 
2.35.1.583.g30faa5f068

Re: [PATCH v2 1/5] pack-bitmap.c: continue looping when first MIDX bitmap is found

[Taylor Blau]

On Thu, Apr 21, 2022 at 09:26:36PM +0800, Teng Long wrote:
> In "open_midx_bitmap()", we do a loop with the MIDX(es) in repo, when
> the first one has been found, then will break out by a "return"
> directly.
>
> But actually, it's better to don't stop the loop until we have visited

s/don't stop/continue

> both the MIDX in our repository, as well as any alternates (along with
> _their_ alternates, recursively).
>
> The discussion link of community:
>
>   https://public-inbox.org/git/YjzCTLLDCby+kJrZ@nand.local/

In the future, it is often worth summarizing the discussion, optionally
linking off to the list archive. In this case, I wouldn't mind a little
more detail beyond "it's better to [continue] the loop until ...".

In particular, you say it's better without saying why that is the case.
The link to <YjzCTLLDCby+kJrZ@nand.local> explains, but this commit
message as-is leaves out some details that are important IMHO.

> ---
>  pack-bitmap.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

The actual changes look good to me.

Thanks,
Taylor

[PATCH v2 2/5] pack-bitmap.c: rename "idx_name" to "bitmap_name"

[Teng Long]

In "open_pack_bitmap_1()" and "open_midx_bitmap_1()" we use
a var named "idx_name" to represent the bitmap filename which
is computed by "midx_bitmap_filename()" or "pack_bitmap_filename()"
before we open it.

There may bring some confusion in this "idx_name" naming, which
might lead us to think of ".idx "or" multi-pack-index" files,
although bitmap is essentially can be understood as a kind of index
, let's define this name a little more accurate here.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 112c2b12c6..f13a6bfe3a 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -313,10 +313,10 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 			      struct multi_pack_index *midx)
 {
 	struct stat st;
-	char *idx_name = midx_bitmap_filename(midx);
-	int fd = git_open(idx_name);
+	char *bitmap_name = midx_bitmap_filename(midx);
+	int fd = git_open(bitmap_name);
 
-	free(idx_name);
+	free(bitmap_name);
 
 	if (fd < 0)
 		return -1;
@@ -368,14 +368,14 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 {
 	int fd;
 	struct stat st;
-	char *idx_name;
+	char *bitmap_name;
 
 	if (open_pack_index(packfile))
 		return -1;
 
-	idx_name = pack_bitmap_filename(packfile);
-	fd = git_open(idx_name);
-	free(idx_name);
+	bitmap_name = pack_bitmap_filename(packfile);
+	fd = git_open(bitmap_name);
+	free(bitmap_name);
 
 	if (fd < 0)
 		return -1;
-- 
2.35.1.583.g30faa5f068

Re: [PATCH v2 2/5] pack-bitmap.c: rename "idx_name" to "bitmap_name"

[Taylor Blau]

On Thu, Apr 21, 2022 at 09:26:37PM +0800, Teng Long wrote:
> although bitmap is essentially can be understood as a kind of index
> , let's define this name a little more accurate here.

The placement of this comma is a little odd, but otherwise the patch
looks good to me.

Thanks,
Taylor

[PATCH v2 3/5] pack-bitmap.c: make warnings more detailed when opening bitmap

[Teng Long]

When calling the "open_midx_bitmap_1()" or "open_pack_bitmap_1()", there
will be a warning if a normal bitmap or MIDX bitmap already has been
opened, then let's make the warning information more detailed. For
example, it makes the error clearer in case of an accidental
regression where we start looking for single-pack bitmaps after
successfully opening a multi-pack one.

At the same time, we made the previous and new warning texts support
translation.

Discussion in community:

  1. https://public-inbox.org/git/YkPGq0mDL4NG6D1o@nand.local/#t
  2. https://public-inbox.org/git/220330.868rsrpohm.gmgdl@evledraar.gmail.com/

Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Helped-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c           | 6 ++++--
 t/t5310-pack-bitmaps.sh | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index f13a6bfe3a..1b268f655e 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -330,7 +330,8 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 		struct strbuf buf = STRBUF_INIT;
 		get_midx_filename(&buf, midx->object_dir);
 		/* ignore extra bitmap file; we can only handle one */
-		warning("ignoring extra bitmap file: %s", buf.buf);
+		warning(_("a normal or midx bitmap already has been opened"));
+		warning(_("ignoring extra bitmap file: %s"), buf.buf);
 		close(fd);
 		strbuf_release(&buf);
 		return -1;
@@ -387,7 +388,8 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 
 	if (bitmap_git->pack || bitmap_git->midx) {
 		/* ignore extra bitmap file; we can only handle one */
-		warning("ignoring extra bitmap file: %s", packfile->pack_name);
+		warning(_("a normal or midx bitmap already has been opened "));
+		warning(_("ignoring extra bitmap file: %s"), packfile->pack_name);
 		close(fd);
 		return -1;
 	}
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index f775fc1ce6..eb63b71852 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -421,6 +421,7 @@ test_expect_success 'complains about multiple pack bitmaps' '
 		test_line_count = 2 bitmaps &&
 
 		git rev-list --use-bitmap-index HEAD 2>err &&
+		grep "a bitmap has been opened" err &&
 		grep "ignoring extra bitmap file" err
 	)
 '
-- 
2.35.1.583.g30faa5f068

Re: [PATCH v2 3/5] pack-bitmap.c: make warnings more detailed when opening bitmap

[Taylor Blau]

On Thu, Apr 21, 2022 at 09:26:38PM +0800, Teng Long wrote:
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index f13a6bfe3a..1b268f655e 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -330,7 +330,8 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
>  		struct strbuf buf = STRBUF_INIT;
>  		get_midx_filename(&buf, midx->object_dir);
>  		/* ignore extra bitmap file; we can only handle one */
> -		warning("ignoring extra bitmap file: %s", buf.buf);
> +		warning(_("a normal or midx bitmap already has been opened"));
> +		warning(_("ignoring extra bitmap file: %s"), buf.buf);

I'm glad that the existing warning has been marked for translation.
There's no reason that it couldn't have been before, so I'm glad to see
it added now.

But I'm not sure that the new warning tells the user anything that the
old one didn't. The old warning says "ignoring extra bitmap file: ...",
leading us to believe that another one has already been opened. The new
warning makes the latter part explicit, but I don't think it adds any
new information that wasn't already readily available.

> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> index f775fc1ce6..eb63b71852 100755
> --- a/t/t5310-pack-bitmaps.sh
> +++ b/t/t5310-pack-bitmaps.sh
> @@ -421,6 +421,7 @@ test_expect_success 'complains about multiple pack bitmaps' '
>  		test_line_count = 2 bitmaps &&
>
>  		git rev-list --use-bitmap-index HEAD 2>err &&
> +		grep "a bitmap has been opened" err &&

This is out of sync with the warning you added, causing t5310.74 to
fail.

Thanks,
Taylor

Re: [PATCH v2 3/5] pack-bitmap.c: make warnings more detailed when opening bitmap

[Teng Long]

On Thu, 21 Apr 2022 13:25:58 -0400, Taylor Blau wrote:

> I'm glad that the existing warning has been marked for translation.
> There's no reason that it couldn't have been before, so I'm glad to see
> it added now.

Thanks and it's actually suggested by Ævar Arnfjörð Bjarmason.

> But I'm not sure that the new warning tells the user anything that the
> old one didn't. The old warning says "ignoring extra bitmap file: ...",
> leading us to believe that another one has already been opened. The new
> warning makes the latter part explicit, but I don't think it adds any
> new information that wasn't already readily available.
> ... 
> This is out of sync with the warning you added, causing t5310.74 to
> fail.

Yeah, I agree with you about this. 
Will roll back on here and fix the broken test in next patch.


Thanks

[PATCH v2 4/5] bitmap: add trace2 outputs during open "bitmap" file

[Teng Long]

It's supported for a Git repo to use bitmap in both normal bitmap way
or a multi-pack-index bitmap.

Sometimes the debug path is not obvious, for example, when executing:

 $git rev-list  --test-bitmap  HEAD
 fatal: failed to load bitmap indexes

If we see the output like this, we are not sure about what's happened,
because the cause should be :

  1. neither normal nor midx bitmap exists
  2. only midx bitmap exists but core.multipackIndex="false"
  3. core.multipackIndex="true" but midx bitmap file is currupt
  4. core.multipackIndex="true" and no midx bitmap exists but
     normal bitmap file is currupt
  ....

These are some of the scenarios I briefly tested, but maybe there are
others (some scenarios is produced manually like "currupt bitmap file",
but it's not represent it's an existed bug.).

Therefore, we added some TRACE2 code so that when we read the bitmap
we can be more clear about the decision path, such as whether it is
working on midx bitmap or normal bitmap, or is it simply because the
related configuration is disabled. This may help with logging, user
troubleshooting, and development debugging.

Here are some output examples when executing
"$GIT_TRACE2_PERF=1 git rev-list --test-bitmap HEAD" under different
situations:

1. _HAVE_ MIDX bitmap and NORMAL bitmap file, but the
"core.multipackIndex" configures as "false":

  19:38:43.007840 common-main.c:49             | d0 | main                     | version      |     |           |           |              | 2.35.1.582.g8e9092487a
  19:38:43.007874 common-main.c:50             | d0 | main                     | start        |     |  0.000305 |           |              | /opt/git/master/bin/git rev-list --test-bitmap HEAD
  19:38:43.007950 compat/linux/procinfo.c:170  | d0 | main                     | cmd_ancestry |     |           |           |              | ancestry:[bash sshd sshd sshd systemd]
  19:38:43.008091 git.c:460                    | d0 | main                     | cmd_name     |     |           |           |              | rev-list (rev-list)
  19:38:43.008284 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000720 |  0.000720 | config       | feature.manyfiles:false
  19:38:43.008297 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000734 |  0.000734 | config       | feature.experimental:false
  19:38:43.008305 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000742 |  0.000742 | config       | core.commitgraph:true
  19:38:43.008313 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000749 |  0.000749 | config       | commitgraph.readchangedpaths:true
  19:38:43.008320 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000756 |  0.000756 | config       | gc.writecommitgraph:true
  19:38:43.008327 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000764 |  0.000764 | config       | fetch.writecommitgraph:false
  19:38:43.008334 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000770 |  0.000770 | config       | pack.usesparse:true
  19:38:43.008341 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000777 |  0.000777 | config       | core.multipackindex:false
  19:38:43.008348 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000784 |  0.000784 | config       | index.sparse:false
  19:38:43.008724 pack-bitmap.c:536            | d0 | main                     | region_enter | r0  |  0.001160 |           | pack-bitmap  | label:open_bitmap
  19:38:43.008738 pack-bitmap.c:390            | d0 | main                     | data         | r0  |  0.001173 |  0.000013 | bitmap       | ..try to open bitmap:/home/tenglong.tl/test/dyrone/.git/objects/pack/pack-2d1b3f749fb859b874710e33263d0847ef009e03.bitmap
  19:38:43.008754 pack-bitmap.c:543            | d0 | main                     | region_leave | r0  |  0.001191 |  0.000031 | pack-bitmap  | label:open_bitmap
  Bitmap v1 test (8 entries loaded)
  Found bitmap for 0a7df7ae92f8d8ed879c240f8ae9cdd33d18085e. 64 bits / 1801edc6 checksum
  19:38:43.009099 progress.c:268               | d0 | main                     | region_enter | r0  |  0.001535 |           | progress     | label:Verifying bitmap entries
  Verifying bitmap entries: 100% (27/27), done.
  19:38:43.009294 progress.c:339               | d0 | main                     | data         | r0  |  0.001730 |  0.000195 | progress     | ..total_objects:27
  19:38:43.009302 progress.c:346               | d0 | main                     | region_leave | r0  |  0.001739 |  0.000204 | progress     | label:Verifying bitmap entries
  OK!
  19:38:43.009321 git.c:718                    | d0 | main                     | exit         |     |  0.001757 |           |              | code:0
  19:38:43.009329 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001766 |           |              | code:0

2. _HAVE_ MIDX bitmap and NORMAL bitmap file, but the
"core.multipackIndex" configures as "true":

  (omit duplicate outputs)
  ...
  20:02:31.288797 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000811 |  0.000811 | config       | core.multipackindex:true
  20:02:31.288806 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000819 |  0.000819 | config       | index.sparse:false
  20:02:31.288836 midx.c:185                   | d0 | main                     | data         | r0  |  0.000849 |  0.000849 | midx         | load/num_packs:1
  20:02:31.288843 midx.c:186                   | d0 | main                     | data         | r0  |  0.000857 |  0.000857 | midx         | load/num_objects:27
  20:02:31.289217 pack-bitmap.c:536            | d0 | main                     | region_enter | r0  |  0.001229 |           | pack-bitmap  | label:open_bitmap
  20:02:31.289230 pack-bitmap.c:318            | d0 | main                     | data         | r0  |  0.001242 |  0.000013 | midx         | ..try to open bitmap:/home/tenglong.tl/test/dyrone/.git/objects/pack/multi-pack-index-b6b04fbe053bd500d9ca13354466d3249dc275ac.bitmap
  20:02:31.289252 pack-revindex.c:315          | d0 | main                     | data         | r0  |  0.001265 |  0.000036 | load_midx_re | ..source:midx
  20:02:31.289261 pack-bitmap.c:543            | d0 | main                     | region_leave | r0  |  0.001274 |  0.000045 | pack-bitmap  | label:open_bitmap
  Bitmap v1 test (8 entries loaded)
  Found bitmap for 0a7df7ae92f8d8ed879c240f8ae9cdd33d18085e. 64 bits / 1801edc6 checksum
  20:02:31.289594 progress.c:268               | d0 | main                     | region_enter | r0  |  0.001607 |           | progress     | label:Verifying bitmap entries
  Verifying bitmap entries: 100% (27/27), done.
  20:02:31.289810 progress.c:339               | d0 | main                     | data         | r0  |  0.001823 |  0.000216 | progress     | ..total_objects:27
  20:02:31.289824 progress.c:346               | d0 | main                     | region_leave | r0  |  0.001837 |  0.000230 | progress     | label:Verifying bitmap entries
  OK!
  20:02:31.289843 git.c:718                    | d0 | main                     | exit         |     |  0.001856 |           |              | code:0
  20:02:31.289860 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001873 |           |              | code:0

3. _HAVE_ MIDX bitmap and a corrupt NORMAL bitmap file, but the
"core.multipackIndex" configures as "false" :

  (omit duplicate outputs)
  ...
  20:14:06.539305 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000794 |  0.000794 | config       | core.multipackindex:false
  20:14:06.539310 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000799 |  0.000799 | config       | index.sparse:false
  20:14:06.539658 pack-bitmap.c:536            | d0 | main                     | region_enter | r0  |  0.001146 |           | pack-bitmap  | label:open_bitmap
  20:14:06.539671 pack-bitmap.c:390            | d0 | main                     | data         | r0  |  0.001160 |  0.000014 | bitmap       | ..try to open bitmap:/home/tenglong.tl/test/dyrone/.git/objects/pack/pack-2d1b3f749fb859b874710e33263d0847ef009e03.bitmap
  20:14:06.539686 usage.c:79                   | d0 | main                     | error        |     |           |           |              | ..Corrupted bitmap index file (wrong header)
  error: Corrupted bitmap index file (wrong header)
  20:14:06.539696 pack-bitmap.c:426            | d0 | main                     | data         | r0  |  0.001185 |  0.000039 | bitmap       | ..load bitmap header:failed
  20:14:06.539709 usage.c:79                   | d0 | main                     | error        |     |           |           |              | ..bitmap header is invalid
  error: bitmap header is invalid
  20:14:06.539719 pack-bitmap.c:543            | d0 | main                     | region_leave | r0  |  0.001208 |  0.000062 | pack-bitmap  | label:open_bitmap
  20:14:06.539726 usage.c:60                   | d0 | main                     | error        |     |           |           |              | failed to load bitmap indexes
  fatal: failed to load bitmap indexes
  20:14:06.539735 usage.c:74                   | d0 | main                     | exit         |     |  0.001224 |           |              | code:128
  20:14:06.539744 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001233 |           |              | code:128

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c           | 40 +++++++++++++++++++++++++++++++++-------
 repo-settings.c         |  1 +
 t/t5310-pack-bitmaps.sh |  4 ++--
 3 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 1b268f655e..a1d06c4252 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -312,9 +312,12 @@ char *pack_bitmap_filename(struct packed_git *p)
 static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 			      struct multi_pack_index *midx)
 {
+	int fd;
 	struct stat st;
 	char *bitmap_name = midx_bitmap_filename(midx);
-	int fd = git_open(bitmap_name);
+	trace2_data_string("midx", the_repository, "try to open bitmap",
+			   bitmap_name);
+	fd = git_open(bitmap_name);
 
 	free(bitmap_name);
 
@@ -322,6 +325,8 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 		return -1;
 
 	if (fstat(fd, &st)) {
+		trace2_data_string("midx", the_repository, "stat bitmap file",
+				   "failed");
 		close(fd);
 		return -1;
 	}
@@ -344,12 +349,19 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 				MAP_PRIVATE, fd, 0);
 	close(fd);
 
-	if (load_bitmap_header(bitmap_git) < 0)
+	if (load_bitmap_header(bitmap_git) < 0) {
+		trace2_data_string("midx", the_repository, "load bitmap header",
+				   "failed");
 		goto cleanup;
+	}
 
-	if (!hasheq(get_midx_checksum(bitmap_git->midx), bitmap_git->checksum))
+	if (!hasheq(get_midx_checksum(bitmap_git->midx), bitmap_git->checksum)) {
+		trace2_data_string("midx", the_repository, "verify checksum",
+				   "mismatch");
 		goto cleanup;
 
+	}
+
 	if (load_midx_revindex(bitmap_git->midx) < 0) {
 		warning(_("multi-pack bitmap is missing required reverse index"));
 		goto cleanup;
@@ -375,6 +387,8 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 		return -1;
 
 	bitmap_name = pack_bitmap_filename(packfile);
+	trace2_data_string("bitmap", the_repository, "try to open bitmap",
+			   bitmap_name);
 	fd = git_open(bitmap_name);
 	free(bitmap_name);
 
@@ -382,6 +396,8 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 		return -1;
 
 	if (fstat(fd, &st)) {
+		trace2_data_string("bitmap", the_repository, "stat bitmap file",
+				   "failed");
 		close(fd);
 		return -1;
 	}
@@ -395,6 +411,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 	}
 
 	if (!is_pack_valid(packfile)) {
+		trace2_data_string("bitmap", the_repository, "packfile", "invalid");
 		close(fd);
 		return -1;
 	}
@@ -406,6 +423,8 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 	close(fd);
 
 	if (load_bitmap_header(bitmap_git) < 0) {
+		trace2_data_string("bitmap", the_repository,
+				   "load bitmap header", "failed");
 		munmap(bitmap_git->map, bitmap_git->map_size);
 		bitmap_git->map = NULL;
 		bitmap_git->map_size = 0;
@@ -511,11 +530,18 @@ static int open_midx_bitmap(struct repository *r,
 static int open_bitmap(struct repository *r,
 		       struct bitmap_index *bitmap_git)
 {
-	assert(!bitmap_git->map);
+	int ret = -1;
 
-	if (!open_midx_bitmap(r, bitmap_git))
-		return 0;
-	return open_pack_bitmap(r, bitmap_git);
+	assert(!bitmap_git->map);
+	trace2_region_enter("pack-bitmap", "open_bitmap", r);
+	if (!open_midx_bitmap(r, bitmap_git)) {
+		ret = 0;
+		goto done;
+	}
+	ret = open_pack_bitmap(r, bitmap_git);
+done:
+	trace2_region_leave("pack-bitmap", "open_bitmap", r);
+	return ret;
 }
 
 struct bitmap_index *prepare_bitmap_git(struct repository *r)
diff --git a/repo-settings.c b/repo-settings.c
index b4fbd16cdc..5bc7a97a6d 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -8,6 +8,7 @@ static void repo_cfg_bool(struct repository *r, const char *key, int *dest,
 {
 	if (repo_config_get_bool(r, key, dest))
 		*dest = def;
+	trace2_data_string("config", r, key, *dest ? "true" : "false");
 }
 
 void prepare_repo_settings(struct repository *r)
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index eb63b71852..664cb88b0b 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -421,8 +421,8 @@ test_expect_success 'complains about multiple pack bitmaps' '
 		test_line_count = 2 bitmaps &&
 
 		git rev-list --use-bitmap-index HEAD 2>err &&
-		grep "a bitmap has been opened" err &&
-		grep "ignoring extra bitmap file" err
+		grep "warning: a normal or midx bitmap already has been opened" err &&
+		grep "warning: ignoring extra bitmap file" err
 	)
 '
 
-- 
2.35.1.583.g30faa5f068

Re: [PATCH v2 4/5] bitmap: add trace2 outputs during open "bitmap" file

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 21 2022, Teng Long wrote:

Thanks for following up..

>   19:38:43.007840 common-main.c:49             | d0 | main                     | version      |     |           |           |              | 2.35.1.582.g8e9092487a
>   19:38:43.007874 common-main.c:50             | d0 | main                     | start        |     |  0.000305 |           |              | /opt/git/master/bin/git rev-list --test-bitmap HEAD

It's really helpful to have these full examples in the commit
message. Thanks.

> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -312,9 +312,12 @@ char *pack_bitmap_filename(struct packed_git *p)
>  static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
>  			      struct multi_pack_index *midx)
>  {
> +	int fd;
>  	struct stat st;
>  	char *bitmap_name = midx_bitmap_filename(midx);
> -	int fd = git_open(bitmap_name);
> +	trace2_data_string("midx", the_repository, "try to open bitmap",
> +			   bitmap_name);
> +	fd = git_open(bitmap_name);
>  
>  	free(bitmap_name);

Hrm, so re my comment on 5/5 are you trying to use the "try to open" as
a timer to see what our start time is?

I think probably not, in which case this whole variable flip-around is
something we won't need.

But if we do actually need it perhaps a sub-region for the timing?


> @@ -511,11 +530,18 @@ static int open_midx_bitmap(struct repository *r,
>  static int open_bitmap(struct repository *r,
>  		       struct bitmap_index *bitmap_git)
>  {
> -	assert(!bitmap_git->map);
> +	int ret = -1;
>  
> -	if (!open_midx_bitmap(r, bitmap_git))
> -		return 0;
> -	return open_pack_bitmap(r, bitmap_git);
> +	assert(!bitmap_git->map);
> +	trace2_region_enter("pack-bitmap", "open_bitmap", r);
> +	if (!open_midx_bitmap(r, bitmap_git)) {
> +		ret = 0;

Nit: I think these "goto" patterns are best if your "int ret = N" picks
an "N" which is the default that you'll "goto", i.e. if you pick "ret =
0" you'll just need "goto done" here....

> +		goto done;
> +	}
> +	ret = open_pack_bitmap(r, bitmap_git);

...which we may then override here.

Just saves you the assignment and the {}, but it tends to add up in
longer functions.

> +done:
> +	trace2_region_leave("pack-bitmap", "open_bitmap", r);
> +	return ret;
>  }

Looks good, aside from the 5/5 comments that much of the data string
logging looks like it becomes redundant in the end due to error() giving
us the same thing.

>  struct bitmap_index *prepare_bitmap_git(struct repository *r)
> diff --git a/repo-settings.c b/repo-settings.c
> index b4fbd16cdc..5bc7a97a6d 100644
> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -8,6 +8,7 @@ static void repo_cfg_bool(struct repository *r, const char *key, int *dest,
>  {
>  	if (repo_config_get_bool(r, key, dest))
>  		*dest = def;
> +	trace2_data_string("config", r, key, *dest ? "true" : "false");
>  }
>  
>  void prepare_repo_settings(struct repository *r)
> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> index eb63b71852..664cb88b0b 100755
> --- a/t/t5310-pack-bitmaps.sh
> +++ b/t/t5310-pack-bitmaps.sh
> @@ -421,8 +421,8 @@ test_expect_success 'complains about multiple pack bitmaps' '
>  		test_line_count = 2 bitmaps &&
>  
>  		git rev-list --use-bitmap-index HEAD 2>err &&
> -		grep "a bitmap has been opened" err &&
> -		grep "ignoring extra bitmap file" err
> +		grep "warning: a normal or midx bitmap already has been opened" err &&
> +		grep "warning: ignoring extra bitmap file" err
>  	)
>  '


I haven't tested but part of this test change looks like it'll break
under bisect, i.e. you changed 1/2 of these strings in 3/5. Did you try
with "git rebase -i -x 'make test T=t*bitmap*" or similar?

Re: [PATCH v2 4/5] bitmap: add trace2 outputs during open "bitmap" file

[Teng Long]

On Thu, 21 Apr 2022 17:51:30 +0200, Ævar Arnfjörð Bjarmason wrote:

> It's really helpful to have these full examples in the commit
> message. Thanks.

From the previous contribution process, I think it is necessary
to provide sufficient information in patch, which can help to save
reviewer's time and find problems faster.

> Hrm, so re my comment on 5/5 are you trying to use the "try to open" as
> a timer to see what our start time is?
> 
> I think probably not, in which case this whole variable flip-around is
> something we won't need.
> 
> But if we do actually need it perhaps a sub-region for the timing?

Yes, I looked back on it and found it's redundant and unnecessary now.
Will delete the two related references in the code in next patch.


> Nit: I think these "goto" patterns are best if your "int ret = N" picks
> an "N" which is the default that you'll "goto", i.e. if you pick "ret =
> 0" you'll just need "goto done" here....
> 
> > +	       goto done;
> > +	       }
> > +	       ret = open_pack_bitmap(r, bitmap_git);
> 
> ...which we may then override here.
> 
> Just saves you the assignment and the {}, but it tends to add up in
> longer functions

Make sense.

> > +done:
> > +	trace2_region_leave("pack-bitmap", "open_bitmap", r);
> > +	return ret;
> >  }
> 
> Looks good, aside from the 5/5 comments that much of the data string
> logging looks like it becomes redundant in the end due to error() giving
> us the same thing.

Oops, I will read the comments and reply them in 5/5. 

> I haven't tested but part of this test change looks like it'll break
> under bisect, i.e. you changed 1/2 of these strings in 3/5. Did you try
> with "git rebase -i -x 'make test T=t*bitmap*" or similar?

Yes, they should be in the same commit in 3/5, but now I the new warning
text seems like is much verbose which suggested by Taylor Blau, and I will
roll back the new warning message and fix the broken test under bisect.

Thanks. 

Re: [PATCH v2 4/5] bitmap: add trace2 outputs during open "bitmap" file

[Teng Long]

> Hrm, so re my comment on 5/5 are you trying to use the "try to open" as
> a timer to see what our start time is?
> 
> I think probably not, in which case this whole variable flip-around is
> something we won't need.
> 
> But if we do actually need it perhaps a sub-region for the timing?

By the way, I think it will be better replace "try to open" with the
bitmap name so we can also know about what kind of bitmap we are opening
by filename.

Before: 

      19:38:43.008724 pack-bitmap.c:536            | d0 | main                     | region_enter | r0  |  0.001160 |           | pack-bitmap  | label:open_bitmap
      19:38:43.008738 pack-bitmap.c:390            | d0 | main                     | data         | r0  |  0.001173 |  0.000013 | bitmap       | ..try to open bitmap:/home/tenglong.tl/test/dyrone/.git/objects/pack/pack-2d1b3f749fb859b874710e33263d0847ef009e03.bitmap
      19:38:43.008754 pack-bitmap.c:543            | d0 | main                     | region_leave | r0  |  0.001191 |  0.000031 | pack-bitmap  | label:open_bitmap

After:
      
      19:38:43.008724 pack-bitmap.c:536            | d0 | main                     | region_enter | r0  |  0.001160 |           | pack-bitmap  | label:open_bitmap
      19:38:43.008738 pack-bitmap.c:390            | d0 | main                     | data         | r0  |  0.001173 |  0.000013 | bitmap       | ../home/tenglong.tl/test/dyrone/.git/objects/pack/pack-2d1b3f749fb859b874710e33263d0847ef009e03.bitmap
      19:38:43.008754 pack-bitmap.c:543            | d0 | main                     | region_leave | r0  |  0.001191 |  0.000031 | pack-bitmap  | label:open_bitmap

Thanks.

Re: [PATCH v2 4/5] bitmap: add trace2 outputs during open "bitmap" file

[Jeff Hostetler]

On 4/21/22 9:26 AM, Teng Long wrote:
> It's supported for a Git repo to use bitmap in both normal bitmap way
> or a multi-pack-index bitmap.
> 
> Sometimes the debug path is not obvious, for example, when executing:
> 
>   $git rev-list  --test-bitmap  HEAD
>   fatal: failed to load bitmap indexes
> 
> If we see the output like this, we are not sure about what's happened,
> because the cause should be :
> 
>    1. neither normal nor midx bitmap exists
>    2. only midx bitmap exists but core.multipackIndex="false"
>    3. core.multipackIndex="true" but midx bitmap file is currupt
>    4. core.multipackIndex="true" and no midx bitmap exists but
>       normal bitmap file is currupt
>    ....
> 
> These are some of the scenarios I briefly tested, but maybe there are
> others (some scenarios is produced manually like "currupt bitmap file",
> but it's not represent it's an existed bug.).
> 
> Therefore, we added some TRACE2 code so that when we read the bitmap
> we can be more clear about the decision path, such as whether it is
> working on midx bitmap or normal bitmap, or is it simply because the
> related configuration is disabled. This may help with logging, user
> troubleshooting, and development debugging.
> 
> Here are some output examples when executing
> "$GIT_TRACE2_PERF=1 git rev-list --test-bitmap HEAD" under different
> situations:
> 
> 1. _HAVE_ MIDX bitmap and NORMAL bitmap file, but the
> "core.multipackIndex" configures as "false":
> 
>    19:38:43.007840 common-main.c:49             | d0 | main                     | version      |     |           |           |              | 2.35.1.582.g8e9092487a
>    19:38:43.007874 common-main.c:50             | d0 | main                     | start        |     |  0.000305 |           |              | /opt/git/master/bin/git rev-list --test-bitmap HEAD
>    19:38:43.007950 compat/linux/procinfo.c:170  | d0 | main                     | cmd_ancestry |     |           |           |              | ancestry:[bash sshd sshd sshd systemd]
>    19:38:43.008091 git.c:460                    | d0 | main                     | cmd_name     |     |           |           |              | rev-list (rev-list)
>    19:38:43.008284 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000720 |  0.000720 | config       | feature.manyfiles:false
>    19:38:43.008297 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000734 |  0.000734 | config       | feature.experimental:false
>    19:38:43.008305 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000742 |  0.000742 | config       | core.commitgraph:true
>    19:38:43.008313 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000749 |  0.000749 | config       | commitgraph.readchangedpaths:true
>    19:38:43.008320 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000756 |  0.000756 | config       | gc.writecommitgraph:true
>    19:38:43.008327 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000764 |  0.000764 | config       | fetch.writecommitgraph:false
>    19:38:43.008334 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000770 |  0.000770 | config       | pack.usesparse:true
>    19:38:43.008341 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000777 |  0.000777 | config       | core.multipackindex:false
>    19:38:43.008348 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000784 |  0.000784 | config       | index.sparse:false
>    19:38:43.008724 pack-bitmap.c:536            | d0 | main                     | region_enter | r0  |  0.001160 |           | pack-bitmap  | label:open_bitmap
>    19:38:43.008738 pack-bitmap.c:390            | d0 | main                     | data         | r0  |  0.001173 |  0.000013 | bitmap       | ..try to open bitmap:/home/tenglong.tl/test/dyrone/.git/objects/pack/pack-2d1b3f749fb859b874710e33263d0847ef009e03.bitmap
>    19:38:43.008754 pack-bitmap.c:543            | d0 | main                     | region_leave | r0  |  0.001191 |  0.000031 | pack-bitmap  | label:open_bitmap
>    Bitmap v1 test (8 entries loaded)
>    Found bitmap for 0a7df7ae92f8d8ed879c240f8ae9cdd33d18085e. 64 bits / 1801edc6 checksum
>    19:38:43.009099 progress.c:268               | d0 | main                     | region_enter | r0  |  0.001535 |           | progress     | label:Verifying bitmap entries
>    Verifying bitmap entries: 100% (27/27), done.
>    19:38:43.009294 progress.c:339               | d0 | main                     | data         | r0  |  0.001730 |  0.000195 | progress     | ..total_objects:27
>    19:38:43.009302 progress.c:346               | d0 | main                     | region_leave | r0  |  0.001739 |  0.000204 | progress     | label:Verifying bitmap entries
>    OK!
>    19:38:43.009321 git.c:718                    | d0 | main                     | exit         |     |  0.001757 |           |              | code:0
>    19:38:43.009329 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001766 |           |              | code:0
> 
> 2. _HAVE_ MIDX bitmap and NORMAL bitmap file, but the
> "core.multipackIndex" configures as "true":
> 
>    (omit duplicate outputs)
>    ...
>    20:02:31.288797 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000811 |  0.000811 | config       | core.multipackindex:true
>    20:02:31.288806 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000819 |  0.000819 | config       | index.sparse:false
>    20:02:31.288836 midx.c:185                   | d0 | main                     | data         | r0  |  0.000849 |  0.000849 | midx         | load/num_packs:1
>    20:02:31.288843 midx.c:186                   | d0 | main                     | data         | r0  |  0.000857 |  0.000857 | midx         | load/num_objects:27
>    20:02:31.289217 pack-bitmap.c:536            | d0 | main                     | region_enter | r0  |  0.001229 |           | pack-bitmap  | label:open_bitmap
>    20:02:31.289230 pack-bitmap.c:318            | d0 | main                     | data         | r0  |  0.001242 |  0.000013 | midx         | ..try to open bitmap:/home/tenglong.tl/test/dyrone/.git/objects/pack/multi-pack-index-b6b04fbe053bd500d9ca13354466d3249dc275ac.bitmap
>    20:02:31.289252 pack-revindex.c:315          | d0 | main                     | data         | r0  |  0.001265 |  0.000036 | load_midx_re | ..source:midx
>    20:02:31.289261 pack-bitmap.c:543            | d0 | main                     | region_leave | r0  |  0.001274 |  0.000045 | pack-bitmap  | label:open_bitmap
>    Bitmap v1 test (8 entries loaded)
>    Found bitmap for 0a7df7ae92f8d8ed879c240f8ae9cdd33d18085e. 64 bits / 1801edc6 checksum
>    20:02:31.289594 progress.c:268               | d0 | main                     | region_enter | r0  |  0.001607 |           | progress     | label:Verifying bitmap entries
>    Verifying bitmap entries: 100% (27/27), done.
>    20:02:31.289810 progress.c:339               | d0 | main                     | data         | r0  |  0.001823 |  0.000216 | progress     | ..total_objects:27
>    20:02:31.289824 progress.c:346               | d0 | main                     | region_leave | r0  |  0.001837 |  0.000230 | progress     | label:Verifying bitmap entries
>    OK!
>    20:02:31.289843 git.c:718                    | d0 | main                     | exit         |     |  0.001856 |           |              | code:0
>    20:02:31.289860 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001873 |           |              | code:0
> 
> 3. _HAVE_ MIDX bitmap and a corrupt NORMAL bitmap file, but the
> "core.multipackIndex" configures as "false" :
> 
>    (omit duplicate outputs)
>    ...
>    20:14:06.539305 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000794 |  0.000794 | config       | core.multipackindex:false
>    20:14:06.539310 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000799 |  0.000799 | config       | index.sparse:false
>    20:14:06.539658 pack-bitmap.c:536            | d0 | main                     | region_enter | r0  |  0.001146 |           | pack-bitmap  | label:open_bitmap
>    20:14:06.539671 pack-bitmap.c:390            | d0 | main                     | data         | r0  |  0.001160 |  0.000014 | bitmap       | ..try to open bitmap:/home/tenglong.tl/test/dyrone/.git/objects/pack/pack-2d1b3f749fb859b874710e33263d0847ef009e03.bitmap
>    20:14:06.539686 usage.c:79                   | d0 | main                     | error        |     |           |           |              | ..Corrupted bitmap index file (wrong header)
>    error: Corrupted bitmap index file (wrong header)
>    20:14:06.539696 pack-bitmap.c:426            | d0 | main                     | data         | r0  |  0.001185 |  0.000039 | bitmap       | ..load bitmap header:failed
>    20:14:06.539709 usage.c:79                   | d0 | main                     | error        |     |           |           |              | ..bitmap header is invalid
>    error: bitmap header is invalid
>    20:14:06.539719 pack-bitmap.c:543            | d0 | main                     | region_leave | r0  |  0.001208 |  0.000062 | pack-bitmap  | label:open_bitmap
>    20:14:06.539726 usage.c:60                   | d0 | main                     | error        |     |           |           |              | failed to load bitmap indexes
>    fatal: failed to load bitmap indexes
>    20:14:06.539735 usage.c:74                   | d0 | main                     | exit         |     |  0.001224 |           |              | code:128
>    20:14:06.539744 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001233 |           |              | code:128
> 
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>   pack-bitmap.c           | 40 +++++++++++++++++++++++++++++++++-------
>   repo-settings.c         |  1 +
>   t/t5310-pack-bitmaps.sh |  4 ++--
>   3 files changed, 36 insertions(+), 9 deletions(-)
> 
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 1b268f655e..a1d06c4252 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -312,9 +312,12 @@ char *pack_bitmap_filename(struct packed_git *p)
>   static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
>   			      struct multi_pack_index *midx)
>   {
> +	int fd;
>   	struct stat st;
>   	char *bitmap_name = midx_bitmap_filename(midx);
> -	int fd = git_open(bitmap_name);
> +	trace2_data_string("midx", the_repository, "try to open bitmap",
> +			   bitmap_name);
> +	fd = git_open(bitmap_name);

This might just be a style thing, but rather than logging the pathname
in a separate data_string message, you can use the _printf version of
region_enter and region_leave to also print the name of the
path -- like I did in read-cache.c for the "do_read_index"
calls.

... | region_enter | ... | index | label:do_read_index .git/index
...
... | region_leave | ... | index | label:do_read_index .git/index

As AEvar suggests in another message in this thread, I'm not sure if
you need the region timing here for reading the bitmap, but all of
the error and any other data messages will be bounded between the
region_enter and region_leave events and that might (or might not)
be helpful.


Also, I agree with AEvar's statements about using error() and getting
the trace2 error messages for free and not needing some of the
trace2_data_string() messages that you have later in this file.

I wonder if it would be worth adding the pathname of the invalid
file to those new error messages.  Granted you'll have it in the
trace2 log, but then you'll also get it on stderr if that would
be helpful.


Jeff

Re: [PATCH v2 4/5] bitmap: add trace2 outputs during open "bitmap" file

[Teng Long]

> This might just be a style thing, but rather than logging the pathname
> in a separate data_string message, you can use the _printf version of
> region_enter and region_leave to also print the name of the
> path -- like I did in read-cache.c for the "do_read_index"
> calls.
> 
> ... | region_enter | ... | index | label:do_read_index .git/index
> ...
> ... | region_leave | ... | index | label:do_read_index .git/index

Appreciate for the input about the _printf version, we can choose to
let the region_enter and region_leave to print the pathname by moving
the related "midx_bitmap_filename()" and "pack_bitmap_filename" at
front, but it's not enough because both midx and normal bitmap support
multiple opening, so it's likely we keep on the current way using
"trace2_data_string()" in "open_pack_bitmap_1()" and "open_midx_bitmap_1()"
is a simpler solution.

I'm not sure If I totally get the meaning about your suggestion,
so correct me if I understand you wrong.

> As AEvar suggests in another message in this thread, I'm not sure if
> you need the region timing here for reading the bitmap, but all of
> the error and any other data messages will be bounded between the
> region_enter and region_leave events and that might (or might not)
> be helpful.

I think it's needed in my opinion, the bounded between the region is
helpful, especially when we want to know the detailed debug info like
we do in "open_midx_bitmap_1()".  


> Also, I agree with AEvar's statements about using error() and getting
> the trace2 error messages for free and not needing some of the
> trace2_data_string() messages that you have later in this file.
> 
> I wonder if it would be worth adding the pathname of the invalid
> file to those new error messages.  Granted you'll have it in the
> trace2 log, but then you'll also get it on stderr if that would
> be helpful.

I think I will remove the redundant "trace2_data_string()" code when
it will return by "error()".


Thanks.

Re: [PATCH v2 4/5] bitmap: add trace2 outputs during open "bitmap" file

[Jeff Hostetler]

On 5/6/22 8:43 AM, Teng Long wrote:
>> This might just be a style thing, but rather than logging the pathname
>> in a separate data_string message, you can use the _printf version of
>> region_enter and region_leave to also print the name of the
>> path -- like I did in read-cache.c for the "do_read_index"
>> calls.
>>
>> ... | region_enter | ... | index | label:do_read_index .git/index
>> ...
>> ... | region_leave | ... | index | label:do_read_index .git/index
> 
> Appreciate for the input about the _printf version, we can choose to
> let the region_enter and region_leave to print the pathname by moving
> the related "midx_bitmap_filename()" and "pack_bitmap_filename" at
> front, but it's not enough because both midx and normal bitmap support
> multiple opening, so it's likely we keep on the current way using
> "trace2_data_string()" in "open_pack_bitmap_1()" and "open_midx_bitmap_1()"
> is a simpler solution.
> 
> I'm not sure If I totally get the meaning about your suggestion,
> so correct me if I understand you wrong.

That's fine.  I was assuming that you were only operating
on a single file at a time and opening, using, closing it
in a nicely bracketed fashion.  The region_enter and _leave
is good for capturing that.  But if you might be handling
both files concurrently, then your messages would be better
since we don't want mix or improperly nest the regions.

(I must admit that I haven't studied the bitmap code, so
I defer to your judgement here.)

> 
>> As AEvar suggests in another message in this thread, I'm not sure if
>> you need the region timing here for reading the bitmap, but all of
>> the error and any other data messages will be bounded between the
>> region_enter and region_leave events and that might (or might not)
>> be helpful.
> 
> I think it's needed in my opinion, the bounded between the region is
> helpful, especially when we want to know the detailed debug info like
> we do in "open_midx_bitmap_1()".
> 
> 
>> Also, I agree with AEvar's statements about using error() and getting
>> the trace2 error messages for free and not needing some of the
>> trace2_data_string() messages that you have later in this file.
>>
>> I wonder if it would be worth adding the pathname of the invalid
>> file to those new error messages.  Granted you'll have it in the
>> trace2 log, but then you'll also get it on stderr if that would
>> be helpful.
> 
> I think I will remove the redundant "trace2_data_string()" code when
> it will return by "error()".
> 
> 
> Thanks.
> 

[PATCH v2 5/5] pack-bitmap.c: using error() instead of silently returning -1

[Teng Long]

In "open_pack_bitmap_1()" and "open_midx_bitmap_1()", it's better to
return error() instead of "-1" when some unexpected error occurs like
"stat bitmap file failed", "bitmap header is invalid" or "checksum
mismatch", etc.

There are places where we do not replace, such as when the bitmap
does not exist (no bitmap in repository is allowed) or when another
bitmap has already been opened (in which case it should be a warning
rather than an error).

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index a1d06c4252..e0dcd06db3 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -328,7 +328,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 		trace2_data_string("midx", the_repository, "stat bitmap file",
 				   "failed");
 		close(fd);
-		return -1;
+		return error("cannot stat bitmap file");
 	}
 
 	if (bitmap_git->pack || bitmap_git->midx) {
@@ -374,7 +374,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 	bitmap_git->map_pos = 0;
 	bitmap_git->map = NULL;
 	bitmap_git->midx = NULL;
-	return -1;
+	return error("cannot open midx bitmap file");
 }
 
 static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git *packfile)
@@ -399,7 +399,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 		trace2_data_string("bitmap", the_repository, "stat bitmap file",
 				   "failed");
 		close(fd);
-		return -1;
+		return error("cannot stat bitmap file");
 	}
 
 	if (bitmap_git->pack || bitmap_git->midx) {
@@ -413,7 +413,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 	if (!is_pack_valid(packfile)) {
 		trace2_data_string("bitmap", the_repository, "packfile", "invalid");
 		close(fd);
-		return -1;
+		return error("packfile is invalid");
 	}
 
 	bitmap_git->pack = packfile;
@@ -430,7 +430,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 		bitmap_git->map_size = 0;
 		bitmap_git->map_pos = 0;
 		bitmap_git->pack = NULL;
-		return -1;
+		return error("bitmap header is invalid");
 	}
 
 	return 0;
-- 
2.35.1.583.g30faa5f068

Re: [PATCH v2 5/5] pack-bitmap.c: using error() instead of silently returning -1

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 21 2022, Teng Long wrote:

> In "open_pack_bitmap_1()" and "open_midx_bitmap_1()", it's better to
> return error() instead of "-1" when some unexpected error occurs like
> "stat bitmap file failed", "bitmap header is invalid" or "checksum
> mismatch", etc.
>
> There are places where we do not replace, such as when the bitmap
> does not exist (no bitmap in repository is allowed) or when another
> bitmap has already been opened (in which case it should be a warning
> rather than an error).
>
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>  pack-bitmap.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index a1d06c4252..e0dcd06db3 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -328,7 +328,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
>  		trace2_data_string("midx", the_repository, "stat bitmap file",
>  				   "failed");
>  		close(fd);
> -		return -1;
> +		return error("cannot stat bitmap file");
>  	}

First, I wondered if we were missing _(), but looking at other string in
the file they're not using that already, looks like these all should,
but we can fix this all up some other time in some i18n commit. It's
fine to keep this for now.

But more importantly: I think this should be your 4/5. I.e. just make
these an error() and you won't need to add e.g. this
trace2_data_string() for a failed stat.

You will be inside your trace2 region, so any failure to stat etc. will
be obvious from the structure of the data and the "error" event, no
reason to have an additional trace2_data_string().

Aside from that & as a general matter: Unless you have some use-case for
trace2 data in this detail I'd think that it would be better just to
skip logging it (except if we get it for free, such as with error()).

I.e. is this particular callsite really different from other places we
fail to stat() or open() something?

It's all a moot point with the region + error, but just something to
keep in mind.

Re: [PATCH v2 5/5] pack-bitmap.c: using error() instead of silently returning -1

[Teng Long]

On Thu, 21 Apr 2022 17:41:36 +0200, Ævar Arnfjörð Bjarmason wrote:

> First, I wondered if we were missing _(), but looking at other string in
> the file they're not using that already, looks like these all should,
> but we can fix this all up some other time in some i18n commit. It's
> fine to keep this for now.

Yes, I agree with you.

I also think I have a willingness to make another patchset to solve
the _() missing problems recently.

> But more importantly: I think this should be your 4/5. I.e. just make
> these an error() and you won't need to add e.g. this
> trace2_data_string() for a failed stat.

Make sense. Will adjust the order in next path.
 
> You will be inside your trace2 region, so any failure to stat etc. will
> be obvious from the structure of the data and the "error" event, no
> reason to have an additional trace2_data_string().

Yeah, I forgot about the "error()" already load the trace2 functions in.
Will remove the redundant trace2_data_string() where it's  obviously will
return error().

> Aside from that & as a general matter: Unless you have some use-case for
> trace2 data in this detail I'd think that it would be better just to
> skip logging it (except if we get it for free, such as with error()).
> 
> I.e. is this particular callsite really different from other places we
> fail to stat() or open() something?
> 
> It's all a moot point with the region + error, but just something to
> keep in mind.

Make sense.

And I think it's a good case in "open_midx_bitmap_1()" to add related
trace2_data_string() because there only a general error info in "cleanup:".


Thanks.

[PATCH v3 0/5]

[Teng Long]

Sorry for the late reply (in emergency project).

I reread the replies in patch v2 and fixed/optimized
in patch v3 in respective, I hope I haven't missed
anything.

Thanks.

Teng Long (5):
  pack-bitmap.c: continue looping when first MIDX bitmap is found
  pack-bitmap.c: rename "idx_name" to "bitmap_name"
  pack-bitmap.c: make warnings support i18N when opening bitmap
  pack-bitmap.c: using error() instead of silently returning -1
  bitmap: add trace2 outputs during open "bitmap" file

 pack-bitmap.c   | 58 +++++++++++++++++++++++++++++++------------------
 repo-settings.c |  1 +
 2 files changed, 38 insertions(+), 21 deletions(-)

Range-diff against v2:
1:  1bfd2fb6ab ! 1:  589e3f4075 pack-bitmap.c: continue looping when first MIDX bitmap is found
    @@ Commit message
         the first one has been found, then will break out by a "return"
         directly.
     
    -    But actually, it's better to don't stop the loop until we have visited
    +    But actually, it's better to continue the loop until we have visited
         both the MIDX in our repository, as well as any alternates (along with
         _their_ alternates, recursively).
     
    +    The reason for this is, there may exist more than one MIDX file in
    +    a repo. The "multi_pack_index" struct is actually designed as a singly
    +    linked list, and if a MIDX file has been already opened successfully,
    +    then the other MIDX files will be skipped and left with a warning
    +    "ignoring extra bitmap file." to the output.
    +
         The discussion link of community:
     
           https://public-inbox.org/git/YjzCTLLDCby+kJrZ@nand.local/
2:  1fff3b3ca7 ! 2:  b6b30047fc pack-bitmap.c: rename "idx_name" to "bitmap_name"
    @@ Commit message
     
         There may bring some confusion in this "idx_name" naming, which
         might lead us to think of ".idx "or" multi-pack-index" files,    -    although bitmap is essentially can be understood as a kind of index
    -    , let's define this name a little more accurate here.
    +    although bitmap is essentially can be understood as a kind of index,
    +    let's define this name a little more accurate here.
     
         Signed-off-by: Teng Long <dyroneteng@gmail.com>
     
3:  5a8f5afccf ! 3:  d8dfe53dd4 pack-bitmap.c: make warnings more detailed when opening bitmap
    @@ Metadata
     Author: Teng Long <dyroneteng@gmail.com>
     
      ## Commit message ##
    -    pack-bitmap.c: make warnings more detailed when opening bitmap
    +    pack-bitmap.c: make warnings support i18N when opening bitmap
     
         When calling the "open_midx_bitmap_1()" or "open_pack_bitmap_1()", there
         will be a warning if a normal bitmap or MIDX bitmap already has been
    -    opened, then let's make the warning information more detailed. For
    -    example, it makes the error clearer in case of an accidental
    -    regression where we start looking for single-pack bitmaps after
    -    successfully opening a multi-pack one.
    -
    -    At the same time, we made the previous and new warning texts support
    -    translation.
    +    opened, then let's make the warning text supporting for translation.
     
         Discussion in community:
     
    @@ pack-bitmap.c: static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
      		get_midx_filename(&buf, midx->object_dir);
      		/* ignore extra bitmap file; we can only handle one */
     -		warning("ignoring extra bitmap file: %s", buf.buf);
    -+		warning(_("a normal or midx bitmap already has been opened"));
     +		warning(_("ignoring extra bitmap file: %s"), buf.buf);
      		close(fd);
      		strbuf_release(&buf);
    @@ pack-bitmap.c: static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, st
      	if (bitmap_git->pack || bitmap_git->midx) {
      		/* ignore extra bitmap file; we can only handle one */
     -		warning("ignoring extra bitmap file: %s", packfile->pack_name);
    -+		warning(_("a normal or midx bitmap already has been opened "));
     +		warning(_("ignoring extra bitmap file: %s"), packfile->pack_name);
      		close(fd);
      		return -1;
      	}
    -
    - ## t/t5310-pack-bitmaps.sh ##
    -@@ t/t5310-pack-bitmaps.sh: test_expect_success 'complains about multiple pack bitmaps' '
    - 		test_line_count = 2 bitmaps &&
    - 
    - 		git rev-list --use-bitmap-index HEAD 2>err &&
    -+		grep "a bitmap has been opened" err &&
    - 		grep "ignoring extra bitmap file" err
    - 	)
    - '
5:  1a169d7b5e ! 4:  72da3b5844 pack-bitmap.c: using error() instead of silently returning -1
    @@ Commit message
     
      ## pack-bitmap.c ##
     @@ pack-bitmap.c: static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
    - 		trace2_data_string("midx", the_repository, "stat bitmap file",
    - 				   "failed");
    + 
    + 	if (fstat(fd, &st)) {
      		close(fd);
     -		return -1;
    -+		return error("cannot stat bitmap file");
    ++		return error(_("cannot stat bitmap file"));
      	}
      
      	if (bitmap_git->pack || bitmap_git->midx) {
    @@ pack-bitmap.c: static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
      
      static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git *packfile)
     @@ pack-bitmap.c: static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
    - 		trace2_data_string("bitmap", the_repository, "stat bitmap file",
    - 				   "failed");
    + 
    + 	if (fstat(fd, &st)) {
      		close(fd);
     -		return -1;
    -+		return error("cannot stat bitmap file");
    ++		return error(_("cannot stat bitmap file"));
      	}
      
      	if (bitmap_git->pack || bitmap_git->midx) {
     @@ pack-bitmap.c: static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
    + 
      	if (!is_pack_valid(packfile)) {
    - 		trace2_data_string("bitmap", the_repository, "packfile", "invalid");
      		close(fd);
     -		return -1;
    -+		return error("packfile is invalid");
    ++		return error(_("packfile is invalid"));
      	}
      
      	bitmap_git->pack = packfile;
    @@ pack-bitmap.c: static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, st
      		bitmap_git->map_pos = 0;
      		bitmap_git->pack = NULL;
     -		return -1;
    -+		return error("bitmap header is invalid");
    ++		return error(_("bitmap header is invalid"));
      	}
      
      	return 0;
4:  2016ef2e34 ! 5:  e118758d1d bitmap: add trace2 outputs during open "bitmap" file
    @@ Metadata
      ## Commit message ##
         bitmap: add trace2 outputs during open "bitmap" file
     
    -    It's supported for a Git repo to use bitmap in both normal bitmap way
    -    or a multi-pack-index bitmap.
    -
    -    Sometimes the debug path is not obvious, for example, when executing:
    +    It's supported for a repo to use bitmap in both "NORMAL" bitmap way
    +    or a MIDX (multi-pack-index) bitmap. Either of two bitmap kinds can
    +    exist in the repository, or both can be stored but let the config
    +    controls which kind of bitmap is used (like "core.multipackIndex",
    +    etc.). Because of this, sometimes the bitmap debug path is not
    +    obvious enough, for example, when executing:
     
          $git rev-list  --test-bitmap  HEAD
          fatal: failed to load bitmap indexes
     
    -    If we see the output like this, we are not sure about what's happened,
    -    because the cause should be :
    +    If we see the output like this, It's not sure for us to know
    +    what's happened concretely, because the cause should be :
     
    -      1. neither normal nor midx bitmap exists
    -      2. only midx bitmap exists but core.multipackIndex="false"
    -      3. core.multipackIndex="true" but midx bitmap file is currupt
    -      4. core.multipackIndex="true" and no midx bitmap exists but
    -         normal bitmap file is currupt
    +      1. Neither normal nor MIDX bitmap exists.
    +      2. Only MIDX bitmap exists but core.multipackIndex="false".
    +      3. Config core.multipackIndex set to "true" but MIDX  bitmap is
    +         corrupted.
    +      4. Config core.multipackIndex set to "true" and no MIDX bitmap
    +         exists but normal bitmap file is corrupted.
           ....
     
         These are some of the scenarios I briefly tested, but maybe there are
    -    others (some scenarios is produced manually like "currupt bitmap file",
    +    others (some scenarios is produced manually like "corrupted bitmap file",
         but it's not represent it's an existed bug.).
     
         Therefore, we added some TRACE2 code so that when we read the bitmap
         we can be more clear about the decision path, such as whether it is
    -    working on midx bitmap or normal bitmap, or is it simply because the
    -    related configuration is disabled. This may help with logging, user
    -    troubleshooting, and development debugging.
    -
    -    Here are some output examples when executing
    -    "$GIT_TRACE2_PERF=1 git rev-list --test-bitmap HEAD" under different
    -    situations:
    -
    -    1. _HAVE_ MIDX bitmap and NORMAL bitmap file, but the
    -    "core.multipackIndex" configures as "false":
    -
    -      19:38:43.007840 common-main.c:49             | d0 | main                     | version      |     |           |           |              | 2.35.1.582.g8e9092487a
    -      19:38:43.007874 common-main.c:50             | d0 | main                     | start        |     |  0.000305 |           |              | /opt/git/master/bin/git rev-list --test-bitmap HEAD
    -      19:38:43.007950 compat/linux/procinfo.c:170  | d0 | main                     | cmd_ancestry |     |           |           |              | ancestry:[bash sshd sshd sshd systemd]
    -      19:38:43.008091 git.c:460                    | d0 | main                     | cmd_name     |     |           |           |              | rev-list (rev-list)
    -      19:38:43.008284 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000720 |  0.000720 | config       | feature.manyfiles:false
    -      19:38:43.008297 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000734 |  0.000734 | config       | feature.experimental:false
    -      19:38:43.008305 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000742 |  0.000742 | config       | core.commitgraph:true
    -      19:38:43.008313 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000749 |  0.000749 | config       | commitgraph.readchangedpaths:true
    -      19:38:43.008320 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000756 |  0.000756 | config       | gc.writecommitgraph:true
    -      19:38:43.008327 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000764 |  0.000764 | config       | fetch.writecommitgraph:false
    -      19:38:43.008334 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000770 |  0.000770 | config       | pack.usesparse:true
    -      19:38:43.008341 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000777 |  0.000777 | config       | core.multipackindex:false
    -      19:38:43.008348 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000784 |  0.000784 | config       | index.sparse:false
    -      19:38:43.008724 pack-bitmap.c:536            | d0 | main                     | region_enter | r0  |  0.001160 |           | pack-bitmap  | label:open_bitmap
    -      19:38:43.008738 pack-bitmap.c:390            | d0 | main                     | data         | r0  |  0.001173 |  0.000013 | bitmap       | ..try to open bitmap:/home/tenglong.tl/test/dyrone/.git/objects/pack/pack-2d1b3f749fb859b874710e33263d0847ef009e03.bitmap
    -      19:38:43.008754 pack-bitmap.c:543            | d0 | main                     | region_leave | r0  |  0.001191 |  0.000031 | pack-bitmap  | label:open_bitmap
    -      Bitmap v1 test (8 entries loaded)
    -      Found bitmap for 0a7df7ae92f8d8ed879c240f8ae9cdd33d18085e. 64 bits / 1801edc6 checksum
    -      19:38:43.009099 progress.c:268               | d0 | main                     | region_enter | r0  |  0.001535 |           | progress     | label:Verifying bitmap entries
    -      Verifying bitmap entries: 100% (27/27), done.
    -      19:38:43.009294 progress.c:339               | d0 | main                     | data         | r0  |  0.001730 |  0.000195 | progress     | ..total_objects:27
    -      19:38:43.009302 progress.c:346               | d0 | main                     | region_leave | r0  |  0.001739 |  0.000204 | progress     | label:Verifying bitmap entries
    -      OK!
    -      19:38:43.009321 git.c:718                    | d0 | main                     | exit         |     |  0.001757 |           |              | code:0
    -      19:38:43.009329 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001766 |           |              | code:0
    -
    -    2. _HAVE_ MIDX bitmap and NORMAL bitmap file, but the
    -    "core.multipackIndex" configures as "true":
    -
    -      (omit duplicate outputs)
    -      ...
    -      20:02:31.288797 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000811 |  0.000811 | config       | core.multipackindex:true
    -      20:02:31.288806 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000819 |  0.000819 | config       | index.sparse:false
    -      20:02:31.288836 midx.c:185                   | d0 | main                     | data         | r0  |  0.000849 |  0.000849 | midx         | load/num_packs:1
    -      20:02:31.288843 midx.c:186                   | d0 | main                     | data         | r0  |  0.000857 |  0.000857 | midx         | load/num_objects:27
    -      20:02:31.289217 pack-bitmap.c:536            | d0 | main                     | region_enter | r0  |  0.001229 |           | pack-bitmap  | label:open_bitmap
    -      20:02:31.289230 pack-bitmap.c:318            | d0 | main                     | data         | r0  |  0.001242 |  0.000013 | midx         | ..try to open bitmap:/home/tenglong.tl/test/dyrone/.git/objects/pack/multi-pack-index-b6b04fbe053bd500d9ca13354466d3249dc275ac.bitmap
    -      20:02:31.289252 pack-revindex.c:315          | d0 | main                     | data         | r0  |  0.001265 |  0.000036 | load_midx_re | ..source:midx
    -      20:02:31.289261 pack-bitmap.c:543            | d0 | main                     | region_leave | r0  |  0.001274 |  0.000045 | pack-bitmap  | label:open_bitmap
    -      Bitmap v1 test (8 entries loaded)
    -      Found bitmap for 0a7df7ae92f8d8ed879c240f8ae9cdd33d18085e. 64 bits / 1801edc6 checksum
    -      20:02:31.289594 progress.c:268               | d0 | main                     | region_enter | r0  |  0.001607 |           | progress     | label:Verifying bitmap entries
    -      Verifying bitmap entries: 100% (27/27), done.
    -      20:02:31.289810 progress.c:339               | d0 | main                     | data         | r0  |  0.001823 |  0.000216 | progress     | ..total_objects:27
    -      20:02:31.289824 progress.c:346               | d0 | main                     | region_leave | r0  |  0.001837 |  0.000230 | progress     | label:Verifying bitmap entries
    -      OK!
    -      20:02:31.289843 git.c:718                    | d0 | main                     | exit         |     |  0.001856 |           |              | code:0
    -      20:02:31.289860 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001873 |           |              | code:0
    -
    -    3. _HAVE_ MIDX bitmap and a corrupt NORMAL bitmap file, but the
    -    "core.multipackIndex" configures as "false" :
    -
    -      (omit duplicate outputs)
    -      ...
    -      20:14:06.539305 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000794 |  0.000794 | config       | core.multipackindex:false
    -      20:14:06.539310 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000799 |  0.000799 | config       | index.sparse:false
    -      20:14:06.539658 pack-bitmap.c:536            | d0 | main                     | region_enter | r0  |  0.001146 |           | pack-bitmap  | label:open_bitmap
    -      20:14:06.539671 pack-bitmap.c:390            | d0 | main                     | data         | r0  |  0.001160 |  0.000014 | bitmap       | ..try to open bitmap:/home/tenglong.tl/test/dyrone/.git/objects/pack/pack-2d1b3f749fb859b874710e33263d0847ef009e03.bitmap
    -      20:14:06.539686 usage.c:79                   | d0 | main                     | error        |     |           |           |              | ..Corrupted bitmap index file (wrong header)
    -      error: Corrupted bitmap index file (wrong header)
    -      20:14:06.539696 pack-bitmap.c:426            | d0 | main                     | data         | r0  |  0.001185 |  0.000039 | bitmap       | ..load bitmap header:failed
    -      20:14:06.539709 usage.c:79                   | d0 | main                     | error        |     |           |           |              | ..bitmap header is invalid
    -      error: bitmap header is invalid
    -      20:14:06.539719 pack-bitmap.c:543            | d0 | main                     | region_leave | r0  |  0.001208 |  0.000062 | pack-bitmap  | label:open_bitmap
    -      20:14:06.539726 usage.c:60                   | d0 | main                     | error        |     |           |           |              | failed to load bitmap indexes
    -      fatal: failed to load bitmap indexes
    -      20:14:06.539735 usage.c:74                   | d0 | main                     | exit         |     |  0.001224 |           |              | code:128
    -      20:14:06.539744 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001233 |           |              | code:128
    +    working on MIDX or NORMAL bitmap at present, or the related config is
    +    enabled or not. This may help with logging, user troubleshooting, and
    +    development debugging.
    +
    +    Here are some brief output examples on different scenarios when
    +    executing:
    +
    +      $GIT_TRACE2_PERF=1 git rev-list --test-bitmap HEAD
    +
    +    Scenario 1: core.multipackIndex [false], midx bitmap exists [Y],
    +    normal bitmap exists [N]
    +
    +           19:21:56.580349 repo-settings.c:11           | d0 | main                     | data         | r1  |  0.000827 |  0.000827 | config       | core.multipackindex:false
    +           19:21:56.580356 repo-settings.c:11           | d0 | main                     | data         | r1  |  0.000834 |  0.000834 | config       | index.sparse:false
    +           19:21:56.580706 pack-bitmap.c:525            | d0 | main                     | region_enter | r1  |  0.001183 |           | pack-bitmap  | label:open_bitmap
    +           19:21:56.580719 pack-bitmap.c:386            | d0 | main                     | data         | r1  |  0.001196 |  0.000013 | bitmap       | ..path:.git/objects/pack/pack-e9eb18e6a423057f4424a762069e13804a75d01e.bitmap
    +           19:21:56.580729 pack-bitmap.c:530            | d0 | main                     | region_leave | r1  |  0.001207 |  0.000024 | pack-bitmap  | label:open_bitmap
    +           19:21:56.580737 usage.c:60                   | d0 | main                     | error        |     |           |           |              | failed to load bitmap indexes
    +           fatal: failed to load bitmap indexes
    +           19:21:56.580746 usage.c:74                   | d0 | main                     | exit         |     |  0.001224 |           |              | code:128
    +           19:21:56.580754 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001232 |           |              | code:128
    +
    +    Scenario 2: core.multipackIndex [false], midx bitmap exists [Y],
    +    normal bitmap exists [Y]
    +
    +            19:23:44.692384 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000765 |  0.000765 | config       | core.multipackindex:false
    +            19:23:44.692755 pack-bitmap.c:525            | d0 | main                     | region_enter | r0  |  0.001135 |           | pack-bitmap  | label:open_bitmap
    +            19:23:44.692768 pack-bitmap.c:386            | d0 | main                     | data         | r0  |  0.001149 |  0.000014 | bitmap       | ..path:/home/tenglong.tl/test/dyrone_bitmap/.git/objects/pack/pack-e9eb18e6a423057f4424a762069e13804a75d01e.bitmap
    +            19:23:44.692790 pack-bitmap.c:530            | d0 | main                     | region_leave | r0  |  0.001171 |  0.000036 | pack-bitmap  | label:open_bitmap
    +            Bitmap v1 test (1 entries loaded)
    +            Found bitmap for d864fefa87415d6cd289c72aa9ffd45b4a8ffd84. 64 bits / 11030517 checksum
    +            19:23:44.693119 progress.c:268               | d0 | main                     | region_enter | r0  |  0.001500 |           | progress     | label:Verifying bitmap entries
    +            Verifying bitmap entries: 100% (3/3), done.
    +            19:23:44.693208 progress.c:339               | d0 | main                     | data         | r0  |  0.001589 |  0.000089 | progress     | ..total_objects:3
    +            19:23:44.693216 progress.c:346               | d0 | main                     | region_leave | r0  |  0.001597 |  0.000097 | progress     | label:Verifying bitmap entries
    +            OK!
    +            19:23:44.693234 git.c:718                    | d0 | main                     | exit         |     |  0.001615 |           |              | code:0
    +            19:23:44.693244 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001625 |           |              | code:0
    +
    +    Scenario 3: core.multipackIndex [true], midx bitmap exists [Y],
    +    normal bitmap exists [Y]
    +
    +            19:26:03.625055 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000760 |  0.000760 | config       | core.multipackindex:true
    +            19:26:03.625090 midx.c:185                   | d0 | main                     | data         | r0  |  0.000795 |  0.000795 | midx         | load/num_packs:1
    +            19:26:03.625097 midx.c:186                   | d0 | main                     | data         | r0  |  0.000803 |  0.000803 | midx         | load/num_objects:3
    +            19:26:03.625455 pack-bitmap.c:525            | d0 | main                     | region_enter | r0  |  0.001160 |           | pack-bitmap  | label:open_bitmap
    +            19:26:03.625470 pack-bitmap.c:318            | d0 | main                     | data         | r0  |  0.001175 |  0.000015 | midx         | ..path:/home/tenglong.tl/test/dyrone_bitmap/.git/objects/pack/multi-pack-index-fe8e96790bd34926423bdf3efd762dbbea9f3213.bitmap
    +            19:26:03.625489 pack-revindex.c:315          | d0 | main                     | data         | r0  |  0.001194 |  0.000034 | load_midx_re | ..source:midx
    +            19:26:03.625496 pack-bitmap.c:530            | d0 | main                     | region_leave | r0  |  0.001202 |  0.000042 | pack-bitmap  | label:open_bitmap
    +            Bitmap v1 test (1 entries loaded)
    +            Found bitmap for d864fefa87415d6cd289c72aa9ffd45b4a8ffd84. 64 bits / 11030517 checksum
    +            19:26:03.625818 progress.c:268               | d0 | main                     | region_enter | r0  |  0.001523 |           | progress     | label:Verifying bitmap entries
    +            Verifying bitmap entries: 100% (3/3), done.
    +            19:26:03.625916 progress.c:339               | d0 | main                     | data         | r0  |  0.001622 |  0.000099 | progress     | ..total_objects:3
    +            19:26:03.625925 progress.c:346               | d0 | main                     | region_leave | r0  |  0.001630 |  0.000107 | progress     | label:Verifying bitmap entries
    +            OK!
    +            19:26:03.625943 git.c:718                    | d0 | main                     | exit         |     |  0.001648 |           |              | code:0
    +            19:26:03.625952 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001658 |           |              | code:0
    +
    +    Situation 4: core.multipackIndex [false], midx bitmap exists [N],
    +    normal bitmap exists [Y].
    +
    +            19:27:15.383037 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000746 |  0.000746 | config       | core.multipackindex:true
    +            19:27:15.383397 pack-bitmap.c:525            | d0 | main                     | region_enter | r0  |  0.001105 |           | pack-bitmap  | label:open_bitmap
    +            19:27:15.383408 pack-bitmap.c:386            | d0 | main                     | data         | r0  |  0.001116 |  0.000011 | bitmap       | ..path:/home/tenglong.tl/test/dyrone_bitmap/.git/objects/pack/pack-e9eb18e6a423057f4424a762069e13804a75d01e.bitmap
    +            19:27:15.383419 pack-bitmap.c:530            | d0 | main                     | region_leave | r0  |  0.001128 |  0.000023 | pack-bitmap  | label:open_bitmap
    +            Bitmap v1 test (1 entries loaded)
    +            Found bitmap for d864fefa87415d6cd289c72aa9ffd45b4a8ffd84. 64 bits / 11030517 checksum
    +            19:27:15.383730 progress.c:268               | d0 | main                     | region_enter | r0  |  0.001439 |           | progress     | label:Verifying bitmap entries
    +            Verifying bitmap entries: 100% (3/3), done.
    +            19:27:15.383822 progress.c:339               | d0 | main                     | data         | r0  |  0.001531 |  0.000092 | progress     | ..total_objects:3
    +            19:27:15.383830 progress.c:346               | d0 | main                     | region_leave | r0  |  0.001539 |  0.000100 | progress     | label:Verifying bitmap entries
    +            OK!
    +            19:27:15.383848 git.c:718                    | d0 | main                     | exit         |     |  0.001557 |           |              | code:0
    +            19:27:15.383867 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001576 |           |              | code:0
    +
    +    Scenario 5: core.multipackIndex [true], midx bitmap exists [Y] but corrupted,
    +    normal bitmap exists [Y]
    +
    +            19:29:25.888233 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000794 |  0.000794 | config       | core.multipackindex:true
    +            19:29:25.888591 pack-bitmap.c:525            | d0 | main                     | region_enter | r0  |  0.001152 |           | pack-bitmap  | label:open_bitmap
    +            19:29:25.888603 pack-bitmap.c:386            | d0 | main                     | data         | r0  |  0.001163 |  0.000011 | bitmap       | ..path:/home/tenglong.tl/test/dyrone_bitmap/.git/objects/pack/pack-e9eb18e6a423057f4424a762069e13804a75d01e.bitmap
    +            19:29:25.888622 usage.c:79                   | d0 | main                     | error        |     |           |           |              | ..Corrupted bitmap index file (wrong header)
    +            error: Corrupted bitmap index file (wrong header)
    +            19:29:25.888638 usage.c:79                   | d0 | main                     | error        |     |           |           |              | ..bitmap header is invalid
    +            error: bitmap header is invalid
    +            19:29:25.888650 pack-bitmap.c:530            | d0 | main                     | region_leave | r0  |  0.001211 |  0.000059 | pack-bitmap  | label:open_bitmap
    +            19:29:25.888659 usage.c:60                   | d0 | main                     | error        |     |           |           |              | failed to load bitmap indexes
    +            fatal: failed to load bitmap indexes
    +            19:29:25.888670 usage.c:74                   | d0 | main                     | exit         |     |  0.001231 |           |              | code:128
    +            19:29:25.888680 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001241 |           |              | code:128
     
         Signed-off-by: Teng Long <dyroneteng@gmail.com>
     
    @@ pack-bitmap.c: char *pack_bitmap_filename(struct packed_git *p)
      	struct stat st;
      	char *bitmap_name = midx_bitmap_filename(midx);
     -	int fd = git_open(bitmap_name);
    -+	trace2_data_string("midx", the_repository, "try to open bitmap",
    -+			   bitmap_name);
    ++	trace2_data_string("midx", the_repository, "path", bitmap_name);
     +	fd = git_open(bitmap_name);
      
      	free(bitmap_name);
      
    -@@ pack-bitmap.c: static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
    - 		return -1;
    - 
    - 	if (fstat(fd, &st)) {
    -+		trace2_data_string("midx", the_repository, "stat bitmap file",
    -+				   "failed");
    - 		close(fd);
    - 		return -1;
    - 	}
     @@ pack-bitmap.c: static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
      				MAP_PRIVATE, fd, 0);
      	close(fd);
    @@ pack-bitmap.c: static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, st
      		return -1;
      
      	bitmap_name = pack_bitmap_filename(packfile);
    -+	trace2_data_string("bitmap", the_repository, "try to open bitmap",
    -+			   bitmap_name);
    ++	trace2_data_string("bitmap", the_repository, "path", bitmap_name);
      	fd = git_open(bitmap_name);
      	free(bitmap_name);
      
    -@@ pack-bitmap.c: static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
    - 		return -1;
    - 
    - 	if (fstat(fd, &st)) {
    -+		trace2_data_string("bitmap", the_repository, "stat bitmap file",
    -+				   "failed");
    - 		close(fd);
    - 		return -1;
    - 	}
    -@@ pack-bitmap.c: static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
    - 	}
    - 
    - 	if (!is_pack_valid(packfile)) {
    -+		trace2_data_string("bitmap", the_repository, "packfile", "invalid");
    - 		close(fd);
    - 		return -1;
    - 	}
    -@@ pack-bitmap.c: static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
    - 	close(fd);
    - 
    - 	if (load_bitmap_header(bitmap_git) < 0) {
    -+		trace2_data_string("bitmap", the_repository,
    -+				   "load bitmap header", "failed");
    - 		munmap(bitmap_git->map, bitmap_git->map_size);
    - 		bitmap_git->map = NULL;
    - 		bitmap_git->map_size = 0;
     @@ pack-bitmap.c: static int open_midx_bitmap(struct repository *r,
      static int open_bitmap(struct repository *r,
      		       struct bitmap_index *bitmap_git)
      {
     -	assert(!bitmap_git->map);
    -+	int ret = -1;
    ++	int ret = 0;
      
    --	if (!open_midx_bitmap(r, bitmap_git))
    --		return 0;
    --	return open_pack_bitmap(r, bitmap_git);
     +	assert(!bitmap_git->map);
     +	trace2_region_enter("pack-bitmap", "open_bitmap", r);
    -+	if (!open_midx_bitmap(r, bitmap_git)) {
    -+		ret = 0;
    + 	if (!open_midx_bitmap(r, bitmap_git))
    +-		return 0;
    +-	return open_pack_bitmap(r, bitmap_git);
     +		goto done;
    -+	}
     +	ret = open_pack_bitmap(r, bitmap_git);
     +done:
     +	trace2_region_leave("pack-bitmap", "open_bitmap", r);
    @@ repo-settings.c: static void repo_cfg_bool(struct repository *r, const char *key
      }
      
      void prepare_repo_settings(struct repository *r)
    -
    - ## t/t5310-pack-bitmaps.sh ##
    -@@ t/t5310-pack-bitmaps.sh: test_expect_success 'complains about multiple pack bitmaps' '
    - 		test_line_count = 2 bitmaps &&
    - 
    - 		git rev-list --use-bitmap-index HEAD 2>err &&
    --		grep "a bitmap has been opened" err &&
    --		grep "ignoring extra bitmap file" err
    -+		grep "warning: a normal or midx bitmap already has been opened" err &&
    -+		grep "warning: ignoring extra bitmap file" err
    - 	)
    - '
    - 
-- 
2.35.1.582.g320e881567

[PATCH v3 1/5] pack-bitmap.c: continue looping when first MIDX bitmap is found

[Teng Long]

In "open_midx_bitmap()", we do a loop with the MIDX(es) in repo, when
the first one has been found, then will break out by a "return"
directly.

But actually, it's better to continue the loop until we have visited
both the MIDX in our repository, as well as any alternates (along with
_their_ alternates, recursively).

The reason for this is, there may exist more than one MIDX file in
a repo. The "multi_pack_index" struct is actually designed as a singly
linked list, and if a MIDX file has been already opened successfully,
then the other MIDX files will be skipped and left with a warning
"ignoring extra bitmap file." to the output.

The discussion link of community:

  https://public-inbox.org/git/YjzCTLLDCby+kJrZ@nand.local/

Helped-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 9c666cdb8b..112c2b12c6 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -494,15 +494,16 @@ 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;
+			ret = 0;
 	}
-	return -1;
+	return ret;
 }
 
 static int open_bitmap(struct repository *r,
-- 
2.35.1.582.g320e881567

[PATCH v3 2/5] pack-bitmap.c: rename "idx_name" to "bitmap_name"

[Teng Long]

In "open_pack_bitmap_1()" and "open_midx_bitmap_1()" we use
a var named "idx_name" to represent the bitmap filename which
is computed by "midx_bitmap_filename()" or "pack_bitmap_filename()"
before we open it.

There may bring some confusion in this "idx_name" naming, which
might lead us to think of ".idx "or" multi-pack-index" files,
although bitmap is essentially can be understood as a kind of index,
let's define this name a little more accurate here.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 112c2b12c6..f13a6bfe3a 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -313,10 +313,10 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 			      struct multi_pack_index *midx)
 {
 	struct stat st;
-	char *idx_name = midx_bitmap_filename(midx);
-	int fd = git_open(idx_name);
+	char *bitmap_name = midx_bitmap_filename(midx);
+	int fd = git_open(bitmap_name);
 
-	free(idx_name);
+	free(bitmap_name);
 
 	if (fd < 0)
 		return -1;
@@ -368,14 +368,14 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 {
 	int fd;
 	struct stat st;
-	char *idx_name;
+	char *bitmap_name;
 
 	if (open_pack_index(packfile))
 		return -1;
 
-	idx_name = pack_bitmap_filename(packfile);
-	fd = git_open(idx_name);
-	free(idx_name);
+	bitmap_name = pack_bitmap_filename(packfile);
+	fd = git_open(bitmap_name);
+	free(bitmap_name);
 
 	if (fd < 0)
 		return -1;
-- 
2.35.1.582.g320e881567

[PATCH v3 3/5] pack-bitmap.c: make warnings support i18N when opening bitmap

[Teng Long]

When calling the "open_midx_bitmap_1()" or "open_pack_bitmap_1()", there
will be a warning if a normal bitmap or MIDX bitmap already has been
opened, then let's make the warning text supporting for translation.

Discussion in community:

  1. https://public-inbox.org/git/YkPGq0mDL4NG6D1o@nand.local/#t
  2. https://public-inbox.org/git/220330.868rsrpohm.gmgdl@evledraar.gmail.com/

Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Helped-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index f13a6bfe3a..af0f41833e 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -330,7 +330,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 		struct strbuf buf = STRBUF_INIT;
 		get_midx_filename(&buf, midx->object_dir);
 		/* ignore extra bitmap file; we can only handle one */
-		warning("ignoring extra bitmap file: %s", buf.buf);
+		warning(_("ignoring extra bitmap file: %s"), buf.buf);
 		close(fd);
 		strbuf_release(&buf);
 		return -1;
@@ -387,7 +387,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 
 	if (bitmap_git->pack || bitmap_git->midx) {
 		/* ignore extra bitmap file; we can only handle one */
-		warning("ignoring extra bitmap file: %s", packfile->pack_name);
+		warning(_("ignoring extra bitmap file: %s"), packfile->pack_name);
 		close(fd);
 		return -1;
 	}
-- 
2.35.1.582.g320e881567

[PATCH v3 4/5] pack-bitmap.c: using error() instead of silently returning -1

[Teng Long]

In "open_pack_bitmap_1()" and "open_midx_bitmap_1()", it's better to
return error() instead of "-1" when some unexpected error occurs like
"stat bitmap file failed", "bitmap header is invalid" or "checksum
mismatch", etc.

There are places where we do not replace, such as when the bitmap
does not exist (no bitmap in repository is allowed) or when another
bitmap has already been opened (in which case it should be a warning
rather than an error).

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index af0f41833e..5654eb7b8d 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -323,7 +323,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 
 	if (fstat(fd, &st)) {
 		close(fd);
-		return -1;
+		return error(_("cannot stat bitmap file"));
 	}
 
 	if (bitmap_git->pack || bitmap_git->midx) {
@@ -361,7 +361,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 	bitmap_git->map_pos = 0;
 	bitmap_git->map = NULL;
 	bitmap_git->midx = NULL;
-	return -1;
+	return error("cannot open midx bitmap file");
 }
 
 static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git *packfile)
@@ -382,7 +382,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 
 	if (fstat(fd, &st)) {
 		close(fd);
-		return -1;
+		return error(_("cannot stat bitmap file"));
 	}
 
 	if (bitmap_git->pack || bitmap_git->midx) {
@@ -394,7 +394,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 
 	if (!is_pack_valid(packfile)) {
 		close(fd);
-		return -1;
+		return error(_("packfile is invalid"));
 	}
 
 	bitmap_git->pack = packfile;
@@ -409,7 +409,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 		bitmap_git->map_size = 0;
 		bitmap_git->map_pos = 0;
 		bitmap_git->pack = NULL;
-		return -1;
+		return error(_("bitmap header is invalid"));
 	}
 
 	return 0;
-- 
2.35.1.582.g320e881567

Re: [PATCH v3 4/5] pack-bitmap.c: using error() instead of silently returning -1

[Taylor Blau]

On Sun, Jun 12, 2022 at 03:44:19PM +0800, Teng Long wrote:
> @@ -323,7 +323,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
>
>  	if (fstat(fd, &st)) {
>  		close(fd);
> -		return -1;
> +		return error(_("cannot stat bitmap file"));

Since we are handling an error from fstat here, the errno variable
contains useful information that we should include in the error via
error_errno().

> @@ -361,7 +361,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
>  	bitmap_git->map_pos = 0;
>  	bitmap_git->map = NULL;
>  	bitmap_git->midx = NULL;
> -	return -1;
> +	return error("cannot open midx bitmap file");

The other error strings are marked for translation, but this one is not.
Was that intentional, or just a typo / oversight?

>  static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git *packfile)
> @@ -382,7 +382,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
>
>  	if (fstat(fd, &st)) {
>  		close(fd);
> -		return -1;
> +		return error(_("cannot stat bitmap file"));

Same note here about using error_errno() instead of just error().

Thanks,
Taylor

Re: [PATCH v3 4/5] pack-bitmap.c: using error() instead of silently returning -1

[Teng Long]

On Mon, 13 Jun 2022 21:15:51 -0400, Taylor Blau wrote:

> Since we are handling an error from fstat here, the errno variable
> contains useful information that we should include in the error via
> error_errno().

Appreciate for reminding this.
Will fix.

> The other error strings are marked for translation, but this one is not.
> Was that intentional, or just a typo / oversight?

It's an oversight.
Will fix.

> Same note here about using error_errno() instead of just error().

As same as the first one, will fix.

Thanks.

[PATCH v3 5/5] bitmap: add trace2 outputs during open "bitmap" file

[Teng Long]

It's supported for a repo to use bitmap in both "NORMAL" bitmap way
or a MIDX (multi-pack-index) bitmap. Either of two bitmap kinds can
exist in the repository, or both can be stored but let the config
controls which kind of bitmap is used (like "core.multipackIndex",
etc.). Because of this, sometimes the bitmap debug path is not
obvious enough, for example, when executing:

 $git rev-list  --test-bitmap  HEAD
 fatal: failed to load bitmap indexes

If we see the output like this, It's not sure for us to know
what's happened concretely, because the cause should be :

  1. Neither normal nor MIDX bitmap exists.
  2. Only MIDX bitmap exists but core.multipackIndex="false".
  3. Config core.multipackIndex set to "true" but MIDX  bitmap is
     corrupted.
  4. Config core.multipackIndex set to "true" and no MIDX bitmap
     exists but normal bitmap file is corrupted.
  ....

These are some of the scenarios I briefly tested, but maybe there are
others (some scenarios is produced manually like "corrupted bitmap file",
but it's not represent it's an existed bug.).

Therefore, we added some TRACE2 code so that when we read the bitmap
we can be more clear about the decision path, such as whether it is
working on MIDX or NORMAL bitmap at present, or the related config is
enabled or not. This may help with logging, user troubleshooting, and
development debugging.

Here are some brief output examples on different scenarios when
executing:

  $GIT_TRACE2_PERF=1 git rev-list --test-bitmap HEAD

Scenario 1: core.multipackIndex [false], midx bitmap exists [Y],
normal bitmap exists [N]

       19:21:56.580349 repo-settings.c:11           | d0 | main                     | data         | r1  |  0.000827 |  0.000827 | config       | core.multipackindex:false
       19:21:56.580356 repo-settings.c:11           | d0 | main                     | data         | r1  |  0.000834 |  0.000834 | config       | index.sparse:false
       19:21:56.580706 pack-bitmap.c:525            | d0 | main                     | region_enter | r1  |  0.001183 |           | pack-bitmap  | label:open_bitmap
       19:21:56.580719 pack-bitmap.c:386            | d0 | main                     | data         | r1  |  0.001196 |  0.000013 | bitmap       | ..path:.git/objects/pack/pack-e9eb18e6a423057f4424a762069e13804a75d01e.bitmap
       19:21:56.580729 pack-bitmap.c:530            | d0 | main                     | region_leave | r1  |  0.001207 |  0.000024 | pack-bitmap  | label:open_bitmap
       19:21:56.580737 usage.c:60                   | d0 | main                     | error        |     |           |           |              | failed to load bitmap indexes
       fatal: failed to load bitmap indexes
       19:21:56.580746 usage.c:74                   | d0 | main                     | exit         |     |  0.001224 |           |              | code:128
       19:21:56.580754 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001232 |           |              | code:128

Scenario 2: core.multipackIndex [false], midx bitmap exists [Y],
normal bitmap exists [Y]

	19:23:44.692384 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000765 |  0.000765 | config       | core.multipackindex:false
	19:23:44.692755 pack-bitmap.c:525            | d0 | main                     | region_enter | r0  |  0.001135 |           | pack-bitmap  | label:open_bitmap
	19:23:44.692768 pack-bitmap.c:386            | d0 | main                     | data         | r0  |  0.001149 |  0.000014 | bitmap       | ..path:/home/tenglong.tl/test/dyrone_bitmap/.git/objects/pack/pack-e9eb18e6a423057f4424a762069e13804a75d01e.bitmap
	19:23:44.692790 pack-bitmap.c:530            | d0 | main                     | region_leave | r0  |  0.001171 |  0.000036 | pack-bitmap  | label:open_bitmap
	Bitmap v1 test (1 entries loaded)
	Found bitmap for d864fefa87415d6cd289c72aa9ffd45b4a8ffd84. 64 bits / 11030517 checksum
	19:23:44.693119 progress.c:268               | d0 | main                     | region_enter | r0  |  0.001500 |           | progress     | label:Verifying bitmap entries
	Verifying bitmap entries: 100% (3/3), done.
	19:23:44.693208 progress.c:339               | d0 | main                     | data         | r0  |  0.001589 |  0.000089 | progress     | ..total_objects:3
	19:23:44.693216 progress.c:346               | d0 | main                     | region_leave | r0  |  0.001597 |  0.000097 | progress     | label:Verifying bitmap entries
	OK!
	19:23:44.693234 git.c:718                    | d0 | main                     | exit         |     |  0.001615 |           |              | code:0
	19:23:44.693244 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001625 |           |              | code:0

Scenario 3: core.multipackIndex [true], midx bitmap exists [Y],
normal bitmap exists [Y]

	19:26:03.625055 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000760 |  0.000760 | config       | core.multipackindex:true
	19:26:03.625090 midx.c:185                   | d0 | main                     | data         | r0  |  0.000795 |  0.000795 | midx         | load/num_packs:1
	19:26:03.625097 midx.c:186                   | d0 | main                     | data         | r0  |  0.000803 |  0.000803 | midx         | load/num_objects:3
	19:26:03.625455 pack-bitmap.c:525            | d0 | main                     | region_enter | r0  |  0.001160 |           | pack-bitmap  | label:open_bitmap
	19:26:03.625470 pack-bitmap.c:318            | d0 | main                     | data         | r0  |  0.001175 |  0.000015 | midx         | ..path:/home/tenglong.tl/test/dyrone_bitmap/.git/objects/pack/multi-pack-index-fe8e96790bd34926423bdf3efd762dbbea9f3213.bitmap
	19:26:03.625489 pack-revindex.c:315          | d0 | main                     | data         | r0  |  0.001194 |  0.000034 | load_midx_re | ..source:midx
	19:26:03.625496 pack-bitmap.c:530            | d0 | main                     | region_leave | r0  |  0.001202 |  0.000042 | pack-bitmap  | label:open_bitmap
	Bitmap v1 test (1 entries loaded)
	Found bitmap for d864fefa87415d6cd289c72aa9ffd45b4a8ffd84. 64 bits / 11030517 checksum
	19:26:03.625818 progress.c:268               | d0 | main                     | region_enter | r0  |  0.001523 |           | progress     | label:Verifying bitmap entries
	Verifying bitmap entries: 100% (3/3), done.
	19:26:03.625916 progress.c:339               | d0 | main                     | data         | r0  |  0.001622 |  0.000099 | progress     | ..total_objects:3
	19:26:03.625925 progress.c:346               | d0 | main                     | region_leave | r0  |  0.001630 |  0.000107 | progress     | label:Verifying bitmap entries
	OK!
	19:26:03.625943 git.c:718                    | d0 | main                     | exit         |     |  0.001648 |           |              | code:0
	19:26:03.625952 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001658 |           |              | code:0

Situation 4: core.multipackIndex [false], midx bitmap exists [N],
normal bitmap exists [Y].

	19:27:15.383037 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000746 |  0.000746 | config       | core.multipackindex:true
	19:27:15.383397 pack-bitmap.c:525            | d0 | main                     | region_enter | r0  |  0.001105 |           | pack-bitmap  | label:open_bitmap
	19:27:15.383408 pack-bitmap.c:386            | d0 | main                     | data         | r0  |  0.001116 |  0.000011 | bitmap       | ..path:/home/tenglong.tl/test/dyrone_bitmap/.git/objects/pack/pack-e9eb18e6a423057f4424a762069e13804a75d01e.bitmap
	19:27:15.383419 pack-bitmap.c:530            | d0 | main                     | region_leave | r0  |  0.001128 |  0.000023 | pack-bitmap  | label:open_bitmap
	Bitmap v1 test (1 entries loaded)
	Found bitmap for d864fefa87415d6cd289c72aa9ffd45b4a8ffd84. 64 bits / 11030517 checksum
	19:27:15.383730 progress.c:268               | d0 | main                     | region_enter | r0  |  0.001439 |           | progress     | label:Verifying bitmap entries
	Verifying bitmap entries: 100% (3/3), done.
	19:27:15.383822 progress.c:339               | d0 | main                     | data         | r0  |  0.001531 |  0.000092 | progress     | ..total_objects:3
	19:27:15.383830 progress.c:346               | d0 | main                     | region_leave | r0  |  0.001539 |  0.000100 | progress     | label:Verifying bitmap entries
	OK!
	19:27:15.383848 git.c:718                    | d0 | main                     | exit         |     |  0.001557 |           |              | code:0
	19:27:15.383867 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001576 |           |              | code:0

Scenario 5: core.multipackIndex [true], midx bitmap exists [Y] but corrupted,
normal bitmap exists [Y]

	19:29:25.888233 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000794 |  0.000794 | config       | core.multipackindex:true
	19:29:25.888591 pack-bitmap.c:525            | d0 | main                     | region_enter | r0  |  0.001152 |           | pack-bitmap  | label:open_bitmap
	19:29:25.888603 pack-bitmap.c:386            | d0 | main                     | data         | r0  |  0.001163 |  0.000011 | bitmap       | ..path:/home/tenglong.tl/test/dyrone_bitmap/.git/objects/pack/pack-e9eb18e6a423057f4424a762069e13804a75d01e.bitmap
	19:29:25.888622 usage.c:79                   | d0 | main                     | error        |     |           |           |              | ..Corrupted bitmap index file (wrong header)
	error: Corrupted bitmap index file (wrong header)
	19:29:25.888638 usage.c:79                   | d0 | main                     | error        |     |           |           |              | ..bitmap header is invalid
	error: bitmap header is invalid
	19:29:25.888650 pack-bitmap.c:530            | d0 | main                     | region_leave | r0  |  0.001211 |  0.000059 | pack-bitmap  | label:open_bitmap
	19:29:25.888659 usage.c:60                   | d0 | main                     | error        |     |           |           |              | failed to load bitmap indexes
	fatal: failed to load bitmap indexes
	19:29:25.888670 usage.c:74                   | d0 | main                     | exit         |     |  0.001231 |           |              | code:128
	19:29:25.888680 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001241 |           |              | code:128

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c   | 27 +++++++++++++++++++++------
 repo-settings.c |  1 +
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 5654eb7b8d..ced5993560 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -312,9 +312,11 @@ char *pack_bitmap_filename(struct packed_git *p)
 static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 			      struct multi_pack_index *midx)
 {
+	int fd;
 	struct stat st;
 	char *bitmap_name = midx_bitmap_filename(midx);
-	int fd = git_open(bitmap_name);
+	trace2_data_string("midx", the_repository, "path", bitmap_name);
+	fd = git_open(bitmap_name);
 
 	free(bitmap_name);
 
@@ -343,12 +345,19 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 				MAP_PRIVATE, fd, 0);
 	close(fd);
 
-	if (load_bitmap_header(bitmap_git) < 0)
+	if (load_bitmap_header(bitmap_git) < 0) {
+		trace2_data_string("midx", the_repository, "load bitmap header",
+				   "failed");
 		goto cleanup;
+	}
 
-	if (!hasheq(get_midx_checksum(bitmap_git->midx), bitmap_git->checksum))
+	if (!hasheq(get_midx_checksum(bitmap_git->midx), bitmap_git->checksum)) {
+		trace2_data_string("midx", the_repository, "verify checksum",
+				   "mismatch");
 		goto cleanup;
 
+	}
+
 	if (load_midx_revindex(bitmap_git->midx) < 0) {
 		warning(_("multi-pack bitmap is missing required reverse index"));
 		goto cleanup;
@@ -374,6 +383,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 		return -1;
 
 	bitmap_name = pack_bitmap_filename(packfile);
+	trace2_data_string("bitmap", the_repository, "path", bitmap_name);
 	fd = git_open(bitmap_name);
 	free(bitmap_name);
 
@@ -509,11 +519,16 @@ static int open_midx_bitmap(struct repository *r,
 static int open_bitmap(struct repository *r,
 		       struct bitmap_index *bitmap_git)
 {
-	assert(!bitmap_git->map);
+	int ret = 0;
 
+	assert(!bitmap_git->map);
+	trace2_region_enter("pack-bitmap", "open_bitmap", r);
 	if (!open_midx_bitmap(r, bitmap_git))
-		return 0;
-	return open_pack_bitmap(r, bitmap_git);
+		goto done;
+	ret = open_pack_bitmap(r, bitmap_git);
+done:
+	trace2_region_leave("pack-bitmap", "open_bitmap", r);
+	return ret;
 }
 
 struct bitmap_index *prepare_bitmap_git(struct repository *r)
diff --git a/repo-settings.c b/repo-settings.c
index b4fbd16cdc..5bc7a97a6d 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -8,6 +8,7 @@ static void repo_cfg_bool(struct repository *r, const char *key, int *dest,
 {
 	if (repo_config_get_bool(r, key, dest))
 		*dest = def;
+	trace2_data_string("config", r, key, *dest ? "true" : "false");
 }
 
 void prepare_repo_settings(struct repository *r)
-- 
2.35.1.582.g320e881567

Re: [PATCH v3 5/5] bitmap: add trace2 outputs during open "bitmap" file

[Junio C Hamano]

Teng Long <dyroneteng@gmail.com> writes:

> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 5654eb7b8d..ced5993560 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -312,9 +312,11 @@ char *pack_bitmap_filename(struct packed_git *p)
>  static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
>  			      struct multi_pack_index *midx)
>  {
> +	int fd;
>  	struct stat st;
>  	char *bitmap_name = midx_bitmap_filename(midx);
> -	int fd = git_open(bitmap_name);
> +	trace2_data_string("midx", the_repository, "path", bitmap_name);
> +	fd = git_open(bitmap_name);
>  
>  	free(bitmap_name);

The patch adds new statements at a wrong place.  The block of
declarations and the first statement in the block were separated by
a blank line, but they no longer are.

These things tend to show up in merges quite clearly.  They do not
cause more unnecessary conflicts but can make resolution more error
prone.

Thanks.

Re: [PATCH v3 5/5] bitmap: add trace2 outputs during open "bitmap" file

[Teng Long]

On Mon, 13 Jun 2022 13:59:05 -0700, Junio C Hamano wrote:


> The patch adds new statements at a wrong place.  The block of
> declarations and the first statement in the block were separated by
> a blank line, but they no longer are.
> 
> These things tend to show up in merges quite clearly.  They do not
> cause more unnecessary conflicts but can make resolution more error
> prone.

You are absolutely right.
Will fix.

Thanks.

Re: [PATCH v3 5/5] bitmap: add trace2 outputs during open "bitmap" file

[Taylor Blau]

On Sun, Jun 12, 2022 at 03:44:20PM +0800, Teng Long wrote:
> It's supported for a repo to use bitmap in both "NORMAL" bitmap way

I'm nitpicking, but I usually say "single-pack bitmap" or "multi-pack
(MIDX) bitmap" when distinguishing between the two.

> or a MIDX (multi-pack-index) bitmap. Either of two bitmap kinds can
> exist in the repository, or both can be stored but let the config
> controls which kind of bitmap is used (like "core.multipackIndex",
> etc.). Because of this, sometimes the bitmap debug path is not
> obvious enough, for example, when executing:
>
>  $git rev-list  --test-bitmap  HEAD
>  fatal: failed to load bitmap indexes

Odd spacing, and there should be a single space character separating "$"
from "git" (like "$ git").

While I'm thinking about it: is the error message here up-to-date with
the changes made by the previous patch?

> If we see the output like this, It's not sure for us to know
> what's happened concretely, because the cause should be :
>
>   1. Neither normal nor MIDX bitmap exists.
>   2. Only MIDX bitmap exists but core.multipackIndex="false".
>   3. Config core.multipackIndex set to "true" but MIDX  bitmap is
>      corrupted.
>   4. Config core.multipackIndex set to "true" and no MIDX bitmap
>      exists but normal bitmap file is corrupted.
>   ....
>
> These are some of the scenarios I briefly tested, but maybe there are
> others (some scenarios is produced manually like "corrupted bitmap file",
> but it's not represent it's an existed bug.).

This could probably be trimmed down for brevity, but I don't feel
strongly about it. If you wanted to make your commit message a tad
shorter, perhaps something like:

  When a user sees output like this, it's unclear which kind(s) of
  .bitmap exist, and which were read. For example, it's possible a MIDX
  bitmap exists, but was not read (e.g., because
  core.multiPackIndex=false), among many other scenarios.

would suffice.

> Therefore, we added some TRACE2 code so that when we read the bitmap

s/TRACE2/trace2

> we can be more clear about the decision path, such as whether it is
> working on MIDX or NORMAL bitmap at present, or the related config is

s/NORMAL/pack

> enabled or not. This may help with logging, user troubleshooting, and
> development debugging.
>
> Here are some brief output examples on different scenarios when
> executing:
>
>   $GIT_TRACE2_PERF=1 git rev-list --test-bitmap HEAD

s/$GIT/$ GIT

> Scenario 1: core.multipackIndex [false], midx bitmap exists [Y],
> normal bitmap exists [N]

The output here is quite wide, and I wonder if this whole section could
be shortened. For example, scenario 2 is arguably more interesting than
scenario 1 (I think readers would reasonably infer what happens in
scenario 1 by reading what happens in scenario 2).

>        19:21:56.580349 repo-settings.c:11           | d0 | main                     | data         | r1  |  0.000827 |  0.000827 | config       | core.multipackindex:false
>        19:21:56.580356 repo-settings.c:11           | d0 | main                     | data         | r1  |  0.000834 |  0.000834 | config       | index.sparse:false
>        19:21:56.580706 pack-bitmap.c:525            | d0 | main                     | region_enter | r1  |  0.001183 |           | pack-bitmap  | label:open_bitmap
>        19:21:56.580719 pack-bitmap.c:386            | d0 | main                     | data         | r1  |  0.001196 |  0.000013 | bitmap       | ..path:.git/objects/pack/pack-e9eb18e6a423057f4424a762069e13804a75d01e.bitmap
>        19:21:56.580729 pack-bitmap.c:530            | d0 | main                     | region_leave | r1  |  0.001207 |  0.000024 | pack-bitmap  | label:open_bitmap
>        19:21:56.580737 usage.c:60                   | d0 | main                     | error        |     |           |           |              | failed to load bitmap indexes
>        fatal: failed to load bitmap indexes
>        19:21:56.580746 usage.c:74                   | d0 | main                     | exit         |     |  0.001224 |           |              | code:128
>        19:21:56.580754 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001232 |           |              | code:128
>
> Scenario 2: core.multipackIndex [false], midx bitmap exists [Y],
> normal bitmap exists [Y]
>
> 	19:23:44.692384 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000765 |  0.000765 | config       | core.multipackindex:false
> 	19:23:44.692755 pack-bitmap.c:525            | d0 | main                     | region_enter | r0  |  0.001135 |           | pack-bitmap  | label:open_bitmap
> 	19:23:44.692768 pack-bitmap.c:386            | d0 | main                     | data         | r0  |  0.001149 |  0.000014 | bitmap       | ..path:/home/tenglong.tl/test/dyrone_bitmap/.git/objects/pack/pack-e9eb18e6a423057f4424a762069e13804a75d01e.bitmap
> 	19:23:44.692790 pack-bitmap.c:530            | d0 | main                     | region_leave | r0  |  0.001171 |  0.000036 | pack-bitmap  | label:open_bitmap
> 	Bitmap v1 test (1 entries loaded)
> 	Found bitmap for d864fefa87415d6cd289c72aa9ffd45b4a8ffd84. 64 bits / 11030517 checksum
> 	19:23:44.693119 progress.c:268               | d0 | main                     | region_enter | r0  |  0.001500 |           | progress     | label:Verifying bitmap entries
> 	Verifying bitmap entries: 100% (3/3), done.
> 	19:23:44.693208 progress.c:339               | d0 | main                     | data         | r0  |  0.001589 |  0.000089 | progress     | ..total_objects:3
> 	19:23:44.693216 progress.c:346               | d0 | main                     | region_leave | r0  |  0.001597 |  0.000097 | progress     | label:Verifying bitmap entries
> 	OK!
> 	19:23:44.693234 git.c:718                    | d0 | main                     | exit         |     |  0.001615 |           |              | code:0
> 	19:23:44.693244 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001625 |           |              | code:0

And scenario 2 could be cleaned up by just showing a few of the columns
from the trace2 output. Perhaps along the lines of:

> 	| data         | r0  |  0.000765 |  0.000765 | config       | core.multipackindex:false
> 	| region_enter | r0  |  0.001135 |           | pack-bitmap  | label:open_bitmap
> 	| data         | r0  |  0.001149 |  0.000014 | bitmap       | ..path:.git/objects/pack/pack-e9eb18e6a423057f4424a762069e13804a75d01e.bitmap
> 	| region_leave | r0  |  0.001171 |  0.000036 | pack-bitmap  | label:open_bitmap
> 	| region_enter | r0  |  0.001500 |           | progress     | label:Verifying bitmap entries

Reading the below scenarios, I think just showing this example is more
than sufficient for illustrating your point.

> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>  pack-bitmap.c   | 27 +++++++++++++++++++++------
>  repo-settings.c |  1 +
>  2 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 5654eb7b8d..ced5993560 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -312,9 +312,11 @@ char *pack_bitmap_filename(struct packed_git *p)
>  static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
>  			      struct multi_pack_index *midx)
>  {
> +	int fd;
>  	struct stat st;
>  	char *bitmap_name = midx_bitmap_filename(midx);
> -	int fd = git_open(bitmap_name);
> +	trace2_data_string("midx", the_repository, "path", bitmap_name);
> +	fd = git_open(bitmap_name);
>
>  	free(bitmap_name);
>
> @@ -343,12 +345,19 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
>  				MAP_PRIVATE, fd, 0);
>  	close(fd);
>
> -	if (load_bitmap_header(bitmap_git) < 0)
> +	if (load_bitmap_header(bitmap_git) < 0) {
> +		trace2_data_string("midx", the_repository, "load bitmap header",
> +				   "failed");

I wonder, why don't we show these errors to the user? Should we
introduce a "ret" variable and set it to (for e.g.)

    ret = error(_("failed to load bitmap header"));

or something?

>  struct bitmap_index *prepare_bitmap_git(struct repository *r)
> diff --git a/repo-settings.c b/repo-settings.c
> index b4fbd16cdc..5bc7a97a6d 100644
> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -8,6 +8,7 @@ static void repo_cfg_bool(struct repository *r, const char *key, int *dest,
>  {
>  	if (repo_config_get_bool(r, key, dest))
>  		*dest = def;
> +	trace2_data_string("config", r, key, *dest ? "true" : "false");

I'm not sure we want to dump the whole repository configuration into
trace2 output. Is there a more convenient place to log any important
value(s) _after_ they have been read?

Thanks,
Taylor

Re: [PATCH v3 5/5] bitmap: add trace2 outputs during open "bitmap" file

[Teng Long]

On Mon, 13 Jun 2022 21:40:10 -0400, Taylor Blau wrote:

> I'm nitpicking, but I usually say "single-pack bitmap" or "multi-pack
> (MIDX) bitmap" when distinguishing between the two.

Ha, it's OK, I'm hungry. Your description is a better way I think.

Will optimize the relevant places.

> Odd spacing, and there should be a single space character separating "$"
> from "git" (like "$ git").

Yeah, will Fix.

> While I'm thinking about it: is the error message here up-to-date with
> the changes made by the previous patch?

It's still the same after the remake and test (actually it's just a case
not all the cases have the same error message. now by the previous path
if "core.multipackindex=false" and single-pack doesn't exist, the error
message will look like this).

> This could probably be trimmed down for brevity, but I don't feel
> strongly about it. If you wanted to make your commit message a tad
> shorter, perhaps something like:
> 
>   When a user sees output like this, it's unclear which kind(s) of
>   .bitmap exist, and which were read. For example, it's possible a MIDX
>   bitmap exists, but was not read (e.g., because
>   core.multiPackIndex=false), among many other scenarios.
> 
> would suffice.

I think yours is better, I will use it instead the listed way.

> s/TRACE2/trace2

Will fix.

> s/NORMAL/pack

Will fix as s/NORMAL/single-pack.

> s/$GIT/$ GIT

Will fix.

> The output here is quite wide, and I wonder if this whole section could
> be shortened. For example, scenario 2 is arguably more interesting than
> scenario 1 (I think readers would reasonably infer what happens in
> scenario 1 by reading what happens in scenario 2).
> ...
> And scenario 2 could be cleaned up by just showing a few of the columns
> from the trace2 output. Perhaps along the lines of:
> ...
> Reading the below scenarios, I think just showing this example is more
> than sufficient for illustrating your point.

That makes sense. I'll keep two scenarios and omit some of the output to
keep it shorter in next patch.


> I wonder, why don't we show these errors to the user? Should we
> introduce a "ret" variable and set it to (for e.g.)
> 
>     ret = error(_("failed to load bitmap header"));
> 
> or something?

Just this "load header error", I think it's unnecessary to show more
errors to uses because in "load_bitmap_header()", the function already
does, but in my patch, it's unnecessary to add "trace2_data_string()"
too, so here I will remove it (directly "goto cleanup" maybe) in
next patch.

> I'm not sure we want to dump the whole repository configuration into
> trace2 output. Is there a more convenient place to log any important
> value(s) _after_ they have been read?

I think maybe I can add a argument like "int trace" in next patch, so
the caller can decide whether to print to trace or not.

Thanks for your help, Taylor.

Re: [PATCH v3 5/5] bitmap: add trace2 outputs during open "bitmap" file

[Jeff Hostetler]

On 6/12/22 3:44 AM, Teng Long wrote:
[...]
>   
>   struct bitmap_index *prepare_bitmap_git(struct repository *r)
> diff --git a/repo-settings.c b/repo-settings.c
> index b4fbd16cdc..5bc7a97a6d 100644
> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -8,6 +8,7 @@ static void repo_cfg_bool(struct repository *r, const char *key, int *dest,
>   {
>   	if (repo_config_get_bool(r, key, dest))
>   		*dest = def;
> +	trace2_data_string("config", r, key, *dest ? "true" : "false");
>   }

We should not be doing.  This would dump every repo-related
boolean value on every command.

I already have a GIT_TRACE2_CONFIG_PARAMS and trace2.configparams
that will dump "interesting" config values to the trace2 log.
Just set one of them to a list of regex's.  Look at the comment above
trace2_cmd_list_config() in trace2.h for details.

We also have GIT_TRACE2_ENV_VARS and trace2.envvars that will dump
the values of "interesting" env vars.

You can use these in your testing to log the config and env var
values that you are interested in.

Jeff

Re: [PATCH v3 5/5] bitmap: add trace2 outputs during open "bitmap" file

[Teng Long]

On Wed, 22 Jun 2022 08:51:18 -0400, Jeff Hostetler wrote:

> We should not be doing.  This would dump every repo-related
> boolean value on every command.

Yes. in v4 patch I use a new arg in repo_cfg_bool to decide
whether to trace or not, but after the above you mentioned, I
think it's still not a correct solution in my v4.

> I already have a GIT_TRACE2_CONFIG_PARAMS and trace2.configparams
> that will dump "interesting" config values to the trace2 log.
> Just set one of them to a list of regex's.  Look at the comment above
> trace2_cmd_list_config() in trace2.h for details.

That's fresh for me, thanks for telling me that.

I remove the changes of "repo_config_bool" from my v4 patch
then I try to know about "GIT_TRACE2_CONFIG_PARAMS" and
"trace2.configparams". Thereby, when I:

Execute "GIT_TRACE2_PERF=1 GIT_TRACE2_CONFIG_PARAMS=core.multipackIndex git rev-list  --test-bitmap HEAD"

15:21:38.812782 git.c:461                    | d0 | main                     | def_param    |     |           |           |              | core.multipackindex:false
15:21:38.812797 git.c:461                    | d0 | main                     | def_param    |     |           |           |              | core.multipackindex:false

I checked my configs, I found if there exists multiple level configs.
it'll print multiple times. Like If I config all the global, system
and local on "core.multipackIndex=false" , the output will be:

15:41:50.614108 git.c:462                    | d0 | main                     | def_param    |     |           |           |              | core.multipackindex:false
15:41:50.614123 git.c:462                    | d0 | main                     | def_param    |     |           |           |              | core.multipackindex:false
15:41:50.614136 git.c:462                    | d0 | main                     | def_param    |     |           |           |              | core.multipackindex:false

And if I modified the local scope of core.multipackIndex to "true",
the output will be:

15:45:39.200172 git.c:462                    | d0 | main                     | def_param    |     |           |           |              | core.multipackindex:false
15:45:39.200186 git.c:462                    | d0 | main                     | def_param    |     |           |           |              | core.multipackindex:false
15:45:39.200200 git.c:462                    | d0 | main                     | def_param    |     |           |           |              | core.multipackindex:true

I'm not sure it's an intentional design or here should be only
print the final value that takes effect or should print all the
values if config multiple scopes on the same config.

Hence, I made a temporary patch below to try to add some
identifying information to know why we output these lines, like:


diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c
index c5c8cfbbaa..37a3163be1 100644
--- a/trace2/tr2_tgt_event.c
+++ b/trace2/tr2_tgt_event.c
@@ -479,9 +479,12 @@ static void fn_param_fl(const char *file, int line, const char *param,
 {
        const char *event_name = "def_param";
        struct json_writer jw = JSON_WRITER_INIT;
+       enum config_scope scope = current_config_scope();
+       const char *scope_name = config_scope_name(scope);
 
commit a089800b9dbc93a5dff9a46da7e66111e6e4343e (HEAD -> master, dyrone/master, dyrone/HEAD)
Author: Teng Long <dyroneteng@gmail.com>
Date:   Thu Jun 23 17:24:15 2022 +0800

    tr2: append scope info when same configs exist in multiple scopes
    
    Signed-off-by: Teng Long <dyroneteng@gmail.com>

diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c
index c5c8cfbbaa..37a3163be1 100644
--- a/trace2/tr2_tgt_event.c
+++ b/trace2/tr2_tgt_event.c
@@ -479,9 +479,12 @@ static void fn_param_fl(const char *file, int line, const char *param,
 {
        const char *event_name = "def_param";
        struct json_writer jw = JSON_WRITER_INIT;
+       enum config_scope scope = current_config_scope();
+       const char *scope_name = config_scope_name(scope);
 
        struct json_writer jw = JSON_WRITER_INIT;
+       enum config_scope scope = current_config_scope();
+       const char *scope_name = config_scope_name(scope);
 
        jw_object_begin(&jw, 0);
        event_fmt_prepare(event_name, file, line, NULL, &jw);
+       jw_object_string(&jw, "scope", scope_name);
        jw_object_string(&jw, "param", param);
        jw_object_string(&jw, "value", value);
        jw_end(&jw);
diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c
index c42fbade7f..e1b036a60c 100644
--- a/trace2/tr2_tgt_normal.c
+++ b/trace2/tr2_tgt_normal.c
@@ -298,8 +298,10 @@ static void fn_param_fl(const char *file, int line, const char *param,
                        const char *value)
 {
        struct strbuf buf_payload = STRBUF_INIT;
+       enum config_scope scope = current_config_scope();
+       const char *scope_name = config_scope_name(scope);
 
-       strbuf_addf(&buf_payload, "def_param %s=%s", param, value);
+       strbuf_addf(&buf_payload, "def_param scope:%s %s=%s", scope_name, param, value);
        normal_io_write_fl(file, line, &buf_payload);
        strbuf_release(&buf_payload);
 }
diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c
index a1eff8bea3..c21bf8e651 100644
--- a/trace2/tr2_tgt_perf.c
+++ b/trace2/tr2_tgt_perf.c
@@ -441,10 +441,12 @@ static void fn_param_fl(const char *file, int line, const char *param,
 {
        const char *event_name = "def_param";
        struct strbuf buf_payload = STRBUF_INIT;
+       enum config_scope scope = current_config_scope();
+       const char *scope_name = config_scope_name(scope);
 
        strbuf_addf(&buf_payload, "%s:%s", param, value);
 
-       perf_io_write_fl(file, line, event_name, NULL, NULL, NULL, NULL,


----
The partial tr2 log looks like:

PERF:
17:00:35.933094 git.c:462                    | d0 | main                     | def_param    |     |           |           | system       | core.multipackindex:false
17:00:35.933110 git.c:462                    | d0 | main                     | def_param    |     |           |           | global       | core.multipackindex:false
17:00:35.933128 git.c:462                    | d0 | main                     | def_param    |     |           |           | local        | core.multipackindex:true

NORMAL:
17:14:32.905359 git.c:462                         def_param scope:system core.multipackindex=false
17:14:32.905370 git.c:462                         def_param scope:global core.multipackindex=false
17:14:32.905383 git.c:462                         def_param scope:local core.multipackindex=true

EVENT:
{"event":"def_param","sid":"20220623T092115.703660Z-H82fddc29-P0001812a","thread":"main","time":"2022-06-23T09:21:15.703920Z","file":"git.c","line":462,"scope":"system","param":"core.multipackindex","value":"false"}
{"event":"def_param","sid":"20220623T092115.703660Z-H82fddc29-P0001812a","thread":"main","time":"2022-06-23T09:21:15.703936Z","file":"git.c","line":462,"scope":"global","param":"core.multipackindex","value":"false"}
{"event":"def_param","sid":"20220623T092115.703660Z-H82fddc29-P0001812a","thread":"main","time":"2022-06-23T09:21:15.703952Z","file":"git.c","line":462,"scope":"local","param":"core.multipackindex","value":"true"}


> We also have GIT_TRACE2_ENV_VARS and trace2.envvars that will dump
> the values of "interesting" env vars.

> You can use these in your testing to log the config and env var
> values that you are interested in.

Wow, cool as GIT_TRACE2_CONFIG_PARAMS and trace2.configparams
you mentioned before. 

Thanks.

Re: [PATCH v3 5/5] bitmap: add trace2 outputs during open "bitmap" file

[Jeff Hostetler]

On 6/23/22 5:38 AM, Teng Long wrote:
> On Wed, 22 Jun 2022 08:51:18 -0400, Jeff Hostetler wrote:
> 
[...]
> I remove the changes of "repo_config_bool" from my v4 patch
> then I try to know about "GIT_TRACE2_CONFIG_PARAMS" and
> "trace2.configparams". Thereby, when I:
> 
> Execute "GIT_TRACE2_PERF=1 GIT_TRACE2_CONFIG_PARAMS=core.multipackIndex git rev-list  --test-bitmap HEAD"
> 
> 15:21:38.812782 git.c:461                    | d0 | main                     | def_param    |     |           |           |              | core.multipackindex:false
> 15:21:38.812797 git.c:461                    | d0 | main                     | def_param    |     |           |           |              | core.multipackindex:false
> 
> I checked my configs, I found if there exists multiple level configs.
> it'll print multiple times. Like If I config all the global, system
> and local on "core.multipackIndex=false" , the output will be:
> 
> 15:41:50.614108 git.c:462                    | d0 | main                     | def_param    |     |           |           |              | core.multipackindex:false
> 15:41:50.614123 git.c:462                    | d0 | main                     | def_param    |     |           |           |              | core.multipackindex:false
> 15:41:50.614136 git.c:462                    | d0 | main                     | def_param    |     |           |           |              | core.multipackindex:false
> 
> And if I modified the local scope of core.multipackIndex to "true",
> the output will be:
> 
> 15:45:39.200172 git.c:462                    | d0 | main                     | def_param    |     |           |           |              | core.multipackindex:false
> 15:45:39.200186 git.c:462                    | d0 | main                     | def_param    |     |           |           |              | core.multipackindex:false
> 15:45:39.200200 git.c:462                    | d0 | main                     | def_param    |     |           |           |              | core.multipackindex:true
> 
> I'm not sure it's an intentional design or here should be only
> print the final value that takes effect or should print all the
> values if config multiple scopes on the same config.
> 
> Hence, I made a temporary patch below to try to add some
> identifying information to know why we output these lines, like:

I had intended it to only print the final "effective" value
that the command would actually use.  I don't remember if I
ever experimented with config values that are set at multiple
levels, but testing it now shows that you're right it does
print a "def_param" message for each level.

I would suggest we fix it to only print the final value
since we really just want to know whether the command will
or will not use a feature.  The inheritance is somewhat
irrelevant.

But the change to do that may be quite a mess, so printing
the scope name as you have here is fine.  And may help with
support when debugging users having confusing problems, so
I'm fine with it.

Thanks,
Jeff

Re: [PATCH v3 5/5] bitmap: add trace2 outputs during open "bitmap"

[Teng Long]

Nn Thu, 23 Jun 2022 11:14:27 -0400, Jeff Hostetler wrote:

> I had intended it to only print the final "effective" value
> that the command would actually use.  I don't remember if I
> ever experimented with config values that are set at multiple
> levels, but testing it now shows that you're right it does
> print a "def_param" message for each level.
> 
> I would suggest we fix it to only print the final value
> since we really just want to know whether the command will
> or will not use a feature.  The inheritance is somewhat
> irrelevant.
> 
> But the change to do that may be quite a mess, so printing
> the scope name as you have here is fine.  And may help with
> support when debugging users having confusing problems, so
> I'm fine with it.

Yes, as I replied both way is OK for me, but sorry for only
replied the append-scope way. I will post a new patch v5
and include the related changes to fix this. 

Thanks.

[PATCH v3 0/5] trace2 output for bitmap decision path

[Teng Long]

Main diff from v4 to v3:

* Optimize the commit message of [5/5].
* Fix the missing blank line in [5/5].
* Only print the config we want to instead of the whole configs .
* use "error_errno()" instead in [4/5].

Thanks for Junio C Hamano and Taylor Blau for their help in v3.


Teng Long (5):
  pack-bitmap.c: continue looping when first MIDX bitmap is found
  pack-bitmap.c: rename "idx_name" to "bitmap_name"
  pack-bitmap.c: make warnings support i18N when opening bitmap
  pack-bitmap.c: using error() instead of silently returning -1
  bitmap: add trace2 outputs during open "bitmap" file

 pack-bitmap.c   | 52 ++++++++++++++++++++++++++++++-------------------
 repo-settings.c | 22 +++++++++++----------
 2 files changed, 44 insertions(+), 30 deletions(-)

Range-diff against v2:
-:  ---------- > 1:  589e3f4075 pack-bitmap.c: continue looping when first MIDX bitmap is found
-:  ---------- > 2:  b6b30047fc pack-bitmap.c: rename "idx_name" to "bitmap_name"
-:  ---------- > 3:  d8dfe53dd4 pack-bitmap.c: make warnings support i18N when opening bitmap
1:  72da3b5844 ! 4:  917551f2b5 pack-bitmap.c: using error() instead of silently returning -1
    @@ pack-bitmap.c: static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
      	if (fstat(fd, &st)) {
      		close(fd);
     -		return -1;
    -+		return error(_("cannot stat bitmap file"));
    ++		return error_errno(_("cannot stat bitmap file"));
      	}
      
      	if (bitmap_git->pack || bitmap_git->midx) {
    @@ pack-bitmap.c: static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
      	bitmap_git->map = NULL;
      	bitmap_git->midx = NULL;
     -	return -1;
    -+	return error("cannot open midx bitmap file");
    ++	return error(_("cannot open midx bitmap file"));
      }
      
      static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git *packfile)
    @@ pack-bitmap.c: static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, st
      	if (fstat(fd, &st)) {
      		close(fd);
     -		return -1;
    -+		return error(_("cannot stat bitmap file"));
    ++		return error_errno(_("cannot stat bitmap file"));
      	}
      
      	if (bitmap_git->pack || bitmap_git->midx) {
2:  e118758d1d ! 5:  8735ae9979 bitmap: add trace2 outputs during open "bitmap" file
    @@ Metadata
      ## Commit message ##
         bitmap: add trace2 outputs during open "bitmap" file
     
    -    It's supported for a repo to use bitmap in both "NORMAL" bitmap way
    -    or a MIDX (multi-pack-index) bitmap. Either of two bitmap kinds can
    +    It's supported for a repo to use bitmap in both single-pack bitmap
    +    way or a multi-pack(MIDX) bitmap. Either of two bitmap kinds can
         exist in the repository, or both can be stored but let the config
         controls which kind of bitmap is used (like "core.multipackIndex",
         etc.). Because of this, sometimes the bitmap debug path is not
         obvious enough, for example, when executing:
     
    -     $git rev-list  --test-bitmap  HEAD
    +     $ git rev-list  --test-bitmap  HEAD
          fatal: failed to load bitmap indexes
     
    -    If we see the output like this, It's not sure for us to know
    -    what's happened concretely, because the cause should be :
    +    When the output look like this, it's unclear which kind(s) of
    +    .bitmap exist, and which were read. For example, it's possible a MIDX
    +    bitmap exists, but was not read (e.g., because
    +    core.multiPackIndex=false), among many other scenarios.
     
    -      1. Neither normal nor MIDX bitmap exists.
    -      2. Only MIDX bitmap exists but core.multipackIndex="false".
    -      3. Config core.multipackIndex set to "true" but MIDX  bitmap is
    -         corrupted.
    -      4. Config core.multipackIndex set to "true" and no MIDX bitmap
    -         exists but normal bitmap file is corrupted.
    -      ....
    -
    -    These are some of the scenarios I briefly tested, but maybe there are
    -    others (some scenarios is produced manually like "corrupted bitmap file",
    -    but it's not represent it's an existed bug.).
    -
    -    Therefore, we added some TRACE2 code so that when we read the bitmap
    +    Therefore, we added some trace2 code so that when we read the bitmap
         we can be more clear about the decision path, such as whether it is
    -    working on MIDX or NORMAL bitmap at present, or the related config is
    -    enabled or not. This may help with logging, user troubleshooting, and
    +    working on MIDX or single-pack bitmap at present, or the related config
    +    is enabled or not. This may help with logging, user troubleshooting, and
         development debugging.
     
    -    Here are some brief output examples on different scenarios when
    +    Here are some brief output (omitted some unrelated or repetitive rows
    +    and columns, using "..." instead of) examples on two scenarios when
         executing:
     
    -      $GIT_TRACE2_PERF=1 git rev-list --test-bitmap HEAD
    -
    -    Scenario 1: core.multipackIndex [false], midx bitmap exists [Y],
    -    normal bitmap exists [N]
    -
    -           19:21:56.580349 repo-settings.c:11           | d0 | main                     | data         | r1  |  0.000827 |  0.000827 | config       | core.multipackindex:false
    -           19:21:56.580356 repo-settings.c:11           | d0 | main                     | data         | r1  |  0.000834 |  0.000834 | config       | index.sparse:false
    -           19:21:56.580706 pack-bitmap.c:525            | d0 | main                     | region_enter | r1  |  0.001183 |           | pack-bitmap  | label:open_bitmap
    -           19:21:56.580719 pack-bitmap.c:386            | d0 | main                     | data         | r1  |  0.001196 |  0.000013 | bitmap       | ..path:.git/objects/pack/pack-e9eb18e6a423057f4424a762069e13804a75d01e.bitmap
    -           19:21:56.580729 pack-bitmap.c:530            | d0 | main                     | region_leave | r1  |  0.001207 |  0.000024 | pack-bitmap  | label:open_bitmap
    -           19:21:56.580737 usage.c:60                   | d0 | main                     | error        |     |           |           |              | failed to load bitmap indexes
    -           fatal: failed to load bitmap indexes
    -           19:21:56.580746 usage.c:74                   | d0 | main                     | exit         |     |  0.001224 |           |              | code:128
    -           19:21:56.580754 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001232 |           |              | code:128
    -
    -    Scenario 2: core.multipackIndex [false], midx bitmap exists [Y],
    -    normal bitmap exists [Y]
    -
    -            19:23:44.692384 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000765 |  0.000765 | config       | core.multipackindex:false
    -            19:23:44.692755 pack-bitmap.c:525            | d0 | main                     | region_enter | r0  |  0.001135 |           | pack-bitmap  | label:open_bitmap
    -            19:23:44.692768 pack-bitmap.c:386            | d0 | main                     | data         | r0  |  0.001149 |  0.000014 | bitmap       | ..path:/home/tenglong.tl/test/dyrone_bitmap/.git/objects/pack/pack-e9eb18e6a423057f4424a762069e13804a75d01e.bitmap
    -            19:23:44.692790 pack-bitmap.c:530            | d0 | main                     | region_leave | r0  |  0.001171 |  0.000036 | pack-bitmap  | label:open_bitmap
    -            Bitmap v1 test (1 entries loaded)
    -            Found bitmap for d864fefa87415d6cd289c72aa9ffd45b4a8ffd84. 64 bits / 11030517 checksum
    -            19:23:44.693119 progress.c:268               | d0 | main                     | region_enter | r0  |  0.001500 |           | progress     | label:Verifying bitmap entries
    -            Verifying bitmap entries: 100% (3/3), done.
    -            19:23:44.693208 progress.c:339               | d0 | main                     | data         | r0  |  0.001589 |  0.000089 | progress     | ..total_objects:3
    -            19:23:44.693216 progress.c:346               | d0 | main                     | region_leave | r0  |  0.001597 |  0.000097 | progress     | label:Verifying bitmap entries
    -            OK!
    -            19:23:44.693234 git.c:718                    | d0 | main                     | exit         |     |  0.001615 |           |              | code:0
    -            19:23:44.693244 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001625 |           |              | code:0
    -
    -    Scenario 3: core.multipackIndex [true], midx bitmap exists [Y],
    -    normal bitmap exists [Y]
    -
    -            19:26:03.625055 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000760 |  0.000760 | config       | core.multipackindex:true
    -            19:26:03.625090 midx.c:185                   | d0 | main                     | data         | r0  |  0.000795 |  0.000795 | midx         | load/num_packs:1
    -            19:26:03.625097 midx.c:186                   | d0 | main                     | data         | r0  |  0.000803 |  0.000803 | midx         | load/num_objects:3
    -            19:26:03.625455 pack-bitmap.c:525            | d0 | main                     | region_enter | r0  |  0.001160 |           | pack-bitmap  | label:open_bitmap
    -            19:26:03.625470 pack-bitmap.c:318            | d0 | main                     | data         | r0  |  0.001175 |  0.000015 | midx         | ..path:/home/tenglong.tl/test/dyrone_bitmap/.git/objects/pack/multi-pack-index-fe8e96790bd34926423bdf3efd762dbbea9f3213.bitmap
    -            19:26:03.625489 pack-revindex.c:315          | d0 | main                     | data         | r0  |  0.001194 |  0.000034 | load_midx_re | ..source:midx
    -            19:26:03.625496 pack-bitmap.c:530            | d0 | main                     | region_leave | r0  |  0.001202 |  0.000042 | pack-bitmap  | label:open_bitmap
    -            Bitmap v1 test (1 entries loaded)
    -            Found bitmap for d864fefa87415d6cd289c72aa9ffd45b4a8ffd84. 64 bits / 11030517 checksum
    -            19:26:03.625818 progress.c:268               | d0 | main                     | region_enter | r0  |  0.001523 |           | progress     | label:Verifying bitmap entries
    -            Verifying bitmap entries: 100% (3/3), done.
    -            19:26:03.625916 progress.c:339               | d0 | main                     | data         | r0  |  0.001622 |  0.000099 | progress     | ..total_objects:3
    -            19:26:03.625925 progress.c:346               | d0 | main                     | region_leave | r0  |  0.001630 |  0.000107 | progress     | label:Verifying bitmap entries
    -            OK!
    -            19:26:03.625943 git.c:718                    | d0 | main                     | exit         |     |  0.001648 |           |              | code:0
    -            19:26:03.625952 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001658 |           |              | code:0
    -
    -    Situation 4: core.multipackIndex [false], midx bitmap exists [N],
    -    normal bitmap exists [Y].
    -
    -            19:27:15.383037 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000746 |  0.000746 | config       | core.multipackindex:true
    -            19:27:15.383397 pack-bitmap.c:525            | d0 | main                     | region_enter | r0  |  0.001105 |           | pack-bitmap  | label:open_bitmap
    -            19:27:15.383408 pack-bitmap.c:386            | d0 | main                     | data         | r0  |  0.001116 |  0.000011 | bitmap       | ..path:/home/tenglong.tl/test/dyrone_bitmap/.git/objects/pack/pack-e9eb18e6a423057f4424a762069e13804a75d01e.bitmap
    -            19:27:15.383419 pack-bitmap.c:530            | d0 | main                     | region_leave | r0  |  0.001128 |  0.000023 | pack-bitmap  | label:open_bitmap
    -            Bitmap v1 test (1 entries loaded)
    -            Found bitmap for d864fefa87415d6cd289c72aa9ffd45b4a8ffd84. 64 bits / 11030517 checksum
    -            19:27:15.383730 progress.c:268               | d0 | main                     | region_enter | r0  |  0.001439 |           | progress     | label:Verifying bitmap entries
    -            Verifying bitmap entries: 100% (3/3), done.
    -            19:27:15.383822 progress.c:339               | d0 | main                     | data         | r0  |  0.001531 |  0.000092 | progress     | ..total_objects:3
    -            19:27:15.383830 progress.c:346               | d0 | main                     | region_leave | r0  |  0.001539 |  0.000100 | progress     | label:Verifying bitmap entries
    -            OK!
    -            19:27:15.383848 git.c:718                    | d0 | main                     | exit         |     |  0.001557 |           |              | code:0
    -            19:27:15.383867 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001576 |           |              | code:0
    -
    -    Scenario 5: core.multipackIndex [true], midx bitmap exists [Y] but corrupted,
    -    normal bitmap exists [Y]
    -
    -            19:29:25.888233 repo-settings.c:11           | d0 | main                     | data         | r0  |  0.000794 |  0.000794 | config       | core.multipackindex:true
    -            19:29:25.888591 pack-bitmap.c:525            | d0 | main                     | region_enter | r0  |  0.001152 |           | pack-bitmap  | label:open_bitmap
    -            19:29:25.888603 pack-bitmap.c:386            | d0 | main                     | data         | r0  |  0.001163 |  0.000011 | bitmap       | ..path:/home/tenglong.tl/test/dyrone_bitmap/.git/objects/pack/pack-e9eb18e6a423057f4424a762069e13804a75d01e.bitmap
    -            19:29:25.888622 usage.c:79                   | d0 | main                     | error        |     |           |           |              | ..Corrupted bitmap index file (wrong header)
    -            error: Corrupted bitmap index file (wrong header)
    -            19:29:25.888638 usage.c:79                   | d0 | main                     | error        |     |           |           |              | ..bitmap header is invalid
    -            error: bitmap header is invalid
    -            19:29:25.888650 pack-bitmap.c:530            | d0 | main                     | region_leave | r0  |  0.001211 |  0.000059 | pack-bitmap  | label:open_bitmap
    -            19:29:25.888659 usage.c:60                   | d0 | main                     | error        |     |           |           |              | failed to load bitmap indexes
    -            fatal: failed to load bitmap indexes
    -            19:29:25.888670 usage.c:74                   | d0 | main                     | exit         |     |  0.001231 |           |              | code:128
    -            19:29:25.888680 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.001241 |           |              | code:128
    +      $ GIT_TRACE2_PERF=1 git rev-list --test-bitmap HEAD
    +
    +    Scenario 1:
    +      core.multipackIndex [false], MIDX bitmap exists [Y], single-pack bitmap exists [N]
    +
    +      ...
    +      ... | main                     | data         | r1  | ... | config       | core.multipackindex:false
    +      ... | d0 | main                | region_enter | r1  | ... | pack-bitmap  | label:open_bitmap
    +      ... | d0 | main                | data         | r1  | ... | bitmap       | ..path:.git/objects/pack/pack-e9eb18e6a423057f4424a762069e13804a75d01e.bitmap
    +      ... | main                     | region_leave | r1  | ... | pack-bitmap  | label:open_bitmap
    +      ... | main                     | error        |     | ... |              | failed to load bitmap indexes
    +      fatal: failed to load bitmap indexes
    +      ... | d0 | main                | exit         |     | ... |              | code:128
    +      ...
    +
    +    Scenario 2:
    +      core.multipackIndex [false], MIDX bitmap exists [Y], single-pack bitmap exists [Y]
    +
    +      ... | d0 | main                | region_enter | r0  | ... | pack-bitmap  | label:open_bitmap
    +      ... | d0 | main                | data         | r0  | ... | bitmap       | ..path:/home/tenglong.tl/test/dyrone_bitmap/.git/objects/pack/pack-e9eb18e6a423057f4424a762069e13804a75d01e.bitmap
    +      ... | main                     | region_leave | r0  | ... | pack-bitmap  | label:open_bitmap
    +      Bitmap v1 test (1 entries loaded)
    +      Found bitmap for d864fefa87415d6cd289c72aa9ffd45b4a8ffd84. 64 bits / 11030517 checksum
    +      ... | main                     | region_enter | r0  | ... | progress     | label:Verifying bitmap entries
    +      Verifying bitmap entries: 100% (3/3), done.
    +      ... | main                     | data         | r0  | ... | progress     | ..total_objects:3
    +      ... | main                     | region_leave | r0  | ... | progress     | label:Verifying bitmap entries
    +      OK!
    +      ... | d0 | main                | exit         |     | ... |              | code:0
    +      ...
     
         Signed-off-by: Teng Long <dyroneteng@gmail.com>
     
    @@ pack-bitmap.c: char *pack_bitmap_filename(struct packed_git *p)
      {
     +	int fd;
      	struct stat st;
    ++
      	char *bitmap_name = midx_bitmap_filename(midx);
     -	int fd = git_open(bitmap_name);
     +	trace2_data_string("midx", the_repository, "path", bitmap_name);
    @@ pack-bitmap.c: char *pack_bitmap_filename(struct packed_git *p)
      	free(bitmap_name);
      
     @@ pack-bitmap.c: static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
    - 				MAP_PRIVATE, fd, 0);
    - 	close(fd);
    - 
    --	if (load_bitmap_header(bitmap_git) < 0)
    -+	if (load_bitmap_header(bitmap_git) < 0) {
    -+		trace2_data_string("midx", the_repository, "load bitmap header",
    -+				   "failed");
    + 	if (load_bitmap_header(bitmap_git) < 0)
      		goto cleanup;
    -+	}
      
     -	if (!hasheq(get_midx_checksum(bitmap_git->midx), bitmap_git->checksum))
     +	if (!hasheq(get_midx_checksum(bitmap_git->midx), bitmap_git->checksum)) {
    -+		trace2_data_string("midx", the_repository, "verify checksum",
    -+				   "mismatch");
    ++		error(_("midx and bitmap checksum don't match"));
      		goto cleanup;
    - 
     +	}
    -+
    + 
      	if (load_midx_revindex(bitmap_git->midx) < 0) {
      		warning(_("multi-pack bitmap is missing required reverse index"));
    - 		goto cleanup;
     @@ pack-bitmap.c: static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
      		return -1;
      
    @@ pack-bitmap.c: static int open_midx_bitmap(struct repository *r,
      struct bitmap_index *prepare_bitmap_git(struct repository *r)
     
      ## repo-settings.c ##
    -@@ repo-settings.c: static void repo_cfg_bool(struct repository *r, const char *key, int *dest,
    +@@
    + #include "midx.h"
    + 
    + static void repo_cfg_bool(struct repository *r, const char *key, int *dest,
    +-			  int def)
    ++			  int def, int trace)
      {
      	if (repo_config_get_bool(r, key, dest))
      		*dest = def;
    -+	trace2_data_string("config", r, key, *dest ? "true" : "false");
    ++	if (trace)
    ++		trace2_data_string("config", r, key, *dest ? "true" : "false");
      }
      
      void prepare_repo_settings(struct repository *r)
    +@@ repo-settings.c: void prepare_repo_settings(struct repository *r)
    + 	r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;
    + 
    + 	/* Booleans config or default, cascades to other settings */
    +-	repo_cfg_bool(r, "feature.manyfiles", &manyfiles, 0);
    +-	repo_cfg_bool(r, "feature.experimental", &experimental, 0);
    ++	repo_cfg_bool(r, "feature.manyfiles", &manyfiles, 0, 0);
    ++	repo_cfg_bool(r, "feature.experimental", &experimental, 0, 0);
    + 
    + 	/* Defaults modified by feature.* */
    + 	if (experimental) {
    +@@ repo-settings.c: void prepare_repo_settings(struct repository *r)
    + 	}
    + 
    + 	/* Boolean config or default, does not cascade (simple)  */
    +-	repo_cfg_bool(r, "core.commitgraph", &r->settings.core_commit_graph, 1);
    +-	repo_cfg_bool(r, "commitgraph.readchangedpaths", &r->settings.commit_graph_read_changed_paths, 1);
    +-	repo_cfg_bool(r, "gc.writecommitgraph", &r->settings.gc_write_commit_graph, 1);
    +-	repo_cfg_bool(r, "fetch.writecommitgraph", &r->settings.fetch_write_commit_graph, 0);
    +-	repo_cfg_bool(r, "pack.usesparse", &r->settings.pack_use_sparse, 1);
    +-	repo_cfg_bool(r, "core.multipackindex", &r->settings.core_multi_pack_index, 1);
    +-	repo_cfg_bool(r, "index.sparse", &r->settings.sparse_index, 0);
    ++	repo_cfg_bool(r, "core.commitgraph", &r->settings.core_commit_graph, 1, 0);
    ++	repo_cfg_bool(r, "commitgraph.readchangedpaths", &r->settings.commit_graph_read_changed_paths, 1, 0);
    ++	repo_cfg_bool(r, "gc.writecommitgraph", &r->settings.gc_write_commit_graph, 1, 0);
    ++	repo_cfg_bool(r, "fetch.writecommitgraph", &r->settings.fetch_write_commit_graph, 0, 0);
    ++	repo_cfg_bool(r, "pack.usesparse", &r->settings.pack_use_sparse, 1, 0);
    ++	repo_cfg_bool(r, "core.multipackindex", &r->settings.core_multi_pack_index, 1, 1);
    ++	repo_cfg_bool(r, "index.sparse", &r->settings.sparse_index, 0, 0);
    + 
    + 	/*
    + 	 * The GIT_TEST_MULTI_PACK_INDEX variable is special in that
-- 
2.35.1.582.g270d558070.dirty

[PATCH v3 1/5] pack-bitmap.c: continue looping when first MIDX bitmap is found

[Teng Long]

In "open_midx_bitmap()", we do a loop with the MIDX(es) in repo, when
the first one has been found, then will break out by a "return"
directly.

But actually, it's better to continue the loop until we have visited
both the MIDX in our repository, as well as any alternates (along with
_their_ alternates, recursively).

The reason for this is, there may exist more than one MIDX file in
a repo. The "multi_pack_index" struct is actually designed as a singly
linked list, and if a MIDX file has been already opened successfully,
then the other MIDX files will be skipped and left with a warning
"ignoring extra bitmap file." to the output.

The discussion link of community:

  https://public-inbox.org/git/YjzCTLLDCby+kJrZ@nand.local/

Helped-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 9c666cdb8b..112c2b12c6 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -494,15 +494,16 @@ 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;
+			ret = 0;
 	}
-	return -1;
+	return ret;
 }
 
 static int open_bitmap(struct repository *r,
-- 
2.35.1.582.g270d558070.dirty

[PATCH v3 2/5] pack-bitmap.c: rename "idx_name" to "bitmap_name"

[Teng Long]

In "open_pack_bitmap_1()" and "open_midx_bitmap_1()" we use
a var named "idx_name" to represent the bitmap filename which
is computed by "midx_bitmap_filename()" or "pack_bitmap_filename()"
before we open it.

There may bring some confusion in this "idx_name" naming, which
might lead us to think of ".idx "or" multi-pack-index" files,
although bitmap is essentially can be understood as a kind of index,
let's define this name a little more accurate here.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 112c2b12c6..f13a6bfe3a 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -313,10 +313,10 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 			      struct multi_pack_index *midx)
 {
 	struct stat st;
-	char *idx_name = midx_bitmap_filename(midx);
-	int fd = git_open(idx_name);
+	char *bitmap_name = midx_bitmap_filename(midx);
+	int fd = git_open(bitmap_name);
 
-	free(idx_name);
+	free(bitmap_name);
 
 	if (fd < 0)
 		return -1;
@@ -368,14 +368,14 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 {
 	int fd;
 	struct stat st;
-	char *idx_name;
+	char *bitmap_name;
 
 	if (open_pack_index(packfile))
 		return -1;
 
-	idx_name = pack_bitmap_filename(packfile);
-	fd = git_open(idx_name);
-	free(idx_name);
+	bitmap_name = pack_bitmap_filename(packfile);
+	fd = git_open(bitmap_name);
+	free(bitmap_name);
 
 	if (fd < 0)
 		return -1;
-- 
2.35.1.582.g270d558070.dirty

[PATCH v3 3/5] pack-bitmap.c: make warnings support i18N when opening bitmap

[Teng Long]

When calling the "open_midx_bitmap_1()" or "open_pack_bitmap_1()", there
will be a warning if a normal bitmap or MIDX bitmap already has been
opened, then let's make the warning text supporting for translation.

Discussion in community:

  1. https://public-inbox.org/git/YkPGq0mDL4NG6D1o@nand.local/#t
  2. https://public-inbox.org/git/220330.868rsrpohm.gmgdl@evledraar.gmail.com/

Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Helped-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index f13a6bfe3a..af0f41833e 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -330,7 +330,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 		struct strbuf buf = STRBUF_INIT;
 		get_midx_filename(&buf, midx->object_dir);
 		/* ignore extra bitmap file; we can only handle one */
-		warning("ignoring extra bitmap file: %s", buf.buf);
+		warning(_("ignoring extra bitmap file: %s"), buf.buf);
 		close(fd);
 		strbuf_release(&buf);
 		return -1;
@@ -387,7 +387,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 
 	if (bitmap_git->pack || bitmap_git->midx) {
 		/* ignore extra bitmap file; we can only handle one */
-		warning("ignoring extra bitmap file: %s", packfile->pack_name);
+		warning(_("ignoring extra bitmap file: %s"), packfile->pack_name);
 		close(fd);
 		return -1;
 	}
-- 
2.35.1.582.g270d558070.dirty

[PATCH v3 4/5] pack-bitmap.c: using error() instead of silently returning -1

[Teng Long]

In "open_pack_bitmap_1()" and "open_midx_bitmap_1()", it's better to
return error() instead of "-1" when some unexpected error occurs like
"stat bitmap file failed", "bitmap header is invalid" or "checksum
mismatch", etc.

There are places where we do not replace, such as when the bitmap
does not exist (no bitmap in repository is allowed) or when another
bitmap has already been opened (in which case it should be a warning
rather than an error).

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index af0f41833e..a54d5a0c9f 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -323,7 +323,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 
 	if (fstat(fd, &st)) {
 		close(fd);
-		return -1;
+		return error_errno(_("cannot stat bitmap file"));
 	}
 
 	if (bitmap_git->pack || bitmap_git->midx) {
@@ -361,7 +361,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 	bitmap_git->map_pos = 0;
 	bitmap_git->map = NULL;
 	bitmap_git->midx = NULL;
-	return -1;
+	return error(_("cannot open midx bitmap file"));
 }
 
 static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git *packfile)
@@ -382,7 +382,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 
 	if (fstat(fd, &st)) {
 		close(fd);
-		return -1;
+		return error_errno(_("cannot stat bitmap file"));
 	}
 
 	if (bitmap_git->pack || bitmap_git->midx) {
@@ -394,7 +394,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 
 	if (!is_pack_valid(packfile)) {
 		close(fd);
-		return -1;
+		return error(_("packfile is invalid"));
 	}
 
 	bitmap_git->pack = packfile;
@@ -409,7 +409,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 		bitmap_git->map_size = 0;
 		bitmap_git->map_pos = 0;
 		bitmap_git->pack = NULL;
-		return -1;
+		return error(_("bitmap header is invalid"));
 	}
 
 	return 0;
-- 
2.35.1.582.g270d558070.dirty

[PATCH v3 5/5] bitmap: add trace2 outputs during open "bitmap" file

[Teng Long]

It's supported for a repo to use bitmap in both single-pack bitmap
way or a multi-pack(MIDX) bitmap. Either of two bitmap kinds can
exist in the repository, or both can be stored but let the config
controls which kind of bitmap is used (like "core.multipackIndex",
etc.). Because of this, sometimes the bitmap debug path is not
obvious enough, for example, when executing:

 $ git rev-list  --test-bitmap  HEAD
 fatal: failed to load bitmap indexes

When the output look like this, it's unclear which kind(s) of
.bitmap exist, and which were read. For example, it's possible a MIDX
bitmap exists, but was not read (e.g., because
core.multiPackIndex=false), among many other scenarios.

Therefore, we added some trace2 code so that when we read the bitmap
we can be more clear about the decision path, such as whether it is
working on MIDX or single-pack bitmap at present, or the related config
is enabled or not. This may help with logging, user troubleshooting, and
development debugging.

Here are some brief output (omitted some unrelated or repetitive rows
and columns, using "..." instead of) examples on two scenarios when
executing:

  $ GIT_TRACE2_PERF=1 git rev-list --test-bitmap HEAD

Scenario 1:
  core.multipackIndex [false], MIDX bitmap exists [Y], single-pack bitmap exists [N]

  ...
  ... | main                     | data         | r1  | ... | config       | core.multipackindex:false
  ... | d0 | main                | region_enter | r1  | ... | pack-bitmap  | label:open_bitmap
  ... | d0 | main                | data         | r1  | ... | bitmap       | ..path:.git/objects/pack/pack-e9eb18e6a423057f4424a762069e13804a75d01e.bitmap
  ... | main                     | region_leave | r1  | ... | pack-bitmap  | label:open_bitmap
  ... | main                     | error        |     | ... |              | failed to load bitmap indexes
  fatal: failed to load bitmap indexes
  ... | d0 | main                | exit         |     | ... |              | code:128
  ...

Scenario 2:
  core.multipackIndex [false], MIDX bitmap exists [Y], single-pack bitmap exists [Y]

  ... | d0 | main                | region_enter | r0  | ... | pack-bitmap  | label:open_bitmap
  ... | d0 | main                | data         | r0  | ... | bitmap       | ..path:/home/tenglong.tl/test/dyrone_bitmap/.git/objects/pack/pack-e9eb18e6a423057f4424a762069e13804a75d01e.bitmap
  ... | main                     | region_leave | r0  | ... | pack-bitmap  | label:open_bitmap
  Bitmap v1 test (1 entries loaded)
  Found bitmap for d864fefa87415d6cd289c72aa9ffd45b4a8ffd84. 64 bits / 11030517 checksum
  ... | main                     | region_enter | r0  | ... | progress     | label:Verifying bitmap entries
  Verifying bitmap entries: 100% (3/3), done.
  ... | main                     | data         | r0  | ... | progress     | ..total_objects:3
  ... | main                     | region_leave | r0  | ... | progress     | label:Verifying bitmap entries
  OK!
  ... | d0 | main                | exit         |     | ... |              | code:0
  ...

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c   | 21 ++++++++++++++++-----
 repo-settings.c | 22 ++++++++++++----------
 2 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index a54d5a0c9f..c39d722592 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -312,9 +312,12 @@ char *pack_bitmap_filename(struct packed_git *p)
 static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 			      struct multi_pack_index *midx)
 {
+	int fd;
 	struct stat st;
+
 	char *bitmap_name = midx_bitmap_filename(midx);
-	int fd = git_open(bitmap_name);
+	trace2_data_string("midx", the_repository, "path", bitmap_name);
+	fd = git_open(bitmap_name);
 
 	free(bitmap_name);
 
@@ -346,8 +349,10 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 	if (load_bitmap_header(bitmap_git) < 0)
 		goto cleanup;
 
-	if (!hasheq(get_midx_checksum(bitmap_git->midx), bitmap_git->checksum))
+	if (!hasheq(get_midx_checksum(bitmap_git->midx), bitmap_git->checksum)) {
+		error(_("midx and bitmap checksum don't match"));
 		goto cleanup;
+	}
 
 	if (load_midx_revindex(bitmap_git->midx) < 0) {
 		warning(_("multi-pack bitmap is missing required reverse index"));
@@ -374,6 +379,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 		return -1;
 
 	bitmap_name = pack_bitmap_filename(packfile);
+	trace2_data_string("bitmap", the_repository, "path", bitmap_name);
 	fd = git_open(bitmap_name);
 	free(bitmap_name);
 
@@ -509,11 +515,16 @@ static int open_midx_bitmap(struct repository *r,
 static int open_bitmap(struct repository *r,
 		       struct bitmap_index *bitmap_git)
 {
-	assert(!bitmap_git->map);
+	int ret = 0;
 
+	assert(!bitmap_git->map);
+	trace2_region_enter("pack-bitmap", "open_bitmap", r);
 	if (!open_midx_bitmap(r, bitmap_git))
-		return 0;
-	return open_pack_bitmap(r, bitmap_git);
+		goto done;
+	ret = open_pack_bitmap(r, bitmap_git);
+done:
+	trace2_region_leave("pack-bitmap", "open_bitmap", r);
+	return ret;
 }
 
 struct bitmap_index *prepare_bitmap_git(struct repository *r)
diff --git a/repo-settings.c b/repo-settings.c
index b4fbd16cdc..115d96ece3 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -4,10 +4,12 @@
 #include "midx.h"
 
 static void repo_cfg_bool(struct repository *r, const char *key, int *dest,
-			  int def)
+			  int def, int trace)
 {
 	if (repo_config_get_bool(r, key, dest))
 		*dest = def;
+	if (trace)
+		trace2_data_string("config", r, key, *dest ? "true" : "false");
 }
 
 void prepare_repo_settings(struct repository *r)
@@ -29,8 +31,8 @@ void prepare_repo_settings(struct repository *r)
 	r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;
 
 	/* Booleans config or default, cascades to other settings */
-	repo_cfg_bool(r, "feature.manyfiles", &manyfiles, 0);
-	repo_cfg_bool(r, "feature.experimental", &experimental, 0);
+	repo_cfg_bool(r, "feature.manyfiles", &manyfiles, 0, 0);
+	repo_cfg_bool(r, "feature.experimental", &experimental, 0, 0);
 
 	/* Defaults modified by feature.* */
 	if (experimental) {
@@ -42,13 +44,13 @@ void prepare_repo_settings(struct repository *r)
 	}
 
 	/* Boolean config or default, does not cascade (simple)  */
-	repo_cfg_bool(r, "core.commitgraph", &r->settings.core_commit_graph, 1);
-	repo_cfg_bool(r, "commitgraph.readchangedpaths", &r->settings.commit_graph_read_changed_paths, 1);
-	repo_cfg_bool(r, "gc.writecommitgraph", &r->settings.gc_write_commit_graph, 1);
-	repo_cfg_bool(r, "fetch.writecommitgraph", &r->settings.fetch_write_commit_graph, 0);
-	repo_cfg_bool(r, "pack.usesparse", &r->settings.pack_use_sparse, 1);
-	repo_cfg_bool(r, "core.multipackindex", &r->settings.core_multi_pack_index, 1);
-	repo_cfg_bool(r, "index.sparse", &r->settings.sparse_index, 0);
+	repo_cfg_bool(r, "core.commitgraph", &r->settings.core_commit_graph, 1, 0);
+	repo_cfg_bool(r, "commitgraph.readchangedpaths", &r->settings.commit_graph_read_changed_paths, 1, 0);
+	repo_cfg_bool(r, "gc.writecommitgraph", &r->settings.gc_write_commit_graph, 1, 0);
+	repo_cfg_bool(r, "fetch.writecommitgraph", &r->settings.fetch_write_commit_graph, 0, 0);
+	repo_cfg_bool(r, "pack.usesparse", &r->settings.pack_use_sparse, 1, 0);
+	repo_cfg_bool(r, "core.multipackindex", &r->settings.core_multi_pack_index, 1, 1);
+	repo_cfg_bool(r, "index.sparse", &r->settings.sparse_index, 0, 0);
 
 	/*
 	 * The GIT_TEST_MULTI_PACK_INDEX variable is special in that
-- 
2.35.1.582.g270d558070.dirty

Re: [PATCH v3 5/5] bitmap: add trace2 outputs during open "bitmap" file

[Jeff Hostetler]

On 6/21/22 9:25 AM, Teng Long wrote:
> It's supported for a repo to use bitmap in both single-pack bitmap
> way or a multi-pack(MIDX) bitmap. Either of two bitmap kinds can
> exist in the repository, or both can be stored but let the config
> controls which kind of bitmap is used (like "core.multipackIndex",
> etc.). Because of this, sometimes the bitmap debug path is not
> obvious enough, for example, when executing:
> 
>   $ git rev-list  --test-bitmap  HEAD
>   fatal: failed to load bitmap indexes
> 
> When the output look like this, it's unclear which kind(s) of
> .bitmap exist, and which were read. For example, it's possible a MIDX
> bitmap exists, but was not read (e.g., because
> core.multiPackIndex=false), among many other scenarios.
> 
> Therefore, we added some trace2 code so that when we read the bitmap
> we can be more clear about the decision path, such as whether it is
> working on MIDX or single-pack bitmap at present, or the related config
> is enabled or not. This may help with logging, user troubleshooting, and
> development debugging.
> 
> Here are some brief output (omitted some unrelated or repetitive rows
> and columns, using "..." instead of) examples on two scenarios when
> executing:
> 
>    $ GIT_TRACE2_PERF=1 git rev-list --test-bitmap HEAD
> 
> Scenario 1:
>    core.multipackIndex [false], MIDX bitmap exists [Y], single-pack bitmap exists [N]
> 
>    ...
>    ... | main                     | data         | r1  | ... | config       | core.multipackindex:false
>    ... | d0 | main                | region_enter | r1  | ... | pack-bitmap  | label:open_bitmap
>    ... | d0 | main                | data         | r1  | ... | bitmap       | ..path:.git/objects/pack/pack-e9eb18e6a423057f4424a762069e13804a75d01e.bitmap
>    ... | main                     | region_leave | r1  | ... | pack-bitmap  | label:open_bitmap
>    ... | main                     | error        |     | ... |              | failed to load bitmap indexes
>    fatal: failed to load bitmap indexes
>    ... | d0 | main                | exit         |     | ... |              | code:128
>    ...
> 
> Scenario 2:
>    core.multipackIndex [false], MIDX bitmap exists [Y], single-pack bitmap exists [Y]
> 
>    ... | d0 | main                | region_enter | r0  | ... | pack-bitmap  | label:open_bitmap
>    ... | d0 | main                | data         | r0  | ... | bitmap       | ..path:/home/tenglong.tl/test/dyrone_bitmap/.git/objects/pack/pack-e9eb18e6a423057f4424a762069e13804a75d01e.bitmap
>    ... | main                     | region_leave | r0  | ... | pack-bitmap  | label:open_bitmap
>    Bitmap v1 test (1 entries loaded)
>    Found bitmap for d864fefa87415d6cd289c72aa9ffd45b4a8ffd84. 64 bits / 11030517 checksum
>    ... | main                     | region_enter | r0  | ... | progress     | label:Verifying bitmap entries
>    Verifying bitmap entries: 100% (3/3), done.
>    ... | main                     | data         | r0  | ... | progress     | ..total_objects:3
>    ... | main                     | region_leave | r0  | ... | progress     | label:Verifying bitmap entries
>    OK!
>    ... | d0 | main                | exit         |     | ... |              | code:0
>    ...
> 
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>   pack-bitmap.c   | 21 ++++++++++++++++-----
>   repo-settings.c | 22 ++++++++++++----------
>   2 files changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index a54d5a0c9f..c39d722592 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -312,9 +312,12 @@ char *pack_bitmap_filename(struct packed_git *p)
>   static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
>   			      struct multi_pack_index *midx)
>   {
> +	int fd;
>   	struct stat st;
> +
>   	char *bitmap_name = midx_bitmap_filename(midx);
> -	int fd = git_open(bitmap_name);
> +	trace2_data_string("midx", the_repository, "path", bitmap_name);
> +	fd = git_open(bitmap_name);
>   
>   	free(bitmap_name);
>   
> @@ -346,8 +349,10 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
>   	if (load_bitmap_header(bitmap_git) < 0)
>   		goto cleanup;
>   
> -	if (!hasheq(get_midx_checksum(bitmap_git->midx), bitmap_git->checksum))
> +	if (!hasheq(get_midx_checksum(bitmap_git->midx), bitmap_git->checksum)) {
> +		error(_("midx and bitmap checksum don't match"));
>   		goto cleanup;
> +	}
>   
>   	if (load_midx_revindex(bitmap_git->midx) < 0) {
>   		warning(_("multi-pack bitmap is missing required reverse index"));
> @@ -374,6 +379,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
>   		return -1;
>   
>   	bitmap_name = pack_bitmap_filename(packfile);
> +	trace2_data_string("bitmap", the_repository, "path", bitmap_name);
>   	fd = git_open(bitmap_name);
>   	free(bitmap_name);
>   
> @@ -509,11 +515,16 @@ static int open_midx_bitmap(struct repository *r,
>   static int open_bitmap(struct repository *r,
>   		       struct bitmap_index *bitmap_git)
>   {
> -	assert(!bitmap_git->map);
> +	int ret = 0;
>   
> +	assert(!bitmap_git->map);
> +	trace2_region_enter("pack-bitmap", "open_bitmap", r);
>   	if (!open_midx_bitmap(r, bitmap_git))
> -		return 0;
> -	return open_pack_bitmap(r, bitmap_git);
> +		goto done;
> +	ret = open_pack_bitmap(r, bitmap_git);
> +done:
> +	trace2_region_leave("pack-bitmap", "open_bitmap", r);
> +	return ret;
>   }
>   
>   struct bitmap_index *prepare_bitmap_git(struct repository *r)
> diff --git a/repo-settings.c b/repo-settings.c
> index b4fbd16cdc..115d96ece3 100644
> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -4,10 +4,12 @@
>   #include "midx.h"
>   
>   static void repo_cfg_bool(struct repository *r, const char *key, int *dest,
> -			  int def)
> +			  int def, int trace)
>   {
>   	if (repo_config_get_bool(r, key, dest))
>   		*dest = def;
> +	if (trace)
> +		trace2_data_string("config", r, key, *dest ? "true" : "false");
>   }
>   

(I just sent a response to your V2 before I saw your V3, so I'll
my response here so that it doesn't get lost.)


We should not be doing this.  This would dump every repo-related
boolean value on every command.  I see that in V3 that you have
a "trace" flag to control this.  But again, this seems wrong here.

I already have a GIT_TRACE2_CONFIG_PARAMS and trace2.configparams
that will dump "interesting" config values to the trace2 log.
Just set one of them to a list of regex's.  Look at the comment above
trace2_cmd_list_config() in trace2.h for details.

We also have GIT_TRACE2_ENV_VARS and trace2.envvars that will dump
the values of "interesting" env vars.

You can use these in your testing to log the config and env var
values that you are interested in.


>   void prepare_repo_settings(struct repository *r)
> @@ -29,8 +31,8 @@ void prepare_repo_settings(struct repository *r)
>   	r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;
>   
>   	/* Booleans config or default, cascades to other settings */
> -	repo_cfg_bool(r, "feature.manyfiles", &manyfiles, 0);
> -	repo_cfg_bool(r, "feature.experimental", &experimental, 0);
> +	repo_cfg_bool(r, "feature.manyfiles", &manyfiles, 0, 0);
> +	repo_cfg_bool(r, "feature.experimental", &experimental, 0, 0);
>   
>   	/* Defaults modified by feature.* */
>   	if (experimental) {
> @@ -42,13 +44,13 @@ void prepare_repo_settings(struct repository *r)
>   	}
>   
>   	/* Boolean config or default, does not cascade (simple)  */
> -	repo_cfg_bool(r, "core.commitgraph", &r->settings.core_commit_graph, 1);
> -	repo_cfg_bool(r, "commitgraph.readchangedpaths", &r->settings.commit_graph_read_changed_paths, 1);
> -	repo_cfg_bool(r, "gc.writecommitgraph", &r->settings.gc_write_commit_graph, 1);
> -	repo_cfg_bool(r, "fetch.writecommitgraph", &r->settings.fetch_write_commit_graph, 0);
> -	repo_cfg_bool(r, "pack.usesparse", &r->settings.pack_use_sparse, 1);
> -	repo_cfg_bool(r, "core.multipackindex", &r->settings.core_multi_pack_index, 1);
> -	repo_cfg_bool(r, "index.sparse", &r->settings.sparse_index, 0);
> +	repo_cfg_bool(r, "core.commitgraph", &r->settings.core_commit_graph, 1, 0);
> +	repo_cfg_bool(r, "commitgraph.readchangedpaths", &r->settings.commit_graph_read_changed_paths, 1, 0);
> +	repo_cfg_bool(r, "gc.writecommitgraph", &r->settings.gc_write_commit_graph, 1, 0);
> +	repo_cfg_bool(r, "fetch.writecommitgraph", &r->settings.fetch_write_commit_graph, 0, 0);
> +	repo_cfg_bool(r, "pack.usesparse", &r->settings.pack_use_sparse, 1, 0);
> +	repo_cfg_bool(r, "core.multipackindex", &r->settings.core_multi_pack_index, 1, 1);
> +	repo_cfg_bool(r, "index.sparse", &r->settings.sparse_index, 0, 0);

So if I'm reading this right, this will only print the value of
"core.multipackindex".

You can get this without the changes here using

GIT_TRACE2_CONFIG_PARAMS="core.multipackindex,...any_other_values_of_interest..."
before running the command (or use the config setting) before running
your commandss.

Alternatively, just emit a "trace2_data_intmax()" on the one value your
want to print here.

Jeff


>   
>   	/*
>   	 * The GIT_TEST_MULTI_PACK_INDEX variable is special in that
> 

Re: [PATCH v3 5/5] bitmap: add trace2 outputs during open "bitmap" file

[Junio C Hamano]

Jeff Hostetler <git@jeffhostetler.com> writes:

> (I just sent a response to your V2 before I saw your V3, so I'll
> my response here so that it doesn't get lost.)
>
>
> We should not be doing this.  This would dump every repo-related
> boolean value on every command.  I see that in V3 that you have
> a "trace" flag to control this.  But again, this seems wrong here.

I noticed that "flag" while merging this with another topic to
"seen" and found it strange, too.  It "allows" the caller to choose
which one gets logged per variable, but in a very hard-coded way; we
probably would have been much better off to have a table of what
gets logged and have repo_cfg_*() calls consult it.  Even without
configurability, at least it would move the hard-coded choice of
what gets logged from code to data.

But ...

> I already have a GIT_TRACE2_CONFIG_PARAMS and trace2.configparams
> that will dump "interesting" config values to the trace2 log.
> Just set one of them to a list of regex's.  Look at the comment above
> trace2_cmd_list_config() in trace2.h for details.

... that does sound like the right way to go.

> ...
> So if I'm reading this right, this will only print the value of
> "core.multipackindex".
>
> You can get this without the changes here using
>
> GIT_TRACE2_CONFIG_PARAMS="core.multipackindex,...any_other_values_of_interest..."
> before running the command (or use the config setting) before running
> your commandss.

Thanks.

[PATCH v5 0/5] tr2: avoid to print "interesting" config repeatedly

[Teng Long]

Changes since v4:

By Jeff Hostetler's suggestion, we can use GIT_TRACE2_CONFIG_PARAMS and
trace2.configparams to dump "interesting" config values to the trace2 log.
So I drop the commit [5/5] 'bitmap: add trace2 outputs during open "bitmap"
file' in v4.

Then I found if a config key exist multiple config values in different scope
the trace2 log will print the value repeatly but not only print the final
active value, so a new commit "avoid to print "interesting" config repeatedly"
is appended in v5.

Finally, compared with [3/5] in v4, commit "pack-bitmap.c: retrieve missing
i18n translations" fix every place which was missing translation in pack-bitmap.c.

Thanks.

Teng Long (5):
  pack-bitmap.c: continue looping when first MIDX bitmap is found
  pack-bitmap.c: rename "idx_name" to "bitmap_name"
  pack-bitmap.c: using error() instead of silently returning -1
  pack-bitmap.c: retrieve missing i18n translations
  tr2: avoid to print "interesting" config repeatedly

 pack-bitmap.c    | 105 ++++++++++++++++++++++++-----------------------
 trace2/tr2_cfg.c |   9 +++-
 2 files changed, 61 insertions(+), 53 deletions(-)

Range-diff against v4:
1:  d8dfe53dd4 < -:  ---------- pack-bitmap.c: make warnings support i18N when opening bitmap
-:  ---------- > 1:  589e3f4075 pack-bitmap.c: continue looping when first MIDX bitmap is found
-:  ---------- > 2:  b6b30047fc pack-bitmap.c: rename "idx_name" to "bitmap_name"
2:  917551f2b5 = 3:  82d4493a6e pack-bitmap.c: using error() instead of silently returning -1
3:  8735ae9979 < -:  ---------- bitmap: add trace2 outputs during open "bitmap" file
-:  ---------- > 4:  065b7c9ccb pack-bitmap.c: retrieve missing i18n translations
-:  ---------- > 5:  f3b87a33da tr2: avoid to print "interesting" config repeatedly
-- 
2.35.1.582.gf3b87a33da

[PATCH v5 1/5] pack-bitmap.c: continue looping when first MIDX bitmap is found

[Teng Long]

In "open_midx_bitmap()", we do a loop with the MIDX(es) in repo, when
the first one has been found, then will break out by a "return"
directly.

But actually, it's better to continue the loop until we have visited
both the MIDX in our repository, as well as any alternates (along with
_their_ alternates, recursively).

The reason for this is, there may exist more than one MIDX file in
a repo. The "multi_pack_index" struct is actually designed as a singly
linked list, and if a MIDX file has been already opened successfully,
then the other MIDX files will be skipped and left with a warning
"ignoring extra bitmap file." to the output.

The discussion link of community:

  https://public-inbox.org/git/YjzCTLLDCby+kJrZ@nand.local/

Helped-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 9c666cdb8b..112c2b12c6 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -494,15 +494,16 @@ 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;
+			ret = 0;
 	}
-	return -1;
+	return ret;
 }
 
 static int open_bitmap(struct repository *r,
-- 
2.35.1.582.gf3b87a33da

[PATCH v5 2/5] pack-bitmap.c: rename "idx_name" to "bitmap_name"

[Teng Long]

In "open_pack_bitmap_1()" and "open_midx_bitmap_1()" we use
a var named "idx_name" to represent the bitmap filename which
is computed by "midx_bitmap_filename()" or "pack_bitmap_filename()"
before we open it.

There may bring some confusion in this "idx_name" naming, which
might lead us to think of ".idx "or" multi-pack-index" files,
although bitmap is essentially can be understood as a kind of index,
let's define this name a little more accurate here.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 112c2b12c6..f13a6bfe3a 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -313,10 +313,10 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 			      struct multi_pack_index *midx)
 {
 	struct stat st;
-	char *idx_name = midx_bitmap_filename(midx);
-	int fd = git_open(idx_name);
+	char *bitmap_name = midx_bitmap_filename(midx);
+	int fd = git_open(bitmap_name);
 
-	free(idx_name);
+	free(bitmap_name);
 
 	if (fd < 0)
 		return -1;
@@ -368,14 +368,14 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 {
 	int fd;
 	struct stat st;
-	char *idx_name;
+	char *bitmap_name;
 
 	if (open_pack_index(packfile))
 		return -1;
 
-	idx_name = pack_bitmap_filename(packfile);
-	fd = git_open(idx_name);
-	free(idx_name);
+	bitmap_name = pack_bitmap_filename(packfile);
+	fd = git_open(bitmap_name);
+	free(bitmap_name);
 
 	if (fd < 0)
 		return -1;
-- 
2.35.1.582.gf3b87a33da

[PATCH v5 3/5] pack-bitmap.c: using error() instead of silently returning -1

[Teng Long]

In "open_pack_bitmap_1()" and "open_midx_bitmap_1()", it's better to
return error() instead of "-1" when some unexpected error occurs like
"stat bitmap file failed", "bitmap header is invalid" or "checksum
mismatch", etc.

There are places where we do not replace, such as when the bitmap
does not exist (no bitmap in repository is allowed) or when another
bitmap has already been opened (in which case it should be a warning
rather than an error).

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index f13a6bfe3a..9f60dbf282 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -323,7 +323,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 
 	if (fstat(fd, &st)) {
 		close(fd);
-		return -1;
+		return error_errno(_("cannot stat bitmap file"));
 	}
 
 	if (bitmap_git->pack || bitmap_git->midx) {
@@ -361,7 +361,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 	bitmap_git->map_pos = 0;
 	bitmap_git->map = NULL;
 	bitmap_git->midx = NULL;
-	return -1;
+	return error(_("cannot open midx bitmap file"));
 }
 
 static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git *packfile)
@@ -382,7 +382,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 
 	if (fstat(fd, &st)) {
 		close(fd);
-		return -1;
+		return error_errno(_("cannot stat bitmap file"));
 	}
 
 	if (bitmap_git->pack || bitmap_git->midx) {
@@ -394,7 +394,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 
 	if (!is_pack_valid(packfile)) {
 		close(fd);
-		return -1;
+		return error(_("packfile is invalid"));
 	}
 
 	bitmap_git->pack = packfile;
@@ -409,7 +409,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 		bitmap_git->map_size = 0;
 		bitmap_git->map_pos = 0;
 		bitmap_git->pack = NULL;
-		return -1;
+		return error(_("bitmap header is invalid"));
 	}
 
 	return 0;
-- 
2.35.1.582.gf3b87a33da

Re: [PATCH v5 3/5] pack-bitmap.c: using error() instead of silently returning -1

[Junio C Hamano]

Teng Long <dyroneteng@gmail.com> writes:

>  	if (fstat(fd, &st)) {
>  		close(fd);
> -		return -1;
> +		return error_errno(_("cannot stat bitmap file"));

"stat" -> "fstat" perhaps?

Not a new problem, but before this hunk, we have

	fd = git_open(...);
	if (fd < 0)
		return -1;

where it probably is legitimate to ignore ENOENT silently.  In
practice, it may be OK to treat a file that exists but cannot be
opened for whatever reason, but I do not think it would hurt to
report such a rare anomaly with a warning, e.g.

	if (fd < 0) {
		if (errno != ENOENT)
			warning("'%s' cannot open '%s'",
				strerror(errno), bitmap_name);
		free(bitmap_name);
		return -1;
	}

or something like that perhaps.

> @@ -361,7 +361,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
>  	bitmap_git->map_pos = 0;
>  	bitmap_git->map = NULL;
>  	bitmap_git->midx = NULL;
> -	return -1;
> +	return error(_("cannot open midx bitmap file"));

This is not exactly "cannot open".  We opened the file, and found
there was something we do not like in it.  If we failed to find midx
lacks revindex chunk, we already would have given a warning, and the
error is not just misleading but is redundant.  load_bitmap_header()
also gives an error() on its own.

I think this patch aims for a good end result, but needs more work.
At least, checksum mismatch that goes to cleanup also needs to issue
its own error() and then we can remove this "cannot open" at the end.

> @@ -382,7 +382,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
>  
>  	if (fstat(fd, &st)) {
>  		close(fd);
> -		return -1;
> +		return error_errno(_("cannot stat bitmap file"));

"stat" -> "fstat"

> @@ -394,7 +394,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
>  
>  	if (!is_pack_valid(packfile)) {
>  		close(fd);
> -		return -1;
> +		return error(_("packfile is invalid"));

Same "sometimes redundant" comment applies here, but not due to this
part of the code but due to the helpers called from is_pack_valid().

Namely, packfile.c::open_packed_git_1() is mostly chatty, but is
silent upon "unable to open" and "unable to fstat" (which I think is
safe to make chatty as well---we do not want to special case ENOENT
in open_packed_git(), so "cannot open because it is not there" is an
error).

And then, this "invalid" will become redundant and fuzzier message
than what is_pack_valid() would have already given, so you can leave
it to silently return -1 here.

> @@ -409,7 +409,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
>  		bitmap_git->map_size = 0;
>  		bitmap_git->map_pos = 0;
>  		bitmap_git->pack = NULL;
> -		return -1;
> +		return error(_("bitmap header is invalid"));

Having shown how to analize these kind of things twice in the above,
I would not bother checking myself, but you should look at the
existing error messages from load_bitmap_header() and see if this
extra message should really be here.  I suspect not.

>  	}
>  
>  	return 0;

[PATCH v5 4/5] pack-bitmap.c: retrieve missing i18n translations

[Teng Long]

In pack-bitmap.c, some printed texts are translated,some are not.
Let's support the translations of the bitmap related output.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c | 76 +++++++++++++++++++++++++--------------------------
 1 file changed, 38 insertions(+), 38 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 9f60dbf282..922b9cbc54 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -138,7 +138,7 @@ static struct ewah_bitmap *read_bitmap_1(struct bitmap_index *index)
 		index->map_size - index->map_pos);
 
 	if (bitmap_size < 0) {
-		error("Failed to load bitmap index (corrupted?)");
+		error(_("Failed to load bitmap index (corrupted?)"));
 		ewah_pool_free(b);
 		return NULL;
 	}
@@ -160,14 +160,14 @@ static int load_bitmap_header(struct bitmap_index *index)
 	size_t header_size = sizeof(*header) - GIT_MAX_RAWSZ + the_hash_algo->rawsz;
 
 	if (index->map_size < header_size + the_hash_algo->rawsz)
-		return error("Corrupted bitmap index (too small)");
+		return error(_("Corrupted bitmap index (too small)"));
 
 	if (memcmp(header->magic, BITMAP_IDX_SIGNATURE, sizeof(BITMAP_IDX_SIGNATURE)) != 0)
-		return error("Corrupted bitmap index file (wrong header)");
+		return error(_("Corrupted bitmap index file (wrong header)"));
 
 	index->version = ntohs(header->version);
 	if (index->version != 1)
-		return error("Unsupported version for bitmap index file (%d)", index->version);
+		return error(_("Unsupported version for bitmap index file (%d)"), index->version);
 
 	/* Parse known bitmap format options */
 	{
@@ -176,12 +176,12 @@ static int load_bitmap_header(struct bitmap_index *index)
 		unsigned char *index_end = index->map + index->map_size - the_hash_algo->rawsz;
 
 		if ((flags & BITMAP_OPT_FULL_DAG) == 0)
-			return error("Unsupported options for bitmap index file "
-				"(Git requires BITMAP_OPT_FULL_DAG)");
+			return error(_("Unsupported options for bitmap index file "
+				"(Git requires BITMAP_OPT_FULL_DAG)"));
 
 		if (flags & BITMAP_OPT_HASH_CACHE) {
 			if (cache_size > index_end - index->map - header_size)
-				return error("corrupted bitmap index file (too short to fit hash cache)");
+				return error(_("corrupted bitmap index file (too short to fit hash cache)"));
 			index->hashes = (void *)(index_end - cache_size);
 			index_end -= cache_size;
 		}
@@ -215,7 +215,7 @@ static struct stored_bitmap *store_bitmap(struct bitmap_index *index,
 	 * because the SHA1 already existed on the map. this is bad, there
 	 * shouldn't be duplicated commits in the index */
 	if (ret == 0) {
-		error("Duplicate entry in bitmap index: %s", oid_to_hex(oid));
+		error(_("Duplicate entry in bitmap index: %s"), oid_to_hex(oid));
 		return NULL;
 	}
 
@@ -259,14 +259,14 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)
 		struct object_id oid;
 
 		if (index->map_size - index->map_pos < 6)
-			return error("corrupt ewah bitmap: truncated header for entry %d", i);
+			return error(_("corrupt ewah bitmap: truncated header for entry %d)"), i);
 
 		commit_idx_pos = read_be32(index->map, &index->map_pos);
 		xor_offset = read_u8(index->map, &index->map_pos);
 		flags = read_u8(index->map, &index->map_pos);
 
 		if (nth_bitmap_object_oid(index, &oid, commit_idx_pos) < 0)
-			return error("corrupt ewah bitmap: commit index %u out of range",
+			return error(_("corrupt ewah bitmap: commit index %u out of range"),
 				     (unsigned)commit_idx_pos);
 
 		bitmap = read_bitmap_1(index);
@@ -274,13 +274,13 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)
 			return -1;
 
 		if (xor_offset > MAX_XOR_OFFSET || xor_offset > i)
-			return error("Corrupted bitmap pack index");
+			return error(_("Corrupted bitmap pack index"));
 
 		if (xor_offset > 0) {
 			xor_bitmap = recent_bitmaps[(i - xor_offset) % MAX_XOR_OFFSET];
 
 			if (xor_bitmap == NULL)
-				return error("Invalid XOR offset in bitmap pack index");
+				return error(_("Invalid XOR offset in bitmap pack index"));
 		}
 
 		recent_bitmaps[i % MAX_XOR_OFFSET] = store_bitmap(
@@ -305,7 +305,7 @@ char *pack_bitmap_filename(struct packed_git *p)
 	size_t len;
 
 	if (!strip_suffix(p->pack_name, ".pack", &len))
-		BUG("pack_name does not end in .pack");
+		BUG(_("pack_name does not end in .pack"));
 	return xstrfmt("%.*s.bitmap", (int)len, p->pack_name);
 }
 
@@ -330,7 +330,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 		struct strbuf buf = STRBUF_INIT;
 		get_midx_filename(&buf, midx->object_dir);
 		/* ignore extra bitmap file; we can only handle one */
-		warning("ignoring extra bitmap file: %s", buf.buf);
+		warning(_("ignoring extra bitmap file: %s"), buf.buf);
 		close(fd);
 		strbuf_release(&buf);
 		return -1;
@@ -387,7 +387,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 
 	if (bitmap_git->pack || bitmap_git->midx) {
 		/* ignore extra bitmap file; we can only handle one */
-		warning("ignoring extra bitmap file: %s", packfile->pack_name);
+		warning(_("ignoring extra bitmap file: %s"), packfile->pack_name);
 		close(fd);
 		return -1;
 	}
@@ -819,7 +819,7 @@ static struct bitmap *find_objects(struct bitmap_index *bitmap_git,
 		revs->include_check_data = &incdata;
 
 		if (prepare_revision_walk(revs))
-			die("revision walk setup failed");
+			die(_("revision walk setup failed"));
 
 		show_data.bitmap_git = bitmap_git;
 		show_data.base = base;
@@ -1134,7 +1134,7 @@ static void filter_bitmap_tree_depth(struct bitmap_index *bitmap_git,
 				     unsigned long limit)
 {
 	if (limit)
-		BUG("filter_bitmap_tree_depth given non-zero limit");
+		BUG(_("filter_bitmap_tree_depth given non-zero limit"));
 
 	filter_bitmap_exclude_type(bitmap_git, tip_objects, to_filter,
 				   OBJ_TREE);
@@ -1148,7 +1148,7 @@ static void filter_bitmap_object_type(struct bitmap_index *bitmap_git,
 				      enum object_type object_type)
 {
 	if (object_type < OBJ_COMMIT || object_type > OBJ_TAG)
-		BUG("filter_bitmap_object_type given invalid object");
+		BUG(_("filter_bitmap_object_type given invalid object"));
 
 	if (object_type != OBJ_TAG)
 		filter_bitmap_exclude_type(bitmap_git, tip_objects, to_filter, OBJ_TAG);
@@ -1304,14 +1304,14 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs,
 		revs->ignore_missing_links = 0;
 
 		if (haves_bitmap == NULL)
-			BUG("failed to perform bitmap walk");
+			BUG(_("failed to perform bitmap walk"));
 	}
 
 	wants_bitmap = find_objects(bitmap_git, revs, wants, haves_bitmap,
 				    filter);
 
 	if (!wants_bitmap)
-		BUG("failed to perform bitmap walk");
+		BUG(_("failed to perform bitmap walk"));
 
 	if (haves_bitmap)
 		bitmap_and_not(wants_bitmap, haves_bitmap);
@@ -1432,7 +1432,7 @@ uint32_t midx_preferred_pack(struct bitmap_index *bitmap_git)
 {
 	struct multi_pack_index *m = bitmap_git->midx;
 	if (!m)
-		BUG("midx_preferred_pack: requires non-empty MIDX");
+		BUG(_("midx_preferred_pack: requires non-empty MIDX"));
 	return nth_midxed_pack_int_id(m, pack_pos_to_midx(bitmap_git->midx, 0));
 }
 
@@ -1629,15 +1629,15 @@ static void test_bitmap_type(struct bitmap_test_data *tdata,
 	}
 
 	if (bitmap_type == OBJ_NONE)
-		die("object %s not found in type bitmaps",
+		die(_("object %s not found in type bitmaps"),
 		    oid_to_hex(&obj->oid));
 
 	if (bitmaps_nr > 1)
-		die("object %s does not have a unique type",
+		die(_("object %s does not have a unique type"),
 		    oid_to_hex(&obj->oid));
 
 	if (bitmap_type != obj->type)
-		die("object %s: real type %s, expected: %s",
+		die(_("object %s: real type %s, expected: %s"),
 		    oid_to_hex(&obj->oid),
 		    type_name(obj->type),
 		    type_name(bitmap_type));
@@ -1651,7 +1651,7 @@ static void test_show_object(struct object *object, const char *name,
 
 	bitmap_pos = bitmap_position(tdata->bitmap_git, &object->oid);
 	if (bitmap_pos < 0)
-		die("Object not in bitmap: %s\n", oid_to_hex(&object->oid));
+		die(_("Object not in bitmap: %s\n"), oid_to_hex(&object->oid));
 	test_bitmap_type(tdata, object, bitmap_pos);
 
 	bitmap_set(tdata->base, bitmap_pos);
@@ -1666,7 +1666,7 @@ static void test_show_commit(struct commit *commit, void *data)
 	bitmap_pos = bitmap_position(tdata->bitmap_git,
 				     &commit->object.oid);
 	if (bitmap_pos < 0)
-		die("Object not in bitmap: %s\n", oid_to_hex(&commit->object.oid));
+		die(_("Object not in bitmap: %s\n"), oid_to_hex(&commit->object.oid));
 	test_bitmap_type(tdata, &commit->object, bitmap_pos);
 
 	bitmap_set(tdata->base, bitmap_pos);
@@ -1683,26 +1683,26 @@ void test_bitmap_walk(struct rev_info *revs)
 	struct ewah_bitmap *bm;
 
 	if (!(bitmap_git = prepare_bitmap_git(revs->repo)))
-		die("failed to load bitmap indexes");
+		die(_("failed to load bitmap indexes"));
 
 	if (revs->pending.nr != 1)
-		die("you must specify exactly one commit to test");
+		die(_("you must specify exactly one commit to test"));
 
-	fprintf(stderr, "Bitmap v%d test (%d entries loaded)\n",
+	fprintf(stderr, _("Bitmap v%d test (%d entries loaded)\n"),
 		bitmap_git->version, bitmap_git->entry_count);
 
 	root = revs->pending.objects[0].item;
 	bm = bitmap_for_commit(bitmap_git, (struct commit *)root);
 
 	if (bm) {
-		fprintf(stderr, "Found bitmap for %s. %d bits / %08x checksum\n",
+		fprintf(stderr, _("Found bitmap for %s. %d bits / %08x checksum\n"),
 			oid_to_hex(&root->oid), (int)bm->bit_size, ewah_checksum(bm));
 
 		result = ewah_to_bitmap(bm);
 	}
 
 	if (result == NULL)
-		die("Commit %s doesn't have an indexed bitmap", oid_to_hex(&root->oid));
+		die(_("Commit %s doesn't have an indexed bitmap"), oid_to_hex(&root->oid));
 
 	revs->tag_objects = 1;
 	revs->tree_objects = 1;
@@ -1711,7 +1711,7 @@ void test_bitmap_walk(struct rev_info *revs)
 	result_popcnt = bitmap_popcount(result);
 
 	if (prepare_revision_walk(revs))
-		die("revision walk setup failed");
+		die(_("revision walk setup failed"));
 
 	tdata.bitmap_git = bitmap_git;
 	tdata.base = bitmap_new();
@@ -1719,7 +1719,7 @@ void test_bitmap_walk(struct rev_info *revs)
 	tdata.trees = ewah_to_bitmap(bitmap_git->trees);
 	tdata.blobs = ewah_to_bitmap(bitmap_git->blobs);
 	tdata.tags = ewah_to_bitmap(bitmap_git->tags);
-	tdata.prg = start_progress("Verifying bitmap entries", result_popcnt);
+	tdata.prg = start_progress(_("Verifying bitmap entries"), result_popcnt);
 	tdata.seen = 0;
 
 	traverse_commit_list(revs, &test_show_commit, &test_show_object, &tdata);
@@ -1727,9 +1727,9 @@ void test_bitmap_walk(struct rev_info *revs)
 	stop_progress(&tdata.prg);
 
 	if (bitmap_equals(result, tdata.base))
-		fprintf(stderr, "OK!\n");
+		fprintf(stderr, _("OK!\n"));
 	else
-		die("mismatch in bitmap results");
+		die(_("mismatch in bitmap results"));
 
 	bitmap_free(result);
 	bitmap_free(tdata.base);
@@ -1747,7 +1747,7 @@ int test_bitmap_commits(struct repository *r)
 	MAYBE_UNUSED void *value;
 
 	if (!bitmap_git)
-		die("failed to load bitmap indexes");
+		die(_("failed to load bitmap indexes"));
 
 	kh_foreach(bitmap_git->bitmaps, oid, value, {
 		printf("%s\n", oid_to_hex(&oid));
@@ -1825,8 +1825,8 @@ uint32_t *create_bitmap_mapping(struct bitmap_index *bitmap_git,
 	if (!bitmap_is_midx(bitmap_git))
 		load_reverse_index(bitmap_git);
 	else if (load_midx_revindex(bitmap_git->midx) < 0)
-		BUG("rebuild_existing_bitmaps: missing required rev-cache "
-		    "extension");
+		BUG(_("rebuild_existing_bitmaps: missing required rev-cache "
+		    "extension"));
 
 	num_objects = bitmap_num_objects(bitmap_git);
 	CALLOC_ARRAY(reposition, num_objects);
-- 
2.35.1.582.gf3b87a33da

Re: [PATCH v5 4/5] pack-bitmap.c: retrieve missing i18n translations

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 28 2022, Teng Long wrote:

> In pack-bitmap.c, some printed texts are translated,some are not.
> Let's support the translations of the bitmap related output.

Usually we don't go for cleanup-while-at-it, but in this case we're
marking messages that don't conform to our CodingGudielines for
translation, mostly because they're error messages that start with an
upper-case letter.

So I think we should fix those issues first, to avoid double-work for
translators (well, a bit less, since they're the translation memory, but
it's quite a bit of churn...).

> -		error("Failed to load bitmap index (corrupted?)");
> +		error(_("Failed to load bitmap index (corrupted?)"));

e.g. here.

> -		return error("Corrupted bitmap index (too small)");
> +		return error(_("Corrupted bitmap index (too small)"));

..and here, etc. etc.

> -		return error("Unsupported version for bitmap index file (%d)", index->version);
> +		return error(_("Unsupported version for bitmap index file (%d)"), index->version);

Let's say "unsupported version '%d' for ..." instead?

> -			return error("Unsupported options for bitmap index file "
> -				"(Git requires BITMAP_OPT_FULL_DAG)");
> +			return error(_("Unsupported options for bitmap index file "
> +				"(Git requires BITMAP_OPT_FULL_DAG)"));

I'm not sure, but shouldn't this be a BUG()?

> -		error("Duplicate entry in bitmap index: %s", oid_to_hex(oid));
> +		error(_("Duplicate entry in bitmap index: %s"), oid_to_hex(oid));

Ditto upper-case, but add '%s' while at it.

>  	if (!strip_suffix(p->pack_name, ".pack", &len))
> -		BUG("pack_name does not end in .pack");
> +		BUG(_("pack_name does not end in .pack"));

Do not translate BUG() messages.

> -		warning("ignoring extra bitmap file: %s", buf.buf);
> +		warning(_("ignoring extra bitmap file: %s"), buf.buf);

Quote the name.

> -		warning("ignoring extra bitmap file: %s", packfile->pack_name);
> +		warning(_("ignoring extra bitmap file: %s"), packfile->pack_name);

ditto.

>  		if (prepare_revision_walk(revs))
> -			die("revision walk setup failed");
> +			die(_("revision walk setup failed"));

Looks good, but aside from this we should really have a tree-wide
xprepare_revision_walk() or something, we have this copy/pasted all over
the place...


> -		BUG("filter_bitmap_tree_depth given non-zero limit");
> +		BUG(_("filter_bitmap_tree_depth given non-zero limit"));

Ditto BUG.
>  
>  	filter_bitmap_exclude_type(bitmap_git, tip_objects, to_filter,
>  				   OBJ_TREE);
> @@ -1148,7 +1148,7 @@ static void filter_bitmap_object_type(struct bitmap_index *bitmap_git,
>  				      enum object_type object_type)
>  {
>  	if (object_type < OBJ_COMMIT || object_type > OBJ_TAG)
> -		BUG("filter_bitmap_object_type given invalid object");
> +		BUG(_("filter_bitmap_object_type given invalid object"));
>  
>  	if (object_type != OBJ_TAG)
>  		filter_bitmap_exclude_type(bitmap_git, tip_objects, to_filter, OBJ_TAG);
> @@ -1304,14 +1304,14 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs,
>  		revs->ignore_missing_links = 0;
>  
>  		if (haves_bitmap == NULL)
> -			BUG("failed to perform bitmap walk");
> +			BUG(_("failed to perform bitmap walk"));
>  	}

etc. etc.

>  
>  	wants_bitmap = find_objects(bitmap_git, revs, wants, haves_bitmap,
>  				    filter);
>  
>  	if (!wants_bitmap)
> -		BUG("failed to perform bitmap walk");
> +		BUG(_("failed to perform bitmap walk"));
>  
>  	if (haves_bitmap)
>  		bitmap_and_not(wants_bitmap, haves_bitmap);
> @@ -1432,7 +1432,7 @@ uint32_t midx_preferred_pack(struct bitmap_index *bitmap_git)
>  {
>  	struct multi_pack_index *m = bitmap_git->midx;
>  	if (!m)
> -		BUG("midx_preferred_pack: requires non-empty MIDX");
> +		BUG(_("midx_preferred_pack: requires non-empty MIDX"));

etc. etc.

>  	return nth_midxed_pack_int_id(m, pack_pos_to_midx(bitmap_git->midx, 0));
>  }
>  
> @@ -1629,15 +1629,15 @@ static void test_bitmap_type(struct bitmap_test_data *tdata,
>  	}
>  
>  	if (bitmap_type == OBJ_NONE)
> -		die("object %s not found in type bitmaps",
> +		die(_("object %s not found in type bitmaps"),
>  		    oid_to_hex(&obj->oid));
>  
>  	if (bitmaps_nr > 1)
> -		die("object %s does not have a unique type",
> +		die(_("object %s does not have a unique type"),
>  		    oid_to_hex(&obj->oid));
>  
>  	if (bitmap_type != obj->type)
> -		die("object %s: real type %s, expected: %s",
> +		die(_("object %s: real type %s, expected: %s"),
>  		    oid_to_hex(&obj->oid),
>  		    type_name(obj->type),
>  		    type_name(bitmap_type));

quote %s for these.

> @@ -1651,7 +1651,7 @@ static void test_show_object(struct object *object, const char *name,
>  
>  	bitmap_pos = bitmap_position(tdata->bitmap_git, &object->oid);
>  	if (bitmap_pos < 0)
> -		die("Object not in bitmap: %s\n", oid_to_hex(&object->oid));
> +		die(_("Object not in bitmap: %s\n"), oid_to_hex(&object->oid));

Lose the \n here, in addition to lower-case & quote %s.

>  	test_bitmap_type(tdata, object, bitmap_pos);
>  
>  	bitmap_set(tdata->base, bitmap_pos);
> @@ -1666,7 +1666,7 @@ static void test_show_commit(struct commit *commit, void *data)
>  	bitmap_pos = bitmap_position(tdata->bitmap_git,
>  				     &commit->object.oid);
>  	if (bitmap_pos < 0)
> -		die("Object not in bitmap: %s\n", oid_to_hex(&commit->object.oid));
> +		die(_("Object not in bitmap: %s\n"), oid_to_hex(&commit->object.oid));

Ditto.

>  	test_bitmap_type(tdata, &commit->object, bitmap_pos);
>  
>  	bitmap_set(tdata->base, bitmap_pos);
> @@ -1683,26 +1683,26 @@ void test_bitmap_walk(struct rev_info *revs)
>  	struct ewah_bitmap *bm;
>  
>  	if (!(bitmap_git = prepare_bitmap_git(revs->repo)))
> -		die("failed to load bitmap indexes");
> +		die(_("failed to load bitmap indexes"));
>  
>  	if (revs->pending.nr != 1)
> -		die("you must specify exactly one commit to test");
> +		die(_("you must specify exactly one commit to test"));
>  
> -	fprintf(stderr, "Bitmap v%d test (%d entries loaded)\n",
> +	fprintf(stderr, _("Bitmap v%d test (%d entries loaded)\n"),
>  		bitmap_git->version, bitmap_git->entry_count);
>  
>  	root = revs->pending.objects[0].item;
>  	bm = bitmap_for_commit(bitmap_git, (struct commit *)root);
>  
>  	if (bm) {
> -		fprintf(stderr, "Found bitmap for %s. %d bits / %08x checksum\n",
> +		fprintf(stderr, _("Found bitmap for %s. %d bits / %08x checksum\n"),
>  			oid_to_hex(&root->oid), (int)bm->bit_size, ewah_checksum(bm));
>  
>  		result = ewah_to_bitmap(bm);
>  	}
>  
>  	if (result == NULL)
> -		die("Commit %s doesn't have an indexed bitmap", oid_to_hex(&root->oid));
> +		die(_("Commit %s doesn't have an indexed bitmap"), oid_to_hex(&root->oid));
>  
>  	revs->tag_objects = 1;
>  	revs->tree_objects = 1;
> @@ -1711,7 +1711,7 @@ void test_bitmap_walk(struct rev_info *revs)
>  	result_popcnt = bitmap_popcount(result);
>  
>  	if (prepare_revision_walk(revs))
> -		die("revision walk setup failed");
> +		die(_("revision walk setup failed"));
>  
>  	tdata.bitmap_git = bitmap_git;
>  	tdata.base = bitmap_new();
> @@ -1719,7 +1719,7 @@ void test_bitmap_walk(struct rev_info *revs)
>  	tdata.trees = ewah_to_bitmap(bitmap_git->trees);
>  	tdata.blobs = ewah_to_bitmap(bitmap_git->blobs);
>  	tdata.tags = ewah_to_bitmap(bitmap_git->tags);
> -	tdata.prg = start_progress("Verifying bitmap entries", result_popcnt);
> +	tdata.prg = start_progress(_("Verifying bitmap entries"), result_popcnt);

Good catch!
>  	tdata.seen = 0;
>  
>  	traverse_commit_list(revs, &test_show_commit, &test_show_object, &tdata);
> @@ -1727,9 +1727,9 @@ void test_bitmap_walk(struct rev_info *revs)
>  	stop_progress(&tdata.prg);
>  
>  	if (bitmap_equals(result, tdata.base))
> -		fprintf(stderr, "OK!\n");
> +		fprintf(stderr, _("OK!\n"));

Ditto don't include \n.
>  	else
> -		die("mismatch in bitmap results");
> +		die(_("mismatch in bitmap results"));
>  
>  	bitmap_free(result);
>  	bitmap_free(tdata.base);
> @@ -1747,7 +1747,7 @@ int test_bitmap_commits(struct repository *r)
>  	MAYBE_UNUSED void *value;
>  
>  	if (!bitmap_git)
> -		die("failed to load bitmap indexes");
> +		die(_("failed to load bitmap indexes"));
>  
>  	kh_foreach(bitmap_git->bitmaps, oid, value, {
>  		printf("%s\n", oid_to_hex(&oid));
> @@ -1825,8 +1825,8 @@ uint32_t *create_bitmap_mapping(struct bitmap_index *bitmap_git,
>  	if (!bitmap_is_midx(bitmap_git))
>  		load_reverse_index(bitmap_git);
>  	else if (load_midx_revindex(bitmap_git->midx) < 0)
> -		BUG("rebuild_existing_bitmaps: missing required rev-cache "
> -		    "extension");
> +		BUG(_("rebuild_existing_bitmaps: missing required rev-cache "
> +		    "extension"));
>  

Ditto don't translate BUG().

Re: [PATCH v5 4/5] pack-bitmap.c: retrieve missing i18n translations

[Eric Sunshine]

On Tue, Jun 28, 2022 at 5:13 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Tue, Jun 28 2022, Teng Long wrote:
> >       if (bitmap_equals(result, tdata.base))
> > -             fprintf(stderr, "OK!\n");
> > +             fprintf(stderr, _("OK!\n"));

As a minor additional bit of help, you can use fprintf_ln() from
strbuf.h which will automatically output the trailing "\n":

    frpintf_ln(stderr, _("OK!"));

(Aside: Use of fprintf() here is a bit odd since there are no
formatting directives in the argument, thus fputs() would have been a
better choice, but that's a cleanup for another day and a different
patch series, probably.)

Re: [PATCH v5 4/5] pack-bitmap.c: retrieve missing i18n translations

[Junio C Hamano]

Teng Long <dyroneteng@gmail.com> writes:

> Subject: Re: [PATCH v5 4/5] pack-bitmap.c: retrieve missing i18n translations

The verb "retrieve" is puzzling.

> In pack-bitmap.c, some printed texts are translated,some are not.

"," -> ", ".

> Let's support the translations of the bitmap related output.

>  	if (bitmap_size < 0) {
> -		error("Failed to load bitmap index (corrupted?)");
> +		error(_("Failed to load bitmap index (corrupted?)"));

If we were to do this, to avoid burdening translators with double
work, we probably would want to fix the "C" locale version of the
string, either as a preliminary clean-up before this step, or as
part of this step.  From Documentation/CodingGuidelines:

        Error Messages

         - Do not end error messages with a full stop.

         - Do not capitalize the first word, only because it is the first word
           in the message ("unable to open %s", not "Unable to open %s").  But
           "SHA-3 not supported" is fine, because the reason the first word is
           capitalized is not because it is at the beginning of the sentence,
           but because the word would be spelled in capital letters even when
           it appeared in the middle of the sentence.

         - Say what the error is first ("cannot open %s", not "%s: cannot open")

[PATCH v5 5/5] tr2: avoid to print "interesting" config repeatedly

[Teng Long]

We can use GIT_TRACE2_CONFIG_PARAMS and trace2.configparams to
dump the config which we are inteseted in to the tr2 log. If
an "interesting" config exists in multiple scopes, it will be
dumped multiple times. So, let's fix it to only print the
final value instead.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
Helped-by: Jeff Hostetler <git@jeffhostetler.com>
---
 trace2/tr2_cfg.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/trace2/tr2_cfg.c b/trace2/tr2_cfg.c
index ec9ac1a6ef..632bb6feec 100644
--- a/trace2/tr2_cfg.c
+++ b/trace2/tr2_cfg.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "config.h"
+#include "strmap.h"
 #include "trace2/tr2_cfg.h"
 #include "trace2/tr2_sysenv.h"
 
@@ -10,6 +11,7 @@ static int tr2_cfg_loaded;
 static struct strbuf **tr2_cfg_env_vars;
 static int tr2_cfg_env_vars_count;
 static int tr2_cfg_env_vars_loaded;
+static struct strset tr_cfg_set = STRSET_INIT;
 
 /*
  * Parse a string containing a comma-delimited list of config keys
@@ -101,12 +103,17 @@ static int tr2_cfg_cb(const char *key, const char *value, void *d)
 {
 	struct strbuf **s;
 	struct tr2_cfg_data *data = (struct tr2_cfg_data *)d;
+	const char *prior_value;
 
 	for (s = tr2_cfg_patterns; *s; s++) {
 		struct strbuf *buf = *s;
 		int wm = wildmatch(buf->buf, key, WM_CASEFOLD);
 		if (wm == WM_MATCH) {
-			trace2_def_param_fl(data->file, data->line, key, value);
+			if (strset_contains(&tr_cfg_set, key)
+			    || git_config_get_value(key, &prior_value))
+				continue;
+			trace2_def_param_fl(data->file, data->line, key, prior_value);
+			strset_add(&tr_cfg_set, key);
 			return 0;
 		}
 	}
-- 
2.35.1.582.gf3b87a33da

Re: [PATCH v5 5/5] tr2: avoid to print "interesting" config repeatedly

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 28 2022, Teng Long wrote:

> We can use GIT_TRACE2_CONFIG_PARAMS and trace2.configparams to
> dump the config which we are inteseted in to the tr2 log. If
> an "interesting" config exists in multiple scopes, it will be
> dumped multiple times. So, let's fix it to only print the
> final value instead.
>
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> Helped-by: Jeff Hostetler <git@jeffhostetler.com>
> ---
>  trace2/tr2_cfg.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/trace2/tr2_cfg.c b/trace2/tr2_cfg.c
> index ec9ac1a6ef..632bb6feec 100644
> --- a/trace2/tr2_cfg.c
> +++ b/trace2/tr2_cfg.c
> @@ -1,5 +1,6 @@
>  #include "cache.h"
>  #include "config.h"
> +#include "strmap.h"
>  #include "trace2/tr2_cfg.h"
>  #include "trace2/tr2_sysenv.h"
>  
> @@ -10,6 +11,7 @@ static int tr2_cfg_loaded;
>  static struct strbuf **tr2_cfg_env_vars;
>  static int tr2_cfg_env_vars_count;
>  static int tr2_cfg_env_vars_loaded;
> +static struct strset tr_cfg_set = STRSET_INIT;
>  
>  /*
>   * Parse a string containing a comma-delimited list of config keys
> @@ -101,12 +103,17 @@ static int tr2_cfg_cb(const char *key, const char *value, void *d)
>  {
>  	struct strbuf **s;
>  	struct tr2_cfg_data *data = (struct tr2_cfg_data *)d;
> +	const char *prior_value;
>  
>  	for (s = tr2_cfg_patterns; *s; s++) {
>  		struct strbuf *buf = *s;
>  		int wm = wildmatch(buf->buf, key, WM_CASEFOLD);
>  		if (wm == WM_MATCH) {
> -			trace2_def_param_fl(data->file, data->line, key, value);
> +			if (strset_contains(&tr_cfg_set, key)
> +			    || git_config_get_value(key, &prior_value))
> +				continue;
> +			trace2_def_param_fl(data->file, data->line, key, prior_value);
> +			strset_add(&tr_cfg_set, key);
>  			return 0;
>  		}
>  	}


Is tr2_cfg_load_patterns() run at early startup, but this perhaps at
runtime? I wonder if this is OK under threading, i.e. concurrent access
to your strset.

The assumption you're making here doesn't hold in general, some config
is "last vaule wins", but for some other config all configured values
are important.

Do we care in this case? I really don't know, but perhaps we can declare
"dedup" using the same facility we're using to wildcard-match keys, and
either make that optional or the default, e.g.:

	GIT_TRACE2_CONFIG_PARAMS='*:dedup,core.*:'

I.e. to make it a list of <glob>[:<options>].

Maybe not worth the effort...

Re: [PATCH v5 5/5] tr2: avoid to print "interesting" config repeatedly

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Do we care in this case? I really don't know, but perhaps we can declare
> "dedup" using the same facility we're using to wildcard-match keys, and
> either make that optional or the default, e.g.:
>
> 	GIT_TRACE2_CONFIG_PARAMS='*:dedup,core.*:'
>
> I.e. to make it a list of <glob>[:<options>].
>
> Maybe not worth the effort...

I happen to think that the trace output is primarily for machine
consumption (i.e. if you want to make it readable by humans, it is
OK to require them to pre-process).

How does this "duplicate output" come about?  If it is because
end-user configuration files have multiple entries that are either
list-valued or relying on last-one-wins semantics, I suspect that it
is better not to prematurely filter these at the trace generation
side (which by definition is an opration that loses information).

So, to me, it smells that the whole "dedup at the source" thing is
not just not worth the effort but is misguided.