From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Teng Long <dyroneteng@gmail.com>
Cc: git@vger.kernel.org, me@ttaylorr.com, derrickstolee@github.com,
tenglong.tl@alibaba-inc.com
Subject: Re: [PATCH v1 0/3] trace2 output for bitmap decision path
Date: Thu, 24 Mar 2022 14:22:17 +0100 [thread overview]
Message-ID: <220324.867d8jo45p.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <cover.1648119652.git.dyroneteng@gmail.com>
On Thu, Mar 24 2022, Teng Long wrote:
> A Git repo can be chosen to use the normal bitmap (before MIDX) and MIDX bitmap.
>
> I recently tried to understand this part of the MIDX implementation because I found
> a bug which has been discovered and repaired in community [1].
>
> I am grateful to Taylor Blau for his help and for introducing me to the testing
> method according to the `git rev-list --test-bitmap <rev>`.
>
> In the process of understanding and troubleshooting by using this command, I found
> when the execution is failed it will output a single line of
> "fatal: failed to load bitmap indexes", sometimes will be more informations like
> if the bitmap file is broken, the outputs maybe contain
> "error: Corrupted bitmap index file (wrong header)".), but most of time are single
> line output I mentioned above. So, this brings a little obstacle for debugging and
> troubleshooting I think, because "failed to load bitmap indexes" can represent
> to much informations (many causes like: midx config turn off, bitmap inexistence, etc.)
>
> Therefore, as a git repo can be chosen to use the normal bitmap (before MIDX) or
> MIDX bitmap, or they can both exist and let git make the decision. I think why not add
> some extra informations based on TRACE2 to help for showing the bitmap decision path
> clearer and more plentiful, so when the failure occurs the user can use it to debug
> around bitmap in a quicker way.
>
> Thanks.
>
> Links:
> 1. https://public-inbox.org/git/cover.1638991570.git.me@ttaylorr.com/)
>
> Teng Long (3):
> pack-bitmap.c: use "ret" in "open_midx_bitmap()"
> pack-bitmap.c: add "break" statement in "open_pack_bitmap()"
> bitmap: add trace outputs during open "bitmap" file
>
> midx.c | 2 ++
> pack-bitmap.c | 17 +++++++++++++----
> 2 files changed, 15 insertions(+), 4 deletions(-)
>
> Range-diff against v0:
> -: ---------- > 1: 3048b4dd29 pack-bitmap.c: use "ret" in "open_midx_bitmap()"
> -: ---------- > 2: 70500b6343 pack-bitmap.c: add "break" statement in "open_pack_bitmap()"
> -: ---------- > 3: 9912450fc1 bitmap: add trace outputs during open "bitmap" file
Was there an on-list v0 (RFC?) or is this a range-diff against nothing?
Best not to include it until a v2 then.
Comments:
Sometimes it's better to split up patches, but I think these 3x should
really be squashed together. We make incremental progress to nowhere in
1/3 and 2/3, and it all comes together in 3/3. The 1-2/3 are trivial
enough that we can squash them in.
We then end up with this, with my comments added:
midx.c | 2 ++
pack-bitmap.c | 17 +++++++++++++----
2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/midx.c b/midx.c
index 865170bad05..fda96440287 100644
--- a/midx.c
+++ b/midx.c
@@ -392,6 +392,8 @@ int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, i
struct multi_pack_index *m_search;
prepare_repo_settings(r);
+ trace2_data_string("midx", r, "core.multipackIndex",
+ r->settings.core_multi_pack_index ? "true" : "false");
Weird indentation here.
Also, if we think it's a good idea to log these shouldn't it be in
repo_cfg_bool() in repo-settings.c, why is core.multipackIndex out of
all in r->settings special?
if (!r->settings.core_multi_pack_index)
return 0;
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 97909d48da3..cac8d4a978f 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -484,25 +484,34 @@ static int open_pack_bitmap(struct repository *r,
assert(!bitmap_git->map);
for (p = get_all_packs(r); p; p = p->next) {
- if (open_pack_bitmap_1(bitmap_git, p) == 0)
+ if (open_pack_bitmap_1(bitmap_git, p) == 0) {
Aside: If we end up changing this line anyway, it's OK to just change it
to "if (!open...".
ret = 0;
+ break;
+ }
}
+ trace2_data_string("bitmap", the_repository, "open bitmap (non-midx)",
+ ret ? "failed" : "ok");
return ret;
}
static int open_midx_bitmap(struct repository *r,
struct bitmap_index *bitmap_git)
{
+ int ret = -1;
struct multi_pack_index *midx;
assert(!bitmap_git->map);
for (midx = get_multi_pack_index(r); midx; midx = midx->next) {
- if (!open_midx_bitmap_1(bitmap_git, midx))
- return 0;
+ if (!open_midx_bitmap_1(bitmap_git, midx)) {
+ ret = 0;
+ break;
+ }
}
- return -1;
+ trace2_data_string("midx", the_repository, "open bitmap (midx)",
+ ret ? "failed" : "ok");
+ return ret;
}
static int open_bitmap(struct repository *r,
It seems odd not to use trace2 regions for this, and to not add add this
data logging open_bitmap(). I came up with this on top of this when
testing this:
diff --git a/pack-bitmap.c b/pack-bitmap.c
index cac8d4a978f..ba71a7ea5cd 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -318,11 +318,14 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
free(idx_name);
- if (fd < 0)
+ if (fd < 0) {
+ /* TODO: Log trace2_data_string() here, do we care? */
return -1;
+ }
if (fstat(fd, &st)) {
close(fd);
+ /* TODO: Log trace2_data_string() here, do we care? */
return -1;
}
@@ -330,6 +333,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
struct strbuf buf = STRBUF_INIT;
get_midx_filename(&buf, midx->object_dir);
/* ignore extra bitmap file; we can only handle one */
+ /* NOTE: You'll already get a warning (well, "error") event due to this, and it'll be in your region */
warning("ignoring extra bitmap file: %s", buf.buf);
close(fd);
strbuf_release(&buf);
@@ -344,9 +348,11 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
close(fd);
if (load_bitmap_header(bitmap_git) < 0)
+ /* TODO: Add trace2_data_string() or warning/error here? */
goto cleanup;
if (!hasheq(get_midx_checksum(bitmap_git->midx), bitmap_git->checksum))
+ /* TODO: Add trace2_data_string() or warning/error here? */
goto cleanup;
if (load_midx_revindex(bitmap_git->midx) < 0) {
@@ -479,49 +485,44 @@ static int open_pack_bitmap(struct repository *r,
struct bitmap_index *bitmap_git)
{
struct packed_git *p;
- int ret = -1;
-
- assert(!bitmap_git->map);
for (p = get_all_packs(r); p; p = p->next) {
if (open_pack_bitmap_1(bitmap_git, p) == 0) {
- ret = 0;
- break;
+ return 0;
}
}
-
- trace2_data_string("bitmap", the_repository, "open bitmap (non-midx)",
- ret ? "failed" : "ok");
- return ret;
+ return -1;
}
static int open_midx_bitmap(struct repository *r,
struct bitmap_index *bitmap_git)
{
- int ret = -1;
struct multi_pack_index *midx;
- assert(!bitmap_git->map);
-
for (midx = get_multi_pack_index(r); midx; midx = midx->next) {
if (!open_midx_bitmap_1(bitmap_git, midx)) {
- ret = 0;
- break;
+ return 0;
}
}
- trace2_data_string("midx", the_repository, "open bitmap (midx)",
- ret ? "failed" : "ok");
- return ret;
+ return -1;
}
static int open_bitmap(struct repository *r,
struct bitmap_index *bitmap_git)
{
+ int ret;
+
assert(!bitmap_git->map);
- if (!open_midx_bitmap(r, bitmap_git))
- return 0;
- return open_pack_bitmap(r, bitmap_git);
+ trace2_region_enter("pack-bitmap", "open_bitmap", r);
+ if (!open_midx_bitmap(r, bitmap_git)) {
+ ret = 0;
+ goto done;
+ }
+ ret = open_pack_bitmap(r, bitmap_git);
+done:
+ trace2_region_leave("pack-bitmap", "open_bitmap", r);
+ return ret;
}
struct bitmap_index *prepare_bitmap_git(struct repository *r)
I.e. surely you just want to create a region, and if you care enough to
log failure shouldn't we log that in open_midx_bitmap_1() if we care,
and perhaps error() there instead of silently returning -1?
next prev parent reply other threads:[~2022-03-24 13:45 UTC|newest]
Thread overview: 128+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-24 11:43 [PATCH v1 0/3] trace2 output for bitmap decision path Teng Long
2022-03-24 11:43 ` [PATCH v1 1/3] pack-bitmap.c: use "ret" in "open_midx_bitmap()" Teng Long
2022-03-24 19:11 ` Taylor Blau
2022-03-28 7:59 ` [PATCH v1 1/3] pack-bitmap.c: use "ret" in "open_midx_bitmap() Teng Long
2022-03-30 2:39 ` Taylor Blau
2022-03-24 11:44 ` [PATCH v1 2/3] pack-bitmap.c: add "break" statement in "open_pack_bitmap()" Teng Long
2022-03-24 18:40 ` Junio C Hamano
2022-03-24 19:06 ` Taylor Blau
2022-03-24 19:03 ` Taylor Blau
2022-03-29 2:49 ` Teng Long
2022-03-30 2:55 ` Taylor Blau
2022-03-30 7:32 ` Teng Long Teng Long
2022-03-30 13:17 ` Ævar Arnfjörð Bjarmason
2022-03-24 11:44 ` [PATCH v1 3/3] bitmap: add trace outputs during open "bitmap" file Teng Long
2022-03-24 18:42 ` Junio C Hamano
2022-03-24 13:22 ` Ævar Arnfjörð Bjarmason [this message]
2022-03-29 7:38 ` Teng Long Teng Long
2022-03-29 8:54 ` Ævar Arnfjörð Bjarmason
2022-04-21 13:26 ` [PATCH v2 0/5] trace2 output for bitmap decision path Teng Long
2022-04-21 13:26 ` [PATCH v2 1/5] pack-bitmap.c: continue looping when first MIDX bitmap is found Teng Long
2022-05-11 21:31 ` Taylor Blau
2022-04-21 13:26 ` [PATCH v2 2/5] pack-bitmap.c: rename "idx_name" to "bitmap_name" Teng Long
2022-05-11 21:31 ` Taylor Blau
2022-04-21 13:26 ` [PATCH v2 3/5] pack-bitmap.c: make warnings more detailed when opening bitmap Teng Long
2022-04-21 17:25 ` Taylor Blau
2022-05-06 9:08 ` Teng Long
2022-04-21 13:26 ` [PATCH v2 4/5] bitmap: add trace2 outputs during open "bitmap" file Teng Long
2022-04-21 15:51 ` Ævar Arnfjörð Bjarmason
2022-05-06 11:27 ` Teng Long
2022-05-06 11:53 ` Teng Long
2022-04-21 16:32 ` Jeff Hostetler
2022-05-06 12:43 ` Teng Long
2022-05-10 20:47 ` Jeff Hostetler
2022-04-21 13:26 ` [PATCH v2 5/5] pack-bitmap.c: using error() instead of silently returning -1 Teng Long
2022-04-21 15:41 ` Ævar Arnfjörð Bjarmason
2022-05-06 12:55 ` Teng Long
2022-06-12 7:44 ` [PATCH v3 0/5] Teng Long
2022-06-12 7:44 ` [PATCH v3 1/5] pack-bitmap.c: continue looping when first MIDX bitmap is found Teng Long
2022-06-12 7:44 ` [PATCH v3 2/5] pack-bitmap.c: rename "idx_name" to "bitmap_name" Teng Long
2022-06-12 7:44 ` [PATCH v3 3/5] pack-bitmap.c: make warnings support i18N when opening bitmap Teng Long
2022-06-12 7:44 ` [PATCH v3 4/5] pack-bitmap.c: using error() instead of silently returning -1 Teng Long
2022-06-14 1:15 ` Taylor Blau
2022-06-20 13:17 ` Teng Long
2022-06-12 7:44 ` [PATCH v3 5/5] bitmap: add trace2 outputs during open "bitmap" file Teng Long
2022-06-13 20:59 ` Junio C Hamano
2022-06-20 13:32 ` Teng Long
2022-06-14 1:40 ` Taylor Blau
2022-06-21 6:58 ` Teng Long
2022-06-22 12:51 ` Jeff Hostetler
2022-06-23 9:38 ` Teng Long
2022-06-23 15:14 ` Jeff Hostetler
2022-06-24 8:27 ` [PATCH v3 5/5] bitmap: add trace2 outputs during open "bitmap" Teng Long
2022-06-21 13:25 ` [PATCH v3 0/5] trace2 output for bitmap decision path Teng Long
2022-06-21 13:25 ` [PATCH v3 1/5] pack-bitmap.c: continue looping when first MIDX bitmap is found Teng Long
2022-06-21 13:25 ` [PATCH v3 2/5] pack-bitmap.c: rename "idx_name" to "bitmap_name" Teng Long
2022-06-21 13:25 ` [PATCH v3 3/5] pack-bitmap.c: make warnings support i18N when opening bitmap Teng Long
2022-06-21 13:25 ` [PATCH v3 4/5] pack-bitmap.c: using error() instead of silently returning -1 Teng Long
2022-06-21 13:25 ` [PATCH v3 5/5] bitmap: add trace2 outputs during open "bitmap" file Teng Long
2022-06-22 13:04 ` Jeff Hostetler
2022-06-22 15:12 ` Junio C Hamano
2022-06-28 8:17 ` [PATCH v5 0/5] tr2: avoid to print "interesting" config repeatedly Teng Long
2022-06-28 8:17 ` [PATCH v5 1/5] pack-bitmap.c: continue looping when first MIDX bitmap is found Teng Long
2022-06-28 8:17 ` [PATCH v5 2/5] pack-bitmap.c: rename "idx_name" to "bitmap_name" Teng Long
2022-06-28 8:17 ` [PATCH v5 3/5] pack-bitmap.c: using error() instead of silently returning -1 Teng Long
2022-06-28 18:04 ` Junio C Hamano
2022-07-05 9:04 ` Teng Long
2022-07-05 18:23 ` Junio C Hamano
2022-06-28 8:17 ` [PATCH v5 4/5] pack-bitmap.c: retrieve missing i18n translations Teng Long
2022-06-28 8:58 ` Ævar Arnfjörð Bjarmason
2022-06-28 17:28 ` Eric Sunshine
2022-07-06 14:19 ` Teng Long
2022-07-06 14:06 ` Teng Long
2022-06-28 18:07 ` Junio C Hamano
2022-07-07 11:59 ` Teng Long
2022-07-07 16:45 ` Junio C Hamano
2022-07-11 11:04 ` Teng Long
2022-06-28 8:17 ` [PATCH v5 5/5] tr2: avoid to print "interesting" config repeatedly Teng Long
2022-06-28 9:13 ` Ævar Arnfjörð Bjarmason
2022-06-28 18:12 ` Junio C Hamano
2022-07-01 14:31 ` Jeff Hostetler
2022-07-11 4:11 ` Teng Long
2022-07-11 3:51 ` Teng Long
2022-07-11 12:43 ` [PATCH v6 0/7] trace2: dump scope when print "interesting" config Teng Long
2022-07-11 12:43 ` [PATCH v6 1/7] clean: fixed issues related to text output format Teng Long
2022-07-11 21:08 ` Junio C Hamano
2022-07-13 11:44 ` Teng Long
2022-07-11 12:43 ` [PATCH v6 2/7] pack-bitmap.c: mark more strings for translations Teng Long
2022-07-11 12:43 ` [PATCH v6 3/7] pack-bitmap.c: rename "idx_name" to "bitmap_name" Teng Long
2022-07-11 12:44 ` [PATCH v6 4/7] pack-bitmap.c: don't ignore ENOENT silently Teng Long
2022-07-11 14:38 ` Ævar Arnfjörð Bjarmason
2022-07-13 14:14 ` Teng Long
2022-07-11 21:22 ` Junio C Hamano
2022-07-14 15:25 ` Teng Long
2022-07-11 12:44 ` [PATCH v6 5/7] pack-bitmap.c: using error() instead of silently returning -1 Teng Long
2022-07-11 14:53 ` Ævar Arnfjörð Bjarmason
2022-07-15 2:34 ` Teng Long
2022-07-11 12:44 ` [PATCH v6 6/7] pack-bitmap.c: continue looping when first MIDX bitmap is found Teng Long
2022-07-11 12:44 ` [PATCH v6 7/7] tr2: dump names if config exist in multiple scopes Teng Long
2022-07-11 14:40 ` Ævar Arnfjörð Bjarmason
2022-07-11 19:19 ` Jeff Hostetler
2022-07-11 14:59 ` [PATCH v6 0/7] trace2: dump scope when print "interesting" config Ævar Arnfjörð Bjarmason
2022-07-18 8:36 ` Teng Long
2022-07-18 16:45 ` [PATCH v7 " Teng Long
2022-07-18 16:46 ` [PATCH v7 1/7] pack-bitmap.c: fix formatting of error messages Teng Long
2022-07-18 16:46 ` [PATCH v7 2/7] pack-bitmap.c: mark more strings for translations Teng Long
2022-07-18 16:46 ` [PATCH v7 3/7] pack-bitmap.c: rename "idx_name" to "bitmap_name" Teng Long
2022-07-18 16:46 ` [PATCH v7 4/7] pack-bitmap.c: do not ignore error when opening a bitmap file Teng Long
2022-07-18 16:46 ` [PATCH v7 5/7] pack-bitmap.c: using error() instead of silently returning -1 Teng Long
2022-07-18 16:46 ` [PATCH v7 6/7] pack-bitmap.c: continue looping when first MIDX bitmap is found Teng Long
2022-07-18 16:46 ` [PATCH v7 7/7] tr2: dump names if config exist in multiple scopes Teng Long
2022-07-18 20:13 ` Jeff Hostetler
2022-07-19 7:40 ` tenglong.tl
2022-07-19 21:03 ` Junio C Hamano
2022-07-20 12:48 ` tenglong.tl
2022-07-18 18:57 ` [PATCH v7 0/7] trace2: dump scope when print "interesting" config Junio C Hamano
2022-07-18 19:07 ` Ævar Arnfjörð Bjarmason
2022-07-19 11:26 ` tenglong.tl
2022-07-19 11:42 ` Ævar Arnfjörð Bjarmason
2022-07-19 12:34 ` tenglong.tl
2022-07-21 9:05 ` [PATCH v8 0/6] pack-bitmap.c: optimize error messages tenglong.tl
2022-07-21 9:05 ` [PATCH v8 1/6] pack-bitmap.c: fix formatting of " tenglong.tl
2022-07-21 9:05 ` [PATCH v8 2/6] pack-bitmap.c: mark more strings for translations tenglong.tl
2022-07-21 9:05 ` [PATCH v8 3/6] pack-bitmap.c: rename "idx_name" to "bitmap_name" tenglong.tl
2022-07-21 9:05 ` [PATCH v8 4/6] pack-bitmap.c: do not ignore error when opening a bitmap file tenglong.tl
2022-07-21 9:05 ` [PATCH v8 5/6] pack-bitmap.c: using error() instead of silently returning -1 tenglong.tl
2022-07-21 9:05 ` [PATCH v8 6/6] pack-bitmap.c: continue looping when first MIDX bitmap is found tenglong.tl
2022-07-21 23:01 ` [PATCH v8 0/6] pack-bitmap.c: optimize error messages Junio C Hamano
2022-07-22 6:17 ` tenglong.tl
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=220324.867d8jo45p.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=derrickstolee@github.com \
--cc=dyroneteng@gmail.com \
--cc=git@vger.kernel.org \
--cc=me@ttaylorr.com \
--cc=tenglong.tl@alibaba-inc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).