[PATCH] completion: complete modified files for checkout with '--'

From: Cornelius Weig <redacted>

The command line completion for git-checkout bails out when seeing '--'
as an isolated argument. For git-checkout this signifies the start of a
list of files which are to be checked out. Checkout of files makes only
sense for modified files, therefore completion can be a bit smarter:
Instead of bailing out, offer modified files for completion.

Signed-off-by: Cornelius Weig <redacted>
---
 contrib/completion/git-completion.bash | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6c6e1c7..d6523fd 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash

@@ -1059,7 +1059,10 @@ _git_bundle ()
 
 _git_checkout ()
 {
-	__git_has_doubledash && return
+	__git_has_doubledash && {
+		__git_complete_index_file "--modified"
+		return
+	}
 
 	case "$cur" in
 	--conflict=*)

-- 
2.10.2

On Tue, Feb 14, 2017 at 12:33 AM,  [off-list ref] wrote:

From: Cornelius Weig <redacted>

The command line completion for git-checkout bails out when seeing '--'
as an isolated argument. For git-checkout this signifies the start of a
list of files which are to be checked out. Checkout of files makes only
sense for modified files,

No, there is e.g. 'git checkout that-branch this-path', too.

On 02/14/2017 01:50 AM, SZEDER Gábor wrote:

On Tue, Feb 14, 2017 at 12:33 AM,  [off-list ref] wrote:

quoted

From: Cornelius Weig <redacted>

The command line completion for git-checkout bails out when seeing '--'
as an isolated argument. For git-checkout this signifies the start of a
list of files which are to be checked out. Checkout of files makes only
sense for modified files,

No, there is e.g. 'git checkout that-branch this-path', too.

Very true. Thanks for prodding me to this palpable oversight.

My error was to aim for a small improvement. I think the correct
approach is to improve the overall completion of git-checkout. IMHO it
is a completion bug that after giving a ref, completion will still offer
refs, e.g.
$ git checkout HEAD <TAB><TAB> --> list of refs

As far as I can see, giving two refs to checkout is always an error. The
correct behavior in the example above would be to offer paths instead.

I'll follow up with an improved version which considers these cases.

From: Cornelius Weig <redacted>

Git-checkout completes words starting with '--' as options and other
words as refs. Even after specifying a ref, further words not starting
with '--' are completed as refs, which is invalid for git-checkout.

This commit ensures that after specifying a ref, further non-option
words are completed as paths. Four cases are considered:

 - If the word contains a ':', do not treat it as reference and use
   regular revlist completion.
 - If no ref is found on the command line, complete non-options as refs
   as before.
 - If the ref is HEAD or @, complete only with modified files because
   checking out unmodified files is a noop.
   This case also applies if no ref is given, but '--' is present.
 - If a ref other than HEAD or @ is found, offer only valid paths from
   that revision.

Note that one corner-case is not covered by the current implementation:
if a refname contains a ':' and is followed by '--' the completion would
not recognize the valid refname.

Signed-off-by: Cornelius Weig <redacted>
---
 contrib/completion/git-completion.bash | 39 +++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 4ab119d..df46f62 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash

@@ -1068,7 +1068,7 @@ _git_bundle ()
 
 _git_checkout ()
 {
-	__git_has_doubledash && return
+	local i c=2 ref="" seen_double_dash=""
 
 	case "$cur" in
 	--conflict=*)

@@ -1081,13 +1081,36 @@ _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=1
-		if [ -n "$(__git_find_on_cmdline "$flags")" ]; then
-			track=''
-		fi
-		__gitcomp_nl "$(__git_refs '' $track)"
+		while [ $c -lt $cword ]; do
+			i="${words[c]}"
+			case "$i" in
+			--) seen_double_dash=1 ;;
+			-*|?*:*) ;;
+			*) ref="$i"; break ;;
+			esac
+			((c++))
+		done
+
+		case "$ref,$seen_double_dash,$cur" in
+		,,*:*)
+		    __git_complete_revlist_file
+		    ;;
+		,,*)
+			# check for --track, --no-track, or --no-guess
+			# if so, disable DWIM mode
+			local flags="--track --no-track --no-guess" track=1
+			if [ -n "$(__git_find_on_cmdline "$flags")" ]; then
+				track=''
+			fi
+			__gitcomp_nl "$(__git_refs '' $track)"
+			;;
+		,1,*|@,*|HEAD,*)
+			__git_complete_index_file "--modified"
+			;;
+		*)
+			__git_complete_tree_file "$ref" "$cur"
+			;;
+		esac
 		;;
 	esac
 }

