[PATCH] Update bash completions to prevent unbound variable errors.

[PATCH] Update bash completions to prevent unbound variable errors.

[Ted Pavlic]

In bash, "set -u" gives an error when a variable is unbound. In this
case, the bash completion script included in the git/contrib directory
produces several errors.

The attached patch replaces things like

         if [ -z "$1" ]

with

         if [ -z "${1-}" ]

so that the unbound variable returns an empty value. Hence, the
completion script will now work even "set -u" set.

Signed-off-by: Ted Pavlic <ted@tedpavlic.com>
---
  contrib/completion/git-completion.bash |   68 
++++++++++++++++----------------
  1 files changed, 34 insertions(+), 34 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 7b074d7..50e345f 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -52,25 +52,25 @@ esac

  __gitdir ()
  {
-	if [ -z "$1" ]; then
-		if [ -n "$__git_dir" ]; then
+	if [ -z "${1-}" ]; then
+		if [ -n "${__git_dir-}" ]; then
  			echo "$__git_dir"
  		elif [ -d .git ]; then
  			echo .git
  		else
  			git rev-parse --git-dir 2>/dev/null
  		fi
-	elif [ -d "$1/.git" ]; then
-		echo "$1/.git"
+	elif [ -d "${1-}/.git" ]; then
+		echo "${1-}/.git"
  	else
-		echo "$1"
+		echo "${1-}"
  	fi
  }

  __git_ps1 ()
  {
  	local g="$(git rev-parse --git-dir 2>/dev/null)"
-	if [ -n "$g" ]; then
+	if [ -n "${g-}" ]; then
  		local r
  		local b
  		if [ -d "$g/rebase-apply" ]
@@ -111,8 +111,8 @@ __git_ps1 ()
  			fi
  		fi

-		if [ -n "$1" ]; then
-			printf "$1" "${b##refs/heads/}$r"
+		if [ -n "${1-}" ]; then
+			printf "${1-}" "${b##refs/heads/}$r"
  		else
  			printf " (%s)" "${b##refs/heads/}$r"
  		fi
@@ -122,11 +122,11 @@ __git_ps1 ()
  __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 " ;;
+	for c in ${1-}; do
+		case "$c${2-}" in
+		--*=*) printf %s$'\n' "$c${2-}" ;;
+		*.)    printf %s$'\n' "$c${2-}" ;;
+		*)     printf %s$'\n' "$c${2-} " ;;
  		esac
  	done
  }
@@ -135,7 +135,7 @@ __gitcomp ()
  {
  	local cur="${COMP_WORDS[COMP_CWORD]}"
  	if [ $# -gt 2 ]; then
-		cur="$3"
+		cur="${3-}"
  	fi
  	case "$cur" in
  	--*=)
@@ -143,8 +143,8 @@ __gitcomp ()
  		;;
  	*)
  		local IFS=$'\n'
-		COMPREPLY=($(compgen -P "$2" \
-			-W "$(__gitcomp_1 "$1" "$4")" \
+		COMPREPLY=($(compgen -P "${2-}" \
+			-W "$(__gitcomp_1 "${1-}" "${4-}")" \
  			-- "$cur"))
  		;;
  	esac
@@ -152,13 +152,13 @@ __gitcomp ()

  __git_heads ()
  {
-	local cmd i is_hash=y dir="$(__gitdir "$1")"
+	local cmd i is_hash=y dir="$(__gitdir "${1-}")"
  	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 "${1-}" 2>/dev/null); do
  		case "$is_hash,$i" in
  		y,*) is_hash=n ;;
  		n,*^{}) is_hash=y ;;
@@ -170,13 +170,13 @@ __git_heads ()

  __git_tags ()
  {
-	local cmd i is_hash=y dir="$(__gitdir "$1")"
+	local cmd i is_hash=y dir="$(__gitdir "${1-}")"
  	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 "${1-}" 2>/dev/null); do
  		case "$is_hash,$i" in
  		y,*) is_hash=n ;;
  		n,*^{}) is_hash=y ;;
