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.

Sign up for GitHub

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

perf(shell/zsh.rs): avoid hook-env execution on Enter without command #2861

Merged
merged 10 commits into from
Nov 5, 2024
Merged

perf(shell/zsh.rs): avoid hook-env execution on Enter without command #2861

Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2715a35
perf(shell/zsh.rs): avoid hook-env execution on Enter without command
powerman Oct 29, 2024
98ae011
fix(shell/zsh.rs): always trigger hook on cd
powerman Oct 29, 2024
6df7f53
fix(shell/zsh.rs): ensure hook-env will run on zsh start
powerman Oct 29, 2024
fe67426
fix(shell/zsh.rs): deactivate _mise_hook_cmd
powerman Oct 30, 2024
a25daa5
fix(shell/zsh.rs): deactivate precmd_functions
powerman Oct 30, 2024
5288683
Merge branch 'main' into patch-2
jdx Nov 4, 2024
92748cd
Merge branch 'main' into patch-2
jdx Nov 4, 2024
760ab74
Merge branch 'main' into patch-2
jdx Nov 5, 2024
1034f3a
test: updated snapshots
jdx Nov 5, 2024
252d6ab
Merge branch 'main' into patch-2
jdx Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions src/shell/zsh.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ impl Shell for Zsh {
export MISE_SHELL=zsh
export __MISE_ORIG_PATH="$PATH"

eval "$({exe} hook-env{flags} -s zsh)"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes two issues:

  • One is related to this PR: first command in a new zsh won't get right env because of this PR's optimization.
  • Another issue is not related to this PR and fixes zsh -i -c '...' - current mise implementation won't provide right env to the command executed inside child zsh.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember why but there is a reason I chose not to run hook-env automatically

Copy link
Contributor Author

@powerman powerman Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember why but there is a reason I chose not to run hook-env automatically

Sometimes only way to recall it is to change a code and wait until something breaks.

I'm not sure is missing env in a zsh -i -c '...' considered a real issue. This is probably a real "corner" case and might not worth supporting: without -i it won't load Mise anyway, but -i with -c makes not much sense and in my experience it used mostly for testing zsh performance - how fast it loads user configs in interactive mode.

Still, your doc example in https://mise.jdx.dev/dev-tools/shims.html#zshrc-bashrc-files ask user to run it manually after activation - which has exactly same effect as my change to run it automatically but require extra manual change in user's config. Also, same 2-command example is repeated in https://mise.jdx.dev/troubleshooting.html#mise-isn-t-working-when-calling-from-tmux-or-another-shell-initialization-script.

So, I'm not joking here: if you can't remember why you chose not to run hook-env automatically then in current situation I think it makes sense to start running it automatically - this way it either will work and let you simplify UX and clean up docs, or breaks and this time you'll add a comment somewhere describing why it's bad idea to run it automatically on activation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this was the reason: #204 (comment)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that dotenv conflict is still actual, then you probably need to mention needs to manually add second line with hook-env in shell rc files at https://mise.jdx.dev/getting-started.html#_2-activate-mise (and maybe with a note about direnv and what to do between these two lines to avoid conflict with dotenv.


mise() {{
local command
command="${{1:-}}"
Expand All @@ -44,9 +46,14 @@ impl Shell for Zsh {
_mise_hook() {{
eval "$({exe} hook-env{flags} -s zsh)";
}}
_mise_hook_cmd() {{
if [[ -n "$ZLAST_COMMANDS" ]]; then
_mise_hook;
fi
}}
typeset -ag precmd_functions;
if [[ -z "${{precmd_functions[(r)_mise_hook]+1}}" ]]; then
precmd_functions=( _mise_hook ${{precmd_functions[@]}} )
if [[ -z "${{precmd_functions[(r)_mise_hook_cmd]+1}}" ]]; then
precmd_functions=( _mise_hook_cmd ${{precmd_functions[@]}} )
fi
typeset -ag chpwd_functions;
if [[ -z "${{chpwd_functions[(r)_mise_hook]+1}}" ]]; then
Expand Down Expand Up @@ -80,8 +87,9 @@ impl Shell for Zsh {

fn deactivate(&self) -> String {
formatdoc! {r#"
precmd_functions=( ${{precmd_functions:#_mise_hook}} )
precmd_functions=( ${{precmd_functions:#_mise_hook_cmd}} )
chpwd_functions=( ${{chpwd_functions:#_mise_hook}} )
unset -f _mise_hook_cmd
unset -f _mise_hook
unset -f mise
unset MISE_SHELL
Expand Down
Loading