Re: [PATCH 02/17] dir: convert create_simplify to use the pathspec struct interface

[Brandon Williams]

On 12/09, Brandon Williams wrote:
> On 12/09, Duy Nguyen wrote:
> > On Fri, Dec 9, 2016 at 1:19 AM, Brandon Williams  wrote:
> > > On 12/08, Duy Nguyen wrote:
> > >> On Thu, Dec 8, 2016 at 7:03 AM, Brandon Williams  
> > >> wrote:
> > >> > On 12/07, Duy Nguyen wrote:
> > >> >> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  
> > >> >> wrote:
> > >> >> > Convert 'create_simplify()' to use the pathspec struct interface 
> > >> >> > from
> > >> >> > using the '_raw' entry in the pathspec.
> > >> >>
> > >> >> It would be even better to kill this create_simplify() and let
> > >> >> simplify_away() handle struct pathspec directly.
> > >> >>
> > >> >> There is a bug in this code, that might have been found if we
> > >> >> simpify_away() handled pathspec directly: the memcmp() in
> > >> >> simplify_away() will not play well with :(icase) magic. My bad. If
> > >> >> :(icase) is used, the easiest/safe way is simplify nothing. Later on
> > >> >> maybe we can teach simplify_away() to do strncasecmp instead. We could
> > >> >> ignore exclude patterns there too (although not excluding is not a
> > >> >> bug).
> > >> >
> > >> > So are you implying that the simplify struct needs to be killed?  That
> > >> > way the pathspec struct itself is being passed around instead?
> > >>
> > >> Yes. simplify struct was a thing when pathspec was an array of char *.
> > >> At this point I think it can retire (when we have time to retire it)
> > >
> > > Alright, then for now I can leave this change as is and have a follow up
> > > series that kills the simplify struct.
> > 
> > Do let me know if you decide to drop it, so I can put it back in my backlog.
> 
> K will do
> 

This actually turned out to be more straight forward than I thought.
I'll reroll this series again (with a few other changes) and include
killing the simplify struct.

-- 
Brandon Williams

Re: [PATCH 02/17] dir: convert create_simplify to use the pathspec struct interface

[Brandon Williams]

On 12/09, Duy Nguyen wrote:
> On Fri, Dec 9, 2016 at 1:19 AM, Brandon Williams  wrote:
> > On 12/08, Duy Nguyen wrote:
> >> On Thu, Dec 8, 2016 at 7:03 AM, Brandon Williams  wrote:
> >> > On 12/07, Duy Nguyen wrote:
> >> >> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  
> >> >> wrote:
> >> >> > Convert 'create_simplify()' to use the pathspec struct interface from
> >> >> > using the '_raw' entry in the pathspec.
> >> >>
> >> >> It would be even better to kill this create_simplify() and let
> >> >> simplify_away() handle struct pathspec directly.
> >> >>
> >> >> There is a bug in this code, that might have been found if we
> >> >> simpify_away() handled pathspec directly: the memcmp() in
> >> >> simplify_away() will not play well with :(icase) magic. My bad. If
> >> >> :(icase) is used, the easiest/safe way is simplify nothing. Later on
> >> >> maybe we can teach simplify_away() to do strncasecmp instead. We could
> >> >> ignore exclude patterns there too (although not excluding is not a
> >> >> bug).
> >> >
> >> > So are you implying that the simplify struct needs to be killed?  That
> >> > way the pathspec struct itself is being passed around instead?
> >>
> >> Yes. simplify struct was a thing when pathspec was an array of char *.
> >> At this point I think it can retire (when we have time to retire it)
> >
> > Alright, then for now I can leave this change as is and have a follow up
> > series that kills the simplify struct.
> 
> Do let me know if you decide to drop it, so I can put it back in my backlog.

K will do

-- 
Brandon Williams

Re: [PATCH 02/17] dir: convert create_simplify to use the pathspec struct interface

[Duy Nguyen]

On Fri, Dec 9, 2016 at 1:19 AM, Brandon Williams  wrote:
> On 12/08, Duy Nguyen wrote:
>> On Thu, Dec 8, 2016 at 7:03 AM, Brandon Williams  wrote:
>> > On 12/07, Duy Nguyen wrote:
>> >> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  
>> >> wrote:
>> >> > Convert 'create_simplify()' to use the pathspec struct interface from
>> >> > using the '_raw' entry in the pathspec.
>> >>
>> >> It would be even better to kill this create_simplify() and let
>> >> simplify_away() handle struct pathspec directly.
>> >>
>> >> There is a bug in this code, that might have been found if we
>> >> simpify_away() handled pathspec directly: the memcmp() in
>> >> simplify_away() will not play well with :(icase) magic. My bad. If
>> >> :(icase) is used, the easiest/safe way is simplify nothing. Later on
>> >> maybe we can teach simplify_away() to do strncasecmp instead. We could
>> >> ignore exclude patterns there too (although not excluding is not a
>> >> bug).
>> >
>> > So are you implying that the simplify struct needs to be killed?  That
>> > way the pathspec struct itself is being passed around instead?
>>
>> Yes. simplify struct was a thing when pathspec was an array of char *.
>> At this point I think it can retire (when we have time to retire it)
>
> Alright, then for now I can leave this change as is and have a follow up
> series that kills the simplify struct.

Do let me know if you decide to drop it, so I can put it back in my backlog.
-- 
Duy

Re: [PATCH 02/17] dir: convert create_simplify to use the pathspec struct interface

[Brandon Williams]

