[PATCH 0/6] completion: improve completion for 'git worktree'

[PATCH 0/6] completion: improve completion for 'git worktree'

[SZEDER Gábor]

Complete paths of working trees and refs for 'git worktree's various
subcommands.

The last two patches do the actual improvements, the first four are
preparatory steps.

An early version of the last patch was already sent to the list as a
PoC over four years ago [1], but I didn't like the part completing
options for 'git worktree add', and thus never seriously submitted it.
I still don't really like that part :), but it is really useful when
working with working trees, and at least other parts of the completion
function got simpler (thanks to automagically completing --options
via parse-options, and after the preparatory steps in this series).


[1] https://public-inbox.org/git/1440190256-21794-1-git-send-email-szeder@ira.uka.de/


SZEDER Gábor (6):
  t9902-completion: add tests for the __git_find_on_cmdline() helper
  completion: clean up the __git_find_on_cmdline() helper function
  completion: return the index of found word from
    __git_find_on_cmdline()
  completion: simplify completing 'git worktree' subcommands and options
  completion: list existing working trees for 'git worktree' subcommands
  completion: list paths and refs for 'git worktree add'

 contrib/completion/git-completion.bash | 117 +++++++++++++++++++------
 t/t9902-completion.sh                  |  57 ++++++++++++
 2 files changed, 148 insertions(+), 26 deletions(-)

-- 
2.23.0.1084.gae250eaa40

[PATCH 1/6] t9902-completion: add tests for the __git_find_on_cmdline() helper

[SZEDER Gábor]

The following two patches will refactor and extend the
__git_find_on_cmdline() helper function, so let's add a few tests
first to make sure that its basic behavior doesn't change.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t9902-completion.sh | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 54f8ce18cb..847ce601d2 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1363,6 +1363,34 @@ test_expect_success 'teardown after path completion tests' '
 	       BS\\dir '$'separators\034in\035dir''
 '
 
