openstenoproject / plover

Open source stenotype engine
http://opensteno.org/plover
GNU General Public License v2.0
2.31k stars 281 forks source link

Clobbers symlinked config file #1628

Open gustaphe opened 11 months ago

gustaphe commented 11 months ago

Describe the bug

If ~/.config/plover/plover.cfg is a symlink to somewhere else, config changes made in Plover delete the symlink and create a new ~/.config/plover/plover.cfg.

To Reproduce

Steps to reproduce the behavior:

  1. $ mv ~/.config/plover/plover.cfg ~/.dotfiles/plover.cfg
  2. $ ln -s ~/.dotfiles/plover.cfg ~/.config/plover/plover.cfg
  3. Open Plover, edit config
  4. diff ~/.dotfiles/plover.cfg ~/.config/plover/plover.cfg
  5. There's a difference between the two files, ~/.config/plover/plover.cfg is no longer a symlink, and ~/.dotfiles/plover.cfg does not have the latest edits

Expected behavior

I expect Plover to follow the symlink and update the indicated config file, rather than create a new one. My guess is the suggested solution in #823 would fix this too.

Operating system

user202729 commented 11 months ago

Not really, the cause is different. Plover writes to a temporary configuration file, and if that is successful, it atomically (I assume?) moves the new configuration file to the original file.

https://github.com/openstenoproject/plover/blob/e6516275ca67105639537b7089913a893a2a495b/plover/resource.py#L41

Maybe we can do something like this.

--- a/plover/resource.py
+++ b/plover/resource.py
@@ -35,7 +35,7 @@ def resource_timestamp(resource_name):
 def resource_update(resource_name):
     if resource_name.startswith(ASSET_SCHEME):
         raise ValueError(f'updating an asset is unsupported: {resource_name}')
-    filename = resource_filename(resource_name)
+    filename = os.path.realpath(resource_filename(resource_name))
     directory = os.path.dirname(filename)
     extension = os.path.splitext(filename)[1]
     tempfile = NamedTemporaryFile(delete=False, dir=directory,

It's important that the temporary file is on the same file system as the target file to be moved to (TODO check)

If the user intentionally create a symbolic link, this seems to be more desirable. However, if in some case a symbolic link is unintentionally created, problem happens.

gustaphe commented 11 months ago

What problems would an unintentional symlink cause?

Fwiw, for my usecase a $PLOVERCONFIG environment variable would solve the same underlying problem.