# grep --no-index and pathspec 8 messages from 2011-02-11 to 2011-02-12. Participants: Lars Noschinski, Michael J Gruber, Junio C Hamano, Nguyen Thai Ngoc Duy. Thread: https://gitlist.dev/t/26470 ## Lars Noschinski, 2011-02-11 08:59 Subject: grep --no-index and pathspec Message-ID: <20110211095938.360726y1zinab9gk@webmail.df.eu> URL: https://gitlist.dev/e/20110211095938.360726y1zinab9gk%40webmail.df.eu ``` Hi everyone, I encountered some strange behaviour with grep when using both the --no-index option and a pathspec. Glob patterns seem to be ignored: ---------- $ git grep -l --no-index . -- '*.bib' paper.bib paper.tex ex1.tex ---------- But on the other hands, leading path matches work: ---------- $ git grep -l --no-index . -- 'paper' paper.bib paper.tex ---------- Without the --no-index option, everything works fine: ---------- $ git grep -l --no-index . -- '*.bib' paper.bib ---------- This is with git version 1.7.4, but I encountered it also with the 1.7.2.3 Debian package. -- Lars ``` ## Michael J Gruber, 2011-02-11 15:04 Subject: Re: grep --no-index and pathspec Message-ID: <4D55500B.1070603@drmicha.warpmail.net> URL: https://gitlist.dev/e/4D55500B.1070603%40drmicha.warpmail.net In-Reply-To: <20110211095938.360726y1zinab9gk@webmail.df.eu> ``` Lars Noschinski venit, vidit, dixit 11.02.2011 09:59: > Hi everyone, > > I encountered some strange behaviour with grep when using both the > --no-index option and a pathspec. Glob patterns seem to be ignored: > > ---------- > $ git grep -l --no-index . -- '*.bib' > paper.bib > paper.tex > ex1.tex > ---------- > > But on the other hands, leading path matches work: > ---------- > $ git grep -l --no-index . -- 'paper' > paper.bib > paper.tex > ---------- > > Without the --no-index option, everything works fine: > ---------- > $ git grep -l --no-index . -- '*.bib' > paper.bib > ---------- > > This is with git version 1.7.4, but I encountered it also with the > 1.7.2.3 Debian package. "grep --no-index" and "grep" have different codepaths for looking up the files/blobs. If I read that correctly then "grep --no-index -- pathspec" only does a literal match at the left boundary, whereas for the normal mode glob patterns are allowed. CC'ing Junio who created "--no-index". Michael ``` ## Michael J Gruber, 2011-02-11 15:06 Subject: [PATCH] grep.txt: document pathspec for --no-index Message-ID: <7150921343449bab0c43401ad204f090c111d7f0.1297436625.git.git@drmicha.warpmail.net> URL: https://gitlist.dev/e/7150921343449bab0c43401ad204f090c111d7f0.1297436625.git.git%40drmicha.warpmail.net In-Reply-To: <4D55500B.1070603@drmicha.warpmail.net> ``` because it allows leading path match only, no globs. Signed-off-by: Michael J Gruber --- Documentation/git-grep.txt | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt index dab0a78..ef01a57 100644 --- a/Documentation/git-grep.txt +++ b/Documentation/git-grep.txt @@ -186,7 +186,8 @@ OPTIONS ...:: If given, limit the search to paths matching at least one pattern. - Both leading paths match and glob(7) patterns are supported. + Both leading paths match and glob(7) patterns are supported + unless `--no-index` is used, which supports only the former. Examples -------- -- 1.7.4.91.g3d0bb ``` ## Junio C Hamano, 2011-02-11 18:27 Subject: Re: grep --no-index and pathspec Message-ID: <7v8vxm1l6q.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v8vxm1l6q.fsf%40alter.siamese.dyndns.org In-Reply-To: <4D55500B.1070603@drmicha.warpmail.net> ``` Michael J Gruber writes: > "grep --no-index" and "grep" have different codepaths for looking up the > files/blobs. If I read that correctly then "grep --no-index -- pathspec" > only does a literal match at the left boundary, whereas for the normal > mode glob patterns are allowed. > > CC'ing Junio who created "--no-index". Anything with --no-index is a quick hack, so I wouldn't be surprised if it ignored the normal pathspec logic. As I do not recall the details of the particular codepath and offhand do not know how involved a change to pay proper attention to the pathspecs would be, but I suspect that it would be more appropriate to fix it on top of nd/struct-pathspec topic than writing the current behaviour down in the documentation outside of BUGS section as if it were a feature ;-). ``` ## Junio C Hamano, 2011-02-11 21:37 Subject: Re: grep --no-index and pathspec Message-ID: <7vwrl6z20p.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vwrl6z20p.fsf%40alter.siamese.dyndns.org In-Reply-To: <7v8vxm1l6q.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano writes: > Michael J Gruber writes: > >> "grep --no-index" and "grep" have different codepaths for looking up the >> files/blobs. If I read that correctly then "grep --no-index -- pathspec" >> only does a literal match at the left boundary, whereas for the normal >> mode glob patterns are allowed. >> >> CC'ing Junio who created "--no-index". > > Anything with --no-index is a quick hack, so I wouldn't be surprised if it > ignored the normal pathspec logic. As I do not recall the details of the > particular codepath and offhand do not know how involved a change to pay > proper attention to the pathspecs would be, but I suspect that it would be > more appropriate to fix it on top of nd/struct-pathspec topic than writing > the current behaviour down in the documentation outside of BUGS section as > if it were a feature ;-). This is a band-aid modelled after what builtin/clean.c does to the returned list from fill_directory(), and it seems to do its job, but I am quite unhappy about it. The function fill_directory() already takes a pathspec, albeit in the degenerate "const char **" form. Why does its output need further filtering? builtin/grep.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/builtin/grep.c b/builtin/grep.c index c3af876..5afee2f 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -626,6 +626,10 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec) fill_directory(&dir, pathspec->raw); for (i = 0; i < dir.nr; i++) { + const char *name = dir.entries[i]->name; + int namelen = strlen(name); + if (!match_pathspec_depth(pathspec, name, namelen, 0, NULL)) + continue; hit |= grep_file(opt, dir.entries[i]->name); if (hit && opt->status_only) break; ``` ## Nguyen Thai Ngoc Duy, 2011-02-12 08:14 Subject: Re: grep --no-index and pathspec Message-ID: URL: https://gitlist.dev/e/AANLkTikG1C%3D7NRGoi%2BHWz8rE9RN8-pF6o0%3DS29GZA3eK%40mail.gmail.com In-Reply-To: <7vwrl6z20p.fsf@alter.siamese.dyndns.org> ``` 2011/2/12 Junio C Hamano : > This is a band-aid modelled after what builtin/clean.c does to the > returned list from fill_directory(), and it seems to do its job, but I am > quite unhappy about it. > > The function fill_directory() already takes a pathspec, albeit in the > degenerate "const char **" form.  Why does its output need further > filtering? Because it was designed so? Quotes from 9fc42d6 (Optimize directory listing with pathspec limiter. - 2007-03-30), which added simplify_away(), the function that does pathspec filtering for fill_directory(): NOTE! This does *not* obviate the need for the caller to do the *exact* pathspec match later. It's a first-level filter on "read_directory()", but it does not do the full pathspec thing. Maybe it should. But in the meantime, builtin-add.c really does need to do first read_directory(dir, .., pathspec); if (pathspec) prune_directory(dir, pathspec, baselen); ie the "prune_directory()" part will do the *exact* pathspec pruning, while the "read_directory()" will use the pathspec just to do some quick high-level pruning of the directories it will recurse into. > @@ -626,6 +626,10 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec) > >        fill_directory(&dir, pathspec->raw); >        for (i = 0; i < dir.nr; i++) { > +               const char *name = dir.entries[i]->name; > +               int namelen = strlen(name); > +               if (!match_pathspec_depth(pathspec, name, namelen, 0, NULL)) > +                       continue; >                hit |= grep_file(opt, dir.entries[i]->name); >                if (hit && opt->status_only) >                        break; Looks good. We could move prune_directory() from builtin/add.c to dir.c and use it here, but the gain is nothing (except noticing people some pathspecs do not match any). -- Duy ``` ## Junio C Hamano, 2011-02-12 08:26 Subject: Re: grep --no-index and pathspec Message-ID: <7vvd0py7xy.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vvd0py7xy.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Nguyen Thai Ngoc Duy writes: > 2011/2/12 Junio C Hamano : >> >> The function fill_directory() already takes a pathspec, albeit in the >> degenerate "const char **" form. Why does its output need further >> filtering? > > Because it was designed so? Quotes from 9fc42d6 (Optimize directory > listing with pathspec limiter. - 2007-03-30), which added > simplify_away(), the function that does pathspec filtering for > fill_directory(): > > NOTE! This does *not* obviate the need for the caller to do the *exact* > pathspec match later. It's a first-level filter on "read_directory()", but > it does not do the full pathspec thing. Maybe it should. But in the > meantime,... I was around back then, so I know how the code came about ;-) The pieces used in the pathspec limiting logic have been restructured well enough that I suspect it may now be feasible for us to revisit the "Maybe it should" part in the above quote. Thanks to nd/struct-pathspec topic, I think we are already half-way there. ``` ## Nguyen Thai Ngoc Duy, 2011-02-12 08:39 Subject: Re: grep --no-index and pathspec Message-ID: URL: https://gitlist.dev/e/AANLkTikZuRyyZ4tErYuo1itmEs1X_gT5aogpTM3s4gON%40mail.gmail.com In-Reply-To: <7vvd0py7xy.fsf@alter.siamese.dyndns.org> ``` On Sat, Feb 12, 2011 at 3:26 PM, Junio C Hamano wrote: > Nguyen Thai Ngoc Duy writes: >>     NOTE! This does *not* obviate the need for the caller to do the *exact* >>     pathspec match later. It's a first-level filter on "read_directory()", but >>     it does not do the full pathspec thing. Maybe it should. But in the >>     meantime,... > > I was around back then, so I know how the code came about ;-) > > The pieces used in the pathspec limiting logic have been restructured well > enough that I suspect it may now be feasible for us to revisit the "Maybe > it should" part in the above quote.  Thanks to nd/struct-pathspec topic, I > think we are already half-way there. I was around too, just oblivious about things. I can look into that. Need to think a bit how to save what pathspecs are "seen", so that prune_directory() in builtin/add.c can be dropped. -- Duy ```