-
Notifications
You must be signed in to change notification settings - Fork 137
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
Update the installer! #43
Update the installer! #43
Conversation
1f86c57
to
8de591b
Compare
doc/install.sh
Outdated
# Get the download-progress bar tool | ||
GIT_PROCESS_SCRIPT_URL="https://raw.githubusercontent.com/zdharma-continuum/zinit/${ZINIT_BRANCH}/git-process-output.zsh" | ||
mkdir -p /tmp/zinit | ||
cd /tmp/zinit || { echo "Failed to cd to /tmp/zinit" >&2; exit 1; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're messing with temp directories, how about fixing the security issues?
Use TMPDIR if available with a fallback to /tmp/zinit-$UID/ and ensure you properly chmod/chown or exit if it already exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to do so but a bit out of scope for this MR. Will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: f4e1468
echo "[34m▓▒░[0m .zshrc already contains \`zinit …' commands – not making changes." | ||
RCUPDATE=0 | ||
fi | ||
|
||
if [ $RCUPDATE -eq 1 ]; then | ||
echo "[34m▓▒░[0m Updating $THE_ZDOTDIR/.zshrc (10 lines of code, at the bottom)" | ||
ZINIT_HOME="$(echo $ZINIT_HOME | sed "s|$HOME|\$HOME|")" | ||
ZINIT_HOME="$(echo "$ZINIT_HOME" | sed "s|$HOME|\$HOME|")" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be done entirely in Zsh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the entire script could in zsh. What's your point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I missed that it was in /bin/sh
. My bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm. Would love to see bash coloring functions instead of inlined escape characters, but it's just a wishlist item.
That actually bothered me as well. Didn't want to create a "I REWROTE THE ENTIRE INSTALLER PLZ REVIEW" MR though :D I'll create an issue to refactor and over-engineer the install script. |
🎉 This PR is included in version 3.8.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
ZINIT_BRANCH
(defaults to master) This might be useful for development, CI and when (if!) we decide to update change the main branch of zinit to "main".