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

Deduplicate $fish_user_paths automatically #8117

Merged
merged 1 commit into from
Jul 14, 2021

Conversation

faho
Copy link
Member

@faho faho commented Jul 5, 2021

In the variable handler, we just go through the entire thing and keep
every element once.

If there's a duplicate, we set it again, which calls the handler
again.

This takes a bit of time, to be paid on each startup. On my system,
with 100 already deduplicated elements, that's about 4ms (compared to
~17ms for adding them to $PATH).

It's also semantically more complicated - now this variable
specifically is deduplicated? Do we just want "unique" variables that
can't have duplicates?

However: This entirely removes the pathological case of appending to
$fish_user_paths in config.fish (which should be an FAQ entry!), and the implementation is quite simple.

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

@faho faho added this to the fish 3.4.0 milestone Jul 5, 2021
@krobelus
Copy link
Member

Looks good, this will get rid of the scenario where users add to a universal $fish_user_paths on each startup.
An alternative would be to warn when duplicates were added during startup, but that seems harder to get right.

@faho
Copy link
Member Author

faho commented Jul 11, 2021

An alternative would be to warn when duplicates were added during startup, but that seems harder to get right.

Yeah, we could warn whenever a duplicate is added, but that's probably too annoying?

Or do naughty things with checking the stacktrace and only warn if it's still in config.fish?

I don't really want this to be something you count on, I'd just like to fix it for the simple case. So I wouldn't even document this, in case we ever decide to remove or replace it.

@IlanCosman
Copy link
Contributor

IlanCosman commented Jul 12, 2021

Doesn't this sort of overlap with fish_add_path? As you said, this problem is worthy of FAQ status, and the answer would be fish_add_path. As long as we advertise fish_add_path in all of the relevant places in the docs, this question should basically go away.

@faho
Copy link
Member Author

faho commented Jul 12, 2021

Doesn't this sort of overlap with fish_add_path?

Well, yes, obviously. But the point is that, instead of constantly telling people not to do the wrong thing, we can just make the wrong thing not broken.

If you understand universal variables it's obvious that appending to one in config.fish is wrong, but enough people don't understand universal variables (which is also a reason for #7379).

I'm tired of diagnosing "my shell has gotten slow" problems that half the time come down to this (and most of the other half is some plugin that we have no control over).

As long as we advertise fish_add_path in all of the relevant places in the docs, this question should basically go away.

A lot of people don't read the docs. I don't know why, maybe they assumed they don't need to, or they assume the docs are bad.

@ridiculousfish
Copy link
Member

This looks good to me too. I think this simple targeted fix is right.

In the variable handler, we just go through the entire thing and keep
every element once.

If there's a duplicate, we set it again, which calls the handler
again.

This takes a bit of time, to be paid on each startup. On my system,
with 100 already deduplicated elements, that's about 4ms (compared to
~17ms for adding them to $PATH).

It's also semantically more complicated - now this variable
specifically is deduplicated? Do we just want "unique" variables that
can't have duplicates?

However: This entirely removes the pathological case of appending to
$fish_user_paths in config.fish (which should be an FAQ entry!), and the implementation is quite simple.
@faho
Copy link
Member Author

faho commented Jul 14, 2021

Okay, I added some tests, merging.

@faho faho merged commit e013422 into fish-shell:master Jul 14, 2021
@faho faho deleted the user-paths-dedup branch July 14, 2021 14:42
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 18, 2022
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.

4 participants