[PATCH 0/3] completion: improvements for git stash

[PATCH 0/3] completion: improvements for git stash

[Thomas Gummerer]

In this series we stop completing the 'git stash save' subcommand in
git-completion.bash.  The command has been deprecated for a while, so
I think we're doing the users a disservice by still completing it,
making them think it's still supported while we already deprecated it.

The first commit is a preparatory step for doing that, while the third
commit is an improvement that I thought would be good to do "while
there".  The most interesting part of the series should be the second
commit, where we actually remove the completion of 'git stash save'.

Thomas Gummerer (3):
  completion: rename save_opts to default_opts for stash
  completion: stop completing 'save' as stash subcommand
  completion: make stash -p and alias for stash push -p

 contrib/completion/git-completion.bash | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

-- 
2.17.0.252.gfe0a9eaf31

[PATCH 1/3] completion: rename save_opts to default_opts for stash

[Thomas Gummerer]

'git stash save' is deprecated, but we still call the options for
completion 'save_opts'.  Simply renaming them to 'push_opts' is not
ideal because for example '--message foo' can't be passed directly to
'git stash', as non-option arguments are not allowed, and foo would be
treated as one.

Completing '--message' in 'git stash --<tab>' would end up being quite
confusing for the user.  Therefore name them 'default_opts', and keep
treating --message specially for 'git stash push'.

The ulterior motive for renaming 'save_opts' is that in the next
commit we're going to stop completing 'git stash save', so 'save_opts'
would be the only remaining thing referring to 'git stash save', which
would probably be confusing for future readers of this code.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 contrib/completion/git-completion.bash | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index a757073945..39c123926c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2773,16 +2773,16 @@ _git_show_branch ()
 
 _git_stash ()
 {
-	local save_opts='--all --keep-index --no-keep-index --quiet --patch --include-untracked'
+	local default_opts='--all --keep-index --no-keep-index --quiet --patch --include-untracked'
 	local subcommands='push save list show apply clear drop pop create branch'
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
 	if [ -z "$subcommand" ]; then
 		case "$cur" in
 		--*)
-			__gitcomp "$save_opts"
+			__gitcomp "$default_opts"
 			;;
 		*)
-			if [ -z "$(__git_find_on_cmdline "$save_opts")" ]; then
+			if [ -z "$(__git_find_on_cmdline "$default_opts")" ]; then
 				__gitcomp "$subcommands"
 			fi
 			;;
@@ -2790,10 +2790,10 @@ _git_stash ()
 	else
 		case "$subcommand,$cur" in
 		push,--*)
-			__gitcomp "$save_opts --message"
+			__gitcomp "$default_opts --message"
 			;;
 		save,--*)
-			__gitcomp "$save_opts"
+			__gitcomp "$default_opts"
 			;;
 		apply,--*|pop,--*)
 			__gitcomp "--index --quiet"
-- 
2.17.0.252.gfe0a9eaf31

[PATCH 2/3] completion: stop completing 'save' as stash subcommand

[Thomas Gummerer]

The 'save' subcommand in git stash has been deprecated in
fd2ebf14db ("stash: mark "git stash save" deprecated in the man page",
2017-10-22).  It is however still completed by the git bash
completion.

Stop completing the 'save' subcommand as a further step in the
deprecation process.  As the only use of the bash completion is
interactive, this wouldn't break any scripts, but may give users a
hint that the command is deprecated.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 contrib/completion/git-completion.bash | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 39c123926c..452c3d4490 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2774,7 +2774,7 @@ _git_show_branch ()
 _git_stash ()
 {
 	local default_opts='--all --keep-index --no-keep-index --quiet --patch --include-untracked'
-	local subcommands='push save list show apply clear drop pop create branch'
+	local subcommands='push list show apply clear drop pop create branch'
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
 	if [ -z "$subcommand" ]; then
 		case "$cur" in
@@ -2792,9 +2792,6 @@ _git_stash ()
 		push,--*)
 			__gitcomp "$default_opts --message"
 			;;
-		save,--*)
-			__gitcomp "$default_opts"
-			;;
 		apply,--*|pop,--*)
 			__gitcomp "--index --quiet"
 			;;
-- 
2.17.0.252.gfe0a9eaf31

[PATCH 3/3] completion: make stash -p and alias for stash push -p

[Thomas Gummerer]

We define 'git stash -p' as an alias for 'git stash push -p' in the
manpage.  Do the same in the completion script, so all options that
can be given to 'git stash push' are being completed when the user is
using 'git stash -p --<tab>'.  Currently the only additional option
the user will get is '--message', but there may be more in the future.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 contrib/completion/git-completion.bash | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 452c3d4490..8bd445b7dc 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2776,6 +2776,9 @@ _git_stash ()
 	local default_opts='--all --keep-index --no-keep-index --quiet --patch --include-untracked'
 	local subcommands='push list show apply clear drop pop create branch'
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
+	if [ -n "$(__git_find_on_cmdline "-p")" ]; then
+		subcommand="push"
+	fi
 	if [ -z "$subcommand" ]; then
 		case "$cur" in
 		--*)
-- 
2.17.0.252.gfe0a9eaf31

Re: [PATCH 0/3] completion: improvements for git stash

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> In this series we stop completing the 'git stash save' subcommand in
> git-completion.bash.  The command has been deprecated for a while,...

Anything deprecated in Oct 2017 is still too young in Git's
timescale, but this is the right thing to do in the longer term.

Regarding [2/3], I think 

 - It is perfectly fine to stop offering "save" among the choices
   when "git stash <TAB>" is given, so that we AVOID actively
   suggesting "save" to those who do not know (or do not want to
   learn) about it.  Instead we would knudge them towards "push".
   After all, that is what "deprecation" is all about.

 - It also is OK to limit the offering to "show" against "git stash
   s<TAB>", even though the user _could_ have meant "save" than the
   above case with totally empty subcommand name.

 - It is questionable to stop offering "save" to "git stash
   sav<TAB>" it is clear that the user wants to say "save" in this
   case, as there is no other subcommand the user could have meant.

 - It is outright hostile to the end user if we stop completing "git
   stash save --q<TAB>" with "--quiet".  At that point, we _know_
   that the user wants "save", and getting in the way of those who
   know what they are using does not feel quite right.

Of course, being more accomodating to existing users by taking the
last two points above seriously would raise a separate issue of when
we stop doing so, and if we should start doing something else.  If
there is a way to implement a "reluctant completion" that gives
"save" as a completion choice while giving a deprecation warning to
let the user know of the plan for removal of "save", that would be
good, for example.

Thanks.

Re: [PATCH 0/3] completion: improvements for git stash

[Thomas Gummerer]

On 04/18, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> > In this series we stop completing the 'git stash save' subcommand in
> > git-completion.bash.  The command has been deprecated for a while,...
> 
> Anything deprecated in Oct 2017 is still too young in Git's
> timescale, but this is the right thing to do in the longer term.

Fair enough.  I vaguely remembered this thread [1], that tried to add
'rm' back to the autocompletion after it was removed originally to try
and start the deprecation process.  Reading the thread again now
though it seems what you outline below to just not show "save" when
"git stash <TAB>" or "git stash s<TAB>" is typed makes more sense as a
step now.

I also notice that we never seemed to have taken any of the
suggestions made there (either adding 'rm' back to the completion
options, or going further in the deprecation process), though that's a
different topic :)

