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

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

Merged
merged 3 commits into from Mar 8, 2021
Merged

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

merged 3 commits into from Mar 8, 2021

Conversation

ilyagr
Copy link
Contributor

@ilyagr ilyagr commented Mar 1, 2021

This is a followup to #7728 that makes fish_history behave consistently with the new behavior of fish_variables. It should also help with issues like #7553.

I combined the tests for the two features, renamed the test file, and fixed the issue where the old test could pass if fish didn't write anything to the expected location of fish_variables. I couldn't find a good way to have a nice diff for the rename. Let me know if you prefer separate tests.

The checks/git.fish test is currently failing, but I believe that is because of <...>. Edit: that wasn't the reason.

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

@faho
Copy link
Member

faho commented Mar 1, 2021

The checks/git.fish test is currently failing, but I believe that is because of 384975c.

Mind posting your error? It's not failing on CI or for me, and it's not skipped either (that test only runs if git is installed). Not that I think it's related, but it might show differences in our setup.

@faho faho added this to the fish 3.3.0 milestone Mar 1, 2021
@ilyagr
Copy link
Contributor Author

ilyagr commented Mar 1, 2021

Yes, here you go:

Testing file checks/function.fish ... ok (24 ms)
Testing file checks/functions.fish ... ok (20 ms)
Testing file checks/git.fish ... Failure:

  The CHECK on line 29 wants:
    add\tAdd file contents to the index

  which failed to match line stdout:1:
    adda\tAlias for add :/

  Context:
    adda\tAlias for add :/ <= no check matches
    add\tAdd file contents to the index
    foo\tUntracked file
     (newbranch)

  when running command:
    ../test/root/bin/fish -i checks/git.fish
Testing file checks/glob.fish ... ok (52 ms)

It fails on the master branch as well for me. I haven't looked at it in detail, I assumed it's the same problem as https://github.com/fish-shell/fish-shell/runs/1997846755?check_suite_focus=true, but that's probably not the case.

In fact, it's probably because the test is affected by my global .gitconfig. I have an alias called adda (add all), and I think that confuses the test.

@faho
Copy link
Member

faho commented Mar 1, 2021

In fact, it's probably because the test is affected by my global .gitconfig. I have an alias called adda (add all), and I think that confuses the test.

Yeah, that's it. We either need to disable .gitconfig here or at least anchor the match some more (grep '^add\t'). Given that git doesn't allow aliases named like commands that's safe.

Either way, not related.

I assumed it's the same problem as https://github.com/fish-shell/fish-shell/runs/1997846755?check_suite_focus=true, but that's probably not the case.

Nah, that's lsan being lsan - we do some funky business in the tests that regularly confuses it. But given these are tests, the funky business is the point.

@ilyagr
Copy link
Contributor Author

ilyagr commented Mar 1, 2021

Yes, exactly. If you like, here's the same fix you mentioned in pull request form: #7755. It works for me.

@faho
Copy link
Member

faho commented Mar 1, 2021

Alright, at a glance this looks okay but I don't want to add it to 3.2.0 this late (hopefully) in the cycle and risk breaking history.

If we do a 3.2.1 it can go in there.

@ilyagr
Copy link
Contributor Author

ilyagr commented Mar 1, 2021

Thanks for looking at it!

I didn't realize you were going to release 3.2 today. Congratulations! I'm happy for this to get in later.

There's one minor issue I fixed in a new commit. It was a bit confusing. Let me know if you think it's potentially problematic.

I wanted to see what happens when the symlink points to an invalid location or another file-system. In the latter case, the history writing fails, which I think is a good outcome. However, no error is shown to the user, which is less good. It is also a bit surprising, since in the case of fish_variables, very similar code does result in errors shown to the user.

The error for fish_variables is shown here:

int ret = wrename(src, dst);
if (ret != 0) {
const char *error = std::strerror(errno);
FLOGF(error, _(L"Unable to rename file from '%ls' to '%ls': %s"), src.c_str(), dst.c_str(),
error);
}

In history.cpp, we have the following.

fish-shell/src/history.cpp

Lines 852 to 854 in 589eb34

if (wrename(tmp_name, target_name) == -1) {
FLOGF(history_file, L"Error %d when renaming history file", errno);
}

I'm not exactly sure why the former code produces output and the latter does not, but it seems to be intended. At least, the fish_variables version produces output when running tests (modified to point symlinks to bad places). I hope it will be shown in release builds of fish as well.

So, I added a commit that adds the first kind of FLOG to history.cpp.

@faho
Copy link
Member

faho commented Mar 2, 2021

I'm not exactly sure why the former code produces output and the latter does not, but it seems to be intended

The former produces output because it's FLOG category "error", which is enabled by default. (this is the first argument to "FLOGF")

The latter has the category "history_file", which isn't. You'd have to start fish with fish -d history-file to see it.

Making it consistent with the former seems correct. There's a point to be made that they should be "warning" because fish is still mostly functional (but less so without universal variables), but that's a question for another time.

I would just change "history_file" to "error" for now - keep the message because it's more informative, but change the category. Definitely no two FLOGs with different categories.

@faho faho modified the milestones: fish 3.3.0, fish 3.2.1 Mar 2, 2021
@ilyagr
Copy link
Contributor Author

ilyagr commented Mar 2, 2021

Thanks, that sounds good, I'll probably do it tomorrow. I also plan to rebase this branch (thus finding out how pull requests deal with rewriting history) and move the the changelog entry to the other section.

Should I use the more informative message even though it is not localized?

Also, there is an option to move the entire env_universal_t::move_new_vars_file_into_place function to somewhere like wutil.h and use it, especially if we want to use the less informative but localized message. I'm not sure how eager I am to do it.

@faho
Copy link
Member

faho commented Mar 2, 2021

Should I use the more informative message even though it is not localized?

Localize it - i.e. put it through _. (not that fish's translations are currently brilliant, but we should at least give the common error messages a chance)

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 622f286
from #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 #7553.
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.
@ilyagr
Copy link
Contributor Author

ilyagr commented Mar 3, 2021

Thank you very much for your helpful suggestions, @krobelus and @faho! I put them all in, I believe.

@faho: I updated the error message, and even managed to cause it to happen on my machine. It now looks like this: error: Error when renaming history file: Invalid cross-device link.

@ilyagr
Copy link
Contributor Author

ilyagr commented Mar 3, 2021

In case the rebase is annoying, here are the changes since the previous version: b1a203d...ilyagr:tmp. Maybe I'll try a merge and lots of fix-ups next time.

@faho faho merged commit 762f3aa into fish-shell:master Mar 8, 2021
@faho
Copy link
Member

faho commented Mar 8, 2021

Merged, thanks!

@ilyagr
Copy link
Contributor Author

ilyagr commented Mar 8, 2021

Thank you!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants