From 762f3aa0ce7cffeabd9830a11a92985b2c3fc702 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Mon, 8 Mar 2021 08:46:17 -0800 Subject: [PATCH] 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 https://github.com/fish-shell/fish-shell/blob/589eb34571e00da530395776166dd873489d4027/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 | 4 +- src/history.cpp | 23 ++++++--- tests/checks/symlink-universal-variables.fish | 18 ------- tests/checks/symlinks-not-overwritten.fish | 49 +++++++++++++++++++ 4 files changed, 68 insertions(+), 26 deletions(-) delete mode 100644 tests/checks/symlink-universal-variables.fish create mode 100644 tests/checks/symlinks-not-overwritten.fish diff --git a/CHANGELOG.rst b/CHANGELOG.rst index d22b499c4..3c8d17d4b 100644 --- a/CHANGELOG.rst +++ b/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 ^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/src/history.cpp b/src/history.cpp index 2b6ac98a9..868c97557 100644 --- a/src/history.cpp +++ b/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 target_name = history_filename(name); + const maybe_t possibly_indirect_target_name = history_filename(name); const maybe_t 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) { - FLOGF(history_file, L"Error %d when renaming history file", errno); + if (wrename(tmp_name, target_name) == -1) { + const char *error = std::strerror(errno); + FLOGF(error, _(L"Error when renaming history file: %s"), error); } // We did it diff --git a/tests/checks/symlink-universal-variables.fish b/tests/checks/symlink-universal-variables.fish deleted file mode 100644 index d15389741..000000000 --- a/tests/checks/symlink-universal-variables.fish +++ /dev/null @@ -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 diff --git a/tests/checks/symlinks-not-overwritten.fish b/tests/checks/symlinks-not-overwritten.fish new file mode 100644 index 000000000..dd77300da --- /dev/null +++ b/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