[PATCH 0/3] completion: Convention updates and DIRTYSTATE fix

[PATCH 0/3] completion: Convention updates and DIRTYSTATE fix

[Ted Pavlic]

Three more patches to git's bash completion script.

The first two are non-critical:
* The first patch changes two "git rev-parse" calls to "__gitdir" to
  match the convention used in the rest of the script.
* The second patch changes "[...]" to "test ..." to match git
  convention. In the one case of "[...] || [...]", a "test || test" call
  is used. Alternatively, a "test ... -o ..." call could be used, but
  that might not be as readable.

The third fixes an ugly error in the new GIT_PS1_DIRTYSTATE. In order to
determine whether the branch name needs a "*" following it, the
DIRTYSTATE implementation uses a "git diff." Because "git diff" is
illegal when not in a working tree, this command gives an ugly error
when changing directory to ".git". This patch detects this case and sets
"--work-tree=..". Is there a better fix?

Ted Pavlic (3):
  completion: For consistency, changed "git rev-parse" to __gitdir
    calls.
  completion: Change "if [...]" to "if test ..." to match git
    convention
  completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is
    .git

 contrib/completion/git-completion.bash |  101 +++++++++++++++-----------------
 1 files changed, 48 insertions(+), 53 deletions(-)

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

[Ted Pavlic]

Signed-off-by: Ted Pavlic <ted@tedpavlic.com>
---
 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/3] completion: Change "if [...]" to "if test ..." to match git convention

[Ted Pavlic]

In the single case of:

    if [...] || [...]

changed to:

    if test ... || test ...

However,

    if test ... -o ...

might be favorable (although arguably less readable).

Also changed:

    if test ...
    then

