PRINT_EXIT_VALUE: Suppress for if/while conditions

PRINT_EXIT_VALUE: Suppress for if/while conditions

[Daniel Shahaf]

[-- Attachment #1: Type: text/plain, Size: 1324 bytes --]

[tldr: question about printjob()]

I'd like to disable the effect of PRINT_EXIT_VALUE while evaluating
if/while conditions, since it's uninformative (conditions sometimes
fail, that's their sine qua non) and annoying (when doing a for/if
interactively and the 'if' condition is false in many iterations, the
option must be disabled to prevent stderr spamming).

So far I've got it working for builtins ("if false ; then : ; fi"
doesn't warn, whereas in git tip it does), but not for external commands
(with the patch, "if =false ; then : ; fi" still warns, but I'd like it
not to warn).  This is related to the MONITOR option:

    % if =false ; then : ; fi
    zsh: exit 1     =false
    % unsetopt monitor
    % if =false ; then : ; fi
    %

I'm guessing that has something to do with printjob(), since it checks
both 'jobbing' and opts[PRINTEXITVALUE], but I don't understand that
function.  Could I get a hint, please?

Would it be correct to just slip a "&& !printexitvalue_depth" to the "if
isset(PRINTEXITVALUE)" checks in printjob()?  I am not sure that would
be correct in the synch=0 case.

Thanks,

Daniel

P.S. I have a few other PRINT_EXIT_VALUE -related patches queued
my working drafts are at https://github.com/danielshahaf/zsh/commits/WIP/pev.
(I'll submit them as usual once they're ready for inclusion.)

[-- Attachment #2: 0001-Suppress-PRINT_EXIT_VALUE-for-if-and-while-condition.patch --]
[-- Type: text/x-patch, Size: 4399 bytes --]

>From 6c04df26989fcf3c363fff43543110a1c323798e Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Thu, 29 Jan 2015 12:34:19 +0000
Subject: [PATCH 1/4] Suppress PRINT_EXIT_VALUE for if and while conditions

The condition is *expected* to fail sometimes, so alerting the user to its
failure is not useful.
---
 Src/exec.c           |  6 ++++--
 Src/init.c           |  1 +
 Src/jobs.c           |  2 ++
 Src/loop.c           | 17 +++++++++++++++++
 Test/E01options.ztst |  4 ++++
 5 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/Src/exec.c b/Src/exec.c
index 7612d43..4c022cf 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -3620,7 +3620,8 @@ execcmd(Estate state, int input, int output, int how, int last1)
 		} else
 		    clearerr(stdout);
 	    }
