completion: improvements for git stash

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

'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.

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

index a757073945..39c123926c 100644
@@ -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

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.

---
 contrib/completion/git-completion.bash | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

index 39c123926c..452c3d4490 100644
@@ -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

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.

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

index 452c3d4490..8bd445b7dc 100644
@@ -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

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.

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 :)

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

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

Previous round was at <[email protected]>.

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

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.

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

index a757073945..9a95b3b7b1 100644
@@ -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

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

Duy

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>

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.

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

index 9a95b3b7b1..adb6516b6d 100644
@@ -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

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

Thanks, will queue.