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

[Jeff Hostetler <git@jeffhostetler.com>]

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

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

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

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