+test_expect_success '__git_find_on_cmdline - single match' '
+	echo list >expect &&
+	(
+		words=(git command --opt list) &&
+		cword=${#words[@]} &&
+		__git_find_on_cmdline "add list remove" >actual
+	) &&
+	test_cmp expect actual
+'
+
+test_expect_success '__git_find_on_cmdline - multiple matches' '
+	echo remove >expect &&
+	(
+		words=(git command -o --opt remove list add) &&
+		cword=${#words[@]} &&
+		__git_find_on_cmdline "add list remove" >actual
+	) &&
+	test_cmp expect actual
+'
+
+test_expect_success '__git_find_on_cmdline - no match' '
+	(
+		words=(git command --opt branch) &&
+		cword=${#words[@]} &&
+		__git_find_on_cmdline "add list remove" >actual
+	) &&
+	test_must_be_empty actual
+'
 
 test_expect_success '__git_get_config_variables' '
 	cat >expect <<-EOF &&
-- 
2.23.0.1084.gae250eaa40

[PATCH 2/6] completion: clean up the __git_find_on_cmdline() helper function

[SZEDER Gábor]

The __git_find_on_cmdline() helper function started its life as
__git_find_subcommand() [1], but it served a more general purpose than
looking for subcommands, so later it was renamed accordingly [2].
However, that rename didn't touch the body of the function, and left
the $subcommand local variable behind, still reminiscent of the
function's original purpose.

Let's clean up the names of __git_find_on_cmdline()'s local variables
and get rid of that $subcommand variable name.

While at it, add a short comment describing the function's purpose.

[1] 3ff1320d4b (bash: refactor searching for subcommands on the
    command line, 2008-03-10),
[2] 918c03c2a7 (bash: rename __git_find_subcommand() to
    __git_find_on_cmdline(), 2009-09-15)

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 00fbe6c03d..2384f91e78 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1070,14 +1070,17 @@ __git_aliased_command ()
 }
 
 # __git_find_on_cmdline requires 1 argument
+# Check whether one of the given words is present on the command line,
+# and print the first word found.
 __git_find_on_cmdline ()
 {
-	local word subcommand c=1
+	local word c=1
+	local wordlist="$1"
+
 	while [ $c -lt $cword ]; do
-		word="${words[c]}"
-		for subcommand in $1; do
-			if [ "$subcommand" = "$word" ]; then
-				echo "$subcommand"
+		for word in $wordlist; do
+			if [ "$word" = "${words[c]}" ]; then
+				echo "$word"
 				return
 			fi
 		done
-- 
2.23.0.1084.gae250eaa40

[PATCH 3/6] completion: return the index of found word from __git_find_on_cmdline()

[SZEDER Gábor]

When using the __git_find_on_cmdline() helper function so far we've
only been interested in which one of a set of words appear on the
command line.  To complete options for some of 'git worktree's
subcommands in the following patches we'll need not only that, but the
index of that word on the command line as well.

Extend __git_find_on_cmdline() to optionally show the index of the
found word on the command line (IOW in the $words array) when the
'--show-idx' option is given.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 20 +++++++++++++++---
 t/t9902-completion.sh                  | 29 ++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 2384f91e78..55a2d3e174 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1069,18 +1069,32 @@ __git_aliased_command ()
 	done
 }
 
-# __git_find_on_cmdline requires 1 argument
 # Check whether one of the given words is present on the command line,
 # and print the first word found.
+#
+# Usage: __git_find_on_cmdline [<option>]... "<wordlist>"
+# --show-idx: Optionally show the index of the found word in the $words array.
 __git_find_on_cmdline ()
 {
-	local word c=1
+	local word c=1 show_idx
+
+	while test $# -gt 1; do
+		case "$1" in
+		--show-idx)	show_idx=y ;;
+		*)		return 1 ;;
+		esac
+		shift
+	done
 	local wordlist="$1"
 
 	while [ $c -lt $cword ]; do
 		for word in $wordlist; do
 			if [ "$word" = "${words[c]}" ]; then
-				echo "$word"
+				if [ -n "$show_idx" ]; then
+					echo "$c $word"
+				else
+					echo "$word"
+				fi
 				return
 			fi
 		done
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 847ce601d2..3e3299819a 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1392,6 +1392,35 @@ test_expect_success '__git_find_on_cmdline - no match' '
 	test_must_be_empty actual
 '
 
+test_expect_success '__git_find_on_cmdline - single match with index' '
+	echo "3 list" >expect &&
+	(
+		words=(git command --opt list) &&
+		cword=${#words[@]} &&
+		__git_find_on_cmdline --show-idx "add list remove" >actual
+	) &&
+	test_cmp expect actual
+'
+
+test_expect_success '__git_find_on_cmdline - multiple matches with index' '
+	echo "4 remove" >expect &&
+	(
+		words=(git command -o --opt remove list add) &&
+		cword=${#words[@]} &&
+		__git_find_on_cmdline --show-idx "add list remove" >actual
+	) &&
+	test_cmp expect actual
+'
+
+test_expect_success '__git_find_on_cmdline - no match with index' '
+	(
+		words=(git command --opt branch) &&
+		cword=${#words[@]} &&
+		__git_find_on_cmdline --show-idx "add list remove" >actual
+	) &&
+	test_must_be_empty actual
+'
+
 test_expect_success '__git_get_config_variables' '
 	cat >expect <<-EOF &&
 	name-1
-- 
2.23.0.1084.gae250eaa40

[PATCH 4/6] completion: simplify completing 'git worktree' subcommands and options

[SZEDER Gábor]

The completion function for 'git worktree' uses separate but very
similar case arms to complete --options for each subcommand.

Combine these into a single case arm to avoid repetition.

Note that after this change we won't complete 'git worktree remove's
'--force' option, but that is consistent with our general stance on
not offering '--force', as it should be used with care.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 30 +++++++-------------------
 1 file changed, 8 insertions(+), 22 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 55a2d3e174..643272eb2f 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2985,29 +2985,15 @@ _git_worktree ()
 {
 	local subcommands="add list lock move prune remove unlock"
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
-	if [ -z "$subcommand" ]; then
+
+	case "$subcommand,$cur" in
+	,*)
 		__gitcomp "$subcommands"
-	else
-		case "$subcommand,$cur" in
-		add,--*)
-			__gitcomp_builtin worktree_add
-			;;
-		list,--*)
-			__gitcomp_builtin worktree_list
-			;;
-		lock,--*)
-			__gitcomp_builtin worktree_lock
-			;;
-		prune,--*)
-			__gitcomp_builtin worktree_prune
-			;;
-		remove,--*)
-			__gitcomp "--force"
-			;;
-		*)
-			;;
-		esac
-	fi
+		;;
+	*,--*)
+		__gitcomp_builtin worktree_$subcommand
+		;;
+	esac
 }
 
 __git_complete_common () {
-- 
2.23.0.1084.gae250eaa40

[PATCH 5/6] completion: list existing working trees for 'git worktree' subcommands

[SZEDER Gábor]

Complete the paths of existing working trees for 'git worktree's
'move', 'remove', 'lock', and 'unlock' subcommands.

Note that 'git worktree list --porcelain' shows absolute paths, so for
simplicity's sake we'll complete full absolute paths as well (as
opposed to turning them into relative paths by finding common leading
directories between $PWD and the working tree's path and removing
them, risking trouble with symbolic links or Windows drive letters; or
completing them one path component at a time).

Never list the path of the main working tree, as it cannot be moved,
removed, locked, or unlocked.

Arguably 'git worktree unlock <TAB>' should only complete locked
working trees, but 'git worktree list --porcelain' doesn't indicate
which working trees are locked.  So for now it will complete the paths
of all existing working trees, including non-locked ones as well.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 28 +++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 643272eb2f..4eb13b06d6 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2981,10 +2981,21 @@ _git_whatchanged ()
 	_git_log
 }
 
+__git_complete_worktree_paths ()
+{
+	local IFS=$'\n'
+	__gitcomp_nl "$(git worktree list --porcelain |
+		sed -n -e '2,$ s/^worktree //p')"
+}
+
 _git_worktree ()
 {
 	local subcommands="add list lock move prune remove unlock"
-	local subcommand="$(__git_find_on_cmdline "$subcommands")"
+	local subcommand subcommand_idx
+
+	subcommand="$(__git_find_on_cmdline --show-idx "$subcommands")"
+	subcommand_idx="${subcommand% *}"
+	subcommand="${subcommand#* }"
 
 	case "$subcommand,$cur" in
 	,*)
@@ -2993,6 +3004,21 @@ _git_worktree ()
 	*,--*)
 		__gitcomp_builtin worktree_$subcommand
 		;;
+	lock,*|remove,*|unlock,*)
+		__git_complete_worktree_paths
+		;;
+	move,*)
+		if [ $cword -eq $((subcommand_idx+1)) ]; then
+			# The first parameter must be an existing working
+			# tree to be moved.
+			__git_complete_worktree_paths
+		else
+			# The second parameter is the destination: it could
+			# be any path, so don't list anything, but let Bash
+			# fall back to filename completion.
+			:
+		fi
+		;;
 	esac
 }
 
