trace2 output for bitmap decision path

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/[email protected]/)

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

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

---
 pack-bitmap.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

index 9c666cdb8b..931219adf0 100644
@@ -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

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.

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

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.

---
 pack-bitmap.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 931219adf0..b1357137bf 100644
@@ -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

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?

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

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

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

Thanks for your explanations.

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:

index 9c666cdb8b..38f53b8f1c 100644
@@ -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.

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

index 97909d48da..6e7c89826d 100644
@@ -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:

index 91a6be358d..e64a85bc59 100644
@@ -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;
        }

index f775fc1ce6..a962f6861f 100755
@@ -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.

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

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

index 865170bad0..fda9644028 100644
@@ -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;
 

index b1357137bf..14cf8abebd 100644
@@ -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

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.

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?

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

Sure.

Will fix.

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

No worries.

*Nod*

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.

Exactly.

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 <[email protected]>

## 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/[email protected]/
+
+ Helped-by: Taylor Blau <[email protected]>

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

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/[email protected]/

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

index 9c666cdb8b..112c2b12c6 100644
@@ -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

s/don't stop/continue

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 <[email protected]> explains, but this commit message as-is leaves out some details that are important IMHO.

The actual changes look good to me.

Thanks,
Taylor

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.

---
 pack-bitmap.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

index 112c2b12c6..f13a6bfe3a 100644
@@ -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

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

Thanks,
Taylor

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/[email protected]/#t
2. https://public-inbox.org/git/[email protected]/

---
 pack-bitmap.c           | 6 ++++--
 t/t5310-pack-bitmaps.sh | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

index f13a6bfe3a..1b268f655e 100644
@@ -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;
 	}

index f775fc1ce6..eb63b71852 100755
@@ -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

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

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

Thanks

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

---
 pack-bitmap.c           | 40 +++++++++++++++++++++++++++++++++-------
 repo-settings.c         |  1 +
 t/t5310-pack-bitmaps.sh |  4 ++--
 3 files changed, 36 insertions(+), 9 deletions(-)

index 1b268f655e..a1d06c4252 100644
@@ -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)

index b4fbd16cdc..5bc7a97a6d 100644
@@ -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)

index eb63b71852..664cb88b0b 100755
@@ -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

On Thu, Apr 21 2022, Teng Long wrote:

Thanks for following up..

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.

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.

Make sense.

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

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.

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.

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.

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

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

Thanks.

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

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

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

index a1d06c4252..e0dcd06db3 100644
@@ -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

Yes, I agree with you.

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

Make sense. Will adjust the order in next path.

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

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.

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/[email protected]/
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.

3: 5a8f5afccf ! 3: d8dfe53dd4 pack-bitmap.c: make warnings more detailed when opening bitmap
@@ Metadata
Author: Teng Long <[email protected]>

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

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

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/[email protected]/

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

index 9c666cdb8b..112c2b12c6 100644
@@ -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

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.

---
 pack-bitmap.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

index 112c2b12c6..f13a6bfe3a 100644
@@ -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

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/[email protected]/#t
2. https://public-inbox.org/git/[email protected]/

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

index f13a6bfe3a..af0f41833e 100644
@@ -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

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

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

index af0f41833e..5654eb7b8d 100644
@@ -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

Appreciate for reminding this.
Will fix.

It's an oversight.
Will fix.

As same as the first one, will fix.

Thanks.

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

---
 pack-bitmap.c   | 27 +++++++++++++++++++++------
 repo-settings.c |  1 +
 2 files changed, 22 insertions(+), 6 deletions(-)

index 5654eb7b8d..ced5993560 100644
@@ -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)

index b4fbd16cdc..5bc7a97a6d 100644
@@ -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

You are absolutely right.
Will fix.

Thanks.

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

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?

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.

s/TRACE2/trace2

s/NORMAL/pack

s/$GIT/$ GIT

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.

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

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

Will optimize the relevant places.

Yeah, will Fix.

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

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

Will fix.

Will fix as s/NORMAL/single-pack.

Will fix.

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

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

On 6/12/22 3:44 AM, Teng Long wrote:
[...]

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.

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:

index c5c8cfbbaa..37a3163be1 100644
@@ -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 <[email protected]>
Date:   Thu Jun 23 17:24:15 2022 +0800

tr2: append scope info when same configs exist in multiple scopes

index c5c8cfbbaa..37a3163be1 100644
@@ -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);

index c42fbade7f..e1b036a60c 100644
@@ -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);
 }

index a1eff8bea3..c21bf8e651 100644
@@ -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"}

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

Thanks.

[...]

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

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.

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

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

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/[email protected]/

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

index 9c666cdb8b..112c2b12c6 100644
@@ -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

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.

---
 pack-bitmap.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

index 112c2b12c6..f13a6bfe3a 100644
@@ -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

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/[email protected]/#t
2. https://public-inbox.org/git/[email protected]/

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

index f13a6bfe3a..af0f41833e 100644
@@ -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

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

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

index af0f41833e..a54d5a0c9f 100644
@@ -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

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

---
 pack-bitmap.c   | 21 ++++++++++++++++-----
 repo-settings.c | 22 ++++++++++++----------
 2 files changed, 28 insertions(+), 15 deletions(-)

index a54d5a0c9f..c39d722592 100644
@@ -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)

index b4fbd16cdc..115d96ece3 100644
@@ -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

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

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

Thanks.

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

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/[email protected]/

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

index 9c666cdb8b..112c2b12c6 100644
@@ -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

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.

---
 pack-bitmap.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

index 112c2b12c6..f13a6bfe3a 100644
@@ -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

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

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

index f13a6bfe3a..9f60dbf282 100644
@@ -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

Yes.

Yes.

It's more accurate here with your suggestion. At the same time I found there actually exists many similar place like "ignore ENOENT silently" in repo. And I think it's not worth to impove them right now in this patch, if you want to do that it could in another pathset and please tell me.

Yes, It's detailed here and I missed it, I will fix this in next
patch: return error() when checksum doesn't match and let "cleanup" return "-1" but not an inaccurate error().

Yes.

Let me try to know about it, the "helpers" means the place where invoke is_pack_valid() like here. In fact, in is_pack_valid() they already return the errors in various scenarios, so it would be no need to return another error.

"chatty" means the code that regularly gives information about its internal operations. Did I understand it right because I'm actually firstly know this description.

"cannot open because it is not there" is an error, but I think it will also could be a BUG, actually I'm not very sure for clarify the difference bwtween the use of the two, but I will look into it to dig something out.

Clear now.

Thanks.

It is sufficien to just mark it as #leftoverbits to find and fix places where the code means to ignore only a missing optional file but ignores all other errors it gets from open/fopen instead.