Associate external commands in functions with extant pgrps

When a function is encountered by exec_job, a new context is created for
its execution from the ground up, with a new job and all, ultimately
resulting in a recursive call to exec_job from the same (main) thread.

Since each time exec_job encounters a new job with external commands
that needs terminal control it creates a new pgrp and gives it control
of the terminal (tcsetpgrp & co), this effectively takes control away
from the previously spawned external commands which may be (and likely
are) expecting to still have terminal access.

This commit attempts to detect when such a situation arises by handling
recursive calls to exec_job (which can only happen if the pipeline
included a function) by borrowing the pgrp from the (necessarily still
active) parent job and spawning new external commands into it.

Closes fish-shell#3952.

src/exec.cpp

@@ -17,6 +17,7 @@
 #include <string.h>
 #include <sys/wait.h>
 #include <unistd.h>
+#include <stack>
 
 #include <algorithm>
 #include <functional>
@@ -610,7 +611,8 @@ static bool handle_builtin_output(job_t *j, process_t *p, io_chain_t *io_chain,
     if (fork_was_skipped) {
         p->completed = 1;
         if (p->is_last_in_job) {
-            debug(4, L"Set status of job %d (%ls) to %d using short circuit", j->job_id, j->preview().c_str(), p->status);
+            debug(4, L"Set status of job %d (%ls) to %d using short circuit", j->job_id,
+                  j->preview().c_str(), p->status);
 
             int status = p->status;
             proc_set_last_status(j->get_flag(job_flag_t::NEGATE) ? (!status) : status);
@@ -996,6 +998,32 @@ void exec_job(parser_t &parser, job_t *j) {
         return;
     }
 
+    // Unfortunately `exec_job()` is called recursively when functions are encountered, with a new
+    // job id (and therefore pgrp) each time, but always from the main thread. This breaks terminal
+    // control since new pgrps take terminal control away from commands upstream in a different pgrp.
+    // We try to work around this with a heuristic to determine whether to reuse the same pgrp as the
+    // last-spawned pgrp if part of an existing job pipeline (keeping in mind that new jobs are
+    // recursively started for both foreground and background jobs, and that a function can expand
+    // to more than one external command, one (or more?) of which may want to read from upstream or
+    // write to downstream of a pipe.
+    // By keeping track of (select) "jobs in flight" we can try to marshall newly-created external
+    // processes into existing pgrps.
+    static std::stack<job_t *> active_jobs{};
+    job_t *parent_job = active_jobs.empty() ? nullptr : active_jobs.top();
+    bool job_pushed = false;
+    if (j->get_flag(job_flag_t::TERMINAL) && j->get_flag(job_flag_t::JOB_CONTROL)) {
+        // This will be popped before this job leaves exec_job
+        active_jobs.push(j);
+        job_pushed = true;
+    }
+
+    if (parent_job && j->processes.front()->type == EXTERNAL) {
+        if (parent_job->pgid != INVALID_PID) {
+            j->pgid = parent_job->pgid;
+            j->set_flag(job_flag_t::JOB_CONTROL, true);
+        }
+    }
+
     // Verify that all IO_BUFFERs are output. We used to support a (single, hacked-in) magical input
     // IO_BUFFER used by fish_pager, but now the claim is that there are no more clients and it is
     // removed. This assertion double-checks that.
@@ -1053,15 +1081,19 @@ void exec_job(parser_t &parser, job_t *j) {
     }
     pipe_next_read.close();
 
-
-    debug(3, L"Created job %d from command '%ls' with pgrp %d", j->job_id, j->command_wcstr(), j->pgid);
+    debug(3, L"Created job %d from command '%ls' with pgrp %d", j->job_id, j->command_wcstr(),
+          j->pgid);
 
     j->set_flag(job_flag_t::CONSTRUCTED, true);
     if (!j->is_foreground()) {
         proc_last_bg_pid = j->pgid;
         env_set(L"last_pid", ENV_GLOBAL, { to_string(proc_last_bg_pid) });
     }
 
+    if (job_pushed) {
+        active_jobs.pop();
+    }
+
     if (!exec_error) {
         j->continue_job(false);
     } else {