{"thread":{"id":"49901","subject":"[PATCH] tests: avoid syntax triggering old dash bug","startedAt":"2018-11-27T16:43:08Z","lastAt":"2019-02-14T09:51:08Z","messageCount":10,"participants":["Ævar Arnfjörð Bjarmason","Eric Sunshine","brian m. carlson","Junio C Hamano","Torsten Bögershausen","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"364137","messageId":"20181127164253.9832-1-avarab@gmail.com","threadId":"49901","inReplyTo":null,"subject":"[PATCH] tests: avoid syntax triggering old dash bug","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-27T16:42:53Z","receivedAt":"2018-11-27T16:43:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Avoid a bug in dash that's been fixed ever since its\nec2c84d (\"[PARSER] Fix clobbering of checkkwd\", 2011-03-15)[1] first\nreleased with dash v0.5.7 in July 2011.\n\nThis fixes 1/2 tests failing on Debian Lenny & Squeeze. The other\nfailure is due to 1b42f45255 (\"git-svn: apply \"svn.pathnameencoding\"\nbefore URL encoding\", 2016-02-09).\n\nThis particular test has been failing since 5f9674243d (\"config: add\n--expiry-date\", 2017-11-18).\n\n1. https://git.kernel.org/pub/scm/utils/dash/dash.git/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t1300-config.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 9652b241c7..7690b518b8 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -892,8 +892,9 @@ test_expect_success 'get --expiry-date' '\n \t1510348087\n \t0\n \tEOF\n+\tdate_valid1=$(git config --expiry-date date.valid1) &&\n \t{\n-\t\techo \"$rel_out $(git config --expiry-date date.valid1)\"\n+\t\techo \"$rel_out $date_valid1\"\n \t\tgit config --expiry-date date.valid2 &&\n \t\tgit config --expiry-date date.valid3 &&\n \t\tgit config --expiry-date date.valid4 &&\n-- \n2.20.0.rc1.379.g1dd7ef354c\n\n"},{"id":"364150","messageId":"CAPig+cS-_m8RBsCME_Gj15EEs1tcFcGn0Acfvq4HgxKiL9PaHg@mail.gmail.com","threadId":"49901","inReplyTo":"20181127164253.9832-1-avarab@gmail.com","subject":"Re: [PATCH] tests: avoid syntax triggering old dash bug","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-11-27T19:16:55Z","receivedAt":"2018-11-27T19:25:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 27, 2018 at 11:43 AM Ævar Arnfjörð Bjarmason\n wrote:\n> Avoid a bug in dash that's been fixed ever since its\n> ec2c84d (\"[PARSER] Fix clobbering of checkkwd\", 2011-03-15)[1] first\n> released with dash v0.5.7 in July 2011.\n\nPerhaps enhance the commit message to explain the nature of the bug\nitself. It is not at all obvious from reading the above or from\nlooking at the diff itself what the actual problem is that the patch\nis fixing. (And it wasn't even immediately obvious by looking at the\ncommit message of ec2c84d in the dash repository.) To help readers of\nthis patch avoid re-introducing this problem or diagnose such a\nfailure, it might be a good idea to give an example of the syntax\nwhich trips up old dash (i.e. a here-doc followed immediately by a\n{...} expression) and the actual error message 'Syntax error: \"}\"\nunexpected'.\n\nThanks.\n\n> This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other\n> failure is due to 1b42f45255 (\"git-svn: apply \"svn.pathnameencoding\"\n> before URL encoding\", 2016-02-09).\n>\n> This particular test has been failing since 5f9674243d (\"config: add\n> --expiry-date\", 2017-11-18).\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n"},{"id":"364151","messageId":"87k1kyz4zs.fsf@evledraar.gmail.com","threadId":"49901","inReplyTo":"CAPig+cS-_m8RBsCME_Gj15EEs1tcFcGn0Acfvq4HgxKiL9PaHg@mail.gmail.com","subject":"Re: [PATCH] tests: avoid syntax triggering old dash bug","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-27T19:37:27Z","receivedAt":"2018-11-27T19:37:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 27 2018, Eric Sunshine wrote:\n\n> On Tue, Nov 27, 2018 at 11:43 AM Ævar Arnfjörð Bjarmason\n> wrote:\n>> Avoid a bug in dash that's been fixed ever since its\n>> ec2c84d (\"[PARSER] Fix clobbering of checkkwd\", 2011-03-15)[1] first\n>> released with dash v0.5.7 in July 2011.\n>\n> Perhaps enhance the commit message to explain the nature of the bug\n> itself. It is not at all obvious from reading the above or from\n> looking at the diff itself what the actual problem is that the patch\n> is fixing. (And it wasn't even immediately obvious by looking at the\n> commit message of ec2c84d in the dash repository.) To help readers of\n> this patch avoid re-introducing this problem or diagnose such a\n> failure, it might be a good idea to give an example of the syntax\n> which trips up old dash (i.e. a here-doc followed immediately by a\n> {...} expression) and the actual error message 'Syntax error: \"}\"\n> unexpected'.\n\nI haven't taken the time to understand the bug either. Our entire test\nsuite had one instance of this, so I think it's obscure enough that it's\nfine to just fix it as a one-off and not spend any more time on making\nsure it doesn't happen again or add some lint for detecting it.\n\n>> This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other\n>> failure is due to 1b42f45255 (\"git-svn: apply \"svn.pathnameencoding\"\n>> before URL encoding\", 2016-02-09).\n>>\n>> This particular test has been failing since 5f9674243d (\"config: add\n>> --expiry-date\", 2017-11-18).\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n"},{"id":"364179","messageId":"20181128014740.GU890086@genre.crustytoothpaste.net","threadId":"49901","inReplyTo":"20181127164253.9832-1-avarab@gmail.com","subject":"Re: [PATCH] tests: avoid syntax triggering old dash bug","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-11-28T01:47:41Z","receivedAt":"2018-11-28T01:47:49Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Nov 27, 2018 at 05:42:53PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> Avoid a bug in dash that's been fixed ever since its\n> ec2c84d (\"[PARSER] Fix clobbering of checkkwd\", 2011-03-15)[1] first\n> released with dash v0.5.7 in July 2011.\n> \n> This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other\n> failure is due to 1b42f45255 (\"git-svn: apply \"svn.pathnameencoding\"\n> before URL encoding\", 2016-02-09).\n\nAre people still using such versions of Debian? I only see wheezy (7)\non the mirrors, not squeeze (6) or lenny (5). It might be better for us\nto encourage users to upgrade to an OS that has security support rather\nthan work around bugs in obsolete OSes.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"364188","messageId":"xmqqo9a9stbs.fsf@gitster-ct.c.googlers.com","threadId":"49901","inReplyTo":"CAPig+cS-_m8RBsCME_Gj15EEs1tcFcGn0Acfvq4HgxKiL9PaHg@mail.gmail.com","subject":"Re: [PATCH] tests: avoid syntax triggering old dash bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-28T04:45:59Z","receivedAt":"2018-11-28T04:46:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Tue, Nov 27, 2018 at 11:43 AM Ævar Arnfjörð Bjarmason\n> wrote:\n>> Avoid a bug in dash that's been fixed ever since its\n>> ec2c84d (\"[PARSER] Fix clobbering of checkkwd\", 2011-03-15)[1] first\n>> released with dash v0.5.7 in July 2011.\n>\n> Perhaps enhance the commit message to explain the nature of the bug\n> itself. It is not at all obvious from reading the above or from\n> looking at the diff itself what the actual problem is that the patch\n> is fixing. (And it wasn't even immediately obvious by looking at the\n> commit message of ec2c84d in the dash repository.) To help readers of\n> this patch avoid re-introducing this problem or diagnose such a\n> failure, it might be a good idea to give an example of the syntax\n> which trips up old dash (i.e. a here-doc followed immediately by a\n> {...} expression) and the actual error message 'Syntax error: \"}\"\n> unexpected'.\n\nIndeed. From the patch text, I would not have even guessed. I was\nwondering if there were interactions with \"\" and $() inside it.\n\nIf having {...} immediately after a here-doc is a problem, then the\npatch should not touch existing code at all, but instead insert a\nnew line, perhaps like\n\n\t: avoid open brace immediately after here-doc for old dash\n\nimmediately before {...}; that would have made it easier to grok.\n\n>@@ -892,8 +892,9 @@ test_expect_success 'get --expiry-date' '\n> \t1510348087\n> \t0\n> \tEOF\n>+\tdate_valid1=$(git config --expiry-date date.valid1) &&\n> \t{\n>-\t\techo \"$rel_out $(git config --expiry-date date.valid1)\"\n>+\t\techo \"$rel_out $date_valid1\"\n> \t\tgit config --expiry-date date.valid2 &&\n> \t\tgit config --expiry-date date.valid3 &&\n> \t\tgit config --expiry-date date.valid4 &&\n"},{"id":"364196","messageId":"20181128070618.GA25168@tor.lan","threadId":"49901","inReplyTo":"20181128014740.GU890086@genre.crustytoothpaste.net","subject":"Re: [PATCH] tests: avoid syntax triggering old dash bug","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2018-11-28T07:06:18Z","receivedAt":"2018-11-28T07:06:45Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Wed, Nov 28, 2018 at 01:47:41AM +0000, brian m. carlson wrote:\n> On Tue, Nov 27, 2018 at 05:42:53PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> > Avoid a bug in dash that's been fixed ever since its\n> > ec2c84d (\"[PARSER] Fix clobbering of checkkwd\", 2011-03-15)[1] first\n> > released with dash v0.5.7 in July 2011.\n> > \n> > This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other\n> > failure is due to 1b42f45255 (\"git-svn: apply \"svn.pathnameencoding\"\n> > before URL encoding\", 2016-02-09).\n> \n> Are people still using such versions of Debian? I only see wheezy (7)\n> on the mirrors, not squeeze (6) or lenny (5). It might be better for us\n> to encourage users to upgrade to an OS that has security support rather\n> than work around bugs in obsolete OSes.\n\nYes, I have an old PowerPC box to test if code handle endians right.\nAnd to ask people to upgrade does not conflict with supporting older\nversions (if that is as easy as this patch).\nI think we can have both.\n\n\n"},{"id":"369227","messageId":"20190213115951.12096-1-avarab@gmail.com","threadId":"49901","inReplyTo":"xmqqo9a9stbs.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v2] tests: avoid syntax triggering old dash bug","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-13T11:59:51Z","receivedAt":"2019-02-13T12:00:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Avoid a bug in dash that's been fixed ever since its\nec2c84d (\"[PARSER] Fix clobbering of checkkwd\", 2011-03-15)[1] first\nreleased with dash v0.5.7 in July 2011. This failing test was\nintroduced in 5f9674243d (\"config: add --expiry-date\", 2017-11-18).\n\nThis fixes 1/2 tests failing on Debian Lenny & Squeeze. The other\nfailure is due to 1b42f45255 (\"git-svn: apply \"svn.pathnameencoding\"\nbefore URL encoding\", 2016-02-09).\n\nThe dash bug is triggered by this test because the heredoc contains a\ncommand embedded in \"$()\" with a \"{}\" block coming right after\nit. Refactoring the \"$()\" to e.g. be a variable that was set earlier\nwill also work around it, but let's instead break up the \"EOF\" and the\n\"{}\".\n\nAn earlier version of this patch[2] mitigated the issue by breaking\nthe \"$()\" out of the \"{}\" block, that worked, but just because it\nbroke up the \"EOF\" and \"{}\" block. Putting e.g. \"echo &&\" between the\ntwo would also work.\n\n1. https://git.kernel.org/pub/scm/utils/dash/dash.git/\n2. https://public-inbox.org/git/20181127164253.9832-1-avarab@gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nOn Wed, Nov 28 2018, Junio C Hamano wrote:\n\n> Eric Sunshine writes:\n>\n>> On Tue, Nov 27, 2018 at 11:43 AM Ævar Arnfjörð Bjarmason\n>> wrote:\n>>> Avoid a bug in dash that's been fixed ever since its\n>>> ec2c84d (\"[PARSER] Fix clobbering of checkkwd\", 2011-03-15)[1] first\n>>> released with dash v0.5.7 in July 2011.\n>>\n>> Perhaps enhance the commit message to explain the nature of the bug\n>> itself. It is not at all obvious from reading the above or from\n>> looking at the diff itself what the actual problem is that the patch\n>> is fixing. (And it wasn't even immediately obvious by looking at the\n>> commit message of ec2c84d in the dash repository.) To help readers of\n>> this patch avoid re-introducing this problem or diagnose such a\n>> failure, it might be a good idea to give an example of the syntax\n>> which trips up old dash (i.e. a here-doc followed immediately by a\n>> {...} expression) and the actual error message 'Syntax error: \"}\"\n>> unexpected'.\n>\n> Indeed. From the patch text, I would not have even guessed. I was\n> wondering if there were interactions with \"\" and $() inside it.\n>\n> If having {...} immediately after a here-doc is a problem, then the\n> patch should not touch existing code at all, but instead insert a\n> new line, perhaps like\n>\n> \t: avoid open brace immediately after here-doc for old dash\n\nLate re-roll. Now using a variant of that suggestion, and with an\nupdated commit message explaining what the issue in dash is exactly\nand why it was triggered.\n\nThis isn't a 2.21 regression, but sending it in the rc window anyway\nin case you'd like to queue an obviously working minor portability\nfix.\n\n t/t1300-config.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 9652b241c7..428177c390 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -892,6 +892,7 @@ test_expect_success 'get --expiry-date' '\n \t1510348087\n \t0\n \tEOF\n+\t: \"work around heredoc parsing bug fixed in dash 0.5.7 (in ec2c84d)\" &&\n \t{\n \t\techo \"$rel_out $(git config --expiry-date date.valid1)\"\n \t\tgit config --expiry-date date.valid2 &&\n-- \n2.20.1.611.gfbb209baf1\n\n"},{"id":"369258","messageId":"xmqqd0nvbbzz.fsf@gitster-ct.c.googlers.com","threadId":"49901","inReplyTo":"20190213115951.12096-1-avarab@gmail.com","subject":"Re: [PATCH v2] tests: avoid syntax triggering old dash bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-13T21:43:28Z","receivedAt":"2019-02-13T21:43:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Avoid a bug in dash that's been fixed ever since its\n> ec2c84d (\"[PARSER] Fix clobbering of checkkwd\", 2011-03-15)[1] first\n> released with dash v0.5.7 in July 2011. This failing test was\n> introduced in 5f9674243d (\"config: add --expiry-date\", 2017-11-18).\n\nThanks for not forgetting an year-old topic. The offending commit\npredates 2.16.0 that is a while ago ;-)\n\n> This isn't a 2.21 regression, but sending it in the rc window anyway\n> in case you'd like to queue an obviously working minor portability\n> fix.\n>\n> t/t1300-config.sh | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n> index 9652b241c7..428177c390 100755\n> --- a/t/t1300-config.sh\n> +++ b/t/t1300-config.sh\n> @@ -892,6 +892,7 @@ test_expect_success 'get --expiry-date' '\n> \t1510348087\n> \t0\n> \tEOF\n> +\t: \"work around heredoc parsing bug fixed in dash 0.5.7 (in ec2c84d)\" &&\n> \t{\n> \t\techo \"$rel_out $(git config --expiry-date date.valid1)\"\n> \t\tgit config --expiry-date date.valid2 &&\n"},{"id":"369260","messageId":"20190213214844.GI1622@szeder.dev","threadId":"49901","inReplyTo":"20190213115951.12096-1-avarab@gmail.com","subject":"Re: [PATCH v2] tests: avoid syntax triggering old dash bug","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-02-13T21:48:44Z","receivedAt":"2019-02-13T21:48:51Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Feb 13, 2019 at 12:59:51PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> Avoid a bug in dash that's been fixed ever since its\n> ec2c84d (\"[PARSER] Fix clobbering of checkkwd\", 2011-03-15)[1] first\n> released with dash v0.5.7 in July 2011. This failing test was\n> introduced in 5f9674243d (\"config: add --expiry-date\", 2017-11-18).\n> \n> This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other\n\nDo I understand this \"1/2\" right? There are two tests failing on\nLenny and Squeeze, and this fixes one of those bugs?\n\n> failure is due to 1b42f45255 (\"git-svn: apply \"svn.pathnameencoding\"\n> before URL encoding\", 2016-02-09).\n> \n> The dash bug is triggered by this test because the heredoc contains a\n> command embedded in \"$()\" with a \"{}\" block coming right after\n> it. Refactoring the \"$()\" to e.g. be a variable that was set earlier\n> will also work around it, but let's instead break up the \"EOF\" and the\n> \"{}\".\n> \n> An earlier version of this patch[2] mitigated the issue by breaking\n> the \"$()\" out of the \"{}\" block, that worked, but just because it\n> broke up the \"EOF\" and \"{}\" block. Putting e.g. \"echo &&\" between the\n> two would also work.\n> \n> 1. https://git.kernel.org/pub/scm/utils/dash/dash.git/\n\nCould you please link directly to the commit fixing that issue?\n\n https://git.kernel.org/pub/scm/utils/dash/dash.git/commit/?id=ec2c84d3c4dba4b74440d72bdd1de416a9acd2a9\n\n> 2. https://public-inbox.org/git/20181127164253.9832-1-avarab@gmail.com/\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n"},{"id":"369303","messageId":"87d0nuk8ag.fsf@evledraar.gmail.com","threadId":"49901","inReplyTo":"20190213214844.GI1622@szeder.dev","subject":"Re: [PATCH v2] tests: avoid syntax triggering old dash bug","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-14T09:51:03Z","receivedAt":"2019-02-14T09:51:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Feb 13 2019, SZEDER Gábor wrote:\n\n> On Wed, Feb 13, 2019 at 12:59:51PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> Avoid a bug in dash that's been fixed ever since its\n>> ec2c84d (\"[PARSER] Fix clobbering of checkkwd\", 2011-03-15)[1] first\n>> released with dash v0.5.7 in July 2011. This failing test was\n>> introduced in 5f9674243d (\"config: add --expiry-date\", 2017-11-18).\n>>\n>> This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other\n>\n> Do I understand this \"1/2\" right? There are two tests failing on\n> Lenny and Squeeze, and this fixes one of those bugs?\n\nYeah, so there's one bug left now, which I haven't tracked down.\n\n>> failure is due to 1b42f45255 (\"git-svn: apply \"svn.pathnameencoding\"\n>> before URL encoding\", 2016-02-09).\n>>\n>> The dash bug is triggered by this test because the heredoc contains a\n>> command embedded in \"$()\" with a \"{}\" block coming right after\n>> it. Refactoring the \"$()\" to e.g. be a variable that was set earlier\n>> will also work around it, but let's instead break up the \"EOF\" and the\n>> \"{}\".\n>>\n>> An earlier version of this patch[2] mitigated the issue by breaking\n>> the \"$()\" out of the \"{}\" block, that worked, but just because it\n>> broke up the \"EOF\" and \"{}\" block. Putting e.g. \"echo &&\" between the\n>> two would also work.\n>>\n>> 1. https://git.kernel.org/pub/scm/utils/dash/dash.git/\n>\n> Could you please link directly to the commit fixing that issue?\n>\n> https://git.kernel.org/pub/scm/utils/dash/dash.git/commit/?id=ec2c84d3c4dba4b74440d72bdd1de416a9acd2a9\n\nShould have done that, but I'll hold off on a re-roll for such a minor\ncosmetic issue since I see Junio's merged it down to \"next\" already. The\ndash.git hash is noted in the commit message, so it's not a practical\nproblem to find the commit, but yeah, would be nice if were a clickable\nlink.\n\n>> 2. https://public-inbox.org/git/20181127164253.9832-1-avarab@gmail.com/\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n"}]}