On 12/08, Duy Nguyen wrote:
> On Thu, Dec 8, 2016 at 7:03 AM, Brandon Williams  wrote:
> > On 12/07, Duy Nguyen wrote:
> >> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  wrote:
> >> > Convert 'create_simplify()' to use the pathspec struct interface from
> >> > using the '_raw' entry in the pathspec.
> >>
> >> It would be even better to kill this create_simplify() and let
> >> simplify_away() handle struct pathspec directly.
> >>
> >> There is a bug in this code, that might have been found if we
> >> simpify_away() handled pathspec directly: the memcmp() in
> >> simplify_away() will not play well with :(icase) magic. My bad. If
> >> :(icase) is used, the easiest/safe way is simplify nothing. Later on
> >> maybe we can teach simplify_away() to do strncasecmp instead. We could
> >> ignore exclude patterns there too (although not excluding is not a
> >> bug).
> >
> > So are you implying that the simplify struct needs to be killed?  That
> > way the pathspec struct itself is being passed around instead?
> 
> Yes. simplify struct was a thing when pathspec was an array of char *.
> At this point I think it can retire (when we have time to retire it)

Alright, then for now I can leave this change as is and have a follow up
series that kills the simplify struct.

-- 
Brandon Williams

Re: [PATCH 02/17] dir: convert create_simplify to use the pathspec struct interface

[Duy Nguyen]

On Thu, Dec 8, 2016 at 7:03 AM, Brandon Williams  wrote:
> On 12/07, Duy Nguyen wrote:
>> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  wrote:
>> > Convert 'create_simplify()' to use the pathspec struct interface from
>> > using the '_raw' entry in the pathspec.
>>
>> It would be even better to kill this create_simplify() and let
>> simplify_away() handle struct pathspec directly.
>>
>> There is a bug in this code, that might have been found if we
>> simpify_away() handled pathspec directly: the memcmp() in
>> simplify_away() will not play well with :(icase) magic. My bad. If
>> :(icase) is used, the easiest/safe way is simplify nothing. Later on
>> maybe we can teach simplify_away() to do strncasecmp instead. We could
>> ignore exclude patterns there too (although not excluding is not a
>> bug).
>
> So are you implying that the simplify struct needs to be killed?  That
> way the pathspec struct itself is being passed around instead?

Yes. simplify struct was a thing when pathspec was an array of char *.
At this point I think it can retire (when we have time to retire it)
-- 
Duy

Re: [PATCH 02/17] dir: convert create_simplify 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 'create_simplify()' to use the pathspec struct interface from
> > using the '_raw' entry in the pathspec.
> 
> It would be even better to kill this create_simplify() and let
> simplify_away() handle struct pathspec directly.
> 
> There is a bug in this code, that might have been found if we
> simpify_away() handled pathspec directly: the memcmp() in
> simplify_away() will not play well with :(icase) magic. My bad. If
> :(icase) is used, the easiest/safe way is simplify nothing. Later on
> maybe we can teach simplify_away() to do strncasecmp instead. We could
> ignore exclude patterns there too (although not excluding is not a
> bug).

So are you implying that the simplify struct needs to be killed?  That
way the pathspec struct itself is being passed around instead?

-- 
Brandon Williams

Re: [PATCH 02/17] dir: convert create_simplify to use the pathspec struct interface

[Duy Nguyen]

On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  wrote:
> Convert 'create_simplify()' to use the pathspec struct interface from
> using the '_raw' entry in the pathspec.

It would be even better to kill this create_simplify() and let
simplify_away() handle struct pathspec directly.

There is a bug in this code, that might have been found if we
simpify_away() handled pathspec directly: the memcmp() in
simplify_away() will not play well with :(icase) magic. My bad. If
:(icase) is used, the easiest/safe way is simplify nothing. Later on
maybe we can teach simplify_away() to do strncasecmp instead. We could
ignore exclude patterns there too (although not excluding is not a
bug).
-- 
Duy

[PATCH 02/17] dir: convert create_simplify to use the pathspec struct interface

[Brandon Williams]

Convert 'create_simplify()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec.

Signed-off-by: Brandon Williams 
---
 dir.c | 25 -
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/dir.c b/dir.c
index bfa8c8a..7df292b 100644
--- a/dir.c
+++ b/dir.c
@@ -1787,25 +1787,24 @@ static int cmp_name(const void *p1, const void *p2)
return name_compare(e1->name, e1->len, e2->name, e2->len);
 }
 
-static struct path_simplify *create_simplify(const char **pathspec)
+static struct path_simplify *create_simplify(const struct pathspec *pathspec)
 {
-   int nr, alloc = 0;
+   int i;
struct path_simplify *simplify = NULL;
 
-   if (!pathspec)
+   if (!pathspec || !pathspec->nr)
return NULL;
 
-   for (nr = 0 ; ; nr++) {
+   ALLOC_ARRAY(simplify, pathspec->nr + 1);
+   for (i = 0; i < pathspec->nr; i++) {
const char *match;
-   ALLOC_GROW(simplify, nr + 1, alloc);
-   match = *pathspec++;
-   if (!match)
-   break;
-   simplify[nr].path = match;
-   simplify[nr].len = simple_length(match);
+   match = pathspec->items[i].match;
+   simplify[i].path = match;
+   simplify[i].len = pathspec->items[i].nowildcard_len;
}
-   simplify[nr].path = NULL;
-   simplify[nr].len = 0;
+   simplify[i].path = NULL;
+   simplify[i].len = 0;
+
return simplify;
 }
 
@@ -2036,7 +2035,7 @@ int read_directory(struct dir_struct *dir, const char 
*path, int len, const stru
 * subset of positive ones, which has no impacts on
 * create_simplify().
 */
-   simplify = create_simplify(pathspec ? pathspec->_raw : NULL);
+   simplify = create_simplify(pathspec);
untracked = validate_untracked_cache(dir, len, pathspec);
if (!untracked)
/*
-- 
2.8.0.rc3.226.g39d4020