-- 
2.23.0.1084.gae250eaa40

[PATCH 6/6] completion: list paths and refs for 'git worktree add'

[SZEDER Gábor]

Complete paths after 'git worktree add <TAB>' and refs after 'git
worktree add -b <TAB>' and 'git worktree add some/dir <TAB>'.

Uncharacteristically for a Git command, 'git worktree add' takes a
mandatory path parameter before a commit-ish as its optional last
parameter.  In addition, it has both standalone --options and options
with a mandatory unstuck parameter ('-b <new-branch>').  Consequently,
trying to complete refs for that last optional commit-ish parameter
resulted in a more convoluted than usual completion function, but
hopefully all the included comments will make it not too hard to
digest.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 36 ++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 4eb13b06d6..4e5cad4966 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3004,6 +3004,42 @@ _git_worktree ()
 	*,--*)
 		__gitcomp_builtin worktree_$subcommand
 		;;
+	add,*)	# usage: git worktree add [<options>] <path> [<commit-ish>]
+		# Here we are not completing an --option, it's either the
+		# path or a ref.
+		case "$prev" in
+		-b|-B)	# Complete refs for branch to be created/reseted.
+			__git_complete_refs
+			;;
+		-*)	# The previous word is an -o|--option without an
+			# unstuck argument: have to complete the path for
+			# the new worktree, so don't list anything, but let
+			# Bash fall back to filename completion.
+			;;
+		*)	# The previous word is not an --option, so it must
+			# be either the 'add' subcommand, the unstuck
+			# argument of an option (e.g. branch for -b|-B), or
+			# the path for the new worktree.
+			if [ $cword -eq $((subcommand_idx+1)) ]; then
+				# Right after the 'add' subcommand: have to
+				# complete the path, so fall back to Bash
+				# filename completion.
+				:
+			else
+				case "${words[cword-2]}" in
+				-b|-B)	# After '-b <branch>': have to
+					# complete the path, so fall back
+					# to Bash filename completion.
+					;;
+				*)	# After the path: have to complete
+					# the ref to be checked out.
+					__git_complete_refs
+					;;
+				esac
+			fi
+			;;
+		esac
+		;;
 	lock,*|remove,*|unlock,*)
 		__git_complete_worktree_paths
 		;;
-- 
2.23.0.1084.gae250eaa40

Re: [PATCH 3/6] completion: return the index of found word from __git_find_on_cmdline()

[Eric Sunshine]

On Thu, Oct 17, 2019 at 1:35 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> When using the __git_find_on_cmdline() helper function so far we've
> only been interested in which one of a set of words appear on the
> command line.  To complete options for some of 'git worktree's
> subcommands in the following patches we'll need not only that, but the
> index of that word on the command line as well.
>
> Extend __git_find_on_cmdline() to optionally show the index of the
> found word on the command line (IOW in the $words array) when the
> '--show-idx' option is given.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> @@ -1069,18 +1069,32 @@ __git_aliased_command ()
>  # Check whether one of the given words is present on the command line,
>  # and print the first word found.
> +#
> +# Usage: __git_find_on_cmdline [<option>]... "<wordlist>"
> +# --show-idx: Optionally show the index of the found word in the $words array.
>  __git_find_on_cmdline ()
>  {
> -       local word c=1
> +       local word c=1 show_idx
> +
> +       while test $# -gt 1; do
> +               case "$1" in
> +               --show-idx)     show_idx=y ;;
> +               *)              return 1 ;;

Should this emit an error message to aid a person debugging a test
which fails on a call to __git_find_on_cmdline()? For instance:

    echo "unrecognized option/argument: $1" >&2
    return 1
    ;;

or something...

> +               esac
> +               shift
> +       done
>         local wordlist="$1"
>
>         while [ $c -lt $cword ]; do
>                 for word in $wordlist; do
>                         if [ "$word" = "${words[c]}" ]; then
> -                               echo "$word"
> +                               if [ -n "$show_idx" ]; then
> +                                       echo "$c $word"
> +                               else
> +                                       echo "$word"
> +                               fi
>                                 return
>                         fi
>                 done

Re: [PATCH 5/6] completion: list existing working trees for 'git worktree' subcommands

[Eric Sunshine]

On Thu, Oct 17, 2019 at 1:35 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> Complete the paths of existing working trees for 'git worktree's
> 'move', 'remove', 'lock', and 'unlock' subcommands.
> [...]
> Arguably 'git worktree unlock <TAB>' should only complete locked
> working trees, but 'git worktree list --porcelain' doesn't indicate
> which working trees are locked.  So for now it will complete the paths
> of all existing working trees, including non-locked ones as well.

It is a long-standing To-Do[1] for "git worktree list [--porcelain]"
to indicate whether a worktree is locked, prunable, etc. Looking at
the implementation of builtin/worktree.c:show_worktree_porcelain(), it
should be easy enough to add. (Adding it for the non-porcelain case
would perhaps require more thinking and design since we might want a
--verbose option to trigger the extra information.)

[1]: https://public-inbox.org/git/CAPig+cTTrv2C7JLu1dr4+N8xo+7YQ+deiwLDA835wBGD6fhS1g@mail.gmail.com/

> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> @@ -2981,10 +2981,21 @@ _git_whatchanged ()
> +__git_complete_worktree_paths ()
> +{
> +       local IFS=$'\n'
> +       __gitcomp_nl "$(git worktree list --porcelain |
> +               sed -n -e '2,$ s/^worktree //p')"
> +}

I know that the commit message talks about it, but it might deserve an
in-code comment here (or a function-level comment) explaining why the
first line of "git worktree list --porcelain" output is thrown away
since it's not necessarily obvious to the casual reader.

Re: [PATCH 3/6] completion: return the index of found word from __git_find_on_cmdline()

[SZEDER Gábor]

On Thu, Oct 17, 2019 at 01:52:27PM -0400, Eric Sunshine wrote:
> >  __git_find_on_cmdline ()
> >  {
> > -       local word c=1
> > +       local word c=1 show_idx
> > +
> > +       while test $# -gt 1; do
> > +               case "$1" in
> > +               --show-idx)     show_idx=y ;;
> > +               *)              return 1 ;;
> 
> Should this emit an error message to aid a person debugging a test
> which fails on a call to __git_find_on_cmdline()? For instance:
> 
>     echo "unrecognized option/argument: $1" >&2
>     return 1
>     ;;
> 
> or something...

When debugging the completion script I frequently resort to 'echo >&2
"<msg>"', for lack of better options.  However, I intentionally did
not add an error message like that here, or in any similar option
parsing loops before, because due to a bug it might spew such a
message to standard error during regular completion (i.e. not during
debugging).

And printing anything to standard error during completion is
inherently bad: it disrupts the command line, can't be deleted (you
hit backspace, and in the terminal it looks as if the error message
was deleted, but in reality it's the command you've already entered
that gets deleted), and the user is eventually fored to Ctrl-C and
start over most of the time.  Well, at least I always end up hitting
Ctrl-C and start over.  Remaining silent about the unrecognized option
is in my opinion better, because then the completion script usually
does nothing, and Bash falls back to filename completion.  Yeah,
that's not ideal, but at least the user can easily correct it and
finish entering the command.

Re: [PATCH 5/6] completion: list existing working trees for 'git worktree' subcommands

[SZEDER Gábor]

On Thu, Oct 17, 2019 at 02:08:12PM -0400, Eric Sunshine wrote:
> On Thu, Oct 17, 2019 at 1:35 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> > Complete the paths of existing working trees for 'git worktree's
> > 'move', 'remove', 'lock', and 'unlock' subcommands.
> > [...]
> > Arguably 'git worktree unlock <TAB>' should only complete locked
> > working trees, but 'git worktree list --porcelain' doesn't indicate
> > which working trees are locked.  So for now it will complete the paths
> > of all existing working trees, including non-locked ones as well.
> 
> It is a long-standing To-Do[1] for "git worktree list [--porcelain]"
> to indicate whether a worktree is locked, prunable, etc. Looking at
> the implementation of builtin/worktree.c:show_worktree_porcelain(), it
> should be easy enough to add.

I didn't look at the implementation, but only at the docs, which says:

  --porcelain
      With list, output in an easy-to-parse format for scripts. This
      format will remain stable across Git versions and regardless of
      user configuration. See below for details.

I'm not sure whether introducing a new boolean attribute (i.e. a line
containing only "locked") would still be considered acceptable, or
would count as changing the format.  I can imagine that a too strict
parser would barf upon encountering the unrecognized "locked"
attribute; but yeah, no sensible parser should be that strict IMO.

Furthermore, I'm not sure what to do with the reason for locking.  In
general I would think that it makes sense to display the reason in an
easy-to-parse format as well.  However, doing so will inherently make
the format less easy to parse, because the reason could span multiple
lines, so without some sort of encoding/escaping it would violate the
"a line per attribute" format.

I would say that this is beyong the scope of this patch series :)

> > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> > @@ -2981,10 +2981,21 @@ _git_whatchanged ()
> > +__git_complete_worktree_paths ()
> > +{
> > +       local IFS=$'\n'
> > +       __gitcomp_nl "$(git worktree list --porcelain |
> > +               sed -n -e '2,$ s/^worktree //p')"
> > +}
> 
> I know that the commit message talks about it, but it might deserve an
> in-code comment

OK.

Re: [PATCH 5/6] completion: list existing working trees for 'git worktree' subcommands

[Eric Sunshine]

On Fri, Oct 18, 2019 at 11:00 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> On Thu, Oct 17, 2019 at 02:08:12PM -0400, Eric Sunshine wrote:
> > It is a long-standing To-Do[1] for "git worktree list [--porcelain]"
> > to indicate whether a worktree is locked, prunable, etc. Looking at
> > the implementation of builtin/worktree.c:show_worktree_porcelain(), it
> > should be easy enough to add.
>
> I didn't look at the implementation, but only at the docs, which says:
>
>   --porcelain
>       With list, output in an easy-to-parse format for scripts. This
>       format will remain stable across Git versions and regardless of
>       user configuration. See below for details.
>
> I'm not sure whether introducing a new boolean attribute (i.e. a line
> containing only "locked") would still be considered acceptable, or
> would count as changing the format.  I can imagine that a too strict
> parser would barf upon encountering the unrecognized "locked"
> attribute; but yeah, no sensible parser should be that strict IMO.

The stanza-based format of the porcelain output was chosen with the
specific intention of being easy to parse _and_ to allow extension
such as the introduction of new attributes. It is unfortunate that the
documentation you quoted, as well as the description of the porcelain
format itself, does a poor job of (or utterly fails at) conveying that
intention. (Had I been around to review the later iteration(s) of the
series which introduced the porcelain format, I likely would have
pointed out these shortcomings, however, Real Life had other ideas,
and I didn't manage to review it until weeks after the series had made
it into an actual release.)

