[PATCH 00/12] Bash completion rework

[PATCH 00/12] Bash completion rework

[Felipe Contreras]

Hi,

Here's a bunch of patches for bash completion. First of all some tests are
added to verify that there are no regressions, then simple cleanups, then some
fixes, and finally support for external aliases.

So now you can do something like 'git_complete gf git_fetch' and it would just work :)

Cheers.

Felipe Contreras (12):
  tests: add initial bash completion tests
  completion: simplify __gitcomp
  completion: simplify __gitcomp_1
  completion: trivial simplification
  completion: add missing global options
  tests: add more bash completion tests
  completion: simplify command stuff
  completion: simplify _git_bundle
  completion: calculate argument position properly
  completion: add new git_complete helper
  test: add tests for aliases in bash completion
  completion: rename _git and _gitk

 contrib/completion/git-completion.bash |  153 +++++++++-------
 t/t9902-completion.sh                  |  314 ++++++++++++++++++++++++++++++++
 2 files changed, 398 insertions(+), 69 deletions(-)
 create mode 100755 t/t9902-completion.sh

-- 
1.7.10.3.g5a738d

[PATCH 01/12] tests: add initial bash completion tests

[Felipe Contreras]

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

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
new file mode 100755
index 0000000..7eb80dd
--- /dev/null
+++ b/t/t9902-completion.sh
@@ -0,0 +1,201 @@
+#!/bin/sh
+#
+# Copyright (c) 2012 Felipe Contreras
+#
+
+if type bash >/dev/null 2>&1
+then
+	# execute inside bash
+	test -z "$BASH" && exec bash "$0" "$@"
+else
+	echo '1..0 #SKIP skipping bash completion tests; bash not available'
+	exit 0
+fi
+
+test_description='test bash completion'
+
+. ./test-lib.sh
+
+complete ()
+{
+	# do nothing
+	return 0
+}
+
+. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash"
+
+print_comp ()
+{
+	local IFS=$'\n'
+	echo "${COMPREPLY[*]}" > out
+}
+
+_get_comp_words_by_ref ()
+{
+	while [ $# -gt 0 ]; do
+		case "$1" in
+		cur)
+			cur=${_words[_cword]}
+			;;
+		prev)
+			prev=${_words[_cword-1]}
+			;;
+		words)
+			words=("${_words[@]}")
+			;;
+		cword)
+			cword=$_cword
+			;;
+		esac
+		shift
+	done
+}
+
+test_completion ()
+{
+	local -a COMPREPLY _words
+	local _cword
+	_words=( $1 )
+	test $# -gt 1 && echo "$2" > expected
+	(( _cword = ${#_words[@]} - 1 ))
+	_git && print_comp &&
+	test_cmp expected out
+}
+
+test_expect_success 'basic' '
+	cat >expected <<-\EOF &&
+	help 
+	add 
+	gc 
+	reflog 
+	get-tar-commit-id 
+	relink 
+	grep 
+	relink.perl 
+	am 
+	remote 
+	am.sh 
+	help 
+	annotate 
+	apply 
+	archimport.perl 
+	imap-send 
+	archive 
+	bisect 
+	init 
+	repack 
+	bisect.sh 
+	instaweb 
+	repack.sh 
+	blame 
+	instaweb.sh 
+	replace 
+	branch 
+	log 
+	bundle 
+	request-pull 
+	lost-found.sh 
+	request-pull.sh 
+	reset 
+	checkout 
+	cherry 
+	revert 
+	cherry-pick 
+	merge 
+	rm 
+	clean 
+	send-email 
+	clone 
+	send-email.perl 
+	commit 
+	config 
+	shortlog 
+	credential-cache 
+	show 
+	show-branch 
+	credential-store 
+	cvsexportcommit.perl 
+	stage 
+	stash 
+	cvsimport.perl 
+	stash.sh 
+	mergetool 
+	status 
+	cvsserver.perl 
+	mergetool.sh 
+	submodule 
+	describe 
+	submodule.sh 
+	diff 
+	mv 
+	svn 
+	name-rev 
+	svn.perl 
+	notes 
+	tag 
+	difftool 
+	difftool.perl 
+	fetch 
+	pull 
+	pull.sh 
+	filter-branch 
+	push 
+	filter-branch.sh 
+	quiltimport.sh 
+	format-patch 
+	rebase 
+	fsck 
+	rebase.sh 
+	whatchanged 
+	cccmd 
+	proxy 
+	send-pull 
+	EOF
+	test_completion "git" &&
+
+	cat >expected <<-\EOF &&
+	help 
+	help 
+	EOF
+	test_completion "git he"
+
+	cat >expected <<-\EOF &&
+	fetch 
+	filter-branch 
+	filter-branch.sh 
+	format-patch 
+	fsck 
+	EOF
+	test_completion "git f"
+'
+
+test_expect_success 'double dash' '
+	cat >expected <<-\EOF &&
+	--paginate 
+	--no-pager 
+	--git-dir=
+	--bare 
+	--version 
+	--exec-path 
+	--html-path 
+	--work-tree=
+	--namespace=
+	--help 
+	EOF
+	test_completion "git --"
+
+	cat >expected <<-\EOF &&
+	--quiet 
+	--ours 
+	--theirs 
+	--track 
+	--no-track 
+	--merge 
+	--conflict=
+	--orphan 
+	--patch 
+	EOF
+	test_completion "git checkout --"
+'
+
+test_done
-- 
1.7.10.3.g5a738d

[PATCH 02/12] completion: simplify __gitcomp

[Felipe Contreras]

The suffix is never used.

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 31f714d..39f5435 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -304,15 +304,14 @@ __git_ps1 ()
 	fi
 }
 
-# __gitcomp_1 requires 2 arguments
 __gitcomp_1 ()
 {
 	local c IFS=' '$'\t'$'\n'
 	for c in $1; do
-		case "$c$2" in
-		--*=*) printf %s$'\n' "$c$2" ;;
-		*.)    printf %s$'\n' "$c$2" ;;
-		*)     printf %s$'\n' "$c$2 " ;;
+		case "$c" in
+		--*=*) printf %s$'\n' "$c" ;;
+		*.)    printf %s$'\n' "$c" ;;
+		*)     printf %s$'\n' "$c " ;;
 		esac
 	done
 }
@@ -479,7 +478,6 @@ fi
 # 1: List of possible completion words.
 # 2: A prefix to be added to each possible completion word (optional).
 # 3: Generate possible completion matches for this word (optional).
-# 4: A suffix to be appended to each possible completion word (optional).
 __gitcomp ()
 {
 	local cur_="${3-$cur}"
@@ -491,7 +489,7 @@ __gitcomp ()
 	*)
 		local IFS=$'\n'
 		COMPREPLY=($(compgen -P "${2-}" \
-			-W "$(__gitcomp_1 "${1-}" "${4-}")" \
+			-W "$(__gitcomp_1 "${1-}")" \
 			-- "$cur_"))
 		;;
 	esac
-- 
1.7.10.3.g5a738d

[PATCH 03/12] completion: simplify __gitcomp_1

[Felipe Contreras]

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 39f5435..5c66674 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -306,13 +306,13 @@ __git_ps1 ()
 
 __gitcomp_1 ()
 {
-	local c IFS=' '$'\t'$'\n'
+	local c s IFS=' '$'\t'$'\n'
 	for c in $1; do
 		case "$c" in
-		--*=*) printf %s$'\n' "$c" ;;
-		*.)    printf %s$'\n' "$c" ;;
-		*)     printf %s$'\n' "$c " ;;
+		--*=* | *.) s="" ;;
+		*)          s=" " ;;
 		esac
+		echo "$c$s"
 	done
 }
 
-- 
1.7.10.3.g5a738d

[PATCH 04/12] completion: trivial simplification

[Felipe Contreras]

cword-1 is the previous word ($prev).

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 5c66674..3bc8d85 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1656,7 +1656,7 @@ _git_notes ()
 		__gitcomp '--ref'
 		;;
 	,*)
-		case "${words[cword-1]}" in
+		case "$prev" in
 		--ref)
 			__gitcomp_nl "$(__git_refs)"
 			;;
@@ -1682,7 +1682,7 @@ _git_notes ()
 	prune,*)
 		;;
 	*)
-		case "${words[cword-1]}" in
+		case "$prev" in
 		-m|-F)
 			;;
 		*)
-- 
1.7.10.3.g5a738d

[PATCH 05/12] completion: add missing global options

[Felipe Contreras]

Otherwise 'git --foo bar<tab>' fails, also, other options are completely
missing.

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 3bc8d85..c9672b2 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2621,7 +2621,10 @@ _git ()
 		case "$i" in
 		--git-dir=*) __git_dir="${i#--git-dir=}" ;;
 		--bare)      __git_dir="." ;;
-		--version|-p|--paginate) ;;
+		--version) ;;
+		-p|--paginate|--no-pager) ;;
+		--exec-path=*|--html-path|--info-path) ;;
+		--work-tree=*|--namespace=*|--no-replace-objects) ;;
 		--help) command="help"; break ;;
 		*) command="$i"; break ;;
 		esac
@@ -2636,10 +2639,12 @@ _git ()
 			--git-dir=
 			--bare
 			--version
-			--exec-path
+			--exec-path=
 			--html-path
+			--info-path
 			--work-tree=
 			--namespace=
+			--no-replace-objects
 			--help
 			"
 			;;
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 7eb80dd..ee5654d 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -198,4 +198,35 @@ test_expect_success 'double dash' '
 	test_completion "git checkout --"
 '
 
+test_expect_success 'general options' '
+	test_completion "git --ver" "--version " &&
+	test_completion "git --hel" "--help " &&
+	test_completion "git --exe" "--exec-path=" &&
+	test_completion "git --htm" "--html-path " &&
+	test_completion "git --pag" "--paginate " &&
+	test_completion "git --no-p" "--no-pager " &&
+	test_completion "git --git" "--git-dir=" &&
+	test_completion "git --wor" "--work-tree=" &&
+	test_completion "git --nam" "--namespace=" &&
+	test_completion "git --bar" "--bare " &&
+	test_completion "git --inf" "--info-path " &&
+	test_completion "git --no-r" "--no-replace-objects "
+'
+
+test_expect_success 'general options plus command' '
+	test_completion "git --version check" "checkout " &&
+	test_completion "git --paginate check" "checkout " &&
+	test_completion "git --git-dir=foo check" "checkout " &&
+	test_completion "git --bare check" "checkout " &&
+	test_completion "git --help des" "describe " &&
+	test_completion "git --exec-path=foo check" "checkout " &&
+	test_completion "git --html-path check" "checkout " &&
+	test_completion "git --no-pager check" "checkout " &&
+	test_completion "git --work-tree=foo check" "checkout " &&
+	test_completion "git --namespace=foo check" "checkout " &&
+	test_completion "git --paginate check" "checkout " &&
+	test_completion "git --info-path check" "checkout " &&
+	test_completion "git --no-replace-objects check" "checkout "
+'
+
 test_done
-- 
1.7.10.3.g5a738d

[PATCH 06/12] tests: add more bash completion tests

[Felipe Contreras]

These tests try to excercise code that deals with 'words' and 'cword'.

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

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index ee5654d..f1b660f 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -62,6 +62,22 @@ test_completion ()
 	test_cmp expected out
 }
 
+setup_repository ()
+{
+	mkdir "$1" && (
+	cd "$1" &&
+	git init &&
+	test_tick &&
+	git commit --allow-empty -m "Initial"
+	)
+}
+
+test_expect_success 'prepare' '
+	setup_repository one &&
+	git clone one test &&
+	cd test
+'
+
 test_expect_success 'basic' '
 	cat >expected <<-\EOF &&
 	help 
@@ -229,4 +245,43 @@ test_expect_success 'general options plus command' '
 	test_completion "git --no-replace-objects check" "checkout "
 '
 
+test_expect_success 'remote or refspec' '
+	test_completion "git fetch o" "origin " &&
+	test_completion "git fetch origin m" "master:master " &&
+	test_completion "git pull o" "origin " &&
+	test_completion "git pull origin m" "master " &&
+	test_completion "git push o" "origin " &&
+	test_completion "git push origin m" "master "
+'
+
+test_expect_success 'subcomands' '
+	test_completion "git bisect st" "start "
+'
+
+test_expect_success 'has double dash' '
+	test_completion "git add -- foo" ""
+'
+
+test_expect_success 'config' '
+	git config --file=foo color.ui auto &&
+	test_completion "git config --file=foo --get c" "color.ui "
+'
+
+test_expect_success 'other' '
+	cat >expected <<-\EOF &&
+	origin/HEAD 
+	origin/master 
+	EOF
+	test_completion "git branch -r o" &&
+	test_completion "git bundle cr" "create " &&
+
+	echo foobar > tags &&
+	test_completion "git grep f" "foobar " &&
+
+	test_completion "git notes --ref m" "master " &&
+
+	git tag v0.1 &&
+	test_completion "git tag -d v" "v0.1 "
+'
+
 test_done
-- 
1.7.10.3.g5a738d

[PATCH 07/12] completion: simplify command stuff

[Felipe Contreras]

No need to recalculate it.

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c9672b2..1fe11f4 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -721,7 +721,7 @@ __git_complete_revlist ()
 
 __git_complete_remote_or_refspec ()
 {
-	local cur_="$cur" cmd="${words[1]}"
+	local cur_="$cur"
 	local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
 	if [ "$cmd" = "remote" ]; then
 		((c++))
@@ -2599,7 +2599,7 @@ _git_whatchanged ()
 
 _git ()
 {
-	local i c=1 command __git_dir
+	local i c=1 cmd __git_dir
 
 	if [[ -n ${ZSH_VERSION-} ]]; then
 		emulate -L bash
@@ -2625,13 +2625,13 @@ _git ()
 		-p|--paginate|--no-pager) ;;
 		--exec-path=*|--html-path|--info-path) ;;
 		--work-tree=*|--namespace=*|--no-replace-objects) ;;
-		--help) command="help"; break ;;
-		*) command="$i"; break ;;
+		--help) cmd="help"; break ;;
+		*) cmd="$i"; break ;;
 		esac
 		((c++))
 	done
 
-	if [ -z "$command" ]; then
+	if [ -z "$cmd" ]; then
 		case "$cur" in
 		--*)   __gitcomp "
 			--paginate
@@ -2654,10 +2654,10 @@ _git ()
 		return
 	fi
 
-	local completion_func="_git_${command//-/_}"
+	local completion_func="_git_${cmd//-/_}"
 	declare -f $completion_func >/dev/null && $completion_func && return
 
-	local expansion=$(__git_aliased_command "$command")
+	local expansion=$(__git_aliased_command "$cmd")
 	if [ -n "$expansion" ]; then
 		completion_func="_git_${expansion//-/_}"
 		declare -f $completion_func >/dev/null && $completion_func
-- 
1.7.10.3.g5a738d

[PATCH 08/12] completion: simplify _git_bundle

[Felipe Contreras]

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 1fe11f4..c26f96c 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1140,16 +1140,18 @@ _git_branch ()
 
 _git_bundle ()
 {
-	local cmd="${words[2]}"
+	local subcommands='create list-heads verify unbundle'
+	local subcommand="$(__git_find_on_cmdline "$subcommands")"
+
 	case "$cword" in
 	2)
-		__gitcomp "create list-heads verify unbundle"
+		__gitcomp "$subcommands"
 		;;
 	3)
 		# looking for a file
 		;;
 	*)
-		case "$cmd" in
+		case "$subcommand" in
 			create)
 				__git_complete_revlist
 			;;
-- 
1.7.10.3.g5a738d

[PATCH 09/12] completion: calculate argument position properly

[Felipe Contreras]

Positions are currently hard-coded, which means completions in the form
of 'git --foo bar' fail, because positions have been shifted.

This fixes the issue that was spotted in the mailing list regarding
certain aliases[1].

[1] http://thread.gmane.org/gmane.comp.version-control.git/185184

Cc: SZEDER Gábor <szeder@ira.uka.de>
Cc: Scott Bronson <bronson@rinspin.com>
Cc: Nathan Broadbent <nathan.f77@gmail.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash |   35 +++++++++++++++++++++++---------
 t/t9902-completion.sh                  |   21 +++++++++++++++++++
 2 files changed, 46 insertions(+), 10 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c26f96c..60ea224 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -719,10 +719,18 @@ __git_complete_revlist ()
 	__git_complete_revlist_file
 }
 
