Skip to contentSkip to conversation

Update bash completions to prevent unbound variable errors.

PATCH In review Ted Pavlic 4 participants 14 messages Jun 15, 2016

lore
Share & tools
lore

Ted Pavlic

author
#0 lore raw

PATCH 1/1 Update bash completions to prevent unbound variable errors.

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.

---
 contrib/completion/git-completion.bash |   68 
++++++++++++++++----------------
 1 files changed, 34 insertions(+), 34 deletions(-)
contrib/completion/git-completion.bash +34 -34
index 7b074d7..50e345f 100755
@@ -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 "
Signature · 1 line
1.6.1.87.g15624
#1 lore raw
Quoted 11 lines · 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?

Signature · 4 lines
Boyd Stephen Smith Jr.                     ,= ,-_-. =. 
[email protected]                     ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy           `-'(. .)`-' 
http://iguanasuicide.net/                      \_/     
#2 lore raw
Quoted 10 lines · Boyd Stephen Smith Jr.
Quoted 7 lines

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.)

Signature · 6 lines
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
#3 lore raw
Quoted 9 lines · On Monday 2009 January 12 14:40:30 Adeodato Simó wrote:
Quoted 8 lines · Boyd Stephen Smith Jr.
Quoted 7 lines

The attached patch replaces things like

        if [ -z "$1" ]

with

        if [ -z "${1-}" ]
Quoted 7 lines
Quoted 2 lines

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.

Signature · 4 lines
Boyd Stephen Smith Jr.                     ,= ,-_-. =. 
[email protected]                     ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy           `-'(. .)`-' 
http://iguanasuicide.net/                      \_/     
#4 lore raw
Quoted 5 lines · Boyd Stephen Smith Jr.

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.

Signature · 1 line
Shawn.
#5 lore raw
Quoted 7 lines · On Monday 2009 January 12 15:31:49 Shawn O. Pearce wrote:
Quoted 4 lines · Boyd Stephen Smith Jr.

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.

Signature · 4 lines
Boyd Stephen Smith Jr.                     ,= ,-_-. =. 
[email protected]                     ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy           `-'(. .)`-' 
http://iguanasuicide.net/                      \_/     
author
#6 lore raw
In reply to Boyd Stephen Smith Jr.
Quoted 4 lines
Quoted 1 line
         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)

Signature · 5 lines
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.
#7 lore raw
Quoted 8 lines · Ted Pavlic
Quoted 2 lines

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.

Quoted 2 lines

(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.

Signature · 4 lines
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.
author
#8 lore raw
Quoted 2 lines

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

Signature · 5 lines
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.
#9 lore raw
Quoted 6 lines · Ted Pavlic
Quoted 2 lines

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.

Quoted 2 lines

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.

Signature · 6 lines
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
author
#10 lore raw
In reply to Ted Pavlic
Quoted 2 lines

(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.

Signature · 5 lines
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.
#11 lore raw
Quoted 5 lines · Ted Pavlic
Quoted 2 lines

(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

Quoted 7 lines

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.

Signature · 1 line
Shawn.
author
#12 lore raw
Quoted 4 lines
Quoted 4 lines

It appears like they use

complete -o bashdefault

Quoted 8 lines
Quoted 5 lines

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

Signature · 5 lines
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.
#13 lore raw
In reply to Ted Pavlic

* 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.:

contrib/completion/git-completion.bash esac · line 55

Quoted 4 lines

__gitdir ()
{

contrib/completion/git-completion.bash +1 -1
-	if [ -z "$1" ]; then
+	if [ -z "${1-}" ]; then

Given the above...

contrib/completion/git-completion.bash esac · line 66

Quoted 7 lines
contrib/completion/git-completion.bash +3 -3
-	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.

contrib/completion/git-completion.bash esac · line 73

Quoted 5 lines

__git_ps1 ()
{

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

Spurious, $g is always set here.

contrib/completion/git-completion.bash __git_ps1 () · line 111

Quoted 3 lines
contrib/completion/git-completion.bash +1 -1
@@ -111,8 +111,8 @@ __git_ps1 ()
-		if [ -n "$1" ]; then
+		if [ -n "${1-}" ]; then

This one is okay...

contrib/completion/git-completion.bash __git_ps1 () · line 115

Quoted 2 lines
contrib/completion/git-completion.bash +1 -1
-			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.

Signature · 5 lines
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