Remove a lot of escaping logic that's unreachable and never used

[elharo]

One of those threads you pull on that unravels half the ball. I could remove more but I'm leaving open the possibility that some public and protected methods are invoked or overridden outside this repo. This is a pretty strong case of YAGNI, and we're even less likely to need this in the future, since Java 11+ provide much better options for doing what these classes do.

[Copilot]

Pull request overview

This PR removes a significant amount of quoting/escaping configurability from the CLI shell abstraction, aiming to simplify command-line construction by eliminating logic believed to be unused/unreachable.

Changes:

• Simplifies Shell by removing most escaping/quoting configuration and related helpers.
• Updates CmdShell/BourneShell to stop using removed configuration and adds missing @Override annotations.
• Makes parts of argument-quoting control less externally accessible (e.g., isQuotedArgumentsEnabled()).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/main/java/org/apache/maven/shared/utils/cli/shell/Shell.java	Removes the core quote/escape implementation and related configuration methods/fields.
src/main/java/org/apache/maven/shared/utils/cli/shell/CmdShell.java	Stops enabling quoted executables (removed API) and annotates the override of getCommandLine().
src/main/java/org/apache/maven/shared/utils/cli/shell/BourneShell.java	Removes now-defunct quoting configuration calls and annotates overrides.

Comments suppressed due to low confidence (2)

src/main/java/org/apache/maven/shared/utils/cli/shell/CmdShell.java:86

• There are existing tests covering CmdShell with simple arguments, but none that assert correct quoting for executables/arguments containing spaces/quotes. Given the behavioral change in how command strings are built here, add a unit test that covers an executable path with spaces and an argument with spaces to prevent regressions in Windows command-line parsing.

    @Override
    public List<String> getCommandLine(String executable, String... arguments) {
        StringBuilder sb = new StringBuilder();
        sb.append('"');
        sb.append(super.getCommandLine(executable, arguments).get(0));
        sb.append('"');

        return Arrays.asList(sb.toString());

src/main/java/org/apache/maven/shared/utils/cli/shell/CmdShell.java:86

• With the base Shell.quoteOneItem() now a no-op, this CmdShell implementation no longer quotes/escapes an executable or arguments that contain spaces/quotes before wrapping the full command line in "...". This is likely to break common Windows cases (paths under Program Files, args with spaces). Either override quoteOneItem() in CmdShell (cmd-specific quoting) or ensure getCommandLine() performs the necessary quoting/escaping for each item before concatenation.

    @Override
    public List<String> getCommandLine(String executable, String... arguments) {
        StringBuilder sb = new StringBuilder();
        sb.append('"');
        sb.append(super.getCommandLine(executable, arguments).get(0));
        sb.append('"');

        return Arrays.asList(sb.toString());

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

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).

Suggested change

	return inputString;
	// 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();

[elharo]

copilot missed that getEscapeChars always returned an empty list so nothing was ever escaped.