[PATCH] bash-completion: Support running with set -u in bash 4.0

[PATCH] bash-completion: Support running with set -u in bash 4.0

[Jonathan Nieder]

From: Thomas Nilsson <thomas.nilsson@unixangst.com>
Date: Tue, 23 Feb 2010 12:13:00 +0100

Starting with bash 4.0-beta, under "set -u" semantics, accessing
undefined local variables is now an error.  Some user environments
enable this setting in the interactive shell, with unpleasant results:

 knirch@traktor:~/source/external/git (master)$ set -o unset
 bash: w: unbound variable
 knirch@traktor:~/source/external/git$ git ^Ibash: command: unbound variable
 bash: w: unbound variable
 knirch@traktor:~/source/external/git$
 bash: w: unbound variable
 knirch@traktor:~/source/external/git$

Most of these variables should be bound to "".  In contexts where the
completion functions should access an undefined variable, accessing a
default empty string (as in "${1-}" instead of "$1") is a reasonable
way to cope, as it silences the undefined variable error while still
supplying an empty string.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Sent by Thomas to <http://bugs.debian.org/571087> (thanks!).  One
might use 'set -u' in an interactive shell while manually
single-stepping through a shell script, perhaps.

 contrib/completion/git-completion.bash |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index fe93747..d97467e 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -84,8 +84,8 @@ __git_ps1 ()
 {
 	local g="$(__gitdir)"
 	if [ -n "$g" ]; then
-		local r
-		local b
+		local r=""
+		local b=""
 		if [ -f "$g/rebase-merge/interactive" ]; then
 			r="|REBASE-i"
 			b="$(cat "$g/rebase-merge/head-name")"
@@ -127,11 +127,11 @@ __git_ps1 ()
 			}
 		fi
 
-		local w
-		local i
-		local s
-		local u
-		local c
+		local w=""
+		local i=""
+		local s=""
+		local u=""
+		local c=""
 
 		if [ "true" = "$(git rev-parse --is-inside-git-dir 2>/dev/null)" ]; then
 			if [ "true" = "$(git rev-parse --is-bare-repository 2>/dev/null)" ]; then
@@ -2181,7 +2181,7 @@ _git ()
 		c=$((++c))
 	done
 
-	if [ -z "$command" ]; then
+	if [ -z "${command-}" ]; then
 		case "${COMP_WORDS[COMP_CWORD]}" in
 		--*)   __gitcomp "
 			--paginate
-- 
1.7.0

Re: [PATCH] bash-completion: Support running with set -u in bash 4.0

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> From: Thomas Nilsson <thomas.nilsson@unixangst.com>
> Date: Tue, 23 Feb 2010 12:13:00 +0100
>
> Starting with bash 4.0-beta, under "set -u" semantics, accessing
> undefined local variables is now an error.  Some user environments
> enable this setting in the interactive shell, with unpleasant results:

Unfortunate but necessary, it seems.

> @@ -2181,7 +2181,7 @@ _git ()
>  		c=$((++c))
>  	done
>  
> -	if [ -z "$command" ]; then
> +	if [ -z "${command-}" ]; then
>  		case "${COMP_WORDS[COMP_CWORD]}" in
>  		--*)   __gitcomp "
>  			--paginate

Why not this patch, instead of the above hunk?

Also if we initialize __git_dir with an empty string on the same line, I
think we can lose "${__git_dir-}" ugliness in __gitdir () function.

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index fe93747..3a65156 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2167,7 +2167,7 @@ _git_tag ()
 
 _git ()
 {
-	local i c=1 command __git_dir
+	local i c=1 command="" __git_dir
 
 	while [ $c -lt $COMP_CWORD ]; do
 		i="${COMP_WORDS[c]}"

Re: [PATCH] bash-completion: Support running with set -u in bash 4.0

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> @@ -2181,7 +2181,7 @@ _git ()
>>  		c=$((++c))
>>  	done
>>  
>> -	if [ -z "$command" ]; then
>> +	if [ -z "${command-}" ]; then
>>  		case "${COMP_WORDS[COMP_CWORD]}" in
>>  		--*)   __gitcomp "
>>  			--paginate
>
> Why not this patch, instead of the above hunk?

