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

Escape whitespace in package path #75

Closed

Conversation

alexeckermann
Copy link

Fix: Escapes whitespace in cwd paths. Closes #74.


Additional information

  • This is a simple patch to add an escape character to any space found in the options.packages path. Fixes Bootstrap fails when destination path has a space #74.
  • It may be beneficial to escape the cwd further up the chain than in this specific option. However, that may interfere with node path commands (not sure how they cope with escapes).

@pomek
Copy link
Member

pomek commented Apr 25, 2018

Thanks for the PR. However, there are two things that should be corrected before the merge:

  1. CI is red (eslint) – https://travis-ci.org/cksource/mgit2/builds/370576453#L1347
  2. Would be good to have a test for proposed changes. We have a few integration tests which check real files (fixtures). You could create a new directory that will contain a space in its name, call the getOptions() function and check whether returned options are escaped correctly.

It may be beneficial to escape the cwd further up the chain than in this specific option. However, that may interfere with node path commands (not sure how they cope with escapes).

I guess you're right. We should consider escaping the whole cwd. I assume that on Linux (and Mac) it will work. The question is – how does it work on Windows?

@pomek
Copy link
Member

pomek commented Apr 25, 2018

It may be beneficial to escape the cwd further up the chain than in this specific option.

I resolved that issue in #77.

@alexeckermann
Copy link
Author

alexeckermann commented Apr 26, 2018

@pomek looks like you've solved this better than I have, where it should be fixed. I'm happy to close this and defer to #77.

Most likely will need a consideration for Windows. In a quick search I found, to my bemusement, that Windows uses ^ to escape spaces in paths — yeah, I know, right?!

@pomek
Copy link
Member

pomek commented Apr 26, 2018

I've just checked on Windows 10 and you're right but it depends on CLI which is run.

E.g. in Git Bash, I could use cd path/some\ space and cd "path/some space". Both cases work fine.

In Windows CMD I had to use ^cd path\some^ space or cd "path\some space".

Because we have a common version – "path wrapped in quotation marks", I would like to use this version in every command that requires the path.

I'll prepare a new PR which does it and remove the escaping from #78 because it won't be necessary.

@pomek
Copy link
Member

pomek commented Apr 26, 2018

Here is the PR – #78.

Becuase #78 resolves both environments (I mean Unix and Windows), will you agree that your PR could be closed and shouldn't be merged?

@Reinmar
Copy link
Member

Reinmar commented May 4, 2018

I merged #78. It's indeed a simpler and more complete solution.

Still, in long-term we should think about finding some args -> cmd converter.

@Reinmar Reinmar closed this May 4, 2018
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.

None yet

3 participants