Re: [PATCH v4 1/8] dir: convert trace calls to trace2 equivalents

> On Tue, May 11, 2021 at 4:12 PM Jeff Hostetler <git@jeffhostetler.com> wrote:
>>
>> On 5/11/21 4:12 PM, Elijah Newren wrote:
>>> On Tue, May 11, 2021 at 12:06 PM Jeff Hostetler <git@jeffhostetler.com> wrote:
>>>>
>>>> On 5/11/21 2:34 PM, Elijah Newren via GitGitGadget wrote:
>>>>> From: Elijah Newren <newren@gmail.com>
>>>>>
>>>>> Signed-off-by: Elijah Newren <newren@gmail.com>
>>>>> ---
>>>>>     dir.c                             |  43 +++++--
>>>>>     t/t7063-status-untracked-cache.sh | 205 ++++++++++++++++++------------
>>>>>     t/t7519-status-fsmonitor.sh       |   8 +-
>>>>>     3 files changed, 155 insertions(+), 101 deletions(-)
>>>>>
>>>>> diff --git a/dir.c b/dir.c
>>>>> index 3474e67e8f3c..122fcbffdf89 100644
>>>>> --- a/dir.c
>>>>> +++ b/dir.c
>>>>> @@ -2760,15 +2760,34 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d
>>>>>         return root;
>>>>>     }
>>>>>
>>>>> +static void trace2_read_directory_statistics(struct dir_struct *dir,
>>>>> +                                          struct repository *repo,
>>>>> +                                          const char *path)
>>>>> +{
>>>>> +     if (!dir->untracked)
>>>>> +             return;
>>>>> +     trace2_data_string("read_directory", repo, "path", path);
>>>>
>>>> I'm probably just nit-picking here, but should this look more like:
>>>
>>> nit-picking and questions are totally fine.  :-)  Thanks for reviewing.
>>>
>>>>
>>>>           if (path && *path)
>>>>                   trace2_data_string(...)
>>>
>>> path is always non-NULL (it'd be an error to call read_directory()
>>> with a NULL path).  So the first part of the check isn't meaningful
>>> for this particular code.  The second half is interesting.  Do we want
>>> to omit the path when it happens to be the toplevel directory (the
>>> case where !*path)?  The original trace_performance_leave() calls
>>> certainly didn't, and I was just trying to provide the same info they
>>> do, as you suggested.  I guess people could determine the path by
>>> knowing that the code doesn't print it when it's empty, but do we want
>>> trace2 users to need to read the code to figure out statistics and
>>> info?
>>
>> that's fine.  it might be easier to just always print it (even if
>> blank) so that post-processors know that rather than have to assume
>> it.
>>
>>>
>>>>           if (!dir->untracked)
>>>>                   return;
>>>>
>>>> Then when you add the visitied fields in the next commit,
>>>> you'll have the path with them (when present).
>>>
>>> There is always a path with them, it's just that the empty string
>>> denotes the toplevel directory.
>>>
>>>> (and it would let you optionally avoid the tmp strbuf in
>>>> the caller.)
>>>
>>> The path in read_directory() is not necessarily NUL-delimited, so
>>> attempting to use it as-is, or even with your checks, would cause us
>>> to possibly print garbage and do out-of-bounds reads.  We need the tmp
>>> strbuf.
>>>
>>
>> I just meant, "if (!len) pass NULL, else build and pass tmp.buf".
> 
> Ah, gotcha, that's why you were checking non-NULL.
> 
> However, what about the other case when len is nonzero.  Let's say
> that len = 8 and path points at
> "filename*%&#)aWholeBunchOfTotalGarbageAfterTheRealFilenameThatShouldNotBeReadOrIncluded\0\0\0\0\0\0\0\0\0\0"
> ?
> 
> How do you make it print "filename" and only "filename" without the
> other stuff without using the tmp strbuf?
>