Update bash completions to prevent unbound variable errors.
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
@@ -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
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/ \_/
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 TravagliaQuoted 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 +uaffects 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 inset -umode. (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/ \_/
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
_gitand_gitkfunctions, 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.
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
_gitand_gitkfunctions, 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/ \_/
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.
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.
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.
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. NietzscheQuoted 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.
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.
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.
* 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
-1local 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