[RFC 0/2] grep: make output consistent with revision syntax
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 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 --- 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
[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 --- 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
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: > Stefan Hajnoczi 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 signature.asc Description: PGP signature
Re: [RFC 2/2] grep: use '/' delimiter for paths
On Thu, Jan 19, 2017 at 10:54:02AM -0800, Junio C Hamano wrote: > Stefan Hajnoczi 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 : vs -- confusing: | : | -- --+--+- git grep | OK | OK --+--+- git show | OK | ignored --+--+- git log | no output| OK --+--+- Neither syntax always does what I expect. If git show -- honored then I could use that syntax consistently. Sorry for going on a tangent. Does it seem reasonable to handle 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! signature.asc Description: PGP signature
Re: [RFC 2/2] grep: use '/' delimiter for paths
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 > > --- > > 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 signature.asc Description: PGP signature
Re: [RFC 0/2] grep: make output consistent with revision syntax
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. signature.asc Description: PGP signature
[PATCH v2 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) Signed-off-by: Stefan Hajnoczi --- builtin/grep.c | 4 +++- t/t7810-grep.sh | 5 + 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/builtin/grep.c b/builtin/grep.c index 90a4f3d..7a7aab9 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -494,7 +494,9 @@ 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/ */ + char delim = obj->type == OBJ_COMMIT ? ':' : '/'; + strbuf_addch(&base, delim); } } init_tree_desc(&tree, data, size); diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh index e804a3f..8a58d5e 100755 --- a/t/t7810-grep.sh +++ b/t/t7810-grep.sh @@ -1445,6 +1445,11 @@ test_expect_success 'grep outputs valid : for HEAD:t/' ' test_cmp expected actual ' +test_expect_success 'grep outputs valid : for HEAD:t' ' + git grep vvv HEAD:t >actual && + test_cmp expected actual +' + cat >expected <
[PATCH v2 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: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 avoids emitting the unnecessary ':' delimiter if the name already ends with ':' or '/': $ git grep malloc v2.9.3: v2.9.3:t/test-lib.sh: setup_malloc_check () { $ git show v2.9.3:t/test-lib.sh (succeeds) Signed-off-by: Stefan Hajnoczi --- builtin/grep.c | 6 +- t/t7810-grep.sh | 21 + 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/builtin/grep.c b/builtin/grep.c index 8887b6a..90a4f3d 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -491,7 +491,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, diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh index de2405c..e804a3f 100755 --- a/t/t7810-grep.sh +++ b/t/t7810-grep.sh @@ -1435,4 +1435,25 @@ test_expect_success 'grep does not report i-t-a and assume unchanged with -L' ' test_cmp expected actual ' +cat >expected <: for HEAD:t/' ' + git grep vvv HEAD:t/ >actual && + test_cmp expected actual +' + +cat >expected <: for HEAD:' ' + git grep vvv HEAD: >actual && + test_cmp expected actual +' + test_done -- 2.9.3
[PATCH v2 0/2] grep: make output consistent with revision syntax
v2: * Use obj->type instead of re-parsing name for delimiter (Followed Brandon's suggestion but named the variable 'delim' since that name is used in other places and 'del' is used for deletion.) * Add tests * Update Patch 1 commit description with a more relevant example * PATCH instead of RFC, now works with all documented git-rev-parse(1) syntax 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. Stefan Hajnoczi (2): grep: only add delimiter if there isn't one already grep: use '/' delimiter for paths builtin/grep.c | 8 +++- t/t7810-grep.sh | 26 ++ 2 files changed, 33 insertions(+), 1 deletion(-) -- 2.9.3
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: > Stefan Hajnoczi 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. signature.asc Description: PGP signature
Re: [RFC 2/2] grep: use '/' delimiter for paths
On Fri, Jan 20, 2017 at 02:56:26PM -0800, Junio C Hamano wrote: > Jeff King 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 > :" actually is naming a blob that sits at the > in the . 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 :" > and "git show -- " 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 signature.asc Description: PGP signature
Re: [PATCH v2 2/2] grep: use '/' delimiter for paths
On Fri, Jan 20, 2017 at 03:51:33PM -0800, Brandon Williams wrote: > On 01/20, Junio C Hamano wrote: > > Stefan Hajnoczi 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' > > > > > > 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) > > > > > > Signed-off-by: Stefan Hajnoczi > > > --- > > > builtin/grep.c | 4 +++- > > > t/t7810-grep.sh | 5 + > > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > > > diff --git a/builtin/grep.c b/builtin/grep.c > > > index 90a4f3d..7a7aab9 100644 > > > --- a/builtin/grep.c > > > +++ b/builtin/grep.c > > > @@ -494,7 +494,9 @@ 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/ */ > > > + char delim = obj->type == OBJ_COMMIT ? ':' : > > > '/'; > > > > Why check the equality with commit, rather than un-equality with > > tree? Wouldn't you want to treat $commit:path and $tag:path the > > same way? > > I assume Stefan just grabbed my naive suggestion hence why it checks > equality with a commit. So that's my fault :) Either of these may > not be enough though, since if you do 'git grep malloc v2.9.3^{tree}' > with this change the output prefix is 'v2.9.3^{tree}/' instead of the > correct prefix 'v2.9.3^{tree}:' I revisited this series again today and am coming to the conclusion that forming output based on the user's rev is really hard to get right in all cases. I don't have a good solution to the v2.9.3^{tree} problem. Perhaps it's better to leave this than to merge code that doesn't work correctly 100% of the time. Stefan signature.asc Description: PGP signature
Re: [PATCH v2 2/2] grep: use '/' delimiter for paths
On Thu, Feb 09, 2017 at 12:20:34AM -0500, Jeff King wrote: > On Wed, Feb 08, 2017 at 09:14:17PM -0800, Junio C Hamano wrote: > > Jeff King writes: > (I _do_ think Stefan's proposed direction is worth it simply because the > result is easier to read, but I agree the whole thing can be avoided by > using pathspecs, as you've noted). I won't be pushing this series further due to limited time. Stefan signature.asc Description: PGP signature