Re: [PATCH] problem with 'ls | less' shell function

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

On Fri, Nov 4, 2022 at 8:12 AM Jun T <takimoto-j@xxxxxxxxxxxxxxxxx> wrote:
>
> 548         /* is this job in the foreground of an interactive shell? */
> 549         if (mypgrp != pgrp && inforeground &&
> 550             (jn->gleader == pgrp ||
> 551              (pgrp > 1 &&
> 552               (kill(-pgrp, 0) == -1 && errno == ESRCH)))) {
>
> But why just "jn->gleader == pgrp" is enough to assume that the foreground
> job has finished?
> [...]
> Another code that looks suspicious to me is (also in jobs.c)
>
> 478         if (pn->pid == jn->gleader)    /* if this process is process group leader */
> 479             status = pn->status;
> 480     }
>
> [...]
> If the line 478 is replaced by
>
> 478         if (WIFSIGNALED(pn->status) || pn->pid == jn->gleader)
>
> then [2] can be killed by a single ^C.

Thank you!  This is the clue I needed.

On line 476 is

476             signalled = WIFSIGNALED(pn->status);

If we change line 550 to be

550             ((jn->gleader == pgrp && signalled) ||

then all three examples start working.

Second patch attached, replaces the one from workers/50862.  Tests
pass, which is not surprising since all of this depends on signaling
interactive shells, but I have no idea how to write a regression test
for that.

diff --git a/Src/jobs.c b/Src/jobs.c
index 707374297..76c762ee5 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -544,16 +544,14 @@ update_job(Job jn)
 
     if (isset(MONITOR)) {
 	pid_t pgrp = gettygrp();           /* get process group of tty      */
+	int deadpgrp = (mypgrp != pgrp && inforeground && pgrp > 1 &&
+			kill(-pgrp, 0) == -1 && errno == ESRCH);
 
 	/* is this job in the foreground of an interactive shell? */
 	if (mypgrp != pgrp && inforeground &&
-	    (jn->gleader == pgrp ||
-	     (pgrp > 1 &&
-	      (kill(-pgrp, 0) == -1 && errno == ESRCH)))) {
+	    ((jn->gleader == pgrp && signalled) || deadpgrp)) {
 	    if (list_pipe) {
-		if (somestopped || (pgrp > 1 &&
-				    kill(-pgrp, 0) == -1 &&
-				    errno == ESRCH)) {
+		if (somestopped || deadpgrp) {
 		    attachtty(mypgrp);
 		    /* check window size and adjust if necessary */
 		    adjustwinsize(0);
@@ -566,6 +564,12 @@ update_job(Job jn)
 		     * when the job is finally deleted.
 		     */
 		    jn->stat |= STAT_ATTACH;
+		    /*
+		     * If we're in shell jobs on the right side of a pipeline
+		     * we should treat it like a job in the current shell.
+		     */
+		    if (inforeground == 2)
+			inforeground = 1;
 		}
 		/* If we have `foo|while true; (( x++ )); done', and hit
 		 * ^C, we have to stop the loop, too. */
@@ -1488,10 +1492,7 @@ addproc(pid_t pid, char *text, int aux, struct timeval *bgtime,
 	 * set it for that, too.
 	 */
 	if (gleader != -1) {
-	    if (jobtab[thisjob].stat & STAT_CURSH)
-		jobtab[thisjob].gleader = gleader;
-	    else
-		jobtab[thisjob].gleader = pid;
+	    jobtab[thisjob].gleader = gleader;
 	    if (list_pipe_job_used != -1)
 		jobtab[list_pipe_job_used].gleader = gleader;
 	    /*
@@ -1500,7 +1501,7 @@ addproc(pid_t pid, char *text, int aux, struct timeval *bgtime,
 	     */
 	    last_attached_pgrp = gleader;
 	} else if (!jobtab[thisjob].gleader)
-		jobtab[thisjob].gleader = pid;
+	    jobtab[thisjob].gleader = pid;
 	/* attach this process to end of process list of current job */
 	pnlist = &jobtab[thisjob].procs;
     }
@@ -2506,6 +2507,7 @@ bin_fg(char *name, char **argv, Options ops, int func)
 		jobtab[job].stat &= ~STAT_CURSH;
 	    }
 	    if ((stopped = (jobtab[job].stat & STAT_STOPPED))) {
+		/* WIFCONTINUED will makerunning() again at killjb() */
 		makerunning(jobtab + job);
 		if (func == BIN_BG) {
 		    /* Set $! to indicate this was backgrounded */