terminal_maybe_give_to_job to operate on groups, not jobs

Assigning the tty is really a function of a job group, not an individual
job. Reflect that in terminal_maybe_give_to_job_group and also
terminal_return_from_job_group.

src/exec.cpp

@@ -347,7 +347,7 @@ static bool fork_child_for_process(const std::shared_ptr<job_t> &job, process_t
     if (int err = execute_setpgid(p->pid, pgid, true /* is parent */)) {
         if (err != EACCES) report_setpgid_error(err, pgid, job.get(), p);
     }
-    terminal_maybe_give_to_job(job.get(), false);
+    terminal_maybe_give_to_job_group(job->group.get(), false);
     return true;
 }
 
@@ -546,7 +546,7 @@ static bool exec_external_command(parser_t &parser, const std::shared_ptr<job_t>
         // therefore the parent may not have seen it be set yet.
         // Ensure it gets set. See #4715, also https://github.com/Microsoft/WSL/issues/2997.
         execute_setpgid(p->pid, pgid, true /* is parent */);
-        terminal_maybe_give_to_job(j.get(), false);
+        terminal_maybe_give_to_job_group(j->group.get(), false);
     } else
 #endif
     {

src/parse_execution.cpp

@@ -1308,7 +1308,6 @@ end_execution_reason_t parse_execution_context_t::run_1_job(const ast::job_t &jo
     props.job_control = wants_job_control;
 
     shared_ptr<job_t> job = std::make_shared<job_t>(props);
-    job->tmodes = tmodes;
 
     // We are about to populate a job. One possible argument to the job is a command substitution
     // which may be interested in the job that's populating it, via '--on-job-exit caller'. Record

src/proc.cpp

@@ -801,17 +801,17 @@ void proc_update_jiffies(parser_t &parser) {
 
 // Return control of the terminal to a job's process group. restore_attrs is true if we are
 // restoring a previously-stopped job, in which case we need to restore terminal attributes.
-int terminal_maybe_give_to_job(const job_t *j, bool continuing_from_stopped) {
+int terminal_maybe_give_to_job_group(const job_group_t *jg, bool continuing_from_stopped) {
     enum { notneeded = 0, success = 1, error = -1 };
 
-    if (!j->group->should_claim_terminal()) {
+    if (!jg->should_claim_terminal()) {
         // The job doesn't want the terminal.
         return notneeded;
     }
 
     // Get the pgid; we may not have one.
     pid_t pgid{};
-    if (auto mpgid = j->get_pgid()) {
+    if (auto mpgid = jg->get_pgid()) {
         pgid = *mpgid;
     } else {
         FLOG(proc_termowner, L"terminal_give_to_job() returning early due to no process group");
@@ -888,7 +888,7 @@ int terminal_maybe_give_to_job(const job_t *j, bool continuing_from_stopped) {
             return notneeded;
         } else {
             FLOGF(warning, _(L"Could not send job %d ('%ls') with pgid %d to foreground"),
-                  j->job_id(), j->command_wcstr(), pgid);
+                  jg->get_id(), jg->get_command().c_str(), pgid);
             wperror(L"tcsetpgrp");
             return error;
         }
@@ -906,8 +906,8 @@ int terminal_maybe_give_to_job(const job_t *j, bool continuing_from_stopped) {
         break;
     }
 
-    if (continuing_from_stopped) {
+    if (continuing_from_stopped && jg->tmodes.has_value()) {
-        auto result = tcsetattr(STDIN_FILENO, TCSADRAIN, &j->tmodes);
+        int result = tcsetattr(STDIN_FILENO, TCSADRAIN, &jg->tmodes.value());
         if (result == -1) {
             wperror(L"tcsetattr");
         }
@@ -916,11 +916,11 @@ int terminal_maybe_give_to_job(const job_t *j, bool continuing_from_stopped) {
     return success;
 }
 
-/// Returns control of the terminal to the shell, and saves the terminal attribute state to the job,
+/// Returns control of the terminal to the shell, and saves the terminal attribute state to the job
-/// so that we can restore the terminal ownership to the job at a later time.
+/// group, so that we can restore the terminal ownership to the job at a later time.
-static bool terminal_return_from_job(job_t *j, int restore_attrs) {
+static bool terminal_return_from_job_group(job_group_t *jg, bool restore_attrs) {
     errno = 0;
-    auto pgid = j->get_pgid();
+    auto pgid = jg->get_pgid();
     if (!pgid.has_value()) {
         FLOG(proc_pgroup, "terminal_return_from_job() returning early due to no process group");
         return true;
@@ -935,7 +935,8 @@ static bool terminal_return_from_job(job_t *j, int restore_attrs) {
     }
 
     // Save jobs terminal modes.
-    if (tcgetattr(STDIN_FILENO, &j->tmodes)) {
+    struct termios tmodes {};
+    if (tcgetattr(STDIN_FILENO, &tmodes)) {
         // If it's not a tty, it's not a tty, and there are no attributes to save (or restore)
         if (errno == ENOTTY) return false;
         if (errno == EIO) redirect_tty_output();
@@ -943,6 +944,7 @@ static bool terminal_return_from_job(job_t *j, int restore_attrs) {
         wperror(L"tcgetattr");
         return false;
     }
+    jg->tmodes = tmodes;
 
     // Need to restore the terminal's attributes or `bind \cF fg` will put the
     // terminal into a broken state (until "enter" is pressed).
@@ -974,17 +976,17 @@ void job_t::continue_job(parser_t &parser, bool reclaim_foreground_pgrp, bool se
 
     // Make sure we retake control of the terminal before leaving this function.
     bool term_transferred = false;
-    cleanup_t take_term_back([&]() {
+    cleanup_t take_term_back([&] {
         if (term_transferred && reclaim_foreground_pgrp) {
             // Only restore terminal attrs if we're continuing a job. See:
             // https://github.com/fish-shell/fish-shell/issues/121
             // https://github.com/fish-shell/fish-shell/issues/2114
-            terminal_return_from_job(this, send_sigcont);
+            terminal_return_from_job_group(this->group.get(), send_sigcont);
         }
     });
 
     if (!is_completed()) {
-        int transfer = terminal_maybe_give_to_job(this, send_sigcont);
+        int transfer = terminal_maybe_give_to_job_group(this->group.get(), send_sigcont);
         if (transfer < 0) {
             // terminal_maybe_give_to_job prints an error.
             return;

src/proc.h

@@ -230,6 +230,11 @@ class job_group_t {
 
     ~job_group_t();
 
+    /// If set, the saved terminal modes of this job. This needs to be saved so that we can restore
+    /// the terminal to the same state after temporarily taking control over the terminal when a job
+    /// stops.
+    maybe_t<struct termios> tmodes{};
+
    private:
     // The pgid to assign to jobs, or none if not yet set.
     maybe_t<pid_t> pgid_{};
@@ -505,11 +510,6 @@ class job_t {
     /// A non-user-visible, never-recycled job ID.
     const internal_job_id_t internal_job_id;
 
-    /// The saved terminal modes of this job. This needs to be saved so that we can restore the
-    /// terminal to the same state after temporarily taking control over the terminal when a job
-    /// stops.
-    struct termios tmodes {};
-
     /// Flags associated with the job.
     struct flags_t {
         /// Whether the specified job is completely constructed: every process in the job has been
@@ -667,14 +667,14 @@ bool is_within_fish_initialization();
 /// Send SIGHUP to the list \p jobs, excepting those which are in fish's pgroup.
 void hup_jobs(const job_list_t &jobs);
 
-/// Give ownership of the terminal to the specified job, if it wants it.
+/// Give ownership of the terminal to the specified job group, if it wants it.
 ///
-/// \param j The job to give the terminal to.
+/// \param jg The job group to give the terminal to.
 /// \param continuing_from_stopped If this variable is set, we are giving back control to a job that
 /// was previously stopped. In that case, we need to set the terminal attributes to those saved in
 /// the job.
 /// \return 1 if transferred, 0 if no transfer was necessary, -1 on error.
-int terminal_maybe_give_to_job(const job_t *j, bool continuing_from_stopped);
+int terminal_maybe_give_to_job_group(const job_group_t *jg, bool continuing_from_stopped);
 
 /// Add a pid to the list of pids we wait on even though they are not associated with any jobs.
 /// Used to avoid zombie processes after disown.