On Tue, May 11, 2021 at 12:06 PM Jeff Hostetler <git@xxxxxxxxxxxxxxxxx> wrote:
>
> On 5/11/21 2:34 PM, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@xxxxxxxxx>
> >
> > Signed-off-by: Elijah Newren <newren@xxxxxxxxx>
> > ---
> > 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?
> 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.
