[PATCH] Fix %- (prevjob) picking wrong job after resuming

[Mikel Ward <mikel@xxxxxxxxxxxxx>]

Three bugs, all causing %- to malfunction after fg %- on a
function superjob (e.g. v() { vim "$@"; }; v, suspend, start
another job, %-).

Bug 1: setprevjob() picks internal jobs as previous job

When fg %- runs, the shuffle code calls setprevjob() to find
a new previous job. It already excludes STAT_SUBJOB jobs, but
not STAT_NOPRINT jobs. The fg builtin itself gets a job table
entry (with STAT_BUILTIN|STAT_NOPRINT), and setprevjob()
picks it. Then the next %- resolves to that internal job,
which bin_fg rejects at the STAT_NOPRINT check:
"%-: no such job".

Fix: exclude STAT_NOPRINT from both loops in setprevjob(),
same as STAT_SUBJOB.

Bug 2: setprevjob() can pick the job being fg'd

The shuffle code in bin_fg calls setprevjob() to choose a new
previous job, but setprevjob() only excludes curjob and
thisjob. Since thisjob at that point is the fg builtin's own
job (not the job being operated on), setprevjob() can pick
the fg'd job itself as prevjob.

Fix: temporarily set thisjob to the fg'd job around the
shuffle block so setprevjob() won't consider it.

Bug 3: Subjob handler doesn't update curjob/prevjob

When a function's child (the subjob) stops, update_job()
takes the STAT_SUBJOB early-return path at line 539. This
path sets STAT_STOPPED on the superjob but never runs the
prevjob = curjob; curjob = job assignment at lines 635-638.
The superjob's own update_job() then returns early too
(line 542: already STAT_STOPPED). So curjob/prevjob are
never updated after re-suspension, leaving prevjob pointing
at whatever stale value the shuffle left behind.

Fix: add the curjob/prevjob update in the subjob handler,
right after it marks the superjob as STAT_STOPPED.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@xxxxxxxxxxxxx>
---
 Src/jobs.c        | 36 +++++++++++++++++++++++++-----------
 Test/W02jobs.ztst | 26 ++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 11 deletions(-)

diff --git a/Src/jobs.c b/Src/jobs.c
index 4905ae925..e32e35bdc 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -523,6 +523,11 @@ update_job(Job jn)
 		 * fg/bg is the superjob) a SIGCONT if we need it.
 		 */
 		sjn->stat |= STAT_CHANGED | STAT_STOPPED;
+		if ((sjn->stat & (STAT_DONE | STAT_STOPPED)) ==
+		    STAT_STOPPED) {
+		    prevjob = curjob;
+		    curjob = i;
+		}
 		if (isset(NOTIFY) && (sjn->stat & STAT_LOCKED) &&
 		    !(sjn->stat & STAT_NOPRINT)) {
 		    /*
@@ -701,13 +706,15 @@ setprevjob(void)
 
     for (i = maxjob; i; i--)
 	if ((jobtab[i].stat & STAT_INUSE) && (jobtab[i].stat & STAT_STOPPED) &&
-	    !(jobtab[i].stat & STAT_SUBJOB) && i != curjob && i != thisjob) {
+	    !(jobtab[i].stat & (STAT_SUBJOB | STAT_NOPRINT)) &&
+	    i != curjob && i != thisjob) {
 	    prevjob = i;
 	    return;
 	}
 
     for (i = maxjob; i; i--)
-	if ((jobtab[i].stat & STAT_INUSE) && !(jobtab[i].stat & STAT_SUBJOB) &&
+	if ((jobtab[i].stat & STAT_INUSE) &&
+	    !(jobtab[i].stat & (STAT_SUBJOB | STAT_NOPRINT)) &&
 	    i != curjob && i != thisjob) {
 	    prevjob = i;
 	    return;
@@ -2634,15 +2641,22 @@ bin_fg(char *name, char **argv, Options ops, int func)
 	    }
 	    /* It's time to shuffle the jobs around!  Reset the current job,
 	    and pick a sensible secondary job. */
-	    if (curjob == job) {
-		curjob = prevjob;
-		prevjob = (func == BIN_BG) ? -1 : job;
-	    }
-	    if (prevjob == job || prevjob == -1)
-		setprevjob();
-	    if (curjob == -1) {
-		curjob = prevjob;
-		setprevjob();
+	    {
+		/* Temporarily set thisjob so that setprevjob() won't pick it
+		 * as the previous job. */
+		int saved_thisjob = thisjob;
+		thisjob = job;
+		if (curjob == job) {
+		    curjob = prevjob;
+		    prevjob = (func == BIN_BG) ? -1 : job;
+		}
+		if (prevjob == job || prevjob == -1)
+		    setprevjob();
+		if (curjob == -1) {
+		    curjob = prevjob;
+		    setprevjob();
+		}
+		thisjob = saved_thisjob;
 	    }
 	    if (func != BIN_WAIT)
 		/* for bg and fg -- show the job we are operating on */
diff --git a/Test/W02jobs.ztst b/Test/W02jobs.ztst
index d52888dd9..5a7e47064 100644
--- a/Test/W02jobs.ztst
+++ b/Test/W02jobs.ztst
@@ -193,6 +193,32 @@
 *>\[2]  ? interrupt*sleep*
 *>\[1]  ? kill*sleep*
 
+# The following test exercises a bug where setprevjob() could pick an
+# internal NOPRINT job (e.g. a builtin's own job entry) as the previous
+# job.  This happened when a function superjob was involved, because the
+# superjob's subjob was excluded but the builtin job was not.
+# The function's child stops itself, triggering the superjob mechanism.
+# Then fg %- resumes the superjob (which exits since the child is done).
+# Afterwards, the remaining sleep job should still be accessible.
+  zpty_start
+  zpty_input "f() { sh -c 'kill -TSTP 0' }"
+  zpty_input 'f'
+  sleep 0.1
+  zpty_input 'sleep 100 &'
+  zpty_line
+  zpty_input 'kill -STOP %sleep'
+  sleep 0.1
+  zpty_input 'fg %-'
+  sleep 0.1
+  zpty_input 'jobs'
+  zpty_stop
+0:fg %- with function superjob does not pick internal job as previous
+*>zsh:*(stopped|suspended)*
+*>\[[0-9]##\] [0-9]##
+*>\[[0-9]##\]  + (stopped|suspended)*sleep*
+*>\[[0-9]##\]    continued*
+*>\[[0-9]##\]  - (stopped|suspended)*sleep*
+
 %clean
 
   zmodload -ui zsh/zpty
-- 
2.51.0