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@xxxxxxxxx> wrote:
>
> Albert Yale <surfingalbert@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html