Fish history file path detection is broken for path prefixed with ~

[Blizarre]

Version: 3.1.0
OS: Ubuntu focal
Terminal: st

Thank you for this project, I have been using fish for years and I love it. I looked at the issues but could not find one like this.

How to reproduce

When I looked at my ~/.local/share/fish/fish_history file, I found that files starting with ~ weren't detected properly as paths:

$ md5sum fish.spec.in ~/test.sh /tmp/fish.desktop
5edd6151e8eee9c252fe08e77775d993  fish.spec.in
48a61d611ce2b7bce16a49119909e625  /home/simon/test.sh
4a104f9e1d24ad65fb4aca68aa4d96b8  /tmp/fish.desktop
$ tail -n 7 ~/.local/share/fish/fish_history
- cmd: md5sum fish.spec.in ~/test.sh /tmp/fish.desktop
  when: 1609108437
  paths:
    - fish.spec.in
    - /tmp/fish.desktop
- cmd: tail -n 7 ~/.local/share/fish/fish_history
  when: 1609108439

Expectations

- cmd: md5sum fish.spec.in ~/test.sh /tmp/fish.desktop
  when: 1609108437
  paths:
    - fish.spec.in
    - ~/test.sh
    - /tmp/fish.desktop
- cmd: tail -n 7 ~/.local/share/fish/fish_history
  when: 1609108439
  paths:
    - ~/.local/share/fish/fish_history

Comments

I do not know if it is really an issue or not, but it explain a few questions that I had in the past where I noticed that the autocomplete was inconsistent, and couldn't really understand why.

[Blizarre]

After looking at the code, it seems that the function that decide if a path should be added is path_is_valid, and that it is not performing tilde expansion.

I spend a bit of time trying to add expand_tilde to path_is_valid. expand_tilde requires access to the environment variables to extract $HOME (environment_t).

I tried to add environment_t vars to path_is_valid and its callers, but I get stuck here because environment_t is virtual and as such cannot be copied.

I see 3 options:

1. pass the only variable that is used for the expansion (HOME) instead. It should be able to be copied. However it should be a maybe<cwstring> or a maybe_t<env_var_t>, which is not very clean, or a naked cwstring but what would be the value if $HOME doesn't exist?
2. Not use any environment variable and use getpwnam_r just like for the other users (a bit slow? need an extra library call to get the username)
3. Use some c++ magic to allow the vars to be passed there

[krobelus]

Hi, thanks for the detailed write-up.

This issue also applies to paths with variables, like in tail $HOME/.local/share/fish/fish_history.

but what would be the value if $HOME doesn't exist?

It seems that fish sets HOME if it doesn't exist - of course that doesn't solve the problem for other variables.

Use some c++ magic to allow the vars to be passed there

Right, in history_t::add_pending_with_file_detection we use iothread_perform([=]() {}), so we capture everything by copy. Copying data is a good idea in general for multithreaded code. In theory someone could modify variables while this is running.

I'm not yet sure what is the best solution. We could copy relevant variables. However, it's probably simpler to capture a reference to the environment, and use our static std::mutex env_lock; to ensure consistency.

[Blizarre]

This issue also applies to paths with variables, like in tail $HOME/.local/share/fish/fish_history.

Ok, I'll try to have a look at passing by reference + use a mutex. in the end it might enable arbitrary variable expansion, which should be even more consistent.

If it doesn't work there is already a special getter for PWD get_pwd_slash that returns a cwstring (and not a maybe<cwstring>), so we could add one for HOME and stay consistent with the existing interfaces.

I'll experiment a bit if that's alright.

[krobelus]

Sounds good - probably you can pass an entire operation_context_tas returned by get_bg_context to add_pending_with_file_detection(). It is already used for highlighting and and autosuggestions; the file detection while writing history is a similar background task.

[Blizarre]

I have pushed a first version as a PR, it might be better to continue the discussion about the implementation there.