fix: assorted correctness issues in getopts builtin (#225)

brush-core/src/builtins/getopts.rs

@@ -1,4 +1,4 @@
-use std::{borrow::Cow, collections::HashMap};
+use std::{borrow::Cow, collections::HashMap, io::Write};
 
 use clap::Parser;
 
@@ -19,11 +19,13 @@ pub(crate) struct GetOptsCommand {
 }
 
 impl builtins::Command for GetOptsCommand {
+    #[allow(clippy::too_many_lines)]
     async fn execute(
         &self,
         context: commands::ExecutionContext<'_>,
     ) -> Result<crate::builtins::ExitCode, crate::error::Error> {
         let mut args = HashMap::<char, bool>::new();
+        let mut treat_unknown_options_as_failure = true;
 
         // Build the args map
         let mut last_char = None;
@@ -32,6 +34,11 @@ impl builtins::Command for GetOptsCommand {
                 if let Some(last_char) = last_char {
                     args.insert(last_char, true);
                     continue;
+                } else if args.is_empty() {
+                    // This is the first character of the options string.
+                    // Its presence indicates a request for unknown
+                    // options *not* to be treated as failures.
+                    treat_unknown_options_as_failure = false;
                 } else {
                     return Ok(builtins::ExitCode::InvalidUsage);
                 }
@@ -53,57 +60,112 @@ impl builtins::Command for GetOptsCommand {
             return Ok(builtins::ExitCode::InvalidUsage);
         }
 
-        let mut next_index_zero_based = next_index - 1;
-        if next_index_zero_based >= self.args.len() {
-            return Ok(builtins::ExitCode::Custom(1));
-        }
-
-        let arg = self.args[next_index_zero_based].as_str();
-        if !arg.starts_with('-') || arg.len() != 2 {
-            return Ok(builtins::ExitCode::Custom(1));
-        }
-
-        // Single character option
-        let c = arg.chars().nth(1).unwrap();
-        if let Some(takes_arg) = args.get(&c) {
-            if *takes_arg {
-                next_index += 1;
-                next_index_zero_based += 1;
+        let new_optarg;
+        let new_optind;
+        let variable_value;
+        let exit_code;
 
-                if next_index_zero_based >= self.args.len() {
-                    return Ok(builtins::ExitCode::Custom(1));
+        // See if there are any args left to parse.
+        let mut next_index_zero_based = next_index - 1;
+        if next_index_zero_based < self.args.len() {
+            let arg = self.args[next_index_zero_based].as_str();
+
+            // See if this is an option.
+            if arg.starts_with('-') && arg.len() == 2 && arg != "--" {
+                // Single character option
+                let c = arg.chars().nth(1).unwrap();
+
+                // Look up the char.
+                if let Some(takes_arg) = args.get(&c) {
+                    variable_value = String::from(c);
+
+                    if *takes_arg {
+                        next_index += 1;
+                        next_index_zero_based += 1;
+
+                        if next_index_zero_based >= self.args.len() {
+                            return Ok(builtins::ExitCode::Custom(1));
+                        }
+
+                        new_optarg = Some(self.args[next_index_zero_based].clone());
+                    } else {
+                        new_optarg = None;
+                    }
+                } else {
+                    // Unknown option; set variable to '?' and OPTARG to the unknown option (sans
+                    // hyphen).
+                    variable_value = String::from("?");
+                    if !treat_unknown_options_as_failure {
+                        new_optarg = Some(String::from(c));
+                    } else {
+                        new_optarg = None;
+                    }
+
+                    if treat_unknown_options_as_failure {
+                        writeln!(context.stderr(), "getopts: illegal option -- {c}")?;
+                    }
                 }
 
-                let opt_arg = self.args[next_index_zero_based].as_str();
-
-                context.shell.env.update_or_add(
-                    "OPTARG",
-                    variables::ShellValueLiteral::Scalar(opt_arg.to_owned()),
-                    |_| Ok(()),
-                    crate::env::EnvironmentLookup::Anywhere,
-                    crate::env::EnvironmentScope::Global,
-                )?;
+                new_optind = next_index + 1;
+                exit_code = builtins::ExitCode::Success;
+            } else {
+                variable_value = String::from("?");
+                new_optarg = None;
+                if arg == "--" {
+                    new_optind = next_index + 1;
+                } else {
+                    new_optind = next_index;
+                }
+                exit_code = builtins::ExitCode::Custom(1);
             }
+        } else {
+            variable_value = String::from("?");
+            new_optarg = None;
+            new_optind = next_index;
+            exit_code = builtins::ExitCode::Custom(1);
+        }
 
+        // Update variable value.
+        context.shell.env.update_or_add(
+            self.variable_name.as_str(),
+            variables::ShellValueLiteral::Scalar(variable_value),
+            |_| Ok(()),
+            crate::env::EnvironmentLookup::Anywhere,
+            crate::env::EnvironmentScope::Global,
+        )?;
+
+        // Update OPTARG
+        if let Some(new_optarg) = new_optarg {
             context.shell.env.update_or_add(
-                "OPTIND",
-                variables::ShellValueLiteral::Scalar((next_index + 1).to_string()),
-                |_| Ok(()),
-                crate::env::EnvironmentLookup::Anywhere,
-                crate::env::EnvironmentScope::Global,
-            )?;
-
-            context.shell.env.update_or_add(
-                self.variable_name.as_str(),
-                variables::ShellValueLiteral::Scalar(c.to_string()),
+                "OPTARG",
+                variables::ShellValueLiteral::Scalar(new_optarg),
                 |_| Ok(()),
                 crate::env::EnvironmentLookup::Anywhere,
                 crate::env::EnvironmentScope::Global,
             )?;
         } else {
-            return Ok(builtins::ExitCode::Custom(1));
+            let _ = context.shell.env.unset("OPTARG")?;
         }
 
-        Ok(builtins::ExitCode::Success)
+        // Update OPTIND
+        context.shell.env.update_or_add(
+            "OPTIND",
+            variables::ShellValueLiteral::Scalar(new_optind.to_string()),
+            |_| Ok(()),
+            crate::env::EnvironmentLookup::Anywhere,
+            crate::env::EnvironmentScope::Global,
+        )?;
+
+        // Initialize OPTERR
+        // TODO: honor OPTERR=0 indicating suppression of error messages
+        context.shell.env.update_or_add(
+            "OPTERR",
+            variables::ShellValueLiteral::Scalar("1".to_string()),
+            |_| Ok(()),
+            crate::env::EnvironmentLookup::Anywhere,
+            crate::env::EnvironmentScope::Global,
+        )?;
+
+        Ok(exit_code)
     }
 }

brush-shell/tests/cases/builtins/getopts.yaml

@@ -11,9 +11,86 @@ cases:
             *) echo "Unknown option: ${option}";;
           esac
           echo "OPTIND is now ${OPTIND}"
+          echo "OPTERR is now ${OPTERR}"
         done
+        echo "Result: $?"
+        echo "option: ${option}"
         echo "End of args"
       }
 
       func -a -b my_b_arg
+      echo "OPTARG: ${OPTARG}"
       echo "OPTIND: ${OPTIND}"
+
+  - name: "getopts: no args given"
+    stdin: |
+      getopts "a:b:" myvar
+      echo "Result: $?"
+      echo "myvar: ${myvar}"
+      echo "OPTARG: ${OPTARG}"
+      echo "OPTIND: ${OPTIND}"
+      echo "OPTERR: ${OPTERR}"
+
+  - name: "getopts: no options given"
+    stdin: |
+      while getopts "a:b:" myvar notoption; do
+        echo "Result: $?"
+        echo "myvar: ${myvar}"
+        echo "OPTARG: ${OPTARG}"
+        echo "OPTIND: ${OPTIND}"
+        echo "OPTERR: ${OPTERR}"
+      done
+      echo "Done; result: $?"
+      echo "myvar: ${myvar}"
+      echo "OPTARG: ${OPTARG}"
+      echo "OPTIND: ${OPTIND}"
+      echo "OPTERR: ${OPTERR}"
+
+  - name: "getopts: options and non-options"
+    stdin: |
+      while getopts "ab:" myvar -a -b value notoption; do
+        echo "Result: $?"
+        echo "myvar: ${myvar}"
+        echo "OPTARG: ${OPTARG}"
+        echo "OPTIND: ${OPTIND}"
+        echo "OPTERR: ${OPTERR}"
+      done
+      echo "Done; result: $?"
+      echo "myvar: ${myvar}"
+      echo "OPTARG: ${OPTARG}"
+      echo "OPTIND: ${OPTIND}"
+      echo "OPTERR: ${OPTERR}"
+
+  - name: "getopts: --"
+    stdin: |
+      while getopts "ab:" myvar -a -b value -- -c notoption; do
+        echo "Result: $?"
+        echo "myvar: ${myvar}"
+        echo "OPTARG: ${OPTARG}"
+        echo "OPTIND: ${OPTIND}"
+        echo "OPTERR: ${OPTERR}"
+      done
+      echo "Done; result: $?"
+      echo "myvar: ${myvar}"
+      echo "OPTARG: ${OPTARG}"
+      echo "OPTIND: ${OPTIND}"
+      echo "OPTERR: ${OPTERR}"
+
+  - name: "getopts: invalid option when optstr starts with colon"
+    stdin: |
+      getopts ":a:" myvar -b value
+      echo "Result: $?"
+      echo "myvar: ${myvar}"
+      echo "OPTARG: ${OPTARG}"
+      echo "OPTIND: ${OPTIND}"
+      echo "OPTERR: ${OPTERR}"
+
+  - name: "getopts: invalid option when optstr does not start with colon"
+    ignore_stderr: true
+    stdin: |
+      getopts "a:" myvar -b value
+      echo "Result: $?"
+      echo "myvar: ${myvar}"
+      echo "OPTARG: ${OPTARG}"
+      echo "OPTIND: ${OPTIND}"
+      echo "OPTERR: ${OPTERR}"