fix: correct issues with ! extglobs and compgen -X (#317)

brush-core/src/builtins/complete.rs

@@ -94,12 +94,19 @@ pub(crate) struct CommonCompleteCommandArgs {
 }
 
 impl CommonCompleteCommandArgs {
-    fn create_spec(&self) -> completion::Spec {
+    fn create_spec(&self, extglob_enabled: bool) -> completion::Spec {
         let filter_pattern_excludes;
         let filter_pattern = if let Some(filter_pattern) = self.filter_pattern.as_ref() {
-            if let Some(filter_pattern) = filter_pattern.strip_prefix('!') {
-                filter_pattern_excludes = false;
-                Some(filter_pattern.to_owned())
+            // If the pattern starts with a '!' that's not the start of an extglob pattern,
+            // then we invert.
+            if let Some(remaining_pattern) = filter_pattern.strip_prefix('!') {
+                if !extglob_enabled || !remaining_pattern.starts_with('(') {
+                    filter_pattern_excludes = false;
+                    Some(remaining_pattern.to_owned())
+                } else {
+                    filter_pattern_excludes = true;
+                    Some(filter_pattern.to_owned())
+                }
             } else {
                 filter_pattern_excludes = true;
                 Some(filter_pattern.clone())
@@ -280,7 +287,10 @@ impl CompleteCommand {
             }
         } else {
             if let Some(target_spec) = target_spec {
-                let mut new_spec = Some(self.common_args.create_spec());
+                let mut new_spec = Some(
+                    self.common_args
+                        .create_spec(context.shell.options.extended_globbing),
+                );
                 std::mem::swap(&mut new_spec, target_spec);
             } else {
                 return error::unimp("set unspecified spec");
@@ -443,7 +453,9 @@ impl CompleteCommand {
             return Ok(true);
         }
 
-        let config = self.common_args.create_spec();
+        let config = self
+            .common_args
+            .create_spec(context.shell.options.extended_globbing);
 
         context.shell.completion_config.set(name, config);
 
@@ -466,7 +478,9 @@ impl builtins::Command for CompGenCommand {
         &self,
         context: commands::ExecutionContext<'_>,
     ) -> Result<crate::builtins::ExitCode, crate::error::Error> {
-        let mut spec = self.common_args.create_spec();
+        let mut spec = self
+            .common_args
+            .create_spec(context.shell.options.extended_globbing);
         spec.options.no_sort = true;
 
         let token_to_complete = self.word.as_deref().unwrap_or_default();

brush-core/src/completion.rs

@@ -291,12 +291,14 @@ impl Spec {
                 let mut updated = IndexSet::new();
 
                 for candidate in candidates {
-                    if !completion_filter_pattern_matches(
+                    let matches = completion_filter_pattern_matches(
                         filter_pattern.as_str(),
                         candidate.as_str(),
                         context.token_to_complete,
                         shell,
-                    )? {
+                    )?;
+
+                    if self.filter_pattern_excludes != matches {
                         updated.insert(candidate);
                     }
                 }
@@ -1184,15 +1186,6 @@ fn completion_filter_pattern_matches(
     token_being_completed: &str,
     shell: &mut Shell,
 ) -> Result<bool, error::Error> {
-    let mut pattern = pattern;
-
-    let invert = if let Some(remaining_pattern) = pattern.strip_prefix('!') {
-        pattern = remaining_pattern;
-        true
-    } else {
-        false
-    };
-
     let pattern = replace_unescaped_ampersands(pattern, token_being_completed);
 
     //
@@ -1205,7 +1198,7 @@ fn completion_filter_pattern_matches(
 
     let matches = pattern.exactly_matches(candidate)?;
 
-    Ok(if invert { !matches } else { matches })
+    Ok(matches)
 }
 
 fn replace_unescaped_ampersands<'a>(pattern: &'a str, replacement: &str) -> Cow<'a, str> {

brush-core/src/patterns.rs

@@ -642,4 +642,174 @@ mod tests {
         );
         Ok(())
     }
+
+    #[test]
+    fn test_matching() -> Result<()> {
+        assert!(Pattern::from("abc").exactly_matches("abc")?);
+
+        assert!(!Pattern::from("abc").exactly_matches("ABC")?);
+        assert!(!Pattern::from("abc").exactly_matches("xabcx")?);
+        assert!(!Pattern::from("abc").exactly_matches("")?);
+        assert!(!Pattern::from("abc").exactly_matches("abcd")?);
+        assert!(!Pattern::from("abc").exactly_matches("def")?);
+
+        assert!(Pattern::from("*").exactly_matches("")?);
+        assert!(Pattern::from("*").exactly_matches("abc")?);
+        assert!(Pattern::from("*").exactly_matches(" ")?);
+
+        assert!(Pattern::from("a*").exactly_matches("a")?);
+        assert!(Pattern::from("a*").exactly_matches("ab")?);
+        assert!(Pattern::from("a*").exactly_matches("a ")?);
+
+        assert!(!Pattern::from("a*").exactly_matches("A")?);
+        assert!(!Pattern::from("a*").exactly_matches("")?);
+        assert!(!Pattern::from("a*").exactly_matches("bc")?);
+        assert!(!Pattern::from("a*").exactly_matches("xax")?);
+        assert!(!Pattern::from("a*").exactly_matches(" a")?);
+
+        assert!(Pattern::from("*a").exactly_matches("a")?);
+        assert!(Pattern::from("*a").exactly_matches("ba")?);
+        assert!(Pattern::from("*a").exactly_matches("aa")?);
+        assert!(Pattern::from("*a").exactly_matches(" a")?);
+
+        assert!(!Pattern::from("*a").exactly_matches("BA")?);
+        assert!(!Pattern::from("*a").exactly_matches("")?);
+        assert!(!Pattern::from("*a").exactly_matches("ab")?);
+        assert!(!Pattern::from("*a").exactly_matches("xax")?);
+
+        Ok(())
+    }
+
+    fn make_extglob(s: &str) -> Pattern {
+        let pattern = Pattern::from(s).set_extended_globbing(true);
+        let regex_str = pattern.to_regex_str(true, true).unwrap();
+        eprintln!("pattern: '{s}' => regex: '{regex_str}'");
+
+        pattern
+    }
+
+    #[test]
+    fn test_extglob_or_matching() -> Result<()> {
+        assert!(make_extglob("@(a|b)").exactly_matches("a")?);
+        assert!(make_extglob("@(a|b)").exactly_matches("b")?);
+
+        assert!(!make_extglob("@(a|b)").exactly_matches("")?);
+        assert!(!make_extglob("@(a|b)").exactly_matches("c")?);
+        assert!(!make_extglob("@(a|b)").exactly_matches("ab")?);
+
+        assert!(!make_extglob("@(a|b)").exactly_matches("")?);
+        assert!(make_extglob("@(a*b|b)").exactly_matches("ab")?);
+        assert!(make_extglob("@(a*b|b)").exactly_matches("axb")?);
+        assert!(make_extglob("@(a*b|b)").exactly_matches("b")?);
+
+        assert!(!make_extglob("@(a*b|b)").exactly_matches("a")?);
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_extglob_not_matching() -> Result<()> {
+        // Basic cases.
+        assert!(make_extglob("!(a)").exactly_matches("")?);
+        assert!(make_extglob("!(a)").exactly_matches(" ")?);
+        assert!(make_extglob("!(a)").exactly_matches("x")?);
+        assert!(make_extglob("!(a)").exactly_matches(" a ")?);
+        assert!(make_extglob("!(a)").exactly_matches("a ")?);
+        assert!(make_extglob("!(a)").exactly_matches("aa")?);
+        assert!(!make_extglob("!(a)").exactly_matches("a")?);
+
+        assert!(make_extglob("a!(a)a").exactly_matches("aa")?);
+        assert!(make_extglob("a!(a)a").exactly_matches("aaaa")?);
+        assert!(make_extglob("a!(a)a").exactly_matches("aba")?);
+        assert!(!make_extglob("a!(a)a").exactly_matches("a")?);
+        assert!(!make_extglob("a!(a)a").exactly_matches("aaa")?);
+        assert!(!make_extglob("a!(a)a").exactly_matches("baaa")?);
+
+        // Alternates.
+        assert!(make_extglob("!(a|b)").exactly_matches("c")?);
+        assert!(make_extglob("!(a|b)").exactly_matches("ab")?);
+        assert!(make_extglob("!(a|b)").exactly_matches("aa")?);
+        assert!(make_extglob("!(a|b)").exactly_matches("bb")?);
+        assert!(!make_extglob("!(a|b)").exactly_matches("a")?);
+        assert!(!make_extglob("!(a|b)").exactly_matches("b")?);
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_extglob_advanced_not_matching() -> Result<()> {
+        assert!(make_extglob("!(a*)").exactly_matches("b")?);
+        assert!(make_extglob("!(a*)").exactly_matches("")?);
+        assert!(!make_extglob("!(a*)").exactly_matches("a")?);
+        assert!(!make_extglob("!(a*)").exactly_matches("abc")?);
+        assert!(!make_extglob("!(a*)").exactly_matches("aabc")?);
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_extglob_not_degenerate_matching() -> Result<()> {
+        // Degenerate case.
+        assert!(make_extglob("!()").exactly_matches("a")?);
+        assert!(!make_extglob("!()").exactly_matches("")?);
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_extglob_zero_or_more_matching() -> Result<()> {
+        assert!(make_extglob("x*(a)x").exactly_matches("xx")?);
+        assert!(make_extglob("x*(a)x").exactly_matches("xax")?);
+        assert!(make_extglob("x*(a)x").exactly_matches("xaax")?);
+
+        assert!(!make_extglob("x*(a)x").exactly_matches("x")?);
+        assert!(!make_extglob("x*(a)x").exactly_matches("xa")?);
+        assert!(!make_extglob("x*(a)x").exactly_matches("xxx")?);
+
+        assert!(make_extglob("*(a|b)").exactly_matches("")?);
+        assert!(make_extglob("*(a|b)").exactly_matches("a")?);
+        assert!(make_extglob("*(a|b)").exactly_matches("b")?);
+        assert!(make_extglob("*(a|b)").exactly_matches("aba")?);
+        assert!(make_extglob("*(a|b)").exactly_matches("aaa")?);
+
+        assert!(!make_extglob("*(a|b)").exactly_matches("c")?);
+        assert!(!make_extglob("*(a|b)").exactly_matches("ca")?);
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_extglob_one_or_more_matching() -> Result<()> {
+        fn make_extglob(s: &str) -> Pattern {
+            Pattern::from(s).set_extended_globbing(true)
+        }
+
+        assert!(make_extglob("x+(a)x").exactly_matches("xax")?);
+        assert!(make_extglob("x+(a)x").exactly_matches("xaax")?);
+
+        assert!(!make_extglob("x+(a)x").exactly_matches("xx")?);
+        assert!(!make_extglob("x+(a)x").exactly_matches("x")?);
+        assert!(!make_extglob("x+(a)x").exactly_matches("xa")?);
+        assert!(!make_extglob("x+(a)x").exactly_matches("xxx")?);
+
+        assert!(make_extglob("+(a|b)").exactly_matches("a")?);
+        assert!(make_extglob("+(a|b)").exactly_matches("b")?);
+        assert!(make_extglob("+(a|b)").exactly_matches("aba")?);
+        assert!(make_extglob("+(a|b)").exactly_matches("aaa")?);
+
+        assert!(!make_extglob("+(a|b)").exactly_matches("")?);
+        assert!(!make_extglob("+(a|b)").exactly_matches("c")?);
+        assert!(!make_extglob("+(a|b)").exactly_matches("ca")?);
+
+        assert!(make_extglob("+(x+(ab)y)").exactly_matches("xaby")?);
+        assert!(make_extglob("+(x+(ab)y)").exactly_matches("xababy")?);
+        assert!(make_extglob("+(x+(ab)y)").exactly_matches("xabababy")?);
+        assert!(make_extglob("+(x+(ab)y)").exactly_matches("xabababyxabababyxabababy")?);
+
+        assert!(!make_extglob("+(x+(ab)y)").exactly_matches("xy")?);
+        assert!(!make_extglob("+(x+(ab)y)").exactly_matches("xay")?);
+        assert!(!make_extglob("+(x+(ab)y)").exactly_matches("xyxy")?);
+
+        Ok(())
+    }
 }

brush-parser/src/pattern.rs

@@ -92,25 +92,28 @@ peg::parser! {
             kind:extended_glob_prefix() "(" branches:extended_glob_body() ")" {
                 let mut s = String::new();
 
-                s.push('(');
-
                 // fancy_regex uses ?! to indicate a negative lookahead.
                 if matches!(kind, ExtendedGlobKind::Exclamation) {
-                    s.push_str("(?!");
-                }
-
-                s.push_str(&branches.join("|"));
-                s.push(')');
-
-                match kind {
-                    ExtendedGlobKind::Plus => s.push('+'),
-                    ExtendedGlobKind::Question => s.push('?'),
-                    ExtendedGlobKind::Star => s.push('*'),
-                    ExtendedGlobKind::At | ExtendedGlobKind::Exclamation => (),
-                }
-
-                if matches!(kind, ExtendedGlobKind::Exclamation) {
-                    s.push_str(".)*?");
+                    if !branches.is_empty() {
+                        s.push_str("(?:(?!");
+                        s.push_str(&branches.join("|"));
+                        s.push_str(").*|(?>");
+                        s.push_str(&branches.join("|"));
+                        s.push_str(").+?|)");
+                    } else {
+                        s.push_str("(?:.+)")
+                    }
+                } else {
+                    s.push('(');
+                    s.push_str(&branches.join("|"));
+                    s.push(')');
+
+                    match kind {
+                        ExtendedGlobKind::Plus => s.push('+'),
+                        ExtendedGlobKind::Question => s.push('?'),
+                        ExtendedGlobKind::Star => s.push('*'),
+                        ExtendedGlobKind::At | ExtendedGlobKind::Exclamation => (),
+                    }
                 }
 
                 s
@@ -124,14 +127,12 @@ peg::parser! {
             "*" { ExtendedGlobKind::Star }
 
         pub(crate) rule extended_glob_body() -> Vec<String> =
-            first_branches:((b:extended_glob_branch() "|" { b })*) last_branch:extended_glob_branch() {
-                let mut branches = first_branches;
-                branches.push(last_branch);
-                branches
-            }
+            extended_glob_branch() ** "|"
 
         rule extended_glob_branch() -> String =
-            pieces:(!['|' | ')'] piece:pattern_piece() { piece })* { pieces.join("") }
+            pieces:(!['|' | ')'] piece:pattern_piece() { piece })+ {
+                pieces.join("")
+            }
     }
 }
 
@@ -185,6 +186,11 @@ mod tests {
             "(ab|ac)*"
         );
 
+        assert_eq!(
+            pattern_to_regex_translator::extended_glob_body("", true)?,
+            Vec::<String>::new(),
+        );
+
         Ok(())
     }
 }

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

@@ -137,6 +137,32 @@ cases:
       echo "[Take 1]"
       compgen -W 'somebody something' -X '&b*' some
 
+  - name: "compgen -X with extglob"
+    stdin: |
+      touch README
+      shopt -s extglob
+
+      echo "[Take 1]"
+      compgen -f READ
+
+      echo "[Take 2]"
+      compgen -f -X "READ" READ
+
+      echo "[Take 3]"
+      compgen -f -X "README" READ
+
+      echo "[Take 4]"
+      compgen -f -X "!(READ)" READ
+
+      echo "[Take 5]"
+      compgen -f -X "!(README)" READ
+
+      echo "[Take 6]"
+      compgen -f -X "!!(READ)" READ
+
+      echo "[Take 7]"
+      compgen -f -X "!!(README)" READ
+
   - name: "compgen -o dirnames"
     stdin: |
       echo "[Take 1]"