Stop acquiring the terminal before running builtins

fish has some unprincipled code that attempts to tcsetpgrp() to own the
terminal before running a builtin; this was added because 'read' might
want to read from the terminal. I added this code before fully
understanding how process groups and terminals work. A better fix would
be to ensure that fish is marked as the pgroup leader in the job when
the builtin is the first process in the job, and we do that now.

Courageously back out the changes to grab the terminal; see #5147 and
also #5133.

src/builtin.cpp

@@ -437,7 +437,7 @@ static const wchar_t *const help_builtins[] = {L"for", L"while",  L"function", L
 static bool cmd_needs_help(const wchar_t *cmd) { return contains(help_builtins, cmd); }
 
 /// Execute a builtin command
-proc_status_t builtin_run(parser_t &parser, int job_pgid, wchar_t **argv, io_streams_t &streams) {
+proc_status_t builtin_run(parser_t &parser, wchar_t **argv, io_streams_t &streams) {
     UNUSED(parser);
     UNUSED(streams);
     if (argv == nullptr || argv[0] == nullptr)
@@ -452,15 +452,7 @@ proc_status_t builtin_run(parser_t &parser, int job_pgid, wchar_t **argv, io_str
     }
 
     if (const builtin_data_t *data = builtin_lookup(argv[0])) {
-        // If we are interactive, save the foreground pgroup and restore it after in case the
-        // builtin needs to read from the terminal. See #4540.
-        bool grab_tty = session_interactivity() != session_interactivity_t::not_interactive &&
-                        isatty(streams.stdin_fd);
-        pid_t pgroup_to_restore = grab_tty ? terminal_acquire_before_builtin(job_pgid) : -1;
         int ret = data->func(parser, streams, argv);
-        if (pgroup_to_restore >= 0) {
-            tcsetpgrp(STDIN_FILENO, pgroup_to_restore);
-        }
         return proc_status_t::from_exit_code(ret);
     }
 

src/builtin.h

@@ -81,7 +81,7 @@ enum { COMMAND_NOT_BUILTIN, BUILTIN_REGULAR, BUILTIN_FUNCTION };
 void builtin_init();
 bool builtin_exists(const wcstring &cmd);
 
-proc_status_t builtin_run(parser_t &parser, int job_pgid, wchar_t **argv, io_streams_t &streams);
+proc_status_t builtin_run(parser_t &parser, wchar_t **argv, io_streams_t &streams);
 
 wcstring_list_t builtin_get_names();
 void builtin_get_names(completion_list_t *list);

src/exec.cpp

@@ -424,7 +424,7 @@ static bool exec_internal_builtin_proc(parser_t &parser, const std::shared_ptr<j
     j->mut_flags().foreground = false;
 
     // Note this call may block for a long time, while the builtin performs I/O.
-    p->status = builtin_run(parser, j->pgid, p->get_argv(), streams);
+    p->status = builtin_run(parser, p->get_argv(), streams);
 
     // Restore the fg flag, which is temporarily set to false during builtin
     // execution so as not to confuse some job-handling builtins.

src/proc.cpp

@@ -829,18 +829,6 @@ int terminal_maybe_give_to_job(const job_t *j, bool continuing_from_stopped) {
     return success;
 }
 
-pid_t terminal_acquire_before_builtin(int job_pgid) {
-    pid_t selfpgid = getpgrp();
-
-    pid_t current_owner = tcgetpgrp(STDIN_FILENO);
-    if (current_owner >= 0 && current_owner != selfpgid && current_owner == job_pgid) {
-        if (tcsetpgrp(STDIN_FILENO, selfpgid) == 0) {
-            return current_owner;
-        }
-    }
-    return -1;
-}
-
 /// 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.
 static bool terminal_return_from_job(job_t *j, int restore_attrs) {

src/proc.h

@@ -594,10 +594,6 @@ void hup_background_jobs(const parser_t &parser);
 /// \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);
 
-/// Given that we are about to run a builtin, acquire the terminal if it is owned by the given job.
-/// Returns the pid to restore after running the builtin, or -1 if there is no pid to restore.
-pid_t terminal_acquire_before_builtin(int job_pgid);
-
 /// 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.
 void add_disowned_pgid(pid_t pgid);