[PATCH 00/11] completion: general cleanups

[PATCH 00/11] completion: general cleanups

[Felipe Contreras]

Hi,

Basically while trying to understand the code for path completion, I found that
a lot of code was duplicated, and for not much gain.

I also noticed that doing 'git add file' doesn't add the trailing space as
before. It's not clear if it should be possible to do that with -o filenames,
but after all, what do -o filenames gives us? Nothing we can't do ourselves,
apparently.

However, in zsh the -f option does give us lots of niceties, so there's a patch
to allow that in a zsh way.

Also, add the missing tests.

Cheers.

Felipe Contreras (11):
  completion: add file completion tests
  completion: document tilde expansion failure in tests
  completion; remove unuseful comments
  completion: use __gitcompadd for __gitcomp_file
  completion: refactor diff_index wrappers
  completion: refactor __git_complete_index_file()
  completion: avoid compopt -o filenames
  completion: get rid of __gitcomp_file()
  completion: add space after completed filename
  completion: remove __git_index_file_list_filter()
  completion: zsh: add proper file support

 contrib/completion/git-completion.bash | 168 +++++----------------------------
 contrib/completion/git-completion.zsh  |  41 +++++++-
 t/t9902-completion.sh                  |  77 +++++++++++++++
 3 files changed, 140 insertions(+), 146 deletions(-)

-- 
1.8.2.1.1031.g2ee5873

[PATCH 01/11] completion: add file completion tests

[Felipe Contreras]

The commit fea16b4 (git-completion.bash: add support for path
completion) introduced quite a few changes, without the usual tests.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t9902-completion.sh | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 6d9d141..385e1e4 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -347,4 +347,72 @@ test_expect_success 'send-email' '
 	test_completion "git send-email ma" "master "
 '
 
+test_expect_success 'complete files' '
+	git init tmp && cd tmp &&
+	test_when_finished "cd .. && rm -rf tmp" &&
+
+	echo "expected" > .gitignore &&
+	echo "out" >> .gitignore &&
+
+	git add .gitignore &&
+	test_completion "git commit " ".gitignore" &&
+
+	git commit -m ignore &&
+
+	touch new &&
+	test_completion "git add " "new" &&
+
+	git add new &&
+	git commit -a -m new &&
+	test_completion "git add " "" &&
+
+	git mv new modified &&
+	echo modify > modified &&
+	test_completion "git add " "modified" &&
+
+	touch untracked &&
+
+	: TODO .gitignore should not be here &&
+	test_completion "git rm " <<-\EOF &&
+	.gitignore
+	modified
+	EOF
+
+	test_completion "git clean " "untracked" &&
+
+	: TODO .gitignore should not be here &&
+	test_completion "git mv " <<-\EOF &&
+	.gitignore
+	modified
+	EOF
+
+	mkdir dir &&
+	touch dir/file-in-dir &&
+	git add dir/file-in-dir &&
+	git commit -m dir &&
+
+	mkdir untracked-dir &&
+
+	: TODO .gitignore should not be here &&
+	test_completion "git mv modified " <<-\EOF &&
+	.gitignore
+	dir
+	modified
+	untracked
+	untracked-dir
+	EOF
+
+	test_completion "git commit " "modified" &&
+
+	: TODO .gitignore should not be here &&
+	test_completion "git ls-files " <<-\EOF
+	.gitignore
+	dir
+	modified
+	EOF
+
+	touch momified &&
+	test_completion "git add mom" "momified"
+'
+
 test_done
-- 
1.8.2.1.1031.g2ee5873

[PATCH 02/11] completion: document tilde expansion failure in tests

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t9902-completion.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 385e1e4..81a1657 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -415,4 +415,13 @@ test_expect_success 'complete files' '
 	test_completion "git add mom" "momified"
 '
 
+test_expect_failure 'complete with tilde expansion' '
+	git init tmp && cd tmp &&
+	test_when_finished "cd .. && rm -rf tmp" &&
+
+	touch ~/tmp/file &&
+
+	test_completion "git add ~/tmp/" "~/tmp/file"
+'
+
 test_done
-- 
1.8.2.1.1031.g2ee5873

[PATCH 03/11] completion; remove unuseful comments

[Felipe Contreras]

The only caller, __git_complete_index_file() doesn't specify any limits
to the options for 'git ls-files', neither should this function.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash | 2 --
 1 file changed, 2 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index bc3fc9e..f7b0f3c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -323,8 +323,6 @@ __git_diff_index_helper ()
 
 # __git_index_files accepts 1 or 2 arguments:
 # 1: Options to pass to ls-files (required).
-#    Supported options are --cached, --modified, --deleted, --others,
-#    and --directory.
 # 2: A directory path (optional).
 #    If provided, only files within the specified directory are listed.
 #    Sub directories are never recursed.  Path must have a trailing
-- 
1.8.2.1.1031.g2ee5873

[PATCH 04/11] completion: use __gitcompadd for __gitcomp_file

[Felipe Contreras]

Like the rest of the script does; let's not access COMPREPLY directly.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index f7b0f3c..7f1ebe4 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -252,7 +252,7 @@ __gitcomp_file ()
 	# since tilde expansion is not applied.
 	# This means that COMPREPLY will be empty and Bash default
 	# completion will be used.
-	COMPREPLY=($(compgen -P "${2-}" -W "$1" -- "${3-$cur}"))
+	__gitcompadd "$1" "${2-}" "${3-$cur}" ""
 
 	# Tell Bash that compspec generates filenames.
 	compopt -o filenames 2>/dev/null
