[PATCH 0/4] completion: Fixes and better non-work-tree support

[PATCH 0/4] completion: Fixes and better non-work-tree support

[Ted Pavlic]

The first patch has already been Acked by spearce.

The second patch uses a consistent "if..." convention throughout the
completion script.

The third patch improves how the __git_ps1 works when (a) the CWD is a
git dir and (b) when the CWD has a .git but no .git/HEAD.

The fourth patch fixes unbound variable errors by replacing $__vars with
${__var-}s.

Ted Pavlic (4):
  completion: For consistency, changed "git rev-parse" to __gitdir
    calls.
  completion: Use consistent if [...] convention. No test.
  completion: Better __git_ps1 support when not in working directory
  completion: More fixes to prevent unbound variable errors.

 contrib/completion/git-completion.bash |   75 ++++++++++++++++----------------
 1 files changed, 37 insertions(+), 38 deletions(-)

[PATCH 1/4] completion: For consistency, changed "git rev-parse" to __gitdir calls.

[Ted Pavlic]

Signed-off-by: Ted Pavlic <ted@tedpavlic.com>
Acked-by: Shawn O. Pearce <spearce@spearce.org>
---
 contrib/completion/git-completion.bash |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index f44f63c..6bbe09a 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -80,7 +80,7 @@ __gitdir ()
 # returns text to add to bash PS1 prompt (includes branch name)
 __git_ps1 ()
 {
-	local g="$(git rev-parse --git-dir 2>/dev/null)"
+	local g="$(__gitdir)"
 	if [ -n "$g" ]; then
 		local r
 		local b
@@ -1797,7 +1797,7 @@ _gitk ()
 	__git_has_doubledash && return
 
 	local cur="${COMP_WORDS[COMP_CWORD]}"
-	local g="$(git rev-parse --git-dir 2>/dev/null)"
+	local g="$(__gitdir)"
 	local merge=""
 	if [ -f $g/MERGE_HEAD ]; then
 		merge="--merge"
-- 
1.6.1.2.390.gba743

[PATCH 2/4] completion: Use consistent if [...] convention. No test.

[Ted Pavlic]

The local coding convention in bash completion is to use [...] rather
than test. Additionally,

    if [...]; then

is preferred over

    if [...]
    then

and so matching "if [...]\nthen" were changed accordingly.

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6bbe09a..7706170 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -84,39 +84,30 @@ __git_ps1 ()
 	if [ -n "$g" ]; then
 		local r
 		local b
-		if [ -d "$g/rebase-apply" ]
-		then
-			if test -f "$g/rebase-apply/rebasing"
-			then
+		if [ -d "$g/rebase-apply" ]; then
+			if [ -f "$g/rebase-apply/rebasing" ]; then
 				r="|REBASE"
-			elif test -f "$g/rebase-apply/applying"
-			then
+            elif [ -f "$g/rebase-apply/applying" ]; then
 				r="|AM"
 			else
 				r="|AM/REBASE"
 			fi
 			b="$(git symbolic-ref HEAD 2>/dev/null)"
-		elif [ -f "$g/rebase-merge/interactive" ]
-		then
+		elif [ -f "$g/rebase-merge/interactive" ]; then
 			r="|REBASE-i"
 			b="$(cat "$g/rebase-merge/head-name")"
-		elif [ -d "$g/rebase-merge" ]
-		then
+		elif [ -d "$g/rebase-merge" ]; then
 			r="|REBASE-m"
 			b="$(cat "$g/rebase-merge/head-name")"
-		elif [ -f "$g/MERGE_HEAD" ]
-		then
+		elif [ -f "$g/MERGE_HEAD" ]; then
 			r="|MERGING"
 			b="$(git symbolic-ref HEAD 2>/dev/null)"
 		else
-			if [ -f "$g/BISECT_LOG" ]
-			then
+			if [ -f "$g/BISECT_LOG" ]; then
 				r="|BISECTING"
 			fi
-			if ! b="$(git symbolic-ref HEAD 2>/dev/null)"
-			then
-				if ! b="$(git describe --exact-match HEAD 2>/dev/null)"
-				then
+			if ! b="$(git symbolic-ref HEAD 2>/dev/null)"; then
+				if ! b="$(git describe --exact-match HEAD 2>/dev/null)"; then
 					b="$(cut -c1-7 "$g/HEAD")..."
 				fi
 			fi
@@ -125,8 +116,8 @@ __git_ps1 ()
 		local w
 		local i
 
-		if test -n "${GIT_PS1_SHOWDIRTYSTATE-}"; then
-			if test "$(git config --bool bash.showDirtyState)" != "false"; then
+		if [ -n "${GIT_PS1_SHOWDIRTYSTATE-}" ]; then
+			if [ "$(git config --bool bash.showDirtyState)" != "false" ]; then
 				git diff --no-ext-diff --ignore-submodules \
 					--quiet --exit-code || w="*"
 				if git rev-parse --quiet --verify HEAD >/dev/null; then
-- 
1.6.1.2.390.gba743

[PATCH 3/4] completion: Better __git_ps1 support when not in working directory

[Ted Pavlic]

If .git/HEAD is not readable, __git_ps1 does nothing.

If --is-in-git-dir, __git_ps1 returns " (GIT_DIR!)" as a cautionary
note. The previous behavior would show the branch name (and would
optionally attempt to determine the dirtyState of the directory, which
was impossible because a "git diff" was used).

If --is-in-work-tree, __git_ps1 returns the branch name. Additionally,
if showDirtyState is on, the dirty state is displayed.

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 7706170..c28d6be 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -108,7 +108,9 @@ __git_ps1 ()
 			fi
 			if ! b="$(git symbolic-ref HEAD 2>/dev/null)"; then
 				if ! b="$(git describe --exact-match HEAD 2>/dev/null)"; then
-					b="$(cut -c1-7 "$g/HEAD")..."
+					if [ -r "$g/HEAD" ]; then
+						b="$(cut -c1-7 "$g/HEAD")..."
+					fi
 				fi
 			fi
 		fi
@@ -116,23 +118,29 @@ __git_ps1 ()
 		local w
 		local i
 
-		if [ -n "${GIT_PS1_SHOWDIRTYSTATE-}" ]; then
-			if [ "$(git config --bool bash.showDirtyState)" != "false" ]; then
-				git diff --no-ext-diff --ignore-submodules \
-					--quiet --exit-code || w="*"
-				if git rev-parse --quiet --verify HEAD >/dev/null; then
-					git diff-index --cached --quiet \
-						--ignore-submodules HEAD -- || i="+"
-				else
-					i="#"
+		if [ "true" = "$(git rev-parse --is-inside-git-dir 2>/dev/null)" ]; then
+			b="GIT_DIR!"
+		elif [ "true" = "$(git rev-parse --is-inside-work-tree 2>/dev/null)" ]; then
+			if [ -n "${GIT_PS1_SHOWDIRTYSTATE-}" ]; then
+				if [ "$(git config --bool bash.showDirtyState)" != "false" ]; then
+					git diff --no-ext-diff --ignore-submodules \
+						--quiet --exit-code || w="*"
+					if git rev-parse --quiet --verify HEAD >/dev/null; then
+						git diff-index --cached --quiet \
+							--ignore-submodules HEAD -- || i="+"
+					else
+						i="#"
+					fi
 				fi
 			fi
 		fi
 
-		if [ -n "${1-}" ]; then
-			printf "$1" "${b##refs/heads/}$w$i$r"
-		else
-			printf " (%s)" "${b##refs/heads/}$w$i$r"
+		if [ -n "$b" ]; then
+			if [ -n "${1-}" ]; then
+				printf "$1" "${b##refs/heads/}$w$i$r"
+			else
+				printf " (%s)" "${b##refs/heads/}$w$i$r"
+			fi
 		fi
 	fi
 }
