threads / patch / 17175

patch, 3 partsbash-completion: Added comments to remind about required arguments

Subject: [PATCH 3/3] bash-completion: Added comments to remind about required arguments

## tl;dr

6 messages between Jan 14, 2009 and Jan 15, 2009. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

ted@tedpavlic.com· Jan 14, 2009, 20:02 UTC · lore

[PATCH 1/3] bash-completion: Support running when set -u is enabled

From: Ted Pavlic <ted@tedpavlic.com>
  Under "set -u" semantics, it is an error to access undefined
  variables. Some user environments may enable this setting in the
  interactive shell.
  In any context where the completion functions access an undefined
  variable, accessing a default empty string (aka "${1-}" instead of
  "$1") is a reasonable way to code the function, as it silences
  the undefined variable error while still supplying an empty string.
  In this patch, functions that should always take an argument still use
  $1. Functions that have optional arguments use ${1-}.
Signed-off-by: Ted Pavlic <ted@tedpavlic.com>
Acked-by: Shawn O. Pearce <spearce@spearce.org>
---
 contrib/completion/git-completion.bash |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)
Show changes to contrib/completion/git-completion.bash +9 −9
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 7b074d7..5d1515c 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -52,7 +52,7 @@ esac
 
 __gitdir ()
 {
-	if [ -z "$1" ]; then
+	if [ -z "${1-}" ]; then
 		if [ -n "$__git_dir" ]; then
 			echo "$__git_dir"
 		elif [ -d .git ]; then
@@ -111,7 +111,7 @@ __git_ps1 ()
 			fi
 		fi
 
-		if [ -n "$1" ]; then
+		if [ -n "${1-}" ]; then
 			printf "$1" "${b##refs/heads/}$r"
 		else
 			printf " (%s)" "${b##refs/heads/}$r"
@@ -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
-- 
1.6.1.87.g15624
ted@tedpavlic.com· Jan 14, 2009, 20:02 UTC · re: ted@tedpavlic.com · lore

[PATCH 2/3] bash-completion: Try bash completions before simple filetype

From: Ted Pavlic <ted@tedpavlic.com>
  When a git completion is not found, a bash shell should try bash-type
  completions first before going to standard filetype completions. This
  patch /adds/ "-o bashdefault" to the completion line. If that option
  is not available, it uses the old method.
  This behavior was inspired by Mercurial's bash completion script.
Signed-off-by: Ted Pavlic <ted@tedpavlic.com>
Acked-by: Shawn O. Pearce <spearce@spearce.org>
---
 contrib/completion/git-completion.bash |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)
Show changes to contrib/completion/git-completion.bash +6 −3
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 5d1515c..201f9a6 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1766,13 +1766,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
ted@tedpavlic.com· Jan 14, 2009, 20:02 UTC · re: ted@tedpavlic.com · lore
From: Ted Pavlic <ted@tedpavlic.com>
  Adds a few simple comments above commands that take arguments. These
  comments are meant to remind developers of potential problems that can
  occur when the script is sourced on systems with "set -u." Any
  function which "requires" arguments really ought to be called with
  explicit arguments given.
  Also adds a #!bash to the top of bash completions so that editing
  software can always identify that the file is of sh type.
Signed-off-by: Ted Pavlic <ted@tedpavlic.com>
Acked-by: Shawn O. Pearce <spearce@spearce.org>
---
 contrib/completion/git-completion.bash |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)
Show changes to contrib/completion/git-completion.bash +15 −0
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 201f9a6..f8b845a 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,6 +51,8 @@ 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
@@ -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)"
@@ -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]}"
@@ -150,6 +158,7 @@ __gitcomp ()
 	esac
 }
 
+# __git_heads accepts 0 or 1 arguments (to pass to __gitdir)
 __git_heads ()
 {
 	local cmd i is_hash=y dir="$(__gitdir "${1-}")"
@@ -168,6 +177,7 @@ __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-}")"
@@ -186,6 +196,7 @@ __git_tags ()
 	done
 }
 
+# __git_refs accepts 0 or 1 arguments (to pass to __gitdir)
 __git_refs ()
 {
 	local i is_hash=y dir="$(__gitdir "${1-}")"
@@ -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
-- 
1.6.1.87.g15624
Shawn O. Pearce· Jan 15, 2009, 15:35 UTC · re: ted@tedpavlic.com · lore

Re: [PATCH 3/3] bash-completion: Added comments to remind about required arguments

ted@tedpavlic.com wrote:
Show 13 quoted lines
> From: Ted Pavlic <ted@tedpavlic.com>
> 
>   Adds a few simple comments above commands that take arguments. These
>   comments are meant to remind developers of potential problems that can
>   occur when the script is sourced on systems with "set -u." Any
>   function which "requires" arguments really ought to be called with
>   explicit arguments given.
> 
>   Also adds a #!bash to the top of bash completions so that editing
>   software can always identify that the file is of sh type.
> 
> Signed-off-by: Ted Pavlic <ted@tedpavlic.com>
> Acked-by: Shawn O. Pearce <spearce@spearce.org>

Yup, this and also 2/3 look fine. But again, the message, it shouldn't be indented.

> ---
>  contrib/completion/git-completion.bash |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)
-- 
Shawn.
Ted Pavlic· Jan 15, 2009, 15:47 UTC · re: Shawn O. Pearce · lore

Re: [PATCH 3/3] bash-completion: Added comments to remind about required arguments

> Yup, this and also 2/3 look fine.  But again, the message, it
> shouldn't be indented.
I will re-submit with no indentation.
Sorry for all the fuss. I've learned a great deal.

Thanks -- 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 15, 2009, 15:34 UTC · re: ted@tedpavlic.com · lore

Re: [PATCH 1/3] bash-completion: Support running when set -u is enabled

ted@tedpavlic.com wrote:
Show 16 quoted lines
> From: Ted Pavlic <ted@tedpavlic.com>
> 
>   Under "set -u" semantics, it is an error to access undefined
>   variables. Some user environments may enable this setting in the
>   interactive shell.
> 
>   In any context where the completion functions access an undefined
>   variable, accessing a default empty string (aka "${1-}" instead of
>   "$1") is a reasonable way to code the function, as it silences
>   the undefined variable error while still supplying an empty string.
> 
>   In this patch, functions that should always take an argument still use
>   $1. Functions that have optional arguments use ${1-}.
> 
> Signed-off-by: Ted Pavlic <ted@tedpavlic.com>
> Acked-by: Shawn O. Pearce <spearce@spearce.org>

Yup. This patch isn't as bad as everyone made it out to be. I think we should go ahead and include it.

But usually we don't indent the commit message like you did above; instead the message should be aligned on the left margin at column 0. git log will indent the message during display, hence any identation you add in the patch email just makes the message that much harder to read from git log.

> ---
>  contrib/completion/git-completion.bash |   18 +++++++++---------
>  1 files changed, 9 insertions(+), 9 deletions(-)
-- 
Shawn.

← back to recent threads