-- 
1.8.2.1.1031.g2ee5873

[PATCH 05/11] completion: refactor diff_index wrappers

[Felipe Contreras]

At the end of the day what we really need is to find out the files that
have been staged, or modified, because those files are the ones that
make sense to pass as arguments to 'git commit'.

We need diff-index to find those out, since 'git ls-files' doesn't do
that.

But we don't need wrappers and wrappers basically identical to the ones
used for 'git ls-files', when we can pretend it receives a --committable
option that would return what we need.

That way, we can remove a bunch of code without any functional changes.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash | 71 ++++++++--------------------------
 1 file changed, 16 insertions(+), 55 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 7f1ebe4..2561265 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -297,30 +297,25 @@ __git_index_file_list_filter ()
 	__git_index_file_list_filter_bash
 }
 
-# Execute git ls-files, returning paths relative to the directory
-# specified in the first argument, and using the options specified in
-# the second argument.
+# Execute 'git ls-files', unless the --committable option is specified, in
+# which case it runs 'git diff-index' to find out the files that can be
+# committed.  It return paths relative to the directory specified in the first
+# argument, and using the options specified in the second argument.
 __git_ls_files_helper ()
 {
 	(
 		test -n "${CDPATH+set}" && unset CDPATH
-		# NOTE: $2 is not quoted in order to support multiple options
-		cd "$1" && git ls-files --exclude-standard $2
+		cd "$1"
+		if [ "$2" == "--committable" ]; then
+			git diff-index --name-only --relative HEAD
+		else
+			# NOTE: $2 is not quoted in order to support multiple options
+			git ls-files --exclude-standard $2
+		fi
 	) 2>/dev/null
 }
 
 
-# Execute git diff-index, returning paths relative to the directory
-# specified in the first argument, and using the tree object id
-# specified in the second argument.
-__git_diff_index_helper ()
-{
-	(
-		test -n "${CDPATH+set}" && unset CDPATH
-		cd "$1" && git diff-index --name-only --relative "$2"
-	) 2>/dev/null
-}
-
 # __git_index_files accepts 1 or 2 arguments:
 # 1: Options to pass to ls-files (required).
 # 2: A directory path (optional).
@@ -337,22 +332,6 @@ __git_index_files ()
 	fi
 }
 
-# __git_diff_index_files accepts 1 or 2 arguments:
-# 1) The id of a tree object.
-# 2) A directory path (optional).
-#    If provided, only files within the specified directory are listed.
-#    Sub directories are never recursed.  Path must have a trailing
-#    slash.
-__git_diff_index_files ()
-{
-	local dir="$(__gitdir)" root="${2-.}"
-
-	if [ -d "$dir" ]; then
-		__git_diff_index_helper "$root" "$1" | __git_index_file_list_filter |
-			sort | uniq
-	fi
-}
-
 __git_heads ()
 {
 	local dir="$(__gitdir)"
@@ -550,8 +529,10 @@ __git_complete_revlist_file ()
 }
 
 
-# __git_complete_index_file requires 1 argument: the options to pass to
-# ls-file
+# __git_complete_index_file requires 1 argument:
+# 1: the options to pass to ls-file
+#
+# The exception is --committable, which finds the files appropriate commit.
 __git_complete_index_file ()
 {
 	local pfx cur_="$cur"
@@ -570,26 +551,6 @@ __git_complete_index_file ()
 	esac
 }
 
-# __git_complete_diff_index_file requires 1 argument: the id of a tree
-# object
-__git_complete_diff_index_file ()
-{
-	local pfx cur_="$cur"
-
-	case "$cur_" in
-	?*/*)
-		pfx="${cur_%/*}"
-		cur_="${cur_##*/}"
-		pfx="${pfx}/"
-
-		__gitcomp_file "$(__git_diff_index_files "$1" "$pfx")" "$pfx" "$cur_"
-		;;
-	*)
-		__gitcomp_file "$(__git_diff_index_files "$1")" "" "$cur_"
-		;;
-	esac
-}
-
 __git_complete_file ()
 {
 	__git_complete_revlist_file
@@ -1211,7 +1172,7 @@ _git_commit ()
 	esac
 
 	if git rev-parse --verify --quiet HEAD >/dev/null; then
-		__git_complete_diff_index_file "HEAD"
+		__git_complete_index_file "--committable"
 	else
 		# This is the first commit
 		__git_complete_index_file "--cached"
-- 
1.8.2.1.1031.g2ee5873

[PATCH 06/11] completion: refactor __git_complete_index_file()

[Felipe Contreras]

The calls to __gitcomp_file() are essentially the same, but with
different prefix.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 2561265..9cea170 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -535,20 +535,17 @@ __git_complete_revlist_file ()
 # The exception is --committable, which finds the files appropriate commit.
 __git_complete_index_file ()
 {
-	local pfx cur_="$cur"
+	local pfx="" cur_="$cur"
 
 	case "$cur_" in
 	?*/*)
 		pfx="${cur_%/*}"
 		cur_="${cur_##*/}"
 		pfx="${pfx}/"
-
-		__gitcomp_file "$(__git_index_files "$1" "$pfx")" "$pfx" "$cur_"
-		;;
-	*)
-		__gitcomp_file "$(__git_index_files "$1")" "" "$cur_"
 		;;
 	esac
