Re: [PATCH 16/17] pathspec: small readability changes

[Duy Nguyen]

On Thu, Dec 8, 2016 at 6:27 AM, Brandon Williams  wrote:
>> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  wrote:
>> > @@ -362,8 +368,6 @@ static unsigned prefix_pathspec(struct pathspec_item 
>> > *item,
>> > } else {
>> > item->original = xstrdup(elt);
>> > }
>> > -   item->len = strlen(item->match);
>> > -   item->prefix = prefixlen;
>> >
>> > if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
>> > strip_submodule_slash_cheap(item);
>> > @@ -371,13 +375,14 @@ static unsigned prefix_pathspec(struct pathspec_item 
>> > *item,
>> > if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
>> > strip_submodule_slash_expensive(item);
>> >
>> > -   if (magic & PATHSPEC_LITERAL)
>> > +   if (magic & PATHSPEC_LITERAL) {
>> > item->nowildcard_len = item->len;
>> > -   else {
>> > +   } else {
>> > item->nowildcard_len = simple_length(item->match);
>> > if (item->nowildcard_len < prefixlen)
>> > item->nowildcard_len = prefixlen;
>> > }
>> > +
>> > item->flags = 0;
>>
>> You probably can move this line up with the others too.
>
> I didn't move the item->flags assignment up since the code immediately
> following this assignment deal with setting item->flags.  I made more
> sense to keep them grouped.

It's probably why I put it there in the beginning :) Yes let's leave
it where it is then.
-- 
Duy

Re: [PATCH 16/17] pathspec: small readability changes

[Brandon Williams]

On 12/07, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  wrote:
> > A few small changes to improve readability.  This is done by grouping 
> > related
> > assignments, adding blank lines, ensuring lines are <80 characters, etc.
> >
> > Signed-off-by: Brandon Williams 
> > ---
> >  pathspec.c | 15 ++-
> >  1 file changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/pathspec.c b/pathspec.c
> > index 41aa213..8a07b02 100644
> > --- a/pathspec.c
> > +++ b/pathspec.c
> > @@ -334,6 +334,7 @@ static unsigned prefix_pathspec(struct pathspec_item 
> > *item,
> > if ((magic & PATHSPEC_LITERAL) && (magic & PATHSPEC_GLOB))
> > die(_("%s: 'literal' and 'glob' are incompatible"), elt);
> >
> > +   /* Create match string which will be used for pathspec matching */
> > if (pathspec_prefix >= 0) {
> > match = xstrdup(copyfrom);
> > prefixlen = pathspec_prefix;
> > @@ -341,11 +342,16 @@ static unsigned prefix_pathspec(struct pathspec_item 
> > *item,
> > match = xstrdup(copyfrom);
> > prefixlen = 0;
> > } else {
> > -   match = prefix_path_gently(prefix, prefixlen, , 
> > copyfrom);
> > +   match = prefix_path_gently(prefix, prefixlen,
> > +  , copyfrom);
> > if (!match)
> > die(_("%s: '%s' is outside repository"), elt, 
> > copyfrom);
> > }
> > +
> > item->match = match;
> > +   item->len = strlen(item->match);
> > +   item->prefix = prefixlen;
> > +
> > /*
> >  * Prefix the pathspec (keep all magic) and assign to
> >  * original. Useful for passing to another command.
> > @@ -362,8 +368,6 @@ static unsigned prefix_pathspec(struct pathspec_item 
> > *item,
> > } else {
> > item->original = xstrdup(elt);
> > }
> > -   item->len = strlen(item->match);
> > -   item->prefix = prefixlen;
> >
> > if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
> > strip_submodule_slash_cheap(item);
> > @@ -371,13 +375,14 @@ static unsigned prefix_pathspec(struct pathspec_item 
> > *item,
> > if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
> > strip_submodule_slash_expensive(item);
> >
> > -   if (magic & PATHSPEC_LITERAL)
> > +   if (magic & PATHSPEC_LITERAL) {
> > item->nowildcard_len = item->len;
> > -   else {
> > +   } else {
> > item->nowildcard_len = simple_length(item->match);
> > if (item->nowildcard_len < prefixlen)
> > item->nowildcard_len = prefixlen;
> > }
> > +
> > item->flags = 0;
> 
> You probably can move this line up with the others too.

I didn't move the item->flags assignment up since the code immediately
following this assignment deal with setting item->flags.  I made more
sense to keep them grouped.

-- 
Brandon Williams

Re: [PATCH 16/17] pathspec: small readability changes

[Brandon Williams]

On 12/07, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  wrote:
> > A few small changes to improve readability.  This is done by grouping 
> > related
> > assignments, adding blank lines, ensuring lines are <80 characters, etc.
> >
> > Signed-off-by: Brandon Williams 
> > ---
> >  pathspec.c | 15 ++-
> >  1 file changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/pathspec.c b/pathspec.c
> > index 41aa213..8a07b02 100644
> > --- a/pathspec.c
> > +++ b/pathspec.c
> > @@ -334,6 +334,7 @@ static unsigned prefix_pathspec(struct pathspec_item 
> > *item,
> 
> btw, since this function has stopped being "just prefix pathspec" for
> a long time, perhaps rename it to parse_pathspec_item, or something.

I was thinking about doing that after I sent this out.  Glad you also
pointed that out.

-- 
Brandon Williams

Re: [PATCH 16/17] pathspec: small readability changes

[Junio C Hamano]

Duy Nguyen  writes:

> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  wrote:
>> A few small changes to improve readability.  This is done by grouping related
>> assignments, adding blank lines, ensuring lines are <80 characters, etc.
>>
>> Signed-off-by: Brandon Williams 
>> ---
>>  pathspec.c | 15 ++-
>>  1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/pathspec.c b/pathspec.c
>> index 41aa213..8a07b02 100644
>> --- a/pathspec.c
>> +++ b/pathspec.c
>> @@ -334,6 +334,7 @@ static unsigned prefix_pathspec(struct pathspec_item 
>> *item,
>
> btw, since this function has stopped being "just prefix pathspec" for
> a long time, perhaps rename it to parse_pathspec_item, or something.

Not specifically responding to this comment, but thanks for all the
constructive feedback messages.

Re: [PATCH 16/17] pathspec: small readability changes

[Duy Nguyen]

On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  wrote:
> A few small changes to improve readability.  This is done by grouping related
> assignments, adding blank lines, ensuring lines are <80 characters, etc.
>
> Signed-off-by: Brandon Williams 
> ---
>  pathspec.c | 15 ++-
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/pathspec.c b/pathspec.c
> index 41aa213..8a07b02 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -334,6 +334,7 @@ static unsigned prefix_pathspec(struct pathspec_item 
> *item,

btw, since this function has stopped being "just prefix pathspec" for
a long time, perhaps rename it to parse_pathspec_item, or something.
-- 
Duy

Re: [PATCH 16/17] pathspec: small readability changes

[Duy Nguyen]

On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  wrote:
> A few small changes to improve readability.  This is done by grouping related
> assignments, adding blank lines, ensuring lines are <80 characters, etc.
>
> Signed-off-by: Brandon Williams 
> ---
>  pathspec.c | 15 ++-
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/pathspec.c b/pathspec.c
> index 41aa213..8a07b02 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -334,6 +334,7 @@ static unsigned prefix_pathspec(struct pathspec_item 
> *item,
> if ((magic & PATHSPEC_LITERAL) && (magic & PATHSPEC_GLOB))
> die(_("%s: 'literal' and 'glob' are incompatible"), elt);
>
> +   /* Create match string which will be used for pathspec matching */
> if (pathspec_prefix >= 0) {
> match = xstrdup(copyfrom);
> prefixlen = pathspec_prefix;
> @@ -341,11 +342,16 @@ static unsigned prefix_pathspec(struct pathspec_item 
> *item,
> match = xstrdup(copyfrom);
> prefixlen = 0;
> } else {
> -   match = prefix_path_gently(prefix, prefixlen, , 
> copyfrom);
> +   match = prefix_path_gently(prefix, prefixlen,
> +  , copyfrom);
> if (!match)
> die(_("%s: '%s' is outside repository"), elt, 
> copyfrom);
> }
> +
> item->match = match;
> +   item->len = strlen(item->match);
> +   item->prefix = prefixlen;
> +
> /*
>  * Prefix the pathspec (keep all magic) and assign to
>  * original. Useful for passing to another command.
> @@ -362,8 +368,6 @@ static unsigned prefix_pathspec(struct pathspec_item 
> *item,
> } else {
> item->original = xstrdup(elt);
> }
> -   item->len = strlen(item->match);
> -   item->prefix = prefixlen;
>
> if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
> strip_submodule_slash_cheap(item);
> @@ -371,13 +375,14 @@ static unsigned prefix_pathspec(struct pathspec_item 
> *item,
> if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
> strip_submodule_slash_expensive(item);
>
> -   if (magic & PATHSPEC_LITERAL)
> +   if (magic & PATHSPEC_LITERAL) {
> item->nowildcard_len = item->len;
> -   else {
> +   } else {
> item->nowildcard_len = simple_length(item->match);
> if (item->nowildcard_len < prefixlen)
> item->nowildcard_len = prefixlen;
> }
> +
> item->flags = 0;

You probably can move this line up with the others too.

And since you have broken this function down so nicely, it made me see
that we could do

item->magic = magic instead of returning "magic" at the end, which is
assigned to item->magic anyway by the caller.
-- 
Duy

Re: [PATCH 16/17] pathspec: small readability changes

[Stefan Beller]

On Tue, Dec 6, 2016 at 1:51 PM, Brandon Williams  wrote:
> A few small changes to improve readability.  This is done by grouping related
> assignments, adding blank lines, ensuring lines are <80 characters, etc.

The 'etc' sounds a bit sloppy in the commit message.
Maybe s/etc/and adding proper comments/ ?

Code looks good.

[PATCH 16/17] pathspec: small readability changes

[Brandon Williams]

A few small changes to improve readability.  This is done by grouping related
assignments, adding blank lines, ensuring lines are <80 characters, etc.

Signed-off-by: Brandon Williams 
---
 pathspec.c | 15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index 41aa213..8a07b02 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -334,6 +334,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
if ((magic & PATHSPEC_LITERAL) && (magic & PATHSPEC_GLOB))
die(_("%s: 'literal' and 'glob' are incompatible"), elt);
 
+   /* Create match string which will be used for pathspec matching */
if (pathspec_prefix >= 0) {
match = xstrdup(copyfrom);
prefixlen = pathspec_prefix;
@@ -341,11 +342,16 @@ static unsigned prefix_pathspec(struct pathspec_item 
*item,
match = xstrdup(copyfrom);
prefixlen = 0;
} else {
-   match = prefix_path_gently(prefix, prefixlen, , 
copyfrom);
+   match = prefix_path_gently(prefix, prefixlen,
+  , copyfrom);
if (!match)
die(_("%s: '%s' is outside repository"), elt, copyfrom);
}
+
item->match = match;
+   item->len = strlen(item->match);
+   item->prefix = prefixlen;
+
/*
 * Prefix the pathspec (keep all magic) and assign to
 * original. Useful for passing to another command.
@@ -362,8 +368,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
} else {
item->original = xstrdup(elt);
}
-   item->len = strlen(item->match);
-   item->prefix = prefixlen;
 
if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
strip_submodule_slash_cheap(item);
@@ -371,13 +375,14 @@ static unsigned prefix_pathspec(struct pathspec_item 
*item,
if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
strip_submodule_slash_expensive(item);
 
-   if (magic & PATHSPEC_LITERAL)
+   if (magic & PATHSPEC_LITERAL) {
item->nowildcard_len = item->len;
-   else {
+   } else {
item->nowildcard_len = simple_length(item->match);
if (item->nowildcard_len < prefixlen)
item->nowildcard_len = prefixlen;
}
+
item->flags = 0;
if (magic & PATHSPEC_GLOB) {
/*
-- 
2.8.0.rc3.226.g39d4020