I'm not convinced, though, that we're locked into exactly the few
attributes shown only in an example in the porcelain section of the
documentation. That documentation is so woefully underspecified --
indeed, it doesn't even talk about what attributes are available, but
only gives a single example showing some of the (possible) attributes
-- that it effectively makes no promises about what will or will not
appear in a stanza. (The only thing is says strongly is that stanzas
will be separated by a blank line -- despite the intention all along
being that each new stanza would start with a "worktree" attributes,
and that blank lines, if output, were to be ignored.)

So, I am of the opinion that we are not prevented from adding new
attributes, such as "locked" and "prunable".

> Furthermore, I'm not sure what to do with the reason for locking.  In
> general I would think that it makes sense to display the reason in an
> easy-to-parse format as well.  However, doing so will inherently make
> the format less easy to parse, because the reason could span multiple
> lines, so without some sort of encoding/escaping it would violate the
> "a line per attribute" format.

Yep, I believe my thinking at the time was that the lock reason and
the prunable reason would be escaped if needed. So, for instance:

    worktree /blah
    branch refs/heads/blah
    locked Sneaker-net removable storage\nNot always mounted

> I would say that this is beyong the scope of this patch series :)

Oh, I wasn't suggesting that this series do anything of the sort.
Instead, I was merely responding to the point in the commit message
that the porcelain format was lacking information about the lock, and
to say that it could (eventually) be solved by extending the output.

Re: [PATCH 3/6] completion: return the index of found word from __git_find_on_cmdline()

[Eric Sunshine]

On Fri, Oct 18, 2019 at 10:37 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> On Thu, Oct 17, 2019 at 01:52:27PM -0400, Eric Sunshine wrote:
> > > +               case "$1" in
> > > +               --show-idx)     show_idx=y ;;
> > > +               *)              return 1 ;;
> >
> > Should this emit an error message to aid a person debugging a test
> > which fails on a call to __git_find_on_cmdline()? [...]
>
> And printing anything to standard error during completion is
> inherently bad: it disrupts the command line, can't be deleted [...]
> Remaining silent about the unrecognized option
> is in my opinion better, because then the completion script usually
> does nothing, and Bash falls back to filename completion.  Yeah,
> that's not ideal, but at least the user can easily correct it and
> finish entering the command.

I had tunnel-vision and was thinking about this only in the context of
the tests. However, while I agree that spewing errors during
completion is not ideal, aren't there two different classes of errors
to consider? Some errors can crop up via normal usage of Git commands
in Real World situations; those errors should be suppressed since they
are expected and can be tolerated. However, the second class of error
(such as passing a bogus option to this internal function) is an
outright programming mistake by a maintainer of the completion script
itself, and it would be helpful to let the programmer know as early as
possible about the mistake.

Or, are there backward-compatibility or other concerns which would
make emitting error messages undesirable even for outright programmer
mistakes?

Re: [PATCH 3/6] completion: return the index of found word from __git_find_on_cmdline()

[SZEDER Gábor]

On Fri, Oct 18, 2019 at 05:01:42PM -0400, Eric Sunshine wrote:
> On Fri, Oct 18, 2019 at 10:37 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> > On Thu, Oct 17, 2019 at 01:52:27PM -0400, Eric Sunshine wrote:
> > > > +               case "$1" in
> > > > +               --show-idx)     show_idx=y ;;
> > > > +               *)              return 1 ;;
> > >
> > > Should this emit an error message to aid a person debugging a test
> > > which fails on a call to __git_find_on_cmdline()? [...]
> >
> > And printing anything to standard error during completion is
> > inherently bad: it disrupts the command line, can't be deleted [...]
> > Remaining silent about the unrecognized option
> > is in my opinion better, because then the completion script usually
> > does nothing, and Bash falls back to filename completion.  Yeah,
> > that's not ideal, but at least the user can easily correct it and
> > finish entering the command.
> 
> I had tunnel-vision and was thinking about this only in the context of
> the tests. However, while I agree that spewing errors during
> completion is not ideal, aren't there two different classes of errors
> to consider? Some errors can crop up via normal usage of Git commands
> in Real World situations; those errors should be suppressed since they
> are expected and can be tolerated. However, the second class of error
> (such as passing a bogus option to this internal function) is an
> outright programming mistake by a maintainer of the completion script
> itself, and it would be helpful to let the programmer know as early as
> possible about the mistake.
> 
> Or, are there backward-compatibility or other concerns which would
> make emitting error messages undesirable even for outright programmer
> mistakes?

It's not necessarily an outright programming mistake, and that error
could be triggered by ordinary users as well.

Let's suppose that a user has a custom 'git-foo' command in $PATH with
a custom '_git_foo' completion function in '~/.my-git-completions',
which the helper function '__git_bar --option' from our completion
script.  Let's also suppose that the user sources this completion
function from '~/.bashrc', but otherwise uses the system-wide git
completion script, and that $HOME is shared across multiple computers.

In this (arguably somewhat convoluted) scenario it might happen that
on a not quote up-to-date computer the system-wide git completion
script already has the '__git_bar' helper function, but it doesn't yet
support '--option'.  If '__git_bar' then prints an error to stderr,
then the command line will get badly messed up, and the user will have
to ctrl-C and start over.

However, if '__git_bar' silently ignores the unknown option, then the
worst that can happen is that completion doesn't work, and e.g. it
falls back to Bash's filename completion or offers something
nonsensical.  In either case, after a brief "Huh?!" moment the user
can correct it by hitting backspace a couple of times and then enter
the rest of the command by hand.

