Rewrite the real file if history file is a symlink (#7754)

* Rewrite the real file if history file is a symlink

When the history file is a symbolic link, `fish` used to overwrite
the link with a real file whenever it saved history. This makes
it follow the symlink and overwrite the real file instead.

The same issue was fixed for the `fish_variables` file in 622f2868e
from https://github.com/fish-shell/fish-shell/pull/7728.
This makes `fish_history` behave in the same way. The implementation
is nearly identical.

Since the tests for the two issues are so similar, I combined them
together and slightly expanded the older test.

This also addresses https://github.com/fish-shell/fish-shell/issues/7553.

* Add user-facing error when history renaming fails

Currently, when history file renaming fails, no message is shown to the
user. This happens, for instance, if the history file is a symlink
pointing to another filesystem.

This copies code (with a bit of variation, after reviewer comments) from

589eb34571/src/env_universal_common.cpp (L486-L491)

into `history.cpp`, so that a message is shown to the user.

* fixup! Rewrite the real file if history file is a symlink

CHANGELOG.rst

@@ -14,7 +14,9 @@ Scripting improvements
 ----------------------
 
 Interactive improvements
-------------------------
+-------------------------
+- The history file can be made a symbolic link without it being overwritten.
+
 
 New or improved bindings
 ^^^^^^^^^^^^^^^^^^^^^^^^

src/history.cpp

@@ -774,12 +774,20 @@ bool history_impl_t::save_internal_via_rewrite() {
     // We want to rewrite the file, while holding the lock for as briefly as possible
     // To do this, we speculatively write a file, and then lock and see if our original file changed
     // Repeat until we succeed or give up
-    const maybe_t<wcstring> target_name = history_filename(name);
+    const maybe_t<wcstring> possibly_indirect_target_name = history_filename(name);
     const maybe_t<wcstring> tmp_name_template = history_filename(name, L".XXXXXX");
-    if (!target_name.has_value() || !tmp_name_template.has_value()) {
+    if (!possibly_indirect_target_name.has_value() || !tmp_name_template.has_value()) {
         return false;
     }
 
+    // If the history file is a symlink, we want to rewrite the real file so long as we can find it.
+    wcstring target_name;
+    if (auto maybe_real_path = wrealpath(*possibly_indirect_target_name)) {
+        target_name = *maybe_real_path;
+    } else {
+        target_name = *possibly_indirect_target_name;
+    }
+
     // Make our temporary file
     // Remember that we have to close this fd!
     wcstring tmp_name;
@@ -792,7 +800,7 @@ bool history_impl_t::save_internal_via_rewrite() {
     for (int i = 0; i < max_save_tries && !done; i++) {
         // Open any target file, but do not lock it right away
         autoclose_fd_t target_fd_before{
-            wopen_cloexec(*target_name, O_RDONLY | O_CREAT, history_file_mode)};
+            wopen_cloexec(target_name, O_RDONLY | O_CREAT, history_file_mode)};
         file_id_t orig_file_id = file_id_for_fd(target_fd_before.fd());  // possibly invalid
         bool wrote = this->rewrite_to_temporary_file(target_fd_before.fd(), tmp_fd);
         target_fd_before.close();
@@ -805,14 +813,14 @@ bool history_impl_t::save_internal_via_rewrite() {
         // were rewriting it. Make an effort to take the lock before checking, to avoid racing.
         // If the open fails, then proceed; this may be because there is no current history
         file_id_t new_file_id = kInvalidFileID;
-        autoclose_fd_t target_fd_after{wopen_cloexec(*target_name, O_RDONLY)};
+        autoclose_fd_t target_fd_after{wopen_cloexec(target_name, O_RDONLY)};
         if (target_fd_after.valid()) {
             // critical to take the lock before checking file IDs,
             // and hold it until after we are done replacing
             // Also critical to check the file at the path, NOT based on our fd
             // It's only OK to replace the file while holding the lock
             history_file_lock(target_fd_after.fd(), LOCK_EX);
-            new_file_id = file_id_for_path(*target_name);
+            new_file_id = file_id_for_path(target_name);
         }
         bool can_replace_file = (new_file_id == orig_file_id || new_file_id == kInvalidFileID);
         if (!can_replace_file) {
@@ -841,8 +849,9 @@ bool history_impl_t::save_internal_via_rewrite() {
             }
 
             // Slide it into place
-            if (wrename(tmp_name, *target_name) == -1) {
+            if (wrename(tmp_name, target_name) == -1) {
-                FLOGF(history_file, L"Error %d when renaming history file", errno);
+                const char *error = std::strerror(errno);
+                FLOGF(error, _(L"Error when renaming history file: %s"), error);
             }
 
             // We did it

tests/checks/symlink-universal-variables.fish

@@ -1,18 +0,0 @@
-#RUN: %fish -C 'set -g fish %fish' %s
-begin
-    set -gx XDG_CONFIG_HOME (mktemp -d)
-    mkdir -p $XDG_CONFIG_HOME/fish
-    set -l target_file $XDG_CONFIG_HOME/fish/target_fish_variables
-    set -l fish_variables $XDG_CONFIG_HOME/fish/fish_variables
-
-    echo > $target_file
-    ln -sf $target_file $fish_variables
-    $fish -c 'set -U variable value'
-
-    if test -L $fish_variables
-       echo fish_variables is still a symlink
-    else
-       echo fish_variables has been overwritten
-    end
-    # CHECK: fish_variables is still a symlink
-end

tests/checks/symlinks-not-overwritten.fish

@@ -0,0 +1,49 @@
+#RUN: %fish -C 'set -g fish %fish' %s
+
+set -gx XDG_CONFIG_HOME (mktemp -d)
+set -gx XDG_DATA_HOME $XDG_CONFIG_HOME
+mkdir -p $XDG_CONFIG_HOME/fish
+
+
+# fish_variables
+set -l target_file $XDG_CONFIG_HOME/fish/target_fish_variables
+set -l fish_variables $XDG_CONFIG_HOME/fish/fish_variables
+set -l backup_file $XDG_CONFIG_HOME/fish/fish_variables_backup
+
+echo >$target_file
+cp $target_file $backup_file
+ln -sf $target_file $fish_variables
+$fish -c 'set -U variable value'
+
+if not test -L $fish_variables
+    echo fish_variables has been overwritten
+else if cmp $target_file $backup_file >/dev/null
+    echo fish_variables was never written
+else
+    echo fish_variables is still a symlink
+end
+# CHECK: fish_variables is still a symlink
+
+
+# fish_history
+set -l history_file $XDG_DATA_HOME/fish/fish_history
+set -l target_file $XDG_DATA_HOME/fish/target_fish_history
+set -l backup_file $XDG_DATA_HOME/fish/fish_history_backup
+
+echo '- cmd: echo I will be deleted from history
+        when: 1614577746' >$target_file
+cp $target_file $backup_file
+ln -sf $target_file $history_file
+# The one way to ensure non-interactive fish writes to the history file
+$fish -c 'echo all | history delete deleted | grep echo'
+# CHECK: [1] echo I will be deleted from history
+
+if not test -L $history_file
+    echo fish_history has been overwritten
+else if cmp $target_file $backup_file &>/dev/null
+    # cmp writes to stderr when one file is empty, thus &> above
+    echo fish_history was never written
+else
+    echo fish_history is still a symlink
+end
+# CHECK: fish_history is still a symlink