> Regarding [2/3], I think 
> 
>  - It is perfectly fine to stop offering "save" among the choices
>    when "git stash <TAB>" is given, so that we AVOID actively
>    suggesting "save" to those who do not know (or do not want to
>    learn) about it.  Instead we would knudge them towards "push".
>    After all, that is what "deprecation" is all about.
> 
>  - It also is OK to limit the offering to "show" against "git stash
>    s<TAB>", even though the user _could_ have meant "save" than the
>    above case with totally empty subcommand name.
> 
>  - It is questionable to stop offering "save" to "git stash
>    sav<TAB>" it is clear that the user wants to say "save" in this
>    case, as there is no other subcommand the user could have meant.
> 
>  - It is outright hostile to the end user if we stop completing "git
>    stash save --q<TAB>" with "--quiet".  At that point, we _know_
>    that the user wants "save", and getting in the way of those who
>    know what they are using does not feel quite right.
> 
> Of course, being more accomodating to existing users by taking the
> last two points above seriously would raise a separate issue of when
> we stop doing so, and if we should start doing something else.  If
> there is a way to implement a "reluctant completion" that gives
> "save" as a completion choice while giving a deprecation warning to
> let the user know of the plan for removal of "save", that would be
> good, for example.

Thanks for the suggestions, I'll take a closer look at what could be
done, and will send a re-roll.

> Thanks.

[1]: 01020160a0004473-277c3d7c-4e3b-4c50-9d44-4a106f37f1d9-000000@eu-west-1.amazonses.com

[PATCH v2 0/2] completion: improvements for git stash

[Thomas Gummerer]

Previous round was at <20180417212945.24002-1-t.gummerer@gmail.com>.

Thanks Junio for your input on the previous round.

This round drops what was 1/3 in the previous round.  We keep
completing the options for 'git stash save', so calling the variable
'save_opts' and defining what would be 'push_opts' as 'save_opts' +
'--message' makes sense.

2/3 (now 1/2) was mostly rewritten.  We now no longer suggest 'save'
in 'git stash <tab>', complete 'git stash s<tab>' will complete to
'git stash show', but 'git stash sa<tab>' (or longer) will keep
completing to 'git stash save', as the user most likely already knows
about 'git stash save', and wanted to type that.  We also keep
completing the options for 'git stash save' on 'git stash save
--<tab>'.

3/3 (now 2/2) stays the same as in the previous round.

I didn't find a good way to implement "reluctant completion" (I'm also
by no means an expert in bash completion, so there may well be a way I
couldn't find by googl'ing around), so I left that out of this
series.

I don't think it's strictly necessary for the deprecation either, as
we can just print a warning message when the user actually uses 'git
stash save' at some point, which would make a message printed when
using the completion redundant anyway.  I feel like that warning
message is not something we're quite ready for yet and I'd rather wait
a few more releases before doing that though.

Thomas Gummerer (2):
  completion: stop showing 'save' for stash by default
  completion: make stash -p and alias for stash push -p

 contrib/completion/git-completion.bash | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

-- 
2.17.0.252.gfe0a9eaf31

[PATCH v2 1/2] completion: stop showing 'save' for stash by default

[Thomas Gummerer]

The 'save' subcommand in git stash has been deprecated in
fd2ebf14db ("stash: mark "git stash save" deprecated in the man page",
2017-10-22).

Stop showing it when the users enters 'git stash <tab>' or 'git stash
s<tab>'.  Keep showing it however when the user enters 'git stash sa<tab>'
or any more characters of the 'save' subcommand.  This is designed to
not encourage users to use 'git stash save', but still leaving the
completion option once it's clear that's what the user means.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 contrib/completion/git-completion.bash | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index a757073945..9a95b3b7b1 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2774,13 +2774,18 @@ _git_show_branch ()
 _git_stash ()
 {
 	local save_opts='--all --keep-index --no-keep-index --quiet --patch --include-untracked'
-	local subcommands='push save list show apply clear drop pop create branch'
-	local subcommand="$(__git_find_on_cmdline "$subcommands")"
+	local subcommands='push list show apply clear drop pop create branch'
+	local subcommand="$(__git_find_on_cmdline "$subcommands save")"
 	if [ -z "$subcommand" ]; then
 		case "$cur" in
 		--*)
 			__gitcomp "$save_opts"
 			;;
