Re: Error status of "repeat" (was Re: [PATCH] typeset: set $? on incidental error)

[Daniel Shahaf <d.s@daniel.shahaf.name>]

Peter Stephenson wrote on Wed, Jan 27, 2016 at 09:52:12 +0000:
> On Tue, 26 Jan 2016 20:15:13 -0800
> Bart Schaefer <schaefer@brasslantern.com> wrote:
> > On Jan 26, 10:50pm, Daniel Shahaf wrote:
> > }
> > } What would you expect "repeat 0+++ (exit 42)" to set $? to?
> >...
> > diff --git a/Src/loop.c b/Src/loop.c
> > index 4def9b6..61dad09 100644
> > --- a/Src/loop.c
> > +++ b/Src/loop.c
> > @@ -493,7 +493,7 @@ execrepeat(Estate state, UNUSED(int do_exec))
> >      tmp = ecgetstr(state, EC_DUPTOK, &htok);
> >      if (htok)
> >  	singsub(&tmp);
> > -    count = atoi(tmp);
> > +    count = mathevali(tmp);
> >      pushheap();
> >      cmdpush(CS_REPEAT);
> >      loops++;
> 
> As it's documented that this is how it works, and it's hard to see how
> it can break anything that already works since that will have to be an
> integer at this point, that looks OK.

I won't say "break", but here are some syntaxes that will now have
a different meaning:

    setopt octalzeroes
    repeat 010 foo

    setopt NO_cbases
    integer N=$(( [#16] 100 ))
    repeat $N foo

    setopt cbases
    integer N=$(( [#16] 100 ))
    repeat $N foo

    typeset total=1,500
    repeat $total foo

    # the cmdsubst evaluates to "42 total"
    repeat $(wc -l foo.c bar.c | tail -n1) foo

    repeat 2016-01-29 foo

    repeat 5.999999999999999999999 foo

The existing code would find
	10, 16, 0, 1, 42, 2016, 5
The new code would find
	8, 100, 100, 500, error, 1986, 6
(The second and third chop the string off at the '#' ox 'x'.  The fourth
one is a comma operator.  The sixth one is a subtraction.  The seventh one
gets rounded.)

---

While at it, here are a few syntaxes that don't change behaviour:

    repeat -42 foo
    # Should this be a usage error?

    repeat inf foo
    # I hoped this one would spell an infinite loop (per strtod(3)).

    repeat +5 foo
    # Works both before and after the patch, same as 'repeat 5 foo'.

---

Should we add a NEWS entry for this?  I realize it's not likely to
affect many people, but on the other hand it is an incompatibility and
we might want to err on the side of caution and mention it:

    diff --git a/NEWS b/NEWS
    index 15822ad..3568dc8 100644
    --- a/NEWS
    +++ b/NEWS
    @@ -4,5 +4,13 @@ CHANGES FROM PREVIOUS VERSIONS OF ZSH
     
     Note also the list of incompatibilities in the README file.
     
    +Changes from 5.2 to 5.3
    +-----------------------
    +
    +The first argument to 'repeat' is now evaluated as an arithmetic
    +expression.  It was always documented to be an arithmetic expression,
    +but until now the decimal integer at the start of the value was used
    +and the remainder of the value discarded.
    +
     Changes from 5.1.1 to 5.2
     -------------------------

Cheers,

Daniel

P.S. If anyone ever looks back on this mail to find corner cases, here's
another one: 9007199254740993.0.  That number is exactly representable
as an integer (if atoi()'s return type is a 64-bit type) but not as
a double, so the round-trip (casting an integer to double and back) will
be lossy.