Re: PRINT_EXIT_VALUE: Suppress for if/while conditions

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxx> 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