On Thu, Apr 21 2022, Teng Long wrote:
Thanks for following up..
> 19:38:43.007840 common-main.c:49 | d0 | main | version | | | | | 2.35.1.582.g8e9092487a
> 19:38:43.007874 common-main.c:50 | d0 | main | start | | 0.000305 | | | /opt/git/master/bin/git rev-list --test-bitmap HEAD
It's really helpful to have these full examples in the commit
message. Thanks.
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -312,9 +312,12 @@ char *pack_bitmap_filename(struct packed_git *p)
> static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
> struct multi_pack_index *midx)
> {
> + int fd;
> struct stat st;
> char *bitmap_name = midx_bitmap_filename(midx);
> - int fd = git_open(bitmap_name);
> + trace2_data_string("midx", the_repository, "try to open bitmap",
> + bitmap_name);
> + fd = git_open(bitmap_name);
>
> free(bitmap_name);
Hrm, so re my comment on 5/5 are you trying to use the "try to open" as
a timer to see what our start time is?
I think probably not, in which case this whole variable flip-around is
something we won't need.
But if we do actually need it perhaps a sub-region for the timing?
> @@ -511,11 +530,18 @@ static int open_midx_bitmap(struct repository *r,
> static int open_bitmap(struct repository *r,
> struct bitmap_index *bitmap_git)
> {
> - assert(!bitmap_git->map);
> + int ret = -1;
>
> - if (!open_midx_bitmap(r, bitmap_git))
> - return 0;
> - return open_pack_bitmap(r, bitmap_git);
> + assert(!bitmap_git->map);
> + trace2_region_enter("pack-bitmap", "open_bitmap", r);
> + if (!open_midx_bitmap(r, bitmap_git)) {
> + ret = 0;
Nit: I think these "goto" patterns are best if your "int ret = N" picks
an "N" which is the default that you'll "goto", i.e. if you pick "ret =
0" you'll just need "goto done" here....
> + goto done;
> + }
> + ret = open_pack_bitmap(r, bitmap_git);
...which we may then override here.
Just saves you the assignment and the {}, but it tends to add up in
longer functions.
> +done:
> + trace2_region_leave("pack-bitmap", "open_bitmap", r);
> + return ret;
> }
Looks good, aside from the 5/5 comments that much of the data string
logging looks like it becomes redundant in the end due to error() giving
us the same thing.
> struct bitmap_index *prepare_bitmap_git(struct repository *r)
> diff --git a/repo-settings.c b/repo-settings.c
> index b4fbd16cdc..5bc7a97a6d 100644
> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -8,6 +8,7 @@ static void repo_cfg_bool(struct repository *r, const char *key, int *dest,
> {
> if (repo_config_get_bool(r, key, dest))
> *dest = def;
> + trace2_data_string("config", r, key, *dest ? "true" : "false");
> }
>
> void prepare_repo_settings(struct repository *r)
> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> index eb63b71852..664cb88b0b 100755
> --- a/t/t5310-pack-bitmaps.sh
> +++ b/t/t5310-pack-bitmaps.sh
> @@ -421,8 +421,8 @@ test_expect_success 'complains about multiple pack bitmaps' '
> test_line_count = 2 bitmaps &&
>
> git rev-list --use-bitmap-index HEAD 2>err &&
> - grep "a bitmap has been opened" err &&
> - grep "ignoring extra bitmap file" err
> + grep "warning: a normal or midx bitmap already has been opened" err &&
> + grep "warning: ignoring extra bitmap file" err
> )
> '
I haven't tested but part of this test change looks like it'll break
under bisect, i.e. you changed 1/2 of these strings in 3/5. Did you try
with "git rebase -i -x 'make test T=t*bitmap*" or similar?
