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

Use global ZDOTDIR environment variable #159783

Merged
merged 2 commits into from Sep 8, 2022

Conversation

pdamianik
Copy link
Contributor

@pdamianik pdamianik commented Sep 1, 2022

Fallback to the users home directory if the variable is not set

Related issue: #159785

@pdamianik pdamianik force-pushed the support-global-zdotdir-env branch 4 times, most recently from 0dc581b to 127caac Compare September 1, 2022 17:47
@pdamianik pdamianik marked this pull request as ready for review September 1, 2022 17:49
@pdamianik pdamianik force-pushed the support-global-zdotdir-env branch 5 times, most recently from d19a85e to 9de6516 Compare September 2, 2022 10:39
@pdamianik pdamianik marked this pull request as draft September 2, 2022 10:40
@pdamianik pdamianik force-pushed the support-global-zdotdir-env branch 2 times, most recently from a0941f7 to d3bdd6f Compare September 2, 2022 10:44
Fallback to the users home directory if the variable is not set
@pdamianik pdamianik marked this pull request as ready for review September 2, 2022 11:10
@meganrogge meganrogge assigned meganrogge and unassigned chrmarti Sep 8, 2022
@meganrogge meganrogge added this to the September 2022 milestone Sep 8, 2022
Comment on lines +5 to 13
if [[ -f $USER_ZDOTDIR/.zshenv ]]; then
VSCODE_ZDOTDIR=$ZDOTDIR
ZDOTDIR=$USER_ZDOTDIR

. $USER_ZDOTDIR/.zshenv

USER_ZDOTDIR=$ZDOTDIR
ZDOTDIR=$VSCODE_ZDOTDIR
else
USER_ZDOTDIR=$HOME
fi
Copy link
Member

Choose a reason for hiding this comment

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

Am I understanding this right in that it fixes the case where $ZDOTDIR (eg. /home/user/myzsh) is set in your profile/login scripts and then have a $ZDOTDIR/.zshenv in that, not in ~/.zshenv?

A comment explaining what's going on here would be good.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Tyriar I believe actually what this solves for is when ~/.zshenv is in a $ZDOTDIR which is not equal to ~ as we previously assumed it to be

Copy link
Contributor

Choose a reason for hiding this comment

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

which is why the user's ZDOTDIR must be captured before these scripts get run

Copy link
Member

Choose a reason for hiding this comment

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

So ZDOTDIR gets set in ~/.profile, or somewhere else I'm not thinking of?

Copy link
Contributor

Choose a reason for hiding this comment

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

set outside of zsh rc files
#159785 (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.

@Tyriar I believe actually what this solves for is when ~/.zshenv is in a $ZDOTDIR which is not equal to ~ as we previously assumed it to be

Exactly

Comment on lines +190 to +191
const userZdotdir = env?.ZDOTDIR ?? os.homedir();
envMixin['USER_ZDOTDIR'] = userZdotdir;
Copy link
Member

Choose a reason for hiding this comment

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

A comment explaining why this is needed would be good

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Lgtm

@meganrogge meganrogge merged commit c192454 into microsoft:main Sep 8, 2022
mjbvz pushed a commit to mjbvz/vscode that referenced this pull request Sep 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants