Re: [PATCH/RFC v3] grep: Add the option '--exclude'
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:52:54
Albert Yale [off-list ref] writes:
quoted hunk
@@ -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.
That makes it sound as if "git grep" will not produce hits for a path that was forcibly added despite it matches a pattern in .gitignore file, which is not true at all.
quoted hunk
-n:: --line-number:: Prefix the line number to matching lines.diff --git a/builtin/grep.c b/builtin/grep.c index 9ce064a..e9e1ac1 100644 --- a/builtin/grep.c +++ b/builtin/grep.c@@ -528,7 +528,8 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int struct cache_entry *ce = active_cache[nr]; if (!S_ISREG(ce->ce_mode)) continue; - if (!match_pathspec_depth(pathspec, ce->name, ce_namelen(ce), 0, NULL)) + if (!match_pathspec_depth(pathspec, ce->name, ce_namelen(ce), + 0, NULL, 1)) continue; /* * If CE_VALID is on, we assume worktree file and its cache entry@@ -566,6 +567,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 (match_pathspec_depth(pathspec->exclude, + entry.path, strlen(entry.path), + 0, NULL, 0)) + continue; +
Why??? IOW, why isn't this if (!match_pathspec_depth(pathspec, ...)) continue; My point of the two previous review messages was that it would be nice if we can make it so that nobody outside match_pathspec_depth() should even need to know existence of pathspec->exclude. Either that was ignored, or perhaps you found a compelling reason why that is not a good idea, but if so I'd like to know why. I suspect that you do not need to add the extra 0 at the end (it makes the caller even harder to read) if you go that route. The extra parameter defeats the whole point of encapsulating the new "negative match" feature behind match_pathspec_depth() implementation.
+ if (obj->type == OBJ_BLOB) {
+ const char *name_without_sha1 = strchr(name, ':') + 1;
+
+ if (match_pathspec_depth(pathspec->exclude,
+ name_without_sha1,
+ strlen(name_without_sha1),
+ 0, NULL, 0))
+ return 0;Likewise.
quoted hunk
diff --git a/cache.h b/cache.h index 9bd8c2d..683458a 100644 --- a/cache.h +++ b/cache.h@@ -533,9 +533,12 @@ struct pathspec { int len; unsigned int use_wildcard:1; } *items; + + struct pathspec *exclude; /* This is never NULL. */ };
"This is never NULL" is blatantly wrong, as pathspec->exclude->exclude
will very likely be NULL.
I've been assuming that the resulting structure would be more like this:
struct pathspec_set {
int nr;
unsigned int has_wildcard:1;
unsigned int recursive:1;
struct pathspec_item {
const char *match;
int len;
unsigned int use_wildcard:1;
} *items;
};
struct pathspec {
const char **raw; /* get_pathspec() result, not freed by free_pathspec() */
int max_depth;
struct pathspec_set include;
struct pathspec_set exclude;
};
I am not including "raw" in the include/exclude because its use should be
deprecated away over time.
The various helpers used in match_pathspec_depth() that know _only_ about
matching and not about excluding will be changed to take a pointer to
"struct pathspec_set" (and possibly max_depth). The top-level callers
would not have to know any of these changes if we structure the API that
way, no?
I think something like that was more or less the structure we discussed
when Nguyễn brought up his negative pathspec work the last time.