PATCH: minor condition fixes

PATCH: minor condition fixes

[Peter Stephenson]

I noticed the condition code is output too many "-"s when it doesn't
recognised a test.  Bizarrely, I copied this error into the test code
without noticing:  it's complaining about a non-existent "-foo" when
the failure was on "-fail foo".

Looking further, it's doing a bad job of guessing what the condition it
doesn't know is (even ignoring the misplaced '-'):

% [[ -fail badly ]]
zsh: unknown condition: -badly
% [[ really -fail badly ]]
zsh: unknown condition: -really

There's no point trying too hard if it couldn't parse it properly
anyway, but it would be sensible to guess that something with a "-" is
the unknown condition, and it might as well be the first one we come
across.

While looking I noticed that that first getconddef() you can see is
checking name + 1 without ever looking at name[0]; presumably it should be
ensuring name[0] is a '-'?

Then there's a problem I haven't fixed, which is for some reason in the
test code the status is returning as 1 for an unrecognised condition
whereas it should return 2 (and does at the command line).  This shows
up with:

% eval '[[ -fail badly ]]'
zsh: unknown condition: -fail
% print $?
1

Index: Src/cond.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/cond.c,v
retrieving revision 1.15
diff -p -u -r1.15 cond.c
--- Src/cond.c	11 May 2008 19:55:21 -0000	1.15
+++ Src/cond.c	19 Feb 2010 11:56:20 -0000
@@ -104,7 +104,7 @@ evalcond(Estate state, char *fromtest)
     case COND_MODI:
 	{
 	    Conddef cd;
-	    char *name = overridename;
+	    char *name = overridename, *errname;
 	    char **strs;
 	    int l = WC_COND_SKIP(code);
 
@@ -122,10 +122,17 @@ evalcond(Estate state, char *fromtest)
 		strs = arrdup(sbuf);
 		l = 2;
 	    }
-	    if ((cd = getconddef((ctype == COND_MODI), name + 1, 1))) {
+	    if (name && name[0] == '-')
+		errname = name;
+	    else if (strs[0] && *strs[0] == '-')
+		errname = strs[0];
+	    else
+		errname = "<null>";
+	    if (name && name[0] == '-' &&
+		(cd = getconddef((ctype == COND_MODI), name + 1, 1))) {
 		if (ctype == COND_MOD &&
 		    (l < cd->min || (cd->max >= 0 && l > cd->max))) {
-		    zwarnnam(fromtest, "unknown condition: -%s", name);
+		    zwarnnam(fromtest, "unknown condition: %s", name);
 		    return 2;
 		}
 		if (tracingcond)
@@ -151,8 +158,8 @@ evalcond(Estate state, char *fromtest)
 		if (name && name[0] == '-' &&
 		    (cd = getconddef(0, name + 1, 1))) {
 		    if (l < cd->min || (cd->max >= 0 && l > cd->max)) {
-			zwarnnam(fromtest, "unknown condition: -%s",
-				 name);
+			zwarnnam(fromtest, "unknown condition: %s",
+				 errname);
 			return 2;
 		    }
 		    if (tracingcond)
@@ -160,8 +167,8 @@ evalcond(Estate state, char *fromtest)
 		    return !cd->handler(strs, cd->condid);
 		} else {
 		    zwarnnam(fromtest,
-			     "unknown condition: -%s",
-			     name ? name : "<null>");
+			     "unknown condition: %s",
+			     errname);
 		}
 	    }
 	    /* module not found, error */
Index: Test/C02cond.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/C02cond.ztst,v
retrieving revision 1.25
diff -p -u -r1.25 C02cond.ztst
--- Test/C02cond.ztst	20 Jan 2010 11:17:13 -0000	1.25
+++ Test/C02cond.ztst	19 Feb 2010 11:56:20 -0000
@@ -285,6 +285,14 @@ F:Failures in these cases do not indicat
 0:MATCH, MBEGIN, MEND, match, mbegin, mend
 >OK
 
+  [[ -fail badly ]]
+1:Error message for unknown prefix condition
+?(eval):1: unknown condition: -fail
+
+  [[ really -fail badly ]]
+1:Error message for unknown infix condition
+?(eval):1: unknown condition: -fail
+
 %clean
   # This works around a bug in rm -f in some versions of Cygwin
   chmod 644 unmodish
