[PATCH] t1503: Fix arithmetic expansion syntax error when using dash

[PATCH] t1503: Fix arithmetic expansion syntax error when using dash

[Ramsay Jones]

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 <ramsay@ramsay1.demon.co.uk>
---
 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

Re*: [PATCH] t1503: Fix arithmetic expansion syntax error when using dash

[Junio C Hamano]

Ramsay Jones <ramsay@ramsay1.demon.co.uk> 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 <ramsay@ramsay1.demon.co.uk>

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 <gitster@pobox.com>
---
 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.

Re: Re*: [PATCH] t1503: Fix arithmetic expansion syntax error when using dash

[Jon Seymour]

Ack. Thanks for the info and corrections.

jon.

On 23/09/2010, at 5:15, Junio C Hamano <gitster@pobox.com> wrote:

> Ramsay Jones <ramsay@ramsay1.demon.co.uk> 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 <ramsay@ramsay1.demon.co.uk>
> 
> 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 <gitster@pobox.com>
> ---
> 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.

Re: Re*: [PATCH] t1503: Fix arithmetic expansion syntax error when using dash

[Ramsay Jones]

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

Re: Re*: [PATCH] t1503: Fix arithmetic expansion syntax error when using dash

[Ævar Arnfjörð Bjarmason]

On Sat, Sep 25, 2010 at 17:08, Ramsay Jones <ramsay@ramsay1.demon.co.uk> 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

Re: Re*: [PATCH] t1503: Fix arithmetic expansion syntax error when using dash

[Junio C Hamano]

Ramsay Jones <ramsay@ramsay1.demon.co.uk> 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.

Re: Re*: [PATCH] t1503: Fix arithmetic expansion syntax error when using dash

[Ramsay Jones]

Ævar Arnfjörð Bjarmason wrote:
> On Sat, Sep 25, 2010 at 17:08, Ramsay Jones <ramsay@ramsay1.demon.co.uk> 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 <herbert@gondor.apana.org.au>
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

$