[PATCH] tests: avoid syntax triggering old dash bug

[PATCH] tests: avoid syntax triggering old dash bug

[Ævar Arnfjörð Bjarmason]

Avoid a bug in dash that's been fixed ever since its
ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
released with dash v0.5.7 in July 2011.

This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other
failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding"
before URL encoding", 2016-02-09).

This particular test has been failing since 5f9674243d ("config: add
--expiry-date", 2017-11-18).

1. https://git.kernel.org/pub/scm/utils/dash/dash.git/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1300-config.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 9652b241c7..7690b518b8 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -892,8 +892,9 @@ test_expect_success 'get --expiry-date' '
 	1510348087
 	0
 	EOF
+	date_valid1=$(git config --expiry-date date.valid1) &&
 	{
-		echo "$rel_out $(git config --expiry-date date.valid1)"
+		echo "$rel_out $date_valid1"
 		git config --expiry-date date.valid2 &&
 		git config --expiry-date date.valid3 &&
 		git config --expiry-date date.valid4 &&
-- 
2.20.0.rc1.379.g1dd7ef354c

Re: [PATCH] tests: avoid syntax triggering old dash bug

[Eric Sunshine]

On Tue, Nov 27, 2018 at 11:43 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Avoid a bug in dash that's been fixed ever since its
> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
> released with dash v0.5.7 in July 2011.

Perhaps enhance the commit message to explain the nature of the bug
itself. It is not at all obvious from reading the above or from
looking at the diff itself what the actual problem is that the patch
is fixing. (And it wasn't even immediately obvious by looking at the
commit message of ec2c84d in the dash repository.) To help readers of
this patch avoid re-introducing this problem or diagnose such a
failure, it might be a good idea to give an example of the syntax
which trips up old dash (i.e. a here-doc followed immediately by a
{...} expression) and the actual error message 'Syntax error: "}"
unexpected'.

Thanks.

> This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other
> failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding"
> before URL encoding", 2016-02-09).
>
> This particular test has been failing since 5f9674243d ("config: add
> --expiry-date", 2017-11-18).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Re: [PATCH] tests: avoid syntax triggering old dash bug

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 27 2018, Eric Sunshine wrote:

> On Tue, Nov 27, 2018 at 11:43 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> Avoid a bug in dash that's been fixed ever since its
>> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
>> released with dash v0.5.7 in July 2011.
>
> Perhaps enhance the commit message to explain the nature of the bug
> itself. It is not at all obvious from reading the above or from
> looking at the diff itself what the actual problem is that the patch
> is fixing. (And it wasn't even immediately obvious by looking at the
> commit message of ec2c84d in the dash repository.) To help readers of
> this patch avoid re-introducing this problem or diagnose such a
> failure, it might be a good idea to give an example of the syntax
> which trips up old dash (i.e. a here-doc followed immediately by a
> {...} expression) and the actual error message 'Syntax error: "}"
> unexpected'.

I haven't taken the time to understand the bug either. Our entire test
suite had one instance of this, so I think it's obscure enough that it's
fine to just fix it as a one-off and not spend any more time on making
sure it doesn't happen again or add some lint for detecting it.

>> This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other
>> failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding"
>> before URL encoding", 2016-02-09).
>>
>> This particular test has been failing since 5f9674243d ("config: add
>> --expiry-date", 2017-11-18).
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Re: [PATCH] tests: avoid syntax triggering old dash bug

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 772 bytes --]

On Tue, Nov 27, 2018 at 05:42:53PM +0100, Ævar Arnfjörð Bjarmason wrote:
> Avoid a bug in dash that's been fixed ever since its
> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
> released with dash v0.5.7 in July 2011.
> 
> This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other
> failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding"
> before URL encoding", 2016-02-09).

Are people still using such versions of Debian?  I only see wheezy (7)
on the mirrors, not squeeze (6) or lenny (5).  It might be better for us
to encourage users to upgrade to an OS that has security support rather
than work around bugs in obsolete OSes.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [PATCH] tests: avoid syntax triggering old dash bug

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Tue, Nov 27, 2018 at 11:43 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> Avoid a bug in dash that's been fixed ever since its
>> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
>> released with dash v0.5.7 in July 2011.
>
> Perhaps enhance the commit message to explain the nature of the bug
> itself. It is not at all obvious from reading the above or from
> looking at the diff itself what the actual problem is that the patch
> is fixing. (And it wasn't even immediately obvious by looking at the
> commit message of ec2c84d in the dash repository.) To help readers of
> this patch avoid re-introducing this problem or diagnose such a
> failure, it might be a good idea to give an example of the syntax
> which trips up old dash (i.e. a here-doc followed immediately by a
> {...} expression) and the actual error message 'Syntax error: "}"
> unexpected'.

Indeed.  From the patch text, I would not have even guessed.  I was
wondering if there were interactions with "" and $() inside it.

If having {...} immediately after a here-doc is a problem, then the
patch should not touch existing code at all, but instead insert a
new line, perhaps like

	: avoid open brace immediately after here-doc for old dash

immediately before {...}; that would have made it easier to grok.

>@@ -892,8 +892,9 @@ test_expect_success 'get --expiry-date' '
> 	1510348087
> 	0
> 	EOF
>+	date_valid1=$(git config --expiry-date date.valid1) &&
> 	{
>-		echo "$rel_out $(git config --expiry-date date.valid1)"
>+		echo "$rel_out $date_valid1"
> 		git config --expiry-date date.valid2 &&
> 		git config --expiry-date date.valid3 &&
> 		git config --expiry-date date.valid4 &&

Re: [PATCH] tests: avoid syntax triggering old dash bug

[Torsten Bögershausen]

On Wed, Nov 28, 2018 at 01:47:41AM +0000, brian m. carlson wrote:
> On Tue, Nov 27, 2018 at 05:42:53PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > Avoid a bug in dash that's been fixed ever since its
> > ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
> > released with dash v0.5.7 in July 2011.
> > 
> > This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other
> > failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding"
> > before URL encoding", 2016-02-09).
> 
> Are people still using such versions of Debian?  I only see wheezy (7)
> on the mirrors, not squeeze (6) or lenny (5).  It might be better for us
> to encourage users to upgrade to an OS that has security support rather
> than work around bugs in obsolete OSes.

Yes, I have an old PowerPC box to test if code handle endians right.
And to ask people to upgrade does not conflict with supporting older
versions (if that is as easy as this patch).
I think we can have both.

[PATCH v2] tests: avoid syntax triggering old dash bug

[Ævar Arnfjörð Bjarmason]

Avoid a bug in dash that's been fixed ever since its
ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
released with dash v0.5.7 in July 2011. This failing test was
introduced in 5f9674243d ("config: add --expiry-date", 2017-11-18).

This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other
failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding"
before URL encoding", 2016-02-09).

The dash bug is triggered by this test because the heredoc contains a
command embedded in "$()" with a "{}" block coming right after
it. Refactoring the "$()" to e.g. be a variable that was set earlier
will also work around it, but let's instead break up the "EOF" and the
"{}".

An earlier version of this patch[2] mitigated the issue by breaking
the "$()" out of the "{}" block, that worked, but just because it
broke up the "EOF" and "{}" block. Putting e.g. "echo &&" between the
two would also work.

1. https://git.kernel.org/pub/scm/utils/dash/dash.git/
2. https://public-inbox.org/git/20181127164253.9832-1-avarab@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Wed, Nov 28 2018, Junio C Hamano wrote:

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> On Tue, Nov 27, 2018 at 11:43 AM Ævar Arnfjörð Bjarmason
>> <avarab@gmail.com> wrote:
>>> Avoid a bug in dash that's been fixed ever since its
>>> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
>>> released with dash v0.5.7 in July 2011.
>>
>> Perhaps enhance the commit message to explain the nature of the bug
>> itself. It is not at all obvious from reading the above or from
>> looking at the diff itself what the actual problem is that the patch
>> is fixing. (And it wasn't even immediately obvious by looking at the
>> commit message of ec2c84d in the dash repository.) To help readers of
>> this patch avoid re-introducing this problem or diagnose such a
>> failure, it might be a good idea to give an example of the syntax
>> which trips up old dash (i.e. a here-doc followed immediately by a
>> {...} expression) and the actual error message 'Syntax error: "}"
>> unexpected'.
>
> Indeed.  From the patch text, I would not have even guessed.  I was
> wondering if there were interactions with "" and $() inside it.
>
> If having {...} immediately after a here-doc is a problem, then the
> patch should not touch existing code at all, but instead insert a
> new line, perhaps like
>
> 	: avoid open brace immediately after here-doc for old dash

Late re-roll. Now using a variant of that suggestion, and with an
updated commit message explaining what the issue in dash is exactly
and why it was triggered.

This isn't a 2.21 regression, but sending it in the rc window anyway
in case you'd like to queue an obviously working minor portability
fix.

 t/t1300-config.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 9652b241c7..428177c390 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -892,6 +892,7 @@ test_expect_success 'get --expiry-date' '
 	1510348087
 	0
 	EOF
+	: "work around heredoc parsing bug fixed in dash 0.5.7 (in ec2c84d)" &&
 	{
 		echo "$rel_out $(git config --expiry-date date.valid1)"
 		git config --expiry-date date.valid2 &&
-- 
2.20.1.611.gfbb209baf1

Re: [PATCH v2] tests: avoid syntax triggering old dash bug

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Avoid a bug in dash that's been fixed ever since its
> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
> released with dash v0.5.7 in July 2011. This failing test was
> introduced in 5f9674243d ("config: add --expiry-date", 2017-11-18).

Thanks for not forgetting an year-old topic.  The offending commit
predates 2.16.0 that is a while ago ;-)

> This isn't a 2.21 regression, but sending it in the rc window anyway
> in case you'd like to queue an obviously working minor portability
> fix.
>
>  t/t1300-config.sh | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index 9652b241c7..428177c390 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -892,6 +892,7 @@ test_expect_success 'get --expiry-date' '
>  	1510348087
>  	0
>  	EOF
> +	: "work around heredoc parsing bug fixed in dash 0.5.7 (in ec2c84d)" &&
>  	{
>  		echo "$rel_out $(git config --expiry-date date.valid1)"
>  		git config --expiry-date date.valid2 &&

Re: [PATCH v2] tests: avoid syntax triggering old dash bug

[SZEDER Gábor]

On Wed, Feb 13, 2019 at 12:59:51PM +0100, Ævar Arnfjörð Bjarmason wrote:
> Avoid a bug in dash that's been fixed ever since its
> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
> released with dash v0.5.7 in July 2011. This failing test was
> introduced in 5f9674243d ("config: add --expiry-date", 2017-11-18).
> 
> This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other

Do I understand this "1/2" right?  There are two tests failing on
Lenny and Squeeze, and this fixes one of those bugs?

> failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding"
> before URL encoding", 2016-02-09).
> 
> The dash bug is triggered by this test because the heredoc contains a
> command embedded in "$()" with a "{}" block coming right after
> it. Refactoring the "$()" to e.g. be a variable that was set earlier
> will also work around it, but let's instead break up the "EOF" and the
> "{}".
> 
> An earlier version of this patch[2] mitigated the issue by breaking
> the "$()" out of the "{}" block, that worked, but just because it
> broke up the "EOF" and "{}" block. Putting e.g. "echo &&" between the
> two would also work.
> 
> 1. https://git.kernel.org/pub/scm/utils/dash/dash.git/

Could you please link directly to the commit fixing that issue?

  https://git.kernel.org/pub/scm/utils/dash/dash.git/commit/?id=ec2c84d3c4dba4b74440d72bdd1de416a9acd2a9

> 2. https://public-inbox.org/git/20181127164253.9832-1-avarab@gmail.com/
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Re: [PATCH v2] tests: avoid syntax triggering old dash bug

[Ævar Arnfjörð Bjarmason]

On Wed, Feb 13 2019, SZEDER Gábor wrote:

> On Wed, Feb 13, 2019 at 12:59:51PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> Avoid a bug in dash that's been fixed ever since its
>> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
>> released with dash v0.5.7 in July 2011. This failing test was
>> introduced in 5f9674243d ("config: add --expiry-date", 2017-11-18).
>>
>> This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other
>
> Do I understand this "1/2" right?  There are two tests failing on
> Lenny and Squeeze, and this fixes one of those bugs?

Yeah, so there's one bug left now, which I haven't tracked down.

>> failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding"
>> before URL encoding", 2016-02-09).
>>
>> The dash bug is triggered by this test because the heredoc contains a
>> command embedded in "$()" with a "{}" block coming right after
>> it. Refactoring the "$()" to e.g. be a variable that was set earlier
>> will also work around it, but let's instead break up the "EOF" and the
>> "{}".
>>
>> An earlier version of this patch[2] mitigated the issue by breaking
>> the "$()" out of the "{}" block, that worked, but just because it
>> broke up the "EOF" and "{}" block. Putting e.g. "echo &&" between the
>> two would also work.
>>
>> 1. https://git.kernel.org/pub/scm/utils/dash/dash.git/
>
> Could you please link directly to the commit fixing that issue?
>
>   https://git.kernel.org/pub/scm/utils/dash/dash.git/commit/?id=ec2c84d3c4dba4b74440d72bdd1de416a9acd2a9

Should have done that, but I'll hold off on a re-roll for such a minor
cosmetic issue since I see Junio's merged it down to "next" already. The
dash.git hash is noted in the commit message, so it's not a practical
problem to find the commit, but yeah, would be nice if were a clickable
link.

>> 2. https://public-inbox.org/git/20181127164253.9832-1-avarab@gmail.com/
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>