Message ID | 20190128094155.2424-1-pclouds@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [PATCH/RFC] completion: complete refs in multiple steps | expand |
On Mon, Jan 28, 2019 at 04:41:55PM +0700, Nguyễn Thái Ngọc Duy wrote: > This is in the same spirit of f22f682695 (completion: complete general > config vars in two steps - 2018-05-27). Instead of considering all full > refs as completion candidates, it completes one "path" component at a > time, e.g. > > $ git switch-branch -d j<TAB> $ git switch-branch git: 'switch-branch' is not a git command. See 'git --help'. Please use only existing Git commands in the examples. > jch/ junio-gpg-pub > > $ git switch-branch -d jch/<TAB> > Display all 154 possibilities? (y or n) > jch/ab/ jch/fc/ > .... > > $ git switch-branch -d jch/nd/<TAB> > jch/nd/attr-pathspec-fix > jch/nd/attr-pathspec-in-tree-walk > ... > > For refs organized in multiple levels like this (and I've seen refs in 4 > levels), especially when there a lot of refs, incremental completion > this way makes it easier to get to what you want. > > The cost of course is more complicated completion and also slower on > systems with slow process creation. So maybe there will be a switch to > turn this on or off? Oh, no. After spending quite some effort to eliminate most of the processes and big shell loops from the ref completion code path, I find this patch simply terrible ;) > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index 499e56f83d..d74ee79866 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -742,6 +742,17 @@ __git_refs () > esac > } > > +__git_collapse_refs () > +{ > + local regex="$(echo "$1" | sed 's/[^/]\+/[^\/]*/g')" Surely this could be done using only shell builtins. > + case "$regex" in > + '') regex='[^\/]*';; Should this really escape that '/'? The 'sed' below would escape it anyway, so it would be escaped twice. And the line below doesn't escape it. > + */) regex="${regex}[^/]*";; > + esac > + regex="$(echo "$regex" | sed 's/\//\\\//g')" And this one, too. > + sed -ne "s/\\($regex\\/\\?\\).*/\\1/p" > +} > + > # Completes refs, short and long, local and remote, symbolic and pseudo. > # > # Usage: __git_complete_refs [<option>]... > @@ -769,7 +780,7 @@ __git_complete_refs () > shift > done > > - __gitcomp_direct "$(__git_refs "$remote" "$track" "$pfx" "$cur_" "$sfx")" > + __gitcomp_direct "$(__git_refs "$remote" "$track" "$pfx" "$cur_" "$sfx" | __git_collapse_refs "$cur_")" > } In general I think it would be much better to rely more on 'git for-each-ref' to do the heavy lifting, extending it with new format specifiers/options as necessary. '%(refname:rstrip=-<N>)' already comes somewhat close to what we would need for full ref completion (i.e. 'refs/b<TAB>' to complete 'refs/bisec/bad'), we only have to figure out how many "ref path components" to show based on the number of path components in the current word to be completed. Alas, it won't add the trailing '/' for "ref directories". For "regular" refs completion we would need to combine 'rstrip=-<N>' with 'lstrip=2' to remove the 'refs/(heads|tags|remotes)/' prefix, but ref-filter doesn't support things like that, it allows only one format option. And, of course, the lack of trailing '/' is an issue in this case as well. So perhaps a new format option like '%(refname:path-components=3,2)' to show two ref path components starting with the third with a trailing '/' appended if necessary, e.g. to turn 'refs/remotes/jch/nd/attr-pathspec-fix' into 'jch/nd/'. Note that we also have __git_head() and __git_tags() to complete only branches and only tags.
On Mon, Jan 28, 2019 at 03:38:28PM +0100, SZEDER Gábor wrote: > > - __gitcomp_direct "$(__git_refs "$remote" "$track" "$pfx" "$cur_" "$sfx")" > > + __gitcomp_direct "$(__git_refs "$remote" "$track" "$pfx" "$cur_" "$sfx" | __git_collapse_refs "$cur_")" > > } > > In general I think it would be much better to rely more on 'git > for-each-ref' to do the heavy lifting, extending it with new format > specifiers/options as necessary. FWIW, that was my first thought, too. > '%(refname:rstrip=-<N>)' already comes somewhat close to what we would > need for full ref completion (i.e. 'refs/b<TAB>' to complete > 'refs/bisec/bad'), we only have to figure out how many "ref path > components" to show based on the number of path components in the > current word to be completed. Alas, it won't add the trailing '/' for > "ref directories". I think it also makes it hard to do one thing which (I think) people would want: if there is a single deep ref, complete the whole thing. E.g., given: $ git for-each-ref --refname='%(refname)' refs/heads/foo/bar refs/heads/foo/baz refs/heads/another/deep/one we'd ideally complete "fo" to "foo/" and "ano" to "another/deep/one", rather than making the user tab through each level. Doing that requires actually understanding that the refs are in a list, and not formatting each one independently. So I kind of wonder if it would be easier to simply have a completion mode in ref-filter. -Peff
On Mon, Jan 28, 2019 at 12:27:07PM -0500, Jeff King wrote: > Doing that requires actually understanding that the refs are in a list, > and not formatting each one independently. So I kind of wonder if it > would be easier to simply have a completion mode in ref-filter. Er, I meant "a completion mode in for-each-ref", though I think it largely works out to be the same thing. -Peff
On Tue, Jan 29, 2019 at 12:27 AM Jeff King <peff@peff.net> wrote: > > On Mon, Jan 28, 2019 at 03:38:28PM +0100, SZEDER Gábor wrote: > > > > - __gitcomp_direct "$(__git_refs "$remote" "$track" "$pfx" "$cur_" "$sfx")" > > > + __gitcomp_direct "$(__git_refs "$remote" "$track" "$pfx" "$cur_" "$sfx" | __git_collapse_refs "$cur_")" > > > } > > > > In general I think it would be much better to rely more on 'git > > for-each-ref' to do the heavy lifting, extending it with new format > > specifiers/options as necessary. > > FWIW, that was my first thought, too. I was more concerned whether it's a good idea to begin with. But it sounds like you two both think it's good otherwise would have objected. > > '%(refname:rstrip=-<N>)' already comes somewhat close to what we would > > need for full ref completion (i.e. 'refs/b<TAB>' to complete > > 'refs/bisec/bad'), we only have to figure out how many "ref path > > components" to show based on the number of path components in the > > current word to be completed. Alas, it won't add the trailing '/' for > > "ref directories". > > I think it also makes it hard to do one thing which (I think) people > would want: if there is a single deep ref, complete the whole thing. > E.g., given: > > $ git for-each-ref --refname='%(refname)' > refs/heads/foo/bar > refs/heads/foo/baz > refs/heads/another/deep/one > > we'd ideally complete "fo" to "foo/" and "ano" to "another/deep/one", > rather than making the user tab through each level. Ah ha, like github sometimes show nested submodule paths. Just one small modification, when doing "refs/heads/<tab>" I would just show refs/heads/foo/ refs/heads/another/ not refs/heads/another/deep/one to save space. But when you do "refs/heads/a<tab>" then you get "refs/heads/another/deep/one" immediately. > Doing that requires actually understanding that the refs are in a list, > and not formatting each one independently. So I kind of wonder if it > would be easier to simply have a completion mode in for-each-mode. That also allows more complicated logic. I think sometimes completion code gets it wrong (I think it's often the case with rev/path ambiguation, but maybe dwim stuff too). And we already have all this logic in C.
On Tue, Jan 29, 2019 at 07:43:45AM +0700, Duy Nguyen wrote: > > > In general I think it would be much better to rely more on 'git > > > for-each-ref' to do the heavy lifting, extending it with new format > > > specifiers/options as necessary. > > > > FWIW, that was my first thought, too. > > I was more concerned whether it's a good idea to begin with. But it > sounds like you two both think it's good otherwise would have > objected. Heh. I do not have any real objection, but I don't think I'd really know until I used it for a while. It may be a matter of preference, in which case we might want $GIT_COMPLETION_REF_COMPONENTS to enable/disable it (I don't have an opinion on what the default should be). > > $ git for-each-ref --refname='%(refname)' > > refs/heads/foo/bar > > refs/heads/foo/baz > > refs/heads/another/deep/one > > > > we'd ideally complete "fo" to "foo/" and "ano" to "another/deep/one", > > rather than making the user tab through each level. > > Ah ha, like github sometimes show nested submodule paths. Just one > small modification, when doing "refs/heads/<tab>" I would just show > > refs/heads/foo/ > refs/heads/another/ > > not refs/heads/another/deep/one to save space. But when you do > "refs/heads/a<tab>" then you get "refs/heads/another/deep/one" > immediately. Yeah. It's still only one entry either way (by definition), but it's going to be much longer than all of the others. Again, I think I'd have to see it in practice to decide how much I cared either way. > > Doing that requires actually understanding that the refs are in a list, > > and not formatting each one independently. So I kind of wonder if it > > would be easier to simply have a completion mode in for-each-mode. > > That also allows more complicated logic. I think sometimes completion > code gets it wrong (I think it's often the case with rev/path > ambiguation, but maybe dwim stuff too). And we already have all this > logic in C. Yeah, agreed. -Peff
On Tue, Jan 29, 2019 at 07:43:45AM +0700, Duy Nguyen wrote: > On Tue, Jan 29, 2019 at 12:27 AM Jeff King <peff@peff.net> wrote: > > > In general I think it would be much better to rely more on 'git > > > for-each-ref' to do the heavy lifting, extending it with new format > > > specifiers/options as necessary. > > > > FWIW, that was my first thought, too. > > I was more concerned whether it's a good idea to begin with. But it > sounds like you two both think it's good otherwise would have > objected. Well, I objected to this RFC implementation, but on purpose refrained from expressing an opinion about whether this is good or bad idea, because I didn't even want to try to guess what others might prefer :) And as far as I can tell it doesn't affect my usage (in general I don't have multi-level hierarchies under refs/heads/, so I mostly complete a single ref path component, and when on occasion I do complete a full ref, then I tend to know what I want, and even if I don't, I only need the list of possibilities only at the last ref path component). > > > '%(refname:rstrip=-<N>)' already comes somewhat close to what we would > > > need for full ref completion (i.e. 'refs/b<TAB>' to complete > > > 'refs/bisec/bad'), we only have to figure out how many "ref path > > > components" to show based on the number of path components in the > > > current word to be completed. Alas, it won't add the trailing '/' for > > > "ref directories". > > > > I think it also makes it hard to do one thing which (I think) people > > would want: if there is a single deep ref, complete the whole thing. > > E.g., given: > > > > $ git for-each-ref --refname='%(refname)' > > refs/heads/foo/bar > > refs/heads/foo/baz > > refs/heads/another/deep/one > > > > we'd ideally complete "fo" to "foo/" and "ano" to "another/deep/one", > > rather than making the user tab through each level. > > Ah ha, like github sometimes show nested submodule paths. Just one > small modification, when doing "refs/heads/<tab>" I would just show > > refs/heads/foo/ > refs/heads/another/ > > not refs/heads/another/deep/one to save space. But when you do > "refs/heads/a<tab>" then you get "refs/heads/another/deep/one" > immediately. But this would be inconsistent with how "regular" path completion works, e.g. if you have 'unique-dir/only-one-dir/only-one-file' and do e.g. 'cat u<TAB>', then Bash will only offer 'unique-dir/', not the whole path to the file in the subdir. Ok, I know, those are real paths not refs, they just look alike, because both happen to use '/' as separator. I can't asses whether people, in general, would prefer this behavior or would rather be surprised or bothered by the inconsistency.
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 499e56f83d..d74ee79866 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -742,6 +742,17 @@ __git_refs () esac } +__git_collapse_refs () +{ + local regex="$(echo "$1" | sed 's/[^/]\+/[^\/]*/g')" + case "$regex" in + '') regex='[^\/]*';; + */) regex="${regex}[^/]*";; + esac + regex="$(echo "$regex" | sed 's/\//\\\//g')" + sed -ne "s/\\($regex\\/\\?\\).*/\\1/p" +} + # Completes refs, short and long, local and remote, symbolic and pseudo. # # Usage: __git_complete_refs [<option>]... @@ -769,7 +780,7 @@ __git_complete_refs () shift done - __gitcomp_direct "$(__git_refs "$remote" "$track" "$pfx" "$cur_" "$sfx")" + __gitcomp_direct "$(__git_refs "$remote" "$track" "$pfx" "$cur_" "$sfx" | __git_collapse_refs "$cur_")" } # __git_refs2 requires 1 argument (to pass to __git_refs)
This is in the same spirit of f22f682695 (completion: complete general config vars in two steps - 2018-05-27). Instead of considering all full refs as completion candidates, it completes one "path" component at a time, e.g. $ git switch-branch -d j<TAB> jch/ junio-gpg-pub $ git switch-branch -d jch/<TAB> Display all 154 possibilities? (y or n) jch/ab/ jch/fc/ .... $ git switch-branch -d jch/nd/<TAB> jch/nd/attr-pathspec-fix jch/nd/attr-pathspec-in-tree-walk ... For refs organized in multiple levels like this (and I've seen refs in 4 levels), especially when there a lot of refs, incremental completion this way makes it easier to get to what you want. The cost of course is more complicated completion and also slower on systems with slow process creation. So maybe there will be a switch to turn this on or off? Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- contrib/completion/git-completion.bash | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)