+
+	__gitcomp_file "$(__git_index_files "$1" "$pfx")" "$pfx" "$cur_"
 }
 
 __git_complete_file ()
-- 
1.8.2.1.1031.g2ee5873

[PATCH 07/11] completion: avoid compopt -o filenames

[Felipe Contreras]

If this option causes so much trouble, lets just not use it.

The supposed advantages of specifying the 'filenames' is that 1) a slash
is added at the end, 2) trailing spaces are removed (which doesn't seem
to be true), and 3) special characters are quoted. We can do all those
things by ourselves.

Also, this will allow us to add a space after the completion, just like
it happened before fea16b4 (git-completion.bash: add support for path
completion), and just like the rest of the script does.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash | 43 ++++------------------------------
 t/t9902-completion.sh                  |  6 ++---
 2 files changed, 7 insertions(+), 42 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 9cea170..5dd6646 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -253,12 +253,12 @@ __gitcomp_file ()
 	# This means that COMPREPLY will be empty and Bash default
 	# completion will be used.
 	__gitcompadd "$1" "${2-}" "${3-$cur}" ""
-
-	# Tell Bash that compspec generates filenames.
-	compopt -o filenames 2>/dev/null
 }
 
-__git_index_file_list_filter_compat ()
+# Process path list returned by "ls-files" and "diff-index --name-only"
+# commands, in order to list only file names relative to a specified
+# directory, and append a slash to directory names.
+__git_index_file_list_filter ()
 {
 	local path
 
@@ -270,33 +270,6 @@ __git_index_file_list_filter_compat ()
 	done
 }
 
-__git_index_file_list_filter_bash ()
-{
-	local path
-
-	while read -r path; do
-		case "$path" in
-		?*/*)
-			# XXX if we append a slash to directory names when using
-			# `compopt -o filenames`, Bash will append another slash.
-			# This is pretty stupid, and this the reason why we have to
-			# define a compatible version for this function.
-			echo "${path%%/*}" ;;
-		*)
-			echo "$path" ;;
-		esac
-	done
-}
-
-# Process path list returned by "ls-files" and "diff-index --name-only"
-# commands, in order to list only file names relative to a specified
-# directory, and append a slash to directory names.
-__git_index_file_list_filter ()
-{
-	# Default to Bash >= 4.x
-	__git_index_file_list_filter_bash
-}
-
 # Execute 'git ls-files', unless the --committable option is specified, in
 # which case it runs 'git diff-index' to find out the files that can be
 # committed.  It return paths relative to the directory specified in the first
@@ -2651,14 +2624,6 @@ if [[ -n ${ZSH_VERSION-} ]]; then
 
 	compdef _git git gitk
 	return
-elif [[ -n ${BASH_VERSION-} ]]; then
-	if ((${BASH_VERSINFO[0]} < 4)); then
-		# compopt is not supported
-		__git_index_file_list_filter ()
-		{
-			__git_index_file_list_filter_compat
-		}
-	fi
 fi
 
 __git_func_wrap ()
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 81a1657..e6cdb05 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -396,10 +396,10 @@ test_expect_success 'complete files' '
 	: TODO .gitignore should not be here &&
 	test_completion "git mv modified " <<-\EOF &&
 	.gitignore
-	dir
+	dir/
 	modified
 	untracked
-	untracked-dir
+	untracked-dir/
 	EOF
 
 	test_completion "git commit " "modified" &&
@@ -407,7 +407,7 @@ test_expect_success 'complete files' '
 	: TODO .gitignore should not be here &&
 	test_completion "git ls-files " <<-\EOF
 	.gitignore
-	dir
+	dir/
 	modified
 	EOF
 
-- 
1.8.2.1.1031.g2ee5873

[PATCH 08/11] completion: get rid of __gitcomp_file()

[Felipe Contreras]

Now that we don't use -o filenames, it's no different from
__gitcomp_nl().

The only reason we might want it is for other shells, like zsh, which
can do more useful things if they know it's a file, like colouring the
output. But that can be done in the zsh completion.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash | 29 +----------------------------
 contrib/completion/git-completion.zsh  |  9 ---------
 2 files changed, 1 insertion(+), 37 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 5dd6646..a9b6a48 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -237,24 +237,6 @@ __gitcomp_nl ()
 	__gitcompadd "$1" "${2-}" "${3-$cur}" "${4- }"
 }
 
-# Generates completion reply with compgen from newline-separated possible
-# completion filenames.
-# It accepts 1 to 3 arguments:
-# 1: List of possible completion filenames, separated by a single newline.
-# 2: A directory prefix to be added to each possible completion filename
-#    (optional).
-# 3: Generate possible completion matches for this word (optional).
-__gitcomp_file ()
-{
-	local IFS=$'\n'
-
-	# XXX does not work when the directory prefix contains a tilde,
-	# since tilde expansion is not applied.
-	# This means that COMPREPLY will be empty and Bash default
-	# completion will be used.
-	__gitcompadd "$1" "${2-}" "${3-$cur}" ""
-}
-
 # Process path list returned by "ls-files" and "diff-index --name-only"
 # commands, in order to list only file names relative to a specified
 # directory, and append a slash to directory names.
@@ -518,7 +500,7 @@ __git_complete_index_file ()
 		;;
 	esac
 
-	__gitcomp_file "$(__git_index_files "$1" "$pfx")" "$pfx" "$cur_"
+	__gitcomp_nl "$(__git_index_files "$1" "$pfx")" "$pfx" "$cur_" ""
 }
 
 __git_complete_file ()
@@ -2594,15 +2576,6 @@ if [[ -n ${ZSH_VERSION-} ]]; then
 		compadd -Q -S "${4- }" -p "${2-}" -- ${=1} && _ret=0
 	}
 
-	__gitcomp_file ()
-	{
-		emulate -L zsh
-
-		local IFS=$'\n'
-		compset -P '*[=:]'
-		compadd -Q -p "${2-}" -f -- ${=1} && _ret=0
-	}
-
 	__git_zsh_helper ()
 	{
 		emulate -L ksh
diff --git a/contrib/completion/git-completion.zsh b/contrib/completion/git-completion.zsh
index cf8116d..4577502 100644
--- a/contrib/completion/git-completion.zsh
+++ b/contrib/completion/git-completion.zsh
@@ -60,15 +60,6 @@ __gitcomp_nl ()
 	compadd -Q -S "${4- }" -p "${2-}" -- ${=1} && _ret=0
 }
 
-__gitcomp_file ()
-{
-	emulate -L zsh
-
-	local IFS=$'\n'
-	compset -P '*[=:]'
-	compadd -Q -p "${2-}" -f -- ${=1} && _ret=0
-}
-
 _git ()
 {
 	local _ret=1
-- 
1.8.2.1.1031.g2ee5873

[PATCH 09/11] completion: add space after completed filename

[Felipe Contreras]

Just like before fea16b4 (git-completion.bash: add support for path
completion).

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash |  2 +-
 t/t9902-completion.sh                  | 28 ++++++++++++++--------------
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index a9b6a48..60a6a0b 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -247,7 +247,7 @@ __git_index_file_list_filter ()
 	while read -r path; do
 		case "$path" in
 		?*/*) echo "${path%%/*}/" ;;
