Eric Sunshine [off-list ref] writes:
On Tue, Nov 27, 2018 at 11:43 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
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.
quoted hunk
@@ -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 &&
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 <redacted>
---
On Wed, Nov 28 2018, Junio C Hamano wrote:
Eric Sunshine [off-list ref] writes:
quoted
On Tue, Nov 27, 2018 at 11:43 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
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
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 <redacted>
On Wed, Feb 13 2019, SZEDER Gábor wrote:
On Wed, Feb 13, 2019 at 12:59:51PM +0100, Ævar Arnfjörð Bjarmason wrote:
quoted
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.
quoted
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.
quoted
2. https://public-inbox.org/git/20181127164253.9832-1-avarab@gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>