Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix broken support for arguments with spaces on Linux #60

Merged
merged 2 commits into from
Dec 3, 2021
Merged

Fix broken support for arguments with spaces on Linux #60

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4381cf4
Fix broken support for arguments with spaces on Linux
vorburger Dec 2, 2021
e6716bb
Update ManagedProcessBuilder.java
vorburger Dec 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions src/main/java/ch/vorburger/exec/ManagedProcessBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,13 +138,13 @@ protected ManagedProcessBuilder addArgument(String argPart1, String separator, S
final StringBuilder sb = new StringBuilder();
final String arg;

// @see MariaDB4j Issue #501 Fix for spaces in data path doesn't work on windows
// https://github.com/vorburger/MariaDB4j/issues/501
// Internally Runtime.exec is being used, which says that an argument such
// @see https://github.com/vorburger/MariaDB4j/issues/501 - Fix for spaces in data path doesn't work on windows:
// Internally Runtime.exec() is being used, which says that an argument such
// as --name="escaped value" isn't escaped, since there's no leading quote
// and it contains a space, so it reescapes it, causing applications to split
// and it contains a space, so it re-escapes it, causing applications such as mysqld.exe to split
// it into multiple pieces, which is why we quote the whole arg (key and value) instead.
if ("=".equals(separator)) {
// We do this trick only on Windows, because on Linux it breaks the behavior.
if ("=".equals(separator) && isWindows()) {
Copy link
Owner Author

@vorburger vorburger Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Blanco27 I was quite tempted to further simplify this and ditch the "=".equals(separator) entirely here, but then opted to "be conservative" and propose it like this for you... but if you have a minute to try it out without that and just as isWindows() I guess that works as well for you on Windows, right? I won't really make any difference in MariaDB4j's usage anyway, but it seems simpler and (a bit, it's a mess anyway) cleaner, to me - what are your thoughts?

Suggested change
if ("=".equals(separator) && isWindows()) {
if (isWindows()) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried it. The isWindows() is enough to make it work.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK then let's just use that; I'll change that and then merge this.

sb.append(argPart1);
sb.append(separator);
sb.append(argPart2);
Expand Down Expand Up @@ -256,7 +256,7 @@ public ManagedProcessBuilder addStdErr(OutputStream stdError) {
/* package-local... let's keep ch.vorburger.exec's API separate from Apache Commons Exec, so it
* COULD be replaced */
CommandLine getCommandLine() {
if ((getWorkingDirectory() == null) && commonsExecCommandLine.isFile()) {
if (getWorkingDirectory() == null && commonsExecCommandLine.isFile()) {
File exec = new File(commonsExecCommandLine.getExecutable());
File dir = exec.getParentFile();
if (dir == null) {
Expand All @@ -278,4 +278,9 @@ CommandLine getCommandLine() {
return commonsExecCommandLine.toString();
}

// inspired by org.apache.commons.lang3.SystemUtils.IS_OS_WINDOWS
// without security manager support; assumes System.getProperty() works
private boolean isWindows() {
return System.getProperty("os.name").startsWith("Windows");
}
}