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

Fix bash login when $CMDER_ROOT has spaces #1078

Merged

Conversation

orionlee
Copy link
Contributor

The patch fixed the errors when using bash and cmder is installed under a directory with spaces in the path, e.g. c:\Users\Foo Bar\cmder . Please review. Thanks.

The first commit fixed errors I encountered (in v1.3.0), while the second commit fixed similar issues.

Note: The development branch (the base) is behind master, in fact, pre v1.3.0. Should I stick with merging to it?

E.g., if ${CMDER_ROOT} is /c/Users/Foo Bar/cmder,
the following errors will occur:
  bash: pushd: /c/Users/Foo: No such file or directory
  bash: [: /c/Users/Foo: binary operator expected
  bash: /c/Users/Foo: No such file or directory
Inspecting the script uncovers similar problems elsewhere
not encountered in my initial testing. They are fixed accordingly.
@orionlee
Copy link
Contributor Author

FYI, the original errors I encountered:

bash: pushd: /c/Users/Foo: No such file or directory
bash: [: /c/Users/Foo: binary operator expected
bash: /c/Users/Foo: No such file or directory

@Stanzilla
Copy link
Member

No, please change to master. @daxgames are you ok with this? I mean it's sort of a trivial change (at least the first commit) but you wrote the code in the first place.

Thanks for the PR, orionlee!

@orionlee orionlee changed the base branch from development to master August 21, 2016 05:34
@orionlee
Copy link
Contributor Author

@Stanzilla I agree 2nd commit is slightly tricky (double quotes cannot handle having spaces along with a wild card), and is more prone to regress.

@daxgames
Copy link
Member

Looks good to me.

@Stanzilla Stanzilla merged commit 355df7a into cmderdev:master Aug 21, 2016
@Stanzilla
Copy link
Member

Thank you!

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.

3 participants