-		*) echo "$path" ;;
+		*) echo "$path " ;;
 		esac
 	done
 }
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index e6cdb05..8968ef6 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -355,12 +355,12 @@ test_expect_success 'complete files' '
 	echo "out" >> .gitignore &&
 
 	git add .gitignore &&
-	test_completion "git commit " ".gitignore" &&
+	test_completion "git commit " ".gitignore " &&
 
 	git commit -m ignore &&
 
 	touch new &&
-	test_completion "git add " "new" &&
+	test_completion "git add " "new " &&
 
 	git add new &&
 	git commit -a -m new &&
@@ -368,22 +368,22 @@ test_expect_success 'complete files' '
 
 	git mv new modified &&
 	echo modify > modified &&
-	test_completion "git add " "modified" &&
+	test_completion "git add " "modified " &&
 
 	touch untracked &&
 
 	: TODO .gitignore should not be here &&
 	test_completion "git rm " <<-\EOF &&
-	.gitignore
-	modified
+	.gitignore Z
+	modified Z
 	EOF
 
-	test_completion "git clean " "untracked" &&
+	test_completion "git clean " "untracked " &&
 
 	: TODO .gitignore should not be here &&
 	test_completion "git mv " <<-\EOF &&
-	.gitignore
-	modified
+	.gitignore Z
+	modified Z
 	EOF
 
 	mkdir dir &&
@@ -395,20 +395,20 @@ test_expect_success 'complete files' '
 
 	: TODO .gitignore should not be here &&
 	test_completion "git mv modified " <<-\EOF &&
-	.gitignore
+	.gitignore Z
 	dir/
-	modified
-	untracked
+	modified Z
+	untracked Z
 	untracked-dir/
 	EOF
 
-	test_completion "git commit " "modified" &&
+	test_completion "git commit " "modified " &&
 
 	: TODO .gitignore should not be here &&
 	test_completion "git ls-files " <<-\EOF
-	.gitignore
+	.gitignore Z
 	dir/
-	modified
+	modified Z
 	EOF
 
 	touch momified &&
-- 
1.8.2.1.1031.g2ee5873

[PATCH 10/11] completion: remove __git_index_file_list_filter()

[Felipe Contreras]

Refactor the code into the only caller; __git_index_files().

Also, Somehow messing up with the 'path' variable messes up the 'PATH'
variable. So let's not do that.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 60a6a0b..8100085 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -237,21 +237,6 @@ __gitcomp_nl ()
 	__gitcompadd "$1" "${2-}" "${3-$cur}" "${4- }"
 }
 
-# Process path list returned by "ls-files" and "diff-index --name-only"
-# commands, in order to list only file names relative to a specified
-# directory, and append a slash to directory names.
-__git_index_file_list_filter ()
-{
-	local path
-
-	while read -r path; do
-		case "$path" in
-		?*/*) echo "${path%%/*}/" ;;
-		*) echo "$path " ;;
-		esac
-	done
-}
-
 # Execute 'git ls-files', unless the --committable option is specified, in
 # which case it runs 'git diff-index' to find out the files that can be
 # committed.  It return paths relative to the directory specified in the first
@@ -279,11 +264,16 @@ __git_ls_files_helper ()
 #    slash.
 __git_index_files ()
 {
-	local dir="$(__gitdir)" root="${2-.}"
+	local dir="$(__gitdir)" root="${2-.}" file
 
 	if [ -d "$dir" ]; then
-		__git_ls_files_helper "$root" "$1" | __git_index_file_list_filter |
-			sort | uniq
+		__git_ls_files_helper "$root" "$1" |
+		while read -r file; do
+			case "$file" in
+			?*/*) echo "${file%%/*}/" ;;
+			*) echo "$file " ;;
+			esac
+		done | sort | uniq
 	fi
 }
 
