Re: [PR] Remove a lot of escaping logic that's unreachable and never used [maven-shared-utils]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #369:
URL: https://github.com/apache/maven-shared-utils/pull/369#discussion_r2787887179


##########
src/main/java/org/apache/maven/shared/utils/cli/shell/BourneShell.java:
##########
@@ -34,16 +34,12 @@ public class BourneShell extends Shell {
     public BourneShell() {
         setUnconditionalQuoting(true);

Review Comment:
   The constructor still calls `setUnconditionalQuoting(true)`, but unconditional quoting no longer has any effect because neither `Shell.quoteOneItem()` nor this class's override consults `unconditionalQuoting` anymore. Either remove this call (if the flag is being deprecated) or reintroduce behavior where `unconditionalQuoting` influences quoting so the public API remains meaningful.
   ```suggestion
   
   ```



##########
src/main/java/org/apache/maven/shared/utils/cli/shell/Shell.java:
##########
@@ -104,14 +94,7 @@ String[] getShellArgs() {
     }
 
     protected String quoteOneItem(String inputString, boolean isExecutable) {
-        char[] escapeChars = getEscapeChars(isSingleQuotedExecutableEscaped(), isDoubleQuotedExecutableEscaped());
-        return StringUtils.quoteAndEscape(
-                inputString,
-                isExecutable ? getExecutableQuoteDelimiter() : getArgumentQuoteDelimiter(),
-                escapeChars,
-                getQuotingTriggerChars(),
-                '\\',
-                unconditionalQuoting);
+        return inputString;

Review Comment:
   `quoteOneItem()` now returns the input unchanged, which effectively disables all quoting/escaping in the base `Shell`. This changes behavior for callers that rely on `setQuotedArgumentsEnabled(true)` (default) to safely/accurately build a shell command string—most notably `CmdShell`, where an executable or argument containing spaces (e.g., `C:\Program Files\...`) will be emitted unquoted inside the `/C "..."` command string and can no longer be executed correctly. Consider restoring quoting/escaping here (e.g., via `StringUtils.quoteAndEscape` or an equivalent implementation), or moving the required quoting logic into each concrete shell implementation (and ensuring `CmdShell` handles Windows quoting rules).
   ```suggestion
           // If quoting is disabled, honor that and return the input unchanged.
           if (!quotedArgumentsEnabled) {
               return inputString;
           }
   
           if (inputString == null) {
               return null;
           }
   
           boolean needsQuoting = unconditionalQuoting;
   
           // Determine if any trigger characters are present that require quoting (e.g., spaces).
           if (!needsQuoting) {
               for (char trigger : DEFAULT_QUOTING_TRIGGER_CHARS) {
                   if (inputString.indexOf(trigger) >= 0) {
                       needsQuoting = true;
                       break;
                   }
               }
           }
   
           if (!needsQuoting) {
               return inputString;
           }
   
           // Quote the argument and escape any embedded double quotes.
           StringBuilder buf = new StringBuilder(inputString.length() + 2);
           buf.append('"');
           for (int i = 0; i < inputString.length(); i++) {
               char ch = inputString.charAt(i);
               if (ch == '"') {
                   buf.append('\\').append('"');
               } else {
                   buf.append(ch);
               }
           }
           buf.append('"');
   
           return buf.toString();
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org