Re: [PATCH 3/6] completion: return the index of found word from __git_find_on_cmdline()

[SZEDER Gábor]

On Fri, Oct 18, 2019 at 04:37:28PM +0200, SZEDER Gábor wrote:
> On Thu, Oct 17, 2019 at 01:52:27PM -0400, Eric Sunshine wrote:
> > >  __git_find_on_cmdline ()
> > >  {
> > > -       local word c=1
> > > +       local word c=1 show_idx
> > > +
> > > +       while test $# -gt 1; do
> > > +               case "$1" in
> > > +               --show-idx)     show_idx=y ;;
> > > +               *)              return 1 ;;
> > 
> > Should this emit an error message to aid a person debugging a test
> > which fails on a call to __git_find_on_cmdline()? For instance:
> > 
> >     echo "unrecognized option/argument: $1" >&2
> >     return 1
> >     ;;
> > 
> > or something...
> 
> When debugging the completion script I frequently resort to 'echo >&2
> "<msg>"', for lack of better options.

Well, there is a better option for debugging: adding 'echo >>~/LOG
"<msg>"' to the completion script and running 'tail -f ~/LOG' in a
separate terminal window is so much more convenient than screwing up
the command line with those messages to stderr every time.

I just wonder why it took me about a dozen years to figure this one
out... ;)

[PATCH v2 0/6] completion: improve completion for 'git worktree'

[SZEDER Gábor]

Complete paths of working trees and refs for 'git worktree's various
subcommands.

The last two patches do the actual improvements, the first four are
preparatory steps.

Changes since v1:

  - Added an in-code comment and clarified the commit message of patch
    5/6.

v1: https://public-inbox.org/git/20191017173501.3198-1-szeder.dev@gmail.com/T/#u

SZEDER Gábor (6):
  t9902-completion: add tests for the __git_find_on_cmdline() helper
  completion: clean up the __git_find_on_cmdline() helper function
  completion: return the index of found word from
    __git_find_on_cmdline()
  completion: simplify completing 'git worktree' subcommands and options
  completion: list existing working trees for 'git worktree' subcommands
  completion: list paths and refs for 'git worktree add'

 contrib/completion/git-completion.bash | 119 +++++++++++++++++++------
 t/t9902-completion.sh                  |  57 ++++++++++++
 2 files changed, 150 insertions(+), 26 deletions(-)

Range-diff against v1:
1:  bd68b0b64b = 1:  b370a97edc t9902-completion: add tests for the __git_find_on_cmdline() helper
2:  b5c0312db6 = 2:  fb8f1e4840 completion: clean up the __git_find_on_cmdline() helper function
3:  6999b0b8e2 = 3:  47aecb9ac0 completion: return the index of found word from __git_find_on_cmdline()
4:  dd38374f2f = 4:  00c5a0f7df completion: simplify completing 'git worktree' subcommands and options
5:  619ff7021c ! 5:  2e4b1b3931 completion: list existing working trees for 'git worktree' subcommands
    @@ Commit message
         Never list the path of the main working tree, as it cannot be moved,
         removed, locked, or unlocked.
     
    -    Arguably 'git worktree unlock <TAB>' should only complete locked
    -    working trees, but 'git worktree list --porcelain' doesn't indicate
    -    which working trees are locked.  So for now it will complete the paths
    -    of all existing working trees, including non-locked ones as well.
    +    Ideally we would only list unlocked working trees for the 'move',
    +    'remove', and 'lock' subcommands, and only locked ones for 'unlock'.
    +    Alas, 'git worktree list --porcelain' doesn't indicate which working
    +    trees are locked, so for now we'll complete the paths of all existing
    +    working trees.
     
         Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
     
    @@ contrib/completion/git-completion.bash: _git_whatchanged ()
     +{
     +	local IFS=$'\n'
     +	__gitcomp_nl "$(git worktree list --porcelain |
    ++		# Skip the first entry: it's the path of the main worktree,
    ++		# which can't be moved, removed, locked, etc.
     +		sed -n -e '2,$ s/^worktree //p')"
     +}
     +
6:  29a32e5d36 = 6:  16a9b5f4b2 completion: list paths and refs for 'git worktree add'
-- 
2.24.1.982.ga4d4aba446

[PATCH v2 1/6] t9902-completion: add tests for the __git_find_on_cmdline() helper

[SZEDER Gábor]

The following two patches will refactor and extend the
__git_find_on_cmdline() helper function, so let's add a few tests
first to make sure that its basic behavior doesn't change.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t9902-completion.sh | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 54f8ce18cb..847ce601d2 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1363,6 +1363,34 @@ test_expect_success 'teardown after path completion tests' '
 	       BS\\dir '$'separators\034in\035dir''
 '
 
