| Message ID | 20190213115951.12096-1-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] tests: avoid syntax triggering old dash bug | expand |
Æ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 &&
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>
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>
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 &&
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(+)