Migrate some job flags into const properties struct

This helps clarify which parts of a job are mutable, and which are constant.

src/builtin_bg.cpp

@@ -19,7 +19,7 @@
 /// Helper function for builtin_bg().
 static int send_to_bg(parser_t &parser, io_streams_t &streams, job_t *j) {
     assert(j != NULL);
-    if (!j->get_flag(job_flag_t::JOB_CONTROL)) {
+    if (!j->wants_job_control()) {
         streams.err.append_format(
             _(L"%ls: Can't put job %d, '%ls' to background because it is not under job control\n"),
             L"bg", j->job_id, j->command_wcstr());
@@ -54,7 +54,7 @@ int builtin_bg(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
         // No jobs were specified so use the most recent (i.e., last) job.
         job_t *job = nullptr;
         for (const auto &j : parser.jobs()) {
-            if (j->is_stopped() && j->get_flag(job_flag_t::JOB_CONTROL) && (!j->is_completed())) {
+            if (j->is_stopped() && j->wants_job_control() && (!j->is_completed())) {
                 job = j.get();
                 break;
             }

src/builtin_fg.cpp

@@ -40,8 +40,7 @@ int builtin_fg(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
 
         for (const auto &j : parser.jobs()) {
             if (j->is_constructed() && (!j->is_completed()) &&
-                ((j->is_stopped() || (!j->is_foreground())) &&
-                 j->get_flag(job_flag_t::JOB_CONTROL))) {
+                ((j->is_stopped() || (!j->is_foreground())) && j->wants_job_control())) {
                 job = j.get();
                 break;
             }
@@ -77,7 +76,7 @@ int builtin_fg(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
             if (!job || !job->is_constructed() || job->is_completed()) {
                 streams.err.append_format(_(L"%ls: No suitable job: %d\n"), cmd, pid);
                 job = nullptr;
-            } else if (!job->get_flag(job_flag_t::JOB_CONTROL)) {
+            } else if (!job->wants_job_control()) {
                 streams.err.append_format(_(L"%ls: Can't put job %d, '%ls' to foreground because "
                                             L"it is not under job control\n"),
                                           cmd, pid, job->command_wcstr());

src/exec.cpp

@@ -278,10 +278,10 @@ void internal_exec_helper(parser_t &parser, parsed_source_ref_t parsed_source, t
 // foreground process group, we don't use posix_spawn if we're going to foreground the process. (If
 // we use fork(), we can call tcsetpgrp after the fork, before the exec, and avoid the race).
 static bool can_use_posix_spawn_for_job(const std::shared_ptr<job_t> &job) {
-    if (job->get_flag(job_flag_t::JOB_CONTROL)) {  //!OCLINT(collapsible if statements)
+    if (job->wants_job_control()) {  //!OCLINT(collapsible if statements)
         // We are going to use job control; therefore when we launch this job it will get its own
         // process group ID. But will it be foregrounded?
-        if (job->get_flag(job_flag_t::TERMINAL) && job->is_foreground()) {
+        if (job->wants_terminal() && job->is_foreground()) {
             // It will be foregrounded, so we will call tcsetpgrp(), therefore do not use
             // posix_spawn.
             return false;
@@ -327,7 +327,7 @@ static void on_process_created(const std::shared_ptr<job_t> &j, pid_t child_pid)
         return;
     }
 
-    if (j->get_flag(job_flag_t::JOB_CONTROL)) {
+    if (j->wants_job_control()) {
         j->pgid = child_pid;
     } else {
         j->pgid = getpgrp();
@@ -744,7 +744,7 @@ static bool exec_external_command(parser_t &parser, const std::shared_ptr<job_t>
         // child group has been set. See discussion here:
         // https://github.com/Microsoft/WSL/issues/2997 And confirmation that this persists
         // past glibc 2.24+ here: https://github.com/fish-shell/fish-shell/issues/4715
-        if (j->get_flag(job_flag_t::JOB_CONTROL) && getpgid(p->pid) != j->pgid) {
+        if (j->wants_job_control() && getpgid(p->pid) != j->pgid) {
             set_child_group(j.get(), p->pid);
         }
 #else

src/parse_execution.cpp

@@ -1231,20 +1231,23 @@ parse_execution_result_t parse_execution_context_t::run_1_job(tnode_t<g::job> jo
         return result;
     }
 
-    shared_ptr<job_t> job = std::make_shared<job_t>(acquire_job_id(), block_io, parent_job);
-    auto &ld = parser->libdata();
-    job->tmodes = tmodes;
+    const auto &ld = parser->libdata();
+
     auto job_control_mode = get_job_control_mode();
-    job->set_flag(job_flag_t::JOB_CONTROL,
-                  (job_control_mode == job_control_t::all) ||
-                      ((job_control_mode == job_control_t::interactive) && shell_is_interactive()));
+    bool wants_job_control =
+        (job_control_mode == job_control_t::all) ||
+        ((job_control_mode == job_control_t::interactive) && shell_is_interactive());
 
-    job->set_flag(job_flag_t::FOREGROUND, !job_node_is_background(job_node));
+    job_t::properties_t props{};
+    props.foreground = !job_node_is_background(job_node);
+    props.wants_terminal = wants_job_control && !ld.is_event;
+    props.skip_notification =
+        ld.is_subshell || ld.is_block || ld.is_event || !shell_is_interactive();
 
-    job->set_flag(job_flag_t::TERMINAL, job->get_flag(job_flag_t::JOB_CONTROL) && !ld.is_event);
+    shared_ptr<job_t> job = std::make_shared<job_t>(acquire_job_id(), props, block_io, parent_job);
+    job->tmodes = tmodes;
 
-    job->set_flag(job_flag_t::SKIP_NOTIFICATION,
-                  ld.is_subshell || ld.is_block || ld.is_event || !shell_is_interactive());
+    job->set_flag(job_flag_t::JOB_CONTROL, wants_job_control);
 
     // 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/postfork.cpp

@@ -41,7 +41,7 @@
 /// process if it is the first in a JOB_CONTROL job.
 /// Returns true on sucess, false on failiure.
 bool child_set_group(job_t *j, process_t *p) {
-    if (j->get_flag(job_flag_t::JOB_CONTROL)) {
+    if (j->wants_job_control()) {
         if (j->pgid == INVALID_PID) {
             j->pgid = p->pid;
         }
@@ -107,7 +107,7 @@ bool child_set_group(job_t *j, process_t *p) {
 /// group in the case of JOB_CONTROL, and we can give the new process group control of the terminal
 /// if it's to run in the foreground.
 bool set_child_group(job_t *j, pid_t child_pid) {
-    if (j->get_flag(job_flag_t::JOB_CONTROL)) {
+    if (j->wants_job_control()) {
         assert(j->pgid != INVALID_PID &&
                "set_child_group called with JOB_CONTROL before job pgid determined!");
 
@@ -135,7 +135,7 @@ bool set_child_group(job_t *j, pid_t child_pid) {
 }
 
 bool maybe_assign_terminal(const job_t *j) {
-    if (j->get_flag(job_flag_t::TERMINAL) && j->is_foreground()) {  //!OCLINT(early exit)
+    if (j->wants_terminal() && j->is_foreground()) {  //!OCLINT(early exit)
         if (tcgetpgrp(STDIN_FILENO) == j->pgid) {
             // We've already assigned the process group control of the terminal when the first
             // process in the job was started. There's no need to do so again, and on some platforms
@@ -168,7 +168,7 @@ int child_setup_process(const job_t *job, process_t *p, const dup2_list_t &dup2s
     // Set the handling for job control signals back to the default.
     signal_reset_handlers();
 
-    if (job != nullptr && job->get_flag(job_flag_t::TERMINAL) && job->is_foreground()) {
+    if (job != nullptr && job->wants_terminal() && job->is_foreground()) {
         // Assign the terminal within the child to avoid the well-known race between tcsetgrp() in
         // the parent and the child executing. We are not interested in error handling here, except
         // we try to avoid this for non-terminals; in particular pipelines often make non-terminal
@@ -241,7 +241,7 @@ bool fork_actions_make_spawn_properties(posix_spawnattr_t *attr,
 
     bool should_set_process_group_id = false;
     int desired_process_group_id = 0;
-    if (j->get_flag(job_flag_t::JOB_CONTROL)) {
+    if (j->wants_job_control()) {
         should_set_process_group_id = true;
 
         // set_child_group puts each job into its own process group

src/proc.cpp

@@ -279,13 +279,9 @@ void process_t::check_generations_before_launch() {
     gens_ = topic_monitor_t::principal().current_generations();
 }
 
-job_t::job_t(job_id_t jobid, io_chain_t bio, std::shared_ptr<job_t> parent)
-    : block_io(std::move(bio)),
-      parent_job(std::move(parent)),
-      pgid(INVALID_PID),
-      tmodes(),
-      job_id(jobid),
-      flags{} {}
+job_t::job_t(job_id_t job_id, const properties_t &props, io_chain_t bio,
+             std::shared_ptr<job_t> parent)
+    : properties(props), block_io(std::move(bio)), parent_job(std::move(parent)), job_id(job_id) {}
 
 job_t::~job_t() { release_job_id(job_id); }
 
@@ -490,7 +486,7 @@ static bool try_clean_process_in_job(process_t *p, job_t *j, std::vector<event_t
 
     // Handle signals other than SIGPIPE.
     // Always report crashes.
-    if (j->get_flag(job_flag_t::SKIP_NOTIFICATION) && !contains(crashsignals, s.signal_code())) {
+    if (j->skip_notification() && !contains(crashsignals, s.signal_code())) {
         return false;
     }
 
@@ -540,7 +536,7 @@ static bool job_wants_message(const shared_ptr<job_t> &j) {
     if (j->get_flag(job_flag_t::NOTIFIED)) return false;
 
     // Do we just skip notifications?
-    if (j->get_flag(job_flag_t::SKIP_NOTIFICATION)) return false;
+    if (j->skip_notification()) return false;
 
     // Are we foreground?
     // The idea here is to not print status messages for jobs that execute in the foreground (i.e.
@@ -872,7 +868,7 @@ void job_t::continue_job(parser_t &parser, bool reclaim_foreground_pgrp, bool se
     });
 
     if (!is_completed()) {
-        if (get_flag(job_flag_t::TERMINAL) && is_foreground()) {
+        if (wants_terminal() && is_foreground()) {
             // Put the job into the foreground and give it control of the terminal.
             // Hack: ensure that stdin is marked as blocking first (issue #176).
             make_fd_blocking(STDIN_FILENO);
@@ -980,7 +976,7 @@ void hup_background_jobs(const parser_t &parser) {
     // TODO: we should probably hup all jobs across all parsers here.
     for (const auto &j : parser.jobs()) {
         // Make sure we don't try to SIGHUP the calling builtin
-        if (j->pgid == INVALID_PID || !j->get_flag(job_flag_t::JOB_CONTROL)) {
+        if (j->pgid == INVALID_PID || !j->wants_job_control()) {
             continue;
         }
 

src/proc.h

@@ -140,6 +140,12 @@ class internal_proc_t {
     internal_proc_t();
 };
 
+/// 0 should not be used; although it is not a valid PGID in userspace,
+///   the Linux kernel will use it for kernel processes.
+/// -1 should not be used; it is a possible return value of the getpgid()
+///   function
+enum { INVALID_PID = -2 };
+
 /// A structure representing a single fish process. Contains variables for tracking process state
 /// and the process argument list. Actually, a fish process can be either a regular external
 /// process, an internal builtin which may or may not spawn a fake IO process during execution, a
@@ -255,15 +261,10 @@ enum class job_flag_t {
     /// Whether the specified job is completely constructed, i.e. completely parsed, and every
     /// process in the job has been forked, etc.
     CONSTRUCTED,
-    /// Whether the specified job is a part of a subshell, event handler or some other form of
-    /// special job that should not be reported.
-    SKIP_NOTIFICATION,
     /// Whether the exit status should be negated. This flag can only be set by the not builtin.
     NEGATE,
     /// Whether the job is under job control, i.e. has its own pgrp.
     JOB_CONTROL,
-    /// Whether the job wants to own the terminal when in the foreground.
-    TERMINAL,
     /// This job is disowned, and should be removed from the active jobs list.
     DISOWN_REQUESTED,
 
@@ -282,6 +283,25 @@ void release_job_id(job_id_t jobid);
 /// A struct represeting a job. A job is basically a pipeline of one or more processes and a couple
 /// of flags.
 class job_t {
+   public:
+    /// A set of jobs properties. These are immutable: they do not change for the lifetime of the
+    /// job.
+    struct properties_t {
+        /// Whether this job is in the foreground, i.e. whether it did NOT have a & at the end.
+        bool foreground{};
+
+        /// Whether the specified job is a part of a subshell, event handler or some other form of
+        /// special job that should not be reported.
+        bool skip_notification{};
+
+        /// Whether the job wants to own the terminal when in the foreground.
+        bool wants_terminal{};
+    };
+
+   private:
+    /// Set of immutable job properties.
+    const properties_t properties;
+
     /// The original command which led to the creation of this job. It is used for displaying
     /// messages about job status on the terminal.
     wcstring command_str;
@@ -298,7 +318,8 @@ class job_t {
     void operator=(const job_t &) = delete;
 
    public:
-    job_t(job_id_t jobid, io_chain_t bio, std::shared_ptr<job_t> parent);
+    job_t(job_id_t job_id, const properties_t &props, io_chain_t bio,
+          std::shared_ptr<job_t> parent);
     ~job_t();
 
     /// Returns whether the command is empty.
@@ -356,21 +377,29 @@ class job_t {
 
     /// Process group ID for the process group that this job is running in.
     /// Set to a nonexistent, non-return-value of getpgid() integer by the constructor
-    pid_t pgid;
+    pid_t pgid{INVALID_PID};
+
+    /// The id of this job.
+    const job_id_t 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;
-    /// The job id of the job. This is a small integer that is a unique identifier of the job within
-    /// this shell, and is used e.g. in process expansion.
-    const job_id_t job_id;
+    struct termios tmodes {};
+
     /// Bitset containing information about the job. A combination of the JOB_* constants.
-    enum_set_t<job_flag_t> flags;
+    enum_set_t<job_flag_t> flags{};
 
     // Get and set flags
     bool get_flag(job_flag_t flag) const;
     void set_flag(job_flag_t flag, bool set);
 
+    /// \return if we want job control.
+    bool wants_job_control() const { return get_flag(job_flag_t::JOB_CONTROL); }
+
+    /// \return if this job wants to own the terminal in the foreground.
+    bool wants_terminal() const { return properties.wants_terminal; }
+
     /// Returns the block IO redirections associated with the job. These are things like the IO
     /// redirections associated with the begin...end statement.
     const io_chain_t &block_io_chain() const { return this->block_io; }
@@ -382,7 +411,7 @@ class job_t {
     /// The job has been fully constructed, i.e. all its member processes have been launched
     bool is_constructed() const { return get_flag(job_flag_t::CONSTRUCTED); }
     /// The job was launched in the foreground and has control of the terminal
-    bool is_foreground() const { return get_flag(job_flag_t::FOREGROUND); }
+    bool is_foreground() const { return properties.foreground; }
     /// The job is complete, i.e. all its member processes have been reaped
     bool is_completed() const;
     /// The job is in a stopped state
@@ -391,6 +420,7 @@ class job_t {
     bool is_visible() const {
         return !is_completed() && is_constructed() && !get_flag(job_flag_t::DISOWN_REQUESTED);
     };
+    bool skip_notification() const { return properties.skip_notification; }
 
     /// \return the parent job, or nullptr.
     const std::shared_ptr<job_t> get_parent() const { return parent_job; }
@@ -509,12 +539,6 @@ pid_t terminal_acquire_before_builtin(int job_pgid);
 /// Used to avoid zombie processes after disown.
 void add_disowned_pgid(pid_t pgid);
 
-/// 0 should not be used; although it is not a valid PGID in userspace,
-///   the Linux kernel will use it for kernel processes.
-/// -1 should not be used; it is a possible return value of the getpgid()
-///   function
-enum { INVALID_PID = -2 };
-
 bool have_proc_stat();
 
 #endif