{"thread":{"id":"26470","subject":"grep --no-index and pathspec","startedAt":"2011-02-11T08:59:38Z","lastAt":"2011-02-12T08:39:31Z","messageCount":8,"participants":["Lars Noschinski","Michael J Gruber","Junio C Hamano","Nguyen Thai Ngoc Duy"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"160883","messageId":"20110211095938.360726y1zinab9gk@webmail.df.eu","threadId":"26470","inReplyTo":null,"subject":"grep --no-index and pathspec","fromName":"Lars Noschinski","fromEmail":"lars@public.noschinski.de","sentAt":"2011-02-11T08:59:38Z","receivedAt":"2011-02-11T08:59:38Z","isPatch":false,"sender":{"key":"lars@public.noschinski.de","avatar":"https://gravatar.com/avatar/ca62bd8b265f2e26c89d39a4bfe7e390bfa6b16d6400e186e222d1c2382c66f2?d=mp&s=160"},"body":"Hi everyone,\n\nI encountered some strange behaviour with grep when using both the \n--no-index option and a pathspec. Glob patterns seem to be ignored:\n\n----------\n$ git grep -l --no-index . -- '*.bib'\npaper.bib\npaper.tex\nex1.tex\n----------\n\nBut on the other hands, leading path matches work:\n----------\n$ git grep -l --no-index . -- 'paper'\npaper.bib\npaper.tex\n----------\n\nWithout the --no-index option, everything works fine:\n----------\n$ git grep -l --no-index . -- '*.bib'\npaper.bib\n----------\n\nThis is with git version 1.7.4, but I encountered it also with the \n1.7.2.3 Debian package.\n\n -- Lars\n"},{"id":"160897","messageId":"4D55500B.1070603@drmicha.warpmail.net","threadId":"26470","inReplyTo":"20110211095938.360726y1zinab9gk@webmail.df.eu","subject":"Re: grep --no-index and pathspec","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-02-11T15:04:43Z","receivedAt":"2011-02-11T15:04:43Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Lars Noschinski venit, vidit, dixit 11.02.2011 09:59:\n> Hi everyone,\n> \n> I encountered some strange behaviour with grep when using both the \n> --no-index option and a pathspec. Glob patterns seem to be ignored:\n> \n> ----------\n> $ git grep -l --no-index . -- '*.bib'\n> paper.bib\n> paper.tex\n> ex1.tex\n> ----------\n> \n> But on the other hands, leading path matches work:\n> ----------\n> $ git grep -l --no-index . -- 'paper'\n> paper.bib\n> paper.tex\n> ----------\n> \n> Without the --no-index option, everything works fine:\n> ----------\n> $ git grep -l --no-index . -- '*.bib'\n> paper.bib\n> ----------\n> \n> This is with git version 1.7.4, but I encountered it also with the \n> 1.7.2.3 Debian package.\n\n\"grep --no-index\" and \"grep\" have different codepaths for looking up the\nfiles/blobs. If I read that correctly then \"grep --no-index -- pathspec\"\nonly does a literal match at the left boundary, whereas for the normal\nmode glob patterns are allowed.\n\nCC'ing Junio who created \"--no-index\".\n\nMichael\n"},{"id":"160898","messageId":"7150921343449bab0c43401ad204f090c111d7f0.1297436625.git.git@drmicha.warpmail.net","threadId":"26470","inReplyTo":"4D55500B.1070603@drmicha.warpmail.net","subject":"[PATCH] grep.txt: document pathspec for --no-index","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-02-11T15:06:47Z","receivedAt":"2011-02-11T15:06:47Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"because it allows leading path match only, no globs.\n\nSigned-off-by: Michael J Gruber \n---\n Documentation/git-grep.txt | 3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex dab0a78..ef01a57 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -186,7 +186,8 @@ OPTIONS\n \n ...::\n \tIf given, limit the search to paths matching at least one pattern.\n-\tBoth leading paths match and glob(7) patterns are supported.\n+\tBoth leading paths match and glob(7) patterns are supported\n+\tunless `--no-index` is used, which supports only the former.\n \n Examples\n --------\n-- \n1.7.4.91.g3d0bb\n"},{"id":"160905","messageId":"7v8vxm1l6q.fsf@alter.siamese.dyndns.org","threadId":"26470","inReplyTo":"4D55500B.1070603@drmicha.warpmail.net","subject":"Re: grep --no-index and pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-11T18:27:09Z","receivedAt":"2011-02-11T18:27:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> \"grep --no-index\" and \"grep\" have different codepaths for looking up the\n> files/blobs. If I read that correctly then \"grep --no-index -- pathspec\"\n> only does a literal match at the left boundary, whereas for the normal\n> mode glob patterns are allowed.\n>\n> CC'ing Junio who created \"--no-index\".\n\nAnything with --no-index is a quick hack, so I wouldn't be surprised if it\nignored the normal pathspec logic. As I do not recall the details of the\nparticular codepath and offhand do not know how involved a change to pay\nproper attention to the pathspecs would be, but I suspect that it would be\nmore appropriate to fix it on top of nd/struct-pathspec topic than writing\nthe current behaviour down in the documentation outside of BUGS section as\nif it were a feature ;-).\n"},{"id":"160912","messageId":"7vwrl6z20p.fsf@alter.siamese.dyndns.org","threadId":"26470","inReplyTo":"7v8vxm1l6q.fsf@alter.siamese.dyndns.org","subject":"Re: grep --no-index and pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-11T21:37:10Z","receivedAt":"2011-02-11T21:37:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Michael J Gruber writes:\n>\n>> \"grep --no-index\" and \"grep\" have different codepaths for looking up the\n>> files/blobs. If I read that correctly then \"grep --no-index -- pathspec\"\n>> only does a literal match at the left boundary, whereas for the normal\n>> mode glob patterns are allowed.\n>>\n>> CC'ing Junio who created \"--no-index\".\n>\n> Anything with --no-index is a quick hack, so I wouldn't be surprised if it\n> ignored the normal pathspec logic. As I do not recall the details of the\n> particular codepath and offhand do not know how involved a change to pay\n> proper attention to the pathspecs would be, but I suspect that it would be\n> more appropriate to fix it on top of nd/struct-pathspec topic than writing\n> the current behaviour down in the documentation outside of BUGS section as\n> if it were a feature ;-).\n\nThis is a band-aid modelled after what builtin/clean.c does to the\nreturned list from fill_directory(), and it seems to do its job, but I am\nquite unhappy about it.\n\nThe function fill_directory() already takes a pathspec, albeit in the\ndegenerate \"const char **\" form. Why does its output need further\nfiltering?\n\n builtin/grep.c | 4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex c3af876..5afee2f 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -626,6 +626,10 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec)\n \n \tfill_directory(&dir, pathspec->raw);\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\t\tcontinue;\n \t\thit |= grep_file(opt, dir.entries[i]->name);\n \t\tif (hit && opt->status_only)\n \t\t\tbreak;\n"},{"id":"160936","messageId":"AANLkTikG1C=7NRGoi+HWz8rE9RN8-pF6o0=S29GZA3eK@mail.gmail.com","threadId":"26470","inReplyTo":"7vwrl6z20p.fsf@alter.siamese.dyndns.org","subject":"Re: grep --no-index and pathspec","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-02-12T08:14:59Z","receivedAt":"2011-02-12T08:14:59Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2011/2/12 Junio C Hamano :\n> This is a band-aid modelled after what builtin/clean.c does to the\n> returned list from fill_directory(), and it seems to do its job, but I am\n> quite unhappy about it.\n>\n> The function fill_directory() already takes a pathspec, albeit in the\n> degenerate \"const char **\" form.  Why does its output need further\n> filtering?\n\nBecause it was designed so? Quotes from 9fc42d6 (Optimize directory\nlisting with pathspec limiter. - 2007-03-30), which added\nsimplify_away(), the function that does pathspec filtering for\nfill_directory():\n\n NOTE! This does *not* obviate the need for the caller to do the *exact*\n pathspec match later. It's a first-level filter on \"read_directory()\", but\n it does not do the full pathspec thing. Maybe it should. But in the\n meantime, builtin-add.c really does need to do first\n\n read_directory(dir, .., pathspec);\n if (pathspec)\n prune_directory(dir, pathspec, baselen);\n\n ie the \"prune_directory()\" part will do the *exact* pathspec pruning,\n while the \"read_directory()\" will use the pathspec just to do some quick\n high-level pruning of the directories it will recurse into.\n\n> @@ -626,6 +626,10 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec)\n>\n>        fill_directory(&dir, pathspec->raw);\n>        for (i = 0; i < dir.nr; i++) {\n> +               const char *name = dir.entries[i]->name;\n> +               int namelen = strlen(name);\n> +               if (!match_pathspec_depth(pathspec, name, namelen, 0, NULL))\n> +                       continue;\n>                hit |= grep_file(opt, dir.entries[i]->name);\n>                if (hit && opt->status_only)\n>                        break;\n\nLooks good. We could move prune_directory() from builtin/add.c to\ndir.c and use it here, but the gain is nothing (except noticing people\nsome pathspecs do not match any).\n-- \nDuy\n"},{"id":"160939","messageId":"7vvd0py7xy.fsf@alter.siamese.dyndns.org","threadId":"26470","inReplyTo":"AANLkTikG1C=7NRGoi+HWz8rE9RN8-pF6o0=S29GZA3eK@mail.gmail.com","subject":"Re: grep --no-index and pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-12T08:26:49Z","receivedAt":"2011-02-12T08:26:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n> 2011/2/12 Junio C Hamano :\n>>\n>> The function fill_directory() already takes a pathspec, albeit in the\n>> degenerate \"const char **\" form. Why does its output need further\n>> filtering?\n>\n> Because it was designed so? Quotes from 9fc42d6 (Optimize directory\n> listing with pathspec limiter. - 2007-03-30), which added\n> simplify_away(), the function that does pathspec filtering for\n> fill_directory():\n>\n> NOTE! This does *not* obviate the need for the caller to do the *exact*\n> pathspec match later. It's a first-level filter on \"read_directory()\", but\n> it does not do the full pathspec thing. Maybe it should. But in the\n> meantime,...\n\nI was around back then, so I know how the code came about ;-)\n\nThe pieces used in the pathspec limiting logic have been restructured well\nenough that I suspect it may now be feasible for us to revisit the \"Maybe\nit should\" part in the above quote. Thanks to nd/struct-pathspec topic, I\nthink we are already half-way there.\n"},{"id":"160941","messageId":"AANLkTikZuRyyZ4tErYuo1itmEs1X_gT5aogpTM3s4gON@mail.gmail.com","threadId":"26470","inReplyTo":"7vvd0py7xy.fsf@alter.siamese.dyndns.org","subject":"Re: grep --no-index and pathspec","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-02-12T08:39:31Z","receivedAt":"2011-02-12T08:39:31Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Feb 12, 2011 at 3:26 PM, Junio C Hamano wrote:\n> Nguyen Thai Ngoc Duy writes:\n>>     NOTE! This does *not* obviate the need for the caller to do the *exact*\n>>     pathspec match later. It's a first-level filter on \"read_directory()\", but\n>>     it does not do the full pathspec thing. Maybe it should. But in the\n>>     meantime,...\n>\n> I was around back then, so I know how the code came about ;-)\n>\n> The pieces used in the pathspec limiting logic have been restructured well\n> enough that I suspect it may now be feasible for us to revisit the \"Maybe\n> it should\" part in the above quote.  Thanks to nd/struct-pathspec topic, I\n> think we are already half-way there.\n\nI was around too, just oblivious about things. I can look into that.\nNeed to think a bit how to save what pathspecs are \"seen\", so that\nprune_directory() in builtin/add.c can be dropped.\n-- \nDuy\n"}]}