Update command line parser argument style detection, add tests. (#1785)

src/main/java/picard/cmdline/CommandLineProgram.java

@@ -86,6 +86,8 @@ public abstract class CommandLineProgram {
     private static String PROPERTY_USE_LEGACY_PARSER = "picard.useLegacyParser";
     private static String PROPERTY_CONVERT_LEGACY_COMMAND_LINE = "picard.convertCommandLine";
     private static Boolean useLegacyParser;
+    public static String SYNTAX_TRANSITION_URL =
+            "https://github.com/broadinstitute/picard/wiki/Command-Line-Syntax-Transition-For-Users-(Pre-Transition)";
 
     /**
      * CommandLineProgramProperties oneLineSummary attribute must be shorted than this in order to maintain
@@ -205,7 +207,7 @@ public int instanceMain(final String[] argv) {
                 "********** NOTE: Picard's command line syntax is changing.",
                 "**********",
                 "********** For more information, please see:",
-                "********** https://github.com/broadinstitute/picard/wiki/Command-Line-Syntax-Transition-For-Users-(Pre-Transition)",
+                "********** ", SYNTAX_TRANSITION_URL,
                 "**********",
                 "********** The command line looks like this in the new syntax:",
                 "**********",

src/main/java/picard/cmdline/CommandLineSyntaxTranslater.java

@@ -1,5 +1,7 @@
 package picard.cmdline;
 
+import htsjdk.samtools.util.Log;
+
 import java.util.Arrays;
 import java.util.List;
 import java.util.stream.Collectors;
@@ -9,6 +11,7 @@
  * used for running tests written with Picard style syntax against the Barclay command line parser.
  */
 public class CommandLineSyntaxTranslater {
+    private final static Log log = Log.getInstance(CommandLineSyntaxTranslater.class);
 
     // Prefixes used by the Barclay parser for short/long prefixes
     private static final String BARCLAY_SHORT_OPTION_PREFIX = "-";
@@ -18,12 +21,23 @@ public class CommandLineSyntaxTranslater {
 
     // Return true when the command line arguments appear to use Picard's legacy syntax.
     public static boolean isLegacyPicardStyle(final String argv[]) {
-        return Arrays.stream(argv).anyMatch(
-                putativeLegacyArg ->
-                        !putativeLegacyArg.startsWith(BARCLAY_SHORT_OPTION_PREFIX) &&
-                           !putativeLegacyArg.startsWith(BARCLAY_LONG_OPTION_PREFIX) &&
-                                putativeLegacyArg.contains(LEGACY_VALUE_SEPARATOR)
+        final boolean anyLegacy = Arrays.stream(argv).anyMatch(
+                arg -> !arg.startsWith(BARCLAY_SHORT_OPTION_PREFIX) &&
+                        !arg.startsWith(BARCLAY_LONG_OPTION_PREFIX) &&
+                        arg.contains(LEGACY_VALUE_SEPARATOR)
         );
+        if (anyLegacy && Arrays.stream(argv).anyMatch(
+                arg -> arg.startsWith(BARCLAY_SHORT_OPTION_PREFIX) || arg.startsWith(BARCLAY_LONG_OPTION_PREFIX))) {
+            // There appear to be both legacy and posix style args. Prefer/choose posix in this case since there are
+            // legitimate cases where argument values might contain embedded "=" (i.e,
+            // "--INPUT path/to/some.bam --SOME_ARG date=01/01/2022"), which makes them appear to be
+            // legacy style args, even though they are not), whereas its very unlikely to encounter a legitimate
+            // legacy option that starts with a posix prefix ("--" or "-")
+            log.warn("!!!!!!Possible mixed (legacy and new style) arguments detected!!!!!!!\n"
+                    + "Assuming new-style arguments are intended. See: " + CommandLineProgram.SYNTAX_TRANSITION_URL);
+            return false;
+        }
+        return anyLegacy;
     }
 
     public static String[] convertPicardStyleToPosixStyle(final String argv[]) {

src/test/java/picard/cmdline/PicardCommandLineTest.java

@@ -5,6 +5,7 @@
 import org.broadinstitute.barclay.argparser.CommandLineProgramProperties;
 import org.testng.Assert;
 import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 import java.util.*;
 
@@ -81,4 +82,38 @@ public void testCommandlineProgramPropertiesOneLineSummaryLength() {
             });
     }
 
+    @DataProvider(name="isLegacyPicardStyleTests")
+    public final Object[][] getIsLegacyPicardStyle() {
+        return new Object[][] {
+                //arg list, is legacy style
+
+                // legacy base cases
+                {Arrays.asList("--INPUT", "path/to/some.bam"), false},
+                {Arrays.asList("--INPUT", "path/to/some.bam", "--VALIDATION_STRINGENCY", "LENIENT"), false},
+
+                // posix base cases
+                {Arrays.asList("INPUT=path/to/some.bam"), true },
+                {Arrays.asList("INPUT=path/to/some.bam", "VALIDATION_STRINGENCY=LENIENT"), true},
+
+                // mixed syntax cases
+
+                // APPEARS to isLegacyPicardStyle to contain a mix of styles, but is actually (in theory) a legitimate
+                // posix style arg list, so select the posix parser, but issue a warning about possible mixed
+                // args set
+                {Arrays.asList("--INPUT", "path/to/some.bam", "--SOME_ARG", "date=01/01/2022"), false},
+
+                // appears to isLegacyPicardStyle to contain a mix of styles, but is probably not valid, so select the
+                // posix parser, issue a warning, and let the parser decide if its legitimate
+                {Arrays.asList("--INPUT", "path/to/some.bam", "VALIDATION_STRINGENCY=LENIENT"), false},
+
+                // appears to isLegacyPicardStyle to contain a mix of styles, but is probably not valid, so select the
+                // posix parser, issue a warning, and let the parser decide if its legitimate
+                {Arrays.asList("INPUT=path/to/some.bam", "--ARG=somevalue"), false},
+        };
+    }
+
+    @Test(dataProvider="isLegacyPicardStyleTests")
+    public void testIsLegacyPicardStyle(final List<String> args, final boolean isLegacy) {
+        Assert.assertEquals(CommandLineSyntaxTranslater.isLegacyPicardStyle(args.toArray(new String[0])), isLegacy);
+    }
 }