From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7290 invoked by alias); 24 Oct 2013 15:48:40 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: X-Seq: 31881 Received: (qmail 4849 invoked from network); 24 Oct 2013 15:48:33 -0000 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_HI, SPF_HELO_PASS autolearn=ham version=3.3.2 X-AuditID: cbfec7f5-b7ef66d00000795a-05-5269414e91e1 Date: Thu, 24 Oct 2013 16:48:29 +0100 From: Peter Stephenson To: zsh-workers@zsh.org Subject: Re: Strange function/pipestatus behavior, maybe a scope bug? Message-id: <20131024164829.55842279@pwslap01u.europe.root.pri> In-reply-to: <20131024095713.44d2982e@pwslap01u.europe.root.pri> References: <20131023092155.5ba8a54b@pwslap01u.europe.root.pri> <20131023221412.5cdecd76@pws-pc.ntlworld.com> <131023221548.ZM2352@torch.brasslantern.com> <20131024095713.44d2982e@pwslap01u.europe.root.pri> Organization: Samsung Cambridge Solution Centre X-Mailer: Claws Mail 3.7.9 (GTK+ 2.22.0; i386-redhat-linux-gnu) MIME-version: 1.0 Content-type: text/plain; charset=US-ASCII Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFupjluLIzCtJLcpLzFFi42I5/e/4ZV0/x8wggxfT9CwONj9kcmD0WHXw A1MAYxSXTUpqTmZZapG+XQJXRtOro4wFq6Uqfm19ytzAuE6ki5GTQ0LAROLJm5vMELaYxIV7 69m6GLk4hASWMkosOz+NEcJZziTR3/uAFaSKRUBVYt+9iywgNpuAocTUTbMZQWwRAXGJs2vP g8WFBVwlzt6ZCRbnFbCXaGroArM5BRwkTkz5xgwx9AmTxKuD/UwgCX4BfYmrfz8xQZxhLzHz yhmoZkGJH5PvgQ1lFtCS2LytiRXClpfYvOYt8wRGgVlIymYhKZuFpGwBI/MqRtHU0uSC4qT0 XCO94sTc4tK8dL3k/NxNjJAw/LqDcekxq0OMAhyMSjy8H19lBAmxJpYVV+YeYpTgYFYS4Z2m lxkkxJuSWFmVWpQfX1Sak1p8iJGJg1OqgXHl41urHd65XLu0IVD9Z9viDXdyLphubcmce+pj +aToe+ZP6sQfuQo9zF2gsfemqUdofZyo8z+7CXfeRjCkBPbLCSdnvmfx/LBg3mazizl+Ym9V ffZfli6453guX9zdVdPts+6dWT+jAxRmZN6OeWrqk1Y08/D7xJkSizt2M/wWlXLWPuTDWq3E UpyRaKjFXFScCADBy5wXIQIAAA== On Thu, 24 Oct 2013 09:57:13 +0100 Peter Stephenson wrote: > > Oh, one other bug which I don't believe is new: the PIPEFAIL option > > doesn't work right if the last command is a builtin. Maybe that > > test should be moved into storepipestats() as well, in which case > > the redundancy I mentioned above might also be eliminated. > > Are you saying that $pipestatus is correct but the overall status is > wrong if you have something like "true | true | true" or "true | true | > false"? It certainly sounds like they ought to run at the same point. > The last command ought to be the easy one --- that's where the status > has always come from, so if those two pipelines are giving different > answers with and without PIPEFAIL something is definitely odd. I can't see any problem here, which might not be too surprising if it's a race, but I don't see how the following fix can be wrong if pipestatus is now working: the most complicated case appears to be if it gets called multiple times and STAT_CURSH is set, but even there it looks like it's idempotent with respect to lastval (assuming the information hasn't changed, of course). I don't know if this potentially makes one of the calls removable. There is some bad interaction with completion, at least on my setup (happens from a vanilla setup, too): % true | true | false % print $pipest 0 This doesn't seem to happen with hooks, so completion is failing to save something it should. diff --git a/Src/jobs.c b/Src/jobs.c index 82ffdf2..200e38e 100644 --- a/Src/jobs.c +++ b/Src/jobs.c @@ -377,7 +377,7 @@ check_cursh_sig(int sig) } /**/ -int +static void storepipestats(Job jn, int inforeground) { int i, pipefail = 0, jpipestats[MAX_PIPESTATS]; @@ -397,7 +397,13 @@ storepipestats(Job jn, int inforeground) numpipestats = i; } - return pipefail; + if (jn == jobtab+thisjob) { + if (jn->stat & STAT_CURSH) { + if (!lastval && isset(PIPEFAIL)) + lastval = pipefail; + } else if (isset(PIPEFAIL)) + lastval = pipefail; + } } /* Update status of job, possibly printing it */ @@ -531,17 +537,8 @@ update_job(Job jn) return; jn->stat |= (somestopped) ? STAT_CHANGED | STAT_STOPPED : STAT_CHANGED | STAT_DONE; - if (jn->stat & STAT_DONE) { - int newlastval = storepipestats(jn, inforeground); - - if (job == thisjob) { - if (jn->stat & STAT_CURSH) { - if (!lastval && isset(PIPEFAIL)) - lastval = newlastval; - } else if (isset(PIPEFAIL)) - lastval = newlastval; - } - } + if (jn->stat & STAT_DONE) + storepipestats(jn, inforeground); if (!inforeground && (jn->stat & (STAT_SUBJOB | STAT_DONE)) == (STAT_SUBJOB | STAT_DONE)) { int su; @@ -991,7 +988,7 @@ printjob(Job jn, int lng, int synch) if (skip_print) { if (jn->stat & STAT_DONE) { - (void) storepipestats(jn, job == thisjob); + storepipestats(jn, job == thisjob); if (should_report_time(jn)) dumptime(jn); deletejob(jn, 0); @@ -1122,7 +1119,7 @@ printjob(Job jn, int lng, int synch) /* delete job if done */ if (jn->stat & STAT_DONE) { - (void) storepipestats(jn, job == thisjob); + storepipestats(jn, job == thisjob); if (should_report_time(jn)) dumptime(jn); deletejob(jn, 0); pws