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

July 5th "Shellcheck recommendations" patch broke profile set up #458

Closed
jcsalem opened this issue Jul 7, 2014 · 8 comments
Closed

July 5th "Shellcheck recommendations" patch broke profile set up #458

jcsalem opened this issue Jul 7, 2014 · 8 comments

Comments

@jcsalem
Copy link

jcsalem commented Jul 7, 2014

In install.sh, changing the printf lines from:
printf $SOURCE_STR
to
printf "%s" "$SOURCE_STR"
broke the profile appending code. With printf "%s", the "\n" characters in $SOURCE_STR are written verbatim rather than a written as new lines. E.G., it looks like this in my .bash_profile:
\nexport NVM_DIR="/home/vagrant/.nvm"\n[ -s "$NVM_DIR/nvm.sh" ] && . "$NVM_DIR/nvm.sh" # This loads nvm

You should revert that change.

Commit ID: e0537ce

PS: You should add a trailing newline to the insertion as well.

@ljharb
Copy link
Member

ljharb commented Jul 7, 2014

Thanks for reporting - (related to #457, ping @koenpunt)

@koenpunt
Copy link
Contributor

koenpunt commented Jul 7, 2014

Not really related, why did you change it to the string substitution version?

@ljharb
Copy link
Member

ljharb commented Jul 7, 2014

I've been trying to follow shellcheck's recommendations, but obviously I didn't understand the implications of this particular change.

@ljharb
Copy link
Member

ljharb commented Jul 7, 2014

That said, this is in master, not a tagged release, so it shouldn't break anyone - nobody should be installing off of master except to test upcoming releases.

@koenpunt
Copy link
Contributor

koenpunt commented Jul 7, 2014

The additional quotes are proper changes, but in the case of printf, it has a format and values, and in this case the string is the format. No biggie

@ljharb ljharb closed this as completed in e4ada9f Jul 7, 2014
@ljharb ljharb changed the title July 5th "Spellcheck recommendations" patch broke profile set up July 5th "Shellcheck recommendations" patch broke profile set up Jul 7, 2014
@jcsalem
Copy link
Author

jcsalem commented Jul 8, 2014

This doesn't fix the issue.
You need to remove the format string in line 125 as well. It should read:
printf "$SOURCE_STR\n" >> "$PROFILE"

koenpunt added a commit to koenpunt/nvm that referenced this issue Jul 8, 2014
ljharb added a commit that referenced this issue Jul 8, 2014
@ljharb
Copy link
Member

ljharb commented Jul 8, 2014

Fixed in #460

@jcsalem
Copy link
Author

jcsalem commented Jul 8, 2014

:-)

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

No branches or pull requests

3 participants