Fixes job handling involving functions and terminal control

While the idiomatic fix to fish' myriad of job control issues would be
to parse all functions prior to beginning the job pipeline so that
everything in the command line can be executed in the context of a
single job, that would require a huge effort to rewrite the core job
flow in fish and does not make sense at this time.

Instead, this patch fixes fish-shell#3952 and fish-shell#206 (but notably not fish-shell#4238) by
having jobs that are part of a single command pipeline, including those
that are functions executing external commands, use the same process
group. This prevents a (parent|child) from crashing with SIGTTIN or
hanging at SIGTTOU because it has a different process group than the
process currently in control of the terminal.

Additionally, since this fix involves removing the code that forces fish
to run in its own process group (which IMHO never made sense, as job
control is the job of the shell, not the process being launched), it
also fixes fish-shell#3805 and works around BashOnWindows#1653.

src/exec.cpp

@@ -401,8 +401,6 @@ void exec_job(parser_t &parser, job_t *j) {
     // Set to true if something goes wrong while exec:ing the job, in which case the cleanup code
     // will kick in.
     bool exec_error = false;
-    bool needs_keepalive = false;
-    process_t keepalive;
 
     CHECK(j, );
     CHECK_BLOCK();
@@ -412,7 +410,17 @@ void exec_job(parser_t &parser, job_t *j) {
         return;
     }
 
+    bool top_level_job;
+    static job_t *parent_job_group = nullptr;
     debug(4, L"Exec job '%ls' with id %d", j->command_wcstr(), j->job_id);
+    if (parent_job_group != nullptr) {
+        top_level_job = false;
+        debug(4, L" Parent job '%ls' with id %d", parent_job_group->command_wcstr(), parent_job_group->job_id);
+    }
+    else {
+        parent_job_group = j;
+        top_level_job = 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
@@ -454,31 +462,39 @@ void exec_job(parser_t &parser, job_t *j) {
     // sure that the process group doesn't die accidentally, and is often needed when a
     // builtin/block/function is inside a pipeline, since that usually means we have to wait for one
     // program to exit before continuing in the pipeline, causing the group leader to exit.
-    if (j->get_flag(JOB_CONTROL) && !exec_error) {
-        for (const process_ptr_t &p : j->processes) {
-            if (p->type != EXTERNAL && (!p->is_last_in_job || !p->is_first_in_job)) {
-                needs_keepalive = true;
-                break;
+    bool needs_keepalive = false;
+    process_t keepalive;
+
+    if (top_level_job) {
+        if (j->get_flag(JOB_CONTROL) && !exec_error) {
+            for (const process_ptr_t &p : j->processes) {
+                if (p->type != EXTERNAL && (!p->is_last_in_job || !p->is_first_in_job)) {
+                    needs_keepalive = true;
+                    break;
+                }
             }
         }
-    }
 
-    if (needs_keepalive) {
-        // Call fork. No need to wait for threads since our use is confined and simple.
-        keepalive.pid = execute_fork(false);
-        if (keepalive.pid == 0) {
-            // Child
-            keepalive.pid = getpid();
-            child_set_group(j, &keepalive);
-            pause();
-            exit_without_destructors(0);
-        } else {
-            // Parent
-            debug(2, L"Fork #%d, pid %d: keepalive fork for '%ls'", g_fork_count, keepalive.pid,
-                  j->command_wcstr());
-            set_child_group(j, keepalive.pid);
+        if (needs_keepalive) {
+            // Call fork. No need to wait for threads since our use is confined and simple.
+            keepalive.pid = execute_fork(false);
+            if (keepalive.pid == 0) {
+                // Child
+                keepalive.pid = getpid();
+                child_set_group(j, &keepalive);
+                pause();
+                exit_without_destructors(0);
+            } else {
+                // Parent
+                debug(2, L"Fork #%d, pid %d: keepalive fork for '%ls'", g_fork_count, keepalive.pid,
+                      j->command_wcstr());
+                set_child_group(j, keepalive.pid);
+            }
         }
     }
+    else {
+        j->pgid = parent_job_group->pgid;
+    }
 
     // This loop loops over every process_t in the job, starting it as appropriate. This turns out
     // to be rather complex, since a process_t can be one of many rather different things.
@@ -1193,6 +1209,10 @@ void exec_job(parser_t &parser, job_t *j) {
         // right place, but it prevents sanity_lose from complaining.
         j->set_flag(JOB_FOREGROUND, false);
     }
+
+    if (top_level_job) {
+        parent_job_group = nullptr;
+    }
 }
 
 static int exec_subshell_internal(const wcstring &cmd, wcstring_list_t *lst, bool apply_exit_status,

src/input.cpp

@@ -408,7 +408,7 @@ static bool input_mapping_is_match(const input_mapping_t &m) {
     wchar_t c = 0;
     int j;
 
-    debug(4, L"trying to match mapping %ls", escape_string(m.seq.c_str(), ESCAPE_ALL).c_str());
+    debug(5, L"trying to match mapping %ls", escape_string(m.seq.c_str(), ESCAPE_ALL).c_str());
     const wchar_t *str = m.seq.c_str();
     for (j = 0; str[j] != L'\0'; j++) {
         bool timed = (j > 0 && iswcntrl(str[0]));

src/reader.cpp

@@ -1626,30 +1626,7 @@ static void reader_interactive_init() {
         signal_set_handlers();
     }
 
-    // Put ourselves in our own process group.
-    shell_pgid = getpid();
-    if (getpgrp() != shell_pgid && setpgid(shell_pgid, shell_pgid) < 0) {
-        debug(0, _(L"Couldn't put the shell in its own process group"));
-        wperror(L"setpgid");
-        exit_without_destructors(1);
-    }
-
-    // Grab control of the terminal.
-    if (tcsetpgrp(STDIN_FILENO, shell_pgid) == -1) {
-        if (errno == ENOTTY) redirect_tty_output();
-        debug(0, _(L"Couldn't grab control of terminal"));
-        wperror(L"tcsetpgrp");
-        exit_without_destructors(1);
-    }
-
     invalidate_termsize();
-
-    // Set the new modes.
-    if (tcsetattr(0, TCSANOW, &shell_modes) == -1) {
-        if (errno == EIO) redirect_tty_output();
-        wperror(L"tcsetattr");
-    }
-
     env_set_one(L"_", ENV_GLOBAL, L"fish");
 }