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

[Ben Barham via Phabricator via llvm-commits]

bnbarham added a comment.

In D121423#3380920 <https://reviews.llvm.org/D121423#3380920>, @bnbarham wrote:

> If we could resolve the path in `OverlayFileSystem` then we could just do a `status` + check that it's a directory on each FS and if so, set CWD in `OverlayFileSystem` and not any underlying FS.
>
> Maybe we should just do that though with one exception - if the returned path is not the one we gave then *don't* replace the path...? External names is a bit of a hack anyway, so hack for a hack 😆?

I went ahead and implemented this. The main problem I can see is that the `File` returned by `RealFileSystem::openFileForRead` looks like it's supposed to give the *real path* for `File::getName` (though this seems to be the case for only `RealFile`). `setPath` currently replaces that as well, which would result in different behaviour for a `OverlayFileSystem` containing a `RealFileSystem` than just using `RealFileSystem` itself:

  OverlayFileSystem
    RealFileSystem
      /a/b
      /f
  
  cd /a/b
  cat ../../f # File::getName + Status::getName == ../../f
  
  RealFileSystem
    /a/b
    /f
  
  cd /a/b
  cat ../../f # File::getName == /a/b/f, Status::getName == ../../f

And this is even more of a problem when `RedirectingFileSystem` with `use-external-names: false`:

  OverlayFileSystem
    RedirectingFileSystem(useExternalNames: false)
      /f2 -> f
    RealFileSystem
      /a/b
      /f
  
  cd /a/b
  cat ../../f2

A `FileWithFixedStatus` is returned that would have `/a/b/../../f2` for `File::getName` and `Status::getName`. Then `OverlayFileSystem` would `setPath("../../f2")` and thus both would now return `../../f2`. I'm not sure what we actually *want* to return here though. Maybe that's fine?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121423