On 02/08, Duy Nguyen wrote:
> On Wed, Feb 8, 2017 at 5:04 AM, René Scharfe <l.s.r@xxxxxx> wrote:
> > Pass the match member of the first pathspec item directly to
> > read_directory() instead of using common_prefix() to duplicate it first,
> > thus avoiding memory duplication, strlen(3) and free(3).
>
> How about killing common_prefix()? There are two other callers in
> ls-files.c and commit.c and it looks safe to do (but I didn't look
> very hard).
>
> > diff --git a/dir.c b/dir.c
> > index 65c3e681b8..4541f9e146 100644
> > --- a/dir.c
> > +++ b/dir.c
> > @@ -174,20 +174,19 @@ char *common_prefix(const struct pathspec *pathspec)
> >
> > int fill_directory(struct dir_struct *dir, const struct pathspec *pathspec)
> > {
> > - char *prefix;
> > + const char *prefix;
> > size_t prefix_len;
> >
> > /*
> > * Calculate common prefix for the pathspec, and
> > * use that to optimize the directory walk
> > */
> > - prefix = common_prefix(pathspec);
> > - prefix_len = prefix ? strlen(prefix) : 0;
> > + prefix_len = common_prefix_len(pathspec);
> > + prefix = prefix_len ? pathspec->items[0].match : "";
>
> There's a subtle difference. Before the patch, prefix[prefix_len] is
> NUL. After the patch, it's not always true. If some code (incorrectly)
> depends on that, this patch exposes it. I had a look inside
> read_directory() though and it looks like no such code exists. So, all
> good.
Yeah I had the exact same thought when looking at this, but I agree
everything looks fine. And if something does indeed depend on prefix
having a \0 at prefix_len then this will allow us to more easily find
the bug and fix it.
>
> >
> > /* Read the directory and prune it */
> > read_directory(dir, prefix, prefix_len, pathspec);
> >
> > - free(prefix);
> > return prefix_len;
> > }
> --
> Duy
--
Brandon Williams