threads / patch / 27276

patcht1507: change quoting in test_did_you_mean to a more general one

Subject: [PATCH] t1507: change quoting in test_did_you_mean to a more general one

## tl;dr

12 messages between May 5, 2011 and May 9, 2011. Diffs are folded; open one to read it.

replies: 11people: 4as markdown or json

Kacper Kornet· May 5, 2011, 19:10 UTC · lore
In bash and some other shells the script:
x=2; unset a; echo "${a:-'$x'}"

prints '2'. However ksh shell prints $x. The quoting is added to reproduce bash behaviour.

Signed-off-by: Kacper Kornet <draenog@pld-linux.org>
---
 t/t1506-rev-parse-diagnosis.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to t/t1506-rev-parse-diagnosis.sh +2 −2
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index 4a6396f..bad09f9 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -8,8 +8,8 @@ exec </dev/null
 
 test_did_you_mean ()
 {
-	printf "fatal: Path '$2$3' $4, but not ${5:-'$3'}.\n" >expected &&
-	printf "Did you mean '$1:$2$3'${2:+ aka '$1:./$3'}?\n" >>expected &&
+	printf "fatal: Path '$2$3' $4, but not ${5:-\'$3\'}.\n" >expected &&
+	printf "Did you mean '$1:$2$3'${2:+ aka \'$1:./$3\'}?\n" >>expected &&
 	test_cmp expected error
 }
 
-- 
1.7.5

-- 
  Kacper Kornet
Junio C Hamano· May 5, 2011, 20:02 UTC · re: Kacper Kornet · lore

Re: [PATCH] t1507: change quoting in test_did_you_mean to a more general one

Kacper Kornet <draenog@pld-linux.org> writes:
Show 6 quoted lines
> In bash and some other shells the script:
>
>   x=2; unset a; echo "${a:-'$x'}"
>
> prints '2'. However ksh shell prints $x. The quoting is added to
> reproduce bash behaviour.
What I happen to have in /usr/bin/ksh
    $ /usr/bin/ksh --version
      version         sh (AT&T Research) 93t+ 2009-05-01
does not seem to have this issue.
Whose ksh is this?  It is broken.

POSIX "2.6.2 Parameter Expansion" [*1*] says in ${parameter<op>word}, "word shall be subjected to tilde expansion, parameter expansion, command substitution, and arithmetic expansion", when "a value of word is needed" based on the state of parameter.

I am not opposed to the change, but because it is not "change to more general one" but "work around a bug in <this implementation of ksh>", and I would like to know what to fill in the blank when I rewrite the proposed commit log message.

Show 22 quoted lines
> Signed-off-by: Kacper Kornet <draenog@pld-linux.org>
> ---
>  t/t1506-rev-parse-diagnosis.sh |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
> index 4a6396f..bad09f9 100755
> --- a/t/t1506-rev-parse-diagnosis.sh
> +++ b/t/t1506-rev-parse-diagnosis.sh
> @@ -8,8 +8,8 @@ exec </dev/null
>  
>  test_did_you_mean ()
>  {
> -	printf "fatal: Path '$2$3' $4, but not ${5:-'$3'}.\n" >expected &&
> -	printf "Did you mean '$1:$2$3'${2:+ aka '$1:./$3'}?\n" >>expected &&
> +	printf "fatal: Path '$2$3' $4, but not ${5:-\'$3\'}.\n" >expected &&
> +	printf "Did you mean '$1:$2$3'${2:+ aka \'$1:./$3\'}?\n" >>expected &&
>  	test_cmp expected error
>  }
>  
> -- 
> 1.7.5
[Reference]
*1* http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02
Michael J Gruber· May 6, 2011, 07:43 UTC · re: Junio C Hamano · lore

Re: [PATCH] t1507: change quoting in test_did_you_mean to a more general one

Junio C Hamano venit, vidit, dixit 05.05.2011 22:02:
Show 27 quoted lines
> Kacper Kornet <draenog@pld-linux.org> writes:
> 
>> In bash and some other shells the script:
>>
>>   x=2; unset a; echo "${a:-'$x'}"
>>
>> prints '2'. However ksh shell prints $x. The quoting is added to
>> reproduce bash behaviour.
> 
> What I happen to have in /usr/bin/ksh
> 
>     $ /usr/bin/ksh --version
>       version         sh (AT&T Research) 93t+ 2009-05-01
> 
> does not seem to have this issue.
> 
> Whose ksh is this?  It is broken.
> 
> POSIX "2.6.2 Parameter Expansion" [*1*] says in ${parameter<op>word},
> "word shall be subjected to tilde expansion, parameter expansion, command
> substitution, and arithmetic expansion", when "a value of word is needed"
> based on the state of parameter.
> 
> I am not opposed to the change, but because it is not "change to more
> general one" but "work around a bug in <this implementation of ksh>", and
> I would like to know what to fill in the blank when I rewrite the proposed
> commit log message.

