From ecb53fb66d95eefa0bf006e62a736ee73bf630c5 Mon Sep 17 00:00:00 2001 From: Andrea Cosentino Date: Wed, 28 Oct 2026 13:33:22 +0100 Subject: [PATCH 1/2] Fix PatternOptionBuilder '!' marking the wrong option as required The Javadoc says '!' goes in front of a mandatory option, and its own example, "vp:!f/", is meant to make -f required. The flag was only applied once the next option character was read, so it ended up on the option before the '!': the example made -p required and left -f optional. It only worked when '!' was the first character of the pattern, which is the one case the tests covered. Now '!' applies to the option that follows it. A '!' with no option after it, as in "a:b!", used to land on the previous option; it now throws IllegalArgumentException, so a pattern written against the old behavior fails right away instead of quietly losing its required option. Co-Authored-By: Claude Opus 5.5 --- src/changes/changes.xml | 1 + .../commons/cli/PatternOptionBuilder.java | 17 +++++++--- .../commons/cli/PatternOptionBuilderTest.java | 33 +++++++++++++++++++ 3 files changed, 46 insertions(+), 5 deletions(-) diff --git a/src/changes/changes.xml b/src/changes/changes.xml index d3f7b92d2..b7b549bae 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -37,6 +37,7 @@ Reject UTF-16 surrogate values in Character converter (#438). Fix HelpFormatter wrapped description indent (#439). Load classes without initializing them in Converter.CLASS (#432). + Fix PatternOptionBuilder '!' marking the option before it as required instead of the option after it, as documented; a '!' that isn't followed by an option now throws IllegalArgumentException. Add AbstractHelpFormatter.printHelp(String, Options). diff --git a/src/main/java/org/apache/commons/cli/PatternOptionBuilder.java b/src/main/java/org/apache/commons/cli/PatternOptionBuilder.java index f68cfaeac..cdd4bcc4e 100644 --- a/src/main/java/org/apache/commons/cli/PatternOptionBuilder.java +++ b/src/main/java/org/apache/commons/cli/PatternOptionBuilder.java @@ -60,8 +60,8 @@ Licensed to the Apache Software Foundation (ASF) under one or more * * *

- * For example, the following allows command line flags of '-v -p string-value -f /dir/file'. The exclamation mark - * precede a mandatory option. + * For example, the following allows command line flags of '-v -p string-value -f /dir/file'. An exclamation mark + * before an option character marks that option as required, here {@code -f}. *

* * 
@@ -169,10 +169,12 @@ public static boolean isValueCode(final char ch) {
      *
      * @param pattern The pattern string
      * @return The {@link Options} instance
+     * @throws IllegalArgumentException if an exclamation mark is not followed by an option character.
      */
     public static Options parsePattern(final String pattern) {
         char opt = Char.SP;
         boolean required = false;
+        boolean requiredNext = false;
         Class type = null;
         Converter converter = Converter.DEFAULT;
 
@@ -182,7 +184,7 @@ public static Options parsePattern(final String pattern) {
             final char ch = pattern.charAt(i);
 
             // a value code comes after an option and specifies
-            // details about it
+            // details about it, except '!' which precedes the option it marks as required
             if (!isValueCode(ch)) {
                 if (opt != Char.SP) {
                     // @formatter:off
@@ -195,14 +197,15 @@ public static Options parsePattern(final String pattern) {
                     // @formatter:on
                     // we have a previous one to deal with
                     options.addOption(option);
-                    required = false;
                     type = null;
                     converter = Converter.DEFAULT;
                 }
 
                 opt = ch;
+                required = requiredNext;
+                requiredNext = false;
             } else if (ch == '!') {
-                required = true;
+                requiredNext = true;
             } else {
                 type = getValueType(ch);
                 final Map, Converter> map = TypeHandler.createDefaultMap();
@@ -212,6 +215,10 @@ public static Options parsePattern(final String pattern) {
             }
         }
 
+        if (requiredNext) {
+            throw new IllegalArgumentException(String.format("Missing option character after '!' in pattern '%s'.", pattern));
+        }
+
         if (opt != Char.SP) {
             final Option option = Option.builder(String.valueOf(opt)).hasArg(type != null).required(required).type(type).get();
 
diff --git a/src/test/java/org/apache/commons/cli/PatternOptionBuilderTest.java b/src/test/java/org/apache/commons/cli/PatternOptionBuilderTest.java
index dd1013eae..d00ab5b8f 100644
--- a/src/test/java/org/apache/commons/cli/PatternOptionBuilderTest.java
+++ b/src/test/java/org/apache/commons/cli/PatternOptionBuilderTest.java
@@ -99,6 +99,27 @@ void testObjectPattern() throws Exception {
         assertNull(line.getOptionObject("n"), "n value");
     }
 
+    @Test
+    void testRequiredMarkPrecedesOption() throws Exception {
+        // The class Javadoc example: '!' marks the following option, -f, as required.
+        final Options options = PatternOptionBuilder.parsePattern("vp:!f/");
+        assertFalse(options.getOption("v").isRequired(), "v");
+        assertFalse(options.getOption("p").isRequired(), "p");
+        assertTrue(options.getOption("f").isRequired(), "f");
+        final MissingOptionException e = assertThrows(MissingOptionException.class,
+                () -> new DefaultParser().parse(options, new String[] { "-v", "-p", "value" }));
+        assertEquals(1, e.getMissingOptions().size());
+        assertTrue(e.getMissingOptions().contains("f"));
+    }
+
+    @Test
+    void testRequiredMarkWithoutOption() {
+        assertThrows(IllegalArgumentException.class, () -> PatternOptionBuilder.parsePattern("!"));
+        assertThrows(IllegalArgumentException.class, () -> PatternOptionBuilder.parsePattern("a!"));
+        assertThrows(IllegalArgumentException.class, () -> PatternOptionBuilder.parsePattern("a:b!"));
+        assertThrows(IllegalArgumentException.class, () -> PatternOptionBuilder.parsePattern("a!:"));
+    }
+
     @Test
     void testRequiredOption() throws Exception {
         final Options options = PatternOptionBuilder.parsePattern("!n%m%");
@@ -108,6 +129,18 @@ void testRequiredOption() throws Exception {
         assertTrue(e.getMissingOptions().contains("n"));
     }
 
+    @Test
+    void testRequiredOptions() throws Exception {
+        Options options = PatternOptionBuilder.parsePattern("a:!b:c:");
+        assertFalse(options.getOption("a").isRequired(), "a");
+        assertTrue(options.getOption("b").isRequired(), "b");
+        assertFalse(options.getOption("c").isRequired(), "c");
+        options = PatternOptionBuilder.parsePattern("!a!b:c");
+        assertTrue(options.getOption("a").isRequired(), "a");
+        assertTrue(options.getOption("b").isRequired(), "b");
+        assertFalse(options.getOption("c").isRequired(), "c");
+    }
+
     @Test
     void testSimplePattern() throws Exception {
         /*

From fb89418e299ac298fa5e9f722a760d1cd3e99ef8 Mon Sep 17 00:00:00 2001
From: Andrea Cosentino 
Date: Thu, 29 Oct 2026 13:33:56 +0100
Subject: [PATCH 2/2] Keep the '!' behavior in PatternOptionBuilder and test it

The earlier commit on this branch made '!' mark the option after it,
following the 2008 Javadoc sentence "The exclamation mark precede a
mandatory option". CLI-55 has the intent the other way round: "hc!<"
makes -c mandatory, so '!' marks the option before it, which is what
the code has always done and what the Javadoc now says.

Back out that change so existing patterns keep working, and add tests
for the current behavior instead: the Javadoc example, the CLI-55
example, a trailing '!' and several '!' in one pattern. Also say that a
'!' at the start of the pattern marks the first option.

Co-Authored-By: Claude Opus 5.5 
Signed-off-by: Andrea Cosentino 
---
 src/changes/changes.xml                       |  1 -
 .../commons/cli/PatternOptionBuilder.java     | 14 ++-----
 .../commons/cli/PatternOptionBuilderTest.java | 38 ++++++++++---------
 3 files changed, 25 insertions(+), 28 deletions(-)

diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index 0d4751445..844f21b79 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -38,7 +38,6 @@
       Fix HelpFormatter wrapped description indent (#439).
       Fix DefaultParser keeping values from its Properties argument across parses that reuse the same Options.
       Load classes without initializing them in Converter.CLASS (#432).
-      Fix PatternOptionBuilder '!' marking the option before it as required instead of the option after it, as documented; a '!' that isn't followed by an option now throws IllegalArgumentException.
       
       Add AbstractHelpFormatter.printHelp(String, Options).
       
diff --git a/src/main/java/org/apache/commons/cli/PatternOptionBuilder.java b/src/main/java/org/apache/commons/cli/PatternOptionBuilder.java
index ae3cb228b..ac53dd972 100644
--- a/src/main/java/org/apache/commons/cli/PatternOptionBuilder.java
+++ b/src/main/java/org/apache/commons/cli/PatternOptionBuilder.java
@@ -70,7 +70,7 @@ Licensed to the Apache Software Foundation (ASF) under one or more
  *
  * 

  * For example, the following accepts command-line arguments such as {@code -v -p string-value -f https://example.com}. The exclamation mark makes the
- * preceding option mandatory.
+ * preceding option mandatory, here {@code -p}. An exclamation mark at the start of the pattern makes the first option mandatory.
  * 

  *
  * 
@@ -207,7 +207,6 @@ public static boolean isValueCode(final char ch) {
     public static Options parsePattern(final String pattern) {
         char opt = Char.SP;
         boolean required = false;
-        boolean requiredNext = false;
         Class type = null;
         Converter converter = Converter.DEFAULT;
 
@@ -217,7 +216,7 @@ public static Options parsePattern(final String pattern) {
             final char ch = pattern.charAt(i);
 
             // a value code comes after an option and specifies
-            // details about it, except '!' which precedes the option it marks as required
+            // details about it
             if (!isValueCode(ch)) {
                 if (opt != Char.SP) {
                     // @formatter:off
@@ -230,15 +229,14 @@ public static Options parsePattern(final String pattern) {
                     // @formatter:on
                     // we have a previous one to deal with
                     options.addOption(option);
+                    required = false;
                     type = null;
                     converter = Converter.DEFAULT;
                 }
 
                 opt = ch;
-                required = requiredNext;
-                requiredNext = false;
             } else if (ch == '!') {
-                requiredNext = true;
+                required = true;
             } else {
                 type = getValueType(ch);
                 final Map, Converter> map = TypeHandler.createDefaultMap();
@@ -248,10 +246,6 @@ public static Options parsePattern(final String pattern) {
             }
         }
 
-        if (requiredNext) {
-            throw new IllegalArgumentException(String.format("Missing option character after '!' in pattern '%s'.", pattern));
-        }
-
         if (opt != Char.SP) {
             final Option option = Option.builder(String.valueOf(opt)).hasArg(type != null).required(required).type(type).get();
 
diff --git a/src/test/java/org/apache/commons/cli/PatternOptionBuilderTest.java b/src/test/java/org/apache/commons/cli/PatternOptionBuilderTest.java
index d00ab5b8f..874c5d68a 100644
--- a/src/test/java/org/apache/commons/cli/PatternOptionBuilderTest.java
+++ b/src/test/java/org/apache/commons/cli/PatternOptionBuilderTest.java
@@ -100,24 +100,32 @@ void testObjectPattern() throws Exception {
     }
 
     @Test
-    void testRequiredMarkPrecedesOption() throws Exception {
-        // The class Javadoc example: '!' marks the following option, -f, as required.
+    void testRequiredMarkAfterOption() throws Exception {
+        // The class Javadoc example: '!' makes the preceding option, -p, mandatory.
         final Options options = PatternOptionBuilder.parsePattern("vp:!f/");
         assertFalse(options.getOption("v").isRequired(), "v");
-        assertFalse(options.getOption("p").isRequired(), "p");
-        assertTrue(options.getOption("f").isRequired(), "f");
+        assertTrue(options.getOption("p").isRequired(), "p");
+        assertFalse(options.getOption("f").isRequired(), "f");
         final MissingOptionException e = assertThrows(MissingOptionException.class,
-                () -> new DefaultParser().parse(options, new String[] { "-v", "-p", "value" }));
+                () -> new DefaultParser().parse(options, new String[] { "-v", "-f", "https://example.com" }));
         assertEquals(1, e.getMissingOptions().size());
-        assertTrue(e.getMissingOptions().contains("f"));
+        assertTrue(e.getMissingOptions().contains("p"));
     }
 
     @Test
-    void testRequiredMarkWithoutOption() {
-        assertThrows(IllegalArgumentException.class, () -> PatternOptionBuilder.parsePattern("!"));
-        assertThrows(IllegalArgumentException.class, () -> PatternOptionBuilder.parsePattern("a!"));
-        assertThrows(IllegalArgumentException.class, () -> PatternOptionBuilder.parsePattern("a:b!"));
-        assertThrows(IllegalArgumentException.class, () -> PatternOptionBuilder.parsePattern("a!:"));
+    void testRequiredMarkAtEnd() {
+        final Options options = PatternOptionBuilder.parsePattern("a:b!");
+        assertFalse(options.getOption("a").isRequired(), "a");
+        assertTrue(options.getOption("b").isRequired(), "b");
+    }
+
+    @Test
+    void testRequiredMarkBeforeValueCode() {
+        // The example from CLI-55: "hc!<" makes -c mandatory.
+        final Options options = PatternOptionBuilder.parsePattern("hc!<");
+        assertFalse(options.getOption("h").isRequired(), "h");
+        assertTrue(options.getOption("c").isRequired(), "c");
+        assertEquals(PatternOptionBuilder.EXISTING_FILE_VALUE, options.getOption("c").getType());
     }
 
     @Test
@@ -130,12 +138,8 @@ void testRequiredOption() throws Exception {
     }
 
     @Test
-    void testRequiredOptions() throws Exception {
-        Options options = PatternOptionBuilder.parsePattern("a:!b:c:");
-        assertFalse(options.getOption("a").isRequired(), "a");
-        assertTrue(options.getOption("b").isRequired(), "b");
-        assertFalse(options.getOption("c").isRequired(), "c");
-        options = PatternOptionBuilder.parsePattern("!a!b:c");
+    void testRequiredOptions() {
+        final Options options = PatternOptionBuilder.parsePattern("a!b:!c");
         assertTrue(options.getOption("a").isRequired(), "a");
         assertTrue(options.getOption("b").isRequired(), "b");
         assertFalse(options.getOption("c").isRequired(), "c");