-- 
1.8.2.1.1031.g2ee5873

[PATCH 11/11] completion: zsh: add proper file support

[Felipe Contreras]

So the files are completed and coloured nicely.

The code would be only a couple of lines if only _multi_parts or
_path_files worked correctly, but alas, they don't.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.zsh | 42 +++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/contrib/completion/git-completion.zsh b/contrib/completion/git-completion.zsh
index 4577502..8c6916a 100644
--- a/contrib/completion/git-completion.zsh
+++ b/contrib/completion/git-completion.zsh
@@ -60,6 +60,48 @@ __gitcomp_nl ()
 	compadd -Q -S "${4- }" -p "${2-}" -- ${=1} && _ret=0
 }
 
+#
+# We need to remove the extra slash for directories, and we need to remove the
+# spaces for files, otherwise zsh can't stat them and threat them as proper files.
+#
+__git_index_files ()
+{
+	local dir="$(__gitdir)" root="${2-.}" file
+
+	if [ -d "$dir" ]; then
+		local -a files
+		emulate ksh -c '__git_ls_files_helper "$root" "$1"' |
+		while read -r file; do
+			case "$file" in
+			?*/*)
+				files+="${file%%/*}"
+				;;
+			*)
+				files+="$file"
+				;;
+			esac
+		done
+		compadd -Q -p "${2-}" -f -a files && _ret=0
+	fi
+}
+
+__git_complete_index_file ()
+{
+	emulate -L zsh
+
+	local pfx="" cur_="$cur"
+
+	case "$cur_" in
+	?*/*)
+		pfx="${cur_%/*}"
+		cur_="${cur_##*/}"
+		pfx="${pfx}/"
+		;;
+	esac
+
+	__git_index_files "$1" "$pfx"
+}
+
 _git ()
 {
 	local _ret=1
-- 
1.8.2.1.1031.g2ee5873

Re: [PATCH 00/11] completion: general cleanups

[Manlio Perillo]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 27/04/2013 12:19, Felipe Contreras ha scritto:
> Hi,
> 
> Basically while trying to understand the code for path completion, I found that
> a lot of code was duplicated, and for not much gain.
> 
> I also noticed that doing 'git add file' doesn't add the trailing space as
> before. It's not clear if it should be possible to do that with -o filenames,
> but after all, what do -o filenames gives us? Nothing we can't do ourselves,
> apparently.
> 

No, you can not do it yourself, as far as I know.

I added the `compopt -o filenames` on Junio request for something like
"It  would be nice if completion for real files would behave like
builtin bash completion", if I remember correctly.

Try `git rm contrib/completion/<TAB>`, in the git reporitory.

Using the new feature, bash will suggest:
"git-completion.bash  git-completion.tcsh  git-completion.zsh
git-prompt.sh"

Old behaviour, instead, was to suggest:
"contrib/completion/git-completion.bash
contrib/completion/git-completion.zsh
contrib/completion/git-completion.tcsh  contrib/completion/git-prompt.sh"

I tried several things, but I was unable to emulate Bash builtin file
completion, whithout having to use `compopt -o filenames`.



As far as the "double slash" problem with the
__git_index_file_list_filter_bash function, please try
`git rm contrib<TAB>`.

With current code, Bash will suggest:
"blameview/ diffall/ git-shell-commands/"

If you remove the __git_index_file_list_filter_bash function and use
__git_index_file_list_filter_compat instead, Bash will suggest:

"blameview// diffall// git-shell-commands//"

I can confirm this on my system, and it was confirmed by another user.
It only happens when you use `compopt -o filenames`. I don't know if
this is a bug or a feature, but I can try to ask to Bash mailing list,
so that we can update the comment to make more clear why a separate
function was needed.


> [...]


Regards  Manlio
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAlF7t5gACgkQscQJ24LbaUSO5QCffllxM8RbGUP47kb7uL5J3drF
hkUAn26ezKptTAC412EJZnxjh7RVcdAO
=Piyz
-----END PGP SIGNATURE-----

Re: [PATCH 00/11] completion: general cleanups

[Felipe Contreras]

On Sat, Apr 27, 2013 at 6:33 AM, Manlio Perillo
<manlio.perillo@gmail.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Il 27/04/2013 12:19, Felipe Contreras ha scritto:
>> Hi,
>>
>> Basically while trying to understand the code for path completion, I found that
>> a lot of code was duplicated, and for not much gain.
>>
>> I also noticed that doing 'git add file' doesn't add the trailing space as
>> before. It's not clear if it should be possible to do that with -o filenames,
>> but after all, what do -o filenames gives us? Nothing we can't do ourselves,
>> apparently.
>>
>
> No, you can not do it yourself, as far as I know.
>
> I added the `compopt -o filenames` on Junio request for something like
> "It  would be nice if completion for real files would behave like
> builtin bash completion", if I remember correctly.
>
> Try `git rm contrib/completion/<TAB>`, in the git reporitory.
>
> Using the new feature, bash will suggest:
> "git-completion.bash  git-completion.tcsh  git-completion.zsh
> git-prompt.sh"
>
> Old behaviour, instead, was to suggest:
> "contrib/completion/git-completion.bash
> contrib/completion/git-completion.zsh
> contrib/completion/git-completion.tcsh  contrib/completion/git-prompt.sh"
>
> I tried several things, but I was unable to emulate Bash builtin file
> completion, whithout having to use `compopt -o filenames`.

I see. I'm not convinced it's such a great feature, but it would be
nice to have.

Anyway, 'compopt -o filenames +o nospace' should restore the old
behavior to add a space after the completion.

> As far as the "double slash" problem with the
> __git_index_file_list_filter_bash function, please try
> `git rm contrib<TAB>`.
>
> With current code, Bash will suggest:
> "blameview/ diffall/ git-shell-commands/"
>
> If you remove the __git_index_file_list_filter_bash function and use
> __git_index_file_list_filter_compat instead, Bash will suggest:
>
> "blameview// diffall// git-shell-commands//"
>
> I can confirm this on my system, and it was confirmed by another user.
> It only happens when you use `compopt -o filenames`. I don't know if
> this is a bug or a feature, but I can try to ask to Bash mailing list,
> so that we can update the comment to make more clear why a separate
> function was needed.

I've managed to reproduce the issue. The slash doesn't appear in the
completion, it appears on the list of completions.

I'll see what I can think to fix the issues while still keep the code simple.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 00/11] completion: general cleanups

[Felipe Contreras]

On Sat, Apr 27, 2013 at 7:36 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Sat, Apr 27, 2013 at 6:33 AM, Manlio Perillo
> <manlio.perillo@gmail.com> wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Il 27/04/2013 12:19, Felipe Contreras ha scritto:
>>> Hi,
>>>
>>> Basically while trying to understand the code for path completion, I found that
>>> a lot of code was duplicated, and for not much gain.
>>>
>>> I also noticed that doing 'git add file' doesn't add the trailing space as
>>> before. It's not clear if it should be possible to do that with -o filenames,
>>> but after all, what do -o filenames gives us? Nothing we can't do ourselves,
>>> apparently.
>>>
>>
>> No, you can not do it yourself, as far as I know.
>>
>> I added the `compopt -o filenames` on Junio request for something like
>> "It  would be nice if completion for real files would behave like
>> builtin bash completion", if I remember correctly.
>>
>> Try `git rm contrib/completion/<TAB>`, in the git reporitory.
>>
>> Using the new feature, bash will suggest:
>> "git-completion.bash  git-completion.tcsh  git-completion.zsh
>> git-prompt.sh"
>>
>> Old behaviour, instead, was to suggest:
>> "contrib/completion/git-completion.bash
>> contrib/completion/git-completion.zsh
>> contrib/completion/git-completion.tcsh  contrib/completion/git-prompt.sh"
>>
>> I tried several things, but I was unable to emulate Bash builtin file
>> completion, whithout having to use `compopt -o filenames`.
>
> I see. I'm not convinced it's such a great feature, but it would be
> nice to have.
>
> Anyway, 'compopt -o filenames +o nospace' should restore the old
> behavior to add a space after the completion.
>
>> As far as the "double slash" problem with the
>> __git_index_file_list_filter_bash function, please try
>> `git rm contrib<TAB>`.
>>
>> With current code, Bash will suggest:
>> "blameview/ diffall/ git-shell-commands/"
>>
>> If you remove the __git_index_file_list_filter_bash function and use
>> __git_index_file_list_filter_compat instead, Bash will suggest:
>>
>> "blameview// diffall// git-shell-commands//"
>>
>> I can confirm this on my system, and it was confirmed by another user.
>> It only happens when you use `compopt -o filenames`. I don't know if
>> this is a bug or a feature, but I can try to ask to Bash mailing list,
>> so that we can update the comment to make more clear why a separate
>> function was needed.
>
> I've managed to reproduce the issue. The slash doesn't appear in the
> completion, it appears on the list of completions.
>
> I'll see what I can think to fix the issues while still keep the code simple.

This should do the trick. No?

--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -262,16 +262,17 @@ __git_ls_files_helper ()
 #    If provided, only files within the specified directory are listed.
 #    Sub directories are never recursed.  Path must have a trailing
 #    slash.
+# 3. Compat mode; set to enable.
 __git_index_files ()
 {
-       local dir="$(__gitdir)" root="${2-.}" file
+       local dir="$(__gitdir)" root="${2-.}" file old="${3-}"

        if [ -d "$dir" ]; then
                __git_ls_files_helper "$root" "$1" |
                while read -r file; do
                        case "$file" in
-                       ?*/*) echo "${file%%/*}/" ;;
-                       *) echo "$file " ;;
+                       ?*/*) echo "${file%%/*}${old:+/}" ;;
+                       *) echo "${file}${old:+ }" ;;
                        esac
                done | sort | uniq
        fi