-- 
1.6.1.2.390.gba743

[PATCH 4/4] completion: More fixes to prevent unbound variable errors.

[Ted Pavlic]

Several functions make use of "[-n ...]" and "[-z ...]". In many cases,
the variables being tested were declared with "local." However, several
__variables are not, and so they must be replaced with their ${__-}
equivalents.

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c28d6be..371148b 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -62,7 +62,7 @@ esac
 __gitdir ()
 {
 	if [ -z "${1-}" ]; then
-		if [ -n "$__git_dir" ]; then
+		if [ -n "${__git_dir-}" ]; then
 			echo "$__git_dir"
 		elif [ -d .git ]; then
 			echo .git
@@ -298,7 +298,7 @@ __git_remotes ()
 
 __git_merge_strategies ()
 {
-	if [ -n "$__git_merge_strategylist" ]; then
+	if [ -n "${__git_merge_strategylist-}" ]; then
 		echo "$__git_merge_strategylist"
 		return
 	fi
@@ -384,7 +384,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
@@ -402,7 +402,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
-- 
1.6.1.2.390.gba743

Re: [PATCH 2/4] completion: Use consistent if [...] convention. No test.

[Shawn O. Pearce]

Ted Pavlic <ted@tedpavlic.com> wrote:
> The local coding convention in bash completion is to use [...] rather
> than test. Additionally,
> 
>     if [...]; then
> 
> is preferred over
> 
>     if [...]
>     then
> 
> and so matching "if [...]\nthen" were changed accordingly.
> 
> Signed-off-by: Ted Pavlic <ted@tedpavlic.com>
> ---
>  contrib/completion/git-completion.bash |   31 +++++++++++--------------------
>  1 files changed, 11 insertions(+), 20 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 6bbe09a..7706170 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -84,39 +84,30 @@ __git_ps1 ()
>  	if [ -n "$g" ]; then
>  		local r
>  		local b
> -		if [ -d "$g/rebase-apply" ]
> -		then
> -			if test -f "$g/rebase-apply/rebasing"
> -			then
> +		if [ -d "$g/rebase-apply" ]; then
> +			if [ -f "$g/rebase-apply/rebasing" ]; then
>  				r="|REBASE"
> -			elif test -f "$g/rebase-apply/applying"
> -			then
> +            elif [ -f "$g/rebase-apply/applying" ]; then

There is some sort of whitespace damage right here, the elif doesn't
seem to line up correctly.

-- 
Shawn.

Re: [PATCH 3/4] completion: Better __git_ps1 support when not in working directory

[Shawn O. Pearce]

Ted Pavlic <ted@tedpavlic.com> wrote:
> If .git/HEAD is not readable, __git_ps1 does nothing.
> 
> If --is-in-git-dir, __git_ps1 returns " (GIT_DIR!)" as a cautionary
> note. The previous behavior would show the branch name (and would
> optionally attempt to determine the dirtyState of the directory, which
> was impossible because a "git diff" was used).
> 
> If --is-in-work-tree, __git_ps1 returns the branch name. Additionally,
> if showDirtyState is on, the dirty state is displayed.
> 
> Signed-off-by: Ted Pavlic <ted@tedpavlic.com>

Acked-by: Shawn O. Pearce <spearce@spearce.org>

> ---
>  contrib/completion/git-completion.bash |   36 +++++++++++++++++++------------
>  1 files changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 7706170..c28d6be 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -108,7 +108,9 @@ __git_ps1 ()
>  			fi
>  			if ! b="$(git symbolic-ref HEAD 2>/dev/null)"; then
>  				if ! b="$(git describe --exact-match HEAD 2>/dev/null)"; then
> -					b="$(cut -c1-7 "$g/HEAD")..."
> +					if [ -r "$g/HEAD" ]; then
> +						b="$(cut -c1-7 "$g/HEAD")..."
> +					fi
>  				fi
>  			fi
>  		fi
> @@ -116,23 +118,29 @@ __git_ps1 ()
>  		local w
>  		local i
>  
> -		if [ -n "${GIT_PS1_SHOWDIRTYSTATE-}" ]; then
> -			if [ "$(git config --bool bash.showDirtyState)" != "false" ]; then
> -				git diff --no-ext-diff --ignore-submodules \
> -					--quiet --exit-code || w="*"
> -				if git rev-parse --quiet --verify HEAD >/dev/null; then
> -					git diff-index --cached --quiet \
> -						--ignore-submodules HEAD -- || i="+"
> -				else
> -					i="#"
> +		if [ "true" = "$(git rev-parse --is-inside-git-dir 2>/dev/null)" ]; then
> +			b="GIT_DIR!"
> +		elif [ "true" = "$(git rev-parse --is-inside-work-tree 2>/dev/null)" ]; then
> +			if [ -n "${GIT_PS1_SHOWDIRTYSTATE-}" ]; then
> +				if [ "$(git config --bool bash.showDirtyState)" != "false" ]; then
> +					git diff --no-ext-diff --ignore-submodules \
> +						--quiet --exit-code || w="*"
> +					if git rev-parse --quiet --verify HEAD >/dev/null; then
> +						git diff-index --cached --quiet \
> +							--ignore-submodules HEAD -- || i="+"
> +					else
> +						i="#"
> +					fi
>  				fi
>  			fi
>  		fi
>  
> -		if [ -n "${1-}" ]; then
> -			printf "$1" "${b##refs/heads/}$w$i$r"
> -		else
> -			printf " (%s)" "${b##refs/heads/}$w$i$r"
> +		if [ -n "$b" ]; then
> +			if [ -n "${1-}" ]; then
> +				printf "$1" "${b##refs/heads/}$w$i$r"
> +			else
> +				printf " (%s)" "${b##refs/heads/}$w$i$r"
> +			fi
>  		fi
>  	fi
>  }
> -- 
> 1.6.1.2.390.gba743
> 

-- 
Shawn.

Re: [PATCH 4/4] completion: More fixes to prevent unbound variable errors.

[Shawn O. Pearce]

Ted Pavlic <ted@tedpavlic.com> wrote:
> Several functions make use of "[-n ...]" and "[-z ...]". In many cases,
> the variables being tested were declared with "local." However, several
> __variables are not, and so they must be replaced with their ${__-}
> equivalents.
> 
> Signed-off-by: Ted Pavlic <ted@tedpavlic.com>

Acked-by: Shawn O. Pearce <spearce@spearce.org>

> ---
>  contrib/completion/git-completion.bash |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index c28d6be..371148b 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -62,7 +62,7 @@ esac
>  __gitdir ()
>  {
>  	if [ -z "${1-}" ]; then
> -		if [ -n "$__git_dir" ]; then
> +		if [ -n "${__git_dir-}" ]; then
>  			echo "$__git_dir"
>  		elif [ -d .git ]; then
>  			echo .git
> @@ -298,7 +298,7 @@ __git_remotes ()
>  
>  __git_merge_strategies ()
>  {
> -	if [ -n "$__git_merge_strategylist" ]; then
> +	if [ -n "${__git_merge_strategylist-}" ]; then
>  		echo "$__git_merge_strategylist"
>  		return
>  	fi
> @@ -384,7 +384,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
> @@ -402,7 +402,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
> -- 
> 1.6.1.2.390.gba743
> 

-- 
Shawn.

Re: [PATCH 2/4] completion: Use consistent if [...] convention. No test.

[Junio C Hamano]

Ted Pavlic <ted@tedpavlic.com> writes:

> -		if [ -d "$g/rebase-apply" ]
> -		then
> -			if test -f "$g/rebase-apply/rebasing"
> -			then
> +		if [ -d "$g/rebase-apply" ]; then
> +			if [ -f "$g/rebase-apply/rebasing" ]; then
>  				r="|REBASE"
> -			elif test -f "$g/rebase-apply/applying"
> -			then
> +            elif [ -f "$g/rebase-apply/applying" ]; then
>  				r="|AM"
>  			else

What's with this funny indentation?

As a general rule, it usually is a good idea to apply clean-up to the
codebase before starting substantial work, but that holds true only when
the clean-up is undisputed.  Otherwise you would end up holding the later,
more "interesting" work a hostage to an earlier potentially controversial
"clean-up".

I think this particular clean-up makes the odd-ball __git_ps1 more
consnstent with the rest of the script, but it ultimately is Shawn's
call.

Re: [PATCH 2/4] completion: Use consistent if [...] convention. No test.

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

>> -			elif test -f "$g/rebase-apply/applying"
>> -			then
>> +            elif [ -f "$g/rebase-apply/applying" ]; then
>
> There is some sort of whitespace damage right here, the elif doesn't
> seem to line up correctly.

If that is the only gripe and otherwise if you are Ok with the patch, I'll
queue the entire series with a fix-up here myself.

Re: [PATCH 2/4] completion: Use consistent if [...] convention. No test.

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> >> -			elif test -f "$g/rebase-apply/applying"
> >> -			then
> >> +            elif [ -f "$g/rebase-apply/applying" ]; then
> >
> > There is some sort of whitespace damage right here, the elif doesn't
> > seem to line up correctly.
> 
> If that is the only gripe and otherwise if you are Ok with the patch, I'll
> queue the entire series with a fix-up here myself.

Yes, this patch is fine, if you can hand-fix the whitespace damage,
please feel free to add my Ack.

-- 
Shawn.

Re: [PATCH 2/4] completion: Use consistent if [...] convention. No test.

[Ted Pavlic]

> What's with this funny indentation?

In Vim, I have "exapndtabs" set by default. I manually turned it off, 
but during testing, I must have closed Vim and re-opened and forgot to 
turn it back off.

Some time ago, I suggested adding a vim modeline to the file to handle 
turning expandtabs off for everyone who edits it with vim. That change 
got denied.

If you'd like, I can clean up the whitespace and resubmit with Shawn's Acks.

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

[PATCH 0/4] completion fixes: Acks, whitespace, and r=""

[Ted Pavlic]

* Added Shawn O. Pearce acks
* Fixed whitespace problem in second patch (vim:expandtabs was on)
* When in GIT_DIR, set r="" in __git_ps1 as well

Ted Pavlic (4):
  completion: For consistency, changed "git rev-parse" to __gitdir
    calls.
  completion: Use consistent if [...] convention. No test.
  completion: Better __git_ps1 support when not in working directory
  completion: More fixes to prevent unbound variable errors.

 contrib/completion/git-completion.bash |   76 ++++++++++++++++----------------
 1 files changed, 38 insertions(+), 38 deletions(-)

[PATCH 1/4] completion: For consistency, changed "git rev-parse" to __gitdir calls.

[Ted Pavlic]

Signed-off-by: Ted Pavlic <ted@tedpavlic.com>
Acked-by: Shawn O. Pearce <spearce@spearce.org>
---
 contrib/completion/git-completion.bash |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index f44f63c..6bbe09a 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -80,7 +80,7 @@ __gitdir ()
 # returns text to add to bash PS1 prompt (includes branch name)
 __git_ps1 ()
 {
-	local g="$(git rev-parse --git-dir 2>/dev/null)"
+	local g="$(__gitdir)"
 	if [ -n "$g" ]; then
 		local r
 		local b
@@ -1797,7 +1797,7 @@ _gitk ()
 	__git_has_doubledash && return
 
 	local cur="${COMP_WORDS[COMP_CWORD]}"
-	local g="$(git rev-parse --git-dir 2>/dev/null)"
+	local g="$(__gitdir)"
 	local merge=""
 	if [ -f $g/MERGE_HEAD ]; then
 		merge="--merge"
-- 
1.6.1.2.390.gba743

[PATCH 2/4] completion: Use consistent if [...] convention. No test.

[Ted Pavlic]

The local coding convention in bash completion is to use [...] rather
than test. Additionally,

    if [...]; then

is preferred over

    if [...]
    then

and so matching "if [...]\nthen" were changed accordingly.

Signed-off-by: Ted Pavlic <ted@tedpavlic.com>
Acked-by: Shawn O. Pearce <spearce@spearce.org>
---
 contrib/completion/git-completion.bash |   31 +++++++++++--------------------
 1 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6bbe09a..e729944 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -84,39 +84,30 @@ __git_ps1 ()
 	if [ -n "$g" ]; then
 		local r
 		local b
-		if [ -d "$g/rebase-apply" ]
-		then
-			if test -f "$g/rebase-apply/rebasing"
-			then
+		if [ -d "$g/rebase-apply" ]; then
+			if [ -f "$g/rebase-apply/rebasing" ]; then
 				r="|REBASE"
-			elif test -f "$g/rebase-apply/applying"
-			then
+			elif [ -f "$g/rebase-apply/applying" ]; then
 				r="|AM"
 			else
 				r="|AM/REBASE"
 			fi
 			b="$(git symbolic-ref HEAD 2>/dev/null)"
-		elif [ -f "$g/rebase-merge/interactive" ]
-		then
+		elif [ -f "$g/rebase-merge/interactive" ]; then
 			r="|REBASE-i"
 			b="$(cat "$g/rebase-merge/head-name")"
-		elif [ -d "$g/rebase-merge" ]
-		then
+		elif [ -d "$g/rebase-merge" ]; then
 			r="|REBASE-m"
 			b="$(cat "$g/rebase-merge/head-name")"
-		elif [ -f "$g/MERGE_HEAD" ]
-		then
+		elif [ -f "$g/MERGE_HEAD" ]; then
 			r="|MERGING"
 			b="$(git symbolic-ref HEAD 2>/dev/null)"
 		else
-			if [ -f "$g/BISECT_LOG" ]
-			then
+			if [ -f "$g/BISECT_LOG" ]; then
 				r="|BISECTING"
 			fi
-			if ! b="$(git symbolic-ref HEAD 2>/dev/null)"
-			then
-				if ! b="$(git describe --exact-match HEAD 2>/dev/null)"
-				then
+			if ! b="$(git symbolic-ref HEAD 2>/dev/null)"; then
+				if ! b="$(git describe --exact-match HEAD 2>/dev/null)"; then
 					b="$(cut -c1-7 "$g/HEAD")..."
 				fi
 			fi
@@ -125,8 +116,8 @@ __git_ps1 ()
 		local w
 		local i
 
-		if test -n "${GIT_PS1_SHOWDIRTYSTATE-}"; then
-			if test "$(git config --bool bash.showDirtyState)" != "false"; then
+		if [ -n "${GIT_PS1_SHOWDIRTYSTATE-}" ]; then
+			if [ "$(git config --bool bash.showDirtyState)" != "false" ]; then
 				git diff --no-ext-diff --ignore-submodules \
 					--quiet --exit-code || w="*"
 				if git rev-parse --quiet --verify HEAD >/dev/null; then
-- 
1.6.1.2.390.gba743

[PATCH 3/4] completion: Better __git_ps1 support when not in working directory

[Ted Pavlic]

If .git/HEAD is not readable, __git_ps1 does nothing.

If --is-in-git-dir, __git_ps1 returns " (GIT_DIR!)" as a cautionary
note. The previous behavior would show the branch name (and would
optionally attempt to determine the dirtyState of the directory, which
was impossible because a "git diff" was used).

If --is-in-work-tree, __git_ps1 returns the branch name. Additionally,
if showDirtyState is on, the dirty state is displayed.

Signed-off-by: Ted Pavlic <ted@tedpavlic.com>
Acked-by: Shawn O. Pearce <spearce@spearce.org>
---
 contrib/completion/git-completion.bash |   37 +++++++++++++++++++------------
 1 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index e729944..3d48a65 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -108,7 +108,9 @@ __git_ps1 ()
 			fi
 			if ! b="$(git symbolic-ref HEAD 2>/dev/null)"; then
 				if ! b="$(git describe --exact-match HEAD 2>/dev/null)"; then
-					b="$(cut -c1-7 "$g/HEAD")..."
+					if [ -r "$g/HEAD" ]; then
+						b="$(cut -c1-7 "$g/HEAD")..."
+					fi
 				fi
 			fi
 		fi
@@ -116,23 +118,30 @@ __git_ps1 ()
 		local w
 		local i
 
-		if [ -n "${GIT_PS1_SHOWDIRTYSTATE-}" ]; then
-			if [ "$(git config --bool bash.showDirtyState)" != "false" ]; then
-				git diff --no-ext-diff --ignore-submodules \
-					--quiet --exit-code || w="*"
-				if git rev-parse --quiet --verify HEAD >/dev/null; then
-					git diff-index --cached --quiet \
-						--ignore-submodules HEAD -- || i="+"
-				else
-					i="#"
+		if [ "true" = "$(git rev-parse --is-inside-git-dir 2>/dev/null)" ]; then
+			b="GIT_DIR!"
+            r=""
+		elif [ "true" = "$(git rev-parse --is-inside-work-tree 2>/dev/null)" ]; then
+			if [ -n "${GIT_PS1_SHOWDIRTYSTATE-}" ]; then
+				if [ "$(git config --bool bash.showDirtyState)" != "false" ]; then
+					git diff --no-ext-diff --ignore-submodules \
+						--quiet --exit-code || w="*"
+					if git rev-parse --quiet --verify HEAD >/dev/null; then
+						git diff-index --cached --quiet \
+							--ignore-submodules HEAD -- || i="+"
+					else
+						i="#"
+					fi
 				fi
 			fi
 		fi
 
-		if [ -n "${1-}" ]; then
-			printf "$1" "${b##refs/heads/}$w$i$r"
-		else
-			printf " (%s)" "${b##refs/heads/}$w$i$r"
+		if [ -n "$b" ]; then
+			if [ -n "${1-}" ]; then
+				printf "$1" "${b##refs/heads/}$w$i$r"
+			else
+				printf " (%s)" "${b##refs/heads/}$w$i$r"
+			fi
 		fi
 	fi
 }
-- 
1.6.1.2.390.gba743

[PATCH 4/4] completion: More fixes to prevent unbound variable errors.

[Ted Pavlic]

Several functions make use of "[-n ...]" and "[-z ...]". In many cases,
the variables being tested were declared with "local." However, several
__variables are not, and so they must be replaced with their ${__-}
equivalents.

Signed-off-by: Ted Pavlic <ted@tedpavlic.com>
Acked-by: Shawn O. Pearce <spearce@spearce.org>
---
 contrib/completion/git-completion.bash |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 3d48a65..8d62b2b 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -62,7 +62,7 @@ esac
 __gitdir ()
 {
 	if [ -z "${1-}" ]; then
-		if [ -n "$__git_dir" ]; then
+		if [ -n "${__git_dir-}" ]; then
 			echo "$__git_dir"
 		elif [ -d .git ]; then
 			echo .git
@@ -299,7 +299,7 @@ __git_remotes ()
 
 __git_merge_strategies ()
 {
-	if [ -n "$__git_merge_strategylist" ]; then
+	if [ -n "${__git_merge_strategylist-}" ]; then
 		echo "$__git_merge_strategylist"
 		return
 	fi
@@ -385,7 +385,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
@@ -403,7 +403,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
-- 
1.6.1.2.390.gba743

Re: [PATCH 2/4] completion: Use consistent if [...] convention. No test.

[Jeff King]

On Wed, Feb 11, 2009 at 01:43:11PM -0500, Ted Pavlic wrote:

> In Vim, I have "exapndtabs" set by default. I manually turned it off, but 
> during testing, I must have closed Vim and re-opened and forgot to turn it 
> back off.
>
> Some time ago, I suggested adding a vim modeline to the file to handle  
> turning expandtabs off for everyone who edits it with vim. That change  
> got denied.

FWIW, I do this in my .vimrc:

  au BufNewFile,BufRead /path/to/git/repo/* set noet sts=8 sw=8 ts=8

-Peff

Re: [PATCH 0/4] completion fixes: Acks, whitespace, and r=""

[Ted Pavlic]

In my latest "git pull," I noticed a few completion changes, but none of 
these changes were included (and thus on some of my machines, I get 
unbound variable errors again).

Were these changes finally rejected?

Thanks --
Ted

On 2/11/09 1:54 PM, Ted Pavlic wrote:
> * Added Shawn O. Pearce acks
> * Fixed whitespace problem in second patch (vim:expandtabs was on)
> * When in GIT_DIR, set r="" in __git_ps1 as well
>
> Ted Pavlic (4):
>    completion: For consistency, changed "git rev-parse" to __gitdir
>      calls.
>    completion: Use consistent if [...] convention. No test.
>    completion: Better __git_ps1 support when not in working directory
>    completion: More fixes to prevent unbound variable errors.

-- 
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 0/4] completion fixes: Acks, whitespace, and r=""

[Thomas Rast]

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

Ted Pavlic wrote:
> In my latest "git pull," I noticed a few completion changes, but none of 
> these changes were included (and thus on some of my machines, I get 
> unbound variable errors again).
> 
> Were these changes finally rejected?

$ git log --pretty=oneline --abbrev-commit -4 --author=Pavlic origin/next
5c9cc64 completion: More fixes to prevent unbound variable errors
e5dd864 completion: Better __git_ps1 support when not in working directory
ad244d2 completion: Use consistent if [...] convention, not "test"
fa26a40 completion: For consistency, change "git rev-parse" to __gitdir calls

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 0/4] completion fixes: Acks, whitespace, and r=""

[Ted Pavlic]

> $ git log --pretty=oneline --abbrev-commit -4 --author=Pavlic origin/next

Ah -- next branch. I git it.

This is all very fancy. Sorry for the spam.

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