-- 
2.10.2

From: Cornelius Weig <redacted>

The function __git_complete_revlist_file understands how to complete a
path such as 'topic:ref<TAB>'. In that case, the revision (topic) and
the path component (ref) are both part of the same word. However,
some cases require that the revision is specified elsewhere than the
current word for completion, such as 'git checkout topic ref<TAB>'.

In order to allow callers to specify the revision, extract a utility
function to complete paths from a tree-ish object. The utility will be
used later to implement path completion for git-checkout.

Signed-off-by: Cornelius Weig <redacted>
---
 contrib/completion/git-completion.bash | 73 +++++++++++++++++++---------------
 1 file changed, 41 insertions(+), 32 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6c6e1c7..4ab119d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash

@@ -442,6 +442,46 @@ __git_compute_merge_strategies ()
 	__git_merge_strategies=$(__git_list_merge_strategies)
 }
 
+# __git_complete_tree_file requires 2 argument:
+# 1: the the tree-like to look at for completion
+# 2: the path component to complete
+__git_complete_tree_file ()
+{
+	local pfx ls ref="$1" cur_="$2"
+	case "$cur_" in
+	?*/*)
+		pfx="${cur_%/*}"
+		cur_="${cur_##*/}"
+		ls="$ref:$pfx"
+		pfx="$pfx/"
+		;;
+	*)
+		ls="$ref"
+		;;
+	esac
+
+	case "$COMP_WORDBREAKS" in
+	*:*) : great ;;
+	*)   pfx="$ref:$pfx" ;;
+	esac
+
+	__gitcomp_nl "$(git --git-dir="$(__gitdir)" ls-tree "$ls" 2>/dev/null \
+				| sed '/^100... blob /{
+						   s,^.*	,,
+						   s,$, ,
+					   }
+					   /^120000 blob /{
+						   s,^.*	,,
+						   s,$, ,
+					   }
+					   /^040000 tree /{
+						   s,^.*	,,
+						   s,$,/,
+					   }
+					   s/^.*	//')" \
+				"$pfx" "$cur_" ""
+}
+
 __git_complete_revlist_file ()
 {
 	local pfx ls ref cur_="$cur"

@@ -452,38 +492,7 @@ __git_complete_revlist_file ()
 	?*:*)
 		ref="${cur_%%:*}"
 		cur_="${cur_#*:}"
-		case "$cur_" in
-		?*/*)
-			pfx="${cur_%/*}"
-			cur_="${cur_##*/}"
-			ls="$ref:$pfx"
-			pfx="$pfx/"
-			;;
-		*)
-			ls="$ref"
-			;;
-		esac
-
-		case "$COMP_WORDBREAKS" in
-		*:*) : great ;;
-		*)   pfx="$ref:$pfx" ;;
-		esac
-
-		__gitcomp_nl "$(git --git-dir="$(__gitdir)" ls-tree "$ls" 2>/dev/null \
-				| sed '/^100... blob /{
-				           s,^.*	,,
-				           s,$, ,
-				       }
-				       /^120000 blob /{
-				           s,^.*	,,
-				           s,$, ,
-				       }
-				       /^040000 tree /{
-				           s,^.*	,,
-				           s,$,/,
-				       }
-				       s/^.*	//')" \
-			"$pfx" "$cur_" ""
+		__git_complete_tree_file "$ref" "$cur_"
 		;;
 	*...*)
 		pfx="${cur_%...*}..."

-- 
2.10.2

On Tue, Feb 14, 2017 at 10:24 PM,  [off-list ref] wrote:

From: Cornelius Weig <redacted>

Git-checkout completes words starting with '--' as options and other
words as refs. Even after specifying a ref, further words not starting
with '--' are completed as refs, which is invalid for git-checkout.

