grep --no-index and pathspec

grep --no-index and pathspec

[Lars Noschinski]

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

Re: grep --no-index and pathspec

[Michael J Gruber]

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

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

[Michael J Gruber]

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

Re: grep --no-index and pathspec

[Junio C Hamano]

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 ;-).

Re: grep --no-index and pathspec

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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;

Re: grep --no-index and pathspec

[Nguyen Thai Ngoc Duy]

2011/2/12 Junio C Hamano <gitster@pobox.com>:
> 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

Re: grep --no-index and pathspec

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> 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.

Re: grep --no-index and pathspec

[Nguyen Thai Ngoc Duy]

On Sat, Feb 12, 2011 at 3:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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