My mistake.  The original patch was as you suggest, then I changed it
to the above to make it follow the style of surrounding code.

If one makes it a goal to lose the ${foo-} lines:

 - In __gitdir, [ -z "${1-}" ] should be [ $# -eq 0 ]
 - In __gitdir callers, "${1-}" should be replaced by "$@"
 - In _git, __git_dir should be initialized to ""
 - In _gitk and __git_ps1, __git_dir should be made local and
   initialized (independent bugfix)
 - In __git_ps1, the code examining $GIT_PS1_DESCRIBE_STYLE
   should be protected by an if [ -n "${GIT_PS1_DESCRIBE_STYLE:+set}" ]
 - The checks for GIT_PS1_SHOWDIRTYSTATE and GIT_PS1_SHOWUNTRACKEDFILES
   would also be adjusted
 - __gitcomp should get some local variables set to "" and then
   conditionally set to its arguments
 - "${1-}" in __git_heads and __git_tags should be "$dir"

These look like good things to do anyway.  Assuming your change
has already been made, the follow-up patch would look something
like this.

-- %< --
Subject: bash-completion: Avoid using uninitialized values

The "${var-}" idiom is obscure and makes it easy to hide uses of
values not initialized by the completion script that could have leaked
from the user’s environment.

Untested.  The only intentional change in functionality from this
patch is that __git_dir has been made local to _gitk() and __git_ps1.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 contrib/completion/git-completion.bash |   52 +++++++++++++++++++++-----------
 1 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 2682f52..e291d8d 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -63,8 +63,8 @@ esac
 # returns location of .git repo
 __gitdir ()
 {
-	if [ -z "${1-}" ]; then
-		if [ -n "${__git_dir-}" ]; then
+	if [ $# -eq 0 ]; then
+		if [ -n "$__git_dir" ]; then
 			echo "$__git_dir"
 		elif [ -d .git ]; then
 			echo .git
@@ -82,6 +82,7 @@ __gitdir ()
 # returns text to add to bash PS1 prompt (includes branch name)
 __git_ps1 ()
 {
+	local __git_dir=""
 	local g="$(__gitdir)"
 	if [ -n "$g" ]; then
 		local r=""
@@ -108,18 +109,20 @@ __git_ps1 ()
 			fi
 
 			b="$(git symbolic-ref HEAD 2>/dev/null)" || {
-
-				b="$(
-				case "${GIT_PS1_DESCRIBE_STYLE-}" in
+				if [ -z "${GIT_PS1_DESCRIBE_STYLE:+set}" ] ||
+					[ "$GIT_PS1_DESCRIBE_STYLE" = default ]; then
+					b=$(git describe --exact-match HEAD 2>/dev/null)
+				else b=$(case "$GIT_PS1_DESCRIBE_STYLE" in
 				(contains)
 					git describe --contains HEAD ;;
 				(branch)
 					git describe --contains --all HEAD ;;
 				(describe)
 					git describe HEAD ;;
-				(* | default)
+				(*)
 					git describe --exact-match HEAD ;;
-				esac 2>/dev/null)" ||
+				esac 2>/dev/null)
+				fi ||
 
 				b="$(cut -c1-7 "$g/HEAD" 2>/dev/null)..." ||
 				b="unknown"
@@ -140,7 +143,7 @@ __git_ps1 ()
 				b="GIT_DIR!"
 			fi
 		elif [ "true" = "$(git rev-parse --is-inside-work-tree 2>/dev/null)" ]; then
-			if [ -n "${GIT_PS1_SHOWDIRTYSTATE-}" ]; then
+			if [ -n "${GIT_PS1_SHOWDIRTYSTATE:+set}" ]; then
 				if [ "$(git config --bool bash.showDirtyState)" != "false" ]; then
 					git diff --no-ext-diff --quiet --exit-code || w="*"
 					if git rev-parse --quiet --verify HEAD >/dev/null; then
@@ -150,11 +153,11 @@ __git_ps1 ()
 					fi
 				fi
 			fi
-			if [ -n "${GIT_PS1_SHOWSTASHSTATE-}" ]; then
+			if [ -n "${GIT_PS1_SHOWSTASHSTATE:+set}" ]; then
 			        git rev-parse --verify refs/stash >/dev/null 2>&1 && s="$"
 			fi
 
-			if [ -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" ]; then
+			if [ -n "${GIT_PS1_SHOWUNTRACKEDFILES:+set}" ]; then
 			   if [ -n "$(git ls-files --others --exclude-standard)" ]; then
 			      u="%"
 			   fi
@@ -183,18 +186,30 @@ __gitcomp_1 ()
 # generates completion reply with compgen
 __gitcomp ()
 {
+	local replies=""
+	local pfx=""
 	local cur="${COMP_WORDS[COMP_CWORD]}"
+	local sfx=""
+	if [ $# -gt 3 ]; then
+		sfx=$4
+	fi
 	if [ $# -gt 2 ]; then
 		cur="$3"
 	fi
+	if [ $# -gt 1 ]; then
+		pfx=$2
+	fi
+	if [ $# -gt 0 ]; then
+		replies=$1
+	fi
 	case "$cur" in
 	--*=)
 		COMPREPLY=()
 		;;
 	*)
 		local IFS=$'\n'
-		COMPREPLY=($(compgen -P "${2-}" \
-			-W "$(__gitcomp_1 "${1-}" "${4-}")" \
+		COMPREPLY=($(compgen -P "$pfx" \
+			-W "$(__gitcomp_1 "$replies" "$sfx")" \
 			-- "$cur"))
 		;;
 	esac
@@ -203,13 +218,13 @@ __gitcomp ()
 # __git_heads accepts 0 or 1 arguments (to pass to __gitdir)
 __git_heads ()
 {
-	local cmd i is_hash=y dir="$(__gitdir "${1-}")"
+	local cmd i is_hash=y dir="$(__gitdir "$@")"
 	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 "$dir" 2>/dev/null); do
 		case "$is_hash,$i" in
 		y,*) is_hash=n ;;
 		n,*^{}) is_hash=y ;;
@@ -222,13 +237,13 @@ __git_heads ()
 # __git_tags accepts 0 or 1 arguments (to pass to __gitdir)
 __git_tags ()
 {
-	local cmd i is_hash=y dir="$(__gitdir "${1-}")"
+	local cmd i is_hash=y dir="$(__gitdir "$@")"
 	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 "$dir" 2>/dev/null); do
 		case "$is_hash,$i" in
 		y,*) is_hash=n ;;
 		n,*^{}) is_hash=y ;;
@@ -241,7 +256,7 @@ __git_tags ()
 # __git_refs accepts 0 or 1 arguments (to pass to __gitdir)
 __git_refs ()
 {
-	local i is_hash=y dir="$(__gitdir "${1-}")"
+	local i is_hash=y dir="$(__gitdir "$@")"
 	local cur="${COMP_WORDS[COMP_CWORD]}" format refs
 	if [ -d "$dir" ]; then
 		case "$cur" in
@@ -2167,7 +2182,7 @@ _git_tag ()
 
 _git ()
 {
-	local i c=1 command="" __git_dir
+	local i c=1 command="" __git_dir=""
 
 	while [ $c -lt $COMP_CWORD ]; do
 		i="${COMP_WORDS[c]}"
@@ -2265,6 +2280,7 @@ _gitk ()
 {
 	__git_has_doubledash && return
 
+	local __git_dir=""
 	local cur="${COMP_WORDS[COMP_CWORD]}"
 	local g="$(__gitdir)"
 	local merge=""
-- 
1.7.0