+test_expect_success '__git_find_on_cmdline - single match' '
+	echo list >expect &&
+	(
+		words=(git command --opt list) &&
+		cword=${#words[@]} &&
+		__git_find_on_cmdline "add list remove" >actual
+	) &&
+	test_cmp expect actual
+'
+
+test_expect_success '__git_find_on_cmdline - multiple matches' '
+	echo remove >expect &&
+	(
+		words=(git command -o --opt remove list add) &&
+		cword=${#words[@]} &&
+		__git_find_on_cmdline "add list remove" >actual
+	) &&
+	test_cmp expect actual
+'
+
+test_expect_success '__git_find_on_cmdline - no match' '
+	(
+		words=(git command --opt branch) &&
+		cword=${#words[@]} &&
+		__git_find_on_cmdline "add list remove" >actual
+	) &&
+	test_must_be_empty actual
+'
 
 test_expect_success '__git_get_config_variables' '
 	cat >expect <<-EOF &&
-- 
2.24.1.982.ga4d4aba446

[PATCH v2 2/6] completion: clean up the __git_find_on_cmdline() helper function

[SZEDER Gábor]

The __git_find_on_cmdline() helper function started its life as
__git_find_subcommand() [1], but it served a more general purpose than
looking for subcommands, so later it was renamed accordingly [2].
However, that rename didn't touch the body of the function, and left
the $subcommand local variable behind, still reminiscent of the
function's original purpose.

Let's clean up the names of __git_find_on_cmdline()'s local variables
and get rid of that $subcommand variable name.

While at it, add a short comment describing the function's purpose.

[1] 3ff1320d4b (bash: refactor searching for subcommands on the
    command line, 2008-03-10),
[2] 918c03c2a7 (bash: rename __git_find_subcommand() to
    __git_find_on_cmdline(), 2009-09-15)

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 00fbe6c03d..2384f91e78 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1070,14 +1070,17 @@ __git_aliased_command ()
 }
 
 # __git_find_on_cmdline requires 1 argument
+# Check whether one of the given words is present on the command line,
+# and print the first word found.
 __git_find_on_cmdline ()
 {
-	local word subcommand c=1
+	local word c=1
+	local wordlist="$1"
+
 	while [ $c -lt $cword ]; do
-		word="${words[c]}"
-		for subcommand in $1; do
-			if [ "$subcommand" = "$word" ]; then
-				echo "$subcommand"
+		for word in $wordlist; do
+			if [ "$word" = "${words[c]}" ]; then
+				echo "$word"
 				return
 			fi
 		done
-- 
2.24.1.982.ga4d4aba446

[PATCH v2 3/6] completion: return the index of found word from __git_find_on_cmdline()

[SZEDER Gábor]

When using the __git_find_on_cmdline() helper function so far we've
only been interested in which one of a set of words appear on the
command line.  To complete options for some of 'git worktree's
subcommands in the following patches we'll need not only that, but the
index of that word on the command line as well.

Extend __git_find_on_cmdline() to optionally show the index of the
found word on the command line (IOW in the $words array) when the
'--show-idx' option is given.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 20 +++++++++++++++---
 t/t9902-completion.sh                  | 29 ++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 2384f91e78..55a2d3e174 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1069,18 +1069,32 @@ __git_aliased_command ()
 	done
 }
 
-# __git_find_on_cmdline requires 1 argument
 # Check whether one of the given words is present on the command line,
 # and print the first word found.
+#
+# Usage: __git_find_on_cmdline [<option>]... "<wordlist>"
+# --show-idx: Optionally show the index of the found word in the $words array.
 __git_find_on_cmdline ()
 {
-	local word c=1
+	local word c=1 show_idx
+
+	while test $# -gt 1; do
+		case "$1" in
+		--show-idx)	show_idx=y ;;
+		*)		return 1 ;;
+		esac
+		shift
+	done
 	local wordlist="$1"
 
 	while [ $c -lt $cword ]; do
 		for word in $wordlist; do
 			if [ "$word" = "${words[c]}" ]; then
-				echo "$word"
+				if [ -n "$show_idx" ]; then
+					echo "$c $word"
+				else
+					echo "$word"
+				fi
 				return
 			fi
 		done
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 847ce601d2..3e3299819a 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1392,6 +1392,35 @@ test_expect_success '__git_find_on_cmdline - no match' '
 	test_must_be_empty actual
 '
 
+test_expect_success '__git_find_on_cmdline - single match with index' '
+	echo "3 list" >expect &&
+	(
+		words=(git command --opt list) &&
+		cword=${#words[@]} &&
+		__git_find_on_cmdline --show-idx "add list remove" >actual
+	) &&
+	test_cmp expect actual
+'
+
+test_expect_success '__git_find_on_cmdline - multiple matches with index' '
+	echo "4 remove" >expect &&
+	(
+		words=(git command -o --opt remove list add) &&
+		cword=${#words[@]} &&
+		__git_find_on_cmdline --show-idx "add list remove" >actual
+	) &&
+	test_cmp expect actual
+'
+
+test_expect_success '__git_find_on_cmdline - no match with index' '
+	(
+		words=(git command --opt branch) &&
+		cword=${#words[@]} &&
+		__git_find_on_cmdline --show-idx "add list remove" >actual
+	) &&
+	test_must_be_empty actual
+'
+
 test_expect_success '__git_get_config_variables' '
 	cat >expect <<-EOF &&
 	name-1
-- 
2.24.1.982.ga4d4aba446

[PATCH v2 4/6] completion: simplify completing 'git worktree' subcommands and options

[SZEDER Gábor]

The completion function for 'git worktree' uses separate but very
similar case arms to complete --options for each subcommand.

Combine these into a single case arm to avoid repetition.

Note that after this change we won't complete 'git worktree remove's
'--force' option, but that is consistent with our general stance on
not offering '--force', as it should be used with care.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 30 +++++++-------------------
 1 file changed, 8 insertions(+), 22 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 55a2d3e174..643272eb2f 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2985,29 +2985,15 @@ _git_worktree ()
 {
 	local subcommands="add list lock move prune remove unlock"
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
-	if [ -z "$subcommand" ]; then
+
+	case "$subcommand,$cur" in
+	,*)
 		__gitcomp "$subcommands"
-	else
-		case "$subcommand,$cur" in
-		add,--*)
-			__gitcomp_builtin worktree_add
-			;;
-		list,--*)
-			__gitcomp_builtin worktree_list
-			;;
-		lock,--*)
-			__gitcomp_builtin worktree_lock
-			;;
-		prune,--*)
-			__gitcomp_builtin worktree_prune
-			;;
-		remove,--*)
-			__gitcomp "--force"
-			;;
-		*)
-			;;
-		esac
-	fi
+		;;
+	*,--*)
+		__gitcomp_builtin worktree_$subcommand
+		;;
+	esac
 }
 
 __git_complete_common () {
-- 
2.24.1.982.ga4d4aba446

[PATCH v2 5/6] completion: list existing working trees for 'git worktree' subcommands

[SZEDER Gábor]

Complete the paths of existing working trees for 'git worktree's
'move', 'remove', 'lock', and 'unlock' subcommands.

Note that 'git worktree list --porcelain' shows absolute paths, so for
simplicity's sake we'll complete full absolute paths as well (as
opposed to turning them into relative paths by finding common leading
directories between $PWD and the working tree's path and removing
them, risking trouble with symbolic links or Windows drive letters; or
completing them one path component at a time).

Never list the path of the main working tree, as it cannot be moved,
removed, locked, or unlocked.

Ideally we would only list unlocked working trees for the 'move',
'remove', and 'lock' subcommands, and only locked ones for 'unlock'.
Alas, 'git worktree list --porcelain' doesn't indicate which working
trees are locked, so for now we'll complete the paths of all existing
working trees.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 30 +++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 643272eb2f..5eae0bfd18 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2981,10 +2981,23 @@ _git_whatchanged ()
 	_git_log
 }
 
