Page MenuHomePhabricator

Don't remove backslashes from arguments unless the following char is recognized.
ClosedPublic

Authored by zturner on Jan 16 2015, 11:47 AM.

Details

Summary

This fixes file paths on Windows, as you can now write, for example, d:\foo\bar.txt, but does not break the case that this tokenization logic was originally designed for, which is to allow escaping of things like quotes and double quotes, so that all of the escapable characters can appear in the same string together.

I think this is a much simpler and more straightforward fix than adding a different tokenizer. Having only 1 tokenizer means we don't have to worry in the future about tests breaking because someone wrote their test on unix and tried to execute a command with a syntax that wouldn't work on Windows, etc. So hopefully we can reach an agreement about this fix.

Diff Detail

Event Timeline

zturner updated this revision to Diff 18309.Jan 16 2015, 11:47 AM
zturner retitled this revision from to Don't remove backslashes from arguments unless the following char is recognized..
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added reviewers: jingham, clayborg.
zturner added a subscriber: Unknown Object (MLST).
clayborg accepted this revision.Jan 16 2015, 1:29 PM
clayborg edited edge metadata.

Seems like it should work as long as the test suite passes fine.

This revision is now accepted and ready to land.Jan 16 2015, 1:29 PM
jingham edited edge metadata.Jan 16 2015, 2:10 PM

If I had a path component that began with a space (dunno if you can do that on Windows) what would I have to do to enter that?

d:\\ foo\bar

?

I guess that's not too horrible...

Jim

If I had a path component that began with a space (dunno if you can do that on Windows) what would I have to do to enter that?

d:\\ foo\bar

?

I guess that's not too horrible...

Jim

That's not a valid file path on Windows. (path components can't have leading or trailing whitespace). Although if for whatever reason you really wanted to specify it, you could still do "d:\ foo\bar"

Going to go ahead and commit this. Did not see test failures on Linux. My Mac is down at the moment, but I think any tests failures that were going to happen should have surfaced on Linux. If we do see failures, whoever sees it first is free to revert to green until I figure it out.

This revision was automatically updated to reflect the committed changes.