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

set --show: Show the originally inherited value, if any #9029

Merged
merged 1 commit into from
Jun 27, 2022

Conversation

faho
Copy link
Member

@faho faho commented Jun 21, 2022

This adds a line to set --shows output like

$PATH: originally inherited as |/home/alfa/.local/bin:/usr/local/sbin:/usr/local/bin:/usr/bin:/usr/bin/site_perl:/usr/bin/vendor_perl:/usr/bin/core_perl:/var/lib/flatpak/exports/bin|

to help with debugging.

Note that this means keeping an additional copy of the original
environment around. At most this would be one ARG_MAX's worth, which
is typically about 2M on linux and typically less elsewhere.

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

This is a quick implementation that might not be ideal, I'm not sure if there isn't some better way to implement it, or if storing the entire environment is seen as too wasteful - an alternative would be to just store the variable names so we can say "was originally inherited" but not with what value.

This adds a line to `set --show`s output like

```
$PATH: originally inherited as |/home/alfa/.local/bin:/usr/local/sbin:/usr/local/bin:/usr/bin:/usr/bin/site_perl:/usr/bin/vendor_perl:/usr/bin/core_perl:/var/lib/flatpak/exports/bin|
```

to help with debugging.

Note that this means keeping an additional copy of the original
environment around. At most this would be one ARG_MAX's worth, which
is about 2M.
@faho faho added this to the fish 3.6.0 milestone Jun 21, 2022
@faho
Copy link
Member Author

faho commented Jun 22, 2022

Note: On my system, the environment is typically under 2K - 1.8K for my normal user, 0.5K for root. On linux, you can see it with wc -c /proc/$fish_pid/environ

@ridiculousfish
Copy link
Member

Nice fix, LGTM. I agree that the imported environment variables are not a major concern with respect to memory usage.

@ridiculousfish ridiculousfish self-requested a review June 23, 2022 03:08
@IlanCosman
Copy link
Contributor

IlanCosman commented Jun 23, 2022

IMO this feels wasteful in complexity and space for the value it provides since set --show is only rarely used. I guess this does help a bit with debugging the messing-with-path problem so it does provide some value 🤷‍♂️ Idk it just doesn't "feel" right, but that's not worth much, especially if y'all think it's a good idea 🙂

@faho
Copy link
Member Author

faho commented Jun 23, 2022

It is, indeed, a debug aid.

We have a lot of people coming in with "where does my $PATH come from? what's up with $LC_ALL?", and if we can tell them to just run set --show PATH, and that will explain whether it got the values from the parent or not, we can then tell them whether it's in fish's config or elsewhere on the system much quicker.

@faho faho merged commit dde2d33 into fish-shell:master Jun 27, 2022
@faho faho deleted the inherited-vars branch December 2, 2022 14:20
@faho faho mentioned this pull request Aug 19, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants