threads / patch / 53317

patch, 11 partsrefactor git switch completion

Subject: [PATCH 00/11] refactor git switch completion

## tl;dr

26 messages between Apr 25, 2020 and May 1, 2020. Diffs are folded; open one to read it.

replies: 25people: 3as markdown or json

Jacob Keller· Apr 25, 2020, 02:20 UTC · lore

From: Jacob Keller <jacob.keller@gmail.com>

completion support for git switch is subpar for a number of cases. Most notable is difference between these two completions:

  $git switch <TAB>
  Display all 784 possibilities? (y or n)
  <list of all references and DWIM remotes>

  $git switch --track<TAB>
  jk-refactor-git-switch-completion master`

If --track is provided, tab completion becomes almost useless, because we would expect to complete remote references, but instead can only complete local branches!

This series was motivated by a desire to fix the completion for the above two cases, but I noticed several other issues on the way, including some issues understanding what the current logic did.

This series aims to improve the completion support, and comes with many additional test cases that should help highlight the buggy behavior and hopefully prevent future regressions.

The first few commits just add new test cases, most of which currently fail.

Following this is a commit to change __git_complete_refs so that it uses "--dwim" instead of "--track", since this made reading _git_checkout() and _git_switch() difficult to read. "--track" was both used as the "enable DWIM remote branch names" and also the option name for --track.

Following this are some patches to extract displaying DWIM remote branch names from __git_refs() and refactoring __git_complete_refs to take a mode argument that switches between calling __git_heads, __git_refs, and a new __git_remotes.

By doing this, it becomes easier to do things like complete DWIM remote branches in addition to just regular branches, rather than all references.

With this series applied, completion for git switch behaves more like the following examples:

  $git switch <TAB>
  HEAD                                master         todo
  jk-refactor-git-switch-completion   next
  maint                               pu

  $git switch --track <TAB>
  origin/HEAD     origin/maint    origin/master   origin/next     origin/pu
  origin/todo

Jacob Keller (11):
  completion: add some simple test cases for git switch completion
  completion: add test showing subpar git switch completion
  completion: add test highlighting subpar git switch --track completion
  completion: add tests showing lack of support for  git switch -c/-C
  completion: remove completion for git switch --orphan
  completion: rename --track option of __git_complete_refs
  completion: extract function __git_dwim_remote_heads
  completion: perform DWIM logic directly in __git_complete_refs
  completion: fix completion for git switch with no options
  completion: recognize -c/-C when completing for git switch
  completion: complete remote branches for git switch --track

 contrib/completion/git-completion.bash | 146 +++++++++++++++++++------
 t/t9902-completion.sh                  | 103 +++++++++++++++++
 2 files changed, 215 insertions(+), 34 deletions(-)

-- 
2.25.2

Jacob Keller· Apr 25, 2020, 02:20 UTC · re: Jacob Keller · lore

[PATCH 01/11] completion: add some simple test cases for git switch completion

From: Jacob Keller <jacob.keller@gmail.com>

Future work is going to refactor the completion support for git switch, so add some new test cases in order to help prevent regressions.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 t/t9902-completion.sh | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

Show changes to t/t9902-completion.sh +25 −0

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 5505e5aa249e..03e8188f023d 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1240,6 +1240,31 @@ test_expect_success '__git_complete_fetch_refspecs - fully qualified & prefix' '
 	test_cmp expected out
 '
 
+test_expect_success 'git switch - with --no-guess, complete only local branches' '
+	test_completion "git switch --no-guess " <<-\EOF
+	master Z
+	matching-branch Z
+	EOF
+'
+
+test_expect_success 'git switch - with --detach, complete all references' '
+	test_completion "git switch --detach " <<-\EOF
+	HEAD Z
+	master Z
+	matching-branch Z
+	matching-tag Z
+	other/branch-in-other Z
+	other/master-in-other Z
+	EOF
+'
+
+test_expect_success 'git switch - with --no-track, complete only local branch names' '
+	test_completion "git switch --no-track " <<-\EOF
+	master Z
+	matching-branch Z
+	EOF
+'
+
 test_expect_success 'teardown after ref completion' '
 	git branch -d matching-branch &&
 	git tag -d matching-tag &&

-- 
2.25.2

Jacob Keller· Apr 25, 2020, 02:20 UTC · re: Jacob Keller · lore

[PATCH 02/11] completion: add test showing subpar git switch completion

From: Jacob Keller <jacob.keller@gmail.com>

With no options, git switch only allows switching branches or DWIM to create a local branch tracking a remote branch of the same name. However, tab completion will expand "git switch <TAB>" to any local reference, including pseudorefs like HEAD, or tags.

Add a test case which highlights this failure, which will be fixed in a future refactoring of git switch completion support.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 t/t9902-completion.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Show changes to t/t9902-completion.sh +13 −0

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 03e8188f023d..af4661cbcc73 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1240,6 +1240,19 @@ test_expect_success '__git_complete_fetch_refspecs - fully qualified & prefix' '
 	test_cmp expected out
 '
 
+# TODO: git switch by default should only include local branches and anything which
+# would be understood by the DWIM logic. Currently it will complete most
+# references including pseudorefs like HEAD and FETCH_HEAD, as well as tags.
+# These should not be completed unless certain options have been enabled.
+test_expect_failure 'git switch - with no options, complete local branches and unique remote branch names for DWIM logic' '
+	test_completion "git switch " <<-\EOF
+	branch-in-other Z
+	master Z
+	master-in-other Z
+	matching-branch Z
+	EOF
+'
+
 test_expect_success 'git switch - with --no-guess, complete only local branches' '
 	test_completion "git switch --no-guess " <<-\EOF
 	master Z

-- 
2.25.2

Jacob Keller· Apr 25, 2020, 02:20 UTC · re: Jacob Keller · lore

[PATCH 03/11] completion: add test highlighting subpar git switch --track completion

From: Jacob Keller <jacob.keller@gmail.com>

If git switch is called with --track, it will attempts to DWIM into creating a local tracking branch that tracks the provided remote branch. It seems reasonable that, to support this behavior, we should only complete remote branches in the form "<remote>/<branch>".

Indeed, current completion is not just sub-par, but could almost be described as entirely useless.

  $git switch --track <TAB>

will only report *local* branch names. Indeed a new test case highlights this quite well:

  --- expected    2020-04-25 00:25:34.424965326 +0000
  +++ out_sorted  2020-04-25 00:25:34.441965370 +0000
  @@ -1,2 +1,2 @@
  -other/branch-in-other
  -other/master-in-other
  +master
  +matching-branch
  not ok 100 - git switch - with --track, complete only remote branches # TODO known breakage

Understanding exactly what causes this is not that simple.

First we enable DWIM output by default. Then, if "--track", "--no-track", or "--no-guess" is enabled on the command line, we disable DWIM. This makes sense, because --track should not include the default "DWIM" remote branch names.

Following this, there is a check for "--detach". If "--detach" is *not* present, then we set only_local_ref=y. this is done because we would like to avoid printing remote references. This immediately seems wrong because --track should allow completing remote references.

Finally, if only_local_ref is 'y', and the track_opt for DWIM logic is disabled, we complete only local branches. This occurs because --track disabled track_opt, and not providing --detach sets only_local_ref to 'y'.

Fixing this correctly is not trivial, so it is left to a follow up change.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 t/t9902-completion.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

Show changes to t/t9902-completion.sh +10 −0

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index af4661cbcc73..002223160058 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1271,6 +1271,16 @@ test_expect_success 'git switch - with --detach, complete all references' '
 	EOF
 '
 
+# TODO: Since --track on its own will perform a DWIM to extract the local
+# branch name, we should complete only the remote branches with their remote
+# name.
+test_expect_failure 'git switch - with --track, complete only remote branches' '
+	test_completion "git switch --track " <<-\EOF
+	other/branch-in-other Z
+	other/master-in-other Z
+	EOF
+'
+
 test_expect_success 'git switch - with --no-track, complete only local branch names' '
 	test_completion "git switch --no-track " <<-\EOF
 	master Z

-- 
2.25.2

Jacob Keller· Apr 25, 2020, 02:20 UTC · re: Jacob Keller · lore

[PATCH 04/11] completion: add tests showing lack of support for git switch -c/-C

From: Jacob Keller <jacob.keller@gmail.com>

Add several tests for git switch completion which highlight the expected behavior if -c or -C have been provided.

Show that properly recognizing and supporting -c should override the behavior for --track.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 t/t9902-completion.sh | 63 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

Show changes to t/t9902-completion.sh +63 −0

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 002223160058..a134a8791076 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1288,6 +1288,69 @@ test_expect_success 'git switch - with --no-track, complete only local branch na
 	EOF
 '
 
+# TODO: git switch completion does not yet support checking for -c, but it
+# should be able to complete all possible references. Based on a quick
+# examination of the switch/checkout code, -c will disable DWIM logic and thus
+# we should not complete unique remote branch names with -c or -C either.
+test_expect_failure 'git switch - with -c, complete all references' '
+	test_completion "git switch -c new-branch " <<-\EOF
+	HEAD Z
+	master Z
+	matching-branch Z
+	matching-tag Z
+	other/branch-in-other Z
+	other/master-in-other Z
+	EOF
+'
+
+test_expect_failure 'git switch - with -c, complete all references' '
+	test_completion "git switch -C new-branch " <<-\EOF
+	HEAD Z
+	master Z
+	matching-branch Z
+	matching-tag Z
+	other/branch-in-other Z
+	other/master-in-other Z
+	EOF
+'
+
+# TODO: ensure that the completion rules for -c override --track
+test_expect_failure 'git switch - with -c and --track, complete all references' '
+	test_completion "git switch -c new-branch --track " <<-EOF
+	HEAD Z
+	master Z
+	matching-branch Z
+	matching-tag Z
+	other/branch-in-other Z
+	other/master-in-other Z
+	EOF
+'
+
+# TODO: git switch with -c and --no-track should allow creating a branch using
+# any reference as a starting point. Because completion support does not
+# recognize -c or -C, this doesn't work yet.
+test_expect_failure 'git switch - with -c and --no-track, complete all references' '
+	test_completion "git switch -c new-branch --no-track " <<-\EOF
+	HEAD Z
+	master Z
+	matching-branch Z
+	matching-tag Z
+	other/branch-in-other Z
+	other/master-in-other Z
+	EOF
+'
+
+test_expect_failure 'git switch - with -C and --no-track, complete all references' '
+	test_completion "git switch -C new-branch --no-track " <<-\EOF
+	HEAD Z
+	master Z
+	matching-branch Z
+	matching-tag Z
+	other/branch-in-other Z
+	other/master-in-other Z
+	EOF
+'
+
 test_expect_success 'teardown after ref completion' '
 	git branch -d matching-branch &&
 	git tag -d matching-tag &&

-- 
2.25.2

Jacob Keller· Apr 25, 2020, 02:20 UTC · re: Jacob Keller · lore

[PATCH 09/11] completion: fix completion for git switch with no options

From: Jacob Keller <jacob.keller@gmail.com>

Add a new --mode option to __git_complete_refs, which allows changing the behavior to call __git_heads instead of __git_refs.

By passing --mode=heads, __git_complete_refs will only output local branches. This enables using "--mode=heads --dwim" to enable listing local branches or the remote unique branch names for DWIM.

Refactor completion support in _git_switch() to decide whether to use this mode instead of calling __git_heads directly.

While doing this, cleanup the function so that it is a little less confusing.

We can simplify this a little by moving the check for GIT_COMPLETION_CHECKOUT_NO_GUESS first, so that we do not get weird interactions by re-enabling --dwim after checking for other options that might disable it.

If --track, --no-track, --no-guess, -d, or --detach are provided, we will disable --dwim, as these DWIM branch names won't work in these modes.

Choose the --mode to specify to __git_complete_refs by also checking command line parameters. Default to using --mode=heads, to list only local branches.

If -d or --detach are provided, switch to --mode=refs, so that we display all references.

In this way, the basic support for completing just "git switch <TAB>" will result in only local branches and remote unique names for DWIM, so switch this test to test_expect_success.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 contrib/completion/git-completion.bash | 59 ++++++++++++++++----------
 t/t9902-completion.sh                  |  6 +--
 2 files changed, 38 insertions(+), 27 deletions(-)

Show changes to 2 files +38 −27

contrib/completion/git-completion.bash, t/t9902-completion.sh

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c582e070711f..0384b136763a 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -782,9 +782,12 @@ __git_refs ()
 #               word to be completed.
 # --sfx=<suffix>: A suffix to be appended to each ref instead of the default
 #                 space.
+# --mode=<mode>: What set of refs to complete, one of 'refs' (the default) to
+#                complete all refs, or 'heads' to complete only branches. Note
+#                that --remote is only compatible with --mode=refs.
 __git_complete_refs ()
 {
-	local remote dwim pfx cur_="$cur" sfx=" "
+	local remote dwim pfx cur_="$cur" sfx=" " mode="refs"
 
 	while test $# != 0; do
 		case "$1" in
@@ -795,13 +798,23 @@ __git_complete_refs ()
 		--pfx=*)	pfx="${1##--pfx=}" ;;
 		--cur=*)	cur_="${1##--cur=}" ;;
 		--sfx=*)	sfx="${1##--sfx=}" ;;
+		--mode=*)	mode="${1##--mode=}" ;;
 		*)		return 1 ;;
 		esac
 		shift
 	done
 
-	__gitcomp_direct "$(__git_refs "$remote" "" "$pfx" "$cur_" "$sfx")"
+	# complete references based on the specified mode
+	case "$mode" in
+		refs)
+			__gitcomp_direct "$(__git_refs "$remote" "" "$pfx" "$cur_" "$sfx")" ;;
+		heads)
+			__gitcomp_direct "$(__git_heads "$pfx" "$cur_" "$sfx")" ;;
+		*)
+			return 1 ;;
+	esac
 
+	# Append DWIM remote branch names if requested
 	if [ "$dwim" = "yes" ]; then
 		__gitcomp_direct_append "$(__git_dwim_remote_heads "$pfx" "$cur_" "$sfx")"
 	fi
@@ -2256,7 +2269,7 @@ _git_switch ()
 		__gitcomp_builtin switch
 		;;
 	*)
-		local dwim_opt="--dwim" only_local_ref=n
+		local dwim_opt="--dwim" mode="heads"
 
 		# --orphan is used to create a branch disconnected from the
 		# current history, based on the empty tree. Since the only
@@ -2266,29 +2279,31 @@ _git_switch ()
 			return
 		fi
 
-		# check if --track, --no-track, or --no-guess was specified
-		# if so, disable DWIM mode
-		if [ "$GIT_COMPLETION_CHECKOUT_NO_GUESS" = "1" ] ||
-		   [ -n "$(__git_find_on_cmdline "--track --no-track --no-guess")" ]; then
+		# By default, git switch will DWIM with remote branch names by
+		# allowing these to expand into creating a local tracking
+		# branch of the same name. Completion for this can be disabled
+		# via GIT_COMPLETION_CHECKOUT_NO_GUESS, unless the user
+		# explicitly asked this behavior with --guess
+		if [ "$GIT_COMPLETION_CHECKOUT_NO_GUESS" = "1" ] &&
+		   [ -z "$(__git_find_on_cmdline "--guess")" ]; then
 			dwim_opt=''
 		fi
-		# explicit --guess enables DWIM mode regardless of
-		# $GIT_COMPLETION_CHECKOUT_NO_GUESS
-		if [ -n "$(__git_find_on_cmdline "--guess")" ]; then
-			dwim_opt='--dwim'
+
+		# Certain combinations of options also disable this DWIM mode,
+		# so we should not complete such names in these cases.
+		if [ -n "$(__git_find_on_cmdline "--track --no-track --no-guess -d --detach")" ]; then
+			dwim_opt=''
 		fi
-		if [ -z "$(__git_find_on_cmdline "-d --detach")" ]; then
-			only_local_ref=y
-		else
-			# --guess --detach is invalid combination, no
-			# dwim will be done when --detach is specified
-			dwim_opt=
-		fi
-		if [ $only_local_ref = y -a -z "$dwim_opt" ]; then
-			__gitcomp_direct "$(__git_heads "" "$cur" " ")"
-		else
-			__git_complete_refs $dwim_opt
+
+		# By default, git switch will only allow switching between
+		# local branches, or DWIM with remote branch names. However,
+		# certain options for creating branches or detaching should
+		# complete all references.
+		if [ -n "$(__git_find_on_cmdline "-d --detach")" ]; then
+			mode="refs"
 		fi
+
+		__git_complete_refs $dwim_opt --mode=$mode
 		;;
 	esac
 }
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 9d02de167219..cfd27e4857b7 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1240,11 +1240,7 @@ test_expect_success '__git_complete_fetch_refspecs - fully qualified & prefix' '
 	test_cmp expected out
 '
 
-# TODO: git switch by default should only include local branches and anything which
-# would be understood by the DWIM logic. Currently it will complete most
-# references including pseudorefs like HEAD and FETCH_HEAD, as well as tags.
-# These should not be completed unless certain options have been enabled.
-test_expect_failure 'git switch - with no options, complete local branches and unique remote branch names for DWIM logic' '
+test_expect_success 'git switch - with no options, complete local branches and unique remote branch names for DWIM logic' '
 	test_completion "git switch " <<-\EOF
 	branch-in-other Z
 	master Z

-- 
2.25.2

Jacob Keller· Apr 25, 2020, 02:20 UTC · re: Jacob Keller · lore

[PATCH 11/11] completion: complete remote branches for git switch --track

From: Jacob Keller <jacob.keller@gmail.com>

git switch --track <remote>/<branch> will be interpreted as a DWIM to create a local tracking branch named <branch> tracking the specified remote.

Completion support for this case is buggy, because it will instead report only local branches.

Fix this by extending __git_complete_refs with a new mode, "remote-heads" which will list all reference under refs/remotes/*.

By doing this, "git switch --track <TAB>" changes from the rather useless set of local branches to only listing remote branches.

Note that if the user has specified "-c" or "-C" already on the command line, this will still enable completing all references, as the check to use the mode "refs" occurs first in an if-elif chain.

This finally fixes the "git switch --track" test case, so it is updated accordingly.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 contrib/completion/git-completion.bash | 25 +++++++++++++++++++++++--
 t/t9902-completion.sh                  |  5 +----
 2 files changed, 24 insertions(+), 6 deletions(-)

Show changes to 2 files +24 −6

contrib/completion/git-completion.bash, t/t9902-completion.sh

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index f9be0dabb03e..cdd141b2ba1d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -624,6 +624,19 @@ __git_heads ()
 			"refs/heads/$cur_*" "refs/heads/$cur_*/**"
 }
 
+# Lists branches from remote repositories.
+# 1: A prefix to be added to each listed branch (optional).
+# 2: List only branches matching this word (optional; list all branches if
+#    unset or empty).
+# 3: A suffix to be appended to each listed branch (optional).
+__git_remote_heads ()
+{
+	local pfx="${1-}" cur_="${2-}" sfx="${3-}"
+
+	__git for-each-ref --format="${pfx//\%/%%}%(refname:strip=2)$sfx" \
+			"refs/remotes/$cur_*" "refs/remotes/$cur_*/**"
+}
+
 # Lists tags from the local repository.
 # Accepts the same positional parameters as __git_heads() above.
 __git_tags ()
@@ -783,8 +796,9 @@ __git_refs ()
 # --sfx=<suffix>: A suffix to be appended to each ref instead of the default
 #                 space.
 # --mode=<mode>: What set of refs to complete, one of 'refs' (the default) to
-#                complete all refs, or 'heads' to complete only branches. Note
-#                that --remote is only compatible with --mode=refs.
+#                complete all refs, 'heads' to complete only branches, or
+#                'remote-heads' to complete only remote branches. Note that
+#                --remote is only compatible with --mode=refs.
 __git_complete_refs ()
 {
 	local remote dwim pfx cur_="$cur" sfx=" " mode="refs"
@@ -810,6 +824,8 @@ __git_complete_refs ()
 			__gitcomp_direct "$(__git_refs "$remote" "" "$pfx" "$cur_" "$sfx")" ;;
 		heads)
 			__gitcomp_direct "$(__git_heads "$pfx" "$cur_" "$sfx")" ;;
+		remote-heads)
+			__gitcomp_direct "$(__git_remote_heads "$pfx" "$cur_" "$sfx")" ;;
 		*)
 			return 1 ;;
 	esac
@@ -2299,8 +2315,13 @@ _git_switch ()
 		# local branches, or DWIM with remote branch names. However,
 		# certain options for creating branches or detaching should
 		# complete all references.
+		#
+		# Additionally, if --track is provided on its own, we should
+		# complete only remote branch names.
 		if [ -n "$(__git_find_on_cmdline "-d --detach -c -C")" ]; then
 			mode="refs"
+		elif [ -n "$(__git_find_on_cmdline "--track")" ]; then
+			mode="remote-heads"
 		fi
 
 		__git_complete_refs $dwim_opt --mode=$mode
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 68296d79a3e9..7491d8c3b72d 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1267,10 +1267,7 @@ test_expect_success 'git switch - with --detach, complete all references' '
 	EOF
 '
 
-# TODO: Since --track on its own will perform a DWIM to extract the local
-# branch name, we should complete only the remote branches with their remote
-# name.
-test_expect_failure 'git switch - with --track, complete only remote branches' '
+test_expect_success 'git switch - with --track, complete only remote branches' '
 	test_completion "git switch --track " <<-\EOF
 	other/branch-in-other Z
 	other/master-in-other Z

-- 
2.25.2

Jacob Keller· Apr 25, 2020, 02:20 UTC · re: Jacob Keller · lore

[PATCH 08/11] completion: perform DWIM logic directly in __git_complete_refs

From: Jacob Keller <jacob.keller@gmail.com>

__git_complete_refs is the main function used for completing references. It is primarily used as a wrapper around __git_refs, and is easier to extend since its arguments are option-like.

One major downside of __git_complete_refs and __git_refs currently, is the lack of ability to complete only a subset of refs such as branches (refs/heads) or tags (refs/tags).

Normally, a caller might just decide to use __git_heads() or __git_tags(). However, in the case of git-switch, it is useful to complete both branches *and* DWIM remote branch names.

Due to the complexity and implementation of __git_refs, it is not easy to extend it to support listing only a subset of references.

Instead, we can extend __git_complete_refs to do this. For this to be done, we must first ensure that "--dwim" support is not tied to calling __git_refs.

Instead of passing $dwim into __git_refs, we can implement a __gitcomp_direct_append function which can append to COMPREPLY after a call to __gitcomp_direct.

If --dwim is passed to __git_complete_refs, use __gitcomp_direct_append to add the output of __git_dwim_remote_heads to the completion list.

In this way, --dwim support is now independent of calling __git_refs.

A future change will add an additional option to control what set of references __git_complete_refs will output.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 contrib/completion/git-completion.bash | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

Show changes to contrib/completion/git-completion.bash +18 −1

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index f1ee25f05690..c582e070711f 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -301,6 +301,19 @@ __gitcomp_direct ()
 	COMPREPLY=($1)
 }
 
+# Similar to __gitcomp_direct, but appends to COMPREPLY instead.
+# Callers must take care of providing only words that match the current word
+# to be completed and adding any prefix and/or suffix (trailing space!), if
+# necessary.
+# 1: List of newline-separated matching completion words, complete with
+#    prefix and suffix.
+__gitcomp_direct_append ()
+{
+	local IFS=$'\n'
+
+	COMPREPLY+=($1)
+}
+
 __gitcompappend ()
 {
 	local x i=${#COMPREPLY[@]}
@@ -787,7 +800,11 @@ __git_complete_refs ()
 		shift
 	done
 
-	__gitcomp_direct "$(__git_refs "$remote" "$dwim" "$pfx" "$cur_" "$sfx")"
+	__gitcomp_direct "$(__git_refs "$remote" "" "$pfx" "$cur_" "$sfx")"
+
+	if [ "$dwim" = "yes" ]; then
+		__gitcomp_direct_append "$(__git_dwim_remote_heads "$pfx" "$cur_" "$sfx")"
+	fi
 }
 
 # __git_refs2 requires 1 argument (to pass to __git_refs)

-- 
2.25.2

Jacob Keller· Apr 25, 2020, 02:20 UTC · re: Jacob Keller · lore

[PATCH 10/11] completion: recognize -c/-C when completing for git switch

From: Jacob Keller <jacob.keller@gmail.com>

If a user wants to create a new branch using git switch, we should be able to complete any starting point reference. Additionally, DWIM logic will not work.

Add -c and -C to the list of options for disabling --dwim and for choosing the --refs mode.

This fixes several known breakages, so update these tests accordingly.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 contrib/completion/git-completion.bash |  4 ++--
 t/t9902-completion.sh                  | 18 +++++-------------
 2 files changed, 7 insertions(+), 15 deletions(-)

Show changes to 2 files +7 −15

contrib/completion/git-completion.bash, t/t9902-completion.sh

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 0384b136763a..f9be0dabb03e 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2291,7 +2291,7 @@ _git_switch ()
 
 		# Certain combinations of options also disable this DWIM mode,
 		# so we should not complete such names in these cases.
-		if [ -n "$(__git_find_on_cmdline "--track --no-track --no-guess -d --detach")" ]; then
+		if [ -n "$(__git_find_on_cmdline "--track --no-track --no-guess -d --detach -c -C")" ]; then
 			dwim_opt=''
 		fi
 
@@ -2299,7 +2299,7 @@ _git_switch ()
 		# local branches, or DWIM with remote branch names. However,
 		# certain options for creating branches or detaching should
 		# complete all references.
-		if [ -n "$(__git_find_on_cmdline "-d --detach")" ]; then
+		if [ -n "$(__git_find_on_cmdline "-d --detach -c -C")" ]; then
 			mode="refs"
 		fi
 
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index cfd27e4857b7..68296d79a3e9 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1284,11 +1284,7 @@ test_expect_success 'git switch - with --no-track, complete only local branch na
 	EOF
 '
 
-# TODO: git switch completion does not yet support checking for -c, but it
-# should be able to complete all possible references. Based on a quick
-# examination of the switch/checkout code, -c will disable DWIM logic and thus
-# we should not complete unique remote branch names with -c or -C either.
-test_expect_failure 'git switch - with -c, complete all references' '
+test_expect_success 'git switch - with -c, complete all references' '
 	test_completion "git switch -c new-branch " <<-\EOF
 	HEAD Z
 	master Z
@@ -1299,7 +1295,7 @@ test_expect_failure 'git switch - with -c, complete all references' '
 	EOF
 '
 
-test_expect_failure 'git switch - with -c, complete all references' '
+test_expect_success 'git switch - with -c, complete all references' '
 	test_completion "git switch -C new-branch " <<-\EOF
 	HEAD Z
 	master Z
@@ -1310,8 +1306,7 @@ test_expect_failure 'git switch - with -c, complete all references' '
 	EOF
 '
 
-# TODO: ensure that the completion rules for -c override --track
-test_expect_failure 'git switch - with -c and --track, complete all references' '
+test_expect_success 'git switch - with -c and --track, complete all references' '
 	test_completion "git switch -c new-branch --track " <<-EOF
 	HEAD Z
 	master Z
@@ -1322,10 +1317,7 @@ test_expect_failure 'git switch - with -c and --track, complete all references'
 	EOF
 '
 
-# TODO: git switch with -c and --no-track should allow creating a branch using
-# any reference as a starting point. Because completion support does not
-# recognize -c or -C, this doesn't work yet.
-test_expect_failure 'git switch - with -c and --no-track, complete all references' '
+test_expect_success 'git switch - with -c and --no-track, complete all references' '
 	test_completion "git switch -c new-branch --no-track " <<-\EOF
 	HEAD Z
 	master Z
@@ -1336,7 +1328,7 @@ test_expect_failure 'git switch - with -c and --no-track, complete all reference
 	EOF
 '
 
-test_expect_failure 'git switch - with -C and --no-track, complete all references' '
+test_expect_success 'git switch - with -C and --no-track, complete all references' '
 	test_completion "git switch -C new-branch --no-track " <<-\EOF
 	HEAD Z
 	master Z

-- 
2.25.2

Jacob Keller· Apr 25, 2020, 02:20 UTC · re: Jacob Keller · lore

[PATCH 07/11] completion: extract function __git_dwim_remote_heads

From: Jacob Keller <jacob.keller@gmail.com>

__git_refs() has the ability to report unique remote names for supporting completion of remote branch names for the DWIMery of git checkout and git switch.

For git checkout, this is fine, because it always supports completing all local references.

However, git switch by default only supports either switching branches or using this DWIMery to create a local branch tracking the remote branch.

Future work to cleanup and improve completion support for git switch will be aided if the remote branch names can be completed separately from __git_refs.

Extract this logic to a function __git_dwim_remote_heads(), and use it in __git_refs.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 contrib/completion/git-completion.bash | 28 +++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

Show changes to contrib/completion/git-completion.bash +21 −7

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 02dc1203559c..f1ee25f05690 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -621,6 +621,26 @@ __git_tags ()
 			"refs/tags/$cur_*" "refs/tags/$cur_*/**"
 }
 
+# List unique branches from refs/remotes used for 'git checkout' and 'git
+# switch' tracking DWIMery.
+# 1: A prefix to be added to each listed branch (optional)
+# 2: List only branches matching this word (optional; list all branches if
+#    unset or empty).
+# 3: A suffix to be appended to each listed branch (optional).
+__git_dwim_remote_heads ()
+{
+	local pfx="${1-}" cur_="${2-}" sfx="${3-}"
+	local fer_pfx="${pfx//\%/%%}" # "escape" for-each-ref format specifiers
+
+	# employ the heuristic used by git checkout and git switch
+	# Try to find a remote branch that cur_es the completion word
+	# but only output if the branch name is unique
+	__git for-each-ref --format="$fer_pfx%(refname:strip=3)$sfx" \
+		--sort="refname:strip=3" \
+		"refs/remotes/*/$cur_*" "refs/remotes/*/$cur_*/**" | \
+	uniq -u
+}
+
 # Lists refs from the local (by default) or from a remote repository.
 # It accepts 0, 1 or 2 arguments:
 # 1: The remote to list refs from (optional; ignored, if set but empty).
@@ -696,13 +716,7 @@ __git_refs ()
 		__git_dir="$dir" __git for-each-ref --format="$fer_pfx%($format)$sfx" \
 			"${refs[@]}"
 		if [ -n "$track" ]; then
-			# employ the heuristic used by git checkout
-			# Try to find a remote branch that matches the completion word
-			# but only output if the branch name is unique
-			__git for-each-ref --format="$fer_pfx%(refname:strip=3)$sfx" \
-				--sort="refname:strip=3" \
-				"refs/remotes/*/$match*" "refs/remotes/*/$match*/**" | \
-			uniq -u
+			__git_dwim_remote_heads "$pfx" "$match" "$sfx"
 		fi
 		return
 	fi

-- 
2.25.2

Jacob Keller· Apr 25, 2020, 02:20 UTC · re: Jacob Keller · lore

[PATCH 06/11] completion: rename --track option of __git_complete_refs

From: Jacob Keller <jacob.keller@gmail.com>

The __git_complete_refs uses the "--track" option to specify when to enable listing of unique remote branches which could be used by the DWIMery of git checkout and git switch.

This is confusing, because both git checkout and git switch have a --track option. This makes looking at the completion code for these two functions confusing.

Rename this parameter to --dwim. Because it is plausible for users to have developed their own completions which rely on __git_complete_ref, keep --track as a synonym for --dwim, but do not use it in our own completion code.

This reduces the confusion when seeing --track in the _git_switch() and _git_checkout() completion functions.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 contrib/completion/git-completion.bash | 28 ++++++++++++++------------
 1 file changed, 15 insertions(+), 13 deletions(-)

Show changes to contrib/completion/git-completion.bash +15 −13

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 08d3406cf3e4..02dc1203559c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -749,7 +749,7 @@ __git_refs ()
 # Usage: __git_complete_refs [<option>]...
 # --remote=<remote>: The remote to list refs from, can be the name of a
 #                    configured remote, a path, or a URL.
-# --track: List unique remote branches for 'git checkout's tracking DWIMery.
+# --dwim: List unique remote branches for 'git switch's tracking DWIMery.
 # --pfx=<prefix>: A prefix to be added to each ref.
 # --cur=<word>: The current ref to be completed.  Defaults to the current
 #               word to be completed.
@@ -757,12 +757,14 @@ __git_refs ()
 #                 space.
 __git_complete_refs ()
 {
-	local remote track pfx cur_="$cur" sfx=" "
+	local remote dwim pfx cur_="$cur" sfx=" "
 
 	while test $# != 0; do
 		case "$1" in
 		--remote=*)	remote="${1##--remote=}" ;;
-		--track)	track="yes" ;;
+		--dwim)		dwim="yes" ;;
+		# --track is an old spelling of --dwim
+		--track)	dwim="yes" ;;
 		--pfx=*)	pfx="${1##--pfx=}" ;;
 		--cur=*)	cur_="${1##--cur=}" ;;
 		--sfx=*)	sfx="${1##--sfx=}" ;;
@@ -771,7 +773,7 @@ __git_complete_refs ()
 		shift
 	done
 
-	__gitcomp_direct "$(__git_refs "$remote" "$track" "$pfx" "$cur_" "$sfx")"
+	__gitcomp_direct "$(__git_refs "$remote" "$dwim" "$pfx" "$cur_" "$sfx")"
 }
 
 # __git_refs2 requires 1 argument (to pass to __git_refs)
@@ -1370,12 +1372,12 @@ _git_checkout ()
 	*)
 		# check if --track, --no-track, or --no-guess was specified
 		# if so, disable DWIM mode
-		local flags="--track --no-track --no-guess" track_opt="--track"
+		local flags="--track --no-track --no-guess" dwim_opt="--dwim"
 		if [ "$GIT_COMPLETION_CHECKOUT_NO_GUESS" = "1" ] ||
 		   [ -n "$(__git_find_on_cmdline "$flags")" ]; then
-			track_opt=''
+			dwim_opt=''
 		fi
-		__git_complete_refs $track_opt
+		__git_complete_refs $dwim_opt
 		;;
 	esac
 }
@@ -2223,7 +2225,7 @@ _git_switch ()
 		__gitcomp_builtin switch
 		;;
 	*)
-		local track_opt="--track" only_local_ref=n
+		local dwim_opt="--dwim" only_local_ref=n
 
 		# --orphan is used to create a branch disconnected from the
 		# current history, based on the empty tree. Since the only
@@ -2237,24 +2239,24 @@ _git_switch ()
 		# if so, disable DWIM mode
 		if [ "$GIT_COMPLETION_CHECKOUT_NO_GUESS" = "1" ] ||
 		   [ -n "$(__git_find_on_cmdline "--track --no-track --no-guess")" ]; then
-			track_opt=''
+			dwim_opt=''
 		fi
 		# explicit --guess enables DWIM mode regardless of
 		# $GIT_COMPLETION_CHECKOUT_NO_GUESS
 		if [ -n "$(__git_find_on_cmdline "--guess")" ]; then
-			track_opt='--track'
+			dwim_opt='--dwim'
 		fi
 		if [ -z "$(__git_find_on_cmdline "-d --detach")" ]; then
 			only_local_ref=y
 		else
 			# --guess --detach is invalid combination, no
 			# dwim will be done when --detach is specified
-			track_opt=
+			dwim_opt=
 		fi
-		if [ $only_local_ref = y -a -z "$track_opt" ]; then
+		if [ $only_local_ref = y -a -z "$dwim_opt" ]; then
 			__gitcomp_direct "$(__git_heads "" "$cur" " ")"
 		else
-			__git_complete_refs $track_opt
+			__git_complete_refs $dwim_opt
 		fi
 		;;
 	esac

-- 
2.25.2

Jacob Keller· Apr 25, 2020, 02:20 UTC · re: Jacob Keller · lore

[PATCH 05/11] completion: remove completion for git switch --orphan

From: Jacob Keller <jacob.keller@gmail.com>

git switch with the --orphan option is used to create a new branch that is not connected to any history and is instead based on the empty tree.

It does not make sense for completion to return anything in this case, because there is nothing to complete. Check for --orphan, and if it's found, immediately return from _git_switch() without completing anything.

Add a test case which documents this expected behavior.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 contrib/completion/git-completion.bash | 11 ++++++++++-
 t/t9902-completion.sh                  |  7 +++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

Show changes to 2 files +17 −1

contrib/completion/git-completion.bash, t/t9902-completion.sh

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c21786f2fd00..08d3406cf3e4 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2223,9 +2223,18 @@ _git_switch ()
 		__gitcomp_builtin switch
 		;;
 	*)
+		local track_opt="--track" only_local_ref=n
+
+		# --orphan is used to create a branch disconnected from the
+		# current history, based on the empty tree. Since the only
+		# option required is the branch name, it doesn't make sense to
+		# complete anything here.
+		if [ -n "$(__git_find_on_cmdline "--orphan")" ]; then
+			return
+		fi
+
 		# check if --track, --no-track, or --no-guess was specified
 		# if so, disable DWIM mode
-		local track_opt="--track" only_local_ref=n
 		if [ "$GIT_COMPLETION_CHECKOUT_NO_GUESS" = "1" ] ||
 		   [ -n "$(__git_find_on_cmdline "--track --no-track --no-guess")" ]; then
 			track_opt=''
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index a134a8791076..9d02de167219 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1351,6 +1351,13 @@ test_expect_failure 'git switch - with -C and --no-track, complete all reference
 	EOF
 '
 
+# TODO: completion does not yet recognize --orphan
+test_expect_failure 'git switch - with --orphan, do not complete anything' '
+	test_completion "git switch --orphan " <<-\EOF
+
+	EOF
+'
+
 test_expect_success 'teardown after ref completion' '
 	git branch -d matching-branch &&
 	git tag -d matching-tag &&

-- 
2.25.2

Jacob Keller· Apr 25, 2020, 02:20 UTC · re: Jacob Keller · lore

[PATCH 05/11] completion: add test showing subpar completion for git switch --orphan

From: Jacob Keller <jacob.keller@gmail.com>

git switch with the --orphan option is used to create a new branch that is not connected to any history and is instead based on the empty tree.

Add a test case which documents this expected behavior.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 contrib/completion/git-completion.bash | 11 ++++++++++-
 t/t9902-completion.sh                  |  7 +++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

Show changes to 2 files +17 −1

contrib/completion/git-completion.bash, t/t9902-completion.sh

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c21786f2fd00..08d3406cf3e4 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2223,9 +2223,18 @@ _git_switch ()
 		__gitcomp_builtin switch
 		;;
 	*)
+		local track_opt="--track" only_local_ref=n
+
+		# --orphan is used to create a branch disconnected from the
+		# current history, based on the empty tree. Since the only
+		# option required is the branch name, it doesn't make sense to
+		# complete anything here.
+		if [ -n "$(__git_find_on_cmdline "--orphan")" ]; then
+			return
+		fi
+
 		# check if --track, --no-track, or --no-guess was specified
 		# if so, disable DWIM mode
-		local track_opt="--track" only_local_ref=n
 		if [ "$GIT_COMPLETION_CHECKOUT_NO_GUESS" = "1" ] ||
 		   [ -n "$(__git_find_on_cmdline "--track --no-track --no-guess")" ]; then
 			track_opt=''
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index a134a8791076..9d02de167219 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1351,6 +1351,13 @@ test_expect_failure 'git switch - with -C and --no-track, complete all reference
 	EOF
 '
 
+# TODO: completion does not yet recognize --orphan
+test_expect_failure 'git switch - with --orphan, do not complete anything' '
+	test_completion "git switch --orphan " <<-\EOF
+
+	EOF
+'
+
 test_expect_success 'teardown after ref completion' '
 	git branch -d matching-branch &&
 	git tag -d matching-tag &&

-- 
2.25.2

Junio C Hamano· Apr 27, 2020, 23:34 UTC · re: Jacob Keller · lore

Re: [PATCH 05/11] completion: add test showing subpar completion for git switch --orphan

Jacob Keller <jacob.e.keller@intel.com> writes:

Show 57 quoted lines

> From: Jacob Keller <jacob.keller@gmail.com>
>
> git switch with the --orphan option is used to create a new branch that
> is not connected to any history and is instead based on the empty tree.
>
> It does not make sense for completion to return anything in this case,
> because there is nothing to complete. Check for --orphan, and if it's
> found, immediately return from _git_switch() without completing
> anything.
>
> Add a test case which documents this expected behavior.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 11 ++++++++++-
>  t/t9902-completion.sh                  |  7 +++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index c21786f2fd00..08d3406cf3e4 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2223,9 +2223,18 @@ _git_switch ()
>  		__gitcomp_builtin switch
>  		;;
>  	*)
> +		local track_opt="--track" only_local_ref=n
> +
> +		# --orphan is used to create a branch disconnected from the
> +		# current history, based on the empty tree. Since the only
> +		# option required is the branch name, it doesn't make sense to
> +		# complete anything here.
> +		if [ -n "$(__git_find_on_cmdline "--orphan")" ]; then
> +			return
> +		fi
> +
>  		# check if --track, --no-track, or --no-guess was specified
>  		# if so, disable DWIM mode
> -		local track_opt="--track" only_local_ref=n
>  		if [ "$GIT_COMPLETION_CHECKOUT_NO_GUESS" = "1" ] ||
>  		   [ -n "$(__git_find_on_cmdline "--track --no-track --no-guess")" ]; then
>  			track_opt=''
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index a134a8791076..9d02de167219 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -1351,6 +1351,13 @@ test_expect_failure 'git switch - with -C and --no-track, complete all reference
>  	EOF
>  '
>  
> +# TODO: completion does not yet recognize --orphan
> +test_expect_failure 'git switch - with --orphan, do not complete anything' '
> +	test_completion "git switch --orphan " <<-\EOF
> +
> +	EOF
> +'
> +

I am getting "TODO passed" at 7276ffe0 (completion: add test showing subpar completion for git switch --orphan, 2020-04-24), which hasn't hit 'next' yet.

Perhaps we got some rebase gotcha here?

>  test_expect_success 'teardown after ref completion' '
>  	git branch -d matching-branch &&
>  	git tag -d matching-tag &&

Jacob Keller· Apr 28, 2020, 02:12 UTC · re: Junio C Hamano · lore

Re: [PATCH 05/11] completion: add test showing subpar completion for git switch --orphan

On Mon, Apr 27, 2020 at 4:34 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 67 quoted lines

>
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
> > From: Jacob Keller <jacob.keller@gmail.com>
> >
> > git switch with the --orphan option is used to create a new branch that
> > is not connected to any history and is instead based on the empty tree.
> >
> > It does not make sense for completion to return anything in this case,
> > because there is nothing to complete. Check for --orphan, and if it's
> > found, immediately return from _git_switch() without completing
> > anything.
> >
> > Add a test case which documents this expected behavior.
> >
> > Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> > ---
> >  contrib/completion/git-completion.bash | 11 ++++++++++-
> >  t/t9902-completion.sh                  |  7 +++++++
> >  2 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> > index c21786f2fd00..08d3406cf3e4 100644
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -2223,9 +2223,18 @@ _git_switch ()
> >               __gitcomp_builtin switch
> >               ;;
> >       *)
> > +             local track_opt="--track" only_local_ref=n
> > +
> > +             # --orphan is used to create a branch disconnected from the
> > +             # current history, based on the empty tree. Since the only
> > +             # option required is the branch name, it doesn't make sense to
> > +             # complete anything here.
> > +             if [ -n "$(__git_find_on_cmdline "--orphan")" ]; then
> > +                     return
> > +             fi
> > +
> >               # check if --track, --no-track, or --no-guess was specified
> >               # if so, disable DWIM mode
> > -             local track_opt="--track" only_local_ref=n
> >               if [ "$GIT_COMPLETION_CHECKOUT_NO_GUESS" = "1" ] ||
> >                  [ -n "$(__git_find_on_cmdline "--track --no-track --no-guess")" ]; then
> >                       track_opt=''
> > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> > index a134a8791076..9d02de167219 100755
> > --- a/t/t9902-completion.sh
> > +++ b/t/t9902-completion.sh
> > @@ -1351,6 +1351,13 @@ test_expect_failure 'git switch - with -C and --no-track, complete all reference
> >       EOF
> >  '
> >
> > +# TODO: completion does not yet recognize --orphan
> > +test_expect_failure 'git switch - with --orphan, do not complete anything' '
> > +     test_completion "git switch --orphan " <<-\EOF
> > +
> > +     EOF
> > +'
> > +
>
> I am getting "TODO passed" at 7276ffe0 (completion: add test showing
> subpar completion for git switch --orphan, 2020-04-24), which hasn't
> hit 'next' yet.
>
> Perhaps we got some rebase gotcha here?
>

Hmm.. Yea, I think there is a problem. Originally I had one commit which added the test case for --orphan and I fixed it later, but I rebased things so it was one commit. I probably just forgot to update the tests.

Thanks, Jake

> >  test_expect_success 'teardown after ref completion' '
> >       git branch -d matching-branch &&
> >       git tag -d matching-tag &&

Jacob Keller· Apr 28, 2020, 02:20 UTC · re: Junio C Hamano · lore

Re: [PATCH 05/11] completion: add test showing subpar completion for git switch --orphan

The proposed SQUASH you have in gitster/jk/complete-git-switch is correct. The commit message body is correct, but the title could be reworded to

"completion: stop completing refs for git switch --orphan"

I can send a v2 if that would be helpful, and I've got it fixed up locally if other review increases the need for a new spin.

Thanks, Jake

On Mon, Apr 27, 2020 at 4:34 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 70 quoted lines

>
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
> > From: Jacob Keller <jacob.keller@gmail.com>
> >
> > git switch with the --orphan option is used to create a new branch that
> > is not connected to any history and is instead based on the empty tree.
> >
> > It does not make sense for completion to return anything in this case,
> > because there is nothing to complete. Check for --orphan, and if it's
> > found, immediately return from _git_switch() without completing
> > anything.
> >
> > Add a test case which documents this expected behavior.
> >
> > Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> > ---
> >  contrib/completion/git-completion.bash | 11 ++++++++++-
> >  t/t9902-completion.sh                  |  7 +++++++
> >  2 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> > index c21786f2fd00..08d3406cf3e4 100644
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -2223,9 +2223,18 @@ _git_switch ()
> >               __gitcomp_builtin switch
> >               ;;
> >       *)
> > +             local track_opt="--track" only_local_ref=n
> > +
> > +             # --orphan is used to create a branch disconnected from the
> > +             # current history, based on the empty tree. Since the only
> > +             # option required is the branch name, it doesn't make sense to
> > +             # complete anything here.
> > +             if [ -n "$(__git_find_on_cmdline "--orphan")" ]; then
> > +                     return
> > +             fi
> > +
> >               # check if --track, --no-track, or --no-guess was specified
> >               # if so, disable DWIM mode
> > -             local track_opt="--track" only_local_ref=n
> >               if [ "$GIT_COMPLETION_CHECKOUT_NO_GUESS" = "1" ] ||
> >                  [ -n "$(__git_find_on_cmdline "--track --no-track --no-guess")" ]; then
> >                       track_opt=''
> > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> > index a134a8791076..9d02de167219 100755
> > --- a/t/t9902-completion.sh
> > +++ b/t/t9902-completion.sh
> > @@ -1351,6 +1351,13 @@ test_expect_failure 'git switch - with -C and --no-track, complete all reference
> >       EOF
> >  '
> >
> > +# TODO: completion does not yet recognize --orphan
> > +test_expect_failure 'git switch - with --orphan, do not complete anything' '
> > +     test_completion "git switch --orphan " <<-\EOF
> > +
> > +     EOF
> > +'
> > +
>
> I am getting "TODO passed" at 7276ffe0 (completion: add test showing
> subpar completion for git switch --orphan, 2020-04-24), which hasn't
> hit 'next' yet.
>
> Perhaps we got some rebase gotcha here?
>
> >  test_expect_success 'teardown after ref completion' '
> >       git branch -d matching-branch &&
> >       git tag -d matching-tag &&

Junio C Hamano· Apr 28, 2020, 16:24 UTC · re: Jacob Keller · lore

Re: [PATCH 05/11] completion: add test showing subpar completion for git switch --orphan

Jacob Keller <jacob.keller@gmail.com> writes:

Show 8 quoted lines

> The proposed SQUASH you have in gitster/jk/complete-git-switch is
> correct. The commit message body is correct, but the title could be
> reworded to
>
> "completion: stop completing refs for git switch --orphan"
>
> I can send a v2 if that would be helpful, and I've got it fixed up
> locally if other review increases the need for a new spin.

Thnaks. In the meantime, what I have is attached (the test title, in addition to the commit title, has been updated).

The same logic would apply to "git checkout -b <TAB>" (or "git switch -c <TAB>") as this change: these are meant to create a new branch so you do not want to offer an existing branch name.

I have a mixed feelings about that reasoning, though. I understand that not offering any existing branch name would avoid offering a branch name that would cause an error message from the command, which at a first glance feels like a nice help to the user, but I am not sure if it is really helping.

When you are on jk/complete-switch branch to work on this topic, you may want to keep the current state of the branch and use a "v2" branch to play around (running "rebase -i" etc.), for which the way I would hope to work would be:

	git checkout -b jk/comp<TAB>

that would complete to an existing branch

	git checkout -b jk/complete-switch

and then I can just type "-v2<Enter>" (or "<BackSpace>-v2<Enter>" to remove the inter-word space completion adds?) after that. After all, "git checkout -b jk/complete-switch" in that scenario would error out by saying that you already use that name, which is a good enough protection.

And that same "is this really helping?" reasoning applies equally to the "--orphan" option.

I dunno.

-- >8 --
From: Jacob Keller <jacob.keller@gmail.com>
Date: Fri, 24 Apr 2020 19:20:38 -0700
Subject: [PATCH] completion: stop completing refs for git switch --orphan

git switch with the --orphan option is used to create a new branch that is not connected to any history and is instead based on the empty tree.

Add a test case which documents this expected behavior.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 contrib/completion/git-completion.bash | 11 ++++++++++-
 t/t9902-completion.sh                  |  6 ++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

Show changes to 2 files +16 −1

contrib/completion/git-completion.bash, t/t9902-completion.sh

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c21786f2fd..08d3406cf3 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2223,9 +2223,18 @@ _git_switch ()
 		__gitcomp_builtin switch
 		;;
 	*)
+		local track_opt="--track" only_local_ref=n
+
+		# --orphan is used to create a branch disconnected from the
+		# current history, based on the empty tree. Since the only
+		# option required is the branch name, it doesn't make sense to
+		# complete anything here.
+		if [ -n "$(__git_find_on_cmdline "--orphan")" ]; then
+			return
+		fi
+
 		# check if --track, --no-track, or --no-guess was specified
 		# if so, disable DWIM mode
-		local track_opt="--track" only_local_ref=n
 		if [ "$GIT_COMPLETION_CHECKOUT_NO_GUESS" = "1" ] ||
 		   [ -n "$(__git_find_on_cmdline "--track --no-track --no-guess")" ]; then
 			track_opt=''
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index a134a87910..7e4dd8e722 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1351,6 +1351,12 @@ test_expect_failure 'git switch - with -C and --no-track, complete all reference
 	EOF
 '
 
+test_expect_success 'git switch --orphan completes no references' '
+	test_completion "git switch --orphan " <<-\EOF
+
+	EOF
+'
+
 test_expect_success 'teardown after ref completion' '
 	git branch -d matching-branch &&
 	git tag -d matching-tag &&

-- 
2.26.2-266-ge870325ee8

Jacob Keller· Apr 28, 2020, 17:32 UTC · re: Junio C Hamano · lore

Re: [PATCH 05/11] completion: add test showing subpar completion for git switch --orphan

On Tue, Apr 28, 2020 at 9:24 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 19 quoted lines

>
> Jacob Keller <jacob.keller@gmail.com> writes:
>
> > The proposed SQUASH you have in gitster/jk/complete-git-switch is
> > correct. The commit message body is correct, but the title could be
> > reworded to
> >
> > "completion: stop completing refs for git switch --orphan"
> >
> > I can send a v2 if that would be helpful, and I've got it fixed up
> > locally if other review increases the need for a new spin.
>
> Thnaks.  In the meantime, what I have is attached (the test title,
> in addition to the commit title, has been updated).
>
> The same logic would apply to "git checkout -b <TAB>" (or "git
> switch -c <TAB>") as this change: these are meant to create a new
> branch so you do not want to offer an existing branch name.
>

While true, you may optionally have a 2nd argument which provides a start point that can be an arbitrary reference.

Show 17 quoted lines

> I have a mixed feelings about that reasoning, though.  I understand
> that not offering any existing branch name would avoid offering a
> branch name that would cause an error message from the command,
> which at a first glance feels like a nice help to the user, but I am
> not sure if it is really helping.
>
> When you are on jk/complete-switch branch to work on this topic, you
> may want to keep the current state of the branch and use a "v2"
> branch to play around (running "rebase -i" etc.), for which the way
> I would hope to work would be:
>
>         git checkout -b jk/comp<TAB>
>
> that would complete to an existing branch
>
>         git checkout -b jk/complete-switch
>

Hmm.. Yes this would be useful.

Show 11 quoted lines

> and then I can just type "-v2<Enter>" (or "<BackSpace>-v2<Enter>" to
> remove the inter-word space completion adds?)  after that.  After
> all, "git checkout -b jk/complete-switch" in that scenario would
> error out by saying that you already use that name, which is a good
> enough protection.
>
> And that same "is this really helping?" reasoning applies equally to
> the "--orphan" option.
>
> I dunno.
>

Fair enough, new branches based on previous branches makes sense.

Thanks, Jake

Show 66 quoted lines

> -- >8 --
> From: Jacob Keller <jacob.keller@gmail.com>
> Date: Fri, 24 Apr 2020 19:20:38 -0700
> Subject: [PATCH] completion: stop completing refs for git switch --orphan
>
> git switch with the --orphan option is used to create a new branch that
> is not connected to any history and is instead based on the empty tree.
>
> It does not make sense for completion to return anything in this case,
> because there is nothing to complete. Check for --orphan, and if it's
> found, immediately return from _git_switch() without completing
> anything.
>
> Add a test case which documents this expected behavior.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  contrib/completion/git-completion.bash | 11 ++++++++++-
>  t/t9902-completion.sh                  |  6 ++++++
>  2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index c21786f2fd..08d3406cf3 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2223,9 +2223,18 @@ _git_switch ()
>                 __gitcomp_builtin switch
>                 ;;
>         *)
> +               local track_opt="--track" only_local_ref=n
> +
> +               # --orphan is used to create a branch disconnected from the
> +               # current history, based on the empty tree. Since the only
> +               # option required is the branch name, it doesn't make sense to
> +               # complete anything here.
> +               if [ -n "$(__git_find_on_cmdline "--orphan")" ]; then
> +                       return
> +               fi
> +
>                 # check if --track, --no-track, or --no-guess was specified
>                 # if so, disable DWIM mode
> -               local track_opt="--track" only_local_ref=n
>                 if [ "$GIT_COMPLETION_CHECKOUT_NO_GUESS" = "1" ] ||
>                    [ -n "$(__git_find_on_cmdline "--track --no-track --no-guess")" ]; then
>                         track_opt=''
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index a134a87910..7e4dd8e722 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -1351,6 +1351,12 @@ test_expect_failure 'git switch - with -C and --no-track, complete all reference
>         EOF
>  '
>
> +test_expect_success 'git switch --orphan completes no references' '
> +       test_completion "git switch --orphan " <<-\EOF
> +
> +       EOF
> +'
> +
>  test_expect_success 'teardown after ref completion' '
>         git branch -d matching-branch &&
>         git tag -d matching-tag &&
> --
> 2.26.2-266-ge870325ee8
>

Junio C Hamano· Apr 28, 2020, 18:10 UTC · re: Jacob Keller · lore

Re: [PATCH 05/11] completion: add test showing subpar completion for git switch --orphan

Jacob Keller <jacob.keller@gmail.com> writes:

Show 7 quoted lines

>> And that same "is this really helping?" reasoning applies equally to
>> the "--orphan" option.
>>
>> I dunno.
>>
>
> Fair enough, new branches based on previous branches makes sense.

I actually do not have a strong opinion either way. I just wanted to say that it would be good to make it consistent across "checkout -b", "switch -c" and "checkout/switch --orphan".

It would be nice if "checkout -B" and "switch -C" pair offered existing branches, as the intention of using the capital letter is clear---the user wants to overwrite an existing one.

On the other hand, I am OK if "checkout -b", "switch -c" and "--orphan" offered either:

 (1) nothing, as your patch does, or

 (2) all branches, except for the currently checked out one.

It would be preferrable if they did the same thing.

Thanks.

Jacob Keller· Apr 28, 2020, 18:45 UTC · re: Junio C Hamano · lore

Re: [PATCH 05/11] completion: add test showing subpar completion for git switch --orphan

On Tue, Apr 28, 2020 at 11:10 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 25 quoted lines

>
> Jacob Keller <jacob.keller@gmail.com> writes:
>
> >> And that same "is this really helping?" reasoning applies equally to
> >> the "--orphan" option.
> >>
> >> I dunno.
> >>
> >
> > Fair enough, new branches based on previous branches makes sense.
>
> I actually do not have a strong opinion either way.  I just wanted
> to say that it would be good to make it consistent across "checkout
> -b", "switch -c" and "checkout/switch --orphan".
>
> It would be nice if "checkout -B" and "switch -C" pair offered
> existing branches, as the intention of using the capital letter is
> clear---the user wants to overwrite an existing one.
>
> On the other hand, I am OK if "checkout -b", "switch -c" and
> "--orphan" offered either:
>
>  (1) nothing, as your patch does, or
>
>  (2) all branches, except for the currently checked out one.

Why reject the currently checked out one? If the premise is: "use a current branch to build a new branch name", I don't see a reason to restrict including the current branch here as well.

Show 5 quoted lines

>
> It would be preferrable if they did the same thing.
>
> Thanks.
>

The problem I see, is that regardless, I would like to see the following:

git switch -c new-branch-name <SPACE><TAB>

complete all references for the starting point.

With this series, that's handled by just checking for "-c/-C" on the command line and enabling all references. There's no positional checks done, so every word can complete to a reference.

I don't know offhand how to add completion which depends on the position of the word we're completing, so I'd have to investigate how to make that happen.

Thanks, Jake

Junio C Hamano· Apr 28, 2020, 19:16 UTC · re: Jacob Keller · lore

Re: [PATCH 05/11] completion: add test showing subpar completion for git switch --orphan

Jacob Keller <jacob.keller@gmail.com> writes:

Show 10 quoted lines

>> On the other hand, I am OK if "checkout -b", "switch -c" and
>> "--orphan" offered either:
>>
>>  (1) nothing, as your patch does, or
>>
>>  (2) all branches, except for the currently checked out one.
>
> Why reject the currently checked out one? If the premise is: "use a
> current branch to build a new branch name", I don't see a reason to
> restrict including the current branch here as well.

Yeah, "(3) all branches, without any exception" makes sense too.

>> It would be preferrable if they did the same thing.
>>
>> Thanks.

Show 5 quoted lines

> The problem I see, is that regardless, I would like to see the following:
>
> git switch -c new-branch-name <SPACE><TAB>
>
> complete all references for the starting point.

Makes sense.  The starting point could be any branch or even a tag.

Show 7 quoted lines

> With this series, that's handled by just checking for "-c/-C" on the
> command line and enabling all references. There's no positional checks
> done, so every word can complete to a reference.
>
> I don't know offhand how to add completion which depends on the
> position of the word we're completing, so I'd have to investigate how
> to make that happen.

I see. Even though I say "(3) all branches" is a reasonable behaviour, too, if the starting point has to be more (i.e. including tags), that would not help the issue you have here. It may not be too bad if we offered all refs (including tags, which may not be a good idea) in that case. I dunno.

Thanks.

Jacob Keller· Apr 28, 2020, 20:41 UTC · re: Junio C Hamano · lore

Re: [PATCH 05/11] completion: add test showing subpar completion for git switch --orphan

On Tue, Apr 28, 2020 at 12:16 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 18 quoted lines

>
> Jacob Keller <jacob.keller@gmail.com> writes:
> > With this series, that's handled by just checking for "-c/-C" on the
> > command line and enabling all references. There's no positional checks
> > done, so every word can complete to a reference.
> >
> > I don't know offhand how to add completion which depends on the
> > position of the word we're completing, so I'd have to investigate how
> > to make that happen.
>
> I see.  Even though I say "(3) all branches" is a reasonable
> behaviour, too, if the starting point has to be more (i.e. including
> tags), that would not help the issue you have here.  It may not be
> too bad if we offered all refs (including tags, which may not be a
> good idea) in that case.  I dunno.
>
> Thanks.
>

Basically, when creating a branch, you often do "git switch -c branch-name start-point"

branch-name would be the new branch name you want, and I think we should complete local branches for this, but start-point can be any reference, i.e. a tag, a local branch, or a remote reference.

I assumed completion would be most useful to complete the start-point, and so I opted to lean towards fixing completion for -c/-C to complete all references. However, I don't think the branch name should necessarily complete from all references and it would make sense to complete that portion only by local branch names.

I'm just not sure how best to implement that in our completion logic, and I would rather ensure that start-point completes properly, if we have to choose.

Thanks, Jake

Junio C Hamano· Apr 28, 2020, 20:57 UTC · re: Jacob Keller · lore

Re: [PATCH 05/11] completion: add test showing subpar completion for git switch --orphan

Jacob Keller <jacob.keller@gmail.com> writes:

Show 9 quoted lines

> I assumed completion would be most useful to complete the start-point,
> and so I opted to lean towards fixing completion for -c/-C to complete
> all references. However, I don't think the branch name should
> necessarily complete from all references and it would make sense to
> complete that portion only by local branch names.
>
> I'm just not sure how best to implement that in our completion logic,
> and I would rather ensure that start-point completes properly, if we
> have to choose.

Sure. What I was saying that if it is easier to implement by completing both new branch name and starting point from the same set, you could choose to include both branches and tags, and your "excuse" for choosing that design could be that a user who wants to fix something in 2.26 release may want the completion and typing progression go like so:

	$ git switch -c v2.2<TAB>
	... offers v2.20, v2.21, ..., v2.26 as candidates
	... complete to v2.26 and type "-frotz-fix"
	$ git switch -c v2.26-frotz-fix
	$ git switch -c v2.26-frotz-fix v2.2<TAB>
	... again offers v2.20, v2.21, ..., v2.26

;-)

Jacob Keller· Apr 25, 2020, 22:14 UTC · re: Jacob Keller · lore

Re: [PATCH 00/11] refactor git switch completion

On Fri, Apr 24, 2020 at 7:20 PM Jacob Keller <jacob.e.keller@intel.com> wrote:

> Following this are some patches to extract displaying DWIM remote branch
> names from __git_refs() and refactoring __git_complete_refs to take a mode
> argument that switches between calling __git_heads, __git_refs, and a new
> __git_remotes.

This should read __git_remote_heads, since I changed the name to be more descriptive to avoid thinking it prints the remote names on their own.

Thanks, Jake

Junio C Hamano· Apr 30, 2020, 22:56 UTC · re: Jacob Keller · lore

Re: [PATCH 00/11] refactor git switch completion

Jacob Keller <jacob.e.keller@intel.com> writes:

Show 12 quoted lines

> From: Jacob Keller <jacob.keller@gmail.com>
>
> completion support for git switch is subpar for a number of cases. Most
> notable is difference between these two completions:
>
>   $git switch <TAB>
>   Display all 784 possibilities? (y or n)
>   <list of all references and DWIM remotes>
>
>   $git switch --track<TAB>
>   jk-refactor-git-switch-completion master`
> ...

We've discussed that it may be a good idea to make sure that "switch -c", "checkout -b" and "switch/checkout --orphan" complete the new branch name the same way, but haven't done anything else. I'd very much appreciate to see the patches reviewed by those involved more in the completion script, before we decide to merge the topic to 'next'.

Thanks.

Jacob Keller· May 1, 2020, 21:53 UTC · re: Junio C Hamano · lore

Re: [PATCH 00/11] refactor git switch completion

On Thu, Apr 30, 2020 at 3:57 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 22 quoted lines

>
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
> > From: Jacob Keller <jacob.keller@gmail.com>
> >
> > completion support for git switch is subpar for a number of cases. Most
> > notable is difference between these two completions:
> >
> >   $git switch <TAB>
> >   Display all 784 possibilities? (y or n)
> >   <list of all references and DWIM remotes>
> >
> >   $git switch --track<TAB>
> >   jk-refactor-git-switch-completion master`
> > ...
>
> We've discussed that it may be a good idea to make sure that "switch
> -c", "checkout -b" and "switch/checkout --orphan" complete the new
> branch name the same way, but haven't done anything else.  I'd very
> much appreciate to see the patches reviewed by those involved more
> in the completion script, before we decide to merge the topic to
> 'next'.

It is my intention to extend this series with a notion of reading the previous word on the command line, and using that to determine if we are completing an argument for an option.

Thanks, Jake