@@ -188,7 +188,7 @@ __git_tags ()

  __git_refs ()
  {
-	local i is_hash=y dir="$(__gitdir "$1")"
+	local i is_hash=y dir="$(__gitdir "${1-}")"
  	local cur="${COMP_WORDS[COMP_CWORD]}" format refs
  	if [ -d "$dir" ]; then
  		case "$cur" in
@@ -221,7 +221,7 @@ __git_refs ()
  __git_refs2 ()
  {
  	local i
-	for i in $(__git_refs "$1"); do
+	for i in $(__git_refs "${1-}"); do
  		echo "$i:$i"
  	done
  }
@@ -229,11 +229,11 @@ __git_refs2 ()
  __git_refs_remotes ()
  {
  	local cmd i is_hash=y
-	for i in $(git ls-remote "$1" 2>/dev/null); do
+	for i in $(git ls-remote "${1-}" 2>/dev/null); do
  		case "$is_hash,$i" in
  		n,refs/heads/*)
  			is_hash=y
-			echo "$i:refs/remotes/$1/${i#refs/heads/}"
+			echo "$i:refs/remotes/${1-}/${i#refs/heads/}"
  			;;
  		y,*) is_hash=n ;;
  		n,*^{}) is_hash=y ;;
@@ -264,7 +264,7 @@ __git_remotes ()

  __git_merge_strategies ()
  {
-	if [ -n "$__git_merge_strategylist" ]; then
+	if [ -n "${__git_merge_strategylist-}" ]; then
  		echo "$__git_merge_strategylist"
  		return
  	fi
@@ -350,7 +350,7 @@ __git_complete_revlist ()

  __git_all_commands ()
  {
-	if [ -n "$__git_all_commandlist" ]; then
+	if [ -n "${__git_all_commandlist-}" ]; then
  		echo "$__git_all_commandlist"
  		return
  	fi
@@ -368,7 +368,7 @@ __git_all_commandlist="$(__git_all_commands 
2>/dev/null)"

  __git_porcelain_commands ()
  {
-	if [ -n "$__git_porcelain_commandlist" ]; then
+	if [ -n "${__git_porcelain_commandlist-}" ]; then
  		echo "$__git_porcelain_commandlist"
  		return
  	fi
@@ -473,7 +473,7 @@ __git_aliases ()
  __git_aliased_command ()
  {
  	local word cmdline=$(git --git-dir="$(__gitdir)" \
-		config --get "alias.$1")
+		config --get "alias.${1-}")
  	for word in $cmdline; do
  		if [ "${word##-*}" ]; then
  			echo $word
@@ -488,7 +488,7 @@ __git_find_subcommand ()

  	while [ $c -lt $COMP_CWORD ]; do
  		word="${COMP_WORDS[c]}"
-		for subcommand in $1; do
+		for subcommand in ${1-}; do
  			if [ "$subcommand" = "$word" ]; then
  				echo "$subcommand"
  				return
@@ -599,7 +599,7 @@ _git_bisect ()

  	local subcommands="start bad good skip reset visualize replay log run"
  	local subcommand="$(__git_find_subcommand "$subcommands")"
-	if [ -z "$subcommand" ]; then
+	if [ -z "${subcommand-}" ]; then
  		__gitcomp "$subcommands"
  		return
  	fi
@@ -1371,7 +1371,7 @@ _git_remote ()
  {
  	local subcommands="add rm show prune update"
  	local subcommand="$(__git_find_subcommand "$subcommands")"
-	if [ -z "$subcommand" ]; then
+	if [ -z "${subcommand-}" ]; then
  		__gitcomp "$subcommands"
  		return
  	fi
@@ -1500,7 +1500,7 @@ _git_stash ()
  {
  	local subcommands='save list show apply clear drop pop create branch'
  	local subcommand="$(__git_find_subcommand "$subcommands")"
-	if [ -z "$subcommand" ]; then
+	if [ -z "${subcommand-}" ]; then
  		__gitcomp "$subcommands"
  	else
  		local cur="${COMP_WORDS[COMP_CWORD]}"
@@ -1552,7 +1552,7 @@ _git_svn ()
  		proplist show-ignore show-externals
  		"
  	local subcommand="$(__git_find_subcommand "$subcommands")"
-	if [ -z "$subcommand" ]; then
+	if [ -z "${subcommand-}" ]; then
  		__gitcomp "$subcommands"
  	else
  		local remote_opts="--username= --config-dir= --no-auth-cache"
@@ -1672,7 +1672,7 @@ _git ()
  		c=$((++c))
  	done

-	if [ -z "$command" ]; then
+	if [ -z "${command-}" ]; then
  		case "${COMP_WORDS[COMP_CWORD]}" in
  		--*=*) COMPREPLY=() ;;
  		--*)   __gitcomp "
-- 
1.6.1.87.g15624

Re: [PATCH] Update bash completions to prevent unbound variable errors.

[Boyd Stephen Smith Jr.]

[-- Attachment #1: Type: text/plain, Size: 660 bytes --]

On Monday 2009 January 12 13:58:28 you wrote:
>In bash, "set -u" gives an error when a variable is unbound. In this
>case, the bash completion script included in the git/contrib directory
>produces several errors.
>
>The attached patch replaces things like
>
>         if [ -z "$1" ]
>
>with
>
>         if [ -z "${1-}" ]

That looks ugly to me.  Any reason we shouldn't just "set +u" at the top of 
the script?
-- 
Boyd Stephen Smith Jr.                     ,= ,-_-. =. 
bss@iguanasuicide.net                     ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy           `-'(. .)`-' 
http://iguanasuicide.net/                      \_/     

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Update bash completions to prevent unbound variable errors.

[Adeodato Simó]

* Boyd Stephen Smith Jr. [Mon, 12 Jan 2009 14:35:35 -0600]:

> >The attached patch replaces things like

> >         if [ -z "$1" ]

> >with

> >         if [ -z "${1-}" ]

> That looks ugly to me.  Any reason we shouldn't just "set +u" at the top of 
> the script?

`set +u` affects the shell globally, not just to the sourced file. If
you do that, you must be aware that you'll be preventing people from
running their shell in `set -u` mode. (Merely stating a fact here, not
giving any opinion.)

-- 
Adeodato Simó                                     dato at net.com.org.es
Debian Developer                                  adeodato at debian.org
 
The problem I have with making an intelligent statement is that some
people then think that it's not an isolated occurrance.
                -- Simon Travaglia

Re: [PATCH] Update bash completions to prevent unbound variable errors.

[Boyd Stephen Smith Jr.]

[-- Attachment #1: Type: text/plain, Size: 1245 bytes --]

On Monday 2009 January 12 14:40:30 Adeodato Simó wrote:
>* Boyd Stephen Smith Jr. [Mon, 12 Jan 2009 14:35:35 -0600]:
>> >The attached patch replaces things like
>> >
>> >         if [ -z "$1" ]
>> >
>> >with
>> >
>> >         if [ -z "${1-}" ]
>>
>> That looks ugly to me.  Any reason we shouldn't just "set +u" at the top
>> of the script?
>
>`set +u` affects the shell globally, not just to the sourced file. If
>you do that, you must be aware that you'll be preventing people from
>running their shell in `set -u` mode. (Merely stating a fact here, not
>giving any opinion.)

I'm not familiar with bash completion exception as a user, I didn't realize 
all these functions had to be sourced into the current shell.

Well, if the user want to run in "set -u" mode preventing it is bogus, IMO.  
We could use subshells and unset at the top of _git and _gitk functions, that 
would be only a +6/-4 patch.  It would also not be something future 
contributors have to think (much) about.
-- 
Boyd Stephen Smith Jr.                     ,= ,-_-. =. 
bss@iguanasuicide.net                     ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy           `-'(. .)`-' 
http://iguanasuicide.net/                      \_/     

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Update bash completions to prevent unbound variable errors.

[Shawn O. Pearce]

"Boyd Stephen Smith Jr." <bss@iguanasuicide.net> wrote:
> 
> Well, if the user want to run in "set -u" mode preventing it is bogus, IMO.  
> We could use subshells and unset at the top of _git and _gitk functions, that 
> would be only a +6/-4 patch.  It would also not be something future 
> contributors have to think (much) about.

Running in subshells is a bad idea.  We'd likely lose access
to the completion word array, and it would take a lot longer per
completion because you need to spin-up and tear-down that subshell.

We have spent some time to make the current completion code use as
few forks as possible to get the results we want, because it runs
faster that way.

-- 
Shawn.

Re: [PATCH] Update bash completions to prevent unbound variable errors.

[Boyd Stephen Smith Jr.]

[-- Attachment #1: Type: text/plain, Size: 1114 bytes --]

On Monday 2009 January 12 15:31:49 Shawn O. Pearce wrote:
>"Boyd Stephen Smith Jr." <bss@iguanasuicide.net> wrote:
>> Well, if the user want to run in "set -u" mode preventing it is bogus,
>> IMO. We could use subshells and unset at the top of _git and _gitk
>> functions, that would be only a +6/-4 patch.  It would also not be
>> something future contributors have to think (much) about.
>
>Running in subshells is a bad idea.

Yeah, not only for all the reasons you mention, but because it would require 
refactoring to use -C instead of -F (so we a longer and uglier patch); our 
changes to COMPREPLY in the subshell wouldn't be seen by bash.

Having tripped over my lack of experience twice in two messages in this 
thread, I'm going to bow out of the rest of it.  My ascetic opinion still 
stands, but I'll take working code, warts and all, over broken code.
-- 
Boyd Stephen Smith Jr.                     ,= ,-_-. =. 
bss@iguanasuicide.net                     ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy           `-'(. .)`-' 
http://iguanasuicide.net/                      \_/     

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Update bash completions to prevent unbound variable errors.

[Ted Pavlic]

>>          if [ -z "${1-}" ]
>
> That looks ugly to me.  Any reason we shouldn't just "set +u" at the top of
> the script?

As already discussed, because the script must be sourced, then the "set 
+u" has global scope.

I suppose that the option could be tested and then reset as appropriate 
at the end of the script.

(note: for some reason Mercurial's bash completion script does not have 
this problem; they use $1 directly without bash complaining)

-- 
Ted Pavlic <ted@tedpavlic.com>

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

Re: [PATCH] Update bash completions to prevent unbound variable errors.

[Ted Pavlic]

> (note: for some reason Mercurial's bash completion script does not have
> this problem; they use $1 directly without bash complaining)

It appears like they use

	complete -o bashdefault

whereas Git's uses

	complete -o default

I think that's the difference.


-- 
Ted Pavlic <ted@tedpavlic.com>

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

Re: [PATCH] Update bash completions to prevent unbound variable errors.

[Shawn O. Pearce]

Ted Pavlic <ted@tedpavlic.com> wrote:
>> (note: for some reason Mercurial's bash completion script does not have
>> this problem; they use $1 directly without bash complaining)
>
> It appears like they use
>
> 	complete -o bashdefault
>
> whereas Git's uses
>
> 	complete -o default
>
> I think that's the difference.

If that's all we need to do, that's a simple 1 line change... which
I like.

-- 
Shawn.

Re: [PATCH] Update bash completions to prevent unbound variable errors.

[Ted Pavlic]

>> It appears like they use
>>
>> 	complete -o bashdefault
>>
>> whereas Git's uses
>>
>> 	complete -o default
>>
>> I think that's the difference.
>
> If that's all we need to do, that's a simple 1 line change... which
> I like.

That was a red herring. The problem is more fundamental than that.

The script needs to be more careful about its use. I'll submit a better 
patch momentarily.

--Ted


-- 
Ted Pavlic <ted@tedpavlic.com>

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

Re: [PATCH] Update bash completions to prevent unbound variable errors.

[Adeodato Simó]

* Ted Pavlic [Mon, 12 Jan 2009 16:11:32 -0500]:

>> That looks ugly to me.  Any reason we shouldn't just "set +u" at the top of
>> the script?

> As already discussed, because the script must be sourced, then the "set  
> +u" has global scope.

> I suppose that the option could be tested and then reset as appropriate  
> at the end of the script.

That does not help, because appart from being global, it of course takes
effect at run time. In other words, it doesn't matter if set -u is
active or not at function definition time, but at function invoation
time.

> (note: for some reason Mercurial's bash completion script does not have  
> this problem; they use $1 directly without bash complaining)

Because (from a quick look) their completion script never expands a
variable which is not known to be set.


-- 
Adeodato Simó                                     dato at net.com.org.es
Debian Developer                                  adeodato at debian.org
 
A hacker does for love what other would not do for money.

Re: [PATCH] Update bash completions to prevent unbound variable errors.

[Ted Pavlic]

> Because (from a quick look) their completion script never expands a
> variable which is not known to be set.

They use $1, $2, etc. In fact, they use $1, $2, and $3 in their _hg, 
which is their main completion function. Why would those be defined there?

In fact, it's $1, $2, $3, and $4 that are causing the problemw ith the 
git completions.

--Ted

-- 
Ted Pavlic <ted@tedpavlic.com>

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

Re: [PATCH] Update bash completions to prevent unbound variable errors.

[Adeodato Simó]

* Ted Pavlic [Mon, 12 Jan 2009 16:37:20 -0500]:

>> Because (from a quick look) their completion script never expands a
>> variable which is not known to be set.

> They use $1, $2, etc. In fact, they use $1, $2, and $3 in their _hg,  
> which is their main completion function. Why would those be defined 
> there?

From http://www.gnu.org/software/bash/manual/bashref.html#Programmable-Completion:

  When the function or command is invoked, the first argument is the name
  of the command whose arguments are being completed, the second argument
  is the word being completed, and the third argument is the word
  preceding the word being completed on the current command line.

> In fact, it's $1, $2, $3, and $4 that are causing the problemw ith the  
> git completions.

They are causing problems in the functions that are called sometimes
with arguments, sometimes without, like __gitdir. If you know that
you'll always be calling a function with $1, you need not use ${1-};
that's what happens in the mercurial completion script AFAICS.

-- 
Adeodato Simó                                     dato at net.com.org.es
Debian Developer                                  adeodato at debian.org
 
The surest way to corrupt a youth is to instruct him to hold in higher
esteem those who think alike than those who think differently.
                -- F. Nietzsche

Re: [PATCH] Update bash completions to prevent unbound variable errors.

[Adeodato Simó]

* Ted Pavlic [Mon, 12 Jan 2009 14:58:28 -0500]:

I don't know if this patch will go forward or not, but there are several
instances of spurious ${x-}, eg.:

>  __gitdir ()
>  {
> -	if [ -z "$1" ]; then
> +	if [ -z "${1-}" ]; then

Given the above...

> -	elif [ -d "$1/.git" ]; then
> -		echo "$1/.git"
> +	elif [ -d "${1-}/.git" ]; then
> +		echo "${1-}/.git"
>  	else
> -		echo "$1"
> +		echo "${1-}"

... this other hunk is redundant, because if [ -z "${1-}" ] fails, then
$1 is surely set.

>  __git_ps1 ()
>  {
>  	local g="$(git rev-parse --git-dir 2>/dev/null)"
> -	if [ -n "$g" ]; then
> +	if [ -n "${g-}" ]; then

Spurious, $g is always set here.

> @@ -111,8 +111,8 @@ __git_ps1 ()
> -		if [ -n "$1" ]; then
> +		if [ -n "${1-}" ]; then

This one is okay...

> -			printf "$1" "${b##refs/heads/}$r"
> +			printf "${1-}" "${b##refs/heads/}$r"

But this one is unnecessary, if [ -n "${1-}" ] succeeds, then $1 is set.

And so on.

-- 
Adeodato Simó                                     dato at net.com.org.es
Debian Developer                                  adeodato at debian.org
 
The true teacher defends his pupils against his own personal influence.
                -- Amos Bronson Alcott