[MNG-6078] Perform a proper merge of the two sources of command line arguments

- Needed to extend Commons CLI's CommandLine just to perform the merged

maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java

@@ -383,7 +383,7 @@ void cli( CliRequest cliRequest )
         CLIManager cliManager = new CLIManager();
 
         List<String> args = new ArrayList<>();
-
+        CommandLine mavenConfig = null;
         try
         {
             File configFile = new File( cliRequest.multiModuleProjectDirectory, MVN_MAVEN_CONFIG );
@@ -398,8 +398,8 @@ void cli( CliRequest cliRequest )
                     }
                 }
 
-                CommandLine config = cliManager.parse( args.toArray( new String[args.size()] ) );
+                mavenConfig = cliManager.parse( args.toArray( new String[args.size()] ) );
-                List<?> unrecongized = config.getArgList();
+                List<?> unrecongized = mavenConfig.getArgList();
                 if ( !unrecongized.isEmpty() )
                 {
                     throw new ParseException( "Unrecognized maven.config entries: " + unrecongized );
@@ -415,21 +415,14 @@ void cli( CliRequest cliRequest )
 
         try
         {
-            int index = 0;
+            if ( mavenConfig == null )
-            for ( String arg : cliRequest.args )
             {
-                if ( arg.startsWith( "-D" ) )
+                cliRequest.commandLine = cliManager.parse( cliRequest.args );
-                {
+            }
-                    // a property definition so needs to come last so that the last property wins
+            else
-                    args.add( arg );
+            {
-                }
+                cliRequest.commandLine = new MergedCommandLine( cliManager.parse( cliRequest.args ), mavenConfig );
-                else
-                {
-                    // not a property definition so needs to come first to override maven.config
-                    args.add( index++, arg );
-                }
             }
-            cliRequest.commandLine = cliManager.parse( args.toArray( new String[args.size()] ) );
         }
         catch ( ParseException e )
         {

maven-embedder/src/main/java/org/apache/maven/cli/MergedCommandLine.java

@@ -0,0 +1,75 @@
+package org.apache.maven.cli;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import org.apache.commons.cli.CommandLine;
+import org.apache.commons.cli.Option;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * A {@link CommandLine} instance that represents a merged command line combining CLI arguments with those from the
+ * {@code .mvn/maven.config} while reflecting the handling of {@link CLIManager#SET_SYSTEM_PROPERTY} versus all the
+ * other command line options (last wins vs first wins respectively).
+ */
+class MergedCommandLine
+    extends CommandLine
+{
+    MergedCommandLine( CommandLine commandLine, CommandLine configFile )
+    {
+        // such a pity that Commons CLI does not offer either a builder or a formatter and we need to extend
+        // to perform the merge. A formatter would mean we could unparse and reparse (not ideal but would work).
+        // A builder would be ideal for this kind of merge like processing.
+        super();
+        // the args are easy, cli first then config file
+        for ( String arg : commandLine.getArgs() )
+        {
+            addArg( arg );
+        }
+        for ( String arg : configFile.getArgs() )
+        {
+            addArg( arg );
+        }
+        // now add all options, except for -D with cli first then config file
+        List<Option> setPropertyOptions = new ArrayList<>();
+        for ( Option opt : commandLine.getOptions() )
+        {
+            if ( String.valueOf( CLIManager.SET_SYSTEM_PROPERTY ).equals( opt.getOpt() ) )
+            {
+                setPropertyOptions.add( opt );
+            }
+            else
+            {
+                addOption( opt );
+            }
+        }
+        for ( Option opt : configFile.getOptions() )
+        {
+            addOption( opt );
+        }
+        // finally add the CLI system properties
+        for ( Option opt : setPropertyOptions )
+        {
+            addOption( opt );
+        }
+    }
+
+}

maven-embedder/src/test/java/org/apache/maven/cli/MavenCliTest.java

@@ -19,11 +19,10 @@
  * under the License.
  */
 
-import java.io.File;
+import junit.framework.TestCase;
-
 import org.apache.commons.cli.ParseException;
 
-import junit.framework.TestCase;
+import java.io.File;
 
 public class MavenCliTest
     extends TestCase
@@ -75,7 +74,8 @@ public void testCalculateDegreeOfConcurrencyWithCoreMultiplier()
     public void testMavenConfig()
         throws Exception
     {
-        System.setProperty( MavenCli.MULTIMODULE_PROJECT_DIRECTORY, new File( "src/test/projects/config" ).getCanonicalPath() );
+        System.setProperty( MavenCli.MULTIMODULE_PROJECT_DIRECTORY,
+                            new File( "src/test/projects/config" ).getCanonicalPath() );
         CliRequest request = new CliRequest( new String[0], null );
 
         // read .mvn/maven.config
@@ -85,7 +85,7 @@ public void testMavenConfig()
         assertEquals( "8", request.commandLine.getOptionValue( CLIManager.THREADS ) );
 
         // override from command line
-        request = new CliRequest( new String[] { "--builder", "foobar" }, null );
+        request = new CliRequest( new String[]{ "--builder", "foobar" }, null );
         cli.cli( request );
         assertEquals( "foobar", request.commandLine.getOptionValue( "builder" ) );
     }
@@ -93,7 +93,8 @@ public void testMavenConfig()
     public void testMavenConfigInvalid()
         throws Exception
     {
-        System.setProperty( MavenCli.MULTIMODULE_PROJECT_DIRECTORY, new File( "src/test/projects/config-illegal" ).getCanonicalPath() );
+        System.setProperty( MavenCli.MULTIMODULE_PROJECT_DIRECTORY,
+                            new File( "src/test/projects/config-illegal" ).getCanonicalPath() );
         CliRequest request = new CliRequest( new String[0], null );
 
         cli.initialize( request );
@@ -107,7 +108,7 @@ public void testMavenConfigInvalid()
 
         }
     }
-    
+
     /**
      * Read .mvn/maven.config with the following definitions:
      * <pre>
@@ -116,19 +117,23 @@ public void testMavenConfigInvalid()
      * </pre>
      * and check if the {@code -T 3} option can be overwritten via command line
      * argument.
+     *
      * @throws Exception in case of failure.
      */
-    public void testMVNConfigurationThreadCanBeOverwrittenViaCommandLine() throws Exception {
+    public void testMVNConfigurationThreadCanBeOverwrittenViaCommandLine()
-        System.setProperty( MavenCli.MULTIMODULE_PROJECT_DIRECTORY, new File( "src/test/projects/mavenConfigProperties" ).getCanonicalPath() );
+        throws Exception
+    {
+        System.setProperty( MavenCli.MULTIMODULE_PROJECT_DIRECTORY,
+                            new File( "src/test/projects/mavenConfigProperties" ).getCanonicalPath() );
         CliRequest request = new CliRequest( new String[]{ "-T", "5" }, null );
 
         cli.initialize( request );
         // read .mvn/maven.config
         cli.cli( request );
-        
+
         assertEquals( "5", request.commandLine.getOptionValue( CLIManager.THREADS ) );
     }
-    
+
     /**
      * Read .mvn/maven.config with the following definitions:
      * <pre>
@@ -137,17 +142,21 @@ public void testMVNConfigurationThreadCanBeOverwrittenViaCommandLine() throws Ex
      * </pre>
      * and check if the {@code -Drevision-1.3.0} option can be overwritten via command line
      * argument.
+     *
      * @throws Exception
      */
-    public void testMVNConfigurationDefinedPropertiesCanBeOverwrittenViaCommandLine() throws Exception {
+    public void testMVNConfigurationDefinedPropertiesCanBeOverwrittenViaCommandLine()
-        System.setProperty( MavenCli.MULTIMODULE_PROJECT_DIRECTORY, new File( "src/test/projects/mavenConfigProperties" ).getCanonicalPath() );
+        throws Exception
-        CliRequest request = new CliRequest( new String[]{ "-Drevision=8.1.0"}, null );
+    {
+        System.setProperty( MavenCli.MULTIMODULE_PROJECT_DIRECTORY,
+                            new File( "src/test/projects/mavenConfigProperties" ).getCanonicalPath() );
+        CliRequest request = new CliRequest( new String[]{ "-Drevision=8.1.0" }, null );
 
         cli.initialize( request );
         // read .mvn/maven.config
         cli.cli( request );
         cli.properties( request );
-        
+
         String revision = System.getProperty( "revision" );
         assertEquals( "8.1.0", revision );
     }
@@ -160,11 +169,15 @@ public void testMVNConfigurationDefinedPropertiesCanBeOverwrittenViaCommandLine(
      * </pre>
      * and check if the {@code -Drevision-1.3.0} option can be overwritten via command line
      * argument.
+     *
      * @throws Exception
      */
-    public void testMVNConfigurationCLIRepeatedPropertiesLastWins() throws Exception {
+    public void testMVNConfigurationCLIRepeatedPropertiesLastWins()
-        System.setProperty( MavenCli.MULTIMODULE_PROJECT_DIRECTORY, new File( "src/test/projects/mavenConfigProperties" ).getCanonicalPath() );
+        throws Exception
-        CliRequest request = new CliRequest( new String[]{ "-Drevision=8.1.0", "-Drevision=8.2.0"}, null );
+    {
+        System.setProperty( MavenCli.MULTIMODULE_PROJECT_DIRECTORY,
+                            new File( "src/test/projects/mavenConfigProperties" ).getCanonicalPath() );
+        CliRequest request = new CliRequest( new String[]{ "-Drevision=8.1.0", "-Drevision=8.2.0" }, null );
 
         cli.initialize( request );
         // read .mvn/maven.config
@@ -174,4 +187,38 @@ public void testMVNConfigurationCLIRepeatedPropertiesLastWins() throws Exception
         String revision = System.getProperty( "revision" );
         assertEquals( "8.2.0", revision );
     }
+
+    /**
+     * Read .mvn/maven.config with the following definitions:
+     * <pre>
+     *   -T 3
+     *   -Drevision=1.3.0
+     * </pre>
+     * and check if the {@code -Drevision-1.3.0} option can be overwritten via command line argument when there are
+     * funky arguments present.
+     *
+     * @throws Exception
+     */
+    public void testMVNConfigurationFunkyArguments()
+        throws Exception
+    {
+        System.setProperty( MavenCli.MULTIMODULE_PROJECT_DIRECTORY,
+                            new File( "src/test/projects/mavenConfigProperties" ).getCanonicalPath() );
+        CliRequest request = new CliRequest(
+            new String[]{ "-Drevision=8.1.0", "--file=-Dpom.xml", "\"-Dfoo=bar ", "\"-Dfoo2=bar two\"",
+                "-Drevision=8.2.0" }, null );
+
+        cli.initialize( request );
+        // read .mvn/maven.config
+        cli.cli( request );
+        cli.properties( request );
+
+        String revision = System.getProperty( "revision" );
+        assertEquals( "8.2.0", revision );
+
+        assertEquals( "bar ", request.getSystemProperties().getProperty( "foo" ) );
+        assertEquals( "bar two", request.getSystemProperties().getProperty( "foo2" ) );
+
+        assertEquals( "-Dpom.xml", request.getCommandLine().getOptionValue( CLIManager.ALTERNATE_POM_FILE ) );
+    }
 }