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.

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.

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.

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

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.

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:

In history.cpp, we have the following.

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.

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.

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.

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)

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.

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.

Merged, thanks!

Thank you!