> 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. > 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.
