Simple update to bash completions to prevent unbound variable errors.

Below is a cleaner patch that prevents problems with unbound arguments when bash is being operated under "set -u."

Some comments were added to remind callers about the requirements of each __git* utility function.

Additionally, the completion call to bash has been modernized a bit.

A vim modeline has also been added for consistency.

---
 contrib/completion/git-completion.bash |   70 
+++++++++++++++++++++++++-------
 1 files changed, 55 insertions(+), 15 deletions(-)

index 7b074d7..619e886 100755
@@ -50,9 +50,11 @@ case "$COMP_WORDBREAKS" in
 *)   COMP_WORDBREAKS="$COMP_WORDBREAKS:"
 esac
 
+# __gitdir accepts 0 or 1 arguments (i.e., location)
+# returns location of .git repo
 __gitdir ()
 {
-	if [ -z "$1" ]; then
+	if [ $# -eq 0 ] || [ -z "$1" ]; then
 		if [ -n "$__git_dir" ]; then
 			echo "$__git_dir"
 		elif [ -d .git ]; then

@@ -67,6 +69,8 @@ __gitdir ()
 	fi
 }
 
+# __git_ps1 accepts 0 or 1 arguments (i.e., format string)
+# returns text to add to bash PS1 prompt (includes branch name)
 __git_ps1 ()
 {
 	local g="$(git rev-parse --git-dir 2>/dev/null)"

@@ -111,7 +115,7 @@ __git_ps1 ()
 			fi
 		fi
 
-		if [ -n "$1" ]; then
+		if [ $# -gt 0 ] && [ -n "$1" ]; then
 			printf "$1" "${b##refs/heads/}$r"
 		else
 			printf " (%s)" "${b##refs/heads/}$r"

@@ -119,6 +123,7 @@ __git_ps1 ()
 	fi
 }
 
+# __gitcomp_1 requires 2 arguments
 __gitcomp_1 ()
 {
 	local c IFS=' '$'\t'$'\n'

@@ -131,11 +136,22 @@ __gitcomp_1 ()
 	done
 }
 
+# __gitcomp accepts 1, 2, 3, or 4 arguments
+# generates completion reply with compgen
 __gitcomp ()
 {
-	local cur="${COMP_WORDS[COMP_CWORD]}"
-	if [ $# -gt 2 ]; then
-		cur="$3"
+	local one two cur="${COMP_WORDS[COMP_CWORD]}" four
+	if [ $# -gt 0 ]; then
+		one="$1"
+		if [ $# -gt 1 ]; then
+			two="$2"
+			if [ $# -gt 2 ]; then
+				cur="$3"
+				if [ $# -gt 3 ]; then
+					four="$4"
+				fi
+			fi
+		fi
 	fi
 	case "$cur" in
 	--*=)

@@ -143,22 +159,27 @@ __gitcomp ()
 		;;
 	*)
 		local IFS=$'\n'
-		COMPREPLY=($(compgen -P "$2" \
-			-W "$(__gitcomp_1 "$1" "$4")" \
+		COMPREPLY=($(compgen -P "$two" \
+			-W "$(__gitcomp_1 "$one" "$four")" \
 			-- "$cur"))
 		;;
 	esac
 }
 
+# __git_heads accepts 0 or 1 arguments (to pass to __gitdir)
 __git_heads ()
 {
-	local cmd i is_hash=y dir="$(__gitdir "$1")"
+	local one
+	if [ $# -gt 0 ]; then
+		one="$1"
+	fi
+	local cmd i is_hash=y dir="$(__gitdir "$one")"
 	if [ -d "$dir" ]; then
 		git --git-dir="$dir" for-each-ref --format='%(refname:short)' \
 			refs/heads
 		return
 	fi
-	for i in $(git ls-remote "$1" 2>/dev/null); do
+	for i in $(git ls-remote "$one" 2>/dev/null); do
 		case "$is_hash,$i" in
 		y,*) is_hash=n ;;
 		n,*^{}) is_hash=y ;;

@@ -168,15 +189,20 @@ __git_heads ()
 	done
 }
 
+# __git_tags accepts 0 or 1 arguments (to pass to __gitdir)
 __git_tags ()
 {
-	local cmd i is_hash=y dir="$(__gitdir "$1")"
+	local one
+	if [ $# -gt 0 ]; then
+		one="$1"
+	fi
+	local cmd i is_hash=y dir="$(__gitdir "$one")"
 	if [ -d "$dir" ]; then
 		git --git-dir="$dir" for-each-ref --format='%(refname:short)' \
 			refs/tags
 		return
 	fi
-	for i in $(git ls-remote "$1" 2>/dev/null); do
+	for i in $(git ls-remote "$one" 2>/dev/null); do
 		case "$is_hash,$i" in
 		y,*) is_hash=n ;;
 		n,*^{}) is_hash=y ;;

@@ -186,9 +212,14 @@ __git_tags ()
 	done
 }
 
+# __git_refs accepts 0 or 1 arguments (to pass to __gitdir)
 __git_refs ()
 {
-	local i is_hash=y dir="$(__gitdir "$1")"
+	local one
+	if [ $# -gt 0 ]; then
+		one="$1"
+	fi
+	local i is_hash=y dir="$(__gitdir "$one")"
 	local cur="${COMP_WORDS[COMP_CWORD]}" format refs
 	if [ -d "$dir" ]; then
 		case "$cur" in

@@ -218,6 +249,7 @@ __git_refs ()
 	done
 }
 
+# __git_refs2 requires 1 argument (to pass to __git_refs)
 __git_refs2 ()
 {
 	local i

@@ -226,6 +258,7 @@ __git_refs2 ()
 	done
 }
 
+# __git_refs_remotes requires 1 argument (to pass to ls-remote)
 __git_refs_remotes ()
 {
 	local cmd i is_hash=y

@@ -470,6 +503,7 @@ __git_aliases ()
 	done
 }
 
+# __git_aliased_command requires 1 argument
 __git_aliased_command ()
 {
 	local word cmdline=$(git --git-dir="$(__gitdir)" \

@@ -482,6 +516,7 @@ __git_aliased_command ()
 	done
 }
 
+# __git_find_subcommand requires 1 argument
 __git_find_subcommand ()
 {
 	local word subcommand c=1

@@ -1766,13 +1801,18 @@ _gitk ()
 	__git_complete_revlist
 }
 
-complete -o default -o nospace -F _git git
-complete -o default -o nospace -F _gitk gitk
+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
 
 # 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 default -o nospace -F _git git.exe
+complete -o bashdefault -o default -o nospace -F _git git.exe 2>/dev/null \
+	|| complete -o default -o nospace -F _git git.exe
 fi
+
+# vim:ft=sh:fdm=marker:ff=unix:noet:ts=4:sw=4

1.6.1.87.g15624

Yuck.

Double yuck.

	local one=${1-} two=${2-} cur=${3-} four=${4-}

While I dislike emacs and use vim pretty much exclusively, I don't really see the need for a vim modeline. On top of that, fdm=marker is a bit silly since there aren't any markers. ff=unix and ft=sh are redundant, as any vim should detect these properly, given the filename.

So, I'm slightly negative on the modeline hunk.

I found "${1-}" ugly, and this a bit better, but I'll defer to Junio.

Definitely agreeing with Junio here. This is far less ascetic than the old patch. Truth be told, this whole thread would probably have been more productive without me chiming in. Sorry about that.

Even better to use variable names that match the usage, if possible.

Boyd Stephen Smith Jr.                     ,= ,-_-. =. 
[email protected]                     ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy           `-'(. .)`-' 
http://iguanasuicide.net/                      \_/     

Without the modeline, the vim I was using didn't set ft=sh. It seemed like the original authors were careful to use tabs everywhere, which was the major reason I used the modeline. I added the fdm=marker just in case someone would want to clean up the code someday and do some folding.

It was just a suggestion.

I don't know much about bash completion. It would be nice if the original authors could add some semantics, as I was just trying to come up with scripts that would actually work with set -u.

--Ted

Ted Pavlic <[email protected]>

  Please visit my ALS association page:
        http://web.alsa.org/goto/tedpavlic
  My family appreciates your support in the fight to defeat ALS.

NOTE: On my system, I save git-completion.bash to .git_bash_completion. Because of that, Vim can't ftdetect off of the file name. The modeline allows ft=sh even when you don't end in .sh or .bash.

An alternative (to a Vim modeline) is to put

#!bash

at the top of the script. That would do the same thing as the modeline (even though it would never actually get used by the sourced "script").

--Ted

Ted Pavlic <[email protected]>

  Please visit my ALS association page:
        http://web.alsa.org/goto/tedpavlic
  My family appreciates your support in the fight to defeat ALS.

I actually like this much better than "# vim:" thing.

It talks about WHAT the file is ("It is a bash scriptlet"), as opposed to "# vim:" that tells HOW one user wants to edit the file ("with this settings I use vim to edit this file"). In case somebody misunderstood me, my "Yuck" was not about Emacs vs vi holy war. [*1*].

As people noted, it may not have any meaning to the shell, but it serves as an important documentation to humans. As the maintainer, I try to be reasonably strict about keeping our shell scripts free of bash-ism to help people on platforms without bash, but this file is about bash (and bash only). We are free to use all bash-isms we usually try to stay away from in our scripts marked with #!/bin/sh (namely, "local", "let", shell arrays, and substitutions outside POSIX such as ${parameter:offset:length} substring, ${#parameter} length, and ${parameter/pattern/string} regexp replacement), as long as they are compatible across recent bash versions.

If tools like vim notice the same hint that says "this is bash" and adjusts its behaviour accordingly, that is great.

Even though I've grown to know Shawn well enough to be able to tell that certain kinds of changes would get his Ack and often apply patches myself without waiting him to give his Ack, the completion script is ultimately his code and you do not necessarily have to make me like it.

Your updated patch still had [ $# -eq 0 -o -z "$1" ] even though the commit log message (which is not the place to describe changes between v1 and v2 submissions, by the way) claimed that you replaced them with "${1-}". I am guessing it would need at least one more iteration to clean up, but I think this patch is improving _provided if_ supporting "set -u" in the user's interactive environment is a good idea to begin with.

I just hope that next person who complains about bash completion scripts would not say that he has "set -e" in his interactive environment ;-) I personally consider that "set -u" is equally silly, but it probably is just me.

[Footnote]

*1* For the same reason, Local Variables section to please Emacs is equally offending; it tends to be much bigger which is even worse.

Another way is to set filetype detection locally. This way the project files don't get filled with editor-specific stuff. You may want add something like the following to your ~/.vim/filetype.vim file:

augroup filetypedetect
autocmd BufNewFile,BufRead .git_bash_completion setl ft=sh
augroup END

Better that than have a mixture of spaces and tabs.

In that case, there's only $1 (the format string).

Note that in most cases, I didn't know what $1 and $2 were. I was just trying to fix it so that it would work on my system.

Hey -- I agree, but no one else liked ${1-}. And hg's bash completion seems far superior because they avoid ever having to worry about it. They actually thought about the arguments ahead of time.

No clue. Patches are welcome.

Why even use one, two three, and four then?

I had ${#-} throughout, but I was told that that was ugly. So the best I could do was come up with the above mess.

--Ted

Ted Pavlic <[email protected]>

  Please visit my ALS association page:
        http://web.alsa.org/goto/tedpavlic
  My family appreciates your support in the fight to defeat ALS.

To be absolutely honest, I think people with "set -u" in their interactive environment are sick. Bourne shells have been substituting unset variables to empty string for eons, and this is not _my_ itch to scratch.

It seems to be yours, though, and I was merely trying to help.