* [PATCH] tests: avoid syntax triggering old dash bug @ 2018-11-27 16:42 Ævar Arnfjörð Bjarmason 2018-11-27 19:16 ` Eric Sunshine 2018-11-28 1:47 ` [PATCH] " brian m. carlson 0 siblings, 2 replies; 10+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-11-27 16:42 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Haaris Mehmood, Æ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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] tests: avoid syntax triggering old dash bug 2018-11-27 16:42 [PATCH] tests: avoid syntax triggering old dash bug Ævar Arnfjörð Bjarmason @ 2018-11-27 19:16 ` Eric Sunshine 2018-11-27 19:37 ` Ævar Arnfjörð Bjarmason 2018-11-28 4:45 ` Junio C Hamano 2018-11-28 1:47 ` [PATCH] " brian m. carlson 1 sibling, 2 replies; 10+ messages in thread From: Eric Sunshine @ 2018-11-27 19:16 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: Git List, Junio C Hamano, hsed 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tests: avoid syntax triggering old dash bug 2018-11-27 19:16 ` Eric Sunshine @ 2018-11-27 19:37 ` Ævar Arnfjörð Bjarmason 2018-11-28 4:45 ` Junio C Hamano 1 sibling, 0 replies; 10+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-11-27 19:37 UTC (permalink / raw) To: Eric Sunshine; +Cc: Git List, Junio C Hamano, hsed 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tests: avoid syntax triggering old dash bug 2018-11-27 19:16 ` Eric Sunshine 2018-11-27 19:37 ` Ævar Arnfjörð Bjarmason @ 2018-11-28 4:45 ` Junio C Hamano 2019-02-13 11:59 ` [PATCH v2] " Ævar Arnfjörð Bjarmason 1 sibling, 1 reply; 10+ messages in thread From: Junio C Hamano @ 2018-11-28 4:45 UTC (permalink / raw) To: Eric Sunshine; +Cc: Ævar Arnfjörð Bjarmason, Git List, hsed 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 && ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] tests: avoid syntax triggering old dash bug 2018-11-28 4:45 ` Junio C Hamano @ 2019-02-13 11:59 ` Ævar Arnfjörð Bjarmason 2019-02-13 21:43 ` Junio C Hamano 2019-02-13 21:48 ` SZEDER Gábor 0 siblings, 2 replies; 10+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2019-02-13 11:59 UTC (permalink / raw) To: git Cc: Junio C Hamano, Eric Sunshine, hsed, Æ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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] tests: avoid syntax triggering old dash bug 2019-02-13 11:59 ` [PATCH v2] " Ævar Arnfjörð Bjarmason @ 2019-02-13 21:43 ` Junio C Hamano 2019-02-13 21:48 ` SZEDER Gábor 1 sibling, 0 replies; 10+ messages in thread From: Junio C Hamano @ 2019-02-13 21:43 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Eric Sunshine, hsed Æ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 && ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] tests: avoid syntax triggering old dash bug 2019-02-13 11:59 ` [PATCH v2] " Ævar Arnfjörð Bjarmason 2019-02-13 21:43 ` Junio C Hamano @ 2019-02-13 21:48 ` SZEDER Gábor 2019-02-14 9:51 ` Ævar Arnfjörð Bjarmason 1 sibling, 1 reply; 10+ messages in thread From: SZEDER Gábor @ 2019-02-13 21:48 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, Eric Sunshine, hsed 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] tests: avoid syntax triggering old dash bug 2019-02-13 21:48 ` SZEDER Gábor @ 2019-02-14 9:51 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 10+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2019-02-14 9:51 UTC (permalink / raw) To: SZEDER Gábor; +Cc: git, Junio C Hamano, Eric Sunshine, hsed 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tests: avoid syntax triggering old dash bug 2018-11-27 16:42 [PATCH] tests: avoid syntax triggering old dash bug Ævar Arnfjörð Bjarmason 2018-11-27 19:16 ` Eric Sunshine @ 2018-11-28 1:47 ` brian m. carlson 2018-11-28 7:06 ` Torsten Bögershausen 1 sibling, 1 reply; 10+ messages in thread From: brian m. carlson @ 2018-11-28 1:47 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, Haaris Mehmood [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tests: avoid syntax triggering old dash bug 2018-11-28 1:47 ` [PATCH] " brian m. carlson @ 2018-11-28 7:06 ` Torsten Bögershausen 0 siblings, 0 replies; 10+ messages in thread From: Torsten Bögershausen @ 2018-11-28 7:06 UTC (permalink / raw) To: brian m. carlson, Ævar Arnfjörð Bjarmason, git, Junio C Hamano, Haaris Mehmood 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-02-14 9:51 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-27 16:42 [PATCH] tests: avoid syntax triggering old dash bug Ævar Arnfjörð Bjarmason 2018-11-27 19:16 ` Eric Sunshine 2018-11-27 19:37 ` Ævar Arnfjörð Bjarmason 2018-11-28 4:45 ` Junio C Hamano 2019-02-13 11:59 ` [PATCH v2] " Ævar Arnfjörð Bjarmason 2019-02-13 21:43 ` Junio C Hamano 2019-02-13 21:48 ` SZEDER Gábor 2019-02-14 9:51 ` Ævar Arnfjörð Bjarmason 2018-11-28 1:47 ` [PATCH] " brian m. carlson 2018-11-28 7:06 ` Torsten Bögershausen
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).