[PATCH] Simple update to bash completions to prevent unbound variable errors.

[PATCH] Simple update to bash completions to prevent unbound variable errors.

[Ted Pavlic]

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.

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

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 7b074d7..619e886 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -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

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

[Junio C Hamano]

Ted Pavlic <ted@tedpavlic.com> writes:

> A vim modeline has also been added for consistency.

Yuck.

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

Double yuck.

> diff --git a/contrib/completion/git-completion.bash
> b/contrib/completion/git-completion.bash
> index 7b074d7..619e886 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -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)

Good.  Would be better if you described what $1 and $2 mean.

> @@ -111,7 +115,7 @@ __git_ps1 ()
>  			fi
>  		fi
>
> -		if [ -n "$1" ]; then
> +		if [ $# -gt 0 ] && [ -n "$1" ]; then

I found the previous round's [ -n "${1-}" ] much easier to read, if we were to
do this.  If -n "${1-}", then "$1" is definitely set so nothing need to
change in the then ... else part.

> +# __gitcomp_1 requires 2 arguments

... and $1 and $2 mean?

>  __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

Yuck.  If you are taking advantage of the fact that "local one"
will bind one to emptiness anyway, can't you do something like:

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

to avoid this ugliness?

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

[Boyd Stephen Smith Jr.]

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

On Monday 2009 January 12 21:14:40 Junio C Hamano wrote:
>Ted Pavlic <ted@tedpavlic.com> writes:
>> A vim modeline has also been added for consistency.
>
>Yuck.

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.

>> @@ -111,7 +115,7 @@ __git_ps1 ()
>>  			fi
>>  		fi
>>
>> -		if [ -n "$1" ]; then
>> +		if [ $# -gt 0 ] && [ -n "$1" ]; then
>
>I found the previous round's [ -n "${1-}" ] much easier to read, if we were
> to do this.  If -n "${1-}", then "$1" is definitely set so nothing need to
> change in the then ... else part.

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

>> @@ -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
>
>Yuck.

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.

>If you are taking advantage of the fact that "local one" 
>will bind one to emptiness anyway, can't you do something like:
>
>	local one=${1-} two=${2-} cur=${3-} four=${4-}

Even better to use variable names that match the usage, if possible. 
-- 
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] Simple update to bash completions to prevent unbound variable errors.

[Ted Pavlic]

>> A vim modeline has also been added for consistency.
>
> Yuck.

Better that than have a mixture of spaces and tabs.

>> +# __git_ps1 accepts 0 or 1 arguments (i.e., format string)
>> +# returns text to add to bash PS1 prompt (includes branch name)
>
> Good.  Would be better if you described what $1 and $2 mean.

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.

>> -		if [ -n "$1" ]; then
>> +		if [ $# -gt 0 ]&&  [ -n "$1" ]; then
>
> I found the previous round's [ -n "${1-}" ] much easier to read, if we were to
> do this.  If -n "${1-}", then "$1" is definitely set so nothing need to
> change in the then ... else part.

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.

>> +# __gitcomp_1 requires 2 arguments
>
> ... and $1 and $2 mean?

No clue. Patches are welcome.

> Yuck.  If you are taking advantage of the fact that "local one"
> will bind one to emptiness anyway, can't you do something like:
>
> 	local one=${1-} two=${2-} cur=${3-} four=${4-}

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 <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] Simple update to bash completions to prevent unbound variable errors.

[Ted Pavlic]

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

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.

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

It was just a suggestion.

>> 	local one=${1-} two=${2-} cur=${3-} four=${4-}
>
> Even better to use variable names that match the usage, if possible.

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 <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] Simple update to bash completions to prevent unbound variable errors.

[Ted Pavlic]

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

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 <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] Simple update to bash completions to prevent unbound variable errors.

[Junio C Hamano]

Ted Pavlic <ted@tedpavlic.com> writes:

>>> +# __gitcomp_1 requires 2 arguments
>>
>> ... and $1 and $2 mean?
>
> No clue. Patches are welcome.

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.

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

[Teemu Likonen]

Ted Pavlic (2009-01-12 23:50 -0500) wrote:

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

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

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

[Junio C Hamano]

Ted Pavlic <ted@tedpavlic.com> writes:

> An alternative (to a Vim modeline) is to put
>
> #!bash

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.