[PATCH] D121423: [VFS] OverlayFileSystem should consistently use the last-added FS first

Tue Mar 15 13:20:07 PDT 2022

bnbarham added a comment.

In D121423#3383595 <https://reviews.llvm.org/D121423#3383595>, @dexonsmith wrote:

> Do you really mean real path here (system call to `realpath`, that sees through symlinks), or just absolute path?
>
> Is it only when `RealFileSystem` has a CWD? If `RealFileSystem` has a CWD, then in practice it needs to use absolute paths for calls to `sys::fs` since the process CWD cannot be trusted. I suspect it's just failing to model the working directory for the paths it returns (which is sometimes hard, in the presence of symlinks). Maybe that's fine for now.
>
> Is this behaviour (overlayfilesystem's behaviour when wrapping a RealFileSystem) a regression/change or is it the same as before?

I mean `realpath`, see https://llvm.org/doxygen/namespacellvm_1_1sys_1_1fs.html#a3e2cd27a3a2e6c44e9c0fc022952ea54, which is what `RealFile::RealName` gets set to. Honestly this seems a bit odd to me, there was a commit back in 2018 that renamed `getName` to `getRealName` but it ended up being reverted because tests failed and never re-added. I certainly wouldn't expect `File::getName` to return a realpath (and then only sometimes). Personally I'd like to remove the realpath behavior, or actually rename it to `getRealName` and make sure all filesystems implement... but I think that's a rabbit hole for a different day.

Anyway, it would be a regression, though one that's easy to fix - just don't set `RealName` in `RealFile::setPath`. This is what I've done (I'll put up new changes after lunch). I was more pointing out the `RedirectingFileSystem` behavior in comparison (which would still be the same since it returns a `FileWithFixedStatus` that just uses `status()->getName()`).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121423/new/

https://reviews.llvm.org/D121423