{"thread":{"id":"29520","subject":"[PATCH/RFC v4] grep: Add the option '--exclude'","startedAt":"2012-02-02T14:25:53Z","lastAt":"2012-02-14T17:35:23Z","messageCount":5,"participants":["Albert Yale","Nguyen Thai Ngoc Duy","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"183603","messageId":"1328192753-29162-1-git-send-email-surfingalbert@gmail.com","threadId":"29520","inReplyTo":null,"subject":"[PATCH/RFC v4] grep: Add the option '--exclude'","fromName":"Albert Yale","fromEmail":"surfingalbert@gmail.com","sentAt":"2012-02-02T14:25:53Z","receivedAt":"2012-02-02T14:25:53Z","isPatch":true,"sender":{"key":"surfingalbert@gmail.com","avatar":null},"body":"grep: Add the option '--exclude'\n\nSigned-off-by: Albert Yale <surfingalbert@gmail.com>\n---\nhi Junio,\n\nYou're correct that adding an extra parameter to\nmatch_pathspec_depth() was unnecessary.\n\nI added a \"struct pathspec_set\" as you suggested\nin your previous review. It had the side effect\nof forcing me to update a few more files than was\npreviously necessary.\n\nI understand that you want to keep the internals\nof a pathspec hidden from its users as much as\npossible. But if someone is already accessing its\ninternals, I don't think it's a problem to change\ntheir usage, for example, from \"ps->nr\" to\n\"ps->include.nr\". Please correct me if I'm wrong.\n\nThere's one thing I did differently. You suggested\nmoving \"recursive\" into \"struct pathspec_set\". I\nkept it in \"struct pathspec\" because it's usually\nmodified alongside \"max_depth\".\n\nbtw, the last two parameters of\nmatch_pathspec_depth(), namely \"prefix\" and\n\"seen\", are not used anywhere. I understand their\npotential usefulness, but I wanted to point this\nout in case you want to further simplify calls to\nthis function.\n\nLooking forward to your feedback,\n\nAlbert Yale\n\n Documentation/git-grep.txt |    5 ++\n builtin/diff.c             |    6 +-\n builtin/fast-export.c      |    2 +-\n builtin/grep.c             |   31 +++++++++++++-\n builtin/log.c              |    2 +-\n builtin/ls-files.c         |    2 +-\n builtin/ls-tree.c          |    6 +-\n cache.h                    |   15 +++++--\n dir.c                      |  100 +++++++++++++++++++++++++++++++++++---------\n list-objects.c             |    2 +-\n revision.c                 |    4 +-\n tree-diff.c                |    2 +-\n tree-walk.c                |    8 ++--\n 13 files changed, 143 insertions(+), 42 deletions(-)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex 6a8b1e3..b45706a 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,10 @@ OPTIONS\n \tUse fixed strings for patterns (don't interpret pattern\n \tas a regex).\n \n+-x <pattern>::\n+--exclude <pattern>::\n+\tSkip files matching pattern.\n+\n -n::\n --line-number::\n \tPrefix the line number to matching lines.\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 387afa7..e910f80 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -365,10 +365,10 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tdie(_(\"unhandled object '%s' given.\"), name);\n \t}\n-\tif (rev.prune_data.nr) {\n+\tif (rev.prune_data.include.nr) {\n \t\tif (!path)\n-\t\t\tpath = rev.prune_data.items[0].match;\n-\t\tpaths += rev.prune_data.nr;\n+\t\t\tpath = rev.prune_data.include.items[0].match;\n+\t\tpaths += rev.prune_data.include.nr;\n \t}\n \n \t/*\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 08fed98..23043a2 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -674,7 +674,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n \tif (import_filename)\n \t\timport_marks(import_filename);\n \n-\tif (import_filename && revs.prune_data.nr)\n+\tif (import_filename && revs.prune_data.include.nr)\n \t\tfull_tree = 1;\n \n \tget_tags_and_duplicates(&revs.pending, &extra_refs);\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 5c2ae94..19d1afd 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -566,6 +566,10 @@ 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,\n+\t\t\t\t\t  entry.path, strlen(entry.path),\n+\t\t\t\t\t  0, NULL))\n+\t\t\tcontinue;\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 +610,16 @@ 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+\t\tif (!match_pathspec_depth(pathspec,\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))\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@@ -764,6 +776,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,6 +812,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+\tstruct string_list exclude_list = STRING_LIST_INIT_NODUP;\n \n \tstruct option options[] = {\n \t\tOPT_BOOLEAN(0, \"cached\", &cached,\n@@ -872,6 +893,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@@ -1054,6 +1077,10 @@ 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\tinit_pathspec_exclude_from_string_list(&pathspec, &exclude_list);\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@@ -1103,5 +1130,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/builtin/log.c b/builtin/log.c\nindex 7d1f6f8..c837616 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -125,7 +125,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \t\trev->always_show_header = 0;\n \tif (DIFF_OPT_TST(&rev->diffopt, FOLLOW_RENAMES)) {\n \t\trev->always_show_header = 0;\n-\t\tif (rev->diffopt.pathspec.nr != 1)\n+\t\tif (rev->diffopt.pathspec.include.nr != 1)\n \t\t\tusage(\"git logs can only follow renames on one pathname at a time\");\n \t}\n \ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 7cff175..8cae770 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -326,7 +326,7 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)\n \t\tmatchbuf[0] = prefix;\n \t\tmatchbuf[1] = NULL;\n \t\tinit_pathspec(&pathspec, matchbuf);\n-\t\tpathspec.items[0].use_wildcard = 0;\n+\t\tpathspec.include.items[0].use_wildcard = 0;\n \t} else\n \t\tinit_pathspec(&pathspec, NULL);\n \tif (read_tree(tree, 1, &pathspec))\ndiff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\nindex 6b666e1..651169c 100644\n--- a/builtin/ls-tree.c\n+++ b/builtin/ls-tree.c\n@@ -167,9 +167,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n \t\tdie(\"Not a valid object name %s\", argv[0]);\n \n \tinit_pathspec(&pathspec, get_pathspec(prefix, argv + 1));\n-\tfor (i = 0; i < pathspec.nr; i++)\n-\t\tpathspec.items[i].use_wildcard = 0;\n-\tpathspec.has_wildcard = 0;\n+\tfor (i = 0; i < pathspec.include.nr; i++)\n+\t\tpathspec.include.items[i].use_wildcard = 0;\n+\tpathspec.include.has_wildcard = 0;\n \ttree = parse_tree_indirect(sha1);\n \tif (!tree)\n \t\tdie(\"not a tree object\");\ndiff --git a/cache.h b/cache.h\nindex 9bd8c2d..ea70b92 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -522,12 +522,9 @@ extern int index_name_is_other(const struct index_state *, const char *, int);\n extern int ie_match_stat(const struct index_state *, struct cache_entry *, struct stat *, unsigned int);\n extern int ie_modified(const struct index_state *, struct cache_entry *, struct stat *, unsigned int);\n \n-struct pathspec {\n-\tconst char **raw; /* get_pathspec() result, not freed by free_pathspec() */\n+struct pathspec_set {\n \tint nr;\n \tunsigned int has_wildcard:1;\n-\tunsigned int recursive:1;\n-\tint max_depth;\n \tstruct pathspec_item {\n \t\tconst char *match;\n \t\tint len;\n@@ -535,7 +532,17 @@ struct pathspec {\n \t} *items;\n };\n \n+struct pathspec {\n+\tconst char **raw; /* get_pathspec() result, not freed by free_pathspec() */\n+\tunsigned int recursive:1;\n+\tint max_depth;\n+\n+\tstruct pathspec_set include;\n+\tstruct pathspec_set exclude;\n+};\n+\n extern int init_pathspec(struct pathspec *, const char **);\n+extern int init_pathspec_exclude_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..faf2db3 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@@ -251,17 +253,18 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,\n  * and a mark is left in seen[] array for pathspec element that\n  * actually matched anything.\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+static int match_pathspec_set_depth(const struct pathspec_set *ps,\n+\t\t\t\t    unsigned int recursive, int max_depth,\n+\t\t\t\t    const char *name, int namelen,\n+\t\t\t\t    int prefix, char *seen)\n {\n \tint i, retval = 0;\n \n \tif (!ps->nr) {\n-\t\tif (!ps->recursive || ps->max_depth == -1)\n+\t\tif (!recursive || max_depth == -1)\n \t\t\treturn MATCHED_RECURSIVELY;\n \n-\t\tif (within_depth(name, namelen, 0, ps->max_depth))\n+\t\tif (within_depth(name, namelen, 0, max_depth))\n \t\t\treturn MATCHED_EXACTLY;\n \t\telse\n \t\t\treturn 0;\n@@ -275,12 +278,12 @@ int match_pathspec_depth(const struct pathspec *ps,\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\tif (ps->recursive && ps->max_depth != -1 &&\n+\t\tif (recursive && 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\t\tlen++;\n-\t\t\tif (within_depth(name+len, namelen-len, 0, ps->max_depth))\n+\t\t\tif (within_depth(name+len, namelen-len, 0, max_depth))\n \t\t\t\thow = MATCHED_EXACTLY;\n \t\t\telse\n \t\t\t\thow = 0;\n@@ -295,6 +298,25 @@ int match_pathspec_depth(const struct pathspec *ps,\n \treturn retval;\n }\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+{\n+\tint retval = match_pathspec_set_depth(&ps->include,\n+\t\t\t\t\t      ps->recursive, ps->max_depth,\n+\t\t\t\t\t      name, namelen, prefix, seen);\n+\n+\tif (retval && ps->exclude.nr)\n+\t{\n+\t\tif (match_pathspec_set_depth(&ps->exclude,\n+\t\t\t\t\t     ps->recursive, ps->max_depth,\n+\t\t\t\t\t     name, namelen, prefix, seen))\n+\t\t\treturn 0;\n+\t}\n+\n+\treturn retval;\n+}\n+\n static int no_wildcard(const char *string)\n {\n \treturn string[strcspn(string, \"*?[{\\\\\")] == '\\0';\n@@ -1259,9 +1281,44 @@ 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 init_pathspec_exclude_from_string_list(struct pathspec *pathspec,\n+\t\t\t\t\t   const struct string_list *path_list)\n+{\n+\tstruct pathspec_set *exclude = &pathspec->exclude;\n+\tint i;\n+\n+\tif (!path_list->nr)\n+\t\treturn 0;\n+\n+\texclude->nr = path_list->nr;\n+\texclude->items = xcalloc(path_list->nr, sizeof(struct pathspec_item));\n+\n+\tfor (i = 0; i < path_list->nr; i++) {\n+\t\tstruct pathspec_item *item = exclude->items+i;\n+\t\tconst char *path = path_list->items[i].string;\n+\n+\t\texclude->has_wildcard |= init_pathspec_item(item, path);\n+\t}\n+\n+\tqsort(exclude->items, exclude->nr,\n+\t      sizeof(struct pathspec_item), pathspec_item_cmp);\n+\n+\treturn 0;\n+}\n+\n int init_pathspec(struct pathspec *pathspec, const char **paths)\n {\n \tconst char **p = paths;\n+\tstruct pathspec_set *include = &pathspec->include;\n \tint i;\n \n \tmemset(pathspec, 0, sizeof(*pathspec));\n@@ -1270,23 +1327,20 @@ int init_pathspec(struct pathspec *pathspec, const char **paths)\n \twhile (*p)\n \t\tp++;\n \tpathspec->raw = paths;\n-\tpathspec->nr = p - paths;\n-\tif (!pathspec->nr)\n+\n+\tinclude->nr = p - paths;\n+\tif (!include->nr)\n \t\treturn 0;\n \n-\tpathspec->items = xmalloc(sizeof(struct pathspec_item)*pathspec->nr);\n-\tfor (i = 0; i < pathspec->nr; i++) {\n-\t\tstruct pathspec_item *item = pathspec->items+i;\n+\tinclude->items = xmalloc(sizeof(struct pathspec_item)*include->nr);\n+\tfor (i = 0; i < include->nr; i++) {\n+\t\tstruct pathspec_item *item = include->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\tinclude->has_wildcard |= init_pathspec_item(item, path);\n \t}\n \n-\tqsort(pathspec->items, pathspec->nr,\n+\tqsort(include->items, include->nr,\n \t      sizeof(struct pathspec_item), pathspec_item_cmp);\n \n \treturn 0;\n@@ -1294,6 +1348,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->include.items) {\n+\t\tfree(pathspec->include.items);\n+\t\tpathspec->include.items = NULL;\n+\t}\n+\tif (pathspec->exclude.items) {\n+\t\tfree(pathspec->exclude.items);\n+\t\tpathspec->exclude.items = NULL;\n+\t}\n }\ndiff --git a/list-objects.c b/list-objects.c\nindex 3dd4a96..cdac52d 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -71,7 +71,7 @@ static void process_tree(struct rev_info *revs,\n \tstruct tree_desc desc;\n \tstruct name_entry entry;\n \tstruct name_path me;\n-\tenum interesting match = revs->diffopt.pathspec.nr == 0 ?\n+\tenum interesting match = revs->diffopt.pathspec.include.nr == 0 ?\n \t\tall_entries_interesting: entry_not_interesting;\n \tint baselen = base->len;\n \ndiff --git a/revision.c b/revision.c\nindex c97d834..a516df4 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -389,7 +389,7 @@ static int rev_compare_tree(struct rev_info *revs, struct commit *parent, struct\n \t\t * tagged commit by specifying both --simplify-by-decoration\n \t\t * and pathspec.\n \t\t */\n-\t\tif (!revs->prune_data.nr)\n+\t\tif (!revs->prune_data.include.nr)\n \t\t\treturn REV_TREE_SAME;\n \t}\n \n@@ -1835,7 +1835,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \tif (revs->topo_order)\n \t\trevs->limited = 1;\n \n-\tif (revs->prune_data.nr) {\n+\tif (revs->prune_data.include.nr) {\n \t\tdiff_tree_setup_paths(revs->prune_data.raw, &revs->pruning);\n \t\t/* Can't prune commits with rename following: the paths change.. */\n \t\tif (!DIFF_OPT_TST(&revs->diffopt, FOLLOW_RENAMES))\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 28ad6db..43aa00f 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -146,7 +146,7 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2,\n \tfor (;;) {\n \t\tif (diff_can_quit_early(opt))\n \t\t\tbreak;\n-\t\tif (opt->pathspec.nr) {\n+\t\tif (opt->pathspec.include.nr) {\n \t\t\tskip_uninteresting(t1, &base, opt, &t1_match);\n \t\t\tskip_uninteresting(t2, &base, opt, &t2_match);\n \t\t}\ndiff --git a/tree-walk.c b/tree-walk.c\nindex 492c7cd..e95cd6a 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -584,10 +584,10 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,\n {\n \tint i;\n \tint pathlen, baselen = base->len - base_offset;\n-\tint never_interesting = ps->has_wildcard ?\n+\tint never_interesting = ps->include.has_wildcard ?\n \t\tentry_not_interesting : all_entries_not_interesting;\n \n-\tif (!ps->nr) {\n+\tif (!ps->include.nr) {\n \t\tif (!ps->recursive || ps->max_depth == -1)\n \t\t\treturn all_entries_interesting;\n \t\treturn within_depth(base->buf + base_offset, baselen,\n@@ -598,8 +598,8 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,\n \n \tpathlen = tree_entry_len(entry);\n \n-\tfor (i = ps->nr - 1; i >= 0; i--) {\n-\t\tconst struct pathspec_item *item = ps->items+i;\n+\tfor (i = ps->include.nr - 1; i >= 0; i--) {\n+\t\tconst struct pathspec_item *item = ps->include.items+i;\n \t\tconst char *match = item->match;\n \t\tconst char *base_str = base->buf + base_offset;\n \t\tint matchlen = item->len;\n-- \n1.7.8.3\n"},{"id":"183604","messageId":"CACsJy8CC+cwV+DEWPZVJqEtMq1Rd_RZtUTpMCeSmem5B_7vDMA@mail.gmail.com","threadId":"29520","inReplyTo":"1328192753-29162-1-git-send-email-surfingalbert@gmail.com","subject":"Re: [PATCH/RFC v4] grep: Add the option '--exclude'","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-02T14:35:25Z","receivedAt":"2012-02-02T14:35:25Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Feb 2, 2012 at 9:25 PM, Albert Yale <surfingalbert@gmail.com> wrote:\n> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\n> index 6a8b1e3..b45706a 100644\n> --- a/Documentation/git-grep.txt\n> +++ b/Documentation/git-grep.txt\n> @@ -22,6 +22,7 @@ SYNOPSIS\n>           [--color[=<when>] | --no-color]\n>           [-A <post-context>] [-B <pre-context>] [-C <context>]\n>           [-f <file>] [-e] <pattern>\n> +          [-x <pattern>|--exclude <pattern>]\n>           [--and|--or|--not|(|)|-e <pattern>...]\n>           [ [--exclude-standard] [--cached | --no-index | --untracked] | <tree>...]\n>           [--] [<pathspec>...]\n\nI'd like to have a careful look at this but haven't found time/energy\nyet. So just a minor comment. Please use the term <pathspec> instead\nof <pattern>. Exclude pattern has complete different syntax than\npathspec and --exclude followed by \"pattern\" may give wrong\nimpression.\n\nI'd also avoid reuse \"-x\" if it's common for exclude patterns, but\nfinding a new letter may be difficult. Keep in mind this feature in\nthe long run benefits more commands than just git-grep and ideally\nthey should all use the same letter for short option name.\n-- \nDuy\n"},{"id":"183606","messageId":"CALEc4zF_Z1icpjFRU+AQ+UvaQ9VFK9AJ0ne8F=vRax_gOLkE+g@mail.gmail.com","threadId":"29520","inReplyTo":"CACsJy8CC+cwV+DEWPZVJqEtMq1Rd_RZtUTpMCeSmem5B_7vDMA@mail.gmail.com","subject":"Re: [PATCH/RFC v4] grep: Add the option '--exclude'","fromName":"Albert Yale","fromEmail":"surfingalbert@gmail.com","sentAt":"2012-02-02T14:41:19Z","receivedAt":"2012-02-02T14:41:19Z","isPatch":true,"sender":{"key":"surfingalbert@gmail.com","avatar":null},"body":"Thank you for this feedback Nguyen. I'll make the change to\ngit-grep.txt in my next revision.\n\nAlbert\n\nOn Thu, Feb 2, 2012 at 9:35 AM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:\n> On Thu, Feb 2, 2012 at 9:25 PM, Albert Yale <surfingalbert@gmail.com> wrote:\n>> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\n>> index 6a8b1e3..b45706a 100644\n>> --- a/Documentation/git-grep.txt\n>> +++ b/Documentation/git-grep.txt\n>> @@ -22,6 +22,7 @@ SYNOPSIS\n>>           [--color[=<when>] | --no-color]\n>>           [-A <post-context>] [-B <pre-context>] [-C <context>]\n>>           [-f <file>] [-e] <pattern>\n>> +          [-x <pattern>|--exclude <pattern>]\n>>           [--and|--or|--not|(|)|-e <pattern>...]\n>>           [ [--exclude-standard] [--cached | --no-index | --untracked] | <tree>...]\n>>           [--] [<pathspec>...]\n>\n> I'd like to have a careful look at this but haven't found time/energy\n> yet. So just a minor comment. Please use the term <pathspec> instead\n> of <pattern>. Exclude pattern has complete different syntax than\n> pathspec and --exclude followed by \"pattern\" may give wrong\n> impression.\n>\n> I'd also avoid reuse \"-x\" if it's common for exclude patterns, but\n> finding a new letter may be difficult. Keep in mind this feature in\n> the long run benefits more commands than just git-grep and ideally\n> they should all use the same letter for short option name.\n> --\n> Duy\n"},{"id":"184027","messageId":"CACsJy8DhtjG6AhPkb0SEm4g6zhtmuRb5x+4+P3A6eS0+_7OQNw@mail.gmail.com","threadId":"29520","inReplyTo":"1328192753-29162-1-git-send-email-surfingalbert@gmail.com","subject":"Re: [PATCH/RFC v4] grep: Add the option '--exclude'","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-06T15:16:59Z","receivedAt":"2012-02-06T15:16:59Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Feb 2, 2012 at 9:25 PM, Albert Yale <surfingalbert@gmail.com> wrote:\n> I added a \"struct pathspec_set\" as you suggested\n> in your previous review. It had the side effect\n> of forcing me to update a few more files than was\n> previously necessary.\n\nPlease make it a separate patch, it's hard to follow an all-in-one\npatch. Although those changes might be unnecessary (see below).\n\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -566,6 +566,10 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n>        while (tree_entry(tree, &entry)) {\n>                int te_len = tree_entry_len(&entry);\n>\n> +               if (!match_pathspec_depth(pathspec,\n> +                                         entry.path, strlen(entry.path),\n> +                                         0, NULL))\n> +                       continue;\n>                if (match != all_entries_interesting) {\n>                        match = tree_entry_interesting(&entry, base, tn_len, pathspec);\n>                        if (match == all_entries_not_interesting)\n\ntree_entry_interesting() is equivalent to match_pathspec_depth(). The\nonly difference is that the former is designed to match on trees why\nthe latter a list. And tree_entry_interesting() is more efficient than\nmatch_pathspec_depth(). As you can see there's\ntree_entry_interesting() call above already. You should make the\ntree_entry_interesting() understand exclude pathspec instead of adding\nmatch_pathspec_depth() in.\n\n> @@ -295,6 +298,25 @@ int match_pathspec_depth(const struct pathspec *ps,\n>        return retval;\n>  }\n>\n> +int match_pathspec_depth(const struct pathspec *ps,\n> +                        const char *name, int namelen,\n> +                        int prefix, char *seen)\n> +{\n> +       int retval = match_pathspec_set_depth(&ps->include,\n> +                                             ps->recursive, ps->max_depth,\n> +                                             name, namelen, prefix, seen);\n> +\n> +       if (retval && ps->exclude.nr)\n> +       {\n> +               if (match_pathspec_set_depth(&ps->exclude,\n> +                                            ps->recursive, ps->max_depth,\n> +                                            name, namelen, prefix, seen))\n> +                       return 0;\n> +       }\n> +\n> +       return retval;\n> +}\n> +\n>  static int no_wildcard(const char *string)\n>  {\n>        return string[strcspn(string, \"*?[{\\\\\")] == '\\0';\n\nIt makes me wonder, why not add match_pathspec_with_exclusion(const\nstruct pathspec *include_ps, const struct pathspec *exclude_ps,...),\nuse the new function in grep.c and revert struct pathspec back to\noriginal? The same can be applied to tree_entry_interesting() (i.e.\nadd a new one that takes two pathspec sets, which supports exclusion)\n\nI think you may make less changes that way. I'm bad at naming. It's up\nto you to rename match_pathspec_with_exclusion to something meaningful\nand short enough.\n-- \nDuy\n"},{"id":"184663","messageId":"7vfwed8gis.fsf@alter.siamese.dyndns.org","threadId":"29520","inReplyTo":"CACsJy8DhtjG6AhPkb0SEm4g6zhtmuRb5x+4+P3A6eS0+_7OQNw@mail.gmail.com","subject":"Re: [PATCH/RFC v4] grep: Add the option '--exclude'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-14T17:35:23Z","receivedAt":"2012-02-14T17:35:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n\n> It makes me wonder, why not add match_pathspec_with_exclusion(const\n> struct pathspec *include_ps, const struct pathspec *exclude_ps,...),\n> use the new function in grep.c and revert struct pathspec back to\n> original? The same can be applied to tree_entry_interesting() (i.e.\n> add a new one that takes two pathspec sets, which supports exclusion)\n\nSorry, but I am the one to blame for this in:\n\n http://thread.gmane.org/gmane.comp.version-control.git/189455/focus=189486\n\n> I think you may make less changes that way.\n\nThe arrangement to use two pathspecs certainly is simpler in the short\nterm, but I think it is a short-sighted hack that is wrong for two\nreasons.\n\n - Doing it that way will not give you a solution where you only have to\n   update the command line parsing of an existing command to stuff\n   negative patterns to the pathspec structure you have been passing down\n   to the existing codepath, and the command starts to understand negative\n   patterns without any other change.  You introduce a new pathspec that\n   holds negative patterns, and pass that along with the positive one you\n   have been passing down in the existing codepath for all commands that\n   need to understand pathspec (otherwise we will be back to a similar\n   situation we were in before you started looking at pathspec where we\n   had three different implementations giving different semantics---some\n   command knows how to handle this kind of pathspec but some others do\n   not).\n\n - Look at the fields in 'struct pathspec' and think about what those\n   outside \"items\" list mean. The recursive and max_depth fields are about\n   how the namespace is traversed [*1*], so if a codepath that used to\n   know only about positive patterns learns to also care about negative\n   ones, using different settings for these two fields in two separate\n   'struct pathspec' does not make sense.  It is conceptually much cleaner\n   to make 'struct pathspec' the data structure used by the updated logic\n   to match paths to pathspec that can have positive and negative patterns.\n\n\n[Footnote]\n\n*1* The has_wildcard field is about optimizing the matching during the\ntraversal so it is a bit different.  Also it is our longer term goal to\nget rid of the \"raw\" field (which does not work well to substring match\nagainst the path when pathspec \"magic\" is involved), so we won't worry\nabout it in this discussion.\n"}]}