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

Expose shell's environment - zsh #237977

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Expose shell's environment - zsh #237977

wants to merge 13 commits into from

Conversation

anthonykim1
Copy link
Contributor

Trying to break #228791 down smaller so that it is easier to review.

Part of #227467

@anthonykim1 anthonykim1 self-assigned this Jan 15, 2025
@anthonykim1 anthonykim1 added this to the January 2025 milestone Jan 15, 2025
@anthonykim1 anthonykim1 requested a review from Tyriar January 16, 2025 14:51
@anthonykim1
Copy link
Contributor Author

Going through more rigorous testing, I realized sometimes (although very not expected), things like -q show up as an environment variable's value. This of course throws error. I've aimed to better handle it now, soo it skips that variable entirely.

Also since hypens shouldn't really be part of variable name, we would skip the 'sending' of the variable if we detect usage of - in an environment variable name.

Screenshot 2025-01-17 at 1 14 56 AM

@anthonykim1 anthonykim1 marked this pull request as ready for review January 17, 2025 06:47
@anthonykim1 anthonykim1 requested a review from Tyriar January 17, 2025 06:47
@anthonykim1 anthonykim1 marked this pull request as draft January 17, 2025 06:47
@anthonykim1 anthonykim1 marked this pull request as ready for review January 17, 2025 06:49
Comment on lines 122 to 128
value=$(builtin printf '%s' "${(P)var}")
# It is not valid to have hypen in variable name
# It is not valid to have hypen to start variable value
if [[ "$value" == -* || "$var" == *-* ]]; then
continue
fi
builtin printf '\e]633;EnvSingleEntry;%s;%s;%s\a' "$var" "$(__vsc_escape_value "$value")" $__vsc_nonce
Copy link
Member

Choose a reason for hiding this comment

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

Going through more rigorous testing, I realized sometimes (although very not expected), things like -q show up as an environment variable's value. This of course throws error. I've aimed to better handle it now, soo it skips that variable entirely.

We want to make sure this works, it sounds like we're passing the string in as a space separated list of args, rather than a single string containing spaces. Try tweaking how $value is passed to that function.

Also since hypens shouldn't really be part of variable name, we would skip the 'sending' of the variable if we detect usage of - in an environment variable name.

I don't think they're valid, but we also want to not special case this behavior either as well as it could hide obscure bugs.

Copy link
Contributor Author

@anthonykim1 anthonykim1 Jan 17, 2025

Choose a reason for hiding this comment

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

I agree, I want to avoid putting special case in update_env function and better handle this.
would something like this be better? - Adding a elif in escape_method so that I would escape it in hex similar to already existing backslashes, semi-colons, newlines.

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.

2 participants