| Message ID | 5991b58c-770c-4aaa-bce5-f396d9f7f16f@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | completion for git-reflog show | expand |
Rubén Justo <rjusto@gmail.com> writes: > When no subcommand is specified to "reflog", we assume "show" [1]: > > $ git reflog -h > usage: git reflog [show] [<log-options>] [<ref>] > ... > > We are not completing correctly this implicit uses of "show": > > With ... > > $ git checkout -b default > > ... we are not completing "default": > > $ git reflog def<TAB><TAB> > > And we are incorrectly returning the "subcommands" when: > > $ git reflog default <TAB><TAB> > delete expire show > > Let's use __gitcomp_subcommand to correctly handle implicit > subcommands. As with a good bug report, if you are showing an "incorrect" behaviour, you should also illustrate what you think is the "correct" behaviour (see below). > 1. cf39f54efc (git reflog show, 2007-02-08) > > Signed-off-by: Rubén Justo <rjusto@gmail.com> > --- > contrib/completion/git-completion.bash | 9 ++++----- > t/t9902-completion.sh | 8 ++++++++ > 2 files changed, 12 insertions(+), 5 deletions(-) > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index 5f2e904b56..c41f25aa80 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -2448,13 +2448,12 @@ _git_rebase () > _git_reflog () > { > local subcommands="show delete expire" > - local subcommand="$(__git_find_on_cmdline "$subcommands")" > > - if [ -z "$subcommand" ]; then > - __gitcomp "$subcommands" > - else > - __git_complete_refs > + if __gitcomp_subcommand "$subcommands"; then > + return > fi > + > + __git_complete_refs > } So, when we see something that could be a subcommand we complete it as a subcommand and return. In your example, how should $ git reflog def<TAB> work? We try to see if there is a subcommand that begins with "def", we see nothing matching, and then run __git_complete_refs? What if the branch you created earlier were not named "default" but, say, "delmonte", and you did "git reflog del<TAB>"? Shouldn't the user be offered "delete" and "delmonte" as potential completions? > __git_send_email_confirm_options="always never auto cc compose" > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh > index aa9a614de3..231e17f378 100755 > --- a/t/t9902-completion.sh > +++ b/t/t9902-completion.sh > @@ -2618,6 +2618,14 @@ test_expect_success 'git clone --config= - value' ' > EOF > ' > > +test_expect_success 'git reflog show' ' > + test_when_finished "git checkout -" && > + git checkout -b shown && > + test_completion "git reflog sho" "show " && IOW, shouldn't this offer both show and shown? > + test_completion "git reflog show sho" "shown " && > + test_completion "git reflog shown sho" "shown " > +' > + > test_expect_success 'options with value' ' > test_completion "git merge -X diff-algorithm=" <<-\EOF
On 26-ene-2024 09:57:25, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > > > When no subcommand is specified to "reflog", we assume "show" [1]: > > > > $ git reflog -h > > usage: git reflog [show] [<log-options>] [<ref>] > > ... > > > > We are not completing correctly this implicit uses of "show": > > > > With ... > > > > $ git checkout -b default > > > > ... we are not completing "default": > > > > $ git reflog def<TAB><TAB> > > > > And we are incorrectly returning the "subcommands" when: > > > > $ git reflog default <TAB><TAB> > > delete expire show > > > > Let's use __gitcomp_subcommand to correctly handle implicit > > subcommands. > > As with a good bug report, if you are showing an "incorrect" > behaviour, you should also illustrate what you think is the > "correct" behaviour (see below). > > > 1. cf39f54efc (git reflog show, 2007-02-08) > > > > Signed-off-by: Rubén Justo <rjusto@gmail.com> > > --- > > contrib/completion/git-completion.bash | 9 ++++----- > > t/t9902-completion.sh | 8 ++++++++ > > 2 files changed, 12 insertions(+), 5 deletions(-) > > > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > > index 5f2e904b56..c41f25aa80 100644 > > --- a/contrib/completion/git-completion.bash > > +++ b/contrib/completion/git-completion.bash > > @@ -2448,13 +2448,12 @@ _git_rebase () > > _git_reflog () > > { > > local subcommands="show delete expire" > > - local subcommand="$(__git_find_on_cmdline "$subcommands")" > > > > - if [ -z "$subcommand" ]; then > > - __gitcomp "$subcommands" > > - else > > - __git_complete_refs > > + if __gitcomp_subcommand "$subcommands"; then > > + return > > fi > > + > > + __git_complete_refs > > } > > So, when we see something that could be a subcommand we complete it > as a subcommand and return. In your example, how should > > $ git reflog def<TAB> > > work? We try to see if there is a subcommand that begins with > "def", we see nothing matching, and then run __git_complete_refs? > What if the branch you created earlier were not named "default" but, > say, "delmonte", and you did "git reflog del<TAB>"? Shouldn't the > user be offered "delete" and "delmonte" as potential completions? > > > __git_send_email_confirm_options="always never auto cc compose" > > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh > > index aa9a614de3..231e17f378 100755 > > --- a/t/t9902-completion.sh > > +++ b/t/t9902-completion.sh > > @@ -2618,6 +2618,14 @@ test_expect_success 'git clone --config= - value' ' > > EOF > > ' > > > > +test_expect_success 'git reflog show' ' > > + test_when_finished "git checkout -" && > > + git checkout -b shown && > > + test_completion "git reflog sho" "show " && > > IOW, shouldn't this offer both show and shown? What should we do? When the user have a branch "show": $ git checkout -b show $ git reflog sho<TAB><TAB> And with: $ git reflog <TAB><TAB> Should we suggest the subcommands alongside the branches? I thought about this, and it's a can of worms. I concluded that a sane an instructive for the user approach is to first try the subcommands and if no option is found, then try the implicit "show". Maybe I'm overthinking it ... > > > + test_completion "git reflog show sho" "shown " && > > + test_completion "git reflog shown sho" "shown " > > +' > > + > > test_expect_success 'options with value' ' > > test_completion "git merge -X diff-algorithm=" <<-\EOF Thanks.
Rubén Justo <rjusto@gmail.com> writes: >> So, when we see something that could be a subcommand we complete it >> as a subcommand and return. In your example, how should >> >> $ git reflog def<TAB> >> >> work? We try to see if there is a subcommand that begins with >> "def", we see nothing matching, and then run __git_complete_refs? >> What if the branch you created earlier were not named "default" but, >> say, "delmonte", and you did "git reflog del<TAB>"? Shouldn't the >> user be offered "delete" and "delmonte" as potential completions? >> >> > __git_send_email_confirm_options="always never auto cc compose" >> > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh >> > index aa9a614de3..231e17f378 100755 >> > --- a/t/t9902-completion.sh >> > +++ b/t/t9902-completion.sh >> > @@ -2618,6 +2618,14 @@ test_expect_success 'git clone --config= - value' ' >> > EOF >> > ' >> > >> > +test_expect_success 'git reflog show' ' >> > + test_when_finished "git checkout -" && >> > + git checkout -b shown && >> > + test_completion "git reflog sho" "show " && >> >> IOW, shouldn't this offer both show and shown? > > What should we do? I would imagine that we should make the above offer both "show" (because it can be what the user meant) and "shown" (because it can also be what the user meant)? But thinking of it again, because "show" is a prefix of "shown", this should offer "show" and then another <TAB> would offer "show" and "shown". At least, that is what I would expect from the usual bash completion behaviour, which would look like: $ mkdir /var/tmp/scratch && cd /var/tmp/scratch $ : >show 2>shown $ echo sho<TAB> The <TAB> makes the above line expand to $ echo show and place the curser immediately after 'w' (without a space after it). Giving another <TAB> at this point offers two possible candidates. $ echo show<TAB> show shown So, I'd expect a similar completion to happen, i.e. $ git reflog sho<TAB> --> $ git reflog show because there are two candidates, "show" and "shown", and I can type another <TAB> at that point to see the two candidates. $ git reflog show<TAB> show shown If the branch name were "shot" instead of "shown", then the possible choices would become "show" and "shot", so we'd skip one step from the above and immediately get the two candidates against "sho<TAB>". $ git reflog sho<TAB> shot show That is what I would expect. Thanks.
On Tue, Feb 20, 2024 at 17:46:20 -0800, Junio C Hamano wrote: > $ git reflog sho<TAB> > shot show > > That is what I would expect. Thank you for responding. Of course that's the logical expectation. However I'm not sure if it is sensible to offer completion for sub-commands mixed with branch names. Furthermore, I am also worried that such an approach implies making the user pay, probably unnecessarily many times, for __git_complete_refs in cases such as: $ git reflog <TAB><TAB> ;# the user may be just exploring the sub-commands $ git reflog s<TAB> ;# the user may be lazy and just want "show " $ git reflog show<TAB> ;# likewise, to complete the SP $ git reflog expir<TAB> ;# how often a expir... branch is expected here? The experienced user, if not most users, should be intuitive enough to circumvent the corner cases: $ git reflog <TAB><TAB> delete expire show ... $ git reflog s<TAB> ... $ git reflog show s<TAB> ... $ git reflog show shot This is why I choose to fallback to __git_complete_ref only when no other option is available. If you think, or anyone else, that these concerns don't make sense, I'm open to make it work as you described.
On Wed, Feb 21, 2024 at 07:06:36PM +0100, Rubén Justo wrote: > On Tue, Feb 20, 2024 at 17:46:20 -0800, Junio C Hamano wrote: > > > $ git reflog sho<TAB> > > shot show > > > > That is what I would expect. > > Thank you for responding. > > Of course that's the logical expectation. > > However I'm not sure if it is sensible to offer completion for > sub-commands mixed with branch names. > > Furthermore, I am also worried that such an approach implies making the > user pay, probably unnecessarily many times, for __git_complete_refs in > cases such as: > > $ git reflog <TAB><TAB> ;# the user may be just exploring the sub-commands > $ git reflog s<TAB> ;# the user may be lazy and just want "show " > $ git reflog show<TAB> ;# likewise, to complete the SP > $ git reflog expir<TAB> ;# how often a expir... branch is expected here? > > The experienced user, if not most users, should be intuitive enough to > circumvent the corner cases: > > $ git reflog <TAB><TAB> > delete expire show > ... > $ git reflog s<TAB> > ... > $ git reflog show s<TAB> > ... > $ git reflog show shot > > This is why I choose to fallback to __git_complete_ref only when no > other option is available. > > If you think, or anyone else, that these concerns don't make sense, I'm > open to make it work as you described. I am happy with the current iteration and I still think that mixing branch names with options is a source of confusion. However, this topic in the latest 'What's cooking' is marked with: Expecting a reroll. cf. <dd106d87-3363-426a-90a2-16e1f2d04661@gmail.com> source: <98daf977-dbad-4d3b-a293-6a769895088f@gmail.com> I am confused about what the expectation is. Consider this message a ping as maybe the message I'm responding to has been missed. Thanks.
Rubén Justo <rjusto@gmail.com> writes: > I am happy with the current iteration and I still think that mixing > branch names with options is a source of confusion. > > However, this topic in the latest 'What's cooking' is marked with: > > Expecting a reroll. > cf. <dd106d87-3363-426a-90a2-16e1f2d04661@gmail.com> > source: <98daf977-dbad-4d3b-a293-6a769895088f@gmail.com> > > I am confused about what the expectation is. Expectation is to show both possible commands and branch names available so that users with enough typing can pick from either, as I do see it even more confusing if only branch names (or only command names) are visible sometimes (namely, if the prefix is shared by both) but some other times command names (or branch names) are completed just fine (namely, if the prefix is unique to only one set of names between command names and branch names).
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 5f2e904b56..c41f25aa80 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -2448,13 +2448,12 @@ _git_rebase () _git_reflog () { local subcommands="show delete expire" - local subcommand="$(__git_find_on_cmdline "$subcommands")" - if [ -z "$subcommand" ]; then - __gitcomp "$subcommands" - else - __git_complete_refs + if __gitcomp_subcommand "$subcommands"; then + return fi + + __git_complete_refs } __git_send_email_confirm_options="always never auto cc compose" diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index aa9a614de3..231e17f378 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -2618,6 +2618,14 @@ test_expect_success 'git clone --config= - value' ' EOF ' +test_expect_success 'git reflog show' ' + test_when_finished "git checkout -" && + git checkout -b shown && + test_completion "git reflog sho" "show " && + test_completion "git reflog show sho" "shown " && + test_completion "git reflog shown sho" "shown " +' + test_expect_success 'options with value' ' test_completion "git merge -X diff-algorithm=" <<-\EOF
When no subcommand is specified to "reflog", we assume "show" [1]: $ git reflog -h usage: git reflog [show] [<log-options>] [<ref>] ... We are not completing correctly this implicit uses of "show": With ... $ git checkout -b default ... we are not completing "default": $ git reflog def<TAB><TAB> And we are incorrectly returning the "subcommands" when: $ git reflog default <TAB><TAB> delete expire show Let's use __gitcomp_subcommand to correctly handle implicit subcommands. 1. cf39f54efc (git reflog show, 2007-02-08) Signed-off-by: Rubén Justo <rjusto@gmail.com> --- contrib/completion/git-completion.bash | 9 ++++----- t/t9902-completion.sh | 8 ++++++++ 2 files changed, 12 insertions(+), 5 deletions(-)