Eliminate pgroup_provenance_t

Now that job trees are a single source of truth for a job's pgid, we no
longer need fancy logic around how the pgroup is assigned.

src/exec.cpp

@@ -52,35 +52,6 @@
 /// Number of calls to fork() or posix_spawn().
 static relaxed_atomic_t<int> s_fork_count{0};
 
-pgroup_provenance_t get_pgroup_provenance(const shared_ptr<job_t> &j) {
-    bool first_proc_is_internal = j->processes.front()->is_internal();
-    bool has_internal = j->has_internal_proc();
-    bool has_external = j->has_external_proc();
-    assert(first_proc_is_internal ? has_internal : has_external);
-
-    if (j->job_tree->get_pgid().has_value()) {
-        // Our job tree already has a pgid.
-        return pgroup_provenance_t::lineage;
-    } else if (!j->wants_job_control()) {
-        // This job doesn't need job control, it can just live in the fish pgroup.
-        return pgroup_provenance_t::fish_itself;
-    } else if (has_external && !first_proc_is_internal) {
-        // The first process is external, it will own the pgroup.
-        return pgroup_provenance_t::first_external_proc;
-    } else if (has_external && first_proc_is_internal) {
-        // The terminal owner has to be the process which is permitted to read from stdin.
-        // This is the first process in the pipeline. When executing, a process in the job will
-        // claim the pgrp if it's not set; therefore set it according to the first process.
-        // Only do this if there's an external process - see #6011.
-        return pgroup_provenance_t::fish_itself;
-    } else {
-        assert(has_internal && !has_external && "Should be internal only");
-        // This job consists of only internal functions or builtins; we do not need to assign a
-        // pgroup (yet).
-        return pgroup_provenance_t::unassigned;
-    }
-}
-
 /// This function is executed by the child process created by a call to fork(). It should be called
 /// after \c child_setup_process. It calls execve to replace the fish process image with the command
 /// specified in \c p. It never returns. Called in a forked child! Do not allocate memory, etc.
@@ -204,11 +175,11 @@ static void internal_exec(env_stack_t &vars, job_t *j, const io_chain_t &block_i
 /// If our pgroup assignment mode wants us to use the first external proc, then apply it here.
 /// \returns the job's pgid, which should always be set to something valid after this call.
 static pid_t maybe_assign_pgid_from_child(const std::shared_ptr<job_t> &j, pid_t child_pid) {
-    if (j->pgroup_provenance == pgroup_provenance_t::first_external_proc &&
-        !j->job_tree->get_pgid()) {
-        j->job_tree->set_pgid(child_pid);
+    auto &jt = j->job_tree;
+    if (jt->needs_pgid_assignment()) {
+        jt->set_pgid(child_pid);
     }
-    return *j->job_tree->get_pgid();
+    return *jt->get_pgid();
 }
 
 /// Construct an internal process for the process p. In the background, write the data \p outdata to
@@ -951,26 +922,6 @@ bool exec_job(parser_t &parser, const shared_ptr<job_t> &j, const io_chain_t &bl
     pid_t pgrp = getpgrp();
     // Check to see if we should reclaim the foreground pgrp after the job finishes or stops.
     const bool reclaim_foreground_pgrp = (tcgetpgrp(STDIN_FILENO) == pgrp);
-
-    // Perhaps we know our pgroup already.
-    switch (j->pgroup_provenance) {
-        case pgroup_provenance_t::lineage: {
-            break;
-        }
-
-        case pgroup_provenance_t::fish_itself:
-            // TODO: should not need this 'if' here. Rationalize this.
-            if (! j->job_tree->is_placeholder()) {
-                j->job_tree->set_pgid(pgrp);
-            }
-            break;
-
-        // The remaining cases are all deferred until later.
-        case pgroup_provenance_t::unassigned:
-        case pgroup_provenance_t::first_external_proc:
-            break;
-    }
-
     const size_t stdout_read_limit = parser.libdata().read_limit;
 
     // Get the list of all FDs so we can ensure our pipes do not conflict.

src/exec.h

@@ -37,10 +37,6 @@ int exec_subshell_for_expand(const wcstring &cmd, parser_t &parser, const job_tr
 /// Loops over close until the syscall was run without being interrupted.
 void exec_close(int fd);
 
-/// Compute the "pgroup provenance" for a job. This is a description of how the pgroup is
-/// assigned. It's factored out because the logic has subtleties, and this centralizes it.
-pgroup_provenance_t get_pgroup_provenance(const std::shared_ptr<job_t> &j);
-
 /// Add signals that should be masked for external processes in this job.
 bool blocked_signals_for_job(const job_t &job, sigset_t *sigmask);
 

src/parse_execution.cpp

@@ -1289,7 +1289,6 @@ end_execution_reason_t parse_execution_context_t::run_1_job(tnode_t<g::job> job_
     if (pop_result == end_execution_reason_t::ok) {
         // Set the pgroup assignment mode and job tree, now that the job is populated.
         job_tree_t::populate_tree_for_job(job.get(), ctx.job_tree);
-        job->pgroup_provenance = get_pgroup_provenance(job);
         assert(job->job_tree && "Should have a job tree");
 
         // Success. Give the job to the parser - it will clean it up.

src/postfork.cpp

@@ -208,8 +208,7 @@ bool fork_actions_make_spawn_properties(posix_spawnattr_t *attr,
     if (auto job_pgid = j->job_tree->get_pgid()) {
         desired_pgid = *job_pgid;
     } else {
-        assert(j->pgroup_provenance == pgroup_provenance_t::first_external_proc &&
-               "We should have already known our pgroup");
+        assert(j->job_tree->needs_pgid_assignment() && "We should be expecting a pgid");
         // We are the first external proc in the job tree. Set the desired_pgid to 0 to indicate we
         // should creating a new process group.
         desired_pgid = 0;

src/proc.cpp

@@ -253,9 +253,9 @@ job_tree_t::~job_tree_t() {
 }
 
 void job_tree_t::set_pgid(pid_t pgid) {
-    // TODO: thread safety?
-    assert(!pgid_.has_value() && "Already has a pgid");
-    assert(!is_placeholder() && "Cannot set a pgid on the placeholder");
+    // Note we need not be concerned about thread safety. job_trees are intended to be shared across
+    // threads, but their pgid should always have been set beforehand.
+    assert(needs_pgid_assignment() && "We should not be setting a pgid");
     assert(pgid >= 0 && "Invalid pgid");
     pgid_ = pgid;
 }
@@ -265,11 +265,12 @@ maybe_t<pid_t> job_tree_t::get_pgid() const { return pgid_; }
 void job_tree_t::populate_tree_for_job(job_t *job, const job_tree_ref_t &proposed) {
     // Note there's three cases to consider:
     //  nullptr         -> this is a root job, there is no inherited job tree
-    //  placeholder     -> we are running as part of a simple function execution, create a new job
-    //                      tree for any spawned jobs
+    //  placeholder     -> the parent is running as part of a simple function execution
+    //                     We may need to create a new job tree if we are going to fork.
     //  non-placeholder -> we are running as part of a real pipeline
     // Decide if this job can use the placeholder tree.
     // This is true if it's a simple foreground execution of an internal proc.
+    bool first_proc_internal = job->processes.front()->is_internal();
     bool can_use_placeholder = !job->is_initially_background() && job->processes.size() == 1 &&
                                job->processes.front()->is_internal();
 
@@ -286,12 +287,22 @@ void job_tree_t::populate_tree_for_job(job_t *job, const job_tree_ref_t &propose
     }
 
     job->mut_flags().is_tree_root = needs_new_tree;
+    bool job_control = job->wants_job_control();
 
     if (!needs_new_tree) {
         job->job_tree = proposed;
+    } else if (can_use_placeholder) {
+        job->job_tree.reset(new job_tree_t(job_control, true));
     } else {
-        job->job_tree =
-            job_tree_ref_t(new job_tree_t(job->wants_job_control(), can_use_placeholder));
+        job->job_tree.reset(new job_tree_t(job_control, false));
+
+        // Perhaps this job should immediately live in fish's pgroup.
+        // There's two reasons why it may be so:
+        //  1. The job doesn't need job control.
+        //  2. The first process in the job is internal to fish; this needs to own the tty.
+        if (!job_control || first_proc_internal) {
+            job->job_tree->set_pgid(getpgrp());
+        }
     }
 }
 

src/proc.h

@@ -191,6 +191,10 @@ class job_tree_t {
     /// \return whether this is a placeholder.
     bool is_placeholder() const { return is_placeholder_; }
 
+    /// \return whether this job tree is awaiting a pgid.
+    /// This is true for non-placeholder trees that don't already have a pgid.
+    bool needs_pgid_assignment() const { return !is_placeholder_ && !pgid_.has_value(); }
+
     /// \return the job ID, or -1 if none.
     job_id_t get_id() const { return job_id_; }
 
@@ -344,24 +348,6 @@ job_id_t acquire_job_id(void);
 void release_job_id(job_id_t jid);
 
 
-/// A job has a mode which describes how its pgroup is assigned (before the value is known).
-/// This is a constant property of the job.
-enum class pgroup_provenance_t {
-    /// The job has no pgroup assignment. This is used for e.g. a simple function invocation with no
-    /// pipeline.
-    unassigned,
-
-    /// The job's pgroup is fish's pgroup. This is used when fish needs to read from the terminal,
-    /// or if job control is disabled.
-    fish_itself,
-
-    /// The job's pgroup will come from its first external process.
-    first_external_proc,
-
-    /// The job's pgroup will come from its lineage. This is used for jobs that are run nested.
-    lineage,
-};
-
 /// A struct representing a job. A job is a pipeline of one or more processes.
 class job_t {
    public:
@@ -466,9 +452,6 @@ class job_t {
     /// This may also be fish itself.
     maybe_t<pid_t> get_pgid() const { return job_tree->get_pgid(); }
 
-    /// How the above pgroup is assigned. This should be set at construction and not modified after.
-    pgroup_provenance_t pgroup_provenance{};
-
     /// The id of this job.
     /// This is user-visible, is recycled, and may be -1.
     job_id_t job_id() const { return job_tree->get_id(); }