[RFC 0/2] grep: make output consistent with revision syntax

[RFC 0/2] grep: make output consistent with revision syntax

[Stefan Hajnoczi]

git-grep(1)'s output is not consistent with git-rev-parse(1) revision syntax.

This means you cannot take "rev:path/to/file.c: foo();" output from git-grep(1)
and expect "git show rev:path/to/file.c" to work.  See the individual patches
for examples of command-lines that produce invalid output.

This series is an incomplete attempt at solving the issue.  I'm not familiar
enough with the git codebase to propose a better solution.  Perhaps someone is
interested in a proper fix?

Stefan Hajnoczi (2):
  grep: only add delimiter if there isn't one already
  grep: use '/' delimiter for paths

 builtin/grep.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

-- 
2.9.3

[RFC 1/2] grep: only add delimiter if there isn't one already

[Stefan Hajnoczi]

git-grep(1) output does not follow git's own syntax:

  $ git grep malloc v2.9.3:
  v2.9.3::Makefile:       COMPAT_CFLAGS += -Icompat/nedmalloc
  $ git show v2.9.3::Makefile
  fatal: Path ':Makefile' does not exist in 'v2.9.3'

This patch avoids emitting the unnecessary ':' delimiter if the name
already ends with ':' or '/':

  $ git grep malloc v2.9.3:
  v2.9.3:Makefile:       COMPAT_CFLAGS += -Icompat/nedmalloc
  $ git show v2.9.3:Makefile
  (succeeds)

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 builtin/grep.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 462e607..3643d8a 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -490,7 +490,11 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
 		strbuf_init(&base, PATH_MAX + len + 1);
 		if (len) {
 			strbuf_add(&base, name, len);
-			strbuf_addch(&base, ':');
+
+			/* Add a delimiter if there isn't one already */
+			if (name[len - 1] != '/' && name[len - 1] != ':') {
+				strbuf_addch(&base, ':');
+			}
 		}
 		init_tree_desc(&tree, data, size);
 		hit = grep_tree(opt, pathspec, &tree, &base, base.len,
-- 
2.9.3

[RFC 2/2] grep: use '/' delimiter for paths

[Stefan Hajnoczi]

If the tree contains a sub-directory then git-grep(1) output contains a
colon character instead of a path separator:

  $ git grep malloc v2.9.3:t
  v2.9.3:t:test-lib.sh:	setup_malloc_check () {
  $ git show v2.9.3:t:test-lib.sh
  fatal: Path 't:test-lib.sh' does not exist in 'v2.9.3'

This patch attempts to use the correct delimiter:

  $ git grep malloc v2.9.3:t
  v2.9.3:t/test-lib.sh:	setup_malloc_check () {
  $ git show v2.9.3:t/test-lib.sh
  (success)

This patch does not cope with @{1979-02-26 18:30:00} syntax and treats
it as a path because it contains colons.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 builtin/grep.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 3643d8a..06f8b47 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -493,7 +493,8 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
 
 			/* Add a delimiter if there isn't one already */
 			if (name[len - 1] != '/' && name[len - 1] != ':') {
-				strbuf_addch(&base, ':');
+				/* rev: or rev:path/ */
+				strbuf_addch(&base, strchr(name, ':') ? '/' : ':');
 			}
 		}
 		init_tree_desc(&tree, data, size);
-- 
2.9.3

Re: [RFC 0/2] grep: make output consistent with revision syntax

[Jeff King]

On Thu, Jan 19, 2017 at 03:03:45PM +0000, Stefan Hajnoczi wrote:

> git-grep(1)'s output is not consistent with git-rev-parse(1) revision syntax.
> 
> This means you cannot take "rev:path/to/file.c: foo();" output from git-grep(1)
> and expect "git show rev:path/to/file.c" to work.  See the individual patches
> for examples of command-lines that produce invalid output.

I think this is a good goal.

I couldn't immediately think of any cases where your patches would
misbehave, but my initial thought was that the "/" versus ":"
distinction is about whether the initial object is a tree or a commit.

You do still have to special case the root tree (so "v2.9.3:" does not
get any delimiter). I think "ends in a colon" is actually a reasonable
way of determining that.

> This series is an incomplete attempt at solving the issue.  I'm not familiar
> enough with the git codebase to propose a better solution.  Perhaps someone is
> interested in a proper fix?

Are there cases you know that aren't covered by your patches?

-Peff

Re: [RFC 0/2] grep: make output consistent with revision syntax

[Brandon Williams]

On 01/19, Jeff King wrote:
> On Thu, Jan 19, 2017 at 03:03:45PM +0000, Stefan Hajnoczi wrote:
> 
> > git-grep(1)'s output is not consistent with git-rev-parse(1) revision syntax.
> > 
> > This means you cannot take "rev:path/to/file.c: foo();" output from git-grep(1)
> > and expect "git show rev:path/to/file.c" to work.  See the individual patches
> > for examples of command-lines that produce invalid output.
> 
> I think this is a good goal.

I agree.

> I couldn't immediately think of any cases where your patches would
> misbehave, but my initial thought was that the "/" versus ":"
> distinction is about whether the initial object is a tree or a commit.

I think this is also the case, I couldn't think of another case where
this decision wasn't based on if the object is a tree or a commit.
Interestingly enough I don't think we have any tests that exist that
test the formatting of grep's output when given a tree object since the
test suite still passes with these changes in. Which means this fix
should probably include a couple tests to ensure there's no regression
in the future.

-- 
Brandon Williams

Re: [RFC 2/2] grep: use '/' delimiter for paths

[Brandon Williams]

On 01/19, Stefan Hajnoczi wrote:
> If the tree contains a sub-directory then git-grep(1) output contains a
> colon character instead of a path separator:
> 
>   $ git grep malloc v2.9.3:t
>   v2.9.3:t:test-lib.sh:	setup_malloc_check () {
>   $ git show v2.9.3:t:test-lib.sh
>   fatal: Path 't:test-lib.sh' does not exist in 'v2.9.3'
> 
> This patch attempts to use the correct delimiter:
> 
>   $ git grep malloc v2.9.3:t
>   v2.9.3:t/test-lib.sh:	setup_malloc_check () {
>   $ git show v2.9.3:t/test-lib.sh
>   (success)
> 
> This patch does not cope with @{1979-02-26 18:30:00} syntax and treats
> it as a path because it contains colons.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  builtin/grep.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 3643d8a..06f8b47 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -493,7 +493,8 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
>  
>  			/* Add a delimiter if there isn't one already */
>  			if (name[len - 1] != '/' && name[len - 1] != ':') {
> -				strbuf_addch(&base, ':');
> +				/* rev: or rev:path/ */
> +				strbuf_addch(&base, strchr(name, ':') ? '/' : ':');

As Jeff mentioned it may be better to base which character gets appended
by checking the obj->type field like this maybe:

diff --git a/builtin/grep.c b/builtin/grep.c
index 69dab5dc5..9dfe11dc7 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -495,7 +495,8 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
 			/* Add a delimiter if there isn't one already */
 			if (name[len - 1] != '/' && name[len - 1] != ':') {
 				/* rev: or rev:path/ */
-				strbuf_addch(&base, strchr(name, ':') ? '/' : ':');
+				char del = obj->type == OBJ_COMMIT ? ':' : '/';
+				strbuf_addch(&base, del);
 			}
 		}
 		init_tree_desc(&tree, data, size);

-- 
Brandon Williams

Re: [RFC 1/2] grep: only add delimiter if there isn't one already

[Junio C Hamano]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> git-grep(1) output does not follow git's own syntax:
>
>   $ git grep malloc v2.9.3:
>   v2.9.3::Makefile:       COMPAT_CFLAGS += -Icompat/nedmalloc
>   $ git show v2.9.3::Makefile
>   fatal: Path ':Makefile' does not exist in 'v2.9.3'
>
> This patch avoids emitting the unnecessary ':' delimiter if the name
> already ends with ':' or '/':
>
>   $ git grep malloc v2.9.3:
>   v2.9.3:Makefile:       COMPAT_CFLAGS += -Icompat/nedmalloc
>   $ git show v2.9.3:Makefile
>   (succeeds)

I am mildly negative on this one.  I suspect that the above example
is a made-up one and you might have a more compelling real-world use
case in mind, but at least the above does not convince me.

The end-user explicitly gave the extra ':' because she wanted to
feed the tree object, not a tag that leads to the tree, for some
reason.  I think the output should respect that and show how she
spelled the starting point.  IOW, it is not "':' added unnecessarily
by Git".  It is ':' added by the end-user for whatever reason.

>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  builtin/grep.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 462e607..3643d8a 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -490,7 +490,11 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
>  		strbuf_init(&base, PATH_MAX + len + 1);
>  		if (len) {
>  			strbuf_add(&base, name, len);
> -			strbuf_addch(&base, ':');
> +
> +			/* Add a delimiter if there isn't one already */
> +			if (name[len - 1] != '/' && name[len - 1] != ':') {
> +				strbuf_addch(&base, ':');
> +			}
>  		}
>  		init_tree_desc(&tree, data, size);
>  		hit = grep_tree(opt, pathspec, &tree, &base, base.len,

Re: [RFC 2/2] grep: use '/' delimiter for paths

[Junio C Hamano]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> If the tree contains a sub-directory then git-grep(1) output contains a
> colon character instead of a path separator:
>
>   $ git grep malloc v2.9.3:t
>   v2.9.3:t:test-lib.sh:	setup_malloc_check () {
>   $ git show v2.9.3:t:test-lib.sh
>   fatal: Path 't:test-lib.sh' does not exist in 'v2.9.3'

I am slightly less negative on this compared to 1/2, but not by a
big margin.  The user wanted to feed a subtree to the command,
instead of doing the more natural

    $ git grep -e malloc v2.9.3 -- t

So again, "contains a colon character" is not coming from what Git
does, but the user gave Git "a colon character" when she didn't have
to.

Having said that, if we wanted to start ignoring what the end-user
said in the initial input like 1/2 and 2/2 does (i.e. "this specific
tree object" as an input), I think the approach to solve for 1/2 and
2/2 should be the same.  I think we should decide to do a slash
instead of a colon, not because v2.9.3: has colon at the end and
v2.9.3:t has colon in it, but because both of these are both bare
tree objects.  The patches presented does not seem to base their
decision on the actual object type but on the textual input, which
seems wrong.

Re: [RFC 1/2] grep: only add delimiter if there isn't one already

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1751 bytes --]

On Thu, Jan 19, 2017 at 10:39:02AM -0800, Junio C Hamano wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
> > git-grep(1) output does not follow git's own syntax:
> >
> >   $ git grep malloc v2.9.3:
> >   v2.9.3::Makefile:       COMPAT_CFLAGS += -Icompat/nedmalloc
> >   $ git show v2.9.3::Makefile
> >   fatal: Path ':Makefile' does not exist in 'v2.9.3'
> >
> > This patch avoids emitting the unnecessary ':' delimiter if the name
> > already ends with ':' or '/':
> >
> >   $ git grep malloc v2.9.3:
> >   v2.9.3:Makefile:       COMPAT_CFLAGS += -Icompat/nedmalloc
> >   $ git show v2.9.3:Makefile
> >   (succeeds)
> 
> I am mildly negative on this one.  I suspect that the above example
> is a made-up one and you might have a more compelling real-world use
> case in mind, but at least the above does not convince me.

Another trailing delimiter example:

  $ git grep malloc v2.9.3:t/
  v2.9.3:t/:test-lib.sh:  setup_malloc_check () {

After Patch 1/2:

  v2.9.3:t/test-lib.sh:  setup_malloc_check () {

> The end-user explicitly gave the extra ':' because she wanted to
> feed the tree object, not a tag that leads to the tree, for some
> reason.  I think the output should respect that and show how she
> spelled the starting point.  IOW, it is not "':' added unnecessarily
> by Git".  It is ':' added by the end-user for whatever reason.

v2.9.3::Makefile may convey that the user originally provided v2.9.3:
but is that actually useful information?  I don't see what the user will
do with this information (and they should already know since they
provided the command-line).

v2.9.3:Makefile can be copy-pasted or extracted by scripts for further
git commands.  That is useful.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [RFC 2/2] grep: use '/' delimiter for paths

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2219 bytes --]

On Thu, Jan 19, 2017 at 10:54:02AM -0800, Junio C Hamano wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
> > If the tree contains a sub-directory then git-grep(1) output contains a
> > colon character instead of a path separator:
> >
> >   $ git grep malloc v2.9.3:t
> >   v2.9.3:t:test-lib.sh:	setup_malloc_check () {
> >   $ git show v2.9.3:t:test-lib.sh
> >   fatal: Path 't:test-lib.sh' does not exist in 'v2.9.3'
> 
> I am slightly less negative on this compared to 1/2, but not by a
> big margin.  The user wanted to feed a subtree to the command,
> instead of doing the more natural
> 
>     $ git grep -e malloc v2.9.3 -- t

I find <rev>:<path> vs <rev> -- <path> confusing:

            | <rev>:<path>         | <rev> -- <path>
  ----------+----------------------+---------------------
  git grep  | OK                   | OK
  ----------+----------------------+---------------------
  git show  | OK                   | <path> ignored
  ----------+----------------------+---------------------
  git log   | no output            | OK
  ----------+----------------------+---------------------

Neither syntax always does what I expect.  If git show <rev> -- <path>
honored <path> then I could use that syntax consistently.

Sorry for going on a tangent.  Does it seem reasonable to handle <path>
in git-show(1) as a UI convenience?

> So again, "contains a colon character" is not coming from what Git
> does, but the user gave Git "a colon character" when she didn't have
> to.
> 
> Having said that, if we wanted to start ignoring what the end-user
> said in the initial input like 1/2 and 2/2 does (i.e. "this specific
> tree object" as an input), I think the approach to solve for 1/2 and
> 2/2 should be the same.  I think we should decide to do a slash
> instead of a colon, not because v2.9.3: has colon at the end and
> v2.9.3:t has colon in it, but because both of these are both bare
> tree objects.  The patches presented does not seem to base their
> decision on the actual object type but on the textual input, which
> seems wrong.

Yes, reparsing the name is ugly and I hoped to get feedback with this
RFC.  Thanks for the quick review!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [RFC 2/2] grep: use '/' delimiter for paths

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2172 bytes --]

On Thu, Jan 19, 2017 at 10:29:34AM -0800, Brandon Williams wrote:
> On 01/19, Stefan Hajnoczi wrote:
> > If the tree contains a sub-directory then git-grep(1) output contains a
> > colon character instead of a path separator:
> > 
> >   $ git grep malloc v2.9.3:t
> >   v2.9.3:t:test-lib.sh:	setup_malloc_check () {
> >   $ git show v2.9.3:t:test-lib.sh
> >   fatal: Path 't:test-lib.sh' does not exist in 'v2.9.3'
> > 
> > This patch attempts to use the correct delimiter:
> > 
> >   $ git grep malloc v2.9.3:t
> >   v2.9.3:t/test-lib.sh:	setup_malloc_check () {
> >   $ git show v2.9.3:t/test-lib.sh
> >   (success)
> > 
> > This patch does not cope with @{1979-02-26 18:30:00} syntax and treats
> > it as a path because it contains colons.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  builtin/grep.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/builtin/grep.c b/builtin/grep.c
> > index 3643d8a..06f8b47 100644
> > --- a/builtin/grep.c
> > +++ b/builtin/grep.c
> > @@ -493,7 +493,8 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
> >  
> >  			/* Add a delimiter if there isn't one already */
> >  			if (name[len - 1] != '/' && name[len - 1] != ':') {
> > -				strbuf_addch(&base, ':');
> > +				/* rev: or rev:path/ */
> > +				strbuf_addch(&base, strchr(name, ':') ? '/' : ':');
> 
> As Jeff mentioned it may be better to base which character gets appended
> by checking the obj->type field like this maybe:
> 
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 69dab5dc5..9dfe11dc7 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -495,7 +495,8 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
>  			/* Add a delimiter if there isn't one already */
>  			if (name[len - 1] != '/' && name[len - 1] != ':') {
>  				/* rev: or rev:path/ */
> -				strbuf_addch(&base, strchr(name, ':') ? '/' : ':');
> +				char del = obj->type == OBJ_COMMIT ? ':' : '/';
> +				strbuf_addch(&base, del);

Nice, that also solves the false positive with @{1979-02-26 18:30:00}.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [RFC 0/2] grep: make output consistent with revision syntax

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1377 bytes --]

On Thu, Jan 19, 2017 at 11:59:59AM -0500, Jeff King wrote:
> On Thu, Jan 19, 2017 at 03:03:45PM +0000, Stefan Hajnoczi wrote:
> 
> > git-grep(1)'s output is not consistent with git-rev-parse(1) revision syntax.
> > 
> > This means you cannot take "rev:path/to/file.c: foo();" output from git-grep(1)
> > and expect "git show rev:path/to/file.c" to work.  See the individual patches
> > for examples of command-lines that produce invalid output.
> 
> I think this is a good goal.
> 
> I couldn't immediately think of any cases where your patches would
> misbehave, but my initial thought was that the "/" versus ":"
> distinction is about whether the initial object is a tree or a commit.
> 
> You do still have to special case the root tree (so "v2.9.3:" does not
> get any delimiter). I think "ends in a colon" is actually a reasonable
> way of determining that.
> 
> > This series is an incomplete attempt at solving the issue.  I'm not familiar
> > enough with the git codebase to propose a better solution.  Perhaps someone is
> > interested in a proper fix?
> 
> Are there cases you know that aren't covered by your patches?

From Patch 2/2:

  This patch does not cope with @{1979-02-26 18:30:00} syntax and treats
  it as a path because it contains colons.

If we use obj->type instead of re-parsing the name then that problem is
solved.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [RFC 2/2] grep: use '/' delimiter for paths

[Jeff King]

On Fri, Jan 20, 2017 at 02:12:12PM +0000, Stefan Hajnoczi wrote:

> I find <rev>:<path> vs <rev> -- <path> confusing:
> 
>             | <rev>:<path>         | <rev> -- <path>
>   ----------+----------------------+---------------------
>   git grep  | OK                   | OK
>   ----------+----------------------+---------------------
>   git show  | OK                   | <path> ignored
>   ----------+----------------------+---------------------
>   git log   | no output            | OK
>   ----------+----------------------+---------------------
> 
> Neither syntax always does what I expect.  If git show <rev> -- <path>
> honored <path> then I could use that syntax consistently.
> 
> Sorry for going on a tangent.  Does it seem reasonable to handle <path>
> in git-show(1) as a UI convenience?

It's not ignored; just as with git-log, it's a pathspec to limit the
diff. E.g.:

  $ git show --name-status v2.9.3
  ...
  M       Documentation/RelNotes/2.9.3.txt
  M       Documentation/git.txt
  M       GIT-VERSION-GEN

  $ git show --name-status v2.9.3 -- Documentation
  M       Documentation/RelNotes/2.9.3.txt
  M       Documentation/git.txt

That's typically less useful than it is with log (where limiting the
diff also kicks in history simplification and omits some commits
entirely). But it does do something.

-Peff

Re: [RFC 0/2] grep: make output consistent with revision syntax

[Jeff King]

On Fri, Jan 20, 2017 at 02:18:32PM +0000, Stefan Hajnoczi wrote:

> > Are there cases you know that aren't covered by your patches?
> 
> From Patch 2/2:
> 
>   This patch does not cope with @{1979-02-26 18:30:00} syntax and treats
>   it as a path because it contains colons.
> 
> If we use obj->type instead of re-parsing the name then that problem is
> solved.

Ah, right. I somehow totally missed that, and blindly assumed your colon
search was only at the end. But of course that wouldn't work at all (it
would miss "v2.9.3:t").

So yes, definitely it should be checking the object type.

-Peff

Re: [RFC 1/2] grep: only add delimiter if there isn't one already

[Junio C Hamano]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> v2.9.3::Makefile may convey that the user originally provided v2.9.3:
> but is that actually useful information?

You are either asking a wrong question, or asking a wrong person
(i.e. Git) the question.  The real question is why the user added a
colon at the end, when "v2.9.3" alone would have sufficed.  What did
the user want to get out of giving an extra colon like "v2.9.3:"?

One answer I can think of is that it is a degenerate case of [2/2],
i.e. if "v2.9.3:t" were an appropriate way to look in the subtree
"t" of "v2.9.3", "v2.9.3:" would be the appropriate way to look in
the whole tree of "v2.9.3".

I understand, from your response to my comment in the thread for
[2/2], that the reason why "v2.9.3:t" was used in the example was
because you felt uncomfortable with using pathspec.  

That's superstition.

My only piece of advice to folks who feel that way is to learn Git
more and get comfortable.  You can do neat things like

   $ git grep -e pattern rev -- t ':!t/helper/'

that you cannot do with "rev:t", for example ;-)

All examples we saw so far are the ones that the user used the colon
syntax when it is more natural to express the command without it.  I
am hesitant to see the behaviour of the command changed to appease
such suboptimal use cases if it requires us to discard a bit of
information, when we haven't established it is OK to lose.

There may be a case

 (1) where the colon syntax is the most natural thing to use, AND
     script reading the output that used that syntax is forced to do
     unnecessary work because "git grep" parrots the colon
     literally, instread of being more intelligent about it
     (i.e. omitting or substituting with slash when the input is a
     tree object, not a tree-ish, as discussed in the thread).

 (2) where the colon syntax is the most natural thing, AND script
     reading the output WANTS to see the distinction in the input
     (remember, "git grep" can take more than one input tree).

We haven't seen either of the above two in the discussion, so the
discussion so far is not sufficient to support the change being
proposed in this RFC, which requires that it is safe to assume that
(1) is the only case where the input is a raw tree (or the input
contains a colon) and (2) will never happen.

So I am still mildly negative on the whole thing.

Re: [RFC 2/2] grep: use '/' delimiter for paths

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It's not ignored; just as with git-log, it's a pathspec to limit the
> diff. E.g.:
>
>   $ git show --name-status v2.9.3
>   ...
>   M       Documentation/RelNotes/2.9.3.txt
>   M       Documentation/git.txt
>   M       GIT-VERSION-GEN
>
>   $ git show --name-status v2.9.3 -- Documentation
>   M       Documentation/RelNotes/2.9.3.txt
>   M       Documentation/git.txt
>
> That's typically less useful than it is with log (where limiting the
> diff also kicks in history simplification and omits some commits
> entirely). But it does do something.

I think Stefan is missing the fact that the argument to "git show
<tree-ish>:<path>" actually is naming a blob that sits at the <path>
in the <tree-ish>.  In other words, "show" is acting as a glorified
"git -p cat-file blob", in that use.

The use of "git show" you are demonstrating is still about showing
the commit object, whose behaviour is defined to show the log
message and the diff relative to its sole parent, limited to the
paths that match the pathspec.

It is perfectly fine and desirable that "git show <commit>:<path>"
and "git show <commit> -- <path>" behaves differently.  These are
two completely different features.

Re: [RFC 1/2] grep: only add delimiter if there isn't one already

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2866 bytes --]

On Fri, Jan 20, 2017 at 10:16:31AM -0800, Junio C Hamano wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
> > v2.9.3::Makefile may convey that the user originally provided v2.9.3:
> > but is that actually useful information?
> 
> You are either asking a wrong question, or asking a wrong person
> (i.e. Git) the question.  The real question is why the user added a
> colon at the end, when "v2.9.3" alone would have sufficed.  What did
> the user want to get out of giving an extra colon like "v2.9.3:"?
> 
> One answer I can think of is that it is a degenerate case of [2/2],
> i.e. if "v2.9.3:t" were an appropriate way to look in the subtree
> "t" of "v2.9.3", "v2.9.3:" would be the appropriate way to look in
> the whole tree of "v2.9.3".
> 
> I understand, from your response to my comment in the thread for
> [2/2], that the reason why "v2.9.3:t" was used in the example was
> because you felt uncomfortable with using pathspec.  
> 
> That's superstition.
> 
> My only piece of advice to folks who feel that way is to learn Git
> more and get comfortable.  You can do neat things like
> 
>    $ git grep -e pattern rev -- t ':!t/helper/'
> 
> that you cannot do with "rev:t", for example ;-)

Neat, thanks for showing the path exclusion syntax.  I wasn't aware of
it.

> All examples we saw so far are the ones that the user used the colon
> syntax when it is more natural to express the command without it.  I
> am hesitant to see the behaviour of the command changed to appease
> such suboptimal use cases if it requires us to discard a bit of
> information, when we haven't established it is OK to lose.
> 
> There may be a case
> 
>  (1) where the colon syntax is the most natural thing to use, AND
>      script reading the output that used that syntax is forced to do
>      unnecessary work because "git grep" parrots the colon
>      literally, instread of being more intelligent about it
>      (i.e. omitting or substituting with slash when the input is a
>      tree object, not a tree-ish, as discussed in the thread).
> 
>  (2) where the colon syntax is the most natural thing, AND script
>      reading the output WANTS to see the distinction in the input
>      (remember, "git grep" can take more than one input tree).
> 
> We haven't seen either of the above two in the discussion, so the
> discussion so far is not sufficient to support the change being
> proposed in this RFC, which requires that it is safe to assume that
> (1) is the only case where the input is a raw tree (or the input
> contains a colon) and (2) will never happen.
> 
> So I am still mildly negative on the whole thing.

Avoiding the colon syntax avoids the whole issue for my use case.

I still think git-grep(1)'s output would be more useful if it used valid
git rev:path syntax in all cases.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [RFC 2/2] grep: use '/' delimiter for paths

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1531 bytes --]

On Fri, Jan 20, 2017 at 02:56:26PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > It's not ignored; just as with git-log, it's a pathspec to limit the
> > diff. E.g.:
> >
> >   $ git show --name-status v2.9.3
> >   ...
> >   M       Documentation/RelNotes/2.9.3.txt
> >   M       Documentation/git.txt
> >   M       GIT-VERSION-GEN
> >
> >   $ git show --name-status v2.9.3 -- Documentation
> >   M       Documentation/RelNotes/2.9.3.txt
> >   M       Documentation/git.txt
> >
> > That's typically less useful than it is with log (where limiting the
> > diff also kicks in history simplification and omits some commits
> > entirely). But it does do something.
> 
> I think Stefan is missing the fact that the argument to "git show
> <tree-ish>:<path>" actually is naming a blob that sits at the <path>
> in the <tree-ish>.  In other words, "show" is acting as a glorified
> "git -p cat-file blob", in that use.
> 
> The use of "git show" you are demonstrating is still about showing
> the commit object, whose behaviour is defined to show the log
> message and the diff relative to its sole parent, limited to the
> paths that match the pathspec.
> 
> It is perfectly fine and desirable that "git show <commit>:<path>"
> and "git show <commit> -- <path>" behaves differently.  These are
> two completely different features.

Thanks for explaining guys.  It all makes logical sense.  I just hope I
remember the distinctions in that table for everyday git use.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [RFC 2/2] grep: use '/' delimiter for paths

[Phil Hord]

On Tue, Jan 24, 2017 at 1:54 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > The use of "git show" you are demonstrating is still about showing
> > the commit object, whose behaviour is defined to show the log
> > message and the diff relative to its sole parent, limited to the
> > paths that match the pathspec.
> >
> > It is perfectly fine and desirable that "git show <commit>:<path>"
> > and "git show <commit> -- <path>" behaves differently.  These are
> > two completely different features.
>
> Thanks for explaining guys.  It all makes logical sense.  I just hope I
> remember the distinctions in that table for everyday git use.


Familiar itch; previous discussion here:

https://public-inbox.org/git/1377528372-31206-1-git-send-email-hordp@cisco.com/

Phil

Re: [RFC 1/2] grep: only add delimiter if there isn't one already

[Jakub Narębski]

W dniu 23.01.2017 o 14:15, Stefan Hajnoczi pisze:
> On Fri, Jan 20, 2017 at 10:16:31AM -0800, Junio C Hamano wrote:

>> My only piece of advice to folks who feel that way is to learn Git
>> more and get comfortable.  You can do neat things like
>>
>>    $ git grep -e pattern rev -- t ':!t/helper/'
>>
>> that you cannot do with "rev:t", for example ;-)
> 
> Neat, thanks for showing the path exclusion syntax.  I wasn't aware of
> it.

That reminds me of mu TODO item: moving extended pathspec information
from gitglossary(7) manpage (sic!) to to-be-created gitpathspec(7).

-- 
Jakub Narębski

Re: [RFC 1/2] grep: only add delimiter if there isn't one already

[Philip Oakley]

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]

From: "Jakub Narębski" <jnareb@gmail.com>
>W dniu 23.01.2017 o 14:15, Stefan Hajnoczi pisze:
>> On Fri, Jan 20, 2017 at 10:16:31AM -0800, Junio C Hamano wrote:
>
>>> My only piece of advice to folks who feel that way is to learn Git
>>> more and get comfortable.  You can do neat things like
>>>
>>>    $ git grep -e pattern rev -- t ':!t/helper/'
>>>
>>> that you cannot do with "rev:t", for example ;-)
>>
>> Neat, thanks for showing the path exclusion syntax.  I wasn't aware of
>> it.
>
> That reminds me of mu TODO item: moving extended pathspec information
> from gitglossary(7) manpage (sic!) to to-be-created gitpathspec(7).
>

