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.