@@ -480,7 +481,7 @@ __git_complete_revlist_file ()
 # The exception is --committable, which finds the files appropriate commit.
 __git_complete_index_file ()
 {
-       local pfx="" cur_="$cur"
+       local pfx="" cur_="$cur" old

        case "$cur_" in
        ?*/*)
@@ -490,7 +491,8 @@ __git_complete_index_file ()
                ;;
        esac

-       __gitcomp_nl "$(__git_index_files "$1" "$pfx")" "$pfx" "$cur_" ""
+       compopt -o filenames +o nospace 2> /dev/null || old=1
+       __gitcomp_nl "$(__git_index_files "$1" "$pfx" "$old")" "$pfx" "$cur_" ""
 }

 __git_complete_file ()

-- 
Felipe Contreras

Re: [PATCH 00/11] completion: general cleanups

[Manlio Perillo]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 27/04/2013 15:07, Felipe Contreras ha scritto:
> [...]
> This should do the trick. No?
> 
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -262,16 +262,17 @@ __git_ls_files_helper ()
>  #    If provided, only files within the specified directory are listed.
>  #    Sub directories are never recursed.  Path must have a trailing
>  #    slash.
> +# 3. Compat mode; set to enable.
>  __git_index_files ()
>  {
> -       local dir="$(__gitdir)" root="${2-.}" file
> +       local dir="$(__gitdir)" root="${2-.}" file old="${3-}"
> 
>         if [ -d "$dir" ]; then
>                 __git_ls_files_helper "$root" "$1" |
>                 while read -r file; do
>                         case "$file" in
> -                       ?*/*) echo "${file%%/*}/" ;;
> -                       *) echo "$file " ;;
> +                       ?*/*) echo "${file%%/*}${old:+/}" ;;
> +                       *) echo "${file}${old:+ }" ;;
>                         esac
>                 done | sort | uniq
>         fi
> @@ -480,7 +481,7 @@ __git_complete_revlist_file ()
>  # The exception is --committable, which finds the files appropriate commit.
>  __git_complete_index_file ()
>  {
> -       local pfx="" cur_="$cur"
> +       local pfx="" cur_="$cur" old
> 
>         case "$cur_" in
>         ?*/*)
> @@ -490,7 +491,8 @@ __git_complete_index_file ()
>                 ;;
>         esac
> 
> -       __gitcomp_nl "$(__git_index_files "$1" "$pfx")" "$pfx" "$cur_" ""
> +       compopt -o filenames +o nospace 2> /dev/null || old=1
> +       __gitcomp_nl "$(__git_index_files "$1" "$pfx" "$old")" "$pfx" "$cur_" ""
>  }
> 
>  __git_complete_file ()
> 

I like the idea (but I have not tested it), however compopt is called
two times, for each completion.

Maybe we can test for `-o filenames` support when script is loaded,
where currently there is a Bash version check, and set a global variable?



Regards   Manlio
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAlF78WcACgkQscQJ24LbaUSjzgCfWq26RMqFLgGU9B8C0mb+Wogu
A5IAnjKpupGbdOZAKtYZkglYKSmbqtqK
=iTzW
-----END PGP SIGNATURE-----

Re: [PATCH 00/11] completion: general cleanups

[Felipe Contreras]

On Sat, Apr 27, 2013 at 10:40 AM, Manlio Perillo
<manlio.perillo@gmail.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Il 27/04/2013 15:07, Felipe Contreras ha scritto:
>> [...]
>> This should do the trick. No?
>>
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -262,16 +262,17 @@ __git_ls_files_helper ()
>>  #    If provided, only files within the specified directory are listed.
>>  #    Sub directories are never recursed.  Path must have a trailing
>>  #    slash.
>> +# 3. Compat mode; set to enable.
>>  __git_index_files ()
>>  {
>> -       local dir="$(__gitdir)" root="${2-.}" file
>> +       local dir="$(__gitdir)" root="${2-.}" file old="${3-}"
>>
>>         if [ -d "$dir" ]; then
>>                 __git_ls_files_helper "$root" "$1" |
>>                 while read -r file; do
>>                         case "$file" in
>> -                       ?*/*) echo "${file%%/*}/" ;;
>> -                       *) echo "$file " ;;
>> +                       ?*/*) echo "${file%%/*}${old:+/}" ;;
>> +                       *) echo "${file}${old:+ }" ;;
>>                         esac
>>                 done | sort | uniq
>>         fi
>> @@ -480,7 +481,7 @@ __git_complete_revlist_file ()
>>  # The exception is --committable, which finds the files appropriate commit.
>>  __git_complete_index_file ()
>>  {
>> -       local pfx="" cur_="$cur"
>> +       local pfx="" cur_="$cur" old
>>
>>         case "$cur_" in
>>         ?*/*)
>> @@ -490,7 +491,8 @@ __git_complete_index_file ()
>>                 ;;
>>         esac
>>
>> -       __gitcomp_nl "$(__git_index_files "$1" "$pfx")" "$pfx" "$cur_" ""
>> +       compopt -o filenames +o nospace 2> /dev/null || old=1
>> +       __gitcomp_nl "$(__git_index_files "$1" "$pfx" "$old")" "$pfx" "$cur_" ""
>>  }
>>
>>  __git_complete_file ()
>>
>
> I like the idea (but I have not tested it), however compopt is called
> two times, for each completion.

Why two times?

> Maybe we can test for `-o filenames` support when script is loaded,
> where currently there is a Bash version check, and set a global variable?

Yeah, that's the way bash-completion used to do it. But I wonder if we
should be worrying about this at this point, even bash-completion
dropped support for bash < 4 more than two years ago, and even debian
stable is at 4.1.

http://anonscm.debian.org/gitweb/?p=bash-completion/bash-completion.git;a=commitdiff;h=f1b3be235722d1ea51160acf50120eb88995a5b7

Cheers.

-- 
Felipe Contreras

Re: [PATCH 00/11] completion: general cleanups

[Felipe Contreras]

On Sat, Apr 27, 2013 at 2:15 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Sat, Apr 27, 2013 at 10:40 AM, Manlio Perillo
> <manlio.perillo@gmail.com> wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Il 27/04/2013 15:07, Felipe Contreras ha scritto:
>>> [...]
>>> This should do the trick. No?
>>>
>>> --- a/contrib/completion/git-completion.bash
>>> +++ b/contrib/completion/git-completion.bash
>>> @@ -262,16 +262,17 @@ __git_ls_files_helper ()
>>>  #    If provided, only files within the specified directory are listed.
>>>  #    Sub directories are never recursed.  Path must have a trailing
>>>  #    slash.
>>> +# 3. Compat mode; set to enable.
>>>  __git_index_files ()
>>>  {
>>> -       local dir="$(__gitdir)" root="${2-.}" file
>>> +       local dir="$(__gitdir)" root="${2-.}" file old="${3-}"
>>>
>>>         if [ -d "$dir" ]; then
>>>                 __git_ls_files_helper "$root" "$1" |
>>>                 while read -r file; do
>>>                         case "$file" in
>>> -                       ?*/*) echo "${file%%/*}/" ;;
>>> -                       *) echo "$file " ;;
>>> +                       ?*/*) echo "${file%%/*}${old:+/}" ;;
>>> +                       *) echo "${file}${old:+ }" ;;
>>>                         esac
>>>                 done | sort | uniq
>>>         fi
>>> @@ -480,7 +481,7 @@ __git_complete_revlist_file ()
>>>  # The exception is --committable, which finds the files appropriate commit.
>>>  __git_complete_index_file ()
>>>  {
>>> -       local pfx="" cur_="$cur"
>>> +       local pfx="" cur_="$cur" old
>>>
>>>         case "$cur_" in
>>>         ?*/*)
>>> @@ -490,7 +491,8 @@ __git_complete_index_file ()
>>>                 ;;
>>>         esac
>>>
>>> -       __gitcomp_nl "$(__git_index_files "$1" "$pfx")" "$pfx" "$cur_" ""
>>> +       compopt -o filenames +o nospace 2> /dev/null || old=1
>>> +       __gitcomp_nl "$(__git_index_files "$1" "$pfx" "$old")" "$pfx" "$cur_" ""
>>>  }
>>>
>>>  __git_complete_file ()
>>>
>>
>> I like the idea (but I have not tested it), however compopt is called
>> two times, for each completion.
>
> Why two times?
>
>> Maybe we can test for `-o filenames` support when script is loaded,
>> where currently there is a Bash version check, and set a global variable?
>
> Yeah, that's the way bash-completion used to do it. But I wonder if we
> should be worrying about this at this point, even bash-completion
> dropped support for bash < 4 more than two years ago, and even debian
> stable is at 4.1.
>
> http://anonscm.debian.org/gitweb/?p=bash-completion/bash-completion.git;a=commitdiff;h=f1b3be235722d1ea51160acf50120eb88995a5b7