Refs completion is never attempted for words after the disambiguating
double-dash.

Even when refs completion is attempted, if it is unsuccessful, i.e.
there is no ref that matches the current word to be completed, then
Bash falls back to standard filename completion.  No refs match
'./<TAB>'.

Furthermore, Bash performs filename completion on Alt-/ independently
from any completion function.

Granted, none of these will limit to only modified files...  But that
might be a good thing, see below.

This commit ensures that after specifying a ref, further non-option
words are completed as paths. Four cases are considered:

 - If the word contains a ':', do not treat it as reference and use
   regular revlist completion.
 - If no ref is found on the command line, complete non-options as refs
   as before.
 - If the ref is HEAD or @, complete only with modified files because
   checking out unmodified files is a noop.

Here you use "modified" in the 'ls-files --modified' sense, but that
doesn't include modifications already staged in the index, see below.

   This case also applies if no ref is given, but '--' is present.
 - If a ref other than HEAD or @ is found, offer only valid paths from
   that revision.

Note that one corner-case is not covered by the current implementation:
if a refname contains a ':' and is followed by '--' the completion would
not recognize the valid refname.

I'm not sure what you mean here.  Refnames can't contain ':'.

I haven't tried it, but this would trigger on e.g. 'git checkout -b
new-feature <TAB>', wouldn't it?

+                       esac
+                       ((c++))
+               done
+
+               case "$ref,$seen_double_dash,$cur" in
+               ,,*:*)
+                   __git_complete_revlist_file
+                   ;;
+               ,,*)
+                       # check for --track, --no-track, or --no-guess
+                       # if so, disable DWIM mode
+                       local flags="--track --no-track --no-guess" track=1
+                       if [ -n "$(__git_find_on_cmdline "$flags")" ]; then
+                               track=''
+                       fi
+                       __gitcomp_nl "$(__git_refs '' $track)"
+                       ;;
+               ,1,*|@,*|HEAD,*)
+                       __git_complete_index_file "--modified"

What about

  $ echo "unintentional change" >>tracked-file && git add -u
  $ git rm important-file
  $ git checkout HEAD <TAB>

?  It seems it will offer neither 'tracked-file' nor 'important-file',
but I think it should offer both.


We have __git_complete_index_file() for a while now, but only use it
for commands that accept only --options and filenames, e.g. 'add',
'clean', 'rm'.  This would be the first case when we would use it for
a command that accept both refs and filenames.  Perhaps similar corner
cases and the easy ways to trigger filename completion are why no one
thought it's worth it.

+                       ;;
+               *)
+                       __git_complete_tree_file "$ref" "$cur"

Well, here you could go all-in, and say that this should complete only
files that are different from the version in $ref, because checking
out files that are still the same is a noop :)

+                       ;;
+               esac
                ;;
        esac
 }
--
2.10.2

Although I'm not convinced that completion of modified files is unnecessary, I'm at least persuaded that not all users would welcome such a change. Given the hint from Gabor that Alt-/ forces filesystem completion, there is even no big win in stopping to offer further refnames after one has already been given.

If you think that this would be desirable, find a revised version below. Otherwise drop it.


On 02/15/2017 04:11 AM, SZEDER Gábor wrote:

On Tue, Feb 14, 2017 at 10:24 PM,  [off-list ref] wrote:

quoted

From: Cornelius Weig <redacted>
Note that one corner-case is not covered by the current implementation:
if a refname contains a ':' and is followed by '--' the completion would
not recognize the valid refname.

I'm not sure what you mean here.  Refnames can't contain ':'.

Yes, my bad. I was confusing it with the case where filename and ref name are identical.

quoted

+               while [ $c -lt $cword ]; do
+                       i="${words[c]}"
+                       case "$i" in
+                       --) seen_double_dash=1 ;;
+                       -*|?*:*) ;;
+                       *) ref="$i"; break ;;

I haven't tried it, but this would trigger on e.g. 'git checkout -b
new-feature <TAB>', wouldn't it?

Correct, good eyes.

What about

  $ echo "unintentional change" >>tracked-file && git add -u
  $ git rm important-file
  $ git checkout HEAD <TAB>

