[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
