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

[Jeff Hostetler <git@jeffhostetler.com>]

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

but i'm nit-picking again.

Jeff