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

From: Brandon Williams <[email-masked]>
Date: Wed, 7 Dec 2016 15:27:24 -0800

On 12/07, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams <[email-masked]> 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 <[email-masked]>
> > ---
> >  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, &prefixlen, copyfrom);
> > +               match = prefix_path_gently(prefix, prefixlen,
> > +                                          &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