?  It seems it will offer neither 'tracked-file' nor 'important-file',
but I think it should offer both.

Ideally yes. The latter of the two would also not work with Alt/.


-------------------------------------------------------------------
From d0e0c9af8a30dec479c393ae7fe75205c9b3b229 Mon Sep 17 00:00:00 2001
From: Cornelius Weig <redacted>
Date: Tue, 14 Feb 2017 21:01:45 +0100
Subject: [PATCH] completion: checkout: complete paths when ref given

Git-checkout completes words starting with '--' as options and other
words as refs. Even after specifying a ref, further words not starting
with '--' are completed as refs, which is invalid for git-checkout.

With this commit completion suppresses refname suggestion after finding
what looks like a refname. Words before a '--' not starting with a '-'
and containing no ':' are considered to be refnames.

Signed-off-by: Cornelius Weig <redacted>
---
 contrib/completion/git-completion.bash | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6c6e1c774d..42e6463b67 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash

@@ -1059,7 +1059,16 @@ _git_bundle ()
 
 _git_checkout ()
 {
-	__git_has_doubledash && return
+	local c=2 seen_ref=""
+	while [ $c -lt $cword ]; do
+		case "${words[c]}" in
+		--) return ;;
+		-b|-B|--orphan|--branch) ((c++)) ;;
+		-*|*:*) ;;
+		*) seen_ref="y" ;;
+		esac
+		((c++))
+	done
 
 	case "$cur" in
 	--conflict=*)

@@ -1072,13 +1081,16 @@ _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=1
-		if [ -n "$(__git_find_on_cmdline "$flags")" ]; then
-			track=''
+		if [ -z "$seen_ref" ]
+		then
+			# check for --track, --no-track, or --no-guess
+			# if so, disable DWIM mode
+			local flags="--track --no-track --no-guess" track=1
+			if [ -n "$(__git_find_on_cmdline "$flags")" ]; then
+				track=''
+			fi
+			__gitcomp_nl "$(__git_refs '' $track)"
 		fi
-		__gitcomp_nl "$(__git_refs '' $track)"
 		;;
 	esac
 }

-- 
2.11.1

On Tue, Feb 14, 2017 at 10:24 PM,  [off-list ref] wrote:

+               *)
+                       __git_complete_tree_file "$ref" "$cur"
+                       ;;

There is one more caveat here.

Both our __git_complete_index_file() and Bash's builtin filename
completion lists matching paths like this:

  $ git rm contrib/co<TAB>
  coccinelle/                        contacts/
  completion/                        convert-grafts-to-replace-refs.sh

i.e. the leading path components are not redundantly repeated.

Now, with this patch in this code path the list would look like this:

  $ git checkout completion-refs-speedup contrib/co<TAB>
  contrib/coccinelle/
  contrib/completion/
  contrib/contacts/
  contrib/convert-grafts-to-replace-refs.sh

See the difference?

I once made a feeble attempt to make completion of the <ref>:<path>
notation (i.e. what you extracted into __git_complete_tree_file())
look like regular filename completion, but couldn't.

Gábor

On 02/15/2017 03:26 PM, SZEDER Gábor wrote:

On Tue, Feb 14, 2017 at 10:24 PM,  [off-list ref] wrote:

quoted

+               *)
+                       __git_complete_tree_file "$ref" "$cur"
+                       ;;

There is one more caveat here.

Both our __git_complete_index_file() and Bash's builtin filename
completion lists matching paths like this:

  $ git rm contrib/co<TAB>
  coccinelle/                        contacts/
  completion/                        convert-grafts-to-replace-refs.sh

i.e. the leading path components are not redundantly repeated.

Now, with this patch in this code path the list would look like this:

  $ git checkout completion-refs-speedup contrib/co<TAB>
  contrib/coccinelle/
  contrib/completion/
  contrib/contacts/
  contrib/convert-grafts-to-replace-refs.sh

See the difference?

Now that you say it.. I had never noticed it though.

I once made a feeble attempt to make completion of the <ref>:<path>
notation (i.e. what you extracted into __git_complete_tree_file())
look like regular filename completion, but couldn't.

Can you dig up what you tried out? Maybe somebody comes up with a good idea.