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

Fixes #41 - Issue with blank spaces in path #355

Merged
merged 3 commits into from
Aug 11, 2018

Conversation

s-h-a-d-o-w
Copy link
Contributor

exec.Command() can't handle spaces, even when quoted.
Command has to be modified using SysProcAttr.
See also:
golang/go#15566

Plus: Minor refactoring - reordering of functions, more consistent capitalization, sorting of imports and whatnot.
Let me know if you don't appreciate this part. ;)

@coreybutler
Copy link
Owner

I know I'm way behind... wanted you to know I'm testing this, targeting inclusion the 1.1.8 release.

@s-h-a-d-o-w
Copy link
Contributor Author

Cool, thanks for the update!

exec.Command() can't handle spaces, even when quoted.
Command has to be modified using SysProcAttr.
See also:
golang/go#15566

Plus: Minor refactoring - reordering of functions, more consistent capitalization, sorting of imports and whatnot.
Forgot some uses of exec.Command() outside of the "use" command.
@coreybutler
Copy link
Owner

@s-h-a-d-o-w - Are you using different line endings? It looks like the entire file is being replaced instead of just your changes... I suspect a different type of line-ending. I'd like to see that cleaned up, then I'll merge this since it seems like it's fixing problems for several folks.

Copy link
Owner

@coreybutler coreybutler left a comment

Choose a reason for hiding this comment

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

The whole file shouldn't be replaced... otherwise looks good.

@s-h-a-d-o-w
Copy link
Contributor Author

s-h-a-d-o-w commented Aug 11, 2018

I have core.autocrlf set to true. Since I've never worked on a project with committed CRLFs and I doubt that I ever will again, could you please fix that? (I assume you're more familiar with this than me)
Might be a good idea to add a .gitattributes with * text eol=crlf to avoid something like this in the future.

@coreybutler coreybutler merged commit 08fb700 into coreybutler:master Aug 11, 2018
@coreybutler
Copy link
Owner

@s-h-a-d-o-w I'm not sure how the line endings got screwed up in the first place, but I've taken your suggestion and added a .gitattributes file. I'm actually going to force line feeds instead of CRLF, which should be a more natural fit for most. I've also taken care of the existing issue and some of the resulting merge conflicts... so, all should be in order now.

Aside from fixing a longstanding issue, I was happy to see some of your other nice changes (which was also a stark reminder of how much of a go novice I was when I wrote some of that code!). Anyhow, this contribution was definitely needed and super-appreciated. There are a few other maintenance things I'll be adding to the project before the next release, but your work should show up then. Unless you object to it, I'll be listing your handle amongst the thank-you's in the README.

@s-h-a-d-o-w
Copy link
Contributor Author

Thank you, it's always great to contribute to a project where one's work is appreciated!
I also appreciate the offer of adding my handle to the README would be nice - I'd like that.

(On a side note: Not sure how to say this because I don't want to take away from your efforts but instead express how happy I was but... I had actually never used Go before and was glad to be done in about 2 hours. Didn't find the experience too pleasant. I for one will stick with JS, C/C++, Python and Java for the time being. 😉 )

@coreybutler
Copy link
Owner

Haha - no worries. I considered using Java instead of Go, but it has been ages since I've done anything real with it. For me, Go is a bit like git was the first time.... took some getting used to, but then I was pretty happy with it. Simple cross-platform compilation was the big winner (and yes, that's a hint about the future of NVM4W).

@s-h-a-d-o-w
Copy link
Contributor Author

Simple cross-platform, huh? Didn't know that about Go. I can see how that's appealing. Makes me think of Python (basically the only language I truly enjoy for its own sake - it's just so concise (mostly) and convenient...) - have mostly worked with that on Windows but it seems like its modules are platform-agnostic.
Anyway - all the best for that future. 😉

@ctsstc
Copy link

ctsstc commented Sep 2, 2018

Any chance of a pre-release for 1.1.8 @coreybutler excited for this fix : ) thanks @s-h-a-d-o-w

@s-h-a-d-o-w
Copy link
Contributor Author

@ctsstc You could use the release from my fork until Corey releases 1.1.8.

@RobertDaleSmith
Copy link

@coreybutler could we get an updated release install including this bug fix?

@owenblacker
Copy link

+1 on the updated release, please? 😊

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.

5 participants