Skip to content
This repository has been archived by the owner on Dec 9, 2023. It is now read-only.

Don't add quotes if the file name already has quotes #40

Merged
merged 1 commit into from
Aug 9, 2019

Conversation

blasten
Copy link

@blasten blasten commented Aug 9, 2019

@@ -26,7 +26,7 @@ String sanitizeExecutablePath(String executable,
if (!platform.isWindows) {
return executable;
}
if (executable.contains(' ') && !executable.startsWith('"')) {
if (executable.contains(' ') && !executable.contains('"')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if it contains \ (an escaped space)?

Copy link
Author

Choose a reason for hiding this comment

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

I could be wrong, but my understanding is that: C:\"foo"\bar resolves as C:\foo\bar. If this is true, then C:\"foo^ bar" resolves as C:\foo^ bar. (^ is Windows way to escape spaces). I don't have a Windows computer, but let me ask to see if we can verify it.

Copy link
Author

Choose a reason for hiding this comment

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

These paths are resolved correctly:

Process.runSync('"C:\\Users\\franciscojma\\Hello World\\git.exe"', const <String>[]);
Process.runSync('\"C:\\Users\\franciscojma\\Hello World\\git.exe\"', const <String>[]);

The ones below returned "Process not found":

Process.runSync('C:\\Users\\franciscojma\\"Hello World"\\git.exe', const <String>[]);
Process.runSync('C:\\Users\\franciscojma\\"Hello^ World"\\git.exe', const <String>[]);
Process.runSync('"C:\\Users\\franciscojma\\Hello^ World"\\git.exe"', const <String>[]);
Process.runSync('C:\\Users\\franciscojma\\Hello^ World\\git.exe', const <String>[]);

Copy link
Contributor

@tvolkert tvolkert left a comment

Choose a reason for hiding this comment

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

LGTM.

I added you to the repo and pub publishers, so you should be able to merge and publish

@blasten blasten merged commit 57a88cd into google:master Aug 9, 2019
@blasten blasten deleted the quote branch August 9, 2019 21:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants