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

fix: Don't error if $OPTS is not yet defined in .zinit-compinit call #241

Merged
merged 1 commit into from
May 2, 2022

Conversation

jankatins
Copy link
Contributor

This codepath was triggered when deleting the whole ~/.ziniti/plugins dir: during recreation in .zinit-prepare-home, .zinit-compinit &> /dev/null would be called and the sourcing of zinit.zsh would error out with

+.zinit-compinit:2> [[.zinit-compinit:2: bad math expression: operand expected at `/Users/jan...'
 -n '' ]]

As the error was redirected to /dev/null, this never showed up. Any zinit install call during that session would not run any hooks as they were not yet registered. The next session would then be fine because the plugins dir would exist.

Motivation and Context

See #129 (comment) -> basically deleting the ~/.zinit/plugins dir should result in complete recreation on the next run, but due to this bug, it had bad side effects.

Closes: #129 (probably, not tested on initial run, only after deleting the plugins dir)

How Has This Been Tested?

Manually deleting the whole folder and starting the whole zinit setup from scratch

rm -rf ~/.zinit/plugins/ && exec zsh

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

This codepath was triggered when deleting the whole `~/.zinit/plugins` dir: during recreation in `.zinit-prepare-home`, `.zinit-compinit &> /dev/null` would be called and the sourcing of `zinit.zsh` would error out with

```
+.zinit-compinit:2> [[.zinit-compinit:2: bad math expression: operand expected at `/Users/jan...'
 -n '' ]]
```

As the error was redirected to `/dev/null`, this never showed up. Any zinit install call during that session would not run any hooks as they were not yet registered. The next session would then be fine because the plugins dir would exist.
@vladdoster
Copy link
Member

@jankatins Awesome find and fix. I really appreciate the use of commit message using the prefix fix:!

@pschmitt pschmitt merged commit 4c5854b into zdharma-continuum:main May 2, 2022
@vladdoster
Copy link
Member

@pschmitt @jankatins The conditional should have used -v

[[ -v SOME_VARIABLE ]]

@pschmitt
Copy link
Member

pschmitt commented May 2, 2022

Pendantic but true, this is effectively the same though.

@vladdoster
Copy link
Member

vladdoster commented May 2, 2022

Since it errors on operand and would result in smaller diff, is there a reason not to use

(( expr ))

I see

[[ expr ]]

used elsewhere, but seems redundant to check the same variable twice.

@jankatins @pschmitt

@vladdoster
Copy link
Member

Regardless of the answer above, this brings up the issue of deleting zinit directories.

As I've said before @jankatins, this is not how you should delete plugins.

Screen Shot 2022-05-02 at 01 55 51

Use zinit delete --all

I would assume this edge-case is handled in the delete command since it deletes subdirectories and not the actual $ZINIT[PLUGINS_DIR] directory. So, is this needed or just not using the proper method of deleting plugins?

@pschmitt
Copy link
Member

pschmitt commented May 2, 2022

zsh man page :

In other words,
[[ $var ]] is the same as [[ -n $var ]]. It is
recommended that the second, explicit, form be used where
possible.

@vladdoster
Copy link
Member

vladdoster commented May 2, 2022

Regardless of the answer above, this brings up the issue of deleting zinit directories.

As I've said before @jankatins, this is not how you should delete plugins.

Screen Shot 2022-05-02 at 01 55 51

Use zinit delete --all

I would assume this edge-case is handled in the delete command since it deletes subdirectories and not the actual $ZINIT[PLUGINS_DIR] directory. So, is this needed or just not using the proper method of deleting plugins?

Link to my previous comment

@vladdoster
Copy link
Member

zsh man page :

In other words,
[[ $var ]] is the same as [[ -n $var ]]. It is
recommended that the second, explicit, form be used where
possible.

TIL :^)

@vladdoster
Copy link
Member

vladdoster commented May 2, 2022

Regardless of the answer above, this brings up the issue of deleting zinit directories.
As I've said before @jankatins, this is not how you should delete plugins.
Screen Shot 2022-05-02 at 01 55 51
Use zinit delete --all
I would assume this edge-case is handled in the delete command since it deletes subdirectories and not the actual $ZINIT[PLUGINS_DIR] directory. So, is this needed or just not using the proper method of deleting plugins?

Link to my previous comment

Indeed, it avoids this issue altogether.

Screen Shot 2022-05-02 at 02 06 52

@github-actions
Copy link

github-actions bot commented Nov 7, 2022

🎉 This PR is included in version 3.8.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug]: mv ice does not rename file on initial run
3 participants