threads / discuss / 26470

grep --no-index and pathspec

Subject: grep --no-index and pathspec

## tl;dr

8 messages between Feb 11, 2011 and Feb 12, 2011.

replies: 7people: 4as markdown or json

Lars Noschinski· Feb 11, 2011, 08:59 UTC · lore
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· Feb 11, 2011, 15:04 UTC · re: Lars Noschinski · lore

Re: grep --no-index and pathspec

Lars Noschinski venit, vidit, dixit 11.02.2011 09:59:
Show 27 quoted lines
> 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· Feb 11, 2011, 15:06 UTC · re: Michael J Gruber · lore

[PATCH] grep.txt: document pathspec for --no-index

because it allows leading path match only, no globs.
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 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
 
 <pathspec>...::
 	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· Feb 11, 2011, 18:27 UTC · re: Michael J Gruber · lore

Re: grep --no-index and pathspec

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 6 quoted lines
> "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· Feb 11, 2011, 21:37 UTC · re: Junio C Hamano · lore

Re: grep --no-index and pathspec

Junio C Hamano <gitster@pobox.com> writes:
Show 16 quoted lines
> Michael J Gruber <git@drmicha.warpmail.net> 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· Feb 12, 2011, 08:14 UTC · re: Junio C Hamano · lore

Re: grep --no-index and pathspec

2011/2/12 Junio C Hamano <gitster@pobox.com>:
Show 7 quoted lines
> 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.
Show 11 quoted lines
> @@ -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· Feb 12, 2011, 08:26 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: grep --no-index and pathspec

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
Show 15 quoted lines
> 2011/2/12 Junio C Hamano <gitster@pobox.com>:
>>
>> 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· Feb 12, 2011, 08:39 UTC · re: Junio C Hamano · lore

Re: grep --no-index and pathspec

On Sat, Feb 12, 2011 at 3:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> 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

← back to recent threads