microsoft / terminal

The new Windows Terminal and the original Windows console host, all in the same place!

MIT License

95.74k stars 8.33k forks source link

Duplicate WT_SESSION and WT_PROFILE_ID in WSLENV env var #7130

Open abarani opened 4 years ago

abarani commented 4 years ago

Environment

Windows build number: 10.0.19041.388
Windows Terminal version (if applicable): 1.1.2021.0

Steps to reproduce

Open Windows Terminal
setx WSLENV "${env:WSLENV}:TEST/p"
Close and reopen Windows Terminal
echo $env:WSLENV

Expected behavior

WT_SESSION:WT_PROFILE_ID:TEST/p

Actual behavior

WT_SESSION:WT_SESSION::WT_PROFILE_ID:TEST/p:WT_PROFILE_ID

I think the right thing to do here is to split WSLENV by : and check those vars are already set instead of just simply prepending and appending a string.

Probable related PRs:

DHowett commented 4 years ago

Good catch! There's an inherent danger in using setx to immortalize the contents of an environment variable at runtime, because even TEST/p is going to get duplicated if you keep doing that ...

I'm not sure it harms anything other than aesthetics, but I'll tag this one up as a code health issue.

huiyooumich commented 2 years ago

Hello, this is my first time making a contribution here! I would like to work on this.

zadjii-msft commented 2 years ago

@huiyooumich awesome, go for it! I don't think you'd be stepping on anyone's toes here. I'm pretty sure ConptyConnection::_LaunchAttachedClient in ConptyConnection.cpp is where we're setting up the environment variables. Let us know if you need any pointers ☺️

huiyooumich commented 2 years ago

Unfortunately, I have stopped working on this due to lack of time. But thank you so much for the advice and I hope if anyone else wants to give this a go they'll find it a lot easier.