On Tue, May 11, 2021 at 4:12 PM Jeff Hostetler <git@xxxxxxxxxxxxxxxxx> wrote:
>
> On 5/11/21 4:12 PM, Elijah Newren wrote:
> > 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?
>
> 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?
