[PATCH] t0008: 4 tests fail with ksh88

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] t0008: 4 tests fail with ksh88

Armin Kunaschik
From: Armin Kunaschik <[hidden email]>

\" in the test t0008 is not treated the same way in bash and in ksh.
In ksh the \ disappears and generates false expect data to
compare with.
Using \\" works portable, the same way in bash and in ksh and
is less ambigous.

Acked-by: Jeff King <[hidden email]>
Signed-off-by: Armin Kunaschik <[hidden email]>
---
diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
index 89544dd..b425f3a 100755
--- a/t/t0008-ignores.sh
+++ b/t/t0008-ignores.sh
@@ -605,7 +605,7 @@ cat <<-EOF >expected-verbose
        a/b/.gitignore:8:!on*   a/b/one
        a/b/.gitignore:8:!on*   a/b/one one
        a/b/.gitignore:8:!on*   a/b/one two
-       a/b/.gitignore:8:!on*   "a/b/one\"three"
+       a/b/.gitignore:8:!on*   "a/b/one\\"three"
        a/b/.gitignore:9:!two   a/b/two
        a/.gitignore:1:two*     a/b/twooo
        $global_excludes:2:!globaltwo   globaltwo
@@ -686,7 +686,7 @@ cat <<-EOF >expected-all
        a/b/.gitignore:8:!on*   b/one
        a/b/.gitignore:8:!on*   b/one one
        a/b/.gitignore:8:!on*   b/one two
-       a/b/.gitignore:8:!on*   "b/one\"three"
+       a/b/.gitignore:8:!on*   "b/one\\"three"
        a/b/.gitignore:9:!two   b/two
        ::      b/not-ignored
        a/.gitignore:1:two*     b/twooo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] t0008: 4 tests fail with ksh88

Junio C Hamano
Armin Kunaschik <[hidden email]> writes:

> From: Armin Kunaschik <[hidden email]>
>
> \" in the test t0008 is not treated the same way in bash and in ksh.

Could you refrain from singling out "bash"?  We don't write for
"bash" specifically (and the test I ran are with "dash" before I
push things out).

Ideally, if you can try ksh93 and if you find out that ksh93 works,
then the above can be made in line with your "Subject" to mark ksh88
as broken (as opposed to other POSIX shells)?  That would help us by
reminding that running test fine with ksh93 is not a sufficient
check to make sure we didn't break ksh88 users.

> In ksh the \ disappears and generates false expect data to
> compare with.
> Using \\" works portable, the same way in bash and in ksh and
> is less ambigous.

All of the above would need s/ksh/&88/g; I'd think.  I just tried

        make SHELL_PATH=/bin/ksh93
        cd t && /bin/ksh93 t0008-*.sh

and this patch is not necessary for ksh93.

> Acked-by: Jeff King <[hidden email]>

I didn't see him acking this exact version, so if you didn't include
this line here, I would have missed it.  Thanks.

> Signed-off-by: Armin Kunaschik <[hidden email]>
> ---
> diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
> index 89544dd..b425f3a 100755
> --- a/t/t0008-ignores.sh
> +++ b/t/t0008-ignores.sh
> @@ -605,7 +605,7 @@ cat <<-EOF >expected-verbose
>         a/b/.gitignore:8:!on*   a/b/one
>         a/b/.gitignore:8:!on*   a/b/one one
>         a/b/.gitignore:8:!on*   a/b/one two
> -       a/b/.gitignore:8:!on*   "a/b/one\"three"
> +       a/b/.gitignore:8:!on*   "a/b/one\\"three"
>         a/b/.gitignore:9:!two   a/b/two
>         a/.gitignore:1:two*     a/b/twooo
>         $global_excludes:2:!globaltwo   globaltwo
> @@ -686,7 +686,7 @@ cat <<-EOF >expected-all
>         a/b/.gitignore:8:!on*   b/one
>         a/b/.gitignore:8:!on*   b/one one
>         a/b/.gitignore:8:!on*   b/one two
> -       a/b/.gitignore:8:!on*   "b/one\"three"
> +       a/b/.gitignore:8:!on*   "b/one\\"three"
>         a/b/.gitignore:9:!two   b/two
>         ::      b/not-ignored
>         a/.gitignore:1:two*     b/twooo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] t0008: 4 tests fail with ksh88

