{"thread":{"id":"50340","subject":"[PATCH/RFC] completion: complete refs in multiple steps","startedAt":"2019-01-28T09:42:07Z","lastAt":"2019-01-29T15:36:59Z","messageCount":7,"participants":["Nguyễn Thái Ngọc Duy","SZEDER Gábor","Jeff King","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"367818","messageId":"20190128094155.2424-1-pclouds@gmail.com","threadId":"50340","inReplyTo":null,"subject":"[PATCH/RFC] completion: complete refs in multiple steps","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-28T09:41:55Z","receivedAt":"2019-01-28T09:42:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This is in the same spirit of f22f682695 (completion: complete general\nconfig vars in two steps - 2018-05-27). Instead of considering all full\nrefs as completion candidates, it completes one \"path\" component at a\ntime, e.g.\n\n    $ git switch-branch -d j<TAB>\n    jch/            junio-gpg-pub\n\n    $ git switch-branch -d jch/<TAB>\n    Display all 154 possibilities? (y or n)\n    jch/ab/            jch/fc/\n    ....\n\n    $ git switch-branch -d jch/nd/<TAB>\n    jch/nd/attr-pathspec-fix\n    jch/nd/attr-pathspec-in-tree-walk\n    ...\n\nFor refs organized in multiple levels like this (and I've seen refs in 4\nlevels), especially when there a lot of refs, incremental completion\nthis way makes it easier to get to what you want.\n\nThe cost of course is more complicated completion and also slower on\nsystems with slow process creation. So maybe there will be a switch to\nturn this on or off?\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n contrib/completion/git-completion.bash | 13 ++++++++++++-\n 1 file changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 499e56f83d..d74ee79866 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -742,6 +742,17 @@ __git_refs ()\n \tesac\n }\n \n+__git_collapse_refs ()\n+{\n+\tlocal regex=\"$(echo \"$1\" | sed 's/[^/]\\+/[^\\/]*/g')\"\n+\tcase \"$regex\" in\n+\t\t'') regex='[^\\/]*';;\n+\t\t*/) regex=\"${regex}[^/]*\";;\n+\tesac\n+\tregex=\"$(echo \"$regex\" | sed 's/\\//\\\\\\//g')\"\n+\tsed -ne \"s/\\\\($regex\\\\/\\\\?\\\\).*/\\\\1/p\"\n+}\n+\n # Completes refs, short and long, local and remote, symbolic and pseudo.\n #\n # Usage: __git_complete_refs [<option>]...\n@@ -769,7 +780,7 @@ __git_complete_refs ()\n \t\tshift\n \tdone\n \n-\t__gitcomp_direct \"$(__git_refs \"$remote\" \"$track\" \"$pfx\" \"$cur_\" \"$sfx\")\"\n+\t__gitcomp_direct \"$(__git_refs \"$remote\" \"$track\" \"$pfx\" \"$cur_\" \"$sfx\" | __git_collapse_refs \"$cur_\")\"\n }\n \n # __git_refs2 requires 1 argument (to pass to __git_refs)\n-- \n2.20.1.560.g70ca8b83ee\n\n"},{"id":"367827","messageId":"20190128143828.GJ6702@szeder.dev","threadId":"50340","inReplyTo":"20190128094155.2424-1-pclouds@gmail.com","subject":"Re: [PATCH/RFC] completion: complete refs in multiple steps","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-01-28T14:38:28Z","receivedAt":"2019-01-28T14:38:35Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Jan 28, 2019 at 04:41:55PM +0700, Nguyễn Thái Ngọc Duy wrote:\n> This is in the same spirit of f22f682695 (completion: complete general\n> config vars in two steps - 2018-05-27). Instead of considering all full\n> refs as completion candidates, it completes one \"path\" component at a\n> time, e.g.\n> \n>     $ git switch-branch -d j<TAB>\n\n  $ git switch-branch\n  git: 'switch-branch' is not a git command. See 'git --help'.\n\nPlease use only existing Git commands in the examples.\n\n>     jch/            junio-gpg-pub\n> \n>     $ git switch-branch -d jch/<TAB>\n>     Display all 154 possibilities? (y or n)\n>     jch/ab/            jch/fc/\n>     ....\n> \n>     $ git switch-branch -d jch/nd/<TAB>\n>     jch/nd/attr-pathspec-fix\n>     jch/nd/attr-pathspec-in-tree-walk\n>     ...\n> \n> For refs organized in multiple levels like this (and I've seen refs in 4\n> levels), especially when there a lot of refs, incremental completion\n> this way makes it easier to get to what you want.\n> \n> The cost of course is more complicated completion and also slower on\n> systems with slow process creation. So maybe there will be a switch to\n> turn this on or off?\n\nOh, no.  After spending quite some effort to eliminate most of the\nprocesses and big shell loops from the ref completion code path, I\nfind this patch simply terrible ;)\n\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 499e56f83d..d74ee79866 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -742,6 +742,17 @@ __git_refs ()\n>  \tesac\n>  }\n>  \n> +__git_collapse_refs ()\n> +{\n> +\tlocal regex=\"$(echo \"$1\" | sed 's/[^/]\\+/[^\\/]*/g')\"\n\nSurely this could be done using only shell builtins.\n\n> +\tcase \"$regex\" in\n> +\t\t'') regex='[^\\/]*';;\n\nShould this really escape that '/'?  The 'sed' below would escape it\nanyway, so it would be escaped twice.  And the line below doesn't\nescape it.\n\n> +\t\t*/) regex=\"${regex}[^/]*\";;\n> +\tesac\n> +\tregex=\"$(echo \"$regex\" | sed 's/\\//\\\\\\//g')\"\n\nAnd this one, too.\n\n> +\tsed -ne \"s/\\\\($regex\\\\/\\\\?\\\\).*/\\\\1/p\"\n> +}\n> +\n>  # Completes refs, short and long, local and remote, symbolic and pseudo.\n>  #\n>  # Usage: __git_complete_refs [<option>]...\n> @@ -769,7 +780,7 @@ __git_complete_refs ()\n>  \t\tshift\n>  \tdone\n>  \n> -\t__gitcomp_direct \"$(__git_refs \"$remote\" \"$track\" \"$pfx\" \"$cur_\" \"$sfx\")\"\n> +\t__gitcomp_direct \"$(__git_refs \"$remote\" \"$track\" \"$pfx\" \"$cur_\" \"$sfx\" | __git_collapse_refs \"$cur_\")\"\n>  }\n\nIn general I think it would be much better to rely more on 'git\nfor-each-ref' to do the heavy lifting, extending it with new format\nspecifiers/options as necessary.\n\n'%(refname:rstrip=-<N>)' already comes somewhat close to what we would\nneed for full ref completion (i.e. 'refs/b<TAB>' to complete\n'refs/bisec/bad'), we only have to figure out how many \"ref path\ncomponents\" to show based on the number of path components in the\ncurrent word to be completed.  Alas, it won't add the trailing '/' for\n\"ref directories\".\n\nFor \"regular\" refs completion we would need to combine 'rstrip=-<N>'\nwith 'lstrip=2' to remove the 'refs/(heads|tags|remotes)/' prefix, but\nref-filter doesn't support things like that, it allows only one format\noption.  And, of course, the lack of trailing '/' is an issue in this\ncase as well.\n\nSo perhaps a new format option like '%(refname:path-components=3,2)'\nto show two ref path components starting with the third with a\ntrailing '/' appended if necessary, e.g. to turn\n'refs/remotes/jch/nd/attr-pathspec-fix' into 'jch/nd/'.\n\n\nNote that we also have __git_head() and __git_tags() to complete only\nbranches and only tags.\n\n"},{"id":"367842","messageId":"20190128172707.GA3050@sigill.intra.peff.net","threadId":"50340","inReplyTo":"20190128143828.GJ6702@szeder.dev","subject":"Re: [PATCH/RFC] completion: complete refs in multiple steps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-28T17:27:07Z","receivedAt":"2019-01-28T17:27:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 28, 2019 at 03:38:28PM +0100, SZEDER Gábor wrote:\n\n> > -\t__gitcomp_direct \"$(__git_refs \"$remote\" \"$track\" \"$pfx\" \"$cur_\" \"$sfx\")\"\n> > +\t__gitcomp_direct \"$(__git_refs \"$remote\" \"$track\" \"$pfx\" \"$cur_\" \"$sfx\" | __git_collapse_refs \"$cur_\")\"\n> >  }\n> \n> In general I think it would be much better to rely more on 'git\n> for-each-ref' to do the heavy lifting, extending it with new format\n> specifiers/options as necessary.\n\nFWIW, that was my first thought, too.\n\n> '%(refname:rstrip=-<N>)' already comes somewhat close to what we would\n> need for full ref completion (i.e. 'refs/b<TAB>' to complete\n> 'refs/bisec/bad'), we only have to figure out how many \"ref path\n> components\" to show based on the number of path components in the\n> current word to be completed.  Alas, it won't add the trailing '/' for\n> \"ref directories\".\n\nI think it also makes it hard to do one thing which (I think) people\nwould want: if there is a single deep ref, complete the whole thing.\nE.g., given:\n\n  $ git for-each-ref --refname='%(refname)'\n  refs/heads/foo/bar\n  refs/heads/foo/baz\n  refs/heads/another/deep/one\n\nwe'd ideally complete \"fo\" to \"foo/\" and \"ano\" to \"another/deep/one\",\nrather than making the user tab through each level.\n\nDoing that requires actually understanding that the refs are in a list,\nand not formatting each one independently. So I kind of wonder if it\nwould be easier to simply have a completion mode in ref-filter.\n\n-Peff\n"},{"id":"367843","messageId":"20190128172742.GA3571@sigill.intra.peff.net","threadId":"50340","inReplyTo":"20190128172707.GA3050@sigill.intra.peff.net","subject":"Re: [PATCH/RFC] completion: complete refs in multiple steps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-28T17:27:42Z","receivedAt":"2019-01-28T17:27:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 28, 2019 at 12:27:07PM -0500, Jeff King wrote:\n\n> Doing that requires actually understanding that the refs are in a list,\n> and not formatting each one independently. So I kind of wonder if it\n> would be easier to simply have a completion mode in ref-filter.\n\nEr, I meant \"a completion mode in for-each-ref\", though I think it\nlargely works out to be the same thing.\n\n-Peff\n"},{"id":"367906","messageId":"CACsJy8BXfY+bZKV6Jxa7-3VQyyE6hGrFS7ZAnToFwpGq_o1bpw@mail.gmail.com","threadId":"50340","inReplyTo":"20190128172707.GA3050@sigill.intra.peff.net","subject":"Re: [PATCH/RFC] completion: complete refs in multiple steps","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-29T00:43:45Z","receivedAt":"2019-01-29T00:44:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jan 29, 2019 at 12:27 AM Jeff King <peff@peff.net> wrote:\n>\n> On Mon, Jan 28, 2019 at 03:38:28PM +0100, SZEDER Gábor wrote:\n>\n> > > -   __gitcomp_direct \"$(__git_refs \"$remote\" \"$track\" \"$pfx\" \"$cur_\" \"$sfx\")\"\n> > > +   __gitcomp_direct \"$(__git_refs \"$remote\" \"$track\" \"$pfx\" \"$cur_\" \"$sfx\" | __git_collapse_refs \"$cur_\")\"\n> > >  }\n> >\n> > In general I think it would be much better to rely more on 'git\n> > for-each-ref' to do the heavy lifting, extending it with new format\n> > specifiers/options as necessary.\n>\n> FWIW, that was my first thought, too.\n\nI was more concerned whether it's a good idea to begin with. But it\nsounds like you two both think it's good otherwise would have\nobjected.\n\n> > '%(refname:rstrip=-<N>)' already comes somewhat close to what we would\n> > need for full ref completion (i.e. 'refs/b<TAB>' to complete\n> > 'refs/bisec/bad'), we only have to figure out how many \"ref path\n> > components\" to show based on the number of path components in the\n> > current word to be completed.  Alas, it won't add the trailing '/' for\n> > \"ref directories\".\n>\n> I think it also makes it hard to do one thing which (I think) people\n> would want: if there is a single deep ref, complete the whole thing.\n> E.g., given:\n>\n>   $ git for-each-ref --refname='%(refname)'\n>   refs/heads/foo/bar\n>   refs/heads/foo/baz\n>   refs/heads/another/deep/one\n>\n> we'd ideally complete \"fo\" to \"foo/\" and \"ano\" to \"another/deep/one\",\n> rather than making the user tab through each level.\n\nAh ha, like github sometimes show nested submodule paths. Just one\nsmall modification, when doing \"refs/heads/<tab>\" I would just show\n\nrefs/heads/foo/\nrefs/heads/another/\n\nnot refs/heads/another/deep/one to save space. But when you do\n\"refs/heads/a<tab>\" then you get \"refs/heads/another/deep/one\"\nimmediately.\n\n> Doing that requires actually understanding that the refs are in a list,\n> and not formatting each one independently. So I kind of wonder if it\n> would be easier to simply have a completion mode in for-each-mode.\n\nThat also allows more complicated logic. I think sometimes completion\ncode gets it wrong (I think it's often the case with rev/path\nambiguation, but maybe dwim stuff too). And we already have all this\nlogic in C.\n-- \nDuy\n"},{"id":"367907","messageId":"20190129004752.GA4327@sigill.intra.peff.net","threadId":"50340","inReplyTo":"CACsJy8BXfY+bZKV6Jxa7-3VQyyE6hGrFS7ZAnToFwpGq_o1bpw@mail.gmail.com","subject":"Re: [PATCH/RFC] completion: complete refs in multiple steps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-29T00:47:53Z","receivedAt":"2019-01-29T00:47:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 29, 2019 at 07:43:45AM +0700, Duy Nguyen wrote:\n\n> > > In general I think it would be much better to rely more on 'git\n> > > for-each-ref' to do the heavy lifting, extending it with new format\n> > > specifiers/options as necessary.\n> >\n> > FWIW, that was my first thought, too.\n> \n> I was more concerned whether it's a good idea to begin with. But it\n> sounds like you two both think it's good otherwise would have\n> objected.\n\nHeh. I do not have any real objection, but I don't think I'd really know\nuntil I used it for a while. It may be a matter of preference, in which\ncase we might want $GIT_COMPLETION_REF_COMPONENTS to enable/disable it\n(I don't have an opinion on what the default should be).\n\n> >   $ git for-each-ref --refname='%(refname)'\n> >   refs/heads/foo/bar\n> >   refs/heads/foo/baz\n> >   refs/heads/another/deep/one\n> >\n> > we'd ideally complete \"fo\" to \"foo/\" and \"ano\" to \"another/deep/one\",\n> > rather than making the user tab through each level.\n> \n> Ah ha, like github sometimes show nested submodule paths. Just one\n> small modification, when doing \"refs/heads/<tab>\" I would just show\n> \n> refs/heads/foo/\n> refs/heads/another/\n> \n> not refs/heads/another/deep/one to save space. But when you do\n> \"refs/heads/a<tab>\" then you get \"refs/heads/another/deep/one\"\n> immediately.\n\nYeah. It's still only one entry either way (by definition), but it's\ngoing to be much longer than all of the others. Again, I think I'd have\nto see it in practice to decide how much I cared either way.\n\n> > Doing that requires actually understanding that the refs are in a list,\n> > and not formatting each one independently. So I kind of wonder if it\n> > would be easier to simply have a completion mode in for-each-mode.\n> \n> That also allows more complicated logic. I think sometimes completion\n> code gets it wrong (I think it's often the case with rev/path\n> ambiguation, but maybe dwim stuff too). And we already have all this\n> logic in C.\n\nYeah, agreed.\n\n-Peff\n"},{"id":"368007","messageId":"20190129153650.GB13764@szeder.dev","threadId":"50340","inReplyTo":"CACsJy8BXfY+bZKV6Jxa7-3VQyyE6hGrFS7ZAnToFwpGq_o1bpw@mail.gmail.com","subject":"Re: [PATCH/RFC] completion: complete refs in multiple steps","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-01-29T15:36:50Z","receivedAt":"2019-01-29T15:36:59Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Jan 29, 2019 at 07:43:45AM +0700, Duy Nguyen wrote:\n> On Tue, Jan 29, 2019 at 12:27 AM Jeff King <peff@peff.net> wrote:\n\n> > > In general I think it would be much better to rely more on 'git\n> > > for-each-ref' to do the heavy lifting, extending it with new format\n> > > specifiers/options as necessary.\n> >\n> > FWIW, that was my first thought, too.\n> \n> I was more concerned whether it's a good idea to begin with. But it\n> sounds like you two both think it's good otherwise would have\n> objected.\n\nWell, I objected to this RFC implementation, but on purpose refrained\nfrom expressing an opinion about whether this is good or bad idea,\nbecause I didn't even want to try to guess what others might prefer :)\n\nAnd as far as I can tell it doesn't affect my usage (in general I\ndon't have multi-level hierarchies under refs/heads/, so I mostly\ncomplete a single ref path component, and when on occasion I do\ncomplete a full ref, then I tend to know what I want, and even if I\ndon't, I only need the list of possibilities only at the last ref path\ncomponent).\n\n> > > '%(refname:rstrip=-<N>)' already comes somewhat close to what we would\n> > > need for full ref completion (i.e. 'refs/b<TAB>' to complete\n> > > 'refs/bisec/bad'), we only have to figure out how many \"ref path\n> > > components\" to show based on the number of path components in the\n> > > current word to be completed.  Alas, it won't add the trailing '/' for\n> > > \"ref directories\".\n> >\n> > I think it also makes it hard to do one thing which (I think) people\n> > would want: if there is a single deep ref, complete the whole thing.\n> > E.g., given:\n> >\n> >   $ git for-each-ref --refname='%(refname)'\n> >   refs/heads/foo/bar\n> >   refs/heads/foo/baz\n> >   refs/heads/another/deep/one\n> >\n> > we'd ideally complete \"fo\" to \"foo/\" and \"ano\" to \"another/deep/one\",\n> > rather than making the user tab through each level.\n> \n> Ah ha, like github sometimes show nested submodule paths. Just one\n> small modification, when doing \"refs/heads/<tab>\" I would just show\n> \n> refs/heads/foo/\n> refs/heads/another/\n> \n> not refs/heads/another/deep/one to save space. But when you do\n> \"refs/heads/a<tab>\" then you get \"refs/heads/another/deep/one\"\n> immediately.\n\nBut this would be inconsistent with how \"regular\" path completion\nworks, e.g. if you have 'unique-dir/only-one-dir/only-one-file' and do\ne.g. 'cat u<TAB>', then Bash will only offer 'unique-dir/', not the\nwhole path to the file in the subdir.  Ok, I know, those are real\npaths not refs, they just look alike, because both happen to use '/'\nas separator.\n\nI can't asses whether people, in general, would prefer this behavior\nor would rather be surprised or bothered by the inconsistency.\n\n"}]}