+__git_complete_worktree_paths ()
+{
+	local IFS=$'\n'
+	__gitcomp_nl "$(git worktree list --porcelain |
+		# Skip the first entry: it's the path of the main worktree,
+		# which can't be moved, removed, locked, etc.
+		sed -n -e '2,$ s/^worktree //p')"
+}
+
 _git_worktree ()
 {
 	local subcommands="add list lock move prune remove unlock"
-	local subcommand="$(__git_find_on_cmdline "$subcommands")"
+	local subcommand subcommand_idx
+
+	subcommand="$(__git_find_on_cmdline --show-idx "$subcommands")"
+	subcommand_idx="${subcommand% *}"
+	subcommand="${subcommand#* }"
 
 	case "$subcommand,$cur" in
 	,*)
@@ -2993,6 +3006,21 @@ _git_worktree ()
 	*,--*)
 		__gitcomp_builtin worktree_$subcommand
 		;;
+	lock,*|remove,*|unlock,*)
+		__git_complete_worktree_paths
+		;;
+	move,*)
+		if [ $cword -eq $((subcommand_idx+1)) ]; then
+			# The first parameter must be an existing working
+			# tree to be moved.
+			__git_complete_worktree_paths
+		else
+			# The second parameter is the destination: it could
+			# be any path, so don't list anything, but let Bash
+			# fall back to filename completion.
+			:
+		fi
+		;;
 	esac
 }
 
-- 
2.24.1.982.ga4d4aba446

[PATCH v2 6/6] completion: list paths and refs for 'git worktree add'

[SZEDER Gábor]

Complete paths after 'git worktree add <TAB>' and refs after 'git
worktree add -b <TAB>' and 'git worktree add some/dir <TAB>'.

Uncharacteristically for a Git command, 'git worktree add' takes a
mandatory path parameter before a commit-ish as its optional last
parameter.  In addition, it has both standalone --options and options
with a mandatory unstuck parameter ('-b <new-branch>').  Consequently,
trying to complete refs for that last optional commit-ish parameter
resulted in a more convoluted than usual completion function, but
hopefully all the included comments will make it not too hard to
digest.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 36 ++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 5eae0bfd18..0b163e2c59 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3006,6 +3006,42 @@ _git_worktree ()
 	*,--*)
 		__gitcomp_builtin worktree_$subcommand
 		;;
+	add,*)	# usage: git worktree add [<options>] <path> [<commit-ish>]
+		# Here we are not completing an --option, it's either the
+		# path or a ref.
+		case "$prev" in
+		-b|-B)	# Complete refs for branch to be created/reseted.
+			__git_complete_refs
+			;;
+		-*)	# The previous word is an -o|--option without an
+			# unstuck argument: have to complete the path for
+			# the new worktree, so don't list anything, but let
+			# Bash fall back to filename completion.
+			;;
+		*)	# The previous word is not an --option, so it must
+			# be either the 'add' subcommand, the unstuck
+			# argument of an option (e.g. branch for -b|-B), or
+			# the path for the new worktree.
+			if [ $cword -eq $((subcommand_idx+1)) ]; then
+				# Right after the 'add' subcommand: have to
+				# complete the path, so fall back to Bash
+				# filename completion.
+				:
+			else
+				case "${words[cword-2]}" in
+				-b|-B)	# After '-b <branch>': have to
+					# complete the path, so fall back
+					# to Bash filename completion.
+					;;
+				*)	# After the path: have to complete
+					# the ref to be checked out.
+					__git_complete_refs
+					;;
+				esac
+			fi
+			;;
+		esac
+		;;
 	lock,*|remove,*|unlock,*)
 		__git_complete_worktree_paths
 		;;
-- 
2.24.1.982.ga4d4aba446