to:

    if test ...; then

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6bbe09a..6772be7 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -61,15 +61,15 @@ esac
 # returns location of .git repo
 __gitdir ()
 {
-	if [ -z "${1-}" ]; then
-		if [ -n "$__git_dir" ]; then
+	if test -z "${1-}"; then
+		if test -n "$__git_dir"; then
 			echo "$__git_dir"
-		elif [ -d .git ]; then
+		elif test -d .git; then
 			echo .git
 		else
 			git rev-parse --git-dir 2>/dev/null
 		fi
-	elif [ -d "$1/.git" ]; then
+	elif test -d "$1/.git"; then
 		echo "$1/.git"
 	else
 		echo "$1"
@@ -81,42 +81,33 @@ __gitdir ()
 __git_ps1 ()
 {
 	local g="$(__gitdir)"
-	if [ -n "$g" ]; then
+	if test -n "$g"; then
 		local r
 		local b
-		if [ -d "$g/rebase-apply" ]
-		then
-			if test -f "$g/rebase-apply/rebasing"
-			then
+		if test -d "$g/rebase-apply"; then
+			if test -f "$g/rebase-apply/rebasing"; then
 				r="|REBASE"
-			elif test -f "$g/rebase-apply/applying"
-			then
+			elif test -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 test -f "$g/rebase-merge/interactive"; then
 			r="|REBASE-i"
 			b="$(cat "$g/rebase-merge/head-name")"
-		elif [ -d "$g/rebase-merge" ]
-		then
+		elif test -d "$g/rebase-merge"; then
 			r="|REBASE-m"
 			b="$(cat "$g/rebase-merge/head-name")"
-		elif [ -f "$g/MERGE_HEAD" ]
-		then
+		elif test -f "$g/MERGE_HEAD"; then
 			r="|MERGING"
 			b="$(git symbolic-ref HEAD 2>/dev/null)"
 		else
-			if [ -f "$g/BISECT_LOG" ]
-			then
+			if test -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
@@ -138,7 +129,7 @@ __git_ps1 ()
 			fi
 		fi
 
-		if [ -n "${1-}" ]; then
+		if test -n "${1-}"; then
 			printf "$1" "${b##refs/heads/}$w$i$r"
 		else
 			printf " (%s)" "${b##refs/heads/}$w$i$r"
@@ -164,7 +155,7 @@ __gitcomp_1 ()
 __gitcomp ()
 {
 	local cur="${COMP_WORDS[COMP_CWORD]}"
-	if [ $# -gt 2 ]; then
+	if test $# -gt 2; then
 		cur="$3"
 	fi
 	case "$cur" in
@@ -184,7 +175,7 @@ __gitcomp ()
 __git_heads ()
 {
 	local cmd i is_hash=y dir="$(__gitdir "${1-}")"
-	if [ -d "$dir" ]; then
+	if test -d "$dir"; then
 		git --git-dir="$dir" for-each-ref --format='%(refname:short)' \
 			refs/heads
 		return
@@ -203,7 +194,7 @@ __git_heads ()
 __git_tags ()
 {
 	local cmd i is_hash=y dir="$(__gitdir "${1-}")"
-	if [ -d "$dir" ]; then
+	if test -d "$dir"; then
 		git --git-dir="$dir" for-each-ref --format='%(refname:short)' \
 			refs/tags
 		return
@@ -223,14 +214,14 @@ __git_refs ()
 {
 	local i is_hash=y dir="$(__gitdir "${1-}")"
 	local cur="${COMP_WORDS[COMP_CWORD]}" format refs
-	if [ -d "$dir" ]; then
+	if test -d "$dir"; then
 		case "$cur" in
 		refs|refs/*)
 			format="refname"
 			refs="${cur%/*}"
 			;;
 		*)
-			if [ -e "$dir/HEAD" ]; then echo HEAD; fi
+			if test -e "$dir/HEAD"; then echo HEAD; fi
 			format="refname:short"
 			refs="refs/tags refs/heads refs/remotes"
 			;;
@@ -299,7 +290,7 @@ __git_remotes ()
 
 __git_merge_strategies ()
 {
-	if [ -n "$__git_merge_strategylist" ]; then
+	if test -n "$__git_merge_strategylist"; then
 		echo "$__git_merge_strategylist"
 		return
 	fi
@@ -385,7 +376,7 @@ __git_complete_revlist ()
 
 __git_all_commands ()
 {
-	if [ -n "$__git_all_commandlist" ]; then
+	if test -n "$__git_all_commandlist"; then
 		echo "$__git_all_commandlist"
 		return
 	fi
@@ -403,7 +394,7 @@ __git_all_commandlist="$(__git_all_commands 2>/dev/null)"
 
 __git_porcelain_commands ()
 {
-	if [ -n "$__git_porcelain_commandlist" ]; then
+	if test -n "$__git_porcelain_commandlist"; then
 		echo "$__git_porcelain_commandlist"
 		return
 	fi
@@ -511,7 +502,7 @@ __git_aliased_command ()
 	local word cmdline=$(git --git-dir="$(__gitdir)" \
 		config --get "alias.$1")
 	for word in $cmdline; do
-		if [ "${word##-*}" ]; then
+		if test "${word##-*}"; then
 			echo $word
 			return
 		fi
@@ -526,7 +517,7 @@ __git_find_subcommand ()
 	while [ $c -lt $COMP_CWORD ]; do
 		word="${COMP_WORDS[c]}"
 		for subcommand in $1; do
-			if [ "$subcommand" = "$word" ]; then
+			if test "$subcommand" = "$word"; then
 				echo "$subcommand"
 				return
 			fi
@@ -539,7 +530,7 @@ __git_has_doubledash ()
 {
 	local c=1
 	while [ $c -lt $COMP_CWORD ]; do
-		if [ "--" = "${COMP_WORDS[c]}" ]; then
+		if test "--" = "${COMP_WORDS[c]}"; then
 			return 0
 		fi
 		c=$((++c))
@@ -552,7 +543,7 @@ __git_whitespacelist="nowarn warn error error-all fix"
 _git_am ()
 {
 	local cur="${COMP_WORDS[COMP_CWORD]}" dir="$(__gitdir)"
-	if [ -d "$dir"/rebase-apply ]; then
+	if test -d "$dir"/rebase-apply; then
 		__gitcomp "--skip --resolved --abort"
 		return
 	fi
@@ -636,7 +627,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 test -z "$subcommand"; then
 		__gitcomp "$subcommands"
 		return
 	fi
@@ -672,7 +663,7 @@ _git_branch ()
 			"
 		;;
 	*)
-		if [ $only_local_ref = "y" -a $has_r = "n" ]; then
+		if test $only_local_ref = "y" -a $has_r = "n"; then
 			__gitcomp "$(__git_heads)"
 		else
 			__gitcomp "$(__git_refs)"
@@ -830,7 +821,7 @@ _git_fetch ()
 {
 	local cur="${COMP_WORDS[COMP_CWORD]}"
 
-	if [ "$COMP_CWORD" = 2 ]; then
+	if test "$COMP_CWORD" = 2; then
 		__gitcomp "$(__git_remotes)"
 	else
 		case "$cur" in
@@ -1088,7 +1079,7 @@ _git_pull ()
 {
 	local cur="${COMP_WORDS[COMP_CWORD]}"
 
-	if [ "$COMP_CWORD" = 2 ]; then
+	if test "$COMP_CWORD" = 2; then
 		__gitcomp "$(__git_remotes)"
 	else
 		__gitcomp "$(__git_refs "${COMP_WORDS[2]}")"
@@ -1099,7 +1090,7 @@ _git_push ()
 {
 	local cur="${COMP_WORDS[COMP_CWORD]}"
 
-	if [ "$COMP_CWORD" = 2 ]; then
+	if test "$COMP_CWORD" = 2; then
 		__gitcomp "$(__git_remotes)"
 	else
 		case "$cur" in
@@ -1125,7 +1116,7 @@ _git_push ()
 _git_rebase ()
 {
 	local cur="${COMP_WORDS[COMP_CWORD]}" dir="$(__gitdir)"
-	if [ -d "$dir"/rebase-apply ] || [ -d "$dir"/rebase-merge ]; then
+	if test -d "$dir"/rebase-apply || test -d "$dir"/rebase-merge; then
 		__gitcomp "--continue --skip --abort"
 		return
 	fi
@@ -1416,7 +1407,7 @@ _git_remote ()
 {
 	local subcommands="add rename rm show prune update"
 	local subcommand="$(__git_find_subcommand "$subcommands")"
-	if [ -z "$subcommand" ]; then
+	if test -z "$subcommand"; then
 		__gitcomp "$subcommands"
 		return
 	fi
@@ -1546,7 +1537,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 test -z "$subcommand"; then
 		__gitcomp "$subcommands"
 	else
 		local cur="${COMP_WORDS[COMP_CWORD]}"
@@ -1576,7 +1567,7 @@ _git_submodule ()
 	__git_has_doubledash && return
 
 	local subcommands="add status init update summary foreach sync"
-	if [ -z "$(__git_find_subcommand "$subcommands")" ]; then
+	if test -z "$(__git_find_subcommand "$subcommands")"; then
 		local cur="${COMP_WORDS[COMP_CWORD]}"
 		case "$cur" in
 		--*)
@@ -1598,7 +1589,7 @@ _git_svn ()
 		proplist show-ignore show-externals
 		"
 	local subcommand="$(__git_find_subcommand "$subcommands")"
-	if [ -z "$subcommand" ]; then
+	if test -z "$subcommand"; then
 		__gitcomp "$subcommands"
 	else
 		local remote_opts="--username= --config-dir= --no-auth-cache"
@@ -1690,7 +1681,7 @@ _git_tag ()
 		COMPREPLY=()
 		;;
 	-*|tag)
-		if [ $f = 1 ]; then
+		if test $f = 1; then
 			__gitcomp "$(__git_tags)"
 		else
 			COMPREPLY=()
@@ -1718,7 +1709,7 @@ _git ()
 		c=$((++c))
 	done
 
-	if [ -z "$command" ]; then
+	if test -z "$command"; then
 		case "${COMP_WORDS[COMP_CWORD]}" in
 		--*)   __gitcomp "
 			--paginate
@@ -1799,7 +1790,7 @@ _gitk ()
 	local cur="${COMP_WORDS[COMP_CWORD]}"
 	local g="$(__gitdir)"
 	local merge=""
-	if [ -f $g/MERGE_HEAD ]; then
+	if test -f $g/MERGE_HEAD; then
 		merge="--merge"
 	fi
 	case "$cur" in
@@ -1820,7 +1811,7 @@ complete -o bashdefault -o default -o nospace -F _gitk gitk 2>/dev/null \
 # when the user has tab-completed the executable name and consequently
 # included the '.exe' suffix.
 #
-if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then
+if test Cygwin = "$(uname -o 2>/dev/null)"; then
 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.2.390.gba743

[PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git

[Ted Pavlic]

The GIT_PS1_DIRTYSTATE support uses a "git diff" to see if a "*" should
be placed after the branch name. The "git diff" fails with an ugly error
if the user has just changed directory into GIT_DIR.

This patch uses "git rev-parse --is-inside-work-tree" to determine
whether a "--work-tree=.." should be added to the "git diff".

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6772be7..ffde82a 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -115,10 +115,14 @@ __git_ps1 ()
 
 		local w
 		local i
+		local worktreestr
 
 		if test -n "${GIT_PS1_SHOWDIRTYSTATE-}"; then
 			if test "$(git config --bool bash.showDirtyState)" != "false"; then
-				git diff --no-ext-diff --ignore-submodules \
+				if test "false" = "$(git rev-parse --is-inside-work-tree 2>/dev/null)"; then
+					worktreestr="--work-tree=.."
+				fi
+				git ${worktreestr} 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 \
-- 
1.6.1.2.390.gba743

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

[Ted Pavlic]

Several functions make use of "test -n" and "test -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>
---

This patch depends on:

    <1234366634-17900-3-git-send-email-ted@tedpavlic.com>
    "[PATCH 2/3] completion: Change "if [...]" to "if test ..." to 
                             match git convention"

If that patch is not applied, I can submit a new patch.

 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 ffde82a..055e4ac 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -62,7 +62,7 @@ esac
 __gitdir ()
 {
 	if test -z "${1-}"; then
-		if test -n "$__git_dir"; then
+		if test -n "${__git_dir-}"; then
 			echo "$__git_dir"
 		elif test -d .git; then
 			echo .git
@@ -294,7 +294,7 @@ __git_remotes ()
 
 __git_merge_strategies ()
 {
-	if test -n "$__git_merge_strategylist"; then
+	if test -n "${__git_merge_strategylist-}"; then
 		echo "$__git_merge_strategylist"
 		return
 	fi
@@ -380,7 +380,7 @@ __git_complete_revlist ()
 
 __git_all_commands ()
 {
-	if test -n "$__git_all_commandlist"; then
+	if test -n "${__git_all_commandlist-}"; then
 		echo "$__git_all_commandlist"
 		return
 	fi
@@ -398,7 +398,7 @@ __git_all_commandlist="$(__git_all_commands 2>/dev/null)"
 
 __git_porcelain_commands ()
 {
-	if test -n "$__git_porcelain_commandlist"; then
+	if test -n "${__git_porcelain_commandlist-}"; then
 		echo "$__git_porcelain_commandlist"
 		return
 	fi
-- 
1.6.1.2.390.gba743

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

[Shawn O. Pearce]

Ted Pavlic <ted@tedpavlic.com> wrote:
> 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
> 

-- 
Shawn.

Re: [PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention

[Shawn O. Pearce]

Ted Pavlic <ted@tedpavlic.com> wrote:
> In the single case of:
> 
>     if [...] || [...]
> 
> changed to:
> 
>     if test ... || test ...

NAK.

This script only runs in bash.  bash supports [...].  The
prevailing convention in the script is to use [...].  Only
4 tests inside of __git_ps1 use "test", the rest of the code
is using [...].

I would agree to a test->[...] conversion patch as its fairly small,
but not this one.  Too large, too much code churn, no benefit.
 
-- 
Shawn.

Re: [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git

[Shawn O. Pearce]

Ted Pavlic <ted@tedpavlic.com> wrote:
> The GIT_PS1_DIRTYSTATE support uses a "git diff" to see if a "*" should
> be placed after the branch name. The "git diff" fails with an ugly error
> if the user has just changed directory into GIT_DIR.
> 
> This patch uses "git rev-parse --is-inside-work-tree" to determine
> whether a "--work-tree=.." should be added to the "git diff".

I think it makes more sense to just drop the work tree stuff from
the prompt if we aren't inside the work tree anymore.  Meaning,
we should behave as though bash.showDirtyState is false.

 
> Signed-off-by: Ted Pavlic <ted@tedpavlic.com>
> ---
>  contrib/completion/git-completion.bash |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 6772be7..ffde82a 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -115,10 +115,14 @@ __git_ps1 ()
>  
>  		local w
>  		local i
> +		local worktreestr
>  
>  		if test -n "${GIT_PS1_SHOWDIRTYSTATE-}"; then
>  			if test "$(git config --bool bash.showDirtyState)" != "false"; then
> -				git diff --no-ext-diff --ignore-submodules \
> +				if test "false" = "$(git rev-parse --is-inside-work-tree 2>/dev/null)"; then
> +					worktreestr="--work-tree=.."
> +				fi
> +				git ${worktreestr} 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 \
> -- 
> 1.6.1.2.390.gba743
> 

-- 
Shawn.

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

[Shawn O. Pearce]

Ted Pavlic <ted@tedpavlic.com> wrote:
> Several functions make use of "test -n" and "test -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>
> ---
> 
> This patch depends on:
> 
>     <1234366634-17900-3-git-send-email-ted@tedpavlic.com>
>     "[PATCH 2/3] completion: Change "if [...]" to "if test ..." to 
>                              match git convention"
> 
> If that patch is not applied, I can submit a new patch.

Looks OK to me, but I NAK'd the dependency, so you'll have to rebase
it without the dependency in there.  Or talk me into why that much
churn is a good thing.

 
>  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 ffde82a..055e4ac 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -62,7 +62,7 @@ esac
>  __gitdir ()
>  {
>  	if test -z "${1-}"; then
> -		if test -n "$__git_dir"; then
> +		if test -n "${__git_dir-}"; then
>  			echo "$__git_dir"
>  		elif test -d .git; then
>  			echo .git
> @@ -294,7 +294,7 @@ __git_remotes ()
>  
>  __git_merge_strategies ()
>  {
> -	if test -n "$__git_merge_strategylist"; then
> +	if test -n "${__git_merge_strategylist-}"; then
>  		echo "$__git_merge_strategylist"
>  		return
>  	fi
> @@ -380,7 +380,7 @@ __git_complete_revlist ()
>  
>  __git_all_commands ()
>  {
> -	if test -n "$__git_all_commandlist"; then
> +	if test -n "${__git_all_commandlist-}"; then
>  		echo "$__git_all_commandlist"
>  		return
>  	fi
> @@ -398,7 +398,7 @@ __git_all_commandlist="$(__git_all_commands 2>/dev/null)"
>  
>  __git_porcelain_commands ()
>  {
> -	if test -n "$__git_porcelain_commandlist"; then
> +	if test -n "${__git_porcelain_commandlist-}"; then
>  		echo "$__git_porcelain_commandlist"
>  		return
>  	fi
> -- 
> 1.6.1.2.390.gba743
> 

-- 
Shawn.

Re: [PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention

[Ted Pavlic]

> NAK.
>
> This script only runs in bash.  bash supports [...].  The
> prevailing convention in the script is to use [...].  Only
> 4 tests inside of __git_ps1 use "test", the rest of the code
> is using [...].

So this trumps Documentation/CodingGuidelines, which says:

  - We prefer "test" over "[ ... ]".

?

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.

Re: [PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention

[Junio C Hamano]

Ted Pavlic <ted@tedpavlic.com> writes:

> In the single case of:
>
>     if [...] || [...]
>
> changed to:
>
>     if test ... || test ...

In [0/3] you talked about "git convention", but please match the local
convention, especially inside contrib/ area.  That is, consistency of the
style within the same file (and files in vicinity), is more important.

> Also changed:
>
>     if test ...
>     then
>
> to:
>
>     if test ...; then

The prevailing style in bash completion script is to write "then" on the
same line as "if", so I think this is a good example of matching the local
convention (if you are trying to match "git convention", "then" is written
on the same column as "if" on a line by itself for readability, so this
change is going backwards).

Re: [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git

[Ted Pavlic]

>> This patch uses "git rev-parse --is-inside-work-tree" to determine
>> whether a "--work-tree=.." should be added to the "git diff".
>
> I think it makes more sense to just drop the work tree stuff from
> the prompt if we aren't inside the work tree anymore.  Meaning,
> we should behave as though bash.showDirtyState is false.

I see.

At first, it seemed like it would be useful to know if the working 
directory was dirty even when you're within .git. However, I guess 
that's problematic when your working tree is in some completely 
unpredictable location. It's probably a bad idea to assume that work-tree=..

So you're right... there's a logical problem with having showDirtyState 
turned on when within .git. It should be disabled there.

So I'll check for "git rev-parse --is-inside-git-dir" and disable 
showDirtyState appropriately.

--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 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git

[Junio C Hamano]

Ted Pavlic <ted@tedpavlic.com> writes:

> The GIT_PS1_DIRTYSTATE support uses a "git diff" to see if a "*" should
> be placed after the branch name. The "git diff" fails with an ugly error
> if the user has just changed directory into GIT_DIR.
>
> This patch uses "git rev-parse --is-inside-work-tree" to determine
> whether a "--work-tree=.." should be added to the "git diff".

Why ".."?  What prevents you from "cd .git/refs/heads"?

Your "is-inside-work-tree" might be a good change, but if you were to
spend a letter to notify the users, "Warning: You are inside GIT_DIR! This
is something unusual, proceed with caution." is a lot more important
notice to give them than "You seem to have unstaged changes" notice.

You have at least three possible states:

 * You are not in git repository at all;

 * You are somewhere in $GIT_DIR, perhaps in a bare repository, perhaps a
   repository with a work tree.

 * You are inside a work tree.

The first should be quiet, the second should say "Proceed with caution,
any 'rm -f file' or 'edit file' you do here should be to recover from
unusual repository corruptoin only; you are welcome to look but don't
touch.", and the last one is Ok to say "You have unstaged changes."

Re: [PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention

[Junio C Hamano]

Ted Pavlic <ted@tedpavlic.com> writes:

>> NAK.
>>
>> This script only runs in bash.  bash supports [...].  The
>> prevailing convention in the script is to use [...].  Only
>> 4 tests inside of __git_ps1 use "test", the rest of the code
>> is using [...].
>
> So this trumps Documentation/CodingGuidelines, which says:
>
>  - We prefer "test" over "[ ... ]".
>
> ?

No, this paragraph from Documentation/CodingGuidelines trumps you.

    As for more concrete guidelines, just imitate the existing code
    (this is a good guideline, no matter which project you are
    contributing to). It is always preferable to match the _local_
    convention. New code added to git suite is expected to match
    the overall style of existing code. Modifications to existing
    code is expected to match the style the surrounding code already
    uses (even if it doesn't match the overall style of existing code).

Re: [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git

[Ted Pavlic]

>> This patch uses "git rev-parse --is-inside-work-tree" to determine
>> whether a "--work-tree=.." should be added to the "git diff".
>
> Why ".."?  What prevents you from "cd .git/refs/heads"?

It was my impression that --work-tree=.. was relative to the GIT_DIR. My 
tests seem to confirm that. Within the git work tree I do:

	echo "test">>COPYING

then

	cd .git/refs/heads
	git --work=tree=.. diff

and that produces a diff of COPYING showing the new line.

("man git" confused me about how WORK_TREE was supposed to work)

> Your "is-inside-work-tree" might be a good change, but if you were to
> spend a letter to notify the users, "Warning: You are inside GIT_DIR! This
> is something unusual, proceed with caution." is a lot more important
> notice to give them than "You seem to have unstaged changes" notice.

Ok, so if "is-inside-git-dir" then send a special "!" flag (and no 
dirtyState flags)? Does that seem reasonable?

Additionally, is it a good idea to echo the branch name when inside the 
git dir? That is, what does it "mean" to be on "master" when you're 
inside .git?

> You have at least three possible states:
>   * You are not in git repository at all;
>   * You are somewhere in $GIT_DIR, perhaps in a bare repository, perhaps a
>     repository with a work tree.
>   * You are inside a work tree.

It seems like (psuedocode)...

	if git rev-parse --is-inside-git-dir; then
		use '!' flag to indicate caution

	elif git rev-parse --is-inside-work-tree; then
		proceed as before (with '*' and '+' flags)

	else
		do nothing

I think that handles those cases. No?

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.

Re: [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git

[Shawn O. Pearce]

Ted Pavlic <ted@tedpavlic.com> wrote:
>
>> You have at least three possible states:
>>   * You are not in git repository at all;
>>   * You are somewhere in $GIT_DIR, perhaps in a bare repository, perhaps a
>>     repository with a work tree.
>>   * You are inside a work tree.
>
> It seems like (psuedocode)...
>
> 	if git rev-parse --is-inside-git-dir; then
> 		use '!' flag to indicate caution
>
> 	elif git rev-parse --is-inside-work-tree; then
> 		proceed as before (with '*' and '+' flags)
>
> 	else
> 		do nothing
>
> I think that handles those cases. No?

Yes, that looks right to me.

-- 
Shawn.