Re: [PATCH] dir: avoid allocation in fill_directory()

[Brandon Williams <bmwill@google.com>]

On 02/08, Duy Nguyen wrote:
> On Wed, Feb 8, 2017 at 5:04 AM, René Scharfe <l.s.r@web.de> 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