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

builtins set doesn't follow symlinks when saving variables #7466

Closed
etu opened this issue Nov 8, 2020 · 8 comments · Fixed by #7728
Closed

builtins set doesn't follow symlinks when saving variables #7466

etu opened this issue Nov 8, 2020 · 8 comments · Fixed by #7728
Milestone

Comments

@etu
Copy link
Contributor

etu commented Nov 8, 2020

So when using set --universal name value it saves to .config/fish/fish_variables. If that file is a symlink it removes the symlink and just stores as a new file instead.

This for me is a problem because both my root partition and my home partition is on a tmpfs filesystem, so when this happens the change isn't persistent to any filesystem and gets lost on next reboot.

I've lost things more than once due to this, such as abbreviations that I've saved.

Recording: https://asciinema.org/a/Y3QcT1kfZcVC8tIv5I5quHy4p

System info:

$ fish --version
fish, version 3.1.2

$ echo $version
3.1.2

$ uname -a
Linux agrajag 5.9.2 #1-NixOS SMP Thu Oct 29 09:12:22 UTC 2020 x86_64 GNU/Linux

$ echo $TERM
xterm-256color     # (but it's really kitty)

I tried to run with a separate home directory:

$ sh -c 'env HOME=$(mktemp -d) fish'

And could perform the same actions to get the same problem.

More about my setup: https://elis.nu/blog/2020/05/nixos-tmpfs-as-root/ https://elis.nu/blog/2020/06/nixos-tmpfs-as-home/

@faho
Copy link
Member

faho commented Nov 8, 2020

You just have to symlink the directory, not the file.

We unlink it so we get atomicity, which simplifies things quite a bit.

@etu
Copy link
Contributor Author

etu commented Nov 8, 2020

Hmm, it's actually not that simple for me to change that... Because the symlinks go to different places. Even though it looks like they go to the sample place, that place just contains symlinks. The symlink for config.fish and functions goes to a read only place while the symlink for fish_variables goes to another symlink that points at a filesystem which is readwrite.

Wouldn't it be possible to resolve the symlinks first, then unlink and replace the file at it's storage location?

@ridiculousfish
Copy link
Member

Probably doable in principle, not trivial though.

@ridiculousfish ridiculousfish added this to the fish-future milestone Nov 9, 2020
@mqudsi
Copy link
Contributor

mqudsi commented Nov 12, 2020

The easiest approach is probably to not unlink the file at all, and just truncate + write instead?

@krobelus
Copy link
Member

It needs to be atomic, so other fish always read a consistent state. Though that should already be guaranteed because we are locking the file.

@zanchey
Copy link
Member

zanchey commented Nov 18, 2020

Locking is too unreliable over NFS. (http://0pointer.de/blog/projects/locking.html, http://0pointer.de/blog/projects/locking2)

@stouset
Copy link

stouset commented Dec 30, 2020

Would the project be willing to accept a patch that checks if ${XDG_CONFIG_HOME}/fish/fish_variables is a symlink, and—if so—performs the same atomic write but to the symlinked location instead?

That would still provide the required atomicity in updates.

The fundamental problem I'm running into is that I symlink configuration into my home directory from a git repo, but I do it file-by-file (using stow --no-folding) for a reasons that aren't particularly important to this discussion other than to say that symlinking parent directories directly would be a headache. fish overwriting ~/.config/fish/fish_variables prevents changes from getting picked up by my git-managed profile.

I considered trying to set XDG_CONFIG_HOME just for the fish process (without exporting the variable), but by the time fish reads this configuration from any user-controlled configuration file, it's already cached the configuration directory in a static variable internally so changes aren't reflected. Of course, this leads to another possible patch: updates to XDG_CONFIG_HOME should perhaps update the static storage used by get_config_directory. I'm open to either, or alternative suggestions.

@krobelus
Copy link
Member

Would the project be willing to accept a patch that checks if ${XDG_CONFIG_HOME}/fish/fish_variables is a symlink, and—if so—performs the same atomic write but to the symlinked location instead?

I think so; that's also what @etu suggested. I don't see an obvious problem with this solution.

ridiculousfish pushed a commit that referenced this issue Feb 20, 2021
Previously, `set -U` would overwrite the symlink with a
regular file.

Fixes #7466
@zanchey zanchey modified the milestones: fish-future, fish 3.2.0 Feb 21, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants