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

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

[Kacper Kornet]

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(-)

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

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

[Junio C Hamano]

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.

> 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

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 05.05.2011 22:02:
> 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.

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

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

[Kacper Kornet]

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

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

[Kacper Kornet]

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.

-- 
  Kacper Kornet

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

[Michael J Gruber]

Kacper Kornet venit, vidit, dixit 06.05.2011 10:51:
> 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

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

[Johannes Sixt]

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.

- 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

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

[Michael J Gruber]

Johannes Sixt venit, vidit, dixit 06.05.2011 12:22:
> 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)?

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

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

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

Nice to have somebody who follows that stuff on the list.

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

[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?

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(-)

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
 }
 

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

[Johannes Sixt]

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

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

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

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