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

make updater.sh check explicitly for Y/y instead of N/n #1511

Merged
merged 1 commit into from Jul 24, 2022

Conversation

infinitewarp
Copy link
Contributor

@infinitewarp infinitewarp commented Jul 19, 2022

Hello user.js folks! 👋 Long time listener, first time caller here.

I recently used the updater.sh script to update one of my profiles, but I fat-fingered my reply to the confirmation prompt with a T instead of a Y, and it proceeded to update anyway. This seems bad! Confirmation prompts like this should err on the side of caution and only perform a destructive operation if the user explicitly enters an affirmative Y/y response. (Yes, I'm aware that the script makes a backup file, but still…)

I dug into the code and realized that you're doing the opposite: updater.sh only aborts upon receiving a N/n response. Any other input appears to be treated as a destructive Y/y. Yikes!

So, I swapped the logic around. It now treats only Y/y as an affirmative response.

Tested using q and then y in this demo of your version versus mine:

asciicast

@infinitewarp infinitewarp changed the title make updater.sh check explicitly for Y/n instead of N/n make updater.sh check explicitly for Y/y instead of N/n Jul 19, 2022
@earthlng
Copy link
Contributor

Hello 1st-time caller, I hope you enjoyed the show so far :)

Thanks for your PR, I agree with you and would gladly merge your PR but please change line 198 to
[[ $REPLY =~ ^[Yy]$ ]] || return 0

@earthlng earthlng merged commit 4b42481 into arkenfox:master Jul 24, 2022
@Thorin-Oakenpants
Copy link
Contributor

Thanks @earthlng ❤️

@infinitewarp infinitewarp deleted the fix-updater-yn branch July 25, 2022 05:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants