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.

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

fixed absolute command paths with spaces #70

Closed
wants to merge 9 commits into from
Closed

fixed absolute command paths with spaces #70

wants to merge 9 commits into from

Conversation

shenanigans
Copy link
Contributor

There's this weird... safety net in forever-monitor that makes sure you haven't submitted arguments with your command. If you intend to pass an absolute path as a command, you'd better make sure it doesn't contain any spaces. In Windows in particular, it turns out that all executables are called exactly "C:/Program".

@jcrugzz
Copy link
Contributor

jcrugzz commented Aug 19, 2014

@shenanigans if this is a special case for windows, we cannot have it break everything else. i.e: the tests should still pass

@shenanigans
Copy link
Contributor Author

It's not a special case for windows, it's just far more likely to be a problem on windows due to the frequency of spaces appearing in common paths (and the infrequency of the same among *nix developers)

With my current environments, tests produce one failure on linux and nine failures on windows with and without the patch applied. As far as I am aware the functionality removed by this pull request is neither documented nor tested.

It just appears to be there to save your bacon if you accidentally do something like

child.start ("node path/to/script", { options:[ "arg0", "arg1" ] });

However, because it is splitting the executable path on spaces naively, it does not permit the execution of any path containing a space. One could do this non-naively but my opinion is that the entire functionality is unnecessary and probably does not exist in the wild. It doesn't appear to exist in the tests.

@shenanigans
Copy link
Contributor Author

The pull request has been updated with the non-naive solution. *nix paths /with/escaped\ spaces will resolve correctly, as will windows paths "c:\with\spaces escaped by quotes". The argument-stripping functionality remains unchanged.

It slipped my mind that `platform` appears on `process`.
* It slipped my mind that `platform` appears on `process`.
* Two-space tabs on a new section.
@fresheneesz
Copy link

Tested it out, seems to work, both with my "Program Files" node installation (that has whitespace) and with a script file with a path that has spaces. Looks good! Thanks!

@jcrugzz
Copy link
Contributor

jcrugzz commented Sep 17, 2014

@shenanigans this LGTM. cc @indexzero

@indexzero
Copy link
Member

This got merged. Will go out in forever@1.4.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants