Re: [PATCH 03/17] dir: convert fill_directory to use the pathspec struct interface

[Brandon Williams]

On 12/07, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  wrote:
> > Convert 'fill_directory()' to use the pathspec struct interface from
> > using the '_raw' entry in the pathspec struct.
> >
> > Signed-off-by: Brandon Williams 
> > ---
> >  dir.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/dir.c b/dir.c
> > index 7df292b..8730a4f 100644
> > --- a/dir.c
> > +++ b/dir.c
> > @@ -188,7 +188,8 @@ int fill_directory(struct dir_struct *dir, const struct 
> > pathspec *pathspec)
> > len = common_prefix_len(pathspec);
> >
> > /* Read the directory and prune it */
> > -   read_directory(dir, pathspec->nr ? pathspec->_raw[0] : "", len, 
> > pathspec);
> > +   read_directory(dir, pathspec->nr ? pathspec->items[0].match : "",
> > +  len, pathspec);
> 
> Or even better, use common_prefix()'s return value here. I took me a
> while to realize this code was not buggy. It is fine to just pick the
> first item because the first  characters of _all_ pathspec items
> must be the same. Something like this
> 
> prefix = common_prefix(..)
> read_directory(..., prefix, strlen(prefix), pathspec);
> 
> expresses it much better. Yeah one extra mem allocation, no big deal
> since fill_directory() is not called very often.

I didn't even notice that.  Now looking at this you're right that its
not immediately obvious that what's there is correct.  I'll change this.

> 
> > return len;
> >  }
> >
> > --
> > 2.8.0.rc3.226.g39d4020
> >
> -- 
> Duy

-- 
Brandon Williams

Re: [PATCH 03/17] dir: convert fill_directory to use the pathspec struct interface

[Duy Nguyen]

On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  wrote:
> Convert 'fill_directory()' to use the pathspec struct interface from
> using the '_raw' entry in the pathspec struct.
>
> Signed-off-by: Brandon Williams 
> ---
>  dir.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/dir.c b/dir.c
> index 7df292b..8730a4f 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -188,7 +188,8 @@ int fill_directory(struct dir_struct *dir, const struct 
> pathspec *pathspec)
> len = common_prefix_len(pathspec);
>
> /* Read the directory and prune it */
> -   read_directory(dir, pathspec->nr ? pathspec->_raw[0] : "", len, 
> pathspec);
> +   read_directory(dir, pathspec->nr ? pathspec->items[0].match : "",
> +  len, pathspec);

Or even better, use common_prefix()'s return value here. I took me a
while to realize this code was not buggy. It is fine to just pick the
first item because the first  characters of _all_ pathspec items
must be the same. Something like this

prefix = common_prefix(..)
read_directory(..., prefix, strlen(prefix), pathspec);

expresses it much better. Yeah one extra mem allocation, no big deal
since fill_directory() is not called very often.

> return len;
>  }
>
> --
> 2.8.0.rc3.226.g39d4020
>
-- 
Duy