LLFile::rename() and LLFile::remove() are not Posix compliant under Windows

[fmartian]

Environment

Current develop branch HEAD

Description

LLFile::rename() fails under Windows when the new_name already exists while the Posix standard for rename() says that it should simply create the new file anyhow, such that at no point there does not exist any new_name for other processes.
This last one may be a bit hard to guarantee under Windows without a lot of testing and debugging and low level file tinkering, but making it not fail when the new_name already exists is fairly trivial by calling directly MoveFileExW() with the according flags rather than the cludgy Windows C runtime replacement _wrename(), which does actually nothing else than calling that function too, but without the according flag. This results in quite some places of the viewer source code in conditional code on LL_WINDOWS that removes the new_name before calling LLFile::rename(). That seems very unnecessary.

Similarly while Posix remove() is documented to work on both files and directories, LLFile::remove() on Windows will only work on files since the used _wremove() does simply call DeleteFileW(), which can't delete empty directories. While this seems to be not something that has been worked around in code to much, it still is an inconsistency.

There are two possible fixes to LLFile::remove():
1: adopt Posix behaviour and find out if the path is for a directory and then call _wrmdir() and otherwise call _wremove()
2: or make LLFile::remove() deviate from the Posix way and change the call in the non-Windows portion to unlink() to only work on files.

I personally tend to favor the first option, not at least because of the comment in the beginning of llfile.cpp about this trying to follow the Posix standard.

Reproduction steps

look at llfile.cpp source

---

This repo is using Opire - what does it mean? 👇

💵 Everyone can add rewards for this issue commenting /reward 100 (replace 100 with the amount).
🕵️‍♂️ If someone starts working on this issue to earn the rewards, they can comment /try to let everyone know!
🙌 And when they open the PR, they can comment /claim #4805 either in the PR description or in a PR's comment.

🪙 Also, everyone can tip any user commenting /tip 20 @fmartian (replace 20 with the amount, and @fmartian with the user to tip).

📖 If you want to learn more, check out our documentation.

[Dan-Linden]

What does this break? What are you trying to do when you run into this issue?

[fmartian]

I noticed that there are several instances where there was an LLFile::remove() (and in one case an LLAPRFile::remove()) before an LLFile::rename() (usually only for LL_WINDOWS) and investigated that. Seems that the Microsoft C runtime file functions, while claiming to follow Posix more or less, aren't quite as compatible. Rather than working around such inconsistencies in various places throughout the code, I think it would be cleaner to actually fix the LLFile functions instead. For rename() that is actually almost trivial since the underlaying MoveFileEx() function already provides this functionality, It's just not used in the Windows rename() and _wrename() C runtime functions, likely for backwards compatibility reason.

So I reported this here and worked on a patch to actually improve LLFile to be more consistent. But the branch I worked on had other unrelated changes in the branch and I only noticed after having submitted the PR. Which is why I closed it right away again. I'm still working on this and plan to submit a clean patch that would close this issue.

That patch might contain controversial things, but I figured submitting it and see if it could be accepted won't hurt anyone.