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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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?