threads / patch / 17141

patchSimplest update to bash completions to prevent unbounded variable errors

Subject: [PATCH] Simplest update to bash completions to prevent unbounded variable errors

## tl;dr

4 messages between Jan 13, 2009 and Jan 13, 2009. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Ted Pavlic· Jan 13, 2009, 04:58 UTC · lore
Another try at fixing bash completions in "set -u" environments.
Here, I've gone back to changing $# to ${#-}, but only where necessary.
Additionally added some comments and omitted things like Vim modelines.
Signed-off-by: Ted Pavlic <ted@tedpavlic.com>
---
  contrib/completion/git-completion.bash |   42 
++++++++++++++++++++++---------
  1 files changed, 30 insertions(+), 12 deletions(-)
Show changes to contrib/completion/git-completion.bash +30 −12
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 7b074d7..323829e 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1,3 +1,4 @@
+#!bash
  #
  # bash completion support for core Git.
  #
@@ -50,9 +51,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 +70,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 +116,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 +124,7 @@ __git_ps1 ()
  	fi
  }

+# __gitcomp_1 requires 2 arguments
  __gitcomp_1 ()
  {
  	local c IFS=' '$'\t'$'\n'
@@ -131,6 +137,8 @@ __gitcomp_1 ()
  	done
  }

+# __gitcomp accepts 1, 2, 3, or 4 arguments
+# generates completion reply with compgen
  __gitcomp ()
  {
  	local cur="${COMP_WORDS[COMP_CWORD]}"
@@ -143,22 +151,23 @@ __gitcomp ()
  		;;
  	*)
  		local IFS=$'\n'
-		COMPREPLY=($(compgen -P "$2" \
-			-W "$(__gitcomp_1 "$1" "$4")" \
+		COMPREPLY=($(compgen -P "${2-}" \
+			-W "$(__gitcomp_1 "${1-}" "${4-}")" \
  			-- "$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 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 ;;
@@ -168,15 +177,16 @@ __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 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 ;;
@@ -186,9 +196,10 @@ __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 i is_hash=y dir="$(__gitdir "${1-}")"
  	local cur="${COMP_WORDS[COMP_CWORD]}" format refs
  	if [ -d "$dir" ]; then
  		case "$cur" in
@@ -218,6 +229,7 @@ __git_refs ()
  	done
  }

+# __git_refs2 requires 1 argument (to pass to __git_refs)
  __git_refs2 ()
  {
  	local i
@@ -226,6 +238,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 +483,7 @@ __git_aliases ()
  	done
  }

+# __git_aliased_command requires 1 argument
  __git_aliased_command ()
  {
  	local word cmdline=$(git --git-dir="$(__gitdir)" \
@@ -482,6 +496,7 @@ __git_aliased_command ()
  	done
  }

+# __git_find_subcommand requires 1 argument
  __git_find_subcommand ()
  {
  	local word subcommand c=1
@@ -1766,13 +1781,16 @@ _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

-- 
1.6.1.87.g15624
Shawn O. Pearce· Jan 13, 2009, 15:20 UTC · re: Ted Pavlic · lore

Re: [PATCH] Simplest update to bash completions to prevent unbounded variable errors

Ted Pavlic <ted@tedpavlic.com> wrote:
> Another try at fixing bash completions in "set -u" environments.

I agree with Junio; setting -u in your interactive shell is as bad as export CDPATH. Its crazy.

> Additionally added some comments and omitted things like Vim modelines.

These are orthogonal to the -u corrections. They should be in a different patch. The comments are wecome. The '#!bash' looks like a good idea. But a vim specific modeline, I don't like, for the reasons Junio has already stated.

Show 6 quoted lines
> +# __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

This is one of those places where [ -z "${1-}" ] is likely easier to read then the || usage you have introduced. We don't care if we got no args, or we got one that is the empty string, either way the $1 cannot be a gitdir and we need to guess it.

Show 7 quoted lines
> @@ -111,7 +116,7 @@ __git_ps1 ()
>  			fi
>  		fi
>
> -		if [ -n "$1" ]; then
> +		if [ $# -gt 0 ] && [ -n "$1" ]; then
>  			printf "$1" "${b##refs/heads/}$r"
Eh, I'd rather see [ -n "${1-}" ] over the && test.
Show 6 quoted lines
> -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

Why are we switching to bashdefault? Is this an unrelated change from the -u stuff and should go into its own commit, with its own justification?

-- 
Shawn.
Ted Pavlic· Jan 13, 2009, 15:30 UTC · re: Shawn O. Pearce · lore

Re: [PATCH] Simplest update to bash completions to prevent unbounded variable errors

>> Another try at fixing bash completions in "set -u" environments.
> I agree with Junio; setting -u in your interactive shell is as bad
> as export CDPATH.  Its crazy.

This whole series of patches was inspired by a group of workstations at a university that set -u by default for all users.

Additionally, doesn't "set -u" make tcsh users feel more at home in bash? Certainly other shells have this same behavior in their interactive modes.

Show 6 quoted lines
>> Additionally added some comments and omitted things like Vim modelines.
>
> These are orthogonal to the -u corrections.  They should be in a
> different patch.  The comments are wecome.  The '#!bash' looks like
> a good idea.  But a vim specific modeline, I don't like, for the
> reasons Junio has already stated.
OK. Can do.
>> -	if [ -z "$1" ]; then
>> +	if [ $# -eq 0 ] || [ -z "$1" ]; then
>
> This is one of those places where [ -z "${1-}" ] is likely easier
That was a mistake. I missed that hunk. I meant to use the ${1-}.
>> +		if [ $# -gt 0 ]&&  [ -n "$1" ]; then
>
> Eh, I'd rather see [ -n "${1-}" ] over the&&  test.
Again, my mistake. It was late and I missed it.
Show 8 quoted lines
>> +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
>
> Why are we switching to bashdefault?  Is this an unrelated change
> from the -u stuff and should go into its own commit, with its own
> justification?
Ok.
 From what I understand, normal bash completion is like setting "-o 
bashdefault -o default". That is, it tries the bash completions first 
before going to the filename completion. This change makes it so that 
git jumps back to bash completion if nothing git-specific is found. If 
nothing bash-specific is found, it will go back to standard default 
filename completion.
--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.
Shawn O. Pearce· Jan 13, 2009, 15:33 UTC · re: Ted Pavlic · lore

Re: [PATCH] Simplest update to bash completions to prevent unbounded variable errors

Ted Pavlic <ted@tedpavlic.com> wrote:
Show 6 quoted lines
>>> Another try at fixing bash completions in "set -u" environments.
>> I agree with Junio; setting -u in your interactive shell is as bad
>> as export CDPATH.  Its crazy.
>
> This whole series of patches was inspired by a group of workstations at  
> a university that set -u by default for all users.

The changes look less nasty than I originally thought. If you can split the history out and justify the changes in the corresponding commit messages, I think I can ACK the series.

-- 
Shawn.

← back to recent threads