Actually, there's a way to trigger this in bash < 4, I've tested it
and it works:

--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -270,8 +270,8 @@ __git_index_files ()
                __git_ls_files_helper "$root" "$1" |
                while read -r file; do
                        case "$file" in
-                       ?*/*) echo "${file%%/*}/" ;;
-                       *) echo "$file " ;;
+                       ?*/*) echo "${file%%/*}" ;;
+                       *) echo "${file}" ;;
                        esac
                done | sort | uniq
        fi
@@ -490,6 +490,10 @@ __git_complete_index_file ()
                ;;
        esac

+       # use a hack to enable file mode in bash < 4
+       compopt -o filenames +o nospace 2> /dev/null ||
+       compgen -f /non-existing-dir/ > /dev/null
+
        __gitcomp_nl "$(__git_index_files "$1" "$pfx")" "$pfx" "$cur_" ""
 }


-- 
Felipe Contreras

Re: [PATCH 00/11] completion: general cleanups

[Manlio Perillo]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 27/04/2013 21:15, Felipe Contreras ha scritto:
> [...]
>>> @@ -480,7 +481,7 @@ __git_complete_revlist_file ()
>>>  # The exception is --committable, which finds the files appropriate commit.
>>>  __git_complete_index_file ()
>>>  {
>>> -       local pfx="" cur_="$cur"
>>> +       local pfx="" cur_="$cur" old
>>>
>>>         case "$cur_" in
>>>         ?*/*)
>>> @@ -490,7 +491,8 @@ __git_complete_index_file ()
>>>                 ;;
>>>         esac
>>>
>>> -       __gitcomp_nl "$(__git_index_files "$1" "$pfx")" "$pfx" "$cur_" ""
>>> +       compopt -o filenames +o nospace 2> /dev/null || old=1
>>> +       __gitcomp_nl "$(__git_index_files "$1" "$pfx" "$old")" "$pfx" "$cur_" ""
>>>  }
>>>
>>>  __git_complete_file ()
>>>
>>
>> I like the idea (but I have not tested it), however compopt is called
>> two times, for each completion.
> 
> Why two times?

Ah, right; sorry.
I missed the fact that you are using __gitcomp_nl instead of my
__gitcomp_file.

> 
>> Maybe we can test for `-o filenames` support when script is loaded,
>> where currently there is a Bash version check, and set a global variable?
> 
> Yeah, that's the way bash-completion used to do it. But I wonder if we
> should be worrying about this at this point, even bash-completion
> dropped support for bash < 4 more than two years ago, and even debian
> stable is at 4.1.
> 

I'm +0.

> [...]


Regards  Manlio
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAlF8MVsACgkQscQJ24LbaUSY/wCgkq8CQeVGNpZFtchiLAKXYpxS
wsAAnR0abrQzA1jW+Do7CSuJOZVMRuJu
=zPgk
-----END PGP SIGNATURE-----