threads / rfc / 44915

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

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

## tl;dr

21 messages between Jan 19, 2017 and Jan 24, 2017.

replies: 20people: 7as markdown or json

Stefan Hajnoczi· Jan 19, 2017, 15:03 UTC · lore
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
Stefan Hajnoczi· Jan 19, 2017, 15:03 UTC · re: Stefan Hajnoczi · lore

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

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
Junio C Hamano· Jan 19, 2017, 18:54 UTC · re: Stefan Hajnoczi · lore

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

Stefan Hajnoczi <stefanha@redhat.com> writes:
Show 7 quoted lines
> 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.

Stefan Hajnoczi· Jan 20, 2017, 14:12 UTC · re: Junio C Hamano · lore

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

On Thu, Jan 19, 2017 at 10:54:02AM -0800, Junio C Hamano wrote:
Show 15 quoted lines
> 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?

Show 13 quoted lines
> 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!

Jeff King· Jan 20, 2017, 14:19 UTC · re: Stefan Hajnoczi · lore

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

On Fri, Jan 20, 2017 at 02:12:12PM +0000, Stefan Hajnoczi wrote:
Show 16 quoted lines
> 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
Junio C Hamano· Jan 20, 2017, 22:56 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 16 quoted lines
> 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.

Stefan Hajnoczi· Jan 23, 2017, 13:29 UTC · re: Junio C Hamano · lore

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

On Fri, Jan 20, 2017 at 02:56:26PM -0800, Junio C Hamano wrote:
Show 32 quoted lines
> 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
Phil Hord· Jan 24, 2017, 17:18 UTC · re: Stefan Hajnoczi · lore

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

On Tue, Jan 24, 2017 at 1:54 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
Show 11 quoted lines
> > 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
Brandon Williams· Jan 19, 2017, 18:29 UTC · re: Stefan Hajnoczi · lore

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

On 01/19, Stefan Hajnoczi wrote:
Show 34 quoted lines
> 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
Stefan Hajnoczi· Jan 20, 2017, 14:17 UTC · re: Brandon Williams · lore

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

On Thu, Jan 19, 2017 at 10:29:34AM -0800, Brandon Williams wrote:
Show 50 quoted lines
> 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
Stefan Hajnoczi· Jan 19, 2017, 15:03 UTC · re: Stefan Hajnoczi · lore

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

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
Junio C Hamano· Jan 19, 2017, 18:39 UTC · re: Stefan Hajnoczi · lore

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

Stefan Hajnoczi <stefanha@redhat.com> writes:
Show 14 quoted lines
> 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.

Show 23 quoted lines
>
> 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,
Stefan Hajnoczi· Jan 20, 2017, 13:56 UTC · re: Junio C Hamano · lore

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

On Thu, Jan 19, 2017 at 10:39:02AM -0800, Junio C Hamano wrote:
Show 20 quoted lines
> 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 () {
Show 5 quoted lines
> 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
Junio C Hamano· Jan 20, 2017, 18:16 UTC · re: Stefan Hajnoczi · lore

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

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.
Stefan Hajnoczi· Jan 23, 2017, 13:15 UTC · re: Junio C Hamano · lore

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

On Fri, Jan 20, 2017 at 10:16:31AM -0800, Junio C Hamano wrote:
Show 27 quoted lines
> 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.

Show 26 quoted lines
> 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.

Jakub Narębski· Jan 24, 2017, 19:07 UTC · re: Stefan Hajnoczi · lore

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

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:
Show 9 quoted lines
>> 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
Philip Oakley· Jan 24, 2017, 20:50 UTC · re: Jakub Narębski · lore

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

From: "Jakub Narębski" <jnareb@gmail.com>
Show 16 quoted lines
>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

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

Jeff King· Jan 19, 2017, 16:59 UTC · re: Stefan Hajnoczi · lore

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

On Thu, Jan 19, 2017 at 03:03:45PM +0000, Stefan Hajnoczi wrote:
Show 5 quoted lines
> 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
Brandon Williams· Jan 19, 2017, 18:26 UTC · re: Jeff King · lore

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

On 01/19, Jeff King wrote:
Show 9 quoted lines
> 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
Stefan Hajnoczi· Jan 20, 2017, 14:18 UTC · re: Jeff King · lore

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

On Thu, Jan 19, 2017 at 11:59:59AM -0500, Jeff King wrote:
Show 23 quoted lines
> 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.

Jeff King· Jan 20, 2017, 14:32 UTC · re: Stefan Hajnoczi · lore

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

On Fri, Jan 20, 2017 at 02:18:32PM +0000, Stefan Hajnoczi wrote:
Show 9 quoted lines
> > 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

← back to recent threads