Junio C Hamano
Junio C Hamano <[hidden email]> writes:

>> diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
>> index 89544dd..b425f3a 100755
>> --- a/t/t0008-ignores.sh
>> +++ b/t/t0008-ignores.sh
>> @@ -605,7 +605,7 @@ cat <<-EOF >expected-verbose
>>         a/b/.gitignore:8:!on*   a/b/one
>>         a/b/.gitignore:8:!on*   a/b/one one

The patch was whitespace-damaged and didn't apply, so I had to redo
it from scratch while updating the log message.  If this looks good
to you, there is no need to resend.

Thanks.

-- >8 --
From: Armin Kunaschik <[hidden email]>
Date: Fri, 20 May 2016 16:31:30 +0200
Subject: [PATCH] t0008: 4 tests fail with ksh88

In t0008, we have

        cat <<-EOF
        ...
        a/b/.gitignore:8:!on* "a/b/one\"three"
        ...
        EOF

ane expect that the backslash-dq is passed through literally.

ksh88 eats \ and generates a wrong expect data to compare with.

Using \\" works this around without breaking other POSIX shells
(which collapse backslash-backslash to a single backslash), and
ksh88 does so, too.

It makes it easier to read, too, because the reason why we are
writing backslash there is *not* because we think dq is special and
want to quote it (if that were the case we would have two more
backslashes on that line).  It is simply because we want a single
literal backslash there.  Since backslash is treated specially in
unquoted here-document, explicitly doubling it to quote it expresses
our intent better than relying on the character that immediately
comes after it (i.e. '"') not being a special character.

Signed-off-by: Armin Kunaschik <[hidden email]>
Acked-by: Jeff King <[hidden email]>
Signed-off-by: Junio C Hamano <[hidden email]>
---
 t/t0008-ignores.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
index 89544dd..b425f3a 100755
--- a/t/t0008-ignores.sh
+++ b/t/t0008-ignores.sh
@@ -605,7 +605,7 @@ cat <<-EOF >expected-verbose
  a/b/.gitignore:8:!on* a/b/one
  a/b/.gitignore:8:!on* a/b/one one
  a/b/.gitignore:8:!on* a/b/one two
- a/b/.gitignore:8:!on* "a/b/one\"three"
+ a/b/.gitignore:8:!on* "a/b/one\\"three"
  a/b/.gitignore:9:!two a/b/two
  a/.gitignore:1:two* a/b/twooo
  $global_excludes:2:!globaltwo globaltwo
@@ -686,7 +686,7 @@ cat <<-EOF >expected-all
  a/b/.gitignore:8:!on* b/one
  a/b/.gitignore:8:!on* b/one one
  a/b/.gitignore:8:!on* b/one two
- a/b/.gitignore:8:!on* "b/one\"three"
+ a/b/.gitignore:8:!on* "b/one\\"three"
  a/b/.gitignore:9:!two b/two
  :: b/not-ignored
  a/.gitignore:1:two* b/twooo
--
2.8.3-625-g89e3711

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] t0008: 4 tests fail with ksh88

Armin Kunaschik
In reply to this post by Junio C Hamano
On Fri, May 20, 2016 at 5:16 PM, Junio C Hamano <[hidden email]> wrote:
> Armin Kunaschik <[hidden email]> writes:
>
>> From: Armin Kunaschik <[hidden email]>
>>
>> \" in the test t0008 is not treated the same way in bash and in ksh.
>
> Could you refrain from singling out "bash"?  We don't write for
> "bash" specifically (and the test I ran are with "dash" before I
> push things out).
I can name it "other shells" if this is more comfortable. But I tested
this only with bash and ksh88 on AIX.