I'm running all tests with "dash" these days, and I'm pretty sure "these days" started before 34df9ab. Anyways, this test passes with dash. I'd be happy to replace my test shell with an even stricter one if needed.

Show 26 quoted lines
>> Signed-off-by: Kacper Kornet <draenog@pld-linux.org>
>> ---
>>  t/t1506-rev-parse-diagnosis.sh |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
>> index 4a6396f..bad09f9 100755
>> --- a/t/t1506-rev-parse-diagnosis.sh
>> +++ b/t/t1506-rev-parse-diagnosis.sh
>> @@ -8,8 +8,8 @@ exec </dev/null
>>  
>>  test_did_you_mean ()
>>  {
>> -	printf "fatal: Path '$2$3' $4, but not ${5:-'$3'}.\n" >expected &&
>> -	printf "Did you mean '$1:$2$3'${2:+ aka '$1:./$3'}?\n" >>expected &&
>> +	printf "fatal: Path '$2$3' $4, but not ${5:-\'$3\'}.\n" >expected &&
>> +	printf "Did you mean '$1:$2$3'${2:+ aka \'$1:./$3\'}?\n" >>expected &&
>>  	test_cmp expected error
>>  }
>>  
>> -- 
>> 1.7.5
> 
> [Reference]
> 
> *1* http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02

Other than that, I have no objections if this patch makes more shells happy and no happy ones unhappy.

Is your ksh OK with all other tests?
Michael
Kacper Kornet· May 6, 2011, 08:51 UTC · re: Michael J Gruber · lore

Re: [PATCH] t1507: change quoting in test_did_you_mean to a more general one

On Fri, May 06, 2011 at 09:43:01AM +0200, Michael J Gruber wrote:
Show 6 quoted lines
> Junio C Hamano venit, vidit, dixit 05.05.2011 22:02:
> > Kacper Kornet <draenog@pld-linux.org> writes:
> >> Signed-off-by: Kacper Kornet <draenog@pld-linux.org>
> >> ---
> >>  t/t1506-rev-parse-diagnosis.sh |    4 ++--
> >>  1 files changed, 2 insertions(+), 2 deletions(-)
Show 5 quoted lines
> >> diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
> >> index 4a6396f..bad09f9 100755
> >> --- a/t/t1506-rev-parse-diagnosis.sh
> >> +++ b/t/t1506-rev-parse-diagnosis.sh
> >> @@ -8,8 +8,8 @@ exec </dev/null
Show 8 quoted lines
> >>  test_did_you_mean ()
> >>  {
> >> -	printf "fatal: Path '$2$3' $4, but not ${5:-'$3'}.\n" >expected &&
> >> -	printf "Did you mean '$1:$2$3'${2:+ aka '$1:./$3'}?\n" >>expected &&
> >> +	printf "fatal: Path '$2$3' $4, but not ${5:-\'$3\'}.\n" >expected &&
> >> +	printf "Did you mean '$1:$2$3'${2:+ aka \'$1:./$3\'}?\n" >>expected &&
> >>  	test_cmp expected error
> >>  }
> >> -- 
> >> 1.7.5
> > [Reference]
> > *1* http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02
> Other than that, I have no objections if this patch makes more shells
> happy and no happy ones unhappy.
> Is your ksh OK with all other tests?

Yes. The only other patch which is applied during our building process is:

diff -ur git-1.7.0.3.orig/t/t1304-default-acl.sh git-1.7.0.3/t/t1304-default-acl .sh --- git-1.7.0.3.orig/t/t1304-default-acl.sh 2010-03-22 01:35:03.000000000 +0 000 +++ git-1.7.0.3/t/t1304-default-acl.sh 2010-03-23 19:53:49.069813289 +0000

Show changes to diff +2 −0
@@ -9,6 +9,8 @@
 # => this must come before . ./test-lib.sh
 umask 077

+LOGNAME=$(whoami)
+
 . ./test-lib.sh

 # We need an arbitrary other user give permission to using ACLs. root


But it is specific to our build environment.
-- 
  Kacper Kornet
Michael J Gruber· May 6, 2011, 09:14 UTC · re: Kacper Kornet · lore

Re: [PATCH] t1507: change quoting in test_did_you_mean to a more general one

Kacper Kornet venit, vidit, dixit 06.05.2011 10:51:
Show 59 quoted lines
> On Fri, May 06, 2011 at 09:43:01AM +0200, Michael J Gruber wrote:
>> Junio C Hamano venit, vidit, dixit 05.05.2011 22:02:
>>> Kacper Kornet <draenog@pld-linux.org> writes:
>>>> Signed-off-by: Kacper Kornet <draenog@pld-linux.org>
>>>> ---
>>>>  t/t1506-rev-parse-diagnosis.sh |    4 ++--
>>>>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
>>>> diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
>>>> index 4a6396f..bad09f9 100755
>>>> --- a/t/t1506-rev-parse-diagnosis.sh
>>>> +++ b/t/t1506-rev-parse-diagnosis.sh
>>>> @@ -8,8 +8,8 @@ exec </dev/null
> 
>>>>  test_did_you_mean ()
>>>>  {
>>>> -	printf "fatal: Path '$2$3' $4, but not ${5:-'$3'}.\n" >expected &&
>>>> -	printf "Did you mean '$1:$2$3'${2:+ aka '$1:./$3'}?\n" >>expected &&
>>>> +	printf "fatal: Path '$2$3' $4, but not ${5:-\'$3\'}.\n" >expected &&
>>>> +	printf "Did you mean '$1:$2$3'${2:+ aka \'$1:./$3\'}?\n" >>expected &&
>>>>  	test_cmp expected error
>>>>  }
> 
>>>> -- 
>>>> 1.7.5
> 
>>> [Reference]
> 
>>> *1* http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02
> 
>> Other than that, I have no objections if this patch makes more shells
>> happy and no happy ones unhappy.
> 
>> Is your ksh OK with all other tests?
> 
> Yes. The only other patch which is applied during our building process
> is: 
> 
> diff -ur git-1.7.0.3.orig/t/t1304-default-acl.sh
> git-1.7.0.3/t/t1304-default-acl
> .sh
> --- git-1.7.0.3.orig/t/t1304-default-acl.sh     2010-03-22
> 01:35:03.000000000 +0
> 000
> +++ git-1.7.0.3/t/t1304-default-acl.sh  2010-03-23 19:53:49.069813289
> +0000
> @@ -9,6 +9,8 @@
>  # => this must come before . ./test-lib.sh
>  umask 077
> 
> +LOGNAME=$(whoami)
> +
>  . ./test-lib.sh
> 
>  # We need an arbitrary other user give permission to using ACLs. root
> 
> 
> But it is specific to our build environment.
> 
Thanks for the info (and for responding despite my botched to/cc).

In a different context I noticed that systems don't agree wrt. the presence of USER/USERNAME/LOGNAME in the environment. We use LOGNAME in t1304 only (and never USER nor USERNAME), but still we might want to do something like

LOGNAME=${LOGNAME:-$USERNAME} LOGNAME=${LOGNAME:-$USER} LOGNAME=${LOGNAME:-$(whoami)}

there, or simpler with ${LOGNAME:=...}, although I don't know what to take for granted.

Michael
Johannes Sixt· May 6, 2011, 10:22 UTC · re: Michael J Gruber · lore

Re: [PATCH] t1507: change quoting in test_did_you_mean to a more general one

Am 5/6/2011 9:43, schrieb Michael J Gruber:
Show 10 quoted lines
> Junio C Hamano venit, vidit, dixit 05.05.2011 22:02:
>> Kacper Kornet <draenog@pld-linux.org> writes:
>>> -	printf "fatal: Path '$2$3' $4, but not ${5:-'$3'}.\n" >expected &&
>>> -	printf "Did you mean '$1:$2$3'${2:+ aka '$1:./$3'}?\n" >>expected &&
>>> +	printf "fatal: Path '$2$3' $4, but not ${5:-\'$3\'}.\n" >expected &&
>>> +	printf "Did you mean '$1:$2$3'${2:+ aka \'$1:./$3\'}?\n" >>expected &&
> Other than that, I have no objections if this patch makes more shells
> happy and no happy ones unhappy.
> 
> Is your ksh OK with all other tests?
Note that:
- With the proposed change, bash now prints the backslashes.
- The printfs should be echos, really.
- The behavior of quoting at the right of :- when the ${...:-...} exansion
appears in double-quotes was debated recently at length at the Austin
group (which revises the POSIX standard). You better move the expansions
to assignments of temporary variables, where you don't need the
surrounding double-quotes:
	butnot=${5:-\'$3\'} aka=${2:+ aka \'$1:./$3\'}
Here, the backslash unambiguously quotes the next character.
-- Hannes
Michael J Gruber· May 6, 2011, 11:10 UTC · re: Johannes Sixt · lore

Re: [PATCH] t1507: change quoting in test_did_you_mean to a more general one

Johannes Sixt venit, vidit, dixit 06.05.2011 12:22:
Show 17 quoted lines
> Am 5/6/2011 9:43, schrieb Michael J Gruber:
>> Junio C Hamano venit, vidit, dixit 05.05.2011 22:02:
>>> Kacper Kornet <draenog@pld-linux.org> writes:
>>>> -	printf "fatal: Path '$2$3' $4, but not ${5:-'$3'}.\n" >expected &&
>>>> -	printf "Did you mean '$1:$2$3'${2:+ aka '$1:./$3'}?\n" >>expected &&
>>>> +	printf "fatal: Path '$2$3' $4, but not ${5:-\'$3\'}.\n" >expected &&
>>>> +	printf "Did you mean '$1:$2$3'${2:+ aka \'$1:./$3\'}?\n" >>expected &&
>> Other than that, I have no objections if this patch makes more shells
>> happy and no happy ones unhappy.
>>
>> Is your ksh OK with all other tests?
> 
> Note that:
> 
> - With the proposed change, bash now prints the backslashes.
> 
> - The printfs should be echos, really.

I thought printfs can be relied upon better than echos (in terms of line endings)?

Show 11 quoted lines
> - The behavior of quoting at the right of :- when the ${...:-...} exansion
> appears in double-quotes was debated recently at length at the Austin
> group (which revises the POSIX standard). You better move the expansions
> to assignments of temporary variables, where you don't need the
> surrounding double-quotes:
> 
> 	butnot=${5:-\'$3\'} aka=${2:+ aka \'$1:./$3\'}
> 
> Here, the backslash unambiguously quotes the next character.
> 
> -- Hannes
Whatever makes most shells happy, as long as that includes current bash...
Michael
Junio C Hamano· May 6, 2011, 14:23 UTC · re: Johannes Sixt · lore

Re: [PATCH] t1507: change quoting in test_did_you_mean to a more general one

Johannes Sixt <j.sixt@viscovery.net> writes:
> - The behavior of quoting at the right of :- when the ${...:-...} exansion
> appears in double-quotes was debated recently at length at the Austin
> group (which revises the POSIX standard).
Nice to have somebody who follows that stuff on the list.
Junio C Hamano· May 9, 2011, 04:40 UTC · re: Johannes Sixt · lore

Re: [PATCH] t1507: change quoting in test_did_you_mean to a more general one

Johannes Sixt <j.sixt@viscovery.net> writes:
> - The behavior of quoting at the right of :- when the ${...:-...} exansion
> appears in double-quotes was debated recently at length at the Austin
> group (which revises the POSIX standard). You better move the expansions
> to assignments of temporary variables,...

What alternative semantics do Austin folks have in mind, by the way? Just declare this undefined?

Anyway, let's do this as a future-proofing.  How does this look?
-- >8 --
Subject: t1507: avoid "${parameter<op>'word'}" inside double-quotes

Kacper Kornet noticed that a $variable in "word" in the above construct is not substituted by his pdksh. Modern POSIX compliant shells (e.g. dash, ksh, bash) all seem to interpret POSIX "2.6.2 Parameter Expansion" that says "word shall be subjected to tilde expansion, parameter expansion, command substitution, and arithmetic expansion" in ${parameter<op>word}, to mean that the word is expanded as if it appeared in dq pairs, so if the word were "'$variable'" (sans dq) it would expand to a single quote, the value of the $variable and then a single quote.

Johannes Sixt reports that the behavior of quoting at the right of :- when the ${...:-...} expansion appears in double-quotes was debated recently at length at the Austin group. We can avoid this issue and future-proof the test by a slight rewrite.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t1506-rev-parse-diagnosis.sh |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)
Show changes to t/t1506-rev-parse-diagnosis.sh +5 −2
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index 58575d6..a1081a8 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -8,8 +8,11 @@ exec </dev/null
 
 test_did_you_mean ()
 {
-	printf "fatal: Path '$2$3' $4, but not ${5:-'$3'}.\n" >expected &&
-	printf "Did you mean '$1:$2$3'${2:+ aka '$1:./$3'}?\n" >>expected &&
+	sq="'"
+	cat >expected <<-EOF &&
+	fatal: Path '$2$3' $4, but not ${5:-$sq$3$sq}.
+	Did you mean '$1:$2$3'${2:+ aka $sq$1:./$3$sq}?
+	EOF
 	test_cmp expected error
 }
 
Johannes Sixt· May 9, 2011, 06:39 UTC · re: Junio C Hamano · lore

Re: [PATCH] t1507: change quoting in test_did_you_mean to a more general one

Am 5/9/2011 6:40, schrieb Junio C Hamano:
Show 9 quoted lines
> Johannes Sixt <j.sixt@viscovery.net> writes:
> 
>> - The behavior of quoting at the right of :- when the ${...:-...} exansion
>> appears in double-quotes was debated recently at length at the Austin
>> group (which revises the POSIX standard). You better move the expansions
>> to assignments of temporary variables,...
> 
> What alternative semantics do Austin folks have in mind, by the way?  Just
> declare this undefined?

Most of the debate centered around how to quote a closing brace: the problem is that usually the backslash within double-quotes is only special when followed by $, `, or \, therefore, it should not be possible to quote a closing brace using \}. It turned out that different shells handled this particular case differently. IIRC, there were also discrepancies whether whitespace generated on the RHS of :- (or other operators) was retained or discarded. The conclusion for us as application developers is not to depend on too many subtle details in such a variable expansions because the specification is too vague, shells implement it differently, and some even get it outright wrong (as the case discovered by Kascper).

> -	printf "fatal: Path '$2$3' $4, but not ${5:-'$3'}.\n" >expected &&
> -	printf "Did you mean '$1:$2$3'${2:+ aka '$1:./$3'}?\n" >>expected &&
> +	sq="'"
This cuts the && chain.
> +	cat >expected <<-EOF &&
> +	fatal: Path '$2$3' $4, but not ${5:-$sq$3$sq}.
> +	Did you mean '$1:$2$3'${2:+ aka $sq$1:./$3$sq}?
> +	EOF

Looks good otherwise; I tested this with various shells, and there were no surprises.

-- Hannes
Junio C Hamano· May 9, 2011, 16:20 UTC · re: Johannes Sixt · lore

Re: [PATCH] t1507: change quoting in test_did_you_mean to a more general one

Johannes Sixt <j.sixt@viscovery.net> writes:
Show 12 quoted lines
> Am 5/9/2011 6:40, schrieb Junio C Hamano:
>> Johannes Sixt <j.sixt@viscovery.net> writes:
>> 
>>> - The behavior of quoting at the right of :- when the ${...:-...} exansion
>>> appears in double-quotes was debated recently at length at the Austin
>>> group (which revises the POSIX standard). You better move the expansions
>>> to assignments of temporary variables,...
>> 
>> What alternative semantics do Austin folks have in mind, by the way?  Just
>> declare this undefined?
>
> Most of the debate centered around how to quote a closing brace: the
Thanks for a summary.
> Looks good otherwise; I tested this with various shells, and there were no
> surprises.
Thanks, will amend.
Kacper Kornet· May 6, 2011, 08:39 UTC · re: Junio C Hamano · lore

Re: [PATCH] t1507: change quoting in test_did_you_mean to a more general one

On Thu, May 05, 2011 at 01:02:16PM -0700, Junio C Hamano wrote:
> Kacper Kornet <draenog@pld-linux.org> writes:
> > In bash and some other shells the script:
> >   x=2; unset a; echo "${a:-'$x'}"
> > prints '2'. However ksh shell prints $x. The quoting is added to
> > reproduce bash behaviour.
> What I happen to have in /usr/bin/ksh
>     $ /usr/bin/ksh --version
>       version         sh (AT&T Research) 93t+ 2009-05-01
> does not seem to have this issue.
> Whose ksh is this
It is pdksh 5.2.14 from http://www.cs.mun.ca/~michael/pdksh/
> It is broken.

You are right. It is corrected in mksh, which claims to be a successor to pdksh. Unfortunately pdksh is used by default as /bin/sh in PLD Linux Distribution which are use. Probably the time to think about the change of this default.

-- 
  Kacper Kornet

← back to recent threads