-	    if (isset(PRINTEXITVALUE) && isset(SHINSTDIN) &&
+	    if (isset(PRINTEXITVALUE) && !printexitvalue_depth &&
+		isset(SHINSTDIN) &&
 		lastval && !subsh) {
 #if defined(ZLONG_IS_LONG_LONG) && defined(PRINTF_HAS_LLD)
 		fprintf(stderr, "zsh: exit %lld\n", lastval);
@@ -4690,7 +4691,8 @@ execfuncdef(Estate state, Eprog redir_prog)
 	    execshfunc(shf, args);
 	    ret = lastval;
 
-	    if (isset(PRINTEXITVALUE) && isset(SHINSTDIN) &&
+	    if (isset(PRINTEXITVALUE) && !printexitvalue_depth &&
+		isset(SHINSTDIN) &&
 		lastval) {
 #if defined(ZLONG_IS_LONG_LONG) && defined(PRINTF_HAS_LLD)
 		fprintf(stderr, "zsh: exit %lld\n", lastval);
diff --git a/Src/init.c b/Src/init.c
index 2ef9099..bc2e459 100644
--- a/Src/init.c
+++ b/Src/init.c
@@ -1060,6 +1060,7 @@ setupvals(void)
 #endif
 
     breaks = loops = 0;
+    printexitvalue_depth = 0;
     lastmailcheck = time(NULL);
     locallevel = sourcelevel = 0;
     sfcontext = SFC_NONE;
diff --git a/Src/jobs.c b/Src/jobs.c
index a71df68..ae1acad 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -979,6 +979,7 @@ printjob(Job jn, int lng, int synch)
 		    sflag = 1;
 		if (job == thisjob && sig == SIGINT)
 		    doputnl = 1;
+		/* ### printexitvalue_depth? */
 		if (isset(PRINTEXITVALUE) && isset(SHINSTDIN)) {
 		    sflag = 1;
 		    skip_print = 0;
@@ -989,6 +990,7 @@ printjob(Job jn, int lng, int synch)
 		    len = strlen(sigmsg(sig));
 		if (job == thisjob && sig == SIGTSTP)
 		    doputnl = 1;
+	    /* ### printexitvalue_depth? */
 	    } else if (isset(PRINTEXITVALUE) && isset(SHINSTDIN) &&
 		       WEXITSTATUS(pn->status)) {
 		sflag = 1;
diff --git a/Src/loop.c b/Src/loop.c
index e4e8e2d..46622d7 100644
--- a/Src/loop.c
+++ b/Src/loop.c
@@ -35,6 +35,11 @@
 /**/
 int loops;
  
+/* nonzero if PRINTEXITVALUE is temporarily suppressed */
+
+/**/
+int printexitvalue_depth;
+
 /* # of continue levels */
  
 /**/
@@ -408,9 +413,14 @@ execwhile(Estate state, UNUSED(int do_exec))
     } else
         for (;;) {
             state->pc = loop;
+
+	    /* Evalute the condition. */
             noerrexit = 1;
+	    ++printexitvalue_depth;
             execlist(state, 1, 0);
+	    --printexitvalue_depth;
             noerrexit = olderrexit;
+
             if (!((lastval == 0) ^ isuntil)) {
                 if (breaks)
                     breaks--;
@@ -421,6 +431,7 @@ execwhile(Estate state, UNUSED(int do_exec))
                 lastval = oldval;
                 break;
             }
+	    /* Evaluate the body. */
             execlist(state, 1, 0);
             if (breaks) {
                 breaks--;
@@ -510,9 +521,14 @@ execif(Estate state, int do_exec)
 	    break;
 	}
 	next = state->pc + WC_IF_SKIP(code);
+
+	/* Evaluate the next condition. */
+	++printexitvalue_depth;
 	cmdpush(s ? CS_ELIF : CS_IF);
 	execlist(state, 1, 0);
 	cmdpop();
+	--printexitvalue_depth;
+
 	if (!lastval) {
 	    run = 1;
 	    break;
@@ -524,6 +540,7 @@ execif(Estate state, int do_exec)
     }
 
     if (run) {
+	/* Evaluate the body. */
 	/* we need to ignore lastval until we reach execcmd() */
 	noerrexit = olderrexit ? olderrexit : lastval ? 2 : 0;
 	cmdpush(run == 2 ? CS_ELSE : (s ? CS_ELIFTHEN : CS_IFTHEN));
diff --git a/Test/E01options.ztst b/Test/E01options.ztst
index 16279b8..c6a48e8 100644
--- a/Test/E01options.ztst
+++ b/Test/E01options.ztst
@@ -817,6 +817,10 @@
 # Goodness only knows why.
   $ZTST_testdir/../Src/zsh -f <<<'
       setopt printexitvalue
+      if false; then :; fi
+      while false; do :; done
+      if =false; then :; fi
+      while =false; do :; done
       func() {
 	  false
       }
-- 
2.1.4

Re: PRINT_EXIT_VALUE: Suppress for if/while conditions

[Peter Stephenson]

On Fri, 31 Jul 2015 23:12:25 +0000
Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> I'd like to disable the effect of PRINT_EXIT_VALUE while evaluating
> if/while conditions, since it's uninformative (conditions sometimes
> fail, that's their sine qua non) and annoying (when doing a for/if
> interactively and the 'if' condition is false in many iterations, the
> option must be disabled to prevent stderr spamming).
> 
> So far I've got it working for builtins ("if false ; then : ; fi"
> doesn't warn, whereas in git tip it does), but not for external commands
> (with the patch, "if =false ; then : ; fi" still warns, but I'd like it
> not to warn).  This is related to the MONITOR option:
> 
>     % if =false ; then : ; fi
>     zsh: exit 1     =false
>     % unsetopt monitor
>     % if =false ; then : ; fi
>     %
> 
> I'm guessing that has something to do with printjob(), since it checks
> both 'jobbing' and opts[PRINTEXITVALUE], but I don't understand that
> function.  Could I get a hint, please?

It's a bit mysterious quite why it's implemented like that --- you might
have thought something parallel to ERREXIT would make more sense --- but
I don'e think what it actually does is that mysterious.  Bascially,
handling of printing exitvalues is divided into two parts: for anything
that runs within the shell it's done immediately the command is run; for
anything else it runs in printjob() when the job status changes (with
the side effect of dependence on MONITOR).  It might be done this way to
handle background jobs, which wouldn't be picked up if you did it in a
way more naturally related to the execution hierarchy.

But the intention is clearly that these are otherwise parallel cases.
So I think anything you do in the one case you can do in the other, as
you're proposing, up to asynchronous effects.

> Would it be correct to just slip a "&& !printexitvalue_depth" to the "if
> isset(PRINTEXITVALUE)" checks in printjob()?  I am not sure that would
> be correct in the synch=0 case.

Yes, I think you probably need something like

  (!printexitvalue_depth && sync == 1)

since the cases of calling it from jobs or bg or fg are irrelevant.

pws

Re: PRINT_EXIT_VALUE: Suppress for if/while conditions

[Daniel Shahaf]

Peter Stephenson wrote on Thu, Aug 13, 2015 at 09:32:38 +0100:
> On Fri, 31 Jul 2015 23:12:25 +0000
> Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > I'd like to disable the effect of PRINT_EXIT_VALUE while evaluating
> > if/while conditions, since it's uninformative (conditions sometimes
> > fail, that's their sine qua non) and annoying (when doing a for/if
> > interactively and the 'if' condition is false in many iterations, the
> > option must be disabled to prevent stderr spamming).
> > 
> > So far I've got it working for builtins ("if false ; then : ; fi"
> > doesn't warn, whereas in git tip it does), but not for external commands
> > (with the patch, "if =false ; then : ; fi" still warns, but I'd like it
> > not to warn).  This is related to the MONITOR option:
> > 
> >     % if =false ; then : ; fi
> >     zsh: exit 1     =false
> >     % unsetopt monitor
> >     % if =false ; then : ; fi
> >     %
> > 
> > I'm guessing that has something to do with printjob(), since it checks
> > both 'jobbing' and opts[PRINTEXITVALUE], but I don't understand that
> > function.  Could I get a hint, please?
> 
> It's a bit mysterious quite why it's implemented like that --- you might
> have thought something parallel to ERREXIT would make more sense --- but
> I don'e think what it actually does is that mysterious.  Bascially,
> handling of printing exitvalues is divided into two parts: for anything
> that runs within the shell it's done immediately the command is run; for
> anything else it runs in printjob() when the job status changes (with
> the side effect of dependence on MONITOR).  It might be done this way to
> handle background jobs, which wouldn't be picked up if you did it in a
> way more naturally related to the execution hierarchy.
> 
> But the intention is clearly that these are otherwise parallel cases.
> So I think anything you do in the one case you can do in the other, as
> you're proposing, up to asynchronous effects.
> 
> > Would it be correct to just slip a "&& !printexitvalue_depth" to the "if
> > isset(PRINTEXITVALUE)" checks in printjob()?  I am not sure that would
> > be correct in the synch=0 case.
> 
> Yes, I think you probably need something like
> 
>   (!printexitvalue_depth && sync == 1)
> 
> since the cases of calling it from jobs or bg or fg are irrelevant.

First of all, thanks for having a look.

I have no problem with ruling out the jobs/bg/fg callsites, as you
propose.  However, checking (synch == 1) would also mean the value of
PRINTEXITVALUE is entirely ignored when printjob() is called
asynchronously.  I can see that that is fine for jobs that don't have
STAT_NOPRINT set.¹  Is it also correct to ignore PRINTEXITVALUE in the
case (synch == 0 && (jn->stat & STAT_NOPRINT))?

(I'm guessing the answer is affirmative, and that if it were negative
I should be testing the value of printexitvalue_depth that was current
when the background job was started.)

In any case, I think I'll wait with this series until after 5.0.9, to
give it as long as possible to stabilize.

Cheers,

Daniel

¹ For jobs without STAT_NOPRINT, the only thing the 'if PRINTEXITVALUE'
block does is set sflag; the only place that tests sflag is a '(sflag ||
job != thisjob)' condition; for asynchronous jobs, the value of this
condition is independent of the value of 'sflag' (the disjunction would
be true because the inequality would be true).  Therefore,
non-STAT_NOPRINT jobs would be printed anyway, regardless of the setting
of PRINTEXITVALUE.

> pws
> 
> pws

Re: PRINT_EXIT_VALUE: Suppress for if/while conditions

[Peter Stephenson]

On Thu, 13 Aug 2015 23:20:20 +0000
Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> I have no problem with ruling out the jobs/bg/fg callsites, as you
> propose.  However, checking (synch == 1) would also mean the value of
> PRINTEXITVALUE is entirely ignored when printjob() is called
> asynchronously.  I can see that that is fine for jobs that don't have
> STAT_NOPRINT set.¹  Is it also correct to ignore PRINTEXITVALUE in the
> case (synch == 0 && (jn->stat & STAT_NOPRINT))?

My point is really that you don't want to *modify* the code unless synch
== 1.  So sync == anything else should continue to do just what it does
at the moment, but if synch == 1 you can apply your extra check.  That
seems likely to do the nearest to what everybody wants.

pws

Re: PRINT_EXIT_VALUE: Suppress for if/while conditions

[Vincent Lefevre]

On 2015-07-31 23:12:25 +0000, Daniel Shahaf wrote:
> I'd like to disable the effect of PRINT_EXIT_VALUE while evaluating
> if/while conditions, since it's uninformative (conditions sometimes
> fail, that's their sine qua non) and annoying (when doing a for/if
> interactively and the 'if' condition is false in many iterations, the
> option must be disabled to prevent stderr spamming).

It's also annoying in loops, e.g.

  for i in **/*.gz; zgrep foo $i

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: PRINT_EXIT_VALUE: Suppress for if/while conditions

[Mikael Magnusson]

On Sat, Aug 15, 2015 at 3:04 AM, Vincent Lefevre <vincent@vinc17.net> wrote:
> On 2015-07-31 23:12:25 +0000, Daniel Shahaf wrote:
>> I'd like to disable the effect of PRINT_EXIT_VALUE while evaluating
>> if/while conditions, since it's uninformative (conditions sometimes
>> fail, that's their sine qua non) and annoying (when doing a for/if
>> interactively and the 'if' condition is false in many iterations, the
>> option must be disabled to prevent stderr spamming).
>
> It's also annoying in loops, e.g.
>
>   for i in **/*.gz; zgrep foo $i

Seeing the exit value of each zgrep invocation seems to be within the
scope of the option. If you find it annoying, maybe you shouldn't have
it set?

For example if you did

for i in file1 file2; do
  veryimportanttask $i
done

you would surely want to see if any of the invocations failed. I could
see the value in making it shut up if you changed the body to


for i in file{1..9999}; do
  notsoimportanttask $i || :
done

but that doesn't currently help.

-- 
Mikael Magnusson

Re: PRINT_EXIT_VALUE: Suppress for if/while conditions

[Daniel Shahaf]

Peter Stephenson wrote on Fri, Aug 14, 2015 at 09:19:45 +0100:
> On Thu, 13 Aug 2015 23:20:20 +0000
> Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > I have no problem with ruling out the jobs/bg/fg callsites, as you
> > propose.  However, checking (synch == 1) would also mean the value of
> > PRINTEXITVALUE is entirely ignored when printjob() is called
> > asynchronously.  I can see that that is fine for jobs that don't have
> > STAT_NOPRINT set.¹  Is it also correct to ignore PRINTEXITVALUE in the
> > case (synch == 0 && (jn->stat & STAT_NOPRINT))?
> 
> My point is really that you don't want to *modify* the code unless synch
> == 1.  So sync == anything else should continue to do just what it does
> at the moment, but if synch == 1 you can apply your extra check.  That
> seems likely to do the nearest to what everybody wants.

I'll give that a shot.  (Was a bit busy so haven't coded it yet.)
Thanks again.

Re: PRINT_EXIT_VALUE: Suppress for if/while conditions

[Vincent Lefevre]

On 2015-08-15 03:20:35 +0200, Mikael Magnusson wrote:
> Seeing the exit value of each zgrep invocation seems to be within the
> scope of the option. If you find it annoying, maybe you shouldn't have
> it set?

I have it set because I like it for individual commands.

> For example if you did
> 
> for i in file1 file2; do
>   veryimportanttask $i
> done
> 
> you would surely want to see if any of the invocations failed.

But you don't know which one:

$ for i in 1 2; do /bin/false $i; done
zsh: exit 1     /bin/false $i
zsh: exit 1     /bin/false $i

One could write

  veryimportanttask $i || echo error for $i

in such a case, which is more informative.

With my example above, without PRINT_EXIT_VALUE:

$ for i in 1 2; do /bin/false $i || echo error for $i; done
error for 1
error for 2

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: PRINT_EXIT_VALUE: Suppress for if/while conditions

[Bart Schaefer]

On Aug 19,  2:06am, Vincent Lefevre wrote:
} Subject: Re: PRINT_EXIT_VALUE: Suppress for if/while conditions
}
} > for i in file1 file2; do
} >   veryimportanttask $i
} > done
} 
} But you don't know which one:
} 
} $ for i in 1 2; do /bin/false $i; done
} zsh: exit 1     /bin/false $i
} zsh: exit 1     /bin/false $i

    eval "veryimportanttask ${(b)i}"

Of course then printexitvalue prints "exit 1" for each of the eval
as well.  *There* is a case where it's probably entirely extraneous.

torch% setopt printexitvalue
torch% for i in 1 2; do eval /bin/false $i; done
zsh: exit 1     /bin/false 1
zsh: exit 1
zsh: exit 1     /bin/false 2
zsh: exit 1