Revert "lint and style cleanups"

This reverts commit acdb81bbca.
It was meant for the major branch.

src/env.h

@@ -71,8 +71,8 @@ void misc_init();
 
 class env_var_t {
    private:
-    wcstring val;
     bool is_missing;
+    wcstring val;
 
    public:
     static env_var_t missing_var() {

src/exec.cpp

@@ -19,7 +19,6 @@
 #include <unistd.h>
 
 #include <algorithm>
-#include <functional>
 #include <map>
 #include <memory>
 #include <string>
@@ -38,7 +37,6 @@
 #include "postfork.h"
 #include "proc.h"
 #include "reader.h"
-#include "signal.h"
 #include "wutil.h"  // IWYU pragma: keep
 
 /// File descriptor redirection error message.
@@ -495,8 +493,8 @@ void exec_job(parser_t &parser, job_t *j) {
     // We are careful to set these to -1 when closed, so if we exit the loop abruptly, we can still
     // close them.
     bool pgrp_set = false;
-    // This is static since processes can block on input/output across jobs the main exec_job loop
+    //this is static since processes can block on input/output across jobs
-    // is only ever run in a single thread, so this is OK.
+    //the main exec_job loop is only ever run in a single thread, so this is OK
     static pid_t blocked_pid = -1;
     int pipe_current_read = -1, pipe_current_write = -1, pipe_next_read = -1;
     for (std::unique_ptr<process_t> &unique_p : j->processes) {
@@ -515,7 +513,7 @@ void exec_job(parser_t &parser, job_t *j) {
 
         // See if we need a pipe.
         const bool pipes_to_next_command = !p->is_last_in_job;
-        // Set to true if we end up forking for this process.
+        //set to true if we end up forking for this process
         bool child_forked = false;
         bool child_spawned = false;
         bool block_child = true;
@@ -624,12 +622,12 @@ void exec_job(parser_t &parser, job_t *j) {
         // a pipeline.
         shared_ptr<io_buffer_t> block_output_io_buffer;
 
-        // Used to SIGCONT the previously SIGSTOP'd process so the main loop or next command in job
+        //used to SIGCONT the previously SIGSTOP'd process so the main loop or next command in job can read
-        // can read from its output.
+        //from its output.
-        auto unblock_previous = [&j]() {
+        auto unblock_previous = [&j] () {
-            // We've already called waitpid after forking the child, so we've guaranteed that
+            //we've already called waitpid after forking the child, so we've guaranteed that they're
-            // they're already SIGSTOP'd. Otherwise we'd be risking a deadlock because we can call
+            //already SIGSTOP'd. Otherwise we'd be risking a deadlock because we can call SIGCONT before
-            // SIGCONT before they've actually stopped, and they'll remain suspended indefinitely.
+            //they've actually stopped, and they'll remain suspended indefinitely.
             if (blocked_pid != -1) {
                 debug(2, L"Unblocking process %d.\n", blocked_pid);
                 kill(blocked_pid, SIGCONT);
@@ -655,7 +653,7 @@ void exec_job(parser_t &parser, job_t *j) {
                 if (block_child) {
                     kill(p->pid, SIGSTOP);
                 }
-                // The parent will wake us up when we're ready to execute.
+                //the parent will wake us up when we're ready to execute
                 setup_child_process(p, process_net_io_chain);
                 child_action();
                 DIE("Child process returned control to do_fork lambda!");
@@ -680,9 +678,10 @@ void exec_job(parser_t &parser, job_t *j) {
             return true;
         };
 
-        // Helper routine executed by INTERNAL_FUNCTION and INTERNAL_BLOCK_NODE to make sure an
+
-        // output buffer exists in case there is another command in the job chain that will be
+        //helper routine executed by INTERNAL_FUNCTION and INTERNAL_BLOCK_NODE to make sure
-        // reading from this command's output.
+        //an output buffer exists in case there is another command in the job chain that will
+        //be reading from this command's output.
         auto verify_buffer_output = [&] () {
             if (!p->is_last_in_job) {
                 // Be careful to handle failure, e.g. too many open fds.
@@ -843,8 +842,8 @@ void exec_job(parser_t &parser, job_t *j) {
                     const int fg = j->get_flag(JOB_FOREGROUND);
                     j->set_flag(JOB_FOREGROUND, false);
 
-                    // Main loop may need to perform a blocking read from previous command's output.
+                    //main loop may need to perform a blocking read from previous command's output.
-                    // Make sure read source is not blocked.
+                    //make sure read source is not blocked
                     unblock_previous();
                     p->status = builtin_run(parser, p->get_argv(), *builtin_io_streams);
 
@@ -940,8 +939,8 @@ void exec_job(parser_t &parser, job_t *j) {
                 bool must_fork = redirection_is_to_real_file(stdout_io.get()) ||
                                  redirection_is_to_real_file(stderr_io.get());
                 if (!must_fork && p->is_last_in_job) {
-                    // We are handling reads directly in the main loop. Make sure source is
+                    //we are handling reads directly in the main loop. Make sure source is unblocked.
-                    // unblocked. Note that we may still end up forking.
+                    //Note that we may still end up forking.
                     unblock_previous();
                     const bool stdout_is_to_buffer = stdout_io && stdout_io->io_mode == IO_BUFFER;
                     const bool no_stdout_output = stdout_buffer.empty();
@@ -1111,19 +1110,18 @@ void exec_job(parser_t &parser, job_t *j) {
 
         bool child_blocked = block_child && child_forked;
         if (child_blocked) {
-            // We have to wait to ensure the child has set their progress group and is in SIGSTOP
+            //we have to wait to ensure the child has set their progress group and is in SIGSTOP state
-            // state otherwise, we can later call SIGCONT before they call SIGSTOP and they'll be
+            //otherwise, we can later call SIGCONT before they call SIGSTOP and they'll be blocked indefinitely.
-            // blocked indefinitely. The child is SIGSTOP'd and is guaranteed to have called
+            //The child is SIGSTOP'd and is guaranteed to have called child_set_group() at this point.
-            // child_set_group() at this point. but we only need to call set_child_group for the
+            //but we only need to call set_child_group for the first process in the group.
-            // first process in the group. If needs_keepalive is set, this has already been called
+            //If needs_keepalive is set, this has already been called for the keepalive process
-            // for the keepalive process.
             pid_t pid_status{};
             int result;
             while ((result = waitpid(p->pid, &pid_status, WUNTRACED)) == -1 && errno == EINTR) {
-                // This could be a superfluous interrupt or Ctrl+C at the terminal In all cases, it
+                //This could be a superfluous interrupt or Ctrl+C at the terminal
-                // is OK to retry since the forking code above is specifically designed to never,
+                //In all cases, it is OK to retry since the forking code above is specifically designed
-                // ever hang/block in a child process before the SIGSTOP call is reached.
+                //to never, ever hang/block in a child process before the SIGSTOP call is reached.
-                ;  // do nothing
+                continue;
             }
             if (result == -1) {
                 exec_error = true;
@@ -1131,26 +1129,26 @@ void exec_job(parser_t &parser, job_t *j) {
                 wperror(L"waitpid");
                 break;
             }
-            // We are not unblocking the child via SIGCONT just yet to give the next process a
+            //we are not unblocking the child via SIGCONT just yet to give the next process a chance to open
-            // chance to open the pipes and join the process group. We only unblock the last process
+            //the pipes and join the process group. We only unblock the last process in the job chain because
-            // in the job chain because no one awaits it.
+            //no one awaits it.
         }
-        // Regardless of whether the child blocked or not: only once per job, and only once we've
+        //regardless of whether the child blocked or not:
-        // actually executed an external command.
+        //only once per job, and only once we've actually executed an external command
         if ((child_spawned || child_forked) && !pgrp_set) {
-            // This should be called after waitpid if child_forked and pipes_to_next_command it can
+            //this should be called after waitpid if child_forked && pipes_to_next_command
-            // be called at any time if child_spawned.
+            //it can be called at any time if child_spawned
             set_child_group(j, p->pid);
-            // we can't rely on p->is_first_in_job because a builtin may have been the first.
+            //we can't rely on p->is_first_in_job because a builtin may have been the first
             pgrp_set = true;
         }
-        // If the command we ran _before_ this one was SIGSTOP'd to let this one catch up, unblock
+        //if the command we ran _before_ this one was SIGSTOP'd to let this one catch up, unblock it now.
-        // it now. this must be after the wait_pid on the process we just started, if any.
+        //this must be after the wait_pid on the process we just started, if any.
         unblock_previous();
 
         if (child_blocked) {
-            // Store the newly-blocked command's PID so that it can be SIGCONT'd once the next
+            //store the newly-blocked command's PID so that it can be SIGCONT'd once the next process
-            // process in the chain is started. That may be in this job or in another job.
+            //in the chain is started. That may be in this job or in another job.
             blocked_pid = p->pid;
         }
 
@@ -1166,7 +1164,7 @@ void exec_job(parser_t &parser, job_t *j) {
             pipe_current_write = -1;
         }
 
-        // Unblock the last process because there's no need for it to stay SIGSTOP'd for anything.
+        //unblock the last process because there's no need for it to stay SIGSTOP'd for anything
         if (p->is_last_in_job) {
             unblock_previous();
         }

src/expand.h

@@ -122,6 +122,8 @@ bool expand_one(wcstring &inout_str, expand_flags_t flags, parse_error_list_t *e
 
 /// Convert the variable value to a human readable form, i.e. escape things, handle arrays, etc.
 /// Suitable for pretty-printing.
+///
+/// \param in the value to escape
 wcstring expand_escape_variable(const env_var_t &var);
 
 /// Perform tilde expansion and nothing else on the specified string, which is modified in place.

src/postfork.cpp

@@ -78,9 +78,9 @@ bool child_set_group(job_t *j, process_t *p) {
         if (j->pgid == -2) {
             j->pgid = p->pid;
         }
-        // Retry on EPERM because there's no way that a child cannot join an existing progress group
+        //retry on EPERM because there's no way that a child cannot join an existing progress group
-        // because we are SIGSTOPing the previous job in the chain. Sometimes we have to try a few
+        //because we are SIGSTOPing the previous job in the chain. Sometimes we have to try a few
-        // times to get the kernel to see the new group. (Linux 4.4.0)
+        //times to get the kernel to see the new group. (Linux 4.4.0)
         int failure = setpgid(p->pid, j->pgid);
         while (failure == -1 && (errno == EPERM || errno == EINTR)) {
             debug_safe(4, "Retrying setpgid in child process");
@@ -112,7 +112,7 @@ bool child_set_group(job_t *j, process_t *p) {
             retval = false;
         }
     } else {
-        // This is probably stays unused in the child.
+        //this is probably stays unused in the child
         j->pgid = getpgrp();
     }
 
@@ -142,16 +142,16 @@ bool set_child_group(job_t *j, pid_t child_pid) {
 
     if (j->get_flag(JOB_TERMINAL) && j->get_flag(JOB_FOREGROUND)) {  //!OCLINT(early exit)
         if (tcgetpgrp(STDIN_FILENO) == j->pgid) {
-            // We've already assigned the process group control of the terminal when the first
+            //we've already assigned the process group control of the terminal when the first process in the job
-            // process in the job was started. There's no need to do so again, and on some platforms
+            //was started. There's no need to do so again, and on some platforms this can cause an EPERM error.
-            // this can cause an EPERM error. In addition, if we've given control of the terminal to
+            //In addition, if we've given control of the terminal to a process group, attempting to call tcsetpgrp
-            // a process group, attempting to call tcsetpgrp from the background will cause SIGTTOU
+            //from the background will cause SIGTTOU to be sent to everything in our process group (unless we
-            // to be sent to everything in our process group (unless we handle it).
+            //handle it)..
             debug(4, L"Process group %d already has control of terminal\n", j->pgid);
         }
         else
         {
-            // No need to duplicate the code here, a function already exists that does just this.
+            //no need to duplicate the code here, a function already exists that does just this
             retval = terminal_give_to_job(j, false /*new job, so not continuing*/);
         }
     }
@@ -264,7 +264,7 @@ int setup_child_process(process_t *p, const io_chain_t &io_chain) {
     bool ok = true;
 
     if (ok) {
-        // In the case of IO_FILE, this can hang until data is available to read/write!
+        //In the case of IO_FILE, this can hang until data is available to read/write!
         ok = (0 == handle_child_io(io_chain));
         if (p != 0 && !ok) {
             debug_safe(4, "handle_child_io failed in setup_child_process");

src/postfork.h

@@ -27,6 +27,7 @@ bool child_set_group(job_t *j, process_t *p); //called by child
 /// inside the exec function, which blocks all signals), and all IO redirections and other file
 /// descriptor actions are performed.
 ///
+/// \param j the job to set up the IO for
 /// \param p the child process to set up
 /// \param io_chain the IO chain to use
 ///

src/proc.cpp

@@ -791,56 +791,53 @@ bool terminal_give_to_job(job_t *j, int cont) {
 
     signal_block();
 
-    // It may not be safe to call tcsetpgrp if we've already done so, as at that point we are no
+    //It may not be safe to call tcsetpgrp if we've already done so, as at that point we are no longer
-    // longer the controlling process group for the terminal and no longer have permission to set
+    //the controlling process group for the terminal and no longer have permission to set the process
-    // the process group that is in control, causing tcsetpgrp to return EPERM, even though that's
+    //group that is in control, causing tcsetpgrp to return EPERM, even though that's not the documented
-    // not the documented behavior in tcsetpgrp(3), which instead says other bad things will happen
+    //behavior in tcsetpgrp(3), which instead says other bad things will happen (it says SIGTTOU will be
-    // (it says SIGTTOU will be sent to all members of the background *calling* process group, but
+    //sent to all members of the background *calling* process group, but it's more complicated than that,
-    // it's more complicated than that, SIGTTOU may or may not be sent depending on the TTY
+    //SIGTTOU may or may not be sent depending on the TTY configuration and whether or not signal handlers
-    // configuration and whether or not signal handlers for SIGTTOU are installed. Read:
+    //for SIGTTOU are installed. Read: http://curiousthing.org/sigttin-sigttou-deep-dive-linux
-    // http://curiousthing.org/sigttin-sigttou-deep-dive-linux In all cases, our goal here was just
+    //In all cases, our goal here was just to hand over control of the terminal to this process group,
-    // to hand over control of the terminal to this process group, which is a no-op if it's already
+    //which is a no-op if it's already been done.
-    // been done.
     if (tcgetpgrp(STDIN_FILENO) == j->pgid) {
         debug(2, L"Process group %d already has control of terminal\n", j->pgid);
     }
     else {
         debug(4, L"Attempting to bring process group to foreground via tcsetpgrp for job->pgid %d\n", j->pgid);
 
-        // The tcsetpgrp(2) man page says that EPERM is thrown if "pgrp has a supported value, but
+        //the tcsetpgrp(2) man page says that EPERM is thrown if "pgrp has a supported value, but is not the
-        // is not the process group ID of a process in the same session as the calling process."
+        //process group ID of a process in the same session as the calling process."
-        // Since we _guarantee_ that this isn't the case (the child calls setpgid before it calls
+        //Since we _guarantee_ that this isn't the case (the child calls setpgid before it calls SIGSTOP, and
-        // SIGSTOP, and the child was created in the same session as us), it seems that EPERM is
+        //the child was created in the same session as us), it seems that EPERM is being thrown because of an
-        // being thrown because of an caching issue - the call to tcsetpgrp isn't seeing the
+        //caching issue - the call to tcsetpgrp isn't seeing the newly-created process group just yet. On this
-        // newly-created process group just yet. On this developer's test machine (WSL running Linux
+        //developer's test machine (WSL running Linux 4.4.0), EPERM does indeed disappear on retry. The important
-        // 4.4.0), EPERM does indeed disappear on retry. The important thing is that we can
+        //thing is that we can guarantee the process isn't going to exit while we wait (which would cause us to
-        // guarantee the process isn't going to exit while we wait (which would cause us to possibly
+        //possibly block indefinitely).
-        // block indefinitely).
+
         while (tcsetpgrp(STDIN_FILENO, j->pgid) != 0) {
             bool pgroup_terminated = false;
             if (errno == EINTR) {
-                ; // Always retry on EINTR, see comments in tcsetattr EINTR code below.
+                //always retry on EINTR, see comments in tcsetattr EINTR code below.
             }
             else if (errno == EINVAL) {
-                // OS X returns EINVAL if the process group no longer lives. Probably other OSes,
+                //OS X returns EINVAL if the process group no longer lives. Probably other OSes, too.
-                // too. Unlike EPERM below, EINVAL can only happen if the process group has
+                //Unlike EPERM below, EINVAL can only happen if the process group has terminated
-                // terminated.
                 pgroup_terminated = true;
             }
             else if (errno == EPERM) {
-                // Retry so long as this isn't because the process group is dead.
+                //retry so long as this isn't because the process group is dead
                 int wait_result = waitpid(-1 * j->pgid, &wait_result, WNOHANG);
                 if (wait_result == -1) {
-                    // Note that -1 is technically an "error" for waitpid in the sense that an
+                    //Note that -1 is technically an "error" for waitpid in the sense that an invalid argument was specified
-                    // invalid argument was specified because no such process group exists any
+                    //because no such process group exists any longer. This is the observed behavior on Linux 4.4.0.
-                    // longer. This is the observed behavior on Linux 4.4.0. a "success" result
+                    //a "success" result would mean processes from the group still exist but is still running in some state
-                    // would mean processes from the group still exist but is still running in some
+                    //or the other.
-                    // state or the other.
                     pgroup_terminated = true;
                 }
                 else {
-                    // Debug the original tcsetpgrp error (not the waitpid errno) to the log, and
+                    //debug the original tcsetpgrp error (not the waitpid errno) to the log, and then retry until not EPERM
-                    // then retry until not EPERM or the process group has exited.
+                    //or the process group has exited.
                     debug(2, L"terminal_give_to_job(): EPERM.\n", j->pgid);
                 }
             }
@@ -853,10 +850,9 @@ bool terminal_give_to_job(job_t *j, int cont) {
             }
 
             if (pgroup_terminated) {
-                // All processes in the process group has exited. Since we force all child procs to
+                //all processes in the process group has exited. Since we force all child procs to SIGSTOP on startup,
-                // SIGSTOP on startup, the only way that can happen is if the very last process in
+                //the only way that can happen is if the very last process in the group terminated, and didn't need
-                // the group terminated, and didn't need to access the terminal, otherwise it would
+                //to access the terminal, otherwise it would have hung waiting for terminal IO (SIGTTIN). We can ignore this.
-                // have hung waiting for terminal IO (SIGTTIN). We can ignore this.
                 debug(3, L"tcsetpgrp called but process group %d has terminated.\n", j->pgid);
                 break;
             }