{"thread":{"id":"25186","subject":"[PATCH] t1503: Fix arithmetic expansion syntax error when using dash","startedAt":"2010-09-21T17:45:09Z","lastAt":"2010-09-29T19:57:40Z","messageCount":7,"participants":["Ramsay Jones","Junio C Hamano","Jon Seymour","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"151225","messageId":"4C98EF25.4070700@ramsay1.demon.co.uk","threadId":"25186","inReplyTo":null,"subject":"[PATCH] t1503: Fix arithmetic expansion syntax error when using dash","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2010-09-21T17:45:09Z","receivedAt":"2010-09-21T17:45:09Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nOn systems which have dash as /bin/sh, such as Ubuntu, the final\ntest (master@{n} for various n) fails with a syntax error while\nprocessing an arithmetic expansion. The syntax error is caused by\nusing a bare name ('N') as a variable reference in the expression.\n\nIn order to avoid the syntax error, we spell the variable reference\nas '$N' rather than simply 'N'.\n\nSigned-off-by: Ramsay Jones \n---\n t/t1503-rev-parse-verify.sh | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh\nindex 100f857..813cc1b 100755\n--- a/t/t1503-rev-parse-verify.sh\n+++ b/t/t1503-rev-parse-verify.sh\n@@ -106,8 +106,8 @@ test_expect_success 'use --default' '\n \n test_expect_success 'master@{n} for various n' '\n \tN=$(git reflog | wc -l) &&\n-\tNm1=$((N-1)) &&\n-\tNp1=$((N+1)) &&\n+\tNm1=$(($N-1)) &&\n+\tNp1=$(($N+1)) &&\n \tgit rev-parse --verify master@{0} &&\n \tgit rev-parse --verify master@{1} &&\n \tgit rev-parse --verify master@{$Nm1} &&\n-- \n1.7.3\n"},{"id":"151318","messageId":"7viq1xsi7a.fsf@alter.siamese.dyndns.org","threadId":"25186","inReplyTo":"4C98EF25.4070700@ramsay1.demon.co.uk","subject":"Re*: [PATCH] t1503: Fix arithmetic expansion syntax error when using dash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-09-22T19:15:37Z","receivedAt":"2010-09-22T19:15:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> On systems which have dash as /bin/sh, such as Ubuntu, the final\n> test (master@{n} for various n) fails with a syntax error while\n> processing an arithmetic expansion. The syntax error is caused by\n> using a bare name ('N') as a variable reference in the expression.\n>\n> In order to avoid the syntax error, we spell the variable reference\n> as '$N' rather than simply 'N'.\n>\n> Signed-off-by: Ramsay Jones \n\nThanks.\n\nPOSIX wants shells to support both \"N\" and \"$N\" and requires them to yield\nthe same answer to $((N)) and $(($N)), but we should aim for portability\nin a case like this, especially when the price we pay to do so is so\nsmall, i.e. a few extra dollars.\n\nBy the way, on my box, I get this:\n\n $ ls l /bin/dash\n -rwxr-xr-x 1 root root 104024 2008-08-26 02:36 /bin/dash*\n $ dpkg -l dash | grep '^ii'\n ii dash 0.5.4-12 POSIX-compliant shell\n $ /bin/dash -c 'N=20 ; echo $(( N + 3 ))'\n 23\n\nI just left it vague by saying \"e.g. older dash\" in below, but we may want\nto be more precise in the documentation.\n\n-- >8 --\nCodingGuidelines: spell Arithmetic Expansion with $(($var))\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/CodingGuidelines | 4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex b8bf618..2cdd76f 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -43,6 +43,10 @@ For shell scripts specifically (not exhaustive):\n \n - We use Arithmetic Expansion $(( ... )).\n \n+ - Inside Arithmetic Expansion, spell shell variables with $ in front\n+ of them, as some shells do not grok $((x)) while accepting $(($x))\n+ just fine (e.g. older dash).\n+\n - No \"Substring Expansion\" ${parameter:offset:length}.\n \n - No shell arrays.\n"},{"id":"151327","messageId":"AFB91AEE-2D2C-4864-A6CD-79780B68C2E9@gmail.com","threadId":"25186","inReplyTo":"7viq1xsi7a.fsf@alter.siamese.dyndns.org","subject":"Re: Re*: [PATCH] t1503: Fix arithmetic expansion syntax error when using dash","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2010-09-22T21:38:24Z","receivedAt":"2010-09-22T21:38:24Z","isPatch":true,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":"Ack. Thanks for the info and corrections.\n\njon.\n\nOn 23/09/2010, at 5:15, Junio C Hamano wrote:\n\n> Ramsay Jones writes:\n> \n>> On systems which have dash as /bin/sh, such as Ubuntu, the final\n>> test (master@{n} for various n) fails with a syntax error while\n>> processing an arithmetic expansion. The syntax error is caused by\n>> using a bare name ('N') as a variable reference in the expression.\n>> \n>> In order to avoid the syntax error, we spell the variable reference\n>> as '$N' rather than simply 'N'.\n>> \n>> Signed-off-by: Ramsay Jones \n> \n> Thanks.\n> \n> POSIX wants shells to support both \"N\" and \"$N\" and requires them to yield\n> the same answer to $((N)) and $(($N)), but we should aim for portability\n> in a case like this, especially when the price we pay to do so is so\n> small, i.e. a few extra dollars.\n> \n> By the way, on my box, I get this:\n> \n> $ ls l /bin/dash\n> -rwxr-xr-x 1 root root 104024 2008-08-26 02:36 /bin/dash*\n> $ dpkg -l dash | grep '^ii'\n> ii dash 0.5.4-12 POSIX-compliant shell\n> $ /bin/dash -c 'N=20 ; echo $(( N + 3 ))'\n> 23\n> \n> I just left it vague by saying \"e.g. older dash\" in below, but we may want\n> to be more precise in the documentation.\n> \n> -- >8 --\n> CodingGuidelines: spell Arithmetic Expansion with $(($var))\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> Documentation/CodingGuidelines | 4 ++++\n> 1 files changed, 4 insertions(+), 0 deletions(-)\n> \n> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> index b8bf618..2cdd76f 100644\n> --- a/Documentation/CodingGuidelines\n> +++ b/Documentation/CodingGuidelines\n> @@ -43,6 +43,10 @@ For shell scripts specifically (not exhaustive):\n> \n> - We use Arithmetic Expansion $(( ... )).\n> \n> + - Inside Arithmetic Expansion, spell shell variables with $ in front\n> + of them, as some shells do not grok $((x)) while accepting $(($x))\n> + just fine (e.g. older dash).\n> +\n> - No \"Substring Expansion\" ${parameter:offset:length}.\n> \n> - No shell arrays.\n"},{"id":"151658","messageId":"4C9E2CA6.2070805@ramsay1.demon.co.uk","threadId":"25186","inReplyTo":"7viq1xsi7a.fsf@alter.siamese.dyndns.org","subject":"Re: Re*: [PATCH] t1503: Fix arithmetic expansion syntax error when using dash","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2010-09-25T17:08:54Z","receivedAt":"2010-09-25T17:08:54Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Junio C Hamano wrote:\n> POSIX wants shells to support both \"N\" and \"$N\" and requires them to yield\n> the same answer to $((N)) and $(($N)), but we should aim for portability\n> in a case like this, especially when the price we pay to do so is so\n> small, i.e. a few extra dollars.\n\nIndeed\n\n> By the way, on my box, I get this:\n> \n> $ ls l /bin/dash\n> -rwxr-xr-x 1 root root 104024 2008-08-26 02:36 /bin/dash*\n> $ dpkg -l dash | grep '^ii'\n> ii dash 0.5.4-12 POSIX-compliant shell\n> $ /bin/dash -c 'N=20 ; echo $(( N + 3 ))'\n> 23\n\nAh, yes, I should have checked for this... particularly since I now\nvaguely remember reading that this had been \"fixed\"... *blush*\nSorry about that.\n\nFor the record, on my system I get:\n\n $ ls -l /bin/dash\n -rwxr-xr-x 1 root root 80500 2007-03-05 06:00 /bin/dash*\n $ dpkg -l dash | grep '^ii'\n ii dash 0.5.3-5ubuntu2 The Debian Almquist Shell\n $ /bin/dash -c 'N=20; echo $(( N + 3 ))'\n /bin/dash: arith: syntax error: \" N + 3 \"\n\n> I just left it vague by saying \"e.g. older dash\" in below, but we may want\n> to be more precise in the documentation.\n\nI found a bug report:\n\n http://bugs.launchpad.net/ubuntu/+source/dash/+bug/92189\n\nwhich had a post against it which implied that this was fixed in\nversion 0.5.4-3. I went over to packages.debian.org to read the\nChangeLog for this version, but I could not conclude anything\nfrom that text. :(\n\nDo we need to be more precise?\n\nShould I re-work the commit message and re-submit?\n\nATB,\nRamsay Jones\n"},{"id":"151660","messageId":"AANLkTimdUC++Lq1z4i5E8cw5ej7nQhZ6hcNoffQ1tM9i@mail.gmail.com","threadId":"25186","inReplyTo":"4C9E2CA6.2070805@ramsay1.demon.co.uk","subject":"Re: Re*: [PATCH] t1503: Fix arithmetic expansion syntax error when using dash","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-25T17:24:05Z","receivedAt":"2010-09-25T17:24:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sat, Sep 25, 2010 at 17:08, Ramsay Jones wrote:\n> Junio C Hamano wrote:\n>> POSIX wants shells to support both \"N\" and \"$N\" and requires them to yield\n>> the same answer to $((N)) and $(($N)), but we should aim for portability\n>> in a case like this, especially when the price we pay to do so is so\n>> small, i.e. a few extra dollars.\n>\n> Indeed\n>\n>> By the way, on my box, I get this:\n>>\n>>     $ ls l /bin/dash\n>>     -rwxr-xr-x 1 root root 104024 2008-08-26 02:36 /bin/dash*\n>>     $ dpkg -l dash | grep '^ii'\n>>     ii  dash              0.5.4-12          POSIX-compliant shell\n>>     $ /bin/dash -c 'N=20 ; echo $(( N + 3 ))'\n>>     23\n>\n> Ah, yes, I should have checked for this... particularly since I now\n> vaguely remember reading that this had been \"fixed\"... *blush*\n> Sorry about that.\n>\n> For the record, on my system I get:\n>\n>    $ ls -l /bin/dash\n>    -rwxr-xr-x 1 root root 80500 2007-03-05 06:00 /bin/dash*\n>    $ dpkg -l dash | grep '^ii'\n>    ii  dash           0.5.3-5ubuntu2 The Debian Almquist Shell\n>    $ /bin/dash -c 'N=20; echo $(( N + 3 ))'\n>    /bin/dash: arith: syntax error: \" N + 3 \"\n>\n>> I just left it vague by saying \"e.g. older dash\" in below, but we may want\n>> to be more precise in the documentation.\n>\n> I found a bug report:\n>\n>    http://bugs.launchpad.net/ubuntu/+source/dash/+bug/92189\n>\n> which had a post against it which implied that this was fixed in\n> version 0.5.4-3. I went over to packages.debian.org to read the\n> ChangeLog for this version, but I could not conclude anything\n> from that text. :(\n>\n> Do we need to be more precise?\n\nIf you want to spend the effort to track it down that would be\ngreat. There's a dash git repository on kernel.org you can probably\nbisect:\n\n http://git.kernel.org/?p=utils/dash/dash.git;a=summary\n"},{"id":"151662","messageId":"7vbp7logcg.fsf@alter.siamese.dyndns.org","threadId":"25186","inReplyTo":"4C9E2CA6.2070805@ramsay1.demon.co.uk","subject":"Re: Re*: [PATCH] t1503: Fix arithmetic expansion syntax error when using dash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-09-25T17:58:23Z","receivedAt":"2010-09-25T17:58:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> Junio C Hamano wrote:\n>> POSIX wants shells to support both \"N\" and \"$N\" and requires them to yield\n>> the same answer to $((N)) and $(($N)), but we should aim for portability\n>> in a case like this, especially when the price we pay to do so is so\n>> small, i.e. a few extra dollars.\n>\n> Indeed\n>\n>> By the way, on my box, I get this:\n>> \n>> $ ls l /bin/dash\n>> -rwxr-xr-x 1 root root 104024 2008-08-26 02:36 /bin/dash*\n>> $ dpkg -l dash | grep '^ii'\n>> ii dash 0.5.4-12 POSIX-compliant shell\n>> $ /bin/dash -c 'N=20 ; echo $(( N + 3 ))'\n>> 23\n>\n> Ah, yes, I should have checked for this... particularly since I now\n> vaguely remember reading that this had been \"fixed\"... *blush*\n> Sorry about that.\n>\n> For the record, on my system I get:\n>\n> $ ls -l /bin/dash\n> -rwxr-xr-x 1 root root 80500 2007-03-05 06:00 /bin/dash*\n> $ dpkg -l dash | grep '^ii'\n> ii dash 0.5.3-5ubuntu2 The Debian Almquist Shell\n> $ /bin/dash -c 'N=20; echo $(( N + 3 ))'\n> /bin/dash: arith: syntax error: \" N + 3 \"\n>\n>> I just left it vague by saying \"e.g. older dash\" in below, but we may want\n>> to be more precise in the documentation.\n>\n> I found a bug report:\n>\n> http://bugs.launchpad.net/ubuntu/+source/dash/+bug/92189\n>\n> which had a post against it which implied that this was fixed in\n> version 0.5.4-3. I went over to packages.debian.org to read the\n> ChangeLog for this version, but I could not conclude anything\n> from that text. :(\n>\n> Do we need to be more precise?\n>\n> Should I re-work the commit message and re-submit?\n\nI don't think so; we now know that dash 0.5.3 or older may have this\nproblem and that is clear enough for our purpose.\n\nThanks.\n"},{"id":"152178","messageId":"4CA39A34.4030909@ramsay1.demon.co.uk","threadId":"25186","inReplyTo":"AANLkTimdUC++Lq1z4i5E8cw5ej7nQhZ6hcNoffQ1tM9i@mail.gmail.com","subject":"Re: Re*: [PATCH] t1503: Fix arithmetic expansion syntax error when using dash","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2010-09-29T19:57:40Z","receivedAt":"2010-09-29T19:57:40Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> On Sat, Sep 25, 2010 at 17:08, Ramsay Jones wrote:\n>> Junio C Hamano wrote:\n>>> I just left it vague by saying \"e.g. older dash\" in below, but we may want\n>>> to be more precise in the documentation.\n>> I found a bug report:\n>>\n>> http://bugs.launchpad.net/ubuntu/+source/dash/+bug/92189\n>>\n>> which had a post against it which implied that this was fixed in\n>> version 0.5.4-3. I went over to packages.debian.org to read the\n>> ChangeLog for this version, but I could not conclude anything\n>> from that text. :(\n>>\n>> Do we need to be more precise?\n> \n> If you want to spend the effort to track it down that would be\n> great. There's a dash git repository on kernel.org you can probably\n> bisect:\n> \n> http://git.kernel.org/?p=utils/dash/dash.git;a=summary\n\nI don't think we need to be more precise here, but just for the\ngiggle, I cloned the dash repository to take a quick look.\n\nIt was actually very easy to spot the commit that adds the missing\ncapability; I fired up gitk and (starting from v0.5.3) just scanned\nthe commits \"upward\" until I found the culprit ;-) Having said that,\nI would not have found it just by reading the commit message; I had\nto read the \"patch\" text to discover the \"fix\", which I suppose\ncould have been an unintended side effect! :-D\n\nBTW the answer is: commit f6e3b2f8a59922405f42c8bc283e0f5546c25d0e\nor, if you prefer:\n\n $ git describe --tags f6e3b2f8\n v0.5.4-26-gf6e3b2f\n\n[But this does not help too much in identifying the downstream\nversion(s) from, say, the debian project.]\n\nI decided to actually perform a git-bisect to confirm that I had\nactually found the correct commit. I used a script (see below) to\nbuild and test dash, so that I could use \"git bisect run ./test.sh\"\nto find it automatically. (The script may seem to return backward\nresults, but we need to consider a version that *does* support the\nextended syntax to be a *bad* commit for the purposes of bisect! ;-) )\n\nA transcript of the git-bisect run is given below.\n\nATB,\nRamsay Jones\n\n$ git tag -l\nv0.5.2\nv0.5.3\nv0.5.4\nv0.5.5\nv0.5.5.1\nv0.5.6\nv0.5.6.1\n\n$ cat -n test.sh\n 1\t#!/bin/sh\n 2\t\n 3\tmake clean >/dev/null 2>&1\n 4\tmake >make-out 2>&1 || { echo CANNOT BUILD; exit 125; }\n 5\t\n 6\tif src/dash -c 'N=20; echo $(( N + 3))'\n 7\tthen\n 8\t\techo \"--- works OK => BAD ---\"\n 9\t\texit 1;\n 10\telse\n 11\t\techo \"--- syntax error => GOOD ---\"\n 12\t\texit 0;\n 13\tfi\n\n$ git bisect start v0.5.5 v0.5.3\nBisecting: 41 revisions left to test after this (roughly 5 steps)\n[aa82f69dea2f2d5fe4337dfb12cea54fabdab175] [BUILTIN] Use intmax_t arithmetic in test\n\n$ git bisect run ./test.sh\nrunning ./test.sh\nsrc/dash: arith: syntax error: \" N + 3\"\n--- syntax error => GOOD ---\nBisecting: 20 revisions left to test after this (roughly 4 steps)\n[f0f930d60cd62f5fe5ba28460b43f333e8062b94] [CD] Restored non-glibc getcwd support\nrunning ./test.sh\n23\n--- works OK => BAD ---\nBisecting: 10 revisions left to test after this (roughly 3 steps)\n[d39c8628b8594c29d234427ba07a12538ab36f41] [PARSER] Fix here-doc corruption\nrunning ./test.sh\n23\n--- works OK => BAD ---\nBisecting: 4 revisions left to test after this (roughly 2 steps)\n[7454c1e3b90f51a49e563323db38bafa50776533] [BUILTIN] Use setvarint to set OPTIND\nrunning ./test.sh\n23\n--- works OK => BAD ---\nBisecting: 2 revisions left to test after this (roughly 1 step)\n[f6e3b2f8a59922405f42c8bc283e0f5546c25d0e] [ARITH] Add assignment and intmax_t support\nrunning ./test.sh\n23\n--- works OK => BAD ---\nBisecting: 0 revisions left to test after this (roughly 0 steps)\n[3df3edd13389ae768010bfacee5612346b413e38] [PARSER] Report substition errors at expansion time\nrunning ./test.sh\nsrc/dash: arith: syntax error: \" N + 3\"\n--- syntax error => GOOD ---\nf6e3b2f8a59922405f42c8bc283e0f5546c25d0e is the first bad commit\ncommit f6e3b2f8a59922405f42c8bc283e0f5546c25d0e\nAuthor: Herbert Xu \nDate: Thu Oct 11 22:36:28 2007 +0800\n\n [ARITH] Add assignment and intmax_t support\n \n This patch adds assignment operator support in arithmetic expansions. It\n also changes the type used to intmax_t.\n\n:100644 100644 69ba464a3219d6979aa1150c15e279d3adf423ac 895c6072294f13f2a434c52752dc839afd412c0b M\tChangeLog\n:040000 040000 6640114c4a8d35cbcfaf6fa44b888195270a3ae1 06a80af3e532959cddd422f921e66cb679fc8760 M\tsrc\nbisect run success\n\n$ git describe --tags f6e3b2f8\nv0.5.4-26-gf6e3b2f\n\n$ \n"}]}