feat: improve error messaging for shell config permission issues (#3788)#3822
feat: improve error messaging for shell config permission issues (#3788)#3822SortedApe wants to merge 2 commits into
Conversation
ljharb
left a comment
There was a problem hiding this comment.
Good direction, but the ordering regresses idempotent re-runs - see my inline comments, including a couple of portability nits and a request for an install_script test as well.
| nvm_echo >&2 "=> OR, append the following lines to $NVM_PROFILE yourself:" | ||
| command printf '%b' "${SOURCE_STR}" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Blocking - idempotency regression. This block runs before the grep -qc '/nvm.sh' check on line 468. Today, a re-run against an already-configured profile is a no-op ("already in"). With this change, if that same profile is non-writable (root-owned in managed envs, intentionally chmod'd, etc.) the installer now aborts even though no write is needed.
Please move the permission check inside the if ! grep -qc '/nvm.sh' ... branch so it only fires when we actually need to append. Worth adding the same guard around the bash_completion append below, too.
There was a problem hiding this comment.
I have moved the permission check inside the "if ! grep -qc '/nvm.sh' "(including the bash_completion block)
Also, I have made the permission check into a function (check on install.sh nvm_check_file_writable)
| fi | ||
| if [ -f "$NVM_PROFILE" ] && [ ! -w "$NVM_PROFILE" ]; then | ||
| local OWNER_NAME | ||
| OWNER_NAME="$(command ls -l "$NVM_PROFILE" | command awk '{print $3}')" |
There was a problem hiding this comment.
Parsing ls -l for the owner is fragile: column 3 is not guaranteed across locales, ACL markers (a + in the mode column), or SELinux contexts - any of those can shift the columns.
Suggest either:
OWNER_NAME="$(command id -un "$(command stat -c %u "$NVM_PROFILE" 2>/dev/null || command stat -f %u "$NVM_PROFILE")" 2>/dev/null)"(handles GNU + BSD stat) or dropping the owner name entirely — the diagnostic value is marginal vs. the parsing risk.
| OWNER_NAME="$(command ls -l "$NVM_PROFILE" | command awk '{print $3}')" | ||
| nvm_echo >&2 "=> Error: $NVM_PROFILE is not writable!" | ||
| nvm_echo >&2 "=> The file is currently owned by '$OWNER_NAME'. Only the owner or root can modify it." | ||
| nvm_echo >&2 "=> Fix this by running: sudo chown \$(whoami) $NVM_PROFILE" |
There was a problem hiding this comment.
Two small things here:
- Prefer
"$USER"or"$(id -un)"overwhoami- it's more portable. - Consider softening the wording.
sudo chownon a dotfile is reasonable advice on a personal laptop, but users on managed multi-user systems may have legitimate reasons the file isn't theirs; a blanket instruction to take ownership of it isn't always the right fix.
| nvm_echo >&2 "=> The file is currently owned by '$OWNER_NAME'. Only the owner or root can modify it." | ||
| nvm_echo >&2 "=> Fix this by running: sudo chown \$(whoami) $NVM_PROFILE" | ||
| nvm_echo >&2 "=> OR, append the following lines to $NVM_PROFILE yourself:" | ||
| command printf '%b' "${SOURCE_STR}" |
There was a problem hiding this comment.
Minor stream-consistency nit: the error lines above all go to stderr, but SOURCE_STR goes to stdout. That matches the existing pattern at line 452, so it's defensible, but on the failure path here the manual-append payload ends up split from its context. Not blocking.
4ecb056 to
6b9332a
Compare
feat: improve error messaging for shell config permission issues (#3788)
Adds a write-permission check for .bashrc and .zshrc to help users
diagnose installation failures caused by incorrect file ownership (e.g., files owned by root).