From: Albert Yale <surfingalbert@gmail.com>
Date: Mon, 30 Jan 2012 00:01:25 GMT
Subject: Re: [PATCH/RFC v2] grep: Add the option '--exclude'
Message-ID: <CALEc4zEQz1eVAgqGz2g5taEDnSMwLWePxHiT=543LWX-7zCDFg@mail.gmail.com>
In-Reply-To: <7vvcnudsda.fsf@alter.siamese.dyndns.org>

Thank you for your feedback Junio.
I'll revise my patch next weekend.

Albert Yale

On Sun, Jan 29, 2012 at 6:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Albert Yale <surfingalbert@gmail.com> writes:
>
> > Feedback would again be appreciated,
>
> Hmm.
>
> >          [-f <file>] [-e] <pattern>
> > +        [-x<pattern>|--exclude<pattern>]
>
> Compare the above two lines and notice some funny in the added one?
>
> > @@ -124,6 +125,12 @@ OPTIONS
> >       Use fixed strings for patterns (don't interpret pattern
> >       as a regex).
> >
> > +-x<pattern>::
> > +--exclude<pattern>::
> > +     In addition to those found in .gitignore (per directory) and
> > +     $GIT_DIR/info/exclude, also consider these patterns to be in the
> > +     set of the ignore rules in effect.
> > +
>
> Likewise.
>
> > diff --git a/builtin/grep.c b/builtin/grep.c
> > index 9ce064a..9772fa4 100644
> > --- a/builtin/grep.c
> > +++ b/builtin/grep.c
> > @@ -530,6 +530,10 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int
> >                       continue;
> >               if (!match_pathspec_depth(pathspec, ce->name, ce_namelen(ce), 0, NULL))
> >                       continue;
> > +             if (pathspec->exclude &&
> > +                     pathspec->exclude->nr &&
> > +                     match_pathspec_depth(pathspec->exclude, ce->name, ce_namelen(ce), 0, NULL))
> > +                     continue;
>
> Why isn't this just
>
>        if (!match_pathspec_depth(pathspec, ce->name, ce_namelen(ce), 0, NULL))
>
> that is, *no change whatsoever* on the existing codepaths that call
> match_pathspec_depth()?  Can't the "even if one of the positive pathspec
> matched, if one of the excluded one matches, declare that the path does
> *not* match" logic live in match_pathspec_depth() itself?
>
> Exactly the same comment applies to all the other additions that calls
> match_pathspec_depth() with pathspec->exclude as its first parameter in
> this patch.
>
> > @@ -1053,6 +1084,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
> >       pathspec.max_depth = opt.max_depth;
> >       pathspec.recursive = 1;
> >
> > +     if( exclude_list.nr ) {
> > +             create_pathspec_from_string_list(&pathspec.exclude, &exclude_list);
> > +             pathspec.exclude->max_depth = opt.max_depth;
> > +             pathspec.exclude->recursive = 1;
> > +     }
> > +
>
> Style. Notice where SPs should be near parentheses on "if" statement in
> our codebase.