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

[Ben Barham via Phabricator via llvm-commits]

bnbarham added a comment.

In D121424#3376299 <https://reviews.llvm.org/D121424#3376299>, @dexonsmith wrote:

>> 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`?

Sorry, I was only trying to give some history here. Adding CWD in caused various complications in RedirectingFileSystem due to the added nesting, ie. what path do we pass to the next FS? Hence all the mess with trying to map back to the original path depending on whether use external names is set, but still passing down the canonical name, etc. I'm certainly not suggesting we remove it, it's much easier to reason about with the simplified RedirectinFileSystem.

>> 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`?



1. I've been using "external" and "underlying" interchangeably. But I mean `ExternalFS`, ie. the FS that's passed into `RedirectingFileSystem`.
2. It returns `no_such_file_or_directory`, as it once did. If we want an overlay, we should use OverlayFileSystem. Most of this commit message is trying to explain how we got to where we are today in order to provide some context.

>> 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.)

The plan is to land them together. I just split them to make it easier to review (ie. smaller chunks). Perhaps this has only just added to the confusion.

To be clear:

- D121421 <https://reviews.llvm.org/D121421> and D121423 <https://reviews.llvm.org/D121423> and both be landed separately
- D121424 <https://reviews.llvm.org/D121424>, D121425 <https://reviews.llvm.org/D121425>, and D121426 <https://reviews.llvm.org/D121426> all need to be landed together. I don't see a way to split them up to avoid that, but if you do please let me know.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121424