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

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

dexonsmith added a comment.

In D121424#3376321 <https://reviews.llvm.org/D121424#3376321>, @bnbarham wrote:

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

Great; I feel like we need the CWD there; the history was useful, I just took the wrong conclusion overall :).

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

Either one would have felt ambiguous to me, because I was wondering if you were referring to the `sys::fs` filesystem and they both kind of point there! Thanks for clarifying.

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

Okay, makes sense.

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

Got it!

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

Got it; I suggest editing each description to make that super clear in those situations ("these will be landed at the same time, split up only for review convenience").

In this case:

- I think there might be a way to re-sequence D121425 <https://reviews.llvm.org/D121425> ahead of this commit.
  - The new API can be added and unit tested without deleting features from RedirectingFS.
  - There may be some tests that fail until this commit lands; you can either check for the temporary wrong behaviour and fix them in this commit, or shift those tests entirely to this commit if it's too confusing. But it'll help to see what tests suddenly start passing related to the new API.
- Maybe this commit could be made runtime-configurable (`OverlayFS` constructor, off-by-default, only used in the new API to start, then the old behaviour removed later), to land it ahead of D121426 <https://reviews.llvm.org/D121426>? Maybe that's too complicated; just pointing out the flexibility.
  - Else, I'd suggest merging D121426 <https://reviews.llvm.org/D121426> with this one. The changes don't touch the same files and it's not a massive patch so I don't think it'll confuse the review much.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121424