Replace INVALID_PID constant with Option<NonZeroU32>

If we end up using this in more places, we can create a `Pid` newtype.
Note that while the constant is no longer used in code, its previous value of -2
is still printed by `jobs` when no pgid is associated with a job. I will open a
PR to change this to something else, likely either `0` or `-`.

src/builtins/jobs.rs

@@ -8,7 +8,7 @@ use crate::common::{escape_string, timef, EscapeFlags, EscapeStringStyle};
 use crate::io::IoStreams;
 use crate::job_group::{JobId, MaybeJobId};
 use crate::parser::Parser;
-use crate::proc::{clock_ticks_to_seconds, have_proc_stat, proc_get_jiffies, Job, INVALID_PID};
+use crate::proc::{clock_ticks_to_seconds, have_proc_stat, proc_get_jiffies, Job};
 use crate::wchar_ext::WExt;
 use crate::wgetopt::{wopt, ArgType, WGetopter, WOption};
 use crate::wutil::wgettext;
@@ -50,12 +50,8 @@ fn cpu_use(j: &Job) -> f64 {
 
 /// Print information about the specified job.
 fn builtin_jobs_print(j: &Job, mode: JobsPrintMode, header: bool, streams: &mut IoStreams) {
-    let mut pgid = INVALID_PID;
-    {
-        if let Some(job_pgid) = j.get_pgid() {
-            pgid = job_pgid;
-        }
-    }
+    // TODO: Breaking change, but don't print meaningless -2 for jobs without a pgid
+    let pgid = j.get_pgid().unwrap_or(-2); // the old INVALID_PGID value
 
     let mut out = WString::new();
     match mode {

src/exec.rs

@@ -37,7 +37,7 @@ use crate::parser::{Block, BlockId, BlockType, EvalRes, Parser};
 use crate::proc::{
     hup_jobs, is_interactive_session, jobs_requiring_warning_on_exit, no_exec,
     print_exit_warning_for_jobs, InternalProc, Job, JobGroupRef, ProcStatus, Process, ProcessType,
-    TtyTransfer, INVALID_PID,
+    TtyTransfer,
 };
 use crate::reader::{reader_run_count, restore_term_mode};
 use crate::redirection::{dup2_list_resolve_chain, Dup2List};
@@ -56,6 +56,7 @@ use nix::fcntl::OFlag;
 use nix::sys::stat;
 use std::ffi::CStr;
 use std::io::{Read, Write};
+use std::num::NonZeroU32;
 use std::os::fd::{AsRawFd, OwnedFd, RawFd};
 use std::slice;
 use std::sync::atomic::Ordering;
@@ -491,7 +492,7 @@ fn internal_exec(vars: &EnvStack, j: &Job, block_io: IoChain) {
     // child_setup_process makes sure signals are properly set up.
     let redirs = dup2_list_resolve_chain(&all_ios);
     if child_setup_process(
-        0, /* not claim_tty */
+        None, /* not claim_tty */
         blocked_signals,
         false, /* not is_forked */
         &redirs,
@@ -677,9 +678,10 @@ fn fork_child_for_process(
 ) -> LaunchResult {
     // Claim the tty from fish, if the job wants it and we are the pgroup leader.
     let claim_tty_from = if p.leads_pgrp && job.group().wants_terminal() {
-        unsafe { libc::getpgrp() }
+        // getpgrp(2) cannot fail and always returns the (positive) caller's pgid
+        Some(NonZeroU32::new(unsafe { libc::getpgrp() } as u32).unwrap())
     } else {
-        INVALID_PID
+        None
     };
 
     // Decide if the job wants to set a custom sigmask.

src/fork_exec/postfork.rs

@@ -8,6 +8,7 @@ use crate::signal::signal_reset_handlers;
 use crate::{common::exit_without_destructors, wutil::fstat};
 use libc::{c_char, pid_t, O_RDONLY};
 use std::ffi::CStr;
+use std::num::NonZeroU32;
 use std::os::unix::fs::MetadataExt;
 use std::time::Duration;
 
@@ -143,7 +144,7 @@ pub fn execute_setpgid(pid: pid_t, pgroup: pid_t, is_parent: bool) -> i32 {
 
 /// Set up redirections and signal handling in the child process.
 pub fn child_setup_process(
-    claim_tty_from: pid_t,
+    claim_tty_from: Option<NonZeroU32>,
     sigmask: Option<&libc::sigset_t>,
     is_forked: bool,
     dup2s: &Dup2List,
@@ -173,7 +174,10 @@ pub fn child_setup_process(
             return err;
         }
     }
-    if claim_tty_from >= 0 && unsafe { libc::tcgetpgrp(libc::STDIN_FILENO) } == claim_tty_from {
+    if claim_tty_from
+        // tcgetpgrp() can return -1 but pid.get() cannot, so cast the latter to the former
+        .is_some_and(|pid| unsafe { libc::tcgetpgrp(libc::STDIN_FILENO) } == pid.get() as i32)
+    {
         // 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

src/proc.rs

@@ -308,12 +308,6 @@ impl InternalProc {
     }
 }
 
-/// 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
-pub const INVALID_PID: i32 = -2;
-
 // Allows transferring the tty to a job group, while it runs.
 #[derive(Default)]
 pub struct TtyTransfer {
@@ -855,6 +849,7 @@ impl Job {
 
     /// Return our pgid, or none if we don't have one, or are internal to fish
     /// This never returns fish's own pgroup.
+    // TODO: Return a type-safe result.
     pub fn get_pgid(&self) -> Option<libc::pid_t> {
         self.group().get_pgid()
     }
@@ -862,6 +857,7 @@ impl Job {
     /// Return the pid of the last external process in the job.
     /// This may be none if the job consists of just internal fish functions or builtins.
     /// This will never be fish's own pid.
+    // TODO: Return a type-safe result.
     pub fn get_last_pid(&self) -> Option<libc::pid_t> {
         self.processes()
             .iter()