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