[PATCH] D121424: [VFS] Revert RedirectingFileSystem to having a single responsibility

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.



> This allowed:
>
> - Overlay paths to be dependent on previous -ivfsoverlay, ie. a second -ivfsoverlay could specify a virtual path for its overlay if that path is in the first overlay.
> - Paths from one overlay to affect those in an earlier, ie. if the right-most overlay specified a mapping from A -> B and the left-most a mapping from B -> C, A could now map to C.
> - The real filesystem to be skipped, ie. all specified paths have to be contained in an overlay (which would still map to the real filesystem).
>
> FileSystem also had a get/set added for the current working directory
> to allow for filesystem-specific CWD rather than a process-wide CWD,
> further complicating the RedirectingFileSystem implementation.

It's not clear from the description what the plan is for the RedirectingFileSystem's CWD. It sounds like you're suggesting dropping it. If so, what happens if the redirecting filesystem remaps things to a new directory `/new/dir` that doesn't exist in the underlying filesystem, and the client calls `cd /new/dir`?

> This change reverts RedirectingFileSystem to a single responsibility -
> mapping paths and passing that path to the underlying filesystem.

I'm not clear about two things:

- What is "the underlying filesystem"?
- What does RedirectingFileSystem return for something not explicitly listed in the `.yaml`?

> A
> later change implements (1) and (3) through the OverlayFileSystem. (2)
> is no longer possible, which fixes llvm-project#53306 as a side-effect.

Sorry if I missed a discussions somewhere, but I'm not sure I fully understand the plan for landing these changes. It sounds as if this is going to land, regressing (1) and (3), and then we'll add back (1) and (3) later; is that the plan? That doesn't quite seem right; is there another way to sequence the changes to avoid temporary regressions in tree? (If not, then they probably need to land together.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121424