Re: [PATCH/RFC v4] grep: Add the option '--exclude'

From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Feb 6, 2012, 15:16 UTC
Message-ID: <CACsJy8DhtjG6AhPkb0SEm4g6zhtmuRb5x+4+P3A6eS0+_7OQNw@mail.gmail.com>
In-Reply-To: <1328192753-29162-1-git-send-email-surfingalbert@gmail.com>

On Thu, Feb 2, 2012 at 9:25 PM, Albert Yale <surfingalbert@gmail.com> wrote:

> I added a "struct pathspec_set" as you suggested
> in your previous review. It had the side effect
> of forcing me to update a few more files than was
> previously necessary.

Please make it a separate patch, it's hard to follow an all-in-one patch. Although those changes might be unnecessary (see below).

Show 13 quoted lines

> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -566,6 +566,10 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
>        while (tree_entry(tree, &entry)) {
>                int te_len = tree_entry_len(&entry);
>
> +               if (!match_pathspec_depth(pathspec,
> +                                         entry.path, strlen(entry.path),
> +                                         0, NULL))
> +                       continue;
>                if (match != all_entries_interesting) {
>                        match = tree_entry_interesting(&entry, base, tn_len, pathspec);
>                        if (match == all_entries_not_interesting)

tree_entry_interesting() is equivalent to match_pathspec_depth(). The only difference is that the former is designed to match on trees why the latter a list. And tree_entry_interesting() is more efficient than match_pathspec_depth(). As you can see there's tree_entry_interesting() call above already. You should make the tree_entry_interesting() understand exclude pathspec instead of adding match_pathspec_depth() in.

Show 26 quoted lines

> @@ -295,6 +298,25 @@ int match_pathspec_depth(const struct pathspec *ps,
>        return retval;
>  }
>
> +int match_pathspec_depth(const struct pathspec *ps,
> +                        const char *name, int namelen,
> +                        int prefix, char *seen)
> +{
> +       int retval = match_pathspec_set_depth(&ps->include,
> +                                             ps->recursive, ps->max_depth,
> +                                             name, namelen, prefix, seen);
> +
> +       if (retval && ps->exclude.nr)
> +       {
> +               if (match_pathspec_set_depth(&ps->exclude,
> +                                            ps->recursive, ps->max_depth,
> +                                            name, namelen, prefix, seen))
> +                       return 0;
> +       }
> +
> +       return retval;
> +}
> +
>  static int no_wildcard(const char *string)
>  {
>        return string[strcspn(string, "*?[{\\")] == '\0';

It makes me wonder, why not add match_pathspec_with_exclusion(const struct pathspec *include_ps, const struct pathspec *exclude_ps,...), use the new function in grep.c and revert struct pathspec back to original? The same can be applied to tree_entry_interesting() (i.e. add a new one that takes two pathspec sets, which supports exclusion)

I think you may make less changes that way. I'm bad at naming. It's up to you to rename match_pathspec_with_exclusion to something meaningful and short enough.

-- 
Duy