Good to see someone else also had it on a ToDo list..

Attached is my collation of all the different path spec info I found from 
trawling the man & guide pages to satisfy my ignorance...
--
Philip 

[-- Attachment #2: gitpathspec.txt --]
[-- Type: text/plain, Size: 12282 bytes --]

gitpathspec(7)
============

NAME
----
gitpathspec - How to specify a path or file to git

SYNOPSIS
--------
$HOME/.config/git/ignore, $GIT_DIR/info/exclude, .gitignore

DESCRIPTION
-----------

Pathspecs are used in a range of git functions.
.gitignore
.gitexclude
gitsparse
git-add -- pathspec
git-checkout -- pathspec (after the double-dash)
git grep (active/non-active wild card matching)
git log (L#:<> pathspec limiters ?? what does it mean)

git rerere (uncontentious)
git status (uncontentious)
gitk (uncontentious, but see 'log' above)
'git' itself --
--literal-pathspecs
Treat pathspecs literally (i.e. no globbing, no pathspec magic). This is equivalent to setting the GIT_LITERAL_PATHSPECS environment variable to 1.

--glob-pathspecs
Add "glob" magic to all pathspec. This is equivalent to setting the GIT_GLOB_PATHSPECS environment variable to 1. Disabling globbing on individual pathspecs can be done using pathspec magic ":(literal)"

--noglob-pathspecs
Add "literal" magic to all pathspec. This is equivalent to setting the GIT_NOGLOB_PATHSPECS environment variable to 1. Enabling globbing on individual pathspecs can be done using pathspec magic ":(glob)"

--icase-pathspecs
Add "icase" magic to all pathspec. This is equivalent to setting the GIT_ICASE_PATHSPECS environment variable to 1.

see glossary-content
pathspec
Pattern used to limit paths in Git commands.

Pathspecs are used on the command line of "git ls-files", "git ls-tree", "git add", "git grep", "git diff", "git checkout", and many other commands to limit the scope of operations to some subset of the tree or worktree. See the documentation of each command for whether paths are relative to the current directory or toplevel. The pathspec syntax is as follows:

any path matches itself

the pathspec up to the last slash represents a directory prefix. The scope of that pathspec is limited to that subtree.

the rest of the pathspec is a pattern for the remainder of the pathname. Paths relative to the directory prefix will be matched against that pattern using fnmatch(3); in particular, * and ? can match directory separators.

For example, Documentation/*.jpg will match all .jpg files in the Documentation subtree, including Documentation/chapter_1/figure_1.jpg.

A pathspec that begins with a colon : has special meaning. In the short form, the leading colon : is followed by zero or more "magic signature" letters (which optionally is terminated by another colon :), and the remainder is the pattern to match against the path. The "magic signature" consists of ASCII symbols that are neither alphanumeric, glob, regex special charaters nor colon. The optional colon that terminates the "magic signature" can be omitted if the pattern begins with a character that does not belong to "magic signature" symbol set and is not a colon.

In the long form, the leading colon : is followed by a open parenthesis (, a comma-separated list of zero or more "magic words", and a close parentheses ), and the remainder is the pattern to match against the path.

A pathspec with only a colon means "there is no pathspec". This form should not be combined with other pathspec.

top
The magic word top (magic signature: /) makes the pattern match from the root of the working tree, even when you are running the command from inside a subdirectory.

literal
Wildcards in the pattern such as * or ? are treated as literal characters.

icase
Case insensitive match.

glob
Git treats the pattern as a shell glob suitable for consumption by fnmatch(3) with the FNM_PATHNAME flag: wildcards in the pattern will not match a / in the pathname. For example, "Documentation/*.html" matches "Documentation/git.html" but not "Documentation/ppc/ppc.html" or "tools/perf/Documentation/perf.html".

Two consecutive asterisks ("**") in patterns matched against full pathname may have special meaning:

A leading "**" followed by a slash means match in all directories. For example, "**/foo" matches file or directory "foo" anywhere, the same as pattern "foo". "**/foo/bar" matches file or directory "bar" anywhere that is directly under directory "foo".

A trailing "/**" matches everything inside. For example, "abc/**" matches all files inside directory "abc", relative to the location of the .gitignore file, with infinite depth.

A slash followed by two consecutive asterisks then a slash matches zero or more directories. For example, "a/**/b" matches "a/b", "a/x/b", "a/x/y/b" and so on.

Other consecutive asterisks are considered invalid.

Glob magic is incompatible with literal magic.

exclude
After a path matches any non-exclude pathspec, it will be run through all exclude pathspec (magic signature: !). If it matches, the path is ignored.

Which characters to escape(\) *:\/ ??

rooting (/*) of a path (a) $GIT_DIR (b) system root.
directory (/) terminator (D/F conflict)

Compare with <path>; <paths>; and <file> 

[glob(7) patterns] (see grep)

` file specifies intentionally untracked files that
Git should ignore.
Files already tracked by Git are not affected; see the NOTES
below for details.

a `pathspec` is specified by a pattern.
When deciding whether a path matches a `pathspec` pattern, Git normally checks
with the following
order of precedence, from highest to lowest (within one level of
precedence, the last matching pattern decides the outcome):

 * Patterns read from the command line for those commands that support
   them.

 * Patterns read from a `.pathspec` file in the same directory
   as the path, or in any parent directory, with patterns in the
   higher level files (up to the toplevel of the work tree) being overridden
   by those in lower level files down to the directory containing the file.
   These patterns match relative to the location of the
   `.pathspec` file.  A project normally includes such
   `.pathspec` files in its repository, containing patterns for
   files generated as part of the project build.

 * Patterns read from `$GIT_DIR/info/exclude`.

 * Patterns read from the file specified by the configuration
   variable 'core.excludesfile'.

Which file to place a pattern in depends on how the pattern is meant to
be used.

 * Patterns which should be version-controlled and distributed to
   other repositories via clone (i.e., files that all developers will want
   to ignore) should go into a `.pathspec` file.

 * Patterns which are
   specific to a particular repository but which do not need to be shared
   with other related repositories (e.g., auxiliary files that live inside
   the repository but are specific to one user's workflow) should go into
   the `$GIT_DIR/info/exclude` file.

 * Patterns which a user wants Git to
   ignore in all situations (e.g., backup or temporary files generated by
   the user's editor of choice) generally go into a file specified by
   `core.excludesfile` in the user's `~/.gitconfig`. Its default value is
   $XDG_CONFIG_HOME/git/ignore. If $XDG_CONFIG_HOME is either not set or
   empty, $HOME/.config/git/ignore is used instead.

The underlying Git plumbing tools, such as
'git ls-files' and 'git read-tree', read
`pathspec` patterns specified by command-line options, or from
files specified by command-line options.  Higher-level Git
tools, such as 'git status' and 'git add',
use patterns from the sources specified above.

PATTERN FORMAT
--------------

 - A blank line matches no files, so it can serve as a separator
   for readability.

 - A line starting with # serves as a comment.
   Put a backslash ("`\`") in front of the first hash for patterns
   that begin with a hash.

 - An optional prefix "`!`" which negates the pattern; any
   matching file excluded by a previous pattern will become
   included again. It is not possible to re-include a file if a parent
   directory of that file is excluded. Git doesn't list excluded
   directories for performance reasons, so any patterns on contained
   files have no effect, no matter where they are defined.
   Put a backslash ("`\`") in front of the first "`!`" for patterns
   that begin with a literal "`!`", for example, "`\!important!.txt`".

 - If the pattern ends with a slash, it is removed for the
   purpose of the following description, but it would only find
   a match with a directory.  In other words, `foo/` will match a
   directory `foo` and paths underneath it, but will not match a
   regular file or a symbolic link `foo` (this is consistent
   with the way how pathspec works in general in Git).

 - If the pattern does not contain a slash '/', Git treats it as
   a shell glob pattern and checks for a match against the
   pathname relative to the location of the `.pathspec` file
   (relative to the toplevel of the work tree if not from a
   `.pathspec` file).

 - Otherwise, Git treats the pattern as a shell glob suitable
   for consumption by fnmatch(3) with the FNM_PATHNAME flag:
   wildcards in the pattern will not match a / in the pathname.
   For example, "Documentation/{asterisk}.html" matches
   "Documentation/git.html" but not "Documentation/ppc/ppc.html"
   or "tools/perf/Documentation/perf.html".

 - A leading slash matches the beginning of the pathname.
   For example, "/{asterisk}.c" matches "cat-file.c" but not
   "mozilla-sha1/sha1.c".

Two consecutive asterisks ("`**`") in patterns matched against
full pathname may have special meaning:

 - A leading "`**`" followed by a slash means match in all
   directories. For example, "`**/foo`" matches file or directory
   "`foo`" anywhere, the same as pattern "`foo`". "`**/foo/bar`"
   matches file or directory "`bar`" anywhere that is directly
   under directory "`foo`".

 - A trailing "`/**`" matches everything inside. For example,
   "`abc/**`" matches all files inside directory "`abc`", relative
   to the location of the `.pathspec` file, with infinite depth.

 - A slash followed by two consecutive asterisks then a slash
   matches zero or more directories. For example, "`a/**/b`"
   matches "`a/b`", "`a/x/b`", "`a/x/y/b`" and so on.

 - Other consecutive asterisks are considered invalid.

NOTES
-----

The purpose of pathspec files is to ensure that certain files
not tracked by Git remain untracked.

To ignore uncommitted changes in a file that is already tracked,
use 'git update-index {litdd}assume-unchanged'.

To stop tracking a file that is currently tracked, use
'git rm --cached'.

EXAMPLES
--------

--------------------------------------------------------------
    $ git status
    [...]
    # Untracked files:
    [...]
    #       Documentation/foo.html
    #       Documentation/pathspec.html
    #       file.o
    #       lib.a
    #       src/internal.o
    [...]
    $ cat .git/info/exclude
    # ignore objects and archives, anywhere in the tree.
    *.[oa]
    $ cat Documentation/.pathspec
    # ignore generated html files,
    *.html
    # except foo.html which is maintained by hand
    !foo.html
    $ git status
    [...]
    # Untracked files:
    [...]
    #       Documentation/foo.html
    [...]
--------------------------------------------------------------

Another example:

--------------------------------------------------------------
    $ cat .pathspec
    vmlinux*
    $ ls arch/foo/kernel/vm*
    arch/foo/kernel/vmlinux.lds.S
    $ echo '!/vmlinux*' >arch/foo/kernel/.pathspec
--------------------------------------------------------------

The second .pathspec prevents Git from ignoring
`arch/foo/kernel/vmlinux.lds.S`.

Example to exclude everything except a specific directory `foo/bar`
(note the `/*` - without the slash, the wildcard would also exclude
everything within `foo/bar`):

--------------------------------------------------------------
    $ cat .pathspec
    # exclude everything except directory foo/bar
    /*
    !/foo
    /foo/*
    !/foo/bar
--------------------------------------------------------------

SEE ALSO
--------
linkgit:git-rm[1],
linkgit:git-update-index[1],
linkgit:gitrepository-layout[5],
linkgit:git-check-ignore[1]

GIT
---
Part of the linkgit:git[1] suite