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

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

[Albert Yale]

Signed-off-by: Albert Yale <surfingalbert@gmail.com>
---
This is a revision to my previous patch.
The exclusion information is now held
inside the pathspec.

Feedback would again be appreciated,

Albert Yale

 Documentation/git-grep.txt |    7 +++++
 builtin/grep.c             |   43 ++++++++++++++++++++++++++++++++-
 cache.h                    |    3 ++
 dir.c                      |   55 ++++++++++++++++++++++++++++++++++++++++----
 4 files changed, 101 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 6a8b1e3..db143e3 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -22,6 +22,7 @@ SYNOPSIS
 	   [--color[=<when>] | --no-color]
 	   [-A <post-context>] [-B <pre-context>] [-C <context>]
 	   [-f <file>] [-e] <pattern>
+	   [-x<pattern>|--exclude<pattern>]
 	   [--and|--or|--not|(|)|-e <pattern>...]
 	   [ [--exclude-standard] [--cached | --no-index | --untracked] | <tree>...]
 	   [--] [<pathspec>...]
@@ -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.
+
 -n::
 --line-number::
 	Prefix the line number to matching lines.
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;
 		/*
 		 * If CE_VALID is on, we assume worktree file and its cache entry
 		 * are identical, even if worktree file has been modified, so use
@@ -566,6 +570,11 @@ 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 (pathspec->exclude &&
+			pathspec->exclude->nr &&
+			match_pathspec_depth(pathspec->exclude, 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)
@@ -606,8 +615,16 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
 static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
 		       struct object *obj, const char *name)
 {
-	if (obj->type == OBJ_BLOB)
+	if (obj->type == OBJ_BLOB) {
+		const char *name_without_sha1 = strchr(name, ':') + 1;
+
+		if (pathspec->exclude &&
+			pathspec->exclude->nr &&
+			match_pathspec_depth(pathspec->exclude, name_without_sha1, strlen(name_without_sha1), 0, NULL))
+			return 0;
+
 		return grep_sha1(opt, obj->sha1, name, 0);
+	}
 	if (obj->type == OBJ_COMMIT || obj->type == OBJ_TREE) {
 		struct tree_desc tree;
 		void *data;
@@ -673,6 +690,10 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,
 		int namelen = strlen(name);
 		if (!match_pathspec_depth(pathspec, name, namelen, 0, NULL))
 			continue;
+		if (pathspec->exclude &&
+			pathspec->exclude->nr &&
+			match_pathspec_depth(pathspec->exclude, name, namelen, 0, NULL))
+			continue;
 		hit |= grep_file(opt, dir.entries[i]->name);
 		if (hit && opt->status_only)
 			break;
@@ -764,6 +785,14 @@ static int pattern_callback(const struct option *opt, const char *arg,
 	return 0;
 }
 
+static int exclude_cb(const struct option *opt, const char *arg,
+			    int unset)
+{
+	struct string_list *exclude_list = opt->value;
+	string_list_append(exclude_list, arg);
+	return 0;
+}
+
 static int help_callback(const struct option *opt, const char *arg, int unset)
 {
 	return -1;
@@ -792,7 +821,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		pattern_type_pcre,
 	};
 	int pattern_type = pattern_type_unspecified;
-
+	struct string_list exclude_list = STRING_LIST_INIT_NODUP;
 	struct option options[] = {
 		OPT_BOOLEAN(0, "cached", &cached,
 			"search in index instead of in the work tree"),
@@ -872,6 +901,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			"read patterns from file", file_callback),
 		{ OPTION_CALLBACK, 'e', NULL, &opt, "pattern",
 			"match <pattern>", PARSE_OPT_NONEG, pattern_callback },
+		{ OPTION_CALLBACK, 'x', "exclude", &exclude_list, "pattern",
+		  "add <pattern> to ignore rules", PARSE_OPT_NONEG, exclude_cb },
 		{ OPTION_CALLBACK, 0, "and", &opt, NULL,
 		  "combine patterns specified with -e",
 		  PARSE_OPT_NOARG | PARSE_OPT_NONEG, and_callback },
@@ -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;
+	}
+
 	if (show_in_pager && (cached || list.nr))
 		die(_("--open-files-in-pager only works on the worktree"));
 
@@ -1102,5 +1139,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	if (hit && show_in_pager)
 		run_pager(&opt, prefix);
 	free_grep_patterns(&opt);
+	free_pathspec(&pathspec);
+	string_list_clear(&exclude_list, 0);
 	return !hit;
 }
diff --git a/cache.h b/cache.h
index 10afd71..882a390 100644
--- a/cache.h
+++ b/cache.h
@@ -533,9 +533,12 @@ struct pathspec {
 		int len;
 		unsigned int use_wildcard:1;
 	} *items;
+
+	struct pathspec *exclude;
 };
 
 extern int init_pathspec(struct pathspec *, const char **);
+extern int create_pathspec_from_string_list(struct pathspec **, const struct string_list *);
 extern void free_pathspec(struct pathspec *);
 extern int ce_path_match(const struct cache_entry *ce, const struct pathspec *pathspec);
 
diff --git a/dir.c b/dir.c
index 0a78d00..fd04afa 100644
--- a/dir.c
+++ b/dir.c
@@ -9,6 +9,8 @@
 #include "dir.h"
 #include "refs.h"
 
+#include "string-list.h"
+
 struct path_simplify {
 	int len;
 	const char *path;
@@ -1259,6 +1261,49 @@ static int pathspec_item_cmp(const void *a_, const void *b_)
 	return strcmp(a->match, b->match);
 }
 
+static int init_pathspec_item( struct pathspec_item *item, const char *path )
+{
+	item->match = path;
+	item->len = strlen(path);
+	item->use_wildcard = !no_wildcard(path);
+
+	return item->use_wildcard;
+}
+
+int create_pathspec_from_string_list(struct pathspec **pathspec, const struct string_list *path_list)
+{
+	int i;
+
+	*pathspec = xcalloc( 1, sizeof( struct pathspec ) );
+
+	if (!path_list->nr)
+		return 0;
+
+	(*pathspec)->nr = path_list->nr;
+	(*pathspec)->items = xcalloc(path_list->nr, sizeof(struct pathspec_item));
+
+	for (i = 0; i < path_list->nr; i++) {
+		struct pathspec_item *item = (*pathspec)->items+i;
+		const char *path = path_list->items[i].string;
+
+		(*pathspec)->has_wildcard |= init_pathspec_item( item, path );
+	}
+
+	qsort((*pathspec)->items, (*pathspec)->nr,
+	      sizeof(struct pathspec_item), pathspec_item_cmp);
+
+	/*
+	* Please comment/review:
+	*
+	* Here, pathspec->raw is NULL despite pathspec->nr being non-zero.
+	* A possible problem might arrise if other areas of the code make
+	* the assumption that a NULL pathspec->raw means that pathspec->nr
+	* is zero.
+	*/
+
+	return 0;
+}
+
 int init_pathspec(struct pathspec *pathspec, const char **paths)
 {
 	const char **p = paths;
@@ -1279,11 +1324,7 @@ int init_pathspec(struct pathspec *pathspec, const char **paths)
 		struct pathspec_item *item = pathspec->items+i;
 		const char *path = paths[i];
 
-		item->match = path;
-		item->len = strlen(path);
-		item->use_wildcard = !no_wildcard(path);
-		if (item->use_wildcard)
-			pathspec->has_wildcard = 1;
+		pathspec->has_wildcard |= init_pathspec_item( item, path );
 	}
 
 	qsort(pathspec->items, pathspec->nr,
@@ -1296,4 +1337,8 @@ void free_pathspec(struct pathspec *pathspec)
 {
 	free(pathspec->items);
 	pathspec->items = NULL;
+	if (pathspec->exclude) {
+		free_pathspec(pathspec->exclude);
+		pathspec->exclude = NULL;
+	}
 }
-- 
1.7.8.3

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

[Junio C Hamano]

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.

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

[Albert Yale]

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.