threads / patch / 34744

patch, 3 partsRevert "bash prompt: avoid command substitution when finalizing gitstring"

Subject: [PATCH 3/3] Revert "bash prompt: avoid command substitution when finalizing gitstring"

## tl;dr

10 messages between Aug 21, 2013 and Aug 22, 2013. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Brandon Casey· Aug 21, 2013, 20:49 UTC · lore

[PATCH 1/3] git-completion.bash: use correct Bash/Zsh array length syntax

From: Brandon Casey <drafnel@gmail.com>

The syntax for retrieving the number of elements in an array is:

   ${#name[@]}

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Show changes to contrib/completion/git-completion.bash +1 −1

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 5da920e..e1b7313 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2580,7 +2580,7 @@ if [[ -n ${ZSH_VERSION-} ]]; then
 				--*=*|*.) ;;
 				*) c="$c " ;;
 				esac
-				array[$#array+1]="$c"
+				array[${#array[@]}+1]="$c"
 			done
 			compset -P '*[=:]'
 			compadd -Q -S '' -p "${2-}" -a -- array && _ret=0

-- 
1.8.4.rc0.2.g6cf5c31


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Brandon Casey· Aug 21, 2013, 20:49 UTC · re: Brandon Casey · lore

[PATCH 2/3] t9902-completion.sh: old Bash still does not support array+=('') notation

From: Brandon Casey <drafnel@gmail.com>

Old Bash (3.0) which is distributed with RHEL 4.X and other ancient platforms that are still in wide use, does not understand the array+=() notation. Let's use an explicit assignment to the new array element which works everywhere, like:

   array[${#array[@]}+1]=''

The right-hand side '' is not strictly necessary, but in this case I think it is more clear.

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
 t/t9902-completion.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Show changes to t/t9902-completion.sh +1 −1

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 272a071..2d4beb5 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -69,7 +69,7 @@ run_completion ()
 	local -a COMPREPLY _words
 	local _cword
 	_words=( $1 )
-	test "${1: -1}" = ' ' && _words+=('')
+	test "${1: -1}" = ' ' && _words[${#_words[@]}+1]=''
 	(( _cword = ${#_words[@]} - 1 ))
 	__git_wrap__git_main && print_comp
 }

-- 
1.8.4.rc0.2.g6cf5c31


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Brandon Casey· Aug 21, 2013, 20:49 UTC · re: Brandon Casey · lore

From: Brandon Casey <drafnel@gmail.com>

This reverts commit 69a8141a5d81925b7e08cb228535e9ea4a7a02e3.

Old Bash (3.0) which is distributed with RHEL 4.X and other ancient platforms that are still in wide use, does not have a printf that supports -v. Let's revert this patch and go back to using printf in the traditional way.

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
 contrib/completion/git-prompt.sh | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Show changes to contrib/completion/git-prompt.sh +1 −5

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index a81ef5a..7698ec4 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -433,11 +433,7 @@ __git_ps1 ()
 	local gitstring="$c${b##refs/heads/}${f:+$z$f}$r$p"
 
 	if [ $pcmode = yes ]; then
-		if [[ -n ${ZSH_VERSION-} ]]; then
-			gitstring=$(printf -- "$printf_format" "$gitstring")
-		else
-			printf -v gitstring -- "$printf_format" "$gitstring"
-		fi
+		gitstring=$(printf -- "$printf_format" "$gitstring")
 		PS1="$ps1pc_start$gitstring$ps1pc_end"
 	else
 		printf -- "$printf_format" "$gitstring"

-- 
1.8.4.rc0.2.g6cf5c31


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Junio C Hamano· Aug 21, 2013, 21:47 UTC · re: Brandon Casey · lore

Re: [PATCH 3/3] Revert "bash prompt: avoid command substitution when finalizing gitstring"

Brandon Casey <bcasey@nvidia.com> writes:

Show 11 quoted lines

> From: Brandon Casey <drafnel@gmail.com>
>
> This reverts commit 69a8141a5d81925b7e08cb228535e9ea4a7a02e3.
>
> Old Bash (3.0) which is distributed with RHEL 4.X and other ancient
> platforms that are still in wide use, does not have a printf that
> supports -v.  Let's revert this patch and go back to using printf
> in the traditional way.
>
> Signed-off-by: Brandon Casey <drafnel@gmail.com>
> ---

Is this something you can detect at load-time once, store the result in a private variable and then switch on it at runtime, something along the lines of...

	# on load...
	printf -v __git_printf_supports_v -- "%s" yes >/dev/null 2>&1
        
	...

	if test "${__git_printf_supports_v}" = yes
        then
		printf -v gitstring -- "$printf_format" "$gitstring"
	else
		gitstring=$(printf -- "$printf_format" "$gitstring")
        fi

Show 20 quoted lines

>  contrib/completion/git-prompt.sh | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index a81ef5a..7698ec4 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -433,11 +433,7 @@ __git_ps1 ()
>  	local gitstring="$c${b##refs/heads/}${f:+$z$f}$r$p"
>  
>  	if [ $pcmode = yes ]; then
> -		if [[ -n ${ZSH_VERSION-} ]]; then
> -			gitstring=$(printf -- "$printf_format" "$gitstring")
> -		else
> -			printf -v gitstring -- "$printf_format" "$gitstring"
> -		fi
> +		gitstring=$(printf -- "$printf_format" "$gitstring")
>  		PS1="$ps1pc_start$gitstring$ps1pc_end"
>  	else
>  		printf -- "$printf_format" "$gitstring"

Brandon Casey· Aug 21, 2013, 22:08 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] Revert "bash prompt: avoid command substitution when finalizing gitstring"

On Wed, Aug 21, 2013 at 2:47 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 30 quoted lines

> Brandon Casey <bcasey@nvidia.com> writes:
>
>> From: Brandon Casey <drafnel@gmail.com>
>>
>> This reverts commit 69a8141a5d81925b7e08cb228535e9ea4a7a02e3.
>>
>> Old Bash (3.0) which is distributed with RHEL 4.X and other ancient
>> platforms that are still in wide use, does not have a printf that
>> supports -v.  Let's revert this patch and go back to using printf
>> in the traditional way.
>>
>> Signed-off-by: Brandon Casey <drafnel@gmail.com>
>> ---
>
> Is this something you can detect at load-time once, store the result
> in a private variable and then switch on it at runtime, something
> along the lines of...
>
>
>         # on load...
>         printf -v __git_printf_supports_v -- "%s" yes >/dev/null 2>&1
>
>         ...
>
>         if test "${__git_printf_supports_v}" = yes
>         then
>                 printf -v gitstring -- "$printf_format" "$gitstring"
>         else
>                 gitstring=$(printf -- "$printf_format" "$gitstring")
>         fi

Yes, that appears to work.

-Brandon

Show 20 quoted lines

>>  contrib/completion/git-prompt.sh | 6 +-----
>>  1 file changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
>> index a81ef5a..7698ec4 100644
>> --- a/contrib/completion/git-prompt.sh
>> +++ b/contrib/completion/git-prompt.sh
>> @@ -433,11 +433,7 @@ __git_ps1 ()
>>       local gitstring="$c${b##refs/heads/}${f:+$z$f}$r$p"
>>
>>       if [ $pcmode = yes ]; then
>> -             if [[ -n ${ZSH_VERSION-} ]]; then
>> -                     gitstring=$(printf -- "$printf_format" "$gitstring")
>> -             else
>> -                     printf -v gitstring -- "$printf_format" "$gitstring"
>> -             fi
>> +             gitstring=$(printf -- "$printf_format" "$gitstring")
>>               PS1="$ps1pc_start$gitstring$ps1pc_end"
>>       else
>>               printf -- "$printf_format" "$gitstring"

Junio C Hamano· Aug 22, 2013, 00:22 UTC · re: Brandon Casey · lore

Re: [PATCH 3/3] Revert "bash prompt: avoid command substitution when finalizing gitstring"

Brandon Casey <drafnel@gmail.com> writes:

Show 33 quoted lines

> On Wed, Aug 21, 2013 at 2:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Brandon Casey <bcasey@nvidia.com> writes:
>>
>>> From: Brandon Casey <drafnel@gmail.com>
>>>
>>> This reverts commit 69a8141a5d81925b7e08cb228535e9ea4a7a02e3.
>>>
>>> Old Bash (3.0) which is distributed with RHEL 4.X and other ancient
>>> platforms that are still in wide use, does not have a printf that
>>> supports -v.  Let's revert this patch and go back to using printf
>>> in the traditional way.
>>>
>>> Signed-off-by: Brandon Casey <drafnel@gmail.com>
>>> ---
>>
>> Is this something you can detect at load-time once, store the result
>> in a private variable and then switch on it at runtime, something
>> along the lines of...
>>
>>
>>         # on load...
>>         printf -v __git_printf_supports_v -- "%s" yes >/dev/null 2>&1
>>
>>         ...
>>
>>         if test "${__git_printf_supports_v}" = yes
>>         then
>>                 printf -v gitstring -- "$printf_format" "$gitstring"
>>         else
>>                 gitstring=$(printf -- "$printf_format" "$gitstring")
>>         fi
>
> Yes, that appears to work.

A real patch needs to be a bit more careful, though. The variable needs to be cleared before all of the above, and the testing would want to consider that the variable may not be set (i.e. use "${var-}" when checking).

Thanks.

Show 23 quoted lines

> -Brandon
>
>
>>>  contrib/completion/git-prompt.sh | 6 +-----
>>>  1 file changed, 1 insertion(+), 5 deletions(-)
>>>
>>> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
>>> index a81ef5a..7698ec4 100644
>>> --- a/contrib/completion/git-prompt.sh
>>> +++ b/contrib/completion/git-prompt.sh
>>> @@ -433,11 +433,7 @@ __git_ps1 ()
>>>       local gitstring="$c${b##refs/heads/}${f:+$z$f}$r$p"
>>>
>>>       if [ $pcmode = yes ]; then
>>> -             if [[ -n ${ZSH_VERSION-} ]]; then
>>> -                     gitstring=$(printf -- "$printf_format" "$gitstring")
>>> -             else
>>> -                     printf -v gitstring -- "$printf_format" "$gitstring"
>>> -             fi
>>> +             gitstring=$(printf -- "$printf_format" "$gitstring")
>>>               PS1="$ps1pc_start$gitstring$ps1pc_end"
>>>       else
>>>               printf -- "$printf_format" "$gitstring"

Brandon Casey· Aug 22, 2013, 00:33 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] Revert "bash prompt: avoid command substitution when finalizing gitstring"

On Wed, Aug 21, 2013 at 5:22 PM, Junio C Hamano <gitster@pobox.com> wrote:

> Brandon Casey <drafnel@gmail.com> writes:
>
>> On Wed, Aug 21, 2013 at 2:47 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 16 quoted lines

>>>         # on load...
>>>         printf -v __git_printf_supports_v -- "%s" yes >/dev/null 2>&1
>>>
>>>         ...
>>>
>>>         if test "${__git_printf_supports_v}" = yes
>>>         then
>>>                 printf -v gitstring -- "$printf_format" "$gitstring"
>>>         else
>>>                 gitstring=$(printf -- "$printf_format" "$gitstring")
>>>         fi
>>
>> Yes, that appears to work.
>
> A real patch needs to be a bit more careful, though.  The variable
> needs to be cleared before all of the above,

Agreed.

> and the testing would
> want to consider that the variable may not be set (i.e. use
> "${var-}" when checking).

Why is "${var-}" necessary? Wouldn't that be equivalent to "${var}" or "$var"? We obviously wouldn't want to do 'if test $var = yes', but I would have thought it was sufficient to wrap the variable dereference in quotes as your original did.

-Brandon

Brandon Casey· Aug 22, 2013, 01:17 UTC · re: Brandon Casey · lore

[PATCH] contrib/git-prompt.sh: handle missing 'printf -v' more gracefully

From: Brandon Casey <drafnel@gmail.com>

Old Bash (3.0) which is distributed with RHEL 4.X and other ancient platforms that are still in wide use, do not have a printf that supports -v. Neither does Zsh (which is already handled in the code).

As suggested by Junio, let's test whether printf supports the -v option and store the result. Then later, we can use it to determine whether 'printf -v' can be used, or whether printf must be called in a subshell.

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---

This replaces [PATCH 3/3] Revert "bash prompt: avoid command substitution when finalizing gitstring".

This may or may not need to be updated to use "${var-}" depending on your response to my other email, but this seems sufficient.

-Brandon

 contrib/completion/git-prompt.sh | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Show changes to contrib/completion/git-prompt.sh +7 −3

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index a81ef5a..639888a 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -84,6 +84,10 @@
 # the colored output of "git status -sb" and are available only when
 # using __git_ps1 for PROMPT_COMMAND or precmd.
 
+# check whether printf supports -v
+__git_printf_supports_v=
+printf -v __git_printf_supports_v -- '%s' yes >/dev/null 2>&1
+
 # stores the divergence from upstream in $p
 # used by GIT_PS1_SHOWUPSTREAM
 __git_ps1_show_upstream ()
@@ -433,10 +437,10 @@ __git_ps1 ()
 	local gitstring="$c${b##refs/heads/}${f:+$z$f}$r$p"
 
 	if [ $pcmode = yes ]; then
-		if [[ -n ${ZSH_VERSION-} ]]; then
-			gitstring=$(printf -- "$printf_format" "$gitstring")
-		else
+		if test "$__git_printf_supports_v" = yes; then
 			printf -v gitstring -- "$printf_format" "$gitstring"
+		else
+			gitstring=$(printf -- "$printf_format" "$gitstring")
 		fi
 		PS1="$ps1pc_start$gitstring$ps1pc_end"
 	else

-- 
1.8.4.rc0.2.g6cf5c31


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Brandon Casey· Aug 22, 2013, 01:39 UTC · re: Brandon Casey · lore

[PATCH] contrib/git-prompt.sh: handle missing 'printf -v' more gracefully

From: Brandon Casey <drafnel@gmail.com>

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---

On 8/21/2013 6:27 PM, Junio C Hamano wrote:> Brandon Casey <drafnel@gmail.com> writes:

Show 5 quoted lines

>
>> Why is "${var-}" necessary?  Wouldn't that be equivalent to "${var}"
>> or "$var"?
>
> set -u

Ah. Thanks. Updated. Also minor tweak to use [ ] instead of test ... to conform with the rest of the script.

-Brandon

 contrib/completion/git-prompt.sh | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Show changes to contrib/completion/git-prompt.sh +7 −3

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index a81ef5a..ca7fb35 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -84,6 +84,10 @@
 # the colored output of "git status -sb" and are available only when
 # using __git_ps1 for PROMPT_COMMAND or precmd.
 
+# check whether printf supports -v
+__git_printf_supports_v=
+printf -v __git_printf_supports_v -- '%s' yes >/dev/null 2>&1
+
 # stores the divergence from upstream in $p
 # used by GIT_PS1_SHOWUPSTREAM
 __git_ps1_show_upstream ()
@@ -433,10 +437,10 @@ __git_ps1 ()
 	local gitstring="$c${b##refs/heads/}${f:+$z$f}$r$p"
 
 	if [ $pcmode = yes ]; then
-		if [[ -n ${ZSH_VERSION-} ]]; then
-			gitstring=$(printf -- "$printf_format" "$gitstring")
-		else
+		if [ "${__git_printf_supports_v-}" = yes ]; then
 			printf -v gitstring -- "$printf_format" "$gitstring"
+		else
+			gitstring=$(printf -- "$printf_format" "$gitstring")
 		fi
 		PS1="$ps1pc_start$gitstring$ps1pc_end"
 	else

-- 
1.8.4.rc0.2.g6cf5c31


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Junio C Hamano· Aug 22, 2013, 01:27 UTC · re: Brandon Casey · lore

Re: [PATCH 3/3] Revert "bash prompt: avoid command substitution when finalizing gitstring"

Brandon Casey <drafnel@gmail.com> writes:

> Why is "${var-}" necessary?  Wouldn't that be equivalent to "${var}"
> or "$var"?

set -u