+		sa*)
+			if [ -z "$(__git_find_on_cmdline "$save_opts")" ]; then
+				__gitcomp "save"
+			fi
+			;;
 		*)
 			if [ -z "$(__git_find_on_cmdline "$save_opts")" ]; then
 				__gitcomp "$subcommands"
-- 
2.17.0.252.gfe0a9eaf31

Re: [PATCH v2 1/2] completion: stop showing 'save' for stash by default

[Duy Nguyen]

On Fri, Apr 20, 2018 at 1:25 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> The 'save' subcommand in git stash has been deprecated in
> fd2ebf14db ("stash: mark "git stash save" deprecated in the man page",
> 2017-10-22).
>
> Stop showing it when the users enters 'git stash <tab>' or 'git stash
> s<tab>'.  Keep showing it however when the user enters 'git stash sa<tab>'
> or any more characters of the 'save' subcommand.

I don't think this is worth it. You only save two keystrokes for 've'
and already waste one on <tab>.
-- 
Duy

Re: [PATCH v2 1/2] completion: stop showing 'save' for stash by default

[Thomas Gummerer]

On 04/20, Duy Nguyen wrote:
> On Fri, Apr 20, 2018 at 1:25 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > The 'save' subcommand in git stash has been deprecated in
> > fd2ebf14db ("stash: mark "git stash save" deprecated in the man page",
> > 2017-10-22).
> >
> > Stop showing it when the users enters 'git stash <tab>' or 'git stash
> > s<tab>'.  Keep showing it however when the user enters 'git stash sa<tab>'
> > or any more characters of the 'save' subcommand.
> 
> I don't think this is worth it. You only save two keystrokes for 've'
> and already waste one on <tab>.

I think the main reason for keeping the completion is not actually
saving keystrokes, but rather not giving the users the false
impression that we removed the command, or that something is wrong
with it, without properly warning them before.  That was the main
reason given in [1] why the completion is useful even for short
commands such as 'rm'.

So while I had the same impression before re-reading that thread, I
think keeping the completion for 'git stash sa<tab>' is the right
thing to do, and we can remove it some time after we started warning
about 'git stash save' being deprecated.

[1]: <01020160a0004473-277c3d7c-4e3b-4c50-9d44-4a106f37f1d9-000000@eu-west-1.amazonses.com>

> -- 
> Duy

[PATCH v2 2/2] completion: make stash -p and alias for stash push -p

[Thomas Gummerer]

We define 'git stash -p' as an alias for 'git stash push -p' in the
manpage.  Do the same in the completion script, so all options that
can be given to 'git stash push' are being completed when the user is
using 'git stash -p --<tab>'.  Currently the only additional option
the user will get is '--message', but there may be more in the future.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 contrib/completion/git-completion.bash | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 9a95b3b7b1..adb6516b6d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2776,6 +2776,9 @@ _git_stash ()
 	local save_opts='--all --keep-index --no-keep-index --quiet --patch --include-untracked'
 	local subcommands='push list show apply clear drop pop create branch'
 	local subcommand="$(__git_find_on_cmdline "$subcommands save")"
+	if [ -n "$(__git_find_on_cmdline "-p")" ]; then
+		subcommand="push"
+	fi
 	if [ -z "$subcommand" ]; then
 		case "$cur" in
 		--*)
-- 
2.17.0.252.gfe0a9eaf31

Re: [PATCH v2 0/2] completion: improvements for git stash

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> I didn't find a good way to implement "reluctant completion" (I'm also
> by no means an expert in bash completion, so there may well be a way I
> couldn't find by googl'ing around), so I left that out of this
> series.
>
> I don't think it's strictly necessary for the deprecation either, as
> we can just print a warning message when the user actually uses 'git
> stash save' at some point, which would make a message printed when
> using the completion redundant anyway.  I feel like that warning
> message is not something we're quite ready for yet and I'd rather wait
> a few more releases before doing that though.

Yeah, I agree that what you outined above is quite sensible.

Thanks, will queue.