> Ideally, if you can try ksh93 and if you find out that ksh93 works,
> then the above can be made in line with your "Subject" to mark ksh88
> as broken (as opposed to other POSIX shells)?  That would help us by
> reminding that running test fine with ksh93 is not a sufficient
> check to make sure we didn't break ksh88 users.
>
>> In ksh the \ disappears and generates false expect data to
>> compare with.
>> Using \\" works portable, the same way in bash and in ksh and
>> is less ambigous.
>
> All of the above would need s/ksh/&88/g; I'd think.  I just tried
>
>         make SHELL_PATH=/bin/ksh93
>         cd t && /bin/ksh93 t0008-*.sh
>
> and this patch is not necessary for ksh93.
Yes, the patch is not necessary with ksh93 on AIX, but it works :-)
The patch is targeting "ksh" on AIX (which actually is a ksh88).

In the discussion Jeff took a look into the POSIX specification
and described the behavior like this:

<snip>
I think either is reasonable (there is no need to backslash-escape a
double-quote inside a here-doc, but one assumes that backslash would
generally have its usual behavior). I'm not quite sure how to interpret
POSIX here (see below), but it seems clear that spelling it with two
backslashes as you suggest is the best bet.
<snip>

I'd not declare ksh88 on AIX broken just because of this ambiguity
since it is not 100% clear in the POSIX description.

Armin
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] t0008: 4 tests fail with ksh88

Armin Kunaschik
In reply to this post by Junio C Hamano
On Fri, May 20, 2016 at 6:10 PM, Junio C Hamano <[hidden email]> wrote:

> Junio C Hamano <[hidden email]> writes:
>
>>> diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
>>> index 89544dd..b425f3a 100755
>>> --- a/t/t0008-ignores.sh
>>> +++ b/t/t0008-ignores.sh
>>> @@ -605,7 +605,7 @@ cat <<-EOF >expected-verbose
>>>         a/b/.gitignore:8:!on*   a/b/one
>>>         a/b/.gitignore:8:!on*   a/b/one one
>
> The patch was whitespace-damaged and didn't apply, so I had to redo
> it from scratch while updating the log message.  If this looks good
> to you, there is no need to resend.

Thanks. Looks like even Google Mail interface in plain text mode eats
white spaces :-(
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] t0008: 4 tests fail with ksh88

Christian Couder-2
In reply to this post by Junio C Hamano
On Fri, May 20, 2016 at 6:10 PM, Junio C Hamano <[hidden email]> wrote:

> Junio C Hamano <[hidden email]> writes:
>
> From: Armin Kunaschik <[hidden email]>
> Date: Fri, 20 May 2016 16:31:30 +0200
> Subject: [PATCH] t0008: 4 tests fail with ksh88
>
> In t0008, we have
>
>         cat <<-EOF
>         ...
>         a/b/.gitignore:8:!on*   "a/b/one\"three"
>         ...
>         EOF
>
> ane expect that the backslash-dq is passed through literally.

s/ane/and/

> ksh88 eats \ and generates a wrong expect data to compare with.
>
> Using \\" works this around without breaking other POSIX shells
> (which collapse backslash-backslash to a single backslash), and
> ksh88 does so, too.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] t0008: 4 tests fail with ksh88

Jeff King
In reply to this post by Junio C Hamano
On Fri, May 20, 2016 at 08:16:43AM -0700, Junio C Hamano wrote:

> > Acked-by: Jeff King <[hidden email]>
>
> I didn't see him acking this exact version, so if you didn't include
> this line here, I would have missed it.  Thanks.

I don't think I ever saw an actual patch to ack until now; I just said
the idea seemed good.

That being said, the patch _does_ look good, and I am fine to have my
ack/reviewed-by on it (though I agree with your points regarding the
commit message).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html