Clean up some logic around when process exit events are sent

src/exec.cpp

@@ -1013,20 +1013,6 @@ static process_t *get_deferred_process(const shared_ptr<job_t> &j) {
 /// \return true if fish should claim the process group for this job.
 /// This is true if there is at least one external process and if the first process is fish code.
 static bool should_claim_process_group_for_job(const shared_ptr<job_t> &j) {
-    // Check if there's an external process.
-    // This is because historically fish has not reported job exits for internal-only processes,
-    // which is determined by comparing the pgrp against INVALID_PID.
-    bool has_external = false;
-    for (const auto &p : j->processes) {
-        if (p->type == process_type_t::external) {
-            has_external = true;
-            break;
-        }
-    }
-    if (!has_external) {
-        return false;
-    }
-
     // Check the first process.
     // 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

src/proc.cpp

@@ -148,6 +148,25 @@ bool job_t::is_completed() const {
     return true;
 }
 
+bool job_t::should_report_process_exits() const {
+    // This implements the behavior of process exit events only being sent for jobs containing an
+    // external process. Bizarrely the process exit event is for the pgroup leader which may be fish
+    // itself.
+    // TODO: rationalize this.
+    // If we never got a pgid then we never launched the external process, so don't report it.
+    if (this->pgid == INVALID_PID) {
+        return false;
+    }
+
+    // Return whether we have an external process.
+    for (const auto &p : this->processes) {
+        if (p->type == process_type_t::external) {
+            return true;
+        }
+    }
+    return false;
+}
+
 bool job_t::job_chain_is_fully_constructed() const {
     const job_t *cursor = this;
     while (cursor) {
@@ -585,7 +604,7 @@ static bool process_clean_after_marking(parser_t &parser, bool allow_interactive
         // Prepare events for completed jobs, except for jobs that themselves came from event
         // handlers.
         if (!j->from_event_handler() && j->is_completed()) {
-            if (j->pgid != INVALID_PID) {
+            if (j->should_report_process_exits()) {
                 exit_events.push_back(
                     proc_create_event(L"JOB_EXIT", event_type_t::exit, -j->pgid, 0));
             }

src/proc.h

@@ -423,6 +423,10 @@ class job_t {
     bool skip_notification() const { return properties.skip_notification; }
     bool from_event_handler() const { return properties.from_event_handler; }
 
+    /// \return whether we should report process exit events.
+    /// This implements some historical behavior which has not been justified.
+    bool should_report_process_exits() const;
+
     /// \return the parent job, or nullptr.
     const std::shared_ptr<job_t> get_parent() const { return parent_job; }