{"thread":{"id":"29491","subject":"[PATCH/RFC v3] grep: Add the option '--exclude'","startedAt":"2012-01-31T14:45:08Z","lastAt":"2012-02-01T00:31:42Z","messageCount":2,"participants":["Albert Yale","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"183415","messageId":"1328021108-4662-1-git-send-email-surfingalbert@gmail.com","threadId":"29491","inReplyTo":null,"subject":"[PATCH/RFC v3] grep: Add the option '--exclude'","fromName":"Albert Yale","fromEmail":"surfingalbert@gmail.com","sentAt":"2012-01-31T14:45:08Z","receivedAt":"2012-01-31T14:45:08Z","isPatch":true,"sender":{"key":"surfingalbert@gmail.com","avatar":null},"body":"[PATCH/RFC v3] grep: Add the option '--exclude'\n\nSigned-off-by: Albert Yale <surfingalbert@gmail.com>\n---\nHere's my latest progress towards a cleaner implementation.\n\nAlbert Yale\n\n Documentation/git-grep.txt |    7 +++\n builtin/grep.c             |   41 +++++++++++++--\n cache.h                    |    3 +\n dir.c                      |  126 +++++++++++++++++++++++++++++++++++++------\n dir.h                      |    3 +-\n read-cache.c               |    2 +-\n 6 files changed, 158 insertions(+), 24 deletions(-)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex 6a8b1e3..67a83aa 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -22,6 +22,7 @@ SYNOPSIS\n \t   [--color[=<when>] | --no-color]\n \t   [-A <post-context>] [-B <pre-context>] [-C <context>]\n \t   [-f <file>] [-e] <pattern>\n+\t   [-x <pattern>|--exclude <pattern>]\n \t   [--and|--or|--not|(|)|-e <pattern>...]\n \t   [ [--exclude-standard] [--cached | --no-index | --untracked] | <tree>...]\n \t   [--] [<pathspec>...]\n@@ -124,6 +125,12 @@ OPTIONS\n \tUse fixed strings for patterns (don't interpret pattern\n \tas a regex).\n \n+-x <pattern>::\n+--exclude <pattern>::\n+\tIn addition to those found in .gitignore (per directory) and\n+\t$GIT_DIR/info/exclude, also consider these patterns to be in the\n+\tset of the ignore rules in effect.\n+\n -n::\n --line-number::\n \tPrefix the line number to matching lines.\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 9ce064a..e9e1ac1 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -528,7 +528,8 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int\n \t\tstruct cache_entry *ce = active_cache[nr];\n \t\tif (!S_ISREG(ce->ce_mode))\n \t\t\tcontinue;\n-\t\tif (!match_pathspec_depth(pathspec, ce->name, ce_namelen(ce), 0, NULL))\n+\t\tif (!match_pathspec_depth(pathspec, ce->name, ce_namelen(ce),\n+\t\t\t\t\t  0, NULL, 1))\n \t\t\tcontinue;\n \t\t/*\n \t\t * If CE_VALID is on, we assume worktree file and its cache entry\n@@ -566,6 +567,11 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \twhile (tree_entry(tree, &entry)) {\n \t\tint te_len = tree_entry_len(&entry);\n \n+\t\tif (match_pathspec_depth(pathspec->exclude,\n+\t\t\t\t\t entry.path, strlen(entry.path),\n+\t\t\t\t\t 0, NULL, 0))\n+\t\t\tcontinue;\n+\n \t\tif (match != all_entries_interesting) {\n \t\t\tmatch = tree_entry_interesting(&entry, base, tn_len, pathspec);\n \t\t\tif (match == all_entries_not_interesting)\n@@ -606,8 +612,17 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\t       struct object *obj, const char *name)\n {\n-\tif (obj->type == OBJ_BLOB)\n+\tif (obj->type == OBJ_BLOB) {\n+\t\tconst char *name_without_sha1 = strchr(name, ':') + 1;\n+\n+\t\tif (match_pathspec_depth(pathspec->exclude,\n+\t\t\t\t\t name_without_sha1,\n+\t\t\t\t\t strlen(name_without_sha1),\n+\t\t\t\t\t 0, NULL, 0))\n+\t\t\treturn 0;\n+\n \t\treturn grep_sha1(opt, obj->sha1, name, 0);\n+\t}\n \tif (obj->type == OBJ_COMMIT || obj->type == OBJ_TREE) {\n \t\tstruct tree_desc tree;\n \t\tvoid *data;\n@@ -671,7 +686,7 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,\n \tfor (i = 0; i < dir.nr; i++) {\n \t\tconst char *name = dir.entries[i]->name;\n \t\tint namelen = strlen(name);\n-\t\tif (!match_pathspec_depth(pathspec, name, namelen, 0, NULL))\n+\t\tif (!match_pathspec_depth(pathspec, name, namelen, 0, NULL, 1))\n \t\t\tcontinue;\n \t\thit |= grep_file(opt, dir.entries[i]->name);\n \t\tif (hit && opt->status_only)\n@@ -764,6 +779,14 @@ static int pattern_callback(const struct option *opt, const char *arg,\n \treturn 0;\n }\n \n+static int exclude_cb(const struct option *opt, const char *arg,\n+\t\t      int unset)\n+{\n+\tstruct string_list *exclude_list = opt->value;\n+\tstring_list_append(exclude_list, arg);\n+\treturn 0;\n+}\n+\n static int help_callback(const struct option *opt, const char *arg, int unset)\n {\n \treturn -1;\n@@ -792,7 +815,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tpattern_type_pcre,\n \t};\n \tint pattern_type = pattern_type_unspecified;\n-\n+\tstruct string_list exclude_list = STRING_LIST_INIT_NODUP;\n \tstruct option options[] = {\n \t\tOPT_BOOLEAN(0, \"cached\", &cached,\n \t\t\t\"search in index instead of in the work tree\"),\n@@ -872,6 +895,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\t\"read patterns from file\", file_callback),\n \t\t{ OPTION_CALLBACK, 'e', NULL, &opt, \"pattern\",\n \t\t\t\"match <pattern>\", PARSE_OPT_NONEG, pattern_callback },\n+\t\t{ OPTION_CALLBACK, 'x', \"exclude\", &exclude_list, \"pattern\",\n+\t\t  \"add <pattern> to ignore rules\", PARSE_OPT_NONEG, exclude_cb },\n \t\t{ OPTION_CALLBACK, 0, \"and\", &opt, NULL,\n \t\t  \"combine patterns specified with -e\",\n \t\t  PARSE_OPT_NOARG | PARSE_OPT_NONEG, and_callback },\n@@ -1053,6 +1078,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tpathspec.max_depth = opt.max_depth;\n \tpathspec.recursive = 1;\n \n+\tif (exclude_list.nr) {\n+\t\tadd_pathspec_item_from_string_list(pathspec.exclude, &exclude_list);\n+\t\tpathspec.exclude->max_depth = opt.max_depth;\n+\t\tpathspec.exclude->recursive = 1;\n+\t}\n+\n \tif (show_in_pager && (cached || list.nr))\n \t\tdie(_(\"--open-files-in-pager only works on the worktree\"));\n \n@@ -1102,5 +1133,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tif (hit && show_in_pager)\n \t\trun_pager(&opt, prefix);\n \tfree_grep_patterns(&opt);\n+\tfree_pathspec(&pathspec);\n+\tstring_list_clear(&exclude_list, 0);\n \treturn !hit;\n }\ndiff --git a/cache.h b/cache.h\nindex 9bd8c2d..683458a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -533,9 +533,12 @@ struct pathspec {\n \t\tint len;\n \t\tunsigned int use_wildcard:1;\n \t} *items;\n+\n+\tstruct pathspec *exclude; /* This is never NULL. */\n };\n \n extern int init_pathspec(struct pathspec *, const char **);\n+extern int add_pathspec_item_from_string_list(struct pathspec *, const struct string_list *);\n extern void free_pathspec(struct pathspec *);\n extern int ce_path_match(const struct cache_entry *ce, const struct pathspec *pathspec);\n \ndiff --git a/dir.c b/dir.c\nindex 0a78d00..9e4312c 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -9,6 +9,8 @@\n #include \"dir.h\"\n #include \"refs.h\"\n \n+#include \"string-list.h\"\n+\n struct path_simplify {\n \tint len;\n \tconst char *path;\n@@ -253,34 +255,47 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,\n  */\n int match_pathspec_depth(const struct pathspec *ps,\n \t\t\t const char *name, int namelen,\n-\t\t\t int prefix, char *seen)\n+\t\t\t int prefix, char *seen,\n+\t\t\t int empty_pathspec_can_match)\n {\n \tint i, retval = 0;\n+\tconst char *name_without_prefix = name + prefix;\n+\tint namelen_without_prefix = namelen - prefix;\n \n \tif (!ps->nr) {\n-\t\tif (!ps->recursive || ps->max_depth == -1)\n-\t\t\treturn MATCHED_RECURSIVELY;\n+\t\tif (empty_pathspec_can_match) {\n+\t\t\tif (match_pathspec_depth(ps->exclude,\n+\t\t\t\t\t\t   name, namelen,\n+\t\t\t\t\t\t   prefix, seen, 0))\n+\t\t\t\treturn 0;\n \n-\t\tif (within_depth(name, namelen, 0, ps->max_depth))\n-\t\t\treturn MATCHED_EXACTLY;\n+\t\t\tif (!ps->recursive || ps->max_depth == -1)\n+\t\t\t\treturn MATCHED_RECURSIVELY;\n+\n+\t\t\tif (within_depth(name, namelen, 0, ps->max_depth))\n+\t\t\t\treturn MATCHED_EXACTLY;\n+\t\t\telse\n+\t\t\t\treturn 0;\n+\t\t}\n \t\telse\n \t\t\treturn 0;\n \t}\n \n-\tname += prefix;\n-\tnamelen -= prefix;\n-\n \tfor (i = ps->nr - 1; i >= 0; i--) {\n \t\tint how;\n \t\tif (seen && seen[i] == MATCHED_EXACTLY)\n \t\t\tcontinue;\n-\t\thow = match_pathspec_item(ps->items+i, prefix, name, namelen);\n+\t\thow = match_pathspec_item(ps->items+i, prefix,\n+\t\t\t\t\t  name_without_prefix,\n+\t\t\t\t\t  namelen_without_prefix);\n \t\tif (ps->recursive && ps->max_depth != -1 &&\n \t\t    how && how != MATCHED_FNMATCH) {\n \t\t\tint len = ps->items[i].len;\n-\t\t\tif (name[len] == '/')\n+\t\t\tif (name_without_prefix[len] == '/')\n \t\t\t\tlen++;\n-\t\t\tif (within_depth(name+len, namelen-len, 0, ps->max_depth))\n+\t\t\tif (within_depth(name_without_prefix+len,\n+\t\t\t\t\t namelen_without_prefix-len,\n+\t\t\t\t\t 0, ps->max_depth))\n \t\t\t\thow = MATCHED_EXACTLY;\n \t\t\telse\n \t\t\t\thow = 0;\n@@ -292,6 +307,12 @@ int match_pathspec_depth(const struct pathspec *ps,\n \t\t\t\tseen[i] = how;\n \t\t}\n \t}\n+\n+\tif (retval && match_pathspec_depth(ps->exclude,\n+\t\t\t\t\t   name, namelen,\n+\t\t\t\t\t   prefix, seen, 0))\n+\t\treturn 0;\n+\n \treturn retval;\n }\n \n@@ -1259,12 +1280,79 @@ static int pathspec_item_cmp(const void *a_, const void *b_)\n \treturn strcmp(a->match, b->match);\n }\n \n+static int init_pathspec_item(struct pathspec_item *item, const char *path)\n+{\n+\titem->match = path;\n+\titem->len = strlen(path);\n+\titem->use_wildcard = !no_wildcard(path);\n+\n+\treturn item->use_wildcard;\n+}\n+\n+int add_pathspec_item_from_string_list(struct pathspec *pathspec,\n+\t\t\t\t       const struct string_list *path_list)\n+{\n+\tint i;\n+\tstruct pathspec_item *new_items;\n+\tint previous_items_count = pathspec->nr;\n+\n+\tif (!path_list->nr)\n+\t\treturn 0;\n+\n+\tpathspec->nr += path_list->nr;\n+\n+\tif (previous_items_count)\n+\t\tpathspec->items = xrealloc(pathspec->items,\n+\t\t\t\t\t   sizeof(struct pathspec_item)*pathspec->nr);\n+\telse\n+\t\tpathspec->items = xcalloc(pathspec->nr, sizeof(struct pathspec_item));\n+\n+\tnew_items = pathspec->items + previous_items_count;\n+\n+\tfor (i = 0; i < path_list->nr; i++) {\n+\t\tstruct pathspec_item *item = new_items+i;\n+\t\tconst char *path = path_list->items[i].string;\n+\n+\t\tpathspec->has_wildcard |= init_pathspec_item(item, path);\n+\t}\n+\n+\t/*\n+\t* Give the pathspec a pathspec->exclude so we can test\n+\t* pathspec->exclude->nr without testing the validity of\n+\t* pathspec->exclude first.\n+\t*/\n+\tif (!pathspec->exclude)\n+\t\tpathspec->exclude = xcalloc(1, sizeof(*pathspec));\n+\n+\tqsort(pathspec->items, pathspec->nr,\n+\t      sizeof(struct pathspec_item), pathspec_item_cmp);\n+\n+\t/*\n+\t* Please comment/review:\n+\t*\n+\t* Here, pathspec->raw is NULL despite pathspec->nr being non-zero.\n+\t*\n+\t* Usually, a NULL pathspec->raw implies that pathspec->nr is zero.\n+\t*\n+\t* A problem might arrise if other areas of the code assume that a\n+\t* NULL pathspec->raw means that pathspec->nr is zero, or vice-versa.\n+\t*/\n+\n+\treturn 0;\n+}\n+\n int init_pathspec(struct pathspec *pathspec, const char **paths)\n {\n \tconst char **p = paths;\n \tint i;\n \n \tmemset(pathspec, 0, sizeof(*pathspec));\n+\t/*\n+\t* We create pathspec->exclude unconditionally so that we don't have\n+\t* to test the validity of pathspec->exclude before using the value\n+\t* of pathspec->exclude->nr.\n+\t*/\n+\tpathspec->exclude = xcalloc(1, sizeof(*pathspec));\n \tif (!p)\n \t\treturn 0;\n \twhile (*p)\n@@ -1279,11 +1367,7 @@ int init_pathspec(struct pathspec *pathspec, const char **paths)\n \t\tstruct pathspec_item *item = pathspec->items+i;\n \t\tconst char *path = paths[i];\n \n-\t\titem->match = path;\n-\t\titem->len = strlen(path);\n-\t\titem->use_wildcard = !no_wildcard(path);\n-\t\tif (item->use_wildcard)\n-\t\t\tpathspec->has_wildcard = 1;\n+\t\tpathspec->has_wildcard |= init_pathspec_item(item, path);\n \t}\n \n \tqsort(pathspec->items, pathspec->nr,\n@@ -1294,6 +1378,12 @@ int init_pathspec(struct pathspec *pathspec, const char **paths)\n \n void free_pathspec(struct pathspec *pathspec)\n {\n-\tfree(pathspec->items);\n-\tpathspec->items = NULL;\n+\tif (pathspec->items) {\n+\t\tfree(pathspec->items);\n+\t\tpathspec->items = NULL;\n+\t}\n+\tif (pathspec->exclude) {\n+\t\tfree_pathspec(pathspec->exclude);\n+\t\tpathspec->exclude = NULL;\n+\t}\n }\ndiff --git a/dir.h b/dir.h\nindex dd6947e..2b9870d 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -68,7 +68,8 @@ extern char *common_prefix(const char **pathspec);\n extern int match_pathspec(const char **pathspec, const char *name, int namelen, int prefix, char *seen);\n extern int match_pathspec_depth(const struct pathspec *pathspec,\n \t\t\t\tconst char *name, int namelen,\n-\t\t\t\tint prefix, char *seen);\n+\t\t\t\tint prefix, char *seen,\n+\t\t\t\tint empty_pathspec_can_match);\n extern int within_depth(const char *name, int namelen, int depth, int max_depth);\n \n extern int fill_directory(struct dir_struct *dir, const char **pathspec);\ndiff --git a/read-cache.c b/read-cache.c\nindex a51bba1..9783c4f 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -708,7 +708,7 @@ int ce_same_name(struct cache_entry *a, struct cache_entry *b)\n \n int ce_path_match(const struct cache_entry *ce, const struct pathspec *pathspec)\n {\n-\treturn match_pathspec_depth(pathspec, ce->name, ce_namelen(ce), 0, NULL);\n+\treturn match_pathspec_depth(pathspec, ce->name, ce_namelen(ce), 0, NULL, 1);\n }\n \n /*\n-- \n1.7.8.3\n"},{"id":"183446","messageId":"7vsjiv5r81.fsf@alter.siamese.dyndns.org","threadId":"29491","inReplyTo":"1328021108-4662-1-git-send-email-surfingalbert@gmail.com","subject":"Re: [PATCH/RFC v3] grep: Add the option '--exclude'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-01T00:31:42Z","receivedAt":"2012-02-01T00:31:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Albert Yale <surfingalbert@gmail.com> writes:\n\n> @@ -124,6 +125,12 @@ OPTIONS\n>  \tUse fixed strings for patterns (don't interpret pattern\n>  \tas a regex).\n>  \n> +-x <pattern>::\n> +--exclude <pattern>::\n> +\tIn addition to those found in .gitignore (per directory) and\n> +\t$GIT_DIR/info/exclude, also consider these patterns to be in the\n> +\tset of the ignore rules in effect.\n\nThat makes it sound as if \"git grep\" will not produce hits for a path that\nwas forcibly added despite it matches a pattern in .gitignore file, which\nis not true at all.\n\n>  -n::\n>  --line-number::\n>  \tPrefix the line number to matching lines.\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 9ce064a..e9e1ac1 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -528,7 +528,8 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int\n>  \t\tstruct cache_entry *ce = active_cache[nr];\n>  \t\tif (!S_ISREG(ce->ce_mode))\n>  \t\t\tcontinue;\n> -\t\tif (!match_pathspec_depth(pathspec, ce->name, ce_namelen(ce), 0, NULL))\n> +\t\tif (!match_pathspec_depth(pathspec, ce->name, ce_namelen(ce),\n> +\t\t\t\t\t  0, NULL, 1))\n>  \t\t\tcontinue;\n>  \t\t/*\n>  \t\t * If CE_VALID is on, we assume worktree file and its cache entry\n> @@ -566,6 +567,11 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n>  \twhile (tree_entry(tree, &entry)) {\n>  \t\tint te_len = tree_entry_len(&entry);\n>  \n> +\t\tif (match_pathspec_depth(pathspec->exclude,\n> +\t\t\t\t\t entry.path, strlen(entry.path),\n> +\t\t\t\t\t 0, NULL, 0))\n> +\t\t\tcontinue;\n> +\n\nWhy???\n\nIOW, why isn't this\n\n\tif (!match_pathspec_depth(pathspec, ...))\n\t\tcontinue;\t\n\nMy point of the two previous review messages was that it would be nice if\nwe can make it so that nobody outside match_pathspec_depth() should even\nneed to know existence of pathspec->exclude. Either that was ignored, or\nperhaps you found a compelling reason why that is not a good idea, but if\nso I'd like to know why.\n\nI suspect that you do not need to add the extra 0 at the end (it makes the\ncaller even harder to read) if you go that route. The extra parameter\ndefeats the whole point of encapsulating the new \"negative match\" feature\nbehind match_pathspec_depth() implementation.\n\n> +\tif (obj->type == OBJ_BLOB) {\n> +\t\tconst char *name_without_sha1 = strchr(name, ':') + 1;\n> +\n> +\t\tif (match_pathspec_depth(pathspec->exclude,\n> +\t\t\t\t\t name_without_sha1,\n> +\t\t\t\t\t strlen(name_without_sha1),\n> +\t\t\t\t\t 0, NULL, 0))\n> +\t\t\treturn 0;\n\nLikewise.\n\n> diff --git a/cache.h b/cache.h\n> index 9bd8c2d..683458a 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -533,9 +533,12 @@ struct pathspec {\n>  \t\tint len;\n>  \t\tunsigned int use_wildcard:1;\n>  \t} *items;\n> +\n> +\tstruct pathspec *exclude; /* This is never NULL. */\n>  };\n\n\"This is never NULL\" is blatantly wrong, as pathspec->exclude->exclude\nwill very likely be NULL.\n\nI've been assuming that the resulting structure would be more like this: \n\n struct pathspec_set {\n         int nr;\n         unsigned int has_wildcard:1;\n         unsigned int recursive:1;\n         struct pathspec_item {\n                 const char *match;\n                 int len;\n                 unsigned int use_wildcard:1;\n         } *items;\n };\n\n struct pathspec {\n         const char **raw; /* get_pathspec() result, not freed by free_pathspec() */\n         int max_depth;\n         struct pathspec_set include;\n         struct pathspec_set exclude;\n };\n \nI am not including \"raw\" in the include/exclude because its use should be\ndeprecated away over time.\n\nThe various helpers used in match_pathspec_depth() that know _only_ about\nmatching and not about excluding will be changed to take a pointer to\n\"struct pathspec_set\" (and possibly max_depth). The top-level callers\nwould not have to know any of these changes if we structure the API that\nway, no?\n\nI think something like that was more or less the structure we discussed\nwhen Nguyễn brought up his negative pathspec work the last time.\n"}]}