+__git_get_pos ()
+{
+	echo $(( t = cmd_pos - 2 + $1 ))
+}
+
 __git_complete_remote_or_refspec ()
 {
 	local cur_="$cur"
-	local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
+	local i c remote="" pfx="" lhs=1 no_complete_refspec=0
+
+	c=$(__git_get_pos 2)
+
 	if [ "$cmd" = "remote" ]; then
 		((c++))
 	fi
@@ -974,7 +982,8 @@ __git_aliased_command ()
 # __git_find_on_cmdline requires 1 argument
 __git_find_on_cmdline ()
 {
-	local word subcommand c=1
+	local word subcommand c
+	c=$(__git_get_pos 1)
 	while [ $c -lt $cword ]; do
 		word="${words[c]}"
 		for subcommand in $1; do
@@ -989,7 +998,8 @@ __git_find_on_cmdline ()
 
 __git_has_doubledash ()
 {
-	local c=1
+	local c
+	c=$(__git_get_pos 1)
 	while [ $c -lt $cword ]; do
 		if [ "--" = "${words[c]}" ]; then
 			return 0
@@ -1109,8 +1119,8 @@ _git_bisect ()
 
 _git_branch ()
 {
-	local i c=1 only_local_ref="n" has_r="n"
-
+	local i c only_local_ref="n" has_r="n"
+	c=$(__git_get_pos 1)
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
 		case "$i" in
@@ -1142,8 +1152,9 @@ _git_bundle ()
 {
 	local subcommands='create list-heads verify unbundle'
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
-
-	case "$cword" in
+	local r
+	(( r = cword - cmd_pos + 2 ))
+	case "$r" in
 	2)
 		__gitcomp "$subcommands"
 		;;
@@ -1427,6 +1438,7 @@ __git_match_ctag() {
 _git_grep ()
 {
 	__git_has_doubledash && return
+	local p=$(__git_get_pos 2)
 
 	case "$cur" in
 	--*)
@@ -1447,7 +1459,7 @@ _git_grep ()
 	esac
 
 	case "$cword,$prev" in
-	2,*|*,-*)
+	$p,*|*,-*)
 		if test -r tags; then
 			__gitcomp_nl "$(__git_match_ctag "$cur" tags)"
 			return
@@ -2562,7 +2574,8 @@ _git_svn ()
 
 _git_tag ()
 {
-	local i c=1 f=0
+	local i c f=0
+	c=$(__git_get_pos 1)
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
 		case "$i" in
@@ -2601,7 +2614,7 @@ _git_whatchanged ()
 
 _git ()
 {
-	local i c=1 cmd __git_dir
+	local i c=1 cmd cmd_pos __git_dir
 
 	if [[ -n ${ZSH_VERSION-} ]]; then
 		emulate -L bash
@@ -2656,6 +2669,8 @@ _git ()
 		return
 	fi
 
+	(( cmd_pos = c + 1 ))
+
 	local completion_func="_git_${cmd//-/_}"
 	declare -f $completion_func >/dev/null && $completion_func && return
 
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index f1b660f..b99fb88 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -284,4 +284,25 @@ test_expect_success 'other' '
 	test_completion "git tag -d v" "v0.1 "
 '
 
+test_expect_success 'global options extra checks' '
+	test_completion "git --no-pager fetch o" "origin " &&
+	test_completion "git --no-pager fetch origin m" "master:master " &&
+	test_completion "git --no-pager pull o" "origin " &&
+	test_completion "git --no-pager pull origin m" "master " &&
+	test_completion "git --no-pager push o" "origin " &&
+	test_completion "git --no-pager push origin m" "master " &&
+	test_completion "git --no-pager bisect st" "start " &&
+	test_completion "git --no-pager add -- foo" "" &&
+	test_completion "git --no-pager config --file=foo --get c" "color.ui " &&
+	cat >expected <<-\EOF &&
+	origin/HEAD 
+	origin/master 
+	EOF
+	test_completion "git --no-pager branch -r o" &&
+	test_completion "git --no-pager bundle cr" "create " &&
+	test_completion "git --no-pager grep f" "foobar " &&
+	test_completion "git --no-pager notes --ref m" "master " &&
+	test_completion "git --no-pager tag -d v" "v0.1 "
+'
+
 test_done
-- 
1.7.10.3.g5a738d

[PATCH 10/12] completion: add new git_complete helper

[Felipe Contreras]

This simplifies the completions, and makes it easier to define aliases:

 git_complete gf git_fetch

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 60ea224..6cf1d98 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2616,21 +2616,6 @@ _git ()
 {
 	local i c=1 cmd cmd_pos __git_dir
 
-	if [[ -n ${ZSH_VERSION-} ]]; then
-		emulate -L bash
-		setopt KSH_TYPESET
-
-		# workaround zsh's bug that leaves 'words' as a special
-		# variable in versions < 4.3.12
-		typeset -h words
-
-		# workaround zsh's bug that quotes spaces in the COMPREPLY
-		# array if IFS doesn't contain spaces.
-		typeset -h IFS
-	fi
-
-	local cur words cword prev
-	_get_comp_words_by_ref -n =: cur words cword prev
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
 		case "$i" in
@@ -2683,22 +2668,6 @@ _git ()
 
 _gitk ()
 {
-	if [[ -n ${ZSH_VERSION-} ]]; then
-		emulate -L bash
-		setopt KSH_TYPESET
-
-		# workaround zsh's bug that leaves 'words' as a special
-		# variable in versions < 4.3.12
-		typeset -h words
-
-		# workaround zsh's bug that quotes spaces in the COMPREPLY
-		# array if IFS doesn't contain spaces.
-		typeset -h IFS
-	fi
-
-	local cur words cword prev
-	_get_comp_words_by_ref -n =: cur words cword prev
-
 	__git_has_doubledash && return
 
 	local g="$(__gitdir)"
@@ -2719,16 +2688,42 @@ _gitk ()
 	__git_complete_revlist
 }
 
-complete -o bashdefault -o default -o nospace -F _git git 2>/dev/null \
-	|| complete -o default -o nospace -F _git git
-complete -o bashdefault -o default -o nospace -F _gitk gitk 2>/dev/null \
-	|| complete -o default -o nospace -F _gitk gitk
+foo_wrap ()
+{
+	local cmd=foo_cmd cmd_pos=1
+	if [[ -n ${ZSH_VERSION-} ]]; then
+		emulate -L bash
+		setopt KSH_TYPESET
+
+		# workaround zsh's bug that leaves 'words' as a special
+		# variable in versions < 4.3.12
+		typeset -h words
+
+		# workaround zsh's bug that quotes spaces in the COMPREPLY
+		# array if IFS doesn't contain spaces.
+		typeset -h IFS
+	fi
+	local cur words cword prev
+	_get_comp_words_by_ref -n =: cur words cword prev
+	foo "$@"
+}
+
+git_complete ()
+{
+	local name="${2-$1}"
+	local cmd="${name#git_}"
+	eval "$(typeset -f foo_wrap | sed -e "s/foo_cmd/$cmd/" -e "s/foo/_$name/")"
+	complete -o bashdefault -o default -o nospace -F _${name}_wrap $1 2>/dev/null \
+		|| complete -o default -o nospace -F _${name}_wrap $1
+}
+
+git_complete git
+git_complete gitk
 
 # The following are necessary only for Cygwin, and only are needed
 # when the user has tab-completed the executable name and consequently
 # included the '.exe' suffix.
 #
 if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then
-complete -o bashdefault -o default -o nospace -F _git git.exe 2>/dev/null \
-	|| complete -o default -o nospace -F _git git.exe
+git_complete git.exe git
 fi
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index b99fb88..7b87f4c 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -58,7 +58,7 @@ test_completion ()
 	_words=( $1 )
 	test $# -gt 1 && echo "$2" > expected
 	(( _cword = ${#_words[@]} - 1 ))
-	_git && print_comp &&
+	_git_wrap && print_comp &&
 	test_cmp expected out
 }
 
-- 
1.7.10.3.g5a738d

[PATCH 11/12] test: add tests for aliases in bash completion

[Felipe Contreras]

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

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 7b87f4c..6c61e7a 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -54,11 +54,12 @@ _get_comp_words_by_ref ()
 test_completion ()
 {
 	local -a COMPREPLY _words
-	local _cword
+	local _cword wrap
 	_words=( $1 )
 	test $# -gt 1 && echo "$2" > expected
+	wrap="${3-_git_wrap}"
 	(( _cword = ${#_words[@]} - 1 ))
-	_git_wrap && print_comp &&
+	$wrap && print_comp &&
 	test_cmp expected out
 }
 
@@ -305,4 +306,9 @@ test_expect_success 'global options extra checks' '
 	test_completion "git --no-pager tag -d v" "v0.1 "
 '
 
+test_expect_success 'aliases' '
+	git_complete gf git_fetch &&
+	test_completion "gf o" "origin " "_git_fetch_wrap"
+'
+
 test_done
-- 
1.7.10.3.g5a738d

[PATCH 12/12] completion: rename _git and _gitk

[Felipe Contreras]

Would be helpful to reuse these for zsh completion; it uses _git, and it
cannot be changed.

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6cf1d98..7d663f7 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2612,7 +2612,7 @@ _git_whatchanged ()
 	_git_log
 }
 
-_git ()
+_main_git ()
 {
 	local i c=1 cmd cmd_pos __git_dir
 
@@ -2666,7 +2666,7 @@ _git ()
 	fi
 }
 
-_gitk ()
+_main_gitk ()
 {
 	__git_has_doubledash && return
 
@@ -2710,7 +2710,7 @@ foo_wrap ()
 
 git_complete ()
 {
-	local name="${2-$1}"
+	local name="${2-main_$1}"
 	local cmd="${name#git_}"
 	eval "$(typeset -f foo_wrap | sed -e "s/foo_cmd/$cmd/" -e "s/foo/_$name/")"
 	complete -o bashdefault -o default -o nospace -F _${name}_wrap $1 2>/dev/null \
@@ -2725,5 +2725,5 @@ git_complete gitk
 # included the '.exe' suffix.
 #
 if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then
-git_complete git.exe git
+git_complete git.exe main_git
 fi
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 6c61e7a..ef1323f 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -57,7 +57,7 @@ test_completion ()
 	local _cword wrap
 	_words=( $1 )
 	test $# -gt 1 && echo "$2" > expected
-	wrap="${3-_git_wrap}"
+	wrap="${3-_main_git_wrap}"
 	(( _cword = ${#_words[@]} - 1 ))
 	$wrap && print_comp &&
 	test_cmp expected out
-- 
1.7.10.3.g5a738d

Re: [PATCH 11/12] test: add tests for aliases in bash completion

[Felipe Contreras]

On Sun, Apr 8, 2012 at 6:07 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/t9902-completion.sh |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)

Maybe something like this would be better:

--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -58,7 +58,7 @@ test_completion ()
        _words=( $1 )
        test $# -gt 1 && echo "$2" > expected
        (( _cword = ${#_words[@]} - 1 ))
-       _git_wrap && print_comp &&
+       ${comp_wrap-_git_wrap} && print_comp &&
        test_cmp expected out
 }

@@ -305,4 +305,11 @@ test_expect_success 'global options extra checks' '
        test_completion "git --no-pager tag -d v" "v0.1 "
 '

+test_expect_success 'aliases' '
+       local comp_wrap=_git_fetch_wrap &&
+       git_complete gf git_fetch &&
+       test_completion "gf o" "origin " &&
+       test_completion "gf origin m" "master:master "
+'
+
 test_done


-- 
Felipe Contreras

Re: [PATCH 01/12] tests: add initial bash completion tests

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/t9902-completion.sh |  201 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 201 insertions(+)
>  create mode 100755 t/t9902-completion.sh
>
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> new file mode 100755
> index 0000000..7eb80dd
> --- /dev/null
> +++ b/t/t9902-completion.sh
> @@ -0,0 +1,201 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2012 Felipe Contreras
> +#
> +
> +if type bash >/dev/null 2>&1
> +then
> +	# execute inside bash
> +	test -z "$BASH" && exec bash "$0" "$@"
> +else
> +	echo '1..0 #SKIP skipping bash completion tests; bash not available'
> +	exit 0
> +fi

What shell do you use on your system as /bin/sh (or if you use SHELL_PATH
in the Makefile to override it, what do you use)?

Bash has a special case code to behave in a "posixly correct" mode, where
a lot of extensions are disabled, when invoked as "sh".  Unfortunately, it
still defines BASH_* variables, including BASH, when operating in this
mode.

This unfortunately breaks the above code on a system where /bin/sh is
symlinked to "bash".  "make" runs test scripts using with SHELL_PATH,
defaulting to /bin/sh, and you say "Ah, bash is available, oh, we have
BASH defined so let's keep going" without doing an "exec", so later,

> +. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash"

may trigger errors like this:

  sh: .../git-completion.bash: line 128: syntax error near unexpected token `<'
  sh: .../git-completion.bash: line 128: `    done < <(git config -z --get...

depending on what bash-ism is used in the then-current version of that
file.

So we may want to be a bit more careful like this:

if test "z${BASH%/bash}" != "z${BASH}"
then
	: ends with bash so we are already running bash as bash
elif type bash >/dev/null 2>&1
then
	exec bash "$0" "$@"
else
	echo '1..0 #SKIP skipping bash completion tests; bash not available'
	exit 0
fi

This incidentally avoids running type twice when we are already running as
bash.

> +
> +test_description='test bash completion'
> +
> +. ./test-lib.sh
> +
> +complete ()
> +{
> +	# do nothing
> +	return 0
> +}
> +
> +. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash"
> +
> +print_comp ()
> +{
> +	local IFS=$'\n'
> +	echo "${COMPREPLY[*]}" > out
> +}
> +
> +_get_comp_words_by_ref ()
> +{
> +	while [ $# -gt 0 ]; do
> +		case "$1" in
> +		cur)
> +			cur=${_words[_cword]}
> +			;;
> +		prev)
> +			prev=${_words[_cword-1]}
> +			;;
> +		words)
> +			words=("${_words[@]}")
> +			;;
> +		cword)
> +			cword=$_cword
> +			;;
> +		esac
> +		shift
> +	done
> +}

Could you add an explanation before this function to describe what is
going on?  The completion script being tested relies on the shell function
of the same name defined there to behave one way, and this overrides it
with a different implementation.  Often such an override is an effective
way to intercept and expose the internal state to test framework, but the
above does not seem to be such an override, and it is unclear what is
going on here.

Re: [PATCH 01/12] tests: add initial bash completion tests

[Jeff King]

On Sat, Apr 07, 2012 at 09:25:29PM -0700, Junio C Hamano wrote:

> Bash has a special case code to behave in a "posixly correct" mode, where
> a lot of extensions are disabled, when invoked as "sh".  Unfortunately, it
> still defines BASH_* variables, including BASH, when operating in this
> mode.
> [...]
> So we may want to be a bit more careful like this:
> 
> if test "z${BASH%/bash}" != "z${BASH}"
> then
> 	: ends with bash so we are already running bash as bash

If bash is in posix mode (including "bash --posix" and being invoked as
"bin/sh"), it will set POSIXLY_CORRECT (but not export it). Similarly,
if POSIXLY_CORRECT is set in the outer environment, it will act more
like sh. So maybe that would be a better test.

-Peff

Re: [PATCH 01/12] tests: add initial bash completion tests

[Jeff King]

On Sun, Apr 08, 2012 at 06:07:48AM +0300, Felipe Contreras wrote:

> +	cat >expected <<-\EOF &&
> +	fetch 
> +	filter-branch 
> +	filter-branch.sh 
> +	format-patch 
> +	fsck 
> +	EOF
> +	test_completion "git f"

This test fails for me. The problem is that I have other git-f* programs
in my PATH, and the completion finds and displays them. In other words,
the environment outside the test suite can pollute the result.

I'm not sure of the right solution. We can't just sanitize the PATH in
test-lib.sh, since those git programs might be in /usr/bin or some other
directory containing other commands necessary to run the test suite. We
could sanitize it temporarily just for the _git completion invocation,
which consists only of builtins (and we know we're running under bash,
so we can trust that things like "test" are builtins). But it still
feels horribly hacky.

That patch might look like this:

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 7eb80dd..713f4b1 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -58,8 +58,11 @@ test_completion ()
 	_words=( $1 )
 	test $# -gt 1 && echo "$2" > expected
 	(( _cword = ${#_words[@]} - 1 ))
+	saved_path=$PATH
+	PATH=$MINIMAL_PATH
 	_git && print_comp &&
 	test_cmp expected out
+	PATH=$saved_path
 }
 
 test_expect_success 'basic' '
diff --git a/t/test-lib.sh b/t/test-lib.sh
index b7d7100..348b68d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -466,12 +466,14 @@ then
 	IFS=$OLDIFS
 	PATH=$GIT_VALGRIND/bin:$PATH
 	GIT_EXEC_PATH=$GIT_VALGRIND/bin
+	MINIMAL_PATH=$GIT_VALGRIND/bin
 	export GIT_VALGRIND
 elif test -n "$GIT_TEST_INSTALLED" ; then
 	GIT_EXEC_PATH=$($GIT_TEST_INSTALLED/git --exec-path)  ||
 	error "Cannot run git from $GIT_TEST_INSTALLED."
 	PATH=$GIT_TEST_INSTALLED:$GIT_BUILD_DIR:$PATH
 	GIT_EXEC_PATH=${GIT_TEST_EXEC_PATH:-$GIT_EXEC_PATH}
+	MINIMAL_PATH=$GIT_TEST_INSTALLED:$GIT_BUILD_DIR
 else # normal case, use ../bin-wrappers only unless $with_dashes:
 	git_bin_dir="$GIT_BUILD_DIR/bin-wrappers"
 	if ! test -x "$git_bin_dir/git" ; then
@@ -482,8 +484,10 @@ else # normal case, use ../bin-wrappers only unless $with_dashes:
 	fi
 	PATH="$git_bin_dir:$PATH"
 	GIT_EXEC_PATH=$GIT_BUILD_DIR
+	MINIMAL_PATH=$git_bin_dir
 	if test -n "$with_dashes" ; then
 		PATH="$GIT_BUILD_DIR:$PATH"
+		MINIMAL_PATH=$MINIMAL_PATH:$GIT_BUILD_DIR
 	fi
 fi
 GIT_TEMPLATE_DIR="$GIT_BUILD_DIR"/templates/blt

Re: [PATCH 01/12] tests: add initial bash completion tests

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Apr 07, 2012 at 09:25:29PM -0700, Junio C Hamano wrote:
>
>> Bash has a special case code to behave in a "posixly correct" mode, where
>> a lot of extensions are disabled, when invoked as "sh".  Unfortunately, it
>> still defines BASH_* variables, including BASH, when operating in this
>> mode.
>> [...]
>> So we may want to be a bit more careful like this:
>> 
>> if test "z${BASH%/bash}" != "z${BASH}"
>> then
>> 	: ends with bash so we are already running bash as bash
>
> If bash is in posix mode (including "bash --posix" and being invoked as
> "bin/sh"), it will set POSIXLY_CORRECT (but not export it). Similarly,
> if POSIXLY_CORRECT is set in the outer environment, it will act more
> like sh. So maybe that would be a better test.

Yes, but the check needs to be careful to make sure the shell that is
running the check is indeed bash, so that it will explicitly exec bash for
somebody who is running dash but exports POSIXLY_CORRECT to make GNU
programs (other than bash) behave more standard compliant way.

In other words,

	if test -n "$POSIXLY_CORRECT" && test -n "$BASH"
	then
        	: we are running bash under posix mode
	elif ...

or somesuch.

Re: [PATCH 01/12] tests: add initial bash completion tests

[Jeff King]

On Sat, Apr 07, 2012 at 10:41:32PM -0700, Junio C Hamano wrote:

> > If bash is in posix mode (including "bash --posix" and being invoked as
> > "bin/sh"), it will set POSIXLY_CORRECT (but not export it). Similarly,
> > if POSIXLY_CORRECT is set in the outer environment, it will act more
> > like sh. So maybe that would be a better test.
> 
> Yes, but the check needs to be careful to make sure the shell that is
> running the check is indeed bash, so that it will explicitly exec bash for
> somebody who is running dash but exports POSIXLY_CORRECT to make GNU
> programs (other than bash) behave more standard compliant way.

Sorry, I thought that was obvious. Yes, this:

> In other words,
> 
> 	if test -n "$POSIXLY_CORRECT" && test -n "$BASH"
> 	then
>         	: we are running bash under posix mode
> 	elif ...
> 
> or somesuch.

is what I meant. Replace the "does it end in /bash" bit with
"POSIXLY_CORRECT" but, keep the $BASH check.

-Peff

Re: [PATCH 01/12] tests: add initial bash completion tests

[Jeff King]

On Sun, Apr 08, 2012 at 01:42:51AM -0400, Jeff King wrote:

> > Yes, but the check needs to be careful to make sure the shell that is
> > running the check is indeed bash, so that it will explicitly exec bash for
> > somebody who is running dash but exports POSIXLY_CORRECT to make GNU
> > programs (other than bash) behave more standard compliant way.
> 
> Sorry, I thought that was obvious. Yes, this:
> 
> > In other words,
> > 
> > 	if test -n "$POSIXLY_CORRECT" && test -n "$BASH"
> > 	then
> >         	: we are running bash under posix mode
> > 	elif ...
> > 
> > or somesuch.
> 
> is what I meant. Replace the "does it end in /bash" bit with
> "POSIXLY_CORRECT" but, keep the $BASH check.

BTW, if you intend to exec bash when we see a posix bash, I think you
would want to unset POSIXLY_CORRECT in case it is exported. Otherwise,
doing this:

 POSIXLY_CORRECT=1 bash ./t9902-*

would loop forever, trying to restart bash, but ending up in the POSIX
version each time. So all together:

  if test -n "$BASH" && test -z "$POSIXLY_CORRECT"; then
          : we are in full-on bash mode. Great.
  elif type bash >/dev/null 2>&1; then
          # We are not bash, or are in posix mode. Run a new bash,
          # making sure not to let any posix environment variable
          # propagate.
          unset POSIXLY_CORRECT
          exec bash "$0" "$@"
  else
          echo '1..0 # SKIP skipping bash completion tests; bash not available'
          exit 0
  fi

-Peff

PS I wondered if we can continue past the "exec" in the second branch if
   it fails, and if we should be exiting explicitly. POSIX specifies
   that "exec" with a command never returns, and that is what dash does.
   Bash will continue after a failed exec. I expected it not to do so
   in posix mode, but it seems to. Which is perhaps a bug in bash, but
   one we might want to deal with. Granted, the likelihood of "type"
   succeeding but the exec failing is low, so it may not be worth caring
   about.

Re: [PATCH 01/12] tests: add initial bash completion tests

[Andreas Schwab]

Jeff King <peff@peff.net> writes:

>    Bash will continue after a failed exec.

Only when interactive.  A non-interactive shell respects the execfail
shopt (off by default).

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 05/12] completion: add missing global options

[John Keeping]

On Sun, Apr 08, 2012 at 06:07:52AM +0300, Felipe Contreras wrote:
> @@ -2621,7 +2621,10 @@ _git ()
>  		case "$i" in
>  		--git-dir=*) __git_dir="${i#--git-dir=}" ;;
>  		--bare)      __git_dir="." ;;
> -		--version|-p|--paginate) ;;
> +		--version) ;;
> +		-p|--paginate|--no-pager) ;;
> +		--exec-path=*|--html-path|--info-path) ;;

Since exec-path's argument is optional, shouldn't this be:

  +		--exec-path*|--html-path|--info-path) ;;

> +		--work-tree=*|--namespace=*|--no-replace-objects) ;;

It would be nice to accept options with argument with and without the
equals sign, as the git command does, so both:
    git --git-dir=/foo/bar
and
    git --git-dir /foo/bar

I guess this looks like (adding in -c as well):

		--git-dir)  ((c++)) ; __git_dir="${words[c]}" ;;
                --work-tree|--namespacei|-c) ((c++)) ;;


>  		--help) command="help"; break ;;
>  		*) command="$i"; break ;;
>  		esac
> @@ -2636,10 +2639,12 @@ _git ()
>  			--git-dir=
>  			--bare
>  			--version
> -			--exec-path
> +			--exec-path=

I think this change is incorrect since giving --exec-path without an
argument is perfectly fine and has a defined meaning.

>  			--html-path
> +			--info-path
>  			--work-tree=
>  			--namespace=
> +			--no-replace-objects
>  			--help

Should "-c" be in this list as well?

>  			"
>  			;;

-- 
John

Re: [PATCH 01/12] tests: add initial bash completion tests

[John Keeping]

On Sun, Apr 08, 2012 at 06:07:48AM +0300, Felipe Contreras wrote:
> +test_completion ()
> +{
> +	local -a COMPREPLY _words
> +	local _cword
> +	_words=( $1 )
> +	test $# -gt 1 && echo "$2" > expected
> +	(( _cword = ${#_words[@]} - 1 ))
> +	_git && print_comp &&
> +	test_cmp expected out
> +}

Could you check that _git doesn't output anything to stderr here?  I
can't see any reason why a completion command should ever do so
legitimately and it's generally annoying if one does.

-- 
John

Re: [PATCH 01/12] tests: add initial bash completion tests

[Jonathan Nieder]

(dropping Shawn from cc list since he hasn't been working on the
 completion script for a couple of years now)
Jeff King wrote:
> On Sun, Apr 08, 2012 at 06:07:48AM +0300, Felipe Contreras wrote:

>> +	cat >expected <<-\EOF &&
>> +	fetch 
>> +	filter-branch 
>> +	filter-branch.sh 
>> +	format-patch 
>> +	fsck 
>> +	EOF
>> +	test_completion "git f"
>
> This test fails for me. The problem is that I have other git-f* programs
> in my PATH
[...]
>                                     We can't just sanitize the PATH in
> test-lib.sh, since those git programs might be in /usr/bin or some other
> directory containing other commands necessary to run the test suite. We
> could sanitize it temporarily just for the _git completion invocation,
> which consists only of builtins (and we know we're running under bash,
> so we can trust that things like "test" are builtins). But it still
> feels horribly hacky.

Not to mention that it would break the TEST_INSTALLED_GIT facility.

Is there some reason we care about the exact list of completions for
"git f"?  The above also looks a little bogus because if someone were
to remove the +x bit on the git-filter-branch.sh source file or ensure
it is not on the PATH when tests are run (I have no particular opinion
about whether that's a good idea; it's just an example) then this
would change the actual output.  That doesn't seem particularly
important to check for when talking about how the tab completion
behaves on the installed system.

Maybe it would make sense to check a few representative items.

	# builtin
	grep "^fetch\$" out &&

	# builtin whose implementation is not in builtin/cmd.c
	grep "^format-patch\$" out &&

	# script
	grep "^filter-branch\$" out &&

	# plumbing
	! grep "^fsck-objects\$" out &&

	sort out >out.sorted &&
	test_cmp out.sorted out

Re: [PATCH 01/12] tests: add initial bash completion tests

[Jeff King]

On Sun, Apr 08, 2012 at 11:07:51AM +0200, Andreas Schwab wrote:

> >    Bash will continue after a failed exec.
> 
> Only when interactive.  A non-interactive shell respects the execfail
> shopt (off by default).

Thanks, I didn't know that. Let's not worry about error-handling after
the exec then.

-Peff

Re: [PATCH 01/12] tests: add initial bash completion tests

[Jeff King]

On Sun, Apr 08, 2012 at 05:30:02AM -0500, Jonathan Nieder wrote:

> > This test fails for me. The problem is that I have other git-f* programs
> > in my PATH
> [...]
> Is there some reason we care about the exact list of completions for
> "git f"?
> [...]
> Maybe it would make sense to check a few representative items.
> 
> 	# builtin
> 	grep "^fetch\$" out &&

I think your approach (to stop worrying about stopping pollution from
the environment, and start being more specific in what the tests care
about) is much saner than the snippet I posted.

-Peff

Re: [PATCH 05/12] completion: add missing global options

[SZEDER Gábor]

Hi,


On Sun, Apr 08, 2012 at 06:07:52AM +0300, Felipe Contreras wrote:
> Otherwise 'git --foo bar<tab>' fails, also, other options are completely
> missing.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/completion/git-completion.bash |    9 +++++++--
>  t/t9902-completion.sh                  |   31 +++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 3bc8d85..c9672b2 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2621,7 +2621,10 @@ _git ()
>  		case "$i" in
>  		--git-dir=*) __git_dir="${i#--git-dir=}" ;;
>  		--bare)      __git_dir="." ;;
> -		--version|-p|--paginate) ;;
> +		--version) ;;
> +		-p|--paginate|--no-pager) ;;
> +		--exec-path=*|--html-path|--info-path) ;;
> +		--work-tree=*|--namespace=*|--no-replace-objects) ;;
>  		--help) command="help"; break ;;
>  		*) command="$i"; break ;;
>  		esac

This is not future proof, it will break again when we add yet another
global option.  This is also incomplete, because the '-c
<configvar=value>' option is missing.

I send a patch to fix this issue a couple of months ago but it got
dropped on the floor.

  http://thread.gmane.org/gmane.comp.version-control.git/180650

My patch handles the -c global option and it won't break when we add
a new global option, so I think it should replace the above hunk.

Please feel free to copy the commit message, too, for the benefit of
other developers who might read the log message a year later.


> +test_expect_success 'general options plus command' '
> +	test_completion "git --version check" "checkout " &&
> +	test_completion "git --paginate check" "checkout " &&
> +	test_completion "git --git-dir=foo check" "checkout " &&
> +	test_completion "git --bare check" "checkout " &&
> +	test_completion "git --help des" "describe " &&
> +	test_completion "git --exec-path=foo check" "checkout " &&
> +	test_completion "git --html-path check" "checkout " &&
> +	test_completion "git --no-pager check" "checkout " &&
> +	test_completion "git --work-tree=foo check" "checkout " &&
> +	test_completion "git --namespace=foo check" "checkout " &&
> +	test_completion "git --paginate check" "checkout " &&
> +	test_completion "git --info-path check" "checkout " &&
> +	test_completion "git --no-replace-objects check" "checkout "
> +'

Then you could add a 

	test_completion "git -c 'name=value' check" "checkout "

test, too.


Best,
Gábor

Re: [PATCH 02/12] completion: simplify __gitcomp

[SZEDER Gábor]

Hi,


On Sun, Apr 08, 2012 at 06:07:49AM +0300, Felipe Contreras wrote:
> The suffix is never used.

The suffix was used to offer completion after 'git config
branch.<TAB>' ending with a period.  It is not used by git's
completion script anymore, because nowadays this case is handled by
the __gitcomp_nl helper() function.

However, users can provide custom completion functions to their
aliases or additional git commands, and there might be a custom
completion function out there that passes a suffix to __gitcomp().

We don't want to break those custom completion functions, so I think
the suffix should stay.


Best,
Gábor



> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/completion/git-completion.bash |   12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 31f714d..39f5435 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -304,15 +304,14 @@ __git_ps1 ()
>  	fi
>  }
>  
> -# __gitcomp_1 requires 2 arguments
>  __gitcomp_1 ()
>  {
>  	local c IFS=' '$'\t'$'\n'
>  	for c in $1; do
> -		case "$c$2" in
> -		--*=*) printf %s$'\n' "$c$2" ;;
> -		*.)    printf %s$'\n' "$c$2" ;;
> -		*)     printf %s$'\n' "$c$2 " ;;
> +		case "$c" in
> +		--*=*) printf %s$'\n' "$c" ;;
> +		*.)    printf %s$'\n' "$c" ;;
> +		*)     printf %s$'\n' "$c " ;;
>  		esac
>  	done
>  }
> @@ -479,7 +478,6 @@ fi
>  # 1: List of possible completion words.
>  # 2: A prefix to be added to each possible completion word (optional).
>  # 3: Generate possible completion matches for this word (optional).
> -# 4: A suffix to be appended to each possible completion word (optional).
>  __gitcomp ()
>  {
>  	local cur_="${3-$cur}"
> @@ -491,7 +489,7 @@ __gitcomp ()
>  	*)
>  		local IFS=$'\n'
>  		COMPREPLY=($(compgen -P "${2-}" \
> -			-W "$(__gitcomp_1 "${1-}" "${4-}")" \
> +			-W "$(__gitcomp_1 "${1-}")" \
>  			-- "$cur_"))
>  		;;
>  	esac
> -- 
> 1.7.10.3.g5a738d
> 

Re: [PATCH 02/12] completion: simplify __gitcomp

[Felipe Contreras]

2012/4/8 SZEDER Gábor <szeder@ira.uka.de>:
> On Sun, Apr 08, 2012 at 06:07:49AM +0300, Felipe Contreras wrote:
>> The suffix is never used.
>
> The suffix was used to offer completion after 'git config
> branch.<TAB>' ending with a period.  It is not used by git's
> completion script anymore, because nowadays this case is handled by
> the __gitcomp_nl helper() function.
>
> However, users can provide custom completion functions to their
> aliases or additional git commands, and there might be a custom
> completion function out there that passes a suffix to __gitcomp().

Yes, there _might_ be, and they would have to be fixed; APIs change.
However, this is highly theoretical, what suffix could anybody
possibly want? I believe __gitcomp_1 already does the job of adding
sensible suffixes, and there's nothing else anybody else could
possibly want.

In any case, shouldn't compgen -S "${4-}" do the trick?

> We don't want to break those custom completion functions, so I think
> the suffix should stay.

So we should never make any cleanups because some custom completion
functions might theoretically break? I think at some point we need to
accept that nobody is using this argument anyway (and they should).

Cheers.

-- 
Felipe Contreras

Re: [PATCH 02/12] completion: simplify __gitcomp

[Jonathan Nieder]

Felipe Contreras wrote:

> So we should never make any cleanups because some custom completion
> functions might theoretically break?

No, tasteful cleanups that don't break custom completion functions are
very welcome.

Also if you have evidence that this is definitely (not just possibly)
only a theoretical problem, then it would not be a regression.  But I
actually find it likely that custom completion code would have
imitated the old completion code in git that used the suffix argument,
so...

Can you remind me what the benefit that the user is getting from this
patch in exchange for us breaking their tab completion?  The title
("simplify __gitcomp") tells me exactly nothing about its impact
except that it is perhaps supposed to result in no behavior change.

Sorry for the fuss and hope that helps,
Jonathan

Re: [PATCH 02/12] completion: simplify __gitcomp

[Felipe Contreras]

On Sun, Apr 8, 2012 at 4:27 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> So we should never make any cleanups because some custom completion
>> functions might theoretically break?
>
> No, tasteful cleanups that don't break custom completion functions are
> very welcome.
>
> Also if you have evidence that this is definitely (not just possibly)
> only a theoretical problem, then it would not be a regression.  But I
> actually find it likely that custom completion code would have
> imitated the old completion code in git that used the suffix argument,
> so...

Show me an example. If you do something like:

 __gitcomp "foo bar opt=" "" "" "suffix"

The result would be 'foo suffix' 'bar suffix' 'opt=suffix'. How could
that *possibly* be useful?

> Can you remind me what the benefit that the user is getting from this
> patch

Absolutely none.

> in exchange for us breaking their tab completion?

Whoa! Breaking their tab completion? Where? Can you show me some evidence?

>  The title
> ("simplify __gitcomp") tells me exactly nothing about its impact
> except that it is perhaps supposed to result in no behavior change.

That is correct. No functional changes. I would need to update the
tests if something changed, right?

Cheers.

-- 
Felipe Contreras

Re: [PATCH 02/12] completion: simplify __gitcomp

[Felipe Contreras]

Hi,

I've looked at the history of __gitcomp and __gitcomp_nl, now I think
it makes sense to keep their arguments similar, at least for the time
being.

So I suggest this patch to be dropped.

On Sun, Apr 8, 2012 at 4:27 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> So we should never make any cleanups because some custom completion
>> functions might theoretically break?
>
> No, tasteful cleanups that don't break custom completion functions are
> very welcome.
>
> Also if you have evidence that this is definitely (not just possibly)
> only a theoretical problem, then it would not be a regression.  But I
> actually find it likely that custom completion code would have
> imitated the old completion code in git that used the suffix argument,
> so...
>
> Can you remind me what the benefit that the user is getting from this
> patch in exchange for us breaking their tab completion?  The title
> ("simplify __gitcomp") tells me exactly nothing about its impact
> except that it is perhaps supposed to result in no behavior change.
>
> Sorry for the fuss and hope that helps,
> Jonathan



-- 

Re: [PATCH 02/12] completion: simplify __gitcomp

[Jonathan Nieder]

(dropping Shawn from cc since I'm not under the impression he works
 on the completion script these days)
Felipe Contreras wrote:

> Whoa! Breaking their tab completion? Where? Can you show me some evidence?

If you weren't listening before, I'm not sure what I can add now[*].

Luckily, I already said what I needed to say.  Yes, cleanups can be
good when they don't break things, and no, cleanups that break things
are not good.  Sometimes it is not obvious which category each case
falls into.  As you well know, "tests pass" is not enough (e.g.,
sometimes there are no tests!).  If you want someone to argue with,
you can find someone else.

Sorry,
Jonathan

[*] To be extra, extra clear: I never said your patch breaks people's
custom tab completion scripts.  I said it might do so and that that is
not very comforting to apply the patch when no one seems to have
thought carefully about how to investigate that and mitigate the
damage if there is any.

Re: [PATCH 02/12] completion: simplify __gitcomp

[Jonathan Nieder]

(-cc: Shawn)
Felipe Contreras wrote:

> I've looked at the history of __gitcomp and __gitcomp_nl, now I think
> it makes sense to keep their arguments similar, at least for the time
> being.
>
> So I suggest this patch to be dropped.

Thanks for looking into it, and sorry I responded to a heated response
instead of waiting for a calmer one.

I do encourage you to examine your communication style.

Cheers,
Jonathan

Re: [PATCH 02/12] completion: simplify __gitcomp

[Felipe Contreras]

On Sun, Apr 8, 2012 at 5:36 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> (dropping Shawn from cc since I'm not under the impression he works
>  on the completion script these days)
> Felipe Contreras wrote:
>
>> Whoa! Breaking their tab completion? Where? Can you show me some evidence?
>
> If you weren't listening before, I'm not sure what I can add now[*].

This is what you said; "in exchange for us breaking their tab
completion". There's a big difference between "breaking", and
"*potentially* breaking". I have never seen any evidence of tab
completion actually being broken.

> Luckily, I already said what I needed to say.  Yes, cleanups can be
> good when they don't break things, and no, cleanups that break things
> are not good.  Sometimes it is not obvious which category each case
> falls into.  As you well know, "tests pass" is not enough (e.g.,
> sometimes there are no tests!).  If you want someone to argue with,
> you can find someone else.

Fortunately there's no evidence of anything being broken; all the
issues mentioned are *theoretical*, and there's not even an example of
a custom completion that would be broken by this.

-- 
Felipe Contreras

Re: [PATCH 02/12] completion: simplify __gitcomp

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> (-cc: Shawn)
> Felipe Contreras wrote:
>
>> I've looked at the history of __gitcomp and __gitcomp_nl, now I think
>> it makes sense to keep their arguments similar, at least for the time
>> being.
>>
>> So I suggest this patch to be dropped.
>
> Thanks for looking into it, and sorry I responded to a heated response
> instead of waiting for a calmer one.

Good to see a civilized discussion of patches on technical merits.

Thanks.

Re: [PATCH 02/12] completion: simplify __gitcomp

[Johannes Sixt]

Am 08.04.2012 16:58, schrieb Felipe Contreras:
> On Sun, Apr 8, 2012 at 5:36 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> (dropping Shawn from cc since I'm not under the impression he works
>>  on the completion script these days)
>> Felipe Contreras wrote:
>>
>>> Whoa! Breaking their tab completion? Where? Can you show me some evidence?
>>
>> If you weren't listening before, I'm not sure what I can add now[*].
> 
> This is what you said; "in exchange for us breaking their tab
> completion". There's a big difference between "breaking", and
> "*potentially* breaking". I have never seen any evidence of tab
> completion actually being broken.

I have:

http://thread.gmane.org/gmane.comp.version-control.msysgit/13310/focus=13335
http://thread.gmane.org/gmane.comp.version-control.git/185184/focus=185189

-- Hannes

Re: [PATCH 02/12] completion: simplify __gitcomp

[Felipe Contreras]

On Mon, Apr 9, 2012 at 9:57 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 08.04.2012 16:58, schrieb Felipe Contreras:
>> On Sun, Apr 8, 2012 at 5:36 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>> (dropping Shawn from cc since I'm not under the impression he works
>>>  on the completion script these days)
>>> Felipe Contreras wrote:
>>>
>>>> Whoa! Breaking their tab completion? Where? Can you show me some evidence?
>>>
>>> If you weren't listening before, I'm not sure what I can add now[*].
>>
>> This is what you said; "in exchange for us breaking their tab
>> completion". There's a big difference between "breaking", and
>> "*potentially* breaking". I have never seen any evidence of tab
>> completion actually being broken.
>
> I have:
>
> http://thread.gmane.org/gmane.comp.version-control.msysgit/13310/focus=13335
> http://thread.gmane.org/gmane.comp.version-control.git/185184/focus=185189

I meant being broken by this patch.

Plus this patch series actually fixes those issues.

-- 
Felipe Contreras

Re: [PATCH 01/12] tests: add initial bash completion tests

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Apr 08, 2012 at 05:30:02AM -0500, Jonathan Nieder wrote:
>
>> > This test fails for me. The problem is that I have other git-f* programs
>> > in my PATH
>> [...]
>> Is there some reason we care about the exact list of completions for
>> "git f"?
>> [...]
>> Maybe it would make sense to check a few representative items.
>> 
>> 	# builtin
>> 	grep "^fetch\$" out &&
>
> I think your approach (to stop worrying about stopping pollution from
> the environment, and start being more specific in what the tests care
> about) is much saner than the snippet I posted.

I have to agree with that; it might give unexpeceted results if the user
has a $HOME/bin/git-update-index or something, though.

Re: [PATCH 01/12] tests: add initial bash completion tests

[Felipe Contreras]

On Sun, Apr 8, 2012 at 7:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  t/t9902-completion.sh |  201 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 201 insertions(+)
>>  create mode 100755 t/t9902-completion.sh
>>
>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>> new file mode 100755
>> index 0000000..7eb80dd
>> --- /dev/null
>> +++ b/t/t9902-completion.sh
>> @@ -0,0 +1,201 @@
>> +#!/bin/sh
>> +#
>> +# Copyright (c) 2012 Felipe Contreras
>> +#
>> +
>> +if type bash >/dev/null 2>&1
>> +then
>> +     # execute inside bash
>> +     test -z "$BASH" && exec bash "$0" "$@"
>> +else
>> +     echo '1..0 #SKIP skipping bash completion tests; bash not available'
>> +     exit 0
>> +fi
>
> What shell do you use on your system as /bin/sh (or if you use SHELL_PATH
> in the Makefile to override it, what do you use)?

/bin/sh is a symlink to bash on my system, and although bash defines
POSIXLY_CORRECT, the tests pass just fine. I even tried to run /bin/sh
--posix.

-- 
Felipe Contreras

Re: [PATCH 07/12] completion: simplify command stuff

[SZEDER Gábor]

Hi,


On Sun, Apr 08, 2012 at 06:07:54AM +0300, Felipe Contreras wrote:
> No need to recalculate it.

Based on this short description I would expect that this "command
stuff" is calculated somewhere, and a helper function is changed to
use the already calculated value ...

> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index c9672b2..1fe11f4 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -721,7 +721,7 @@ __git_complete_revlist ()
>  
>  __git_complete_remote_or_refspec ()
>  {
> -	local cur_="$cur" cmd="${words[1]}"
> +	local cur_="$cur"
>  	local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
>  	if [ "$cmd" = "remote" ]; then
>  		((c++))
> @@ -2599,7 +2599,7 @@ _git_whatchanged ()
>  
>  _git ()
>  {
> -	local i c=1 command __git_dir
> +	local i c=1 cmd __git_dir
>  
>  	if [[ -n ${ZSH_VERSION-} ]]; then
>  		emulate -L bash
> @@ -2625,13 +2625,13 @@ _git ()
>  		-p|--paginate|--no-pager) ;;
>  		--exec-path=*|--html-path|--info-path) ;;
>  		--work-tree=*|--namespace=*|--no-replace-objects) ;;
> -		--help) command="help"; break ;;
> -		*) command="$i"; break ;;
> +		--help) cmd="help"; break ;;
> +		*) cmd="$i"; break ;;
>  		esac
>  		((c++))
>  	done
>  
> -	if [ -z "$command" ]; then
> +	if [ -z "$cmd" ]; then
>  		case "$cur" in
>  		--*)   __gitcomp "
>  			--paginate
> @@ -2654,10 +2654,10 @@ _git ()
>  		return
>  	fi
>  
> -	local completion_func="_git_${command//-/_}"
> +	local completion_func="_git_${cmd//-/_}"
>  	declare -f $completion_func >/dev/null && $completion_func && return
>  
> -	local expansion=$(__git_aliased_command "$command")
> +	local expansion=$(__git_aliased_command "$cmd")
>  	if [ -n "$expansion" ]; then
>  		completion_func="_git_${expansion//-/_}"
>  		declare -f $completion_func >/dev/null && $completion_func
> -- 
> 1.7.10.3.g5a738d
> 

... but the bulk of this change is in the top-level _git() function,
as it renames a variable there to match the variable name in the
helper function.  Perhaps it should be the other way around.

However, this change "promotes" the command (or cmd) variable from
being just a variable in _git() to a variable that is used in other
completion functions, too.  Why not go one more step further, and
denote this by adding a __git prefix (i.e. renaming the variable to
__git_command)?


Best,
Gábor

Re: [PATCH 07/12] completion: simplify command stuff

[Felipe Contreras]

2012/4/12 SZEDER Gábor <szeder@ira.uka.de>:
> Hi,
>
>
> On Sun, Apr 08, 2012 at 06:07:54AM +0300, Felipe Contreras wrote:
>> No need to recalculate it.
>
> Based on this short description I would expect that this "command
> stuff" is calculated somewhere, and a helper function is changed to
> use the already calculated value ...

[...]

> ... but the bulk of this change is in the top-level _git() function,
> as it renames a variable there to match the variable name in the
> helper function.  Perhaps it should be the other way around.
>
> However, this change "promotes" the command (or cmd) variable from
> being just a variable in _git() to a variable that is used in other
> completion functions, too.  Why not go one more step further, and
> denote this by adding a __git prefix (i.e. renaming the variable to
> __git_command)?

After thinking more about it, and analyzing the next patches, I don't
think it makes sense to have such a variable; it's only used by
__git_complete_remote_or_refspec, and it would be easier to make it an
argument to that function; that would simplify this patch series a
lot.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 10/12] completion: add new git_complete helper

[SZEDER Gábor]

Hi,


On Sun, Apr 08, 2012 at 06:07:57AM +0300, Felipe Contreras wrote:
> This simplifies the completions, and makes it easier to define aliases:
> 
>  git_complete gf git_fetch
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/completion/git-completion.bash |   69 +++++++++++++++-----------------
>  t/t9902-completion.sh                  |    2 +-
>  2 files changed, 33 insertions(+), 38 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 60ea224..6cf1d98 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2616,21 +2616,6 @@ _git ()
>  {
>  	local i c=1 cmd cmd_pos __git_dir
>  
> -	if [[ -n ${ZSH_VERSION-} ]]; then
> -		emulate -L bash
> -		setopt KSH_TYPESET
> -
> -		# workaround zsh's bug that leaves 'words' as a special
> -		# variable in versions < 4.3.12
> -		typeset -h words
> -
> -		# workaround zsh's bug that quotes spaces in the COMPREPLY
> -		# array if IFS doesn't contain spaces.
> -		typeset -h IFS
> -	fi
> -
> -	local cur words cword prev
> -	_get_comp_words_by_ref -n =: cur words cword prev
>  	while [ $c -lt $cword ]; do
>  		i="${words[c]}"
>  		case "$i" in
> @@ -2683,22 +2668,6 @@ _git ()
>  
>  _gitk ()
>  {
> -	if [[ -n ${ZSH_VERSION-} ]]; then
> -		emulate -L bash
> -		setopt KSH_TYPESET
> -
> -		# workaround zsh's bug that leaves 'words' as a special
> -		# variable in versions < 4.3.12
> -		typeset -h words
> -
> -		# workaround zsh's bug that quotes spaces in the COMPREPLY
> -		# array if IFS doesn't contain spaces.
> -		typeset -h IFS
> -	fi
> -
> -	local cur words cword prev
> -	_get_comp_words_by_ref -n =: cur words cword prev
> -

Eliminating code duplication, great!

>  	__git_has_doubledash && return
>  
>  	local g="$(__gitdir)"
> @@ -2719,16 +2688,42 @@ _gitk ()
>  	__git_complete_revlist
>  }
>  
> -complete -o bashdefault -o default -o nospace -F _git git 2>/dev/null \
> -	|| complete -o default -o nospace -F _git git
> -complete -o bashdefault -o default -o nospace -F _gitk gitk 2>/dev/null \
> -	|| complete -o default -o nospace -F _gitk gitk

More greatness.

> +foo_wrap ()

This git-completion-specific internal helper function will pollute the
user's environment.  It should get a __git_ prefix.

> +{
> +	local cmd=foo_cmd cmd_pos=1
> +	if [[ -n ${ZSH_VERSION-} ]]; then
> +		emulate -L bash
> +		setopt KSH_TYPESET
> +
> +		# workaround zsh's bug that leaves 'words' as a special
> +		# variable in versions < 4.3.12
> +		typeset -h words
> +
> +		# workaround zsh's bug that quotes spaces in the COMPREPLY
> +		# array if IFS doesn't contain spaces.
> +		typeset -h IFS
> +	fi
> +	local cur words cword prev
> +	_get_comp_words_by_ref -n =: cur words cword prev
> +	foo "$@"
> +}
> +
> +git_complete ()

Maybe this should also get a prefix, but I'm not sure, because this
is, strictly speaking, not a bash completion function.

> +{
> +	local name="${2-$1}"
> +	local cmd="${name#git_}"
> +	eval "$(typeset -f foo_wrap | sed -e "s/foo_cmd/$cmd/" -e "s/foo/_$name/")"
> +	complete -o bashdefault -o default -o nospace -F _${name}_wrap $1 2>/dev/null \
> +		|| complete -o default -o nospace -F _${name}_wrap $1
> +}
> +
> +git_complete git
> +git_complete gitk

Clever ;)

But it needs a subshell and a sed invocation, so Windows people might
not be all too happy about this.  Perhaps it's not a big deal in the
default case, because it will be done only twice at startup time.
However, if someone wants several aliases on Windows, it can cause
noticeable delay.  It's a startup speed vs. convenience trade-off.
Anyway, I don't have any ideas how those fork()s and exec() could be
avoided.


Best,
Gábor

Re: [PATCH 07/12] completion: simplify command stuff

[SZEDER Gábor]

On Thu, Apr 12, 2012 at 01:21:17AM +0300, Felipe Contreras wrote:
> 2012/4/12 SZEDER Gábor <szeder@ira.uka.de>:
> > However, this change "promotes" the command (or cmd) variable from
> > being just a variable in _git() to a variable that is used in other
> > completion functions, too.  Why not go one more step further, and
> > denote this by adding a __git prefix (i.e. renaming the variable to
> > __git_command)?
> 
> After thinking more about it, and analyzing the next patches, I don't
> think it makes sense to have such a variable; it's only used by
> __git_complete_remote_or_refspec, and it would be easier to make it an
> argument to that function; that would simplify this patch series a
> lot.

You mean that _git_fetch() would call

  __git_complete_remote_or_refspec "fetch"

while _git_pull() and _git_push() will pass "pull" and "push",
respectively?


Best,
Gábor

Re: [PATCH 10/12] completion: add new git_complete helper

[Felipe Contreras]

2012/4/12 SZEDER Gábor <szeder@ira.uka.de>:

>> +{
>> +     local name="${2-$1}"
>> +     local cmd="${name#git_}"
>> +     eval "$(typeset -f foo_wrap | sed -e "s/foo_cmd/$cmd/" -e "s/foo/_$name/")"
>> +     complete -o bashdefault -o default -o nospace -F _${name}_wrap $1 2>/dev/null \
>> +             || complete -o default -o nospace -F _${name}_wrap $1
>> +}
>> +
>> +git_complete git
>> +git_complete gitk
>
> Clever ;)
>
> But it needs a subshell and a sed invocation, so Windows people might
> not be all too happy about this.  Perhaps it's not a big deal in the
> default case, because it will be done only twice at startup time.
> However, if someone wants several aliases on Windows, it can cause
> noticeable delay.  It's a startup speed vs. convenience trade-off.
> Anyway, I don't have any ideas how those fork()s and exec() could be
> avoided.

Well, if you look at the foo_wrap function you would notice there is
not much 'foo' in it. We could have a __git_wrap () function instead,
but we would need to find an alternative way to set the 'cmd'
variable. The easiest would be to just don't set it at all. As I
suggested, only __git_complete_remote_or_refspec needs it, and can be
easily worked around.

-- 
Felipe Contreras

Re: [PATCH 07/12] completion: simplify command stuff

[Felipe Contreras]

2012/4/12 SZEDER Gábor <szeder@ira.uka.de>:
> On Thu, Apr 12, 2012 at 01:21:17AM +0300, Felipe Contreras wrote:
>> 2012/4/12 SZEDER Gábor <szeder@ira.uka.de>:
>> > However, this change "promotes" the command (or cmd) variable from
>> > being just a variable in _git() to a variable that is used in other
>> > completion functions, too.  Why not go one more step further, and
>> > denote this by adding a __git prefix (i.e. renaming the variable to
>> > __git_command)?
>>
>> After thinking more about it, and analyzing the next patches, I don't
>> think it makes sense to have such a variable; it's only used by
>> __git_complete_remote_or_refspec, and it would be easier to make it an
>> argument to that function; that would simplify this patch series a
>> lot.
>
> You mean that _git_fetch() would call
>
>  __git_complete_remote_or_refspec "fetch"
>
> while _git_pull() and _git_push() will pass "pull" and "push",
> respectively?

Yeap. Then there would not be any need for the foo_wrap () stuff.

-- 
Felipe Contreras

Re: [PATCH 01/12] tests: add initial bash completion tests

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Sun, Apr 8, 2012 at 7:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> What shell do you use on your system as /bin/sh (or if you use SHELL_PATH
>> in the Makefile to override it, what do you use)?
>
> /bin/sh is a symlink to bash on my system, and although bash defines
> POSIXLY_CORRECT, the tests pass just fine. I even tried to run /bin/sh
> --posix.

It really depends on what's in contrib/git-completion.bash and what you
test.  

For example, if a (possibly future) version of completion script had lines
like these (taken from 6486ca6d7):

	__some_function ()
        {
		...
                while read key value; do
                        ...
                done < <(git config -z --get-regexp ...
		...
	}

invoking "bash --posix" would have failed like this:

        $ bash --posix
        bash-4.1$ _a ()
        > {
        >   while read x; do
        >     echo "* $x"
        >   done < <(echo a; echo b)
        bash: syntax error near unexpected token `<'

Perhaps the "initial tests" you posted, together with the version of the
completion script we happen to have near 'master', didn't have any
construct that gives a visible failure like the above, but that is just by
dumb luck and is not a reason for us to be complacent and not to be
careful.

The early part of your re-roll (i.e. v2) seems to be done carefully to
make sure we run the script in full-fledged bash, from a quick glance.

Thanks.

Re: [PATCH 07/12] completion: simplify command stuff

[Felipe Contreras]

2012/4/12 Felipe Contreras <felipe.contreras@gmail.com>:
> 2012/4/12 SZEDER Gábor <szeder@ira.uka.de>:
>> On Thu, Apr 12, 2012 at 01:21:17AM +0300, Felipe Contreras wrote:
>>> 2012/4/12 SZEDER Gábor <szeder@ira.uka.de>:
>>> > However, this change "promotes" the command (or cmd) variable from
>>> > being just a variable in _git() to a variable that is used in other
>>> > completion functions, too.  Why not go one more step further, and
>>> > denote this by adding a __git prefix (i.e. renaming the variable to
>>> > __git_command)?
>>>
>>> After thinking more about it, and analyzing the next patches, I don't
>>> think it makes sense to have such a variable; it's only used by
>>> __git_complete_remote_or_refspec, and it would be easier to make it an
>>> argument to that function; that would simplify this patch series a
>>> lot.
>>
>> You mean that _git_fetch() would call
>>
>>  __git_complete_remote_or_refspec "fetch"
>>
>> while _git_pull() and _git_push() will pass "pull" and "push",
>> respectively?
>
> Yeap. Then there would not be any need for the foo_wrap () stuff.

Actually, there would be, we need to call _git_fetch(), but if we have
generic wrap function we don't know that; all we have is the alias
(e.g. gf from 'git fetch'). I guess we could do something like 'eval
$(alias gf)' and find out the right function to call, but that already
has an 'eval' there, I'm not sure it's worth it.

Any ideas?

-- 
Felipe Contreras