# [PATCH] t1503: Fix arithmetic expansion syntax error when using dash 7 messages from 2010-09-21 to 2010-09-29. Participants: Ramsay Jones, Junio C Hamano, Jon Seymour, Ævar Arnfjörð Bjarmason. Thread: https://gitlist.dev/t/25186 ## Ramsay Jones, 2010-09-21 17:45 Subject: [PATCH] t1503: Fix arithmetic expansion syntax error when using dash Message-ID: <4C98EF25.4070700@ramsay1.demon.co.uk> URL: https://gitlist.dev/e/4C98EF25.4070700%40ramsay1.demon.co.uk ``` On systems which have dash as /bin/sh, such as Ubuntu, the final test (master@{n} for various n) fails with a syntax error while processing an arithmetic expansion. The syntax error is caused by using a bare name ('N') as a variable reference in the expression. In order to avoid the syntax error, we spell the variable reference as '$N' rather than simply 'N'. Signed-off-by: Ramsay Jones --- t/t1503-rev-parse-verify.sh | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh index 100f857..813cc1b 100755 --- a/t/t1503-rev-parse-verify.sh +++ b/t/t1503-rev-parse-verify.sh @@ -106,8 +106,8 @@ test_expect_success 'use --default' ' test_expect_success 'master@{n} for various n' ' N=$(git reflog | wc -l) && - Nm1=$((N-1)) && - Np1=$((N+1)) && + Nm1=$(($N-1)) && + Np1=$(($N+1)) && git rev-parse --verify master@{0} && git rev-parse --verify master@{1} && git rev-parse --verify master@{$Nm1} && -- 1.7.3 ``` ## Junio C Hamano, 2010-09-22 19:15 Subject: Re*: [PATCH] t1503: Fix arithmetic expansion syntax error when using dash Message-ID: <7viq1xsi7a.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7viq1xsi7a.fsf%40alter.siamese.dyndns.org In-Reply-To: <4C98EF25.4070700@ramsay1.demon.co.uk> ``` Ramsay Jones writes: > On systems which have dash as /bin/sh, such as Ubuntu, the final > test (master@{n} for various n) fails with a syntax error while > processing an arithmetic expansion. The syntax error is caused by > using a bare name ('N') as a variable reference in the expression. > > In order to avoid the syntax error, we spell the variable reference > as '$N' rather than simply 'N'. > > Signed-off-by: Ramsay Jones Thanks. POSIX wants shells to support both "N" and "$N" and requires them to yield the same answer to $((N)) and $(($N)), but we should aim for portability in a case like this, especially when the price we pay to do so is so small, i.e. a few extra dollars. By the way, on my box, I get this: $ ls l /bin/dash -rwxr-xr-x 1 root root 104024 2008-08-26 02:36 /bin/dash* $ dpkg -l dash | grep '^ii' ii dash 0.5.4-12 POSIX-compliant shell $ /bin/dash -c 'N=20 ; echo $(( N + 3 ))' 23 I just left it vague by saying "e.g. older dash" in below, but we may want to be more precise in the documentation. -- >8 -- CodingGuidelines: spell Arithmetic Expansion with $(($var)) Signed-off-by: Junio C Hamano --- Documentation/CodingGuidelines | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines index b8bf618..2cdd76f 100644 --- a/Documentation/CodingGuidelines +++ b/Documentation/CodingGuidelines @@ -43,6 +43,10 @@ For shell scripts specifically (not exhaustive): - We use Arithmetic Expansion $(( ... )). + - Inside Arithmetic Expansion, spell shell variables with $ in front + of them, as some shells do not grok $((x)) while accepting $(($x)) + just fine (e.g. older dash). + - No "Substring Expansion" ${parameter:offset:length}. - No shell arrays. ``` ## Jon Seymour, 2010-09-22 21:38 Subject: Re: Re*: [PATCH] t1503: Fix arithmetic expansion syntax error when using dash Message-ID: URL: https://gitlist.dev/e/AFB91AEE-2D2C-4864-A6CD-79780B68C2E9%40gmail.com In-Reply-To: <7viq1xsi7a.fsf@alter.siamese.dyndns.org> ``` Ack. Thanks for the info and corrections. jon. On 23/09/2010, at 5:15, Junio C Hamano wrote: > Ramsay Jones writes: > >> On systems which have dash as /bin/sh, such as Ubuntu, the final >> test (master@{n} for various n) fails with a syntax error while >> processing an arithmetic expansion. The syntax error is caused by >> using a bare name ('N') as a variable reference in the expression. >> >> In order to avoid the syntax error, we spell the variable reference >> as '$N' rather than simply 'N'. >> >> Signed-off-by: Ramsay Jones > > Thanks. > > POSIX wants shells to support both "N" and "$N" and requires them to yield > the same answer to $((N)) and $(($N)), but we should aim for portability > in a case like this, especially when the price we pay to do so is so > small, i.e. a few extra dollars. > > By the way, on my box, I get this: > > $ ls l /bin/dash > -rwxr-xr-x 1 root root 104024 2008-08-26 02:36 /bin/dash* > $ dpkg -l dash | grep '^ii' > ii dash 0.5.4-12 POSIX-compliant shell > $ /bin/dash -c 'N=20 ; echo $(( N + 3 ))' > 23 > > I just left it vague by saying "e.g. older dash" in below, but we may want > to be more precise in the documentation. > > -- >8 -- > CodingGuidelines: spell Arithmetic Expansion with $(($var)) > > Signed-off-by: Junio C Hamano > --- > Documentation/CodingGuidelines | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines > index b8bf618..2cdd76f 100644 > --- a/Documentation/CodingGuidelines > +++ b/Documentation/CodingGuidelines > @@ -43,6 +43,10 @@ For shell scripts specifically (not exhaustive): > > - We use Arithmetic Expansion $(( ... )). > > + - Inside Arithmetic Expansion, spell shell variables with $ in front > + of them, as some shells do not grok $((x)) while accepting $(($x)) > + just fine (e.g. older dash). > + > - No "Substring Expansion" ${parameter:offset:length}. > > - No shell arrays. ``` ## Ramsay Jones, 2010-09-25 17:08 Subject: Re: Re*: [PATCH] t1503: Fix arithmetic expansion syntax error when using dash Message-ID: <4C9E2CA6.2070805@ramsay1.demon.co.uk> URL: https://gitlist.dev/e/4C9E2CA6.2070805%40ramsay1.demon.co.uk In-Reply-To: <7viq1xsi7a.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano wrote: > POSIX wants shells to support both "N" and "$N" and requires them to yield > the same answer to $((N)) and $(($N)), but we should aim for portability > in a case like this, especially when the price we pay to do so is so > small, i.e. a few extra dollars. Indeed > By the way, on my box, I get this: > > $ ls l /bin/dash > -rwxr-xr-x 1 root root 104024 2008-08-26 02:36 /bin/dash* > $ dpkg -l dash | grep '^ii' > ii dash 0.5.4-12 POSIX-compliant shell > $ /bin/dash -c 'N=20 ; echo $(( N + 3 ))' > 23 Ah, yes, I should have checked for this... particularly since I now vaguely remember reading that this had been "fixed"... *blush* Sorry about that. For the record, on my system I get: $ ls -l /bin/dash -rwxr-xr-x 1 root root 80500 2007-03-05 06:00 /bin/dash* $ dpkg -l dash | grep '^ii' ii dash 0.5.3-5ubuntu2 The Debian Almquist Shell $ /bin/dash -c 'N=20; echo $(( N + 3 ))' /bin/dash: arith: syntax error: " N + 3 " > I just left it vague by saying "e.g. older dash" in below, but we may want > to be more precise in the documentation. I found a bug report: http://bugs.launchpad.net/ubuntu/+source/dash/+bug/92189 which had a post against it which implied that this was fixed in version 0.5.4-3. I went over to packages.debian.org to read the ChangeLog for this version, but I could not conclude anything from that text. :( Do we need to be more precise? Should I re-work the commit message and re-submit? ATB, Ramsay Jones ``` ## Ævar Arnfjörð Bjarmason, 2010-09-25 17:24 Subject: Re: Re*: [PATCH] t1503: Fix arithmetic expansion syntax error when using dash Message-ID: URL: https://gitlist.dev/e/AANLkTimdUC%2B%2BLq1z4i5E8cw5ej7nQhZ6hcNoffQ1tM9i%40mail.gmail.com In-Reply-To: <4C9E2CA6.2070805@ramsay1.demon.co.uk> ``` On Sat, Sep 25, 2010 at 17:08, Ramsay Jones wrote: > Junio C Hamano wrote: >> POSIX wants shells to support both "N" and "$N" and requires them to yield >> the same answer to $((N)) and $(($N)), but we should aim for portability >> in a case like this, especially when the price we pay to do so is so >> small, i.e. a few extra dollars. > > Indeed > >> By the way, on my box, I get this: >> >>     $ ls l /bin/dash >>     -rwxr-xr-x 1 root root 104024 2008-08-26 02:36 /bin/dash* >>     $ dpkg -l dash | grep '^ii' >>     ii  dash              0.5.4-12          POSIX-compliant shell >>     $ /bin/dash -c 'N=20 ; echo $(( N + 3 ))' >>     23 > > Ah, yes, I should have checked for this... particularly since I now > vaguely remember reading that this had been "fixed"... *blush* > Sorry about that. > > For the record, on my system I get: > >    $ ls -l /bin/dash >    -rwxr-xr-x 1 root root 80500 2007-03-05 06:00 /bin/dash* >    $ dpkg -l dash | grep '^ii' >    ii  dash           0.5.3-5ubuntu2 The Debian Almquist Shell >    $ /bin/dash -c 'N=20; echo $(( N + 3 ))' >    /bin/dash: arith: syntax error: " N + 3 " > >> I just left it vague by saying "e.g. older dash" in below, but we may want >> to be more precise in the documentation. > > I found a bug report: > >    http://bugs.launchpad.net/ubuntu/+source/dash/+bug/92189 > > which had a post against it which implied that this was fixed in > version 0.5.4-3. I went over to packages.debian.org to read the > ChangeLog for this version, but I could not conclude anything > from that text. :( > > Do we need to be more precise? If you want to spend the effort to track it down that would be great. There's a dash git repository on kernel.org you can probably bisect: http://git.kernel.org/?p=utils/dash/dash.git;a=summary ``` ## Junio C Hamano, 2010-09-25 17:58 Subject: Re: Re*: [PATCH] t1503: Fix arithmetic expansion syntax error when using dash Message-ID: <7vbp7logcg.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vbp7logcg.fsf%40alter.siamese.dyndns.org In-Reply-To: <4C9E2CA6.2070805@ramsay1.demon.co.uk> ``` Ramsay Jones writes: > Junio C Hamano wrote: >> POSIX wants shells to support both "N" and "$N" and requires them to yield >> the same answer to $((N)) and $(($N)), but we should aim for portability >> in a case like this, especially when the price we pay to do so is so >> small, i.e. a few extra dollars. > > Indeed > >> By the way, on my box, I get this: >> >> $ ls l /bin/dash >> -rwxr-xr-x 1 root root 104024 2008-08-26 02:36 /bin/dash* >> $ dpkg -l dash | grep '^ii' >> ii dash 0.5.4-12 POSIX-compliant shell >> $ /bin/dash -c 'N=20 ; echo $(( N + 3 ))' >> 23 > > Ah, yes, I should have checked for this... particularly since I now > vaguely remember reading that this had been "fixed"... *blush* > Sorry about that. > > For the record, on my system I get: > > $ ls -l /bin/dash > -rwxr-xr-x 1 root root 80500 2007-03-05 06:00 /bin/dash* > $ dpkg -l dash | grep '^ii' > ii dash 0.5.3-5ubuntu2 The Debian Almquist Shell > $ /bin/dash -c 'N=20; echo $(( N + 3 ))' > /bin/dash: arith: syntax error: " N + 3 " > >> I just left it vague by saying "e.g. older dash" in below, but we may want >> to be more precise in the documentation. > > I found a bug report: > > http://bugs.launchpad.net/ubuntu/+source/dash/+bug/92189 > > which had a post against it which implied that this was fixed in > version 0.5.4-3. I went over to packages.debian.org to read the > ChangeLog for this version, but I could not conclude anything > from that text. :( > > Do we need to be more precise? > > Should I re-work the commit message and re-submit? I don't think so; we now know that dash 0.5.3 or older may have this problem and that is clear enough for our purpose. Thanks. ``` ## Ramsay Jones, 2010-09-29 19:57 Subject: Re: Re*: [PATCH] t1503: Fix arithmetic expansion syntax error when using dash Message-ID: <4CA39A34.4030909@ramsay1.demon.co.uk> URL: https://gitlist.dev/e/4CA39A34.4030909%40ramsay1.demon.co.uk In-Reply-To: ``` Ævar Arnfjörð Bjarmason wrote: > On Sat, Sep 25, 2010 at 17:08, Ramsay Jones wrote: >> Junio C Hamano wrote: >>> I just left it vague by saying "e.g. older dash" in below, but we may want >>> to be more precise in the documentation. >> I found a bug report: >> >> http://bugs.launchpad.net/ubuntu/+source/dash/+bug/92189 >> >> which had a post against it which implied that this was fixed in >> version 0.5.4-3. I went over to packages.debian.org to read the >> ChangeLog for this version, but I could not conclude anything >> from that text. :( >> >> Do we need to be more precise? > > If you want to spend the effort to track it down that would be > great. There's a dash git repository on kernel.org you can probably > bisect: > > http://git.kernel.org/?p=utils/dash/dash.git;a=summary I don't think we need to be more precise here, but just for the giggle, I cloned the dash repository to take a quick look. It was actually very easy to spot the commit that adds the missing capability; I fired up gitk and (starting from v0.5.3) just scanned the commits "upward" until I found the culprit ;-) Having said that, I would not have found it just by reading the commit message; I had to read the "patch" text to discover the "fix", which I suppose could have been an unintended side effect! :-D BTW the answer is: commit f6e3b2f8a59922405f42c8bc283e0f5546c25d0e or, if you prefer: $ git describe --tags f6e3b2f8 v0.5.4-26-gf6e3b2f [But this does not help too much in identifying the downstream version(s) from, say, the debian project.] I decided to actually perform a git-bisect to confirm that I had actually found the correct commit. I used a script (see below) to build and test dash, so that I could use "git bisect run ./test.sh" to find it automatically. (The script may seem to return backward results, but we need to consider a version that *does* support the extended syntax to be a *bad* commit for the purposes of bisect! ;-) ) A transcript of the git-bisect run is given below. ATB, Ramsay Jones $ git tag -l v0.5.2 v0.5.3 v0.5.4 v0.5.5 v0.5.5.1 v0.5.6 v0.5.6.1 $ cat -n test.sh 1 #!/bin/sh 2 3 make clean >/dev/null 2>&1 4 make >make-out 2>&1 || { echo CANNOT BUILD; exit 125; } 5 6 if src/dash -c 'N=20; echo $(( N + 3))' 7 then 8 echo "--- works OK => BAD ---" 9 exit 1; 10 else 11 echo "--- syntax error => GOOD ---" 12 exit 0; 13 fi $ git bisect start v0.5.5 v0.5.3 Bisecting: 41 revisions left to test after this (roughly 5 steps) [aa82f69dea2f2d5fe4337dfb12cea54fabdab175] [BUILTIN] Use intmax_t arithmetic in test $ git bisect run ./test.sh running ./test.sh src/dash: arith: syntax error: " N + 3" --- syntax error => GOOD --- Bisecting: 20 revisions left to test after this (roughly 4 steps) [f0f930d60cd62f5fe5ba28460b43f333e8062b94] [CD] Restored non-glibc getcwd support running ./test.sh 23 --- works OK => BAD --- Bisecting: 10 revisions left to test after this (roughly 3 steps) [d39c8628b8594c29d234427ba07a12538ab36f41] [PARSER] Fix here-doc corruption running ./test.sh 23 --- works OK => BAD --- Bisecting: 4 revisions left to test after this (roughly 2 steps) [7454c1e3b90f51a49e563323db38bafa50776533] [BUILTIN] Use setvarint to set OPTIND running ./test.sh 23 --- works OK => BAD --- Bisecting: 2 revisions left to test after this (roughly 1 step) [f6e3b2f8a59922405f42c8bc283e0f5546c25d0e] [ARITH] Add assignment and intmax_t support running ./test.sh 23 --- works OK => BAD --- Bisecting: 0 revisions left to test after this (roughly 0 steps) [3df3edd13389ae768010bfacee5612346b413e38] [PARSER] Report substition errors at expansion time running ./test.sh src/dash: arith: syntax error: " N + 3" --- syntax error => GOOD --- f6e3b2f8a59922405f42c8bc283e0f5546c25d0e is the first bad commit commit f6e3b2f8a59922405f42c8bc283e0f5546c25d0e Author: Herbert Xu Date: Thu Oct 11 22:36:28 2007 +0800 [ARITH] Add assignment and intmax_t support This patch adds assignment operator support in arithmetic expansions. It also changes the type used to intmax_t. :100644 100644 69ba464a3219d6979aa1150c15e279d3adf423ac 895c6072294f13f2a434c52752dc839afd412c0b M ChangeLog :040000 040000 6640114c4a8d35cbcfaf6fa44b888195270a3ae1 06a80af3e532959cddd422f921e66cb679fc8760 M src bisect run success $ git describe --tags f6e3b2f8 v0.5.4-26-gf6e3b2f $ ```