Index: Test/V01zmodload.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/V01zmodload.ztst,v
retrieving revision 1.13
diff -p -u -r1.13 V01zmodload.ztst
--- Test/V01zmodload.ztst	13 Aug 2008 21:07:03 -0000	1.13
+++ Test/V01zmodload.ztst	19 Feb 2010 11:56:20 -0000
@@ -151,7 +151,7 @@
   print "Shouldn't get here.")
 2:Failed condition autoload
 ?(eval):3: module `zsh/parameter' has no such feature: `c:fail': autoload cancelled
-?(eval):3: unknown condition: -foo
+?(eval):3: unknown condition: -fail
 
   (zmodload -u zsh/parameter
   zmodload -aF zsh/parameter f:fail


-- 
Peter Stephenson <pws@csr.com>            Software Engineer
Tel: +44 (0)1223 692070                   Cambridge Silicon Radio Limited
Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, UK


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

Re: PATCH: minor condition fixes

[Peter Stephenson]

On Fri, 19 Feb 2010 12:02:37 +0000
Peter Stephenson <pws@csr.com> wrote:
> Then there's a problem I haven't fixed, which is for some reason in the
> test code the status is returning as 1 for an unrecognised condition
> whereas it should return 2 (and does at the command line).  This shows
> up with:
> 
> % eval '[[ -fail badly ]]'
> zsh: unknown condition: -fail
> % print $?
> 1

I think it's as simple as the following; certainly the documentation for
"eval" says quite clearly it returns the same status as executing the code
directly, and so does the principle of least surprise, so I don't see how
the current behaviour can be right.  One would hope that the "if" I've
modified is redundant, in fact: the code already executed should have set
lastval.  However, it should now be harmless, I hope.

This means fixing a glob test.  It really is a fix; again, the status 2 was
already returned if there was no eval, and that's what we should be testing.
(Of course if you tell me the pattern code should really return status 1
that's a different bug, but I think it's correct with the patch.)

I'll modify the other patch accordingly.

Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.239
diff -p -u -r1.239 builtin.c
--- Src/builtin.c	7 Feb 2010 18:49:36 -0000	1.239
+++ Src/builtin.c	19 Feb 2010 12:06:16 -0000
@@ -4885,7 +4885,7 @@ eval(char **argv)
 	} else {
 	    execode(prog, 1, 0);
 
-	    if (errflag)
+	    if (errflag && !lastval)
 		lastval = errflag;
 	}
     } else {
Index: Test/D02glob.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/D02glob.ztst,v
retrieving revision 1.16
diff -p -u -r1.16 D02glob.ztst
--- Test/D02glob.ztst	9 Feb 2010 17:47:04 -0000	1.16
+++ Test/D02glob.ztst	19 Feb 2010 12:06:16 -0000
@@ -425,7 +425,7 @@
 >/ [[:WORD:]] yes
 
  [[ foo = (#c0)foo ]]
-1:Misplaced (#c...) flag
+2:Misplaced (#c...) flag
 ?(eval):1: bad pattern: (#c0)foo
 
  mkdir glob.tmp/dir5

-- 
Peter Stephenson <pws@csr.com>            Software Engineer
Tel: +44 (0)1223 692070                   Cambridge Silicon Radio Limited
Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, UK


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

Re: PATCH: minor condition fixes

[Peter Stephenson]

Peter Stephenson wrote:
> On Fri, 19 Feb 2010 12:02:37 +0000
> Peter Stephenson <pws@csr.com> wrote:
> > Then there's a problem I haven't fixed, which is for some reason in the
> > test code the status is returning as 1 for an unrecognised condition
> > whereas it should return 2 (and does at the command line).  This shows
> > up with:
> > 
> > % eval '[[ -fail badly ]]'
> > zsh: unknown condition: -fail
> > % print $?
> > 1
> 
> I think it's as simple as the following; certainly the documentation for
> "eval" says quite clearly it returns the same status as executing the code
> directly, and so does the principle of least surprise, so I don't see how
> the current behaviour can be right.

Interestingly, bash also returns status 1 on an unrecognised condition
when it's in an eval (else status 258).  I checked the standard, and
it seems to agree with the zsh documentation (what I think I've now
implemented):

  If there are no arguments, or only null arguments, eval shall return a
  zero exit status; otherwise, it shall return the exit status of the
  command defined by the string of concatenated arguments separated by
  <space> characters.

with no exception for errors.  The only mention of errors is

  CONSEQUENCES OF ERRORS

  Default.

which I think simply refers to whether or not the shell exits.

-- 
Peter Stephenson <pws@csr.com>            Software Engineer
Tel: +44 (0)1223 692070                   Cambridge Silicon Radio Limited
Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, UK


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom