{"thread":{"id":"44924","subject":"[PATCH v2 1/2] grep: only add delimiter if there isn't one already","startedAt":"2017-01-20T17:11:47Z","lastAt":"2017-02-14T13:38:10Z","messageCount":15,"participants":["Stefan Hajnoczi","Junio C Hamano","Brandon Williams","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"309841","messageId":"20170120171126.16269-2-stefanha@redhat.com","threadId":"44924","inReplyTo":"20170120171126.16269-1-stefanha@redhat.com","subject":"[PATCH v2 1/2] grep: only add delimiter if there isn't one already","fromName":"Stefan Hajnoczi","fromEmail":"stefanha@redhat.com","sentAt":"2017-01-20T17:11:25Z","receivedAt":"2017-01-20T17:11:47Z","isPatch":true,"sender":{"key":"stefanha@redhat.com","avatar":null},"body":"git-grep(1) output does not follow git's own syntax:\n\n $ git grep malloc v2.9.3:t/\n v2.9.3:t/:test-lib.sh: setup_malloc_check () {\n $ git show v2.9.3:t/:test-lib.sh\n fatal: Path 't/:test-lib.sh' does not exist in 'v2.9.3'\n\nThis patch avoids emitting the unnecessary ':' delimiter if the name\nalready ends with ':' or '/':\n\n $ git grep malloc v2.9.3:\n v2.9.3:t/test-lib.sh: setup_malloc_check () {\n $ git show v2.9.3:t/test-lib.sh\n (succeeds)\n\nSigned-off-by: Stefan Hajnoczi \n---\n builtin/grep.c | 6 +++++-\n t/t7810-grep.sh | 21 +++++++++++++++++++++\n 2 files changed, 26 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 8887b6a..90a4f3d 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -491,7 +491,11 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\tstrbuf_init(&base, PATH_MAX + len + 1);\n \t\tif (len) {\n \t\t\tstrbuf_add(&base, name, len);\n-\t\t\tstrbuf_addch(&base, ':');\n+\n+\t\t\t/* Add a delimiter if there isn't one already */\n+\t\t\tif (name[len - 1] != '/' && name[len - 1] != ':') {\n+\t\t\t\tstrbuf_addch(&base, ':');\n+\t\t\t}\n \t\t}\n \t\tinit_tree_desc(&tree, data, size);\n \t\thit = grep_tree(opt, pathspec, &tree, &base, base.len,\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex de2405c..e804a3f 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -1435,4 +1435,25 @@ test_expect_success 'grep does not report i-t-a and assume unchanged with -L' '\n \ttest_cmp expected actual\n '\n \n+cat >expected <: for HEAD:t/' '\n+\tgit grep vvv HEAD:t/ >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+cat >expected <: for HEAD:' '\n+\tgit grep vvv HEAD: >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.9.3\n\n"},{"id":"309842","messageId":"20170120171126.16269-3-stefanha@redhat.com","threadId":"44924","inReplyTo":"20170120171126.16269-1-stefanha@redhat.com","subject":"[PATCH v2 2/2] grep: use '/' delimiter for paths","fromName":"Stefan Hajnoczi","fromEmail":"stefanha@redhat.com","sentAt":"2017-01-20T17:11:26Z","receivedAt":"2017-01-20T17:11:50Z","isPatch":true,"sender":{"key":"stefanha@redhat.com","avatar":null},"body":"If the tree contains a sub-directory then git-grep(1) output contains a\ncolon character instead of a path separator:\n\n $ git grep malloc v2.9.3:t\n v2.9.3:t:test-lib.sh:\tsetup_malloc_check () {\n $ git show v2.9.3:t:test-lib.sh\n fatal: Path 't:test-lib.sh' does not exist in 'v2.9.3'\n\nThis patch attempts to use the correct delimiter:\n\n $ git grep malloc v2.9.3:t\n v2.9.3:t/test-lib.sh:\tsetup_malloc_check () {\n $ git show v2.9.3:t/test-lib.sh\n (success)\n\nSigned-off-by: Stefan Hajnoczi \n---\n builtin/grep.c | 4 +++-\n t/t7810-grep.sh | 5 +++++\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 90a4f3d..7a7aab9 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -494,7 +494,9 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,\n \n \t\t\t/* Add a delimiter if there isn't one already */\n \t\t\tif (name[len - 1] != '/' && name[len - 1] != ':') {\n-\t\t\t\tstrbuf_addch(&base, ':');\n+\t\t\t\t/* rev: or rev:path/ */\n+\t\t\t\tchar delim = obj->type == OBJ_COMMIT ? ':' : '/';\n+\t\t\t\tstrbuf_addch(&base, delim);\n \t\t\t}\n \t\t}\n \t\tinit_tree_desc(&tree, data, size);\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex e804a3f..8a58d5e 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -1445,6 +1445,11 @@ test_expect_success 'grep outputs valid : for HEAD:t/' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'grep outputs valid : for HEAD:t' '\n+\tgit grep vvv HEAD:t >actual &&\n+\ttest_cmp expected actual\n+'\n+\n cat >expected <type instead of re-parsing name for delimiter\n (Followed Brandon's suggestion but named the variable 'delim' since that\n name is used in other places and 'del' is used for deletion.)\n * Add tests\n * Update Patch 1 commit description with a more relevant example\n * PATCH instead of RFC, now works with all documented git-rev-parse(1) syntax\n\ngit-grep(1)'s output is not consistent with git-rev-parse(1) revision syntax.\n\nThis means you cannot take \"rev:path/to/file.c: foo();\" output from git-grep(1)\nand expect \"git show rev:path/to/file.c\" to work. See the individual patches\nfor examples of command-lines that produce invalid output.\n\nStefan Hajnoczi (2):\n grep: only add delimiter if there isn't one already\n grep: use '/' delimiter for paths\n\n builtin/grep.c | 8 +++++++-\n t/t7810-grep.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 1 deletion(-)\n\n-- \n2.9.3\n\n"},{"id":"309863","messageId":"xmqqpojhwf2r.fsf@gitster.mtv.corp.google.com","threadId":"44924","inReplyTo":"20170120171126.16269-3-stefanha@redhat.com","subject":"Re: [PATCH v2 2/2] grep: use '/' delimiter for paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-20T22:18:20Z","receivedAt":"2017-01-20T22:18:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Hajnoczi writes:\n\n> If the tree contains a sub-directory then git-grep(1) output contains a\n> colon character instead of a path separator:\n>\n> $ git grep malloc v2.9.3:t\n> v2.9.3:t:test-lib.sh:\tsetup_malloc_check () {\n> $ git show v2.9.3:t:test-lib.sh\n> fatal: Path 't:test-lib.sh' does not exist in 'v2.9.3'\n>\n> This patch attempts to use the correct delimiter:\n>\n> $ git grep malloc v2.9.3:t\n> v2.9.3:t/test-lib.sh:\tsetup_malloc_check () {\n> $ git show v2.9.3:t/test-lib.sh\n> (success)\n>\n> Signed-off-by: Stefan Hajnoczi \n> ---\n> builtin/grep.c | 4 +++-\n> t/t7810-grep.sh | 5 +++++\n> 2 files changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 90a4f3d..7a7aab9 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -494,7 +494,9 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,\n> \n> \t\t\t/* Add a delimiter if there isn't one already */\n> \t\t\tif (name[len - 1] != '/' && name[len - 1] != ':') {\n> -\t\t\t\tstrbuf_addch(&base, ':');\n> +\t\t\t\t/* rev: or rev:path/ */\n> +\t\t\t\tchar delim = obj->type == OBJ_COMMIT ? ':' : '/';\n\nWhy check the equality with commit, rather than un-equality with\ntree? Wouldn't you want to treat $commit:path and $tag:path the\nsame way?\n\nI also think the two patches should be squashed together, as it is\nonly after this patch that the code does the \"right thing\" by\nchanging the delimiter depending on obj->type.\n\n> +\t\t\t\tstrbuf_addch(&base, delim);\n> \t\t\t}\n> \t\t}\n> \t\tinit_tree_desc(&tree, data, size);\n> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\n> index e804a3f..8a58d5e 100755\n> --- a/t/t7810-grep.sh\n> +++ b/t/t7810-grep.sh\n> @@ -1445,6 +1445,11 @@ test_expect_success 'grep outputs valid : for HEAD:t/' '\n> \ttest_cmp expected actual\n> '\n> \n> +test_expect_success 'grep outputs valid : for HEAD:t' '\n> +\tgit grep vvv HEAD:t >actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n\nPerhaps you want an annotated tag object refs/tags/v1.0 that points\nat the commit at the HEAD, and then run the same grep on v1.0:t, in\naddition to the above test.\n\n> cat >expected < HEAD:t/a/v:vvv\n> HEAD:t/v:vvv\n"},{"id":"309864","messageId":"xmqqinp9wf1c.fsf@gitster.mtv.corp.google.com","threadId":"44924","inReplyTo":"20170120171126.16269-2-stefanha@redhat.com","subject":"Re: [PATCH v2 1/2] grep: only add delimiter if there isn't one already","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-20T22:19:11Z","receivedAt":"2017-01-20T22:19:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Hajnoczi writes:\n\n> git-grep(1) output does not follow git's own syntax:\n>\n> $ git grep malloc v2.9.3:t/\n> v2.9.3:t/:test-lib.sh: setup_malloc_check () {\n> $ git show v2.9.3:t/:test-lib.sh\n> fatal: Path 't/:test-lib.sh' does not exist in 'v2.9.3'\n>\n> This patch avoids emitting the unnecessary ':' delimiter if the name\n> already ends with ':' or '/':\n>\n> $ git grep malloc v2.9.3:\n> v2.9.3:t/test-lib.sh: setup_malloc_check () {\n> $ git show v2.9.3:t/test-lib.sh\n> (succeeds)\n\n>\n> Signed-off-by: Stefan Hajnoczi \n> ---\n> builtin/grep.c | 6 +++++-\n> t/t7810-grep.sh | 21 +++++++++++++++++++++\n> 2 files changed, 26 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 8887b6a..90a4f3d 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -491,7 +491,11 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,\n> \t\tstrbuf_init(&base, PATH_MAX + len + 1);\n> \t\tif (len) {\n> \t\t\tstrbuf_add(&base, name, len);\n> -\t\t\tstrbuf_addch(&base, ':');\n> +\n> +\t\t\t/* Add a delimiter if there isn't one already */\n> +\t\t\tif (name[len - 1] != '/' && name[len - 1] != ':') {\n> +\t\t\t\tstrbuf_addch(&base, ':');\n> +\t\t\t}\n\nI agree with peff and Brandon that checking treeness is the right\nway to make the decision, and you seem to have done that in the\nupdated 2/2, but why doesn't it apply here as well?\n\n\n> \t\t}\n> \t\tinit_tree_desc(&tree, data, size);\n> \t\thit = grep_tree(opt, pathspec, &tree, &base, base.len,\n"},{"id":"309871","messageId":"xmqq1svxwclj.fsf@gitster.mtv.corp.google.com","threadId":"44924","inReplyTo":"20170120171126.16269-1-stefanha@redhat.com","subject":"Re: [PATCH v2 0/2] grep: make output consistent with revision syntax","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-20T23:11:52Z","receivedAt":"2017-01-20T23:16:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Hajnoczi writes:\n\n> v2:\n> * Use obj->type instead of re-parsing name for delimiter\n> (Followed Brandon's suggestion but named the variable 'delim' since that\n> name is used in other places and 'del' is used for deletion.)\n> * Add tests\n> * Update Patch 1 commit description with a more relevant example\n> * PATCH instead of RFC, now works with all documented git-rev-parse(1) syntax\n>\n> git-grep(1)'s output is not consistent with git-rev-parse(1) revision syntax.\n\nWhile I was queuing this series (which I think should become a\nsingle patch in the final version), I was trying to see how it\nshould be described in the release note (aka an entry in the\nperiodicall \"What's cooking\" report). Here is how I explained it.\nYou may want to borrow some parts of the description, especially the\npart that talks about : being a way to name a blob,\nwhen updating the commit log message.\n\n \"git grep\", when fed a tree-ish as an input, shows each hit\n prefixed with \":::\". As is\n almost always either a commit or a tag that points at a commit,\n the early part of the output \":\" can be used as\n the name of the blob and given to \"git show\". \n\n When is a tree given in the extended SHA-1 syntax\n (e.g. \":\", or \":\"), however, this results\n in a string that does not name a blob (e.g. \"::\"\n or \"::\"). \"git grep\" has been taught to be\n a bit more intelligent about these cases and omit a colon (in\n the former case) or use slash (in the latter case) to produce\n \":\" and \":/\" that can be used\n as the name of a blob.\n\nAs a release note entry is written in a style different from the\ncommit log message, you would need to adjust the voice of the last\nsentence (i.e. \"Teach 'git grep' to ...\" to give an order to the\ncodebase), but otherwise the above would make an understandable\njustification for the change suitable in a log message.\n\n"},{"id":"309875","messageId":"20170120235133.GA146274@google.com","threadId":"44924","inReplyTo":"xmqqpojhwf2r.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] grep: use '/' delimiter for paths","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-01-20T23:51:33Z","receivedAt":"2017-01-20T23:52:28Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 01/20, Junio C Hamano wrote:\n> Stefan Hajnoczi writes:\n> \n> > If the tree contains a sub-directory then git-grep(1) output contains a\n> > colon character instead of a path separator:\n> >\n> > $ git grep malloc v2.9.3:t\n> > v2.9.3:t:test-lib.sh:\tsetup_malloc_check () {\n> > $ git show v2.9.3:t:test-lib.sh\n> > fatal: Path 't:test-lib.sh' does not exist in 'v2.9.3'\n> >\n> > This patch attempts to use the correct delimiter:\n> >\n> > $ git grep malloc v2.9.3:t\n> > v2.9.3:t/test-lib.sh:\tsetup_malloc_check () {\n> > $ git show v2.9.3:t/test-lib.sh\n> > (success)\n> >\n> > Signed-off-by: Stefan Hajnoczi \n> > ---\n> > builtin/grep.c | 4 +++-\n> > t/t7810-grep.sh | 5 +++++\n> > 2 files changed, 8 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/builtin/grep.c b/builtin/grep.c\n> > index 90a4f3d..7a7aab9 100644\n> > --- a/builtin/grep.c\n> > +++ b/builtin/grep.c\n> > @@ -494,7 +494,9 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,\n> > \n> > \t\t\t/* Add a delimiter if there isn't one already */\n> > \t\t\tif (name[len - 1] != '/' && name[len - 1] != ':') {\n> > -\t\t\t\tstrbuf_addch(&base, ':');\n> > +\t\t\t\t/* rev: or rev:path/ */\n> > +\t\t\t\tchar delim = obj->type == OBJ_COMMIT ? ':' : '/';\n> \n> Why check the equality with commit, rather than un-equality with\n> tree? Wouldn't you want to treat $commit:path and $tag:path the\n> same way?\n\nI assume Stefan just grabbed my naive suggestion hence why it checks\nequality with a commit. So that's my fault :) Either of these may\nnot be enough though, since if you do 'git grep malloc v2.9.3^{tree}'\nwith this change the output prefix is 'v2.9.3^{tree}/' instead of the\ncorrect prefix 'v2.9.3^{tree}:'\n\n> \n> I also think the two patches should be squashed together, as it is\n> only after this patch that the code does the \"right thing\" by\n> changing the delimiter depending on obj->type.\n> \n> > +\t\t\t\tstrbuf_addch(&base, delim);\n> > \t\t\t}\n> > \t\t}\n> > \t\tinit_tree_desc(&tree, data, size);\n> > diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\n> > index e804a3f..8a58d5e 100755\n> > --- a/t/t7810-grep.sh\n> > +++ b/t/t7810-grep.sh\n> > @@ -1445,6 +1445,11 @@ test_expect_success 'grep outputs valid : for HEAD:t/' '\n> > \ttest_cmp expected actual\n> > '\n> > \n> > +test_expect_success 'grep outputs valid : for HEAD:t' '\n> > +\tgit grep vvv HEAD:t >actual &&\n> > +\ttest_cmp expected actual\n> > +'\n> > +\n> \n> Perhaps you want an annotated tag object refs/tags/v1.0 that points\n> at the commit at the HEAD, and then run the same grep on v1.0:t, in\n> addition to the above test.\n> \n> > cat >expected < > HEAD:t/a/v:vvv\n> > HEAD:t/v:vvv\n\n-- \nBrandon Williams\n"},{"id":"310987","messageId":"20170207150414.GD8583@stefanha-x1.localdomain","threadId":"44924","inReplyTo":"20170120235133.GA146274@google.com","subject":"Re: [PATCH v2 2/2] grep: use '/' delimiter for paths","fromName":"Stefan Hajnoczi","fromEmail":"stefanha@redhat.com","sentAt":"2017-02-07T15:04:14Z","receivedAt":"2017-02-07T15:04:32Z","isPatch":true,"sender":{"key":"stefanha@redhat.com","avatar":null},"body":"On Fri, Jan 20, 2017 at 03:51:33PM -0800, Brandon Williams wrote:\n> On 01/20, Junio C Hamano wrote:\n> > Stefan Hajnoczi writes:\n> > \n> > > If the tree contains a sub-directory then git-grep(1) output contains a\n> > > colon character instead of a path separator:\n> > >\n> > > $ git grep malloc v2.9.3:t\n> > > v2.9.3:t:test-lib.sh:\tsetup_malloc_check () {\n> > > $ git show v2.9.3:t:test-lib.sh\n> > > fatal: Path 't:test-lib.sh' does not exist in 'v2.9.3'\n> > >\n> > > This patch attempts to use the correct delimiter:\n> > >\n> > > $ git grep malloc v2.9.3:t\n> > > v2.9.3:t/test-lib.sh:\tsetup_malloc_check () {\n> > > $ git show v2.9.3:t/test-lib.sh\n> > > (success)\n> > >\n> > > Signed-off-by: Stefan Hajnoczi \n> > > ---\n> > > builtin/grep.c | 4 +++-\n> > > t/t7810-grep.sh | 5 +++++\n> > > 2 files changed, 8 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/builtin/grep.c b/builtin/grep.c\n> > > index 90a4f3d..7a7aab9 100644\n> > > --- a/builtin/grep.c\n> > > +++ b/builtin/grep.c\n> > > @@ -494,7 +494,9 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,\n> > > \n> > > \t\t\t/* Add a delimiter if there isn't one already */\n> > > \t\t\tif (name[len - 1] != '/' && name[len - 1] != ':') {\n> > > -\t\t\t\tstrbuf_addch(&base, ':');\n> > > +\t\t\t\t/* rev: or rev:path/ */\n> > > +\t\t\t\tchar delim = obj->type == OBJ_COMMIT ? ':' : '/';\n> > \n> > Why check the equality with commit, rather than un-equality with\n> > tree? Wouldn't you want to treat $commit:path and $tag:path the\n> > same way?\n> \n> I assume Stefan just grabbed my naive suggestion hence why it checks\n> equality with a commit. So that's my fault :) Either of these may\n> not be enough though, since if you do 'git grep malloc v2.9.3^{tree}'\n> with this change the output prefix is 'v2.9.3^{tree}/' instead of the\n> correct prefix 'v2.9.3^{tree}:'\n\nI revisited this series again today and am coming to the conclusion that\nforming output based on the user's rev is really hard to get right in\nall cases. I don't have a good solution to the v2.9.3^{tree} problem.\n\nPerhaps it's better to leave this than to merge code that doesn't work\ncorrectly 100% of the time.\n\nStefan\n"},{"id":"311000","messageId":"20170207195011.2yvq6hutssl4htol@sigill.intra.peff.net","threadId":"44924","inReplyTo":"20170207150414.GD8583@stefanha-x1.localdomain","subject":"Re: [PATCH v2 2/2] grep: use '/' delimiter for paths","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-07T19:50:12Z","receivedAt":"2017-02-07T19:50:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 07, 2017 at 03:04:14PM +0000, Stefan Hajnoczi wrote:\n\n> > I assume Stefan just grabbed my naive suggestion hence why it checks\n> > equality with a commit. So that's my fault :) Either of these may\n> > not be enough though, since if you do 'git grep malloc v2.9.3^{tree}'\n> > with this change the output prefix is 'v2.9.3^{tree}/' instead of the\n> > correct prefix 'v2.9.3^{tree}:'\n> \n> I revisited this series again today and am coming to the conclusion that\n> forming output based on the user's rev is really hard to get right in\n> all cases. I don't have a good solution to the v2.9.3^{tree} problem.\n\nI think the rule you need is not \"are we at a tree\", but rather \"did we\ntraverse a path while resolving the name?\". Only the get_sha1() parser\ncan tell you that. I think:\n\n char delim = ':';\n struct object_context oc;\n if (get_sha1_with_context(name, 0, sha1, &oc))\n die(\"...\");\n if (oc.path[0])\n delim = '/'; /* name had a partial path */\n\nwould work. Root trees via \"v2.9.3^{tree}\" or \"v2.9.3:\" would have no\npath, but \"v2.9.3:Documentation\" would. I think you'd still need to\navoid duplicating a trailing delimiter, but I can't think of a case\nwhere it is wrong to do that based purely on the name.\n\n-Peff\n"},{"id":"311004","messageId":"xmqq8tphzr41.fsf@gitster.mtv.corp.google.com","threadId":"44924","inReplyTo":"20170207150414.GD8583@stefanha-x1.localdomain","subject":"Re: [PATCH v2 2/2] grep: use '/' delimiter for paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-07T20:24:30Z","receivedAt":"2017-02-07T20:24:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Hajnoczi writes:\n\n> Perhaps it's better to leave this than to merge code that doesn't work\n> correctly 100% of the time.\n\nI am not sure if you are shooting for is \"work correctly\" to begin\nwith, to be honest. The current code always shows the \"correct\"\noutput which is \"the tree-ish object name (expressed in a way easier\nto understand by the humans), followed by a colon, followed by the\npath in the tree-ish the hit lies\". You are making it \"incorrect\nbut often more convenient\", and sometimes that is a worth goal, but\nfor the particular use cases you presented, i.e.\n\n $ git grep -e \"$pattern\" \"$commit:path\"\n\na more natural way to express \"I want to find this pattern in the\ncommit under that path\" exists:\n\n $ git grep -e \"$pattern\" \"$commit\" -- path\n\nand because of that, I do not think the former form of the query\nshould happen _less_ often in the first place, which would make it\n\"incorrect but more convenient if the user gives an unusual query\".\n\nSo I am not sure if the change to \"grep\" is worth it.\n\nHaving said that, I actually think \"make it more convenient\" without\nmaking anything incorrect would be to teach the revision parser to\nunderstand\n\n \n\nas an extended SHA-1 expression to name the blob or the tree at that\npath in the tree-ish, e.g. if we can make the revision parser to\ntake this\n\n master:Documentation:git.txt\n\nas the name of the blob object, then the current output is both\ncorrect and more convenient. After all, this sample string starts\nat \"master:Documentation\" (which is an extended SHA-1 expression to\nname a tree-ish), followed by a colon, then followed by the path\n\"git.txt\" in it, and \"grep -e pattern master:Documentation\" would\nshow hits in that blob prefixed with it.\n\nI.e.\n\n\tT=$(git rev-parse master:Documentation)\n\tgit cat-file blob $T:git.txt\n\nwould give you the contents of the source to the Git manual. It is\nnot all that unreasonable to expect\n\n\tgit cat-file blob master:Documentation:git.txt\n\nto be able to show the same thing as well. You'd need to backtrack\nthe parsing (e.g. attempt to find \"Documentation:git.txt\" in\n\"master\", fail to find any, then fall back to find \"git.txt\" in\n\"master:Documentation\", find one, and be happy, or something like\nthat), and define how to resolve potential ambiguity (e.g. there may\nindeed be \"Documentation:git.txt\" and \"Documentation/git.txt\" in the\ntree-ish \"master\"), though.\n\n\n"},{"id":"311005","messageId":"xmqq4m05zqht.fsf@gitster.mtv.corp.google.com","threadId":"44924","inReplyTo":"xmqq8tphzr41.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] grep: use '/' delimiter for paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-07T20:37:50Z","receivedAt":"2017-02-07T20:38:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\nSorry, one shouldn't type while being sick and in bed X-<.\n\n> I am not sure if you are shooting for is \"work correctly\" to begin\n> with, to be honest. The current code always shows the \"correct\"\n> output which is \"the tree-ish object name (expressed in a way easier\n> to understand by the humans), followed by a colon, followed by the\n> path in the tree-ish the hit lies\". You are making it \"incorrect\n> but often more convenient\", and sometimes that is a worth goal, but\n\ns/worth/&y/;\n\n> for the particular use cases you presented, i.e.\n>\n> $ git grep -e \"$pattern\" \"$commit:path\"\n>\n> a more natural way to express \"I want to find this pattern in the\n> commit under that path\" exists:\n>\n> $ git grep -e \"$pattern\" \"$commit\" -- path\n>\n> and because of that, I do not think the former form of the query\n\ns/do not think/do think/\n\n> should happen _less_ often in the first place, which would make it\n> \"incorrect but more convenient if the user gives an unusual query\".\n>\n> So I am not sure if the change to \"grep\" is worth it.\n\nAlso, it may be fairer to do s/incorrect/inconsistent/.\n"},{"id":"311141","messageId":"20170209035839.wqsh6ibgnmxyjusi@sigill.intra.peff.net","threadId":"44924","inReplyTo":"xmqq8tphzr41.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] grep: use '/' delimiter for paths","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-09T03:58:39Z","receivedAt":"2017-02-09T03:59:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 07, 2017 at 12:24:30PM -0800, Junio C Hamano wrote:\n\n> Having said that, I actually think \"make it more convenient\" without\n> making anything incorrect would be to teach the revision parser to\n> understand\n> \n> \n> \n> as an extended SHA-1 expression to name the blob or the tree at that\n> path in the tree-ish, e.g. if we can make the revision parser to\n> take this\n> \n> master:Documentation:git.txt\n\nSo here I was wondering what happens when you have a filename with a\ncolon in it, but then...\n\n> to be able to show the same thing as well. You'd need to backtrack\n> the parsing (e.g. attempt to find \"Documentation:git.txt\" in\n> \"master\", fail to find any, then fall back to find \"git.txt\" in\n> \"master:Documentation\", find one, and be happy, or something like\n> that), and define how to resolve potential ambiguity (e.g. there may\n> indeed be \"Documentation:git.txt\" and \"Documentation/git.txt\" in the\n> tree-ish \"master\"), though.\n\n...you obviously did think of that. Backtracking sounds pretty nasty,\nthough. What's the time complexity of parsing:\n\n master:a:a:a:a:a:a:a:a:a:a:a\n\nI think there are 2^(n-1) possible paths (each colon can be a real colon\nor a slash). Though I guess if you walk the trees as you go, you only\nhave to examine at most \"n\" paths to find the first-level tree, and then\nat most \"n-1\" paths at the second level, and so on.\n\nUnless you really do have ambiguous trees, in which case you have to\nwalk down multiple paths.\n\nIt certainly would not be the first combinatoric explosion you can\nconvince Git to perform. But it does seem like a lot of complication for\nsomething as simple as path lookups.\n\n-Peff\n"},{"id":"311147","messageId":"xmqqtw84rlna.fsf@gitster.mtv.corp.google.com","threadId":"44924","inReplyTo":"20170209035839.wqsh6ibgnmxyjusi@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] grep: use '/' delimiter for paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-09T05:14:17Z","receivedAt":"2017-02-09T05:14:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> master:a:a:a:a:a:a:a:a:a:a:a\n>\n> I think there are 2^(n-1) possible paths (each colon can be a real colon\n> or a slash). Though I guess if you walk the trees as you go, you only\n> have to examine at most \"n\" paths to find the first-level tree, and then\n> at most \"n-1\" paths at the second level, and so on.\n>\n> Unless you really do have ambiguous trees, in which case you have to\n> walk down multiple paths.\n>\n> It certainly would not be the first combinatoric explosion you can\n> convince Git to perform. But it does seem like a lot of complication for\n> something as simple as path lookups.\n\nThat is true, and we may want to avoid the implementation complexity\nof the backtracking name resolution. If you are on the other hand\nworried about the runtime cost, it will be an issue to begin with\nonly for those who do \"git grep -e pattern HEAD:t/perf\", which is an\nunnatural way to do \"git grep -e pattern HEAD -- t/perf\", and the\noutput from the latter won't have such an issue, so...\n\n\n"},{"id":"311149","messageId":"20170209052034.ksoupjcj4qs7x4hz@sigill.intra.peff.net","threadId":"44924","inReplyTo":"xmqqtw84rlna.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] grep: use '/' delimiter for paths","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-09T05:20:34Z","receivedAt":"2017-02-09T05:27:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 08, 2017 at 09:14:17PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > master:a:a:a:a:a:a:a:a:a:a:a\n> >\n> > I think there are 2^(n-1) possible paths (each colon can be a real colon\n> > or a slash). Though I guess if you walk the trees as you go, you only\n> > have to examine at most \"n\" paths to find the first-level tree, and then\n> > at most \"n-1\" paths at the second level, and so on.\n> >\n> > Unless you really do have ambiguous trees, in which case you have to\n> > walk down multiple paths.\n> >\n> > It certainly would not be the first combinatoric explosion you can\n> > convince Git to perform. But it does seem like a lot of complication for\n> > something as simple as path lookups.\n> \n> That is true, and we may want to avoid the implementation complexity\n> of the backtracking name resolution. If you are on the other hand\n> worried about the runtime cost, it will be an issue to begin with\n> only for those who do \"git grep -e pattern HEAD:t/perf\", which is an\n> unnatural way to do \"git grep -e pattern HEAD -- t/perf\", and the\n> output from the latter won't have such an issue, so...\n\nI thought your point was to move it into the get_sha1() parser (so that\nwhile the form is only generated by \"git grep\", it can be accepted by\nany git command). That exposes it in a lot of places, including ones\nwhich are network accessible to things like gitweb (or GitHub, of\ncourse, which is my concern).\n\nEven without the runtime cost, though, I think the general complexity\nmakes it an ugly path to go down (e.g., handling ambiguous cases). I\nwouldn't want to have to write the documentation for it. :)\n\n(I _do_ think Stefan's proposed direction is worth it simply because the\nresult is easier to read, but I agree the whole thing can be avoided by\nusing pathspecs, as you've noted).\n\n-Peff\n"},{"id":"311540","messageId":"20170214094320.GA23310@stefanha-x1.localdomain","threadId":"44924","inReplyTo":"20170209052034.ksoupjcj4qs7x4hz@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] grep: use '/' delimiter for paths","fromName":"Stefan Hajnoczi","fromEmail":"stefanha@redhat.com","sentAt":"2017-02-14T09:43:20Z","receivedAt":"2017-02-14T13:38:10Z","isPatch":true,"sender":{"key":"stefanha@redhat.com","avatar":null},"body":"On Thu, Feb 09, 2017 at 12:20:34AM -0500, Jeff King wrote:\n> On Wed, Feb 08, 2017 at 09:14:17PM -0800, Junio C Hamano wrote:\n> > Jeff King writes:\n> (I _do_ think Stefan's proposed direction is worth it simply because the\n> result is easier to read, but I agree the whole thing can be avoided by\n> using pathspecs, as you've noted).\n\nI won't be pushing this series further due to limited time.\n\nStefan\n"}]}