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

perf: Skip unnecessary indirection in starship init zsh #5322

Merged
merged 1 commit into from Aug 31, 2023

Conversation

WhyNotHugo
Copy link
Contributor

The installation instructions indicate that one should add this snippet to zsh's configuration:

eval "$(starship init zsh)"

The command starship init zsh prints a little shell script for zsh to execute:

> starship init zsh
source <(/usr/bin/starship init zsh --print-full-init)%

Running starship init zsh --print-full-init prints yet another script that zsh executes. There is an intermediate step that seems redundant; starship prints a script for zsh to execute, and this script prints another script for zsh to execute.

This patch skips the intermediate execution and prints the final script in starship init. This is backwards compatible and does not require any changes in the installation instructions, so it could be release without a major version bump.

Note that it would still be possible to update the installation instructions to source <(starship init zsh); this patch works with both source and eval. Picking the most performant one is beyond this scope of this patch.

See: #2637

How Has This Been Tested?

  • I have tested using MacOS
  • I have tested using Linux
  • I have tested using Windows

Checklist:

  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.

The installation instructions indicate that one should add this snippet
to zsh's configuration:

    eval "$(starship init zsh)"

The command `starship init zsh` prints a little shell script for zsh to
execute:

    > starship init zsh
    source <(/usr/bin/starship init zsh --print-full-init)%

Running `starship init zsh --print-full-init` prints yet another script
that zsh executes. There is an intermediate step that seems redundant;
starship prints a script for zsh to execute, and this script prints
another script for zsh to execute.

This commit skips the intermediate execution and prints the final script
in `starship init`. This is backwards compatible and does not require
any changes in the installation instructions, so it could be release
without a major version bump.

Note that it would still be possible to update the installation
instructions to `source <(starship init zsh)`; this patch works with
both `source` and `eval`. Picking the most performant one is beyond this
scope of this commit.

See: starship#2637
@davidkna
Copy link
Member

@chipbuster When this was added in #168, was there a specific reasoning behind using a different eval-approach for the inner zsh init?

@chipbuster
Copy link
Contributor

Sorry for the delay.

There were two justifications for the original two-phase init:

  1. Some shells did not deal well with reading a script directly from stdin, and you had to write the scripts assuming all the newlines would be stripped out. I believe this primarily applied to bash.
  2. At the time, we were trying to maintain a common init line across all shells.

(2) is now completely pointless since we've added too many shells for that to be feasible, so that is no longer a concern.

However, I do have one reservation about merging this patch: this is not quite 100% backwards compatible in the sense that an incorrect initialization line can cause the init to fail. Specifically, if you have eval $(starship init zsh) in your init file (no quotes), the current init works just fine, while the new init will cause the init to silently fail because the entire initscript is treated as a single line that happens to start with a comment.

If we are okay with breaking initialization for users who may have a slightly malformed init line, I'm happy to approve of this. I've tested it against Zsh 5.0 (which is the oldest version that we officially support ever since we switched to add-zsh-hook based inits), as well as 5.4 and 5.9, and outside of that one quirk, everything here looks great.

@chipbuster
Copy link
Contributor

Okay, I just did some searching in the repo and it looks like we have never actually recommended quote-less init for zsh. Unclear if I just screwed up or changed something for testing in my starship config and never changed it back.

Given that the quoteless init is basically user error, I'm happy to approve this as-is.

@davidkna davidkna changed the title Skip unnecessary indirection in starship init zsh perf: Skip unnecessary indirection in starship init zsh Aug 31, 2023
@davidkna davidkna merged commit 5ca8daa into starship:master Aug 31, 2023
16 of 17 checks passed
@davidkna
Copy link
Member

Thanks for contributing @WhyNotHugo, and thanks for reviewing this @chipbuster!

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

Successfully merging this pull request may close these issues.

None yet

3 participants