{"thread":{"id":"29467","subject":"[PATCH/RFC v2] grep: Add the option '--exclude'","startedAt":"2012-01-29T22:42:14Z","lastAt":"2012-01-30T00:01:25Z","messageCount":3,"participants":["Albert Yale","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"183262","messageId":"1327876934-61526-1-git-send-email-surfingalbert@gmail.com","threadId":"29467","inReplyTo":null,"subject":"[PATCH/RFC v2] grep: Add the option '--exclude'","fromName":"Albert Yale","fromEmail":"surfingalbert@gmail.com","sentAt":"2012-01-29T22:42:14Z","receivedAt":"2012-01-29T22:42:14Z","isPatch":true,"sender":{"key":"surfingalbert@gmail.com","avatar":null},"body":"Signed-off-by: Albert Yale <surfingalbert@gmail.com>\n---\nThis is a revision to my previous patch.\nThe exclusion information is now held\ninside the pathspec.\n\nFeedback would again be appreciated,\n\nAlbert Yale\n\n Documentation/git-grep.txt |    7 +++++\n builtin/grep.c             |   43 ++++++++++++++++++++++++++++++++-\n cache.h                    |    3 ++\n dir.c                      |   55 ++++++++++++++++++++++++++++++++++++++++----\n 4 files changed, 101 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex 6a8b1e3..db143e3 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..9772fa4 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -530,6 +530,10 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int\n \t\t\tcontinue;\n \t\tif (!match_pathspec_depth(pathspec, ce->name, ce_namelen(ce), 0, NULL))\n \t\t\tcontinue;\n+\t\tif (pathspec->exclude &&\n+\t\t\tpathspec->exclude->nr &&\n+\t\t\tmatch_pathspec_depth(pathspec->exclude, ce->name, ce_namelen(ce), 0, NULL))\n+\t\t\tcontinue;\n \t\t/*\n \t\t * If CE_VALID is on, we assume worktree file and its cache entry\n \t\t * are identical, even if worktree file has been modified, so use\n@@ -566,6 +570,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 (pathspec->exclude &&\n+\t\t\tpathspec->exclude->nr &&\n+\t\t\tmatch_pathspec_depth(pathspec->exclude, entry.path, strlen(entry.path), 0, NULL))\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 +615,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+\n+\t\tif (pathspec->exclude &&\n+\t\t\tpathspec->exclude->nr &&\n+\t\t\tmatch_pathspec_depth(pathspec->exclude, name_without_sha1, strlen(name_without_sha1), 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@@ -673,6 +690,10 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\tint namelen = strlen(name);\n \t\tif (!match_pathspec_depth(pathspec, name, namelen, 0, NULL))\n \t\t\tcontinue;\n+\t\tif (pathspec->exclude &&\n+\t\t\tpathspec->exclude->nr &&\n+\t\t\tmatch_pathspec_depth(pathspec->exclude, name, namelen, 0, NULL))\n+\t\t\tcontinue;\n \t\thit |= grep_file(opt, dir.entries[i]->name);\n \t\tif (hit && opt->status_only)\n \t\t\tbreak;\n@@ -764,6 +785,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\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 +821,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 +901,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 +1084,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\tcreate_pathspec_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 +1139,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 10afd71..882a390 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;\n };\n \n extern int init_pathspec(struct pathspec *, const char **);\n+extern int create_pathspec_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..fd04afa 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@@ -1259,6 +1261,49 @@ 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 create_pathspec_from_string_list(struct pathspec **pathspec, const struct string_list *path_list)\n+{\n+\tint i;\n+\n+\t*pathspec = xcalloc( 1, sizeof( struct pathspec ) );\n+\n+\tif (!path_list->nr)\n+\t\treturn 0;\n+\n+\t(*pathspec)->nr = path_list->nr;\n+\t(*pathspec)->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 = (*pathspec)->items+i;\n+\t\tconst char *path = path_list->items[i].string;\n+\n+\t\t(*pathspec)->has_wildcard |= init_pathspec_item( item, path );\n+\t}\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* A possible problem might arrise if other areas of the code make\n+\t* the assumption that a NULL pathspec->raw means that pathspec->nr\n+\t* is zero.\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@@ -1279,11 +1324,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@@ -1296,4 +1337,8 @@ void free_pathspec(struct pathspec *pathspec)\n {\n \tfree(pathspec->items);\n \tpathspec->items = NULL;\n+\tif (pathspec->exclude) {\n+\t\tfree_pathspec(pathspec->exclude);\n+\t\tpathspec->exclude = NULL;\n+\t}\n }\n-- \n1.7.8.3\n"},{"id":"183263","messageId":"7vvcnudsda.fsf@alter.siamese.dyndns.org","threadId":"29467","inReplyTo":"1327876934-61526-1-git-send-email-surfingalbert@gmail.com","subject":"Re: [PATCH/RFC v2] grep: Add the option '--exclude'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-29T23:02:57Z","receivedAt":"2012-01-29T23:02:57Z","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> Feedback would again be appreciated,\n\nHmm.\n\n>  \t   [-f <file>] [-e] <pattern>\n> +\t   [-x<pattern>|--exclude<pattern>]\n\nCompare the above two lines and notice some funny in the added one?\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> +\n\nLikewise.\n\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 9ce064a..9772fa4 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -530,6 +530,10 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int\n>  \t\t\tcontinue;\n>  \t\tif (!match_pathspec_depth(pathspec, ce->name, ce_namelen(ce), 0, NULL))\n>  \t\t\tcontinue;\n> +\t\tif (pathspec->exclude &&\n> +\t\t\tpathspec->exclude->nr &&\n> +\t\t\tmatch_pathspec_depth(pathspec->exclude, ce->name, ce_namelen(ce), 0, NULL))\n> +\t\t\tcontinue;\n\nWhy isn't this just\n\n\tif (!match_pathspec_depth(pathspec, ce->name, ce_namelen(ce), 0, NULL))\n\nthat is, *no change whatsoever* on the existing codepaths that call\nmatch_pathspec_depth()?  Can't the \"even if one of the positive pathspec\nmatched, if one of the excluded one matches, declare that the path does\n*not* match\" logic live in match_pathspec_depth() itself?\n\nExactly the same comment applies to all the other additions that calls\nmatch_pathspec_depth() with pathspec->exclude as its first parameter in\nthis patch.\n\n> @@ -1053,6 +1084,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\tcreate_pathspec_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\nStyle. Notice where SPs should be near parentheses on \"if\" statement in\nour codebase.\n"},{"id":"183272","messageId":"CALEc4zEQz1eVAgqGz2g5taEDnSMwLWePxHiT=543LWX-7zCDFg@mail.gmail.com","threadId":"29467","inReplyTo":"7vvcnudsda.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC v2] grep: Add the option '--exclude'","fromName":"Albert Yale","fromEmail":"surfingalbert@gmail.com","sentAt":"2012-01-30T00:01:25Z","receivedAt":"2012-01-30T00:01:25Z","isPatch":true,"sender":{"key":"surfingalbert@gmail.com","avatar":null},"body":"Thank you for your feedback Junio.\nI'll revise my patch next weekend.\n\nAlbert Yale\n\nOn Sun, Jan 29, 2012 at 6:02 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Albert Yale <surfingalbert@gmail.com> writes:\n>\n> > Feedback would again be appreciated,\n>\n> Hmm.\n>\n> >          [-f <file>] [-e] <pattern>\n> > +        [-x<pattern>|--exclude<pattern>]\n>\n> Compare the above two lines and notice some funny in the added one?\n>\n> > @@ -124,6 +125,12 @@ OPTIONS\n> >       Use fixed strings for patterns (don't interpret pattern\n> >       as a regex).\n> >\n> > +-x<pattern>::\n> > +--exclude<pattern>::\n> > +     In addition to those found in .gitignore (per directory) and\n> > +     $GIT_DIR/info/exclude, also consider these patterns to be in the\n> > +     set of the ignore rules in effect.\n> > +\n>\n> Likewise.\n>\n> > diff --git a/builtin/grep.c b/builtin/grep.c\n> > index 9ce064a..9772fa4 100644\n> > --- a/builtin/grep.c\n> > +++ b/builtin/grep.c\n> > @@ -530,6 +530,10 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int\n> >                       continue;\n> >               if (!match_pathspec_depth(pathspec, ce->name, ce_namelen(ce), 0, NULL))\n> >                       continue;\n> > +             if (pathspec->exclude &&\n> > +                     pathspec->exclude->nr &&\n> > +                     match_pathspec_depth(pathspec->exclude, ce->name, ce_namelen(ce), 0, NULL))\n> > +                     continue;\n>\n> Why isn't this just\n>\n>        if (!match_pathspec_depth(pathspec, ce->name, ce_namelen(ce), 0, NULL))\n>\n> that is, *no change whatsoever* on the existing codepaths that call\n> match_pathspec_depth()?  Can't the \"even if one of the positive pathspec\n> matched, if one of the excluded one matches, declare that the path does\n> *not* match\" logic live in match_pathspec_depth() itself?\n>\n> Exactly the same comment applies to all the other additions that calls\n> match_pathspec_depth() with pathspec->exclude as its first parameter in\n> this patch.\n>\n> > @@ -1053,6 +1084,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> >       pathspec.max_depth = opt.max_depth;\n> >       pathspec.recursive = 1;\n> >\n> > +     if( exclude_list.nr ) {\n> > +             create_pathspec_from_string_list(&pathspec.exclude, &exclude_list);\n> > +             pathspec.exclude->max_depth = opt.max_depth;\n> > +             pathspec.exclude->recursive = 1;\n> > +     }\n> > +\n>\n> Style. Notice where SPs should be near parentheses on \"if\" statement in\n> our codebase.\n"}]}