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

Mon Mar 14 15:47:21 PDT 2022

bnbarham added a comment.

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

> - The client's view of the OverlayFS *should* be a single (virtual) filesystem. But without top-level CWD modelling, we can't actually provide that. Consider:
>
>   overlay:
>     base:
>       /file
>     memory:
>       /dir/
>   
>   operations:
>     cd /dir
>     cat ../file
>
> This *should* give the content from `/file` in `base`. But even with our proposed changes it's not going to work correctly. I think one way to be correct would be:
>
> - Find the "first" FS (like CWD?) where `getRealPath(dirname(relativepath))` does not fail
> - Then combine that real-path with `filename(relativepath)`, and go through the overlays with that path
>
> ... but this would probably be too slow. Not sure if you have ideas on how to fix it; and I don't think it's made worse by this patch (off topic?). Maybe worth adding a FIXME or description somewhere documenting that it doesn't work.
>
> (Regardless, I think your plan should work.)

The real problem here is that we can't just resolve the path in `OverlayFileSystem` and pass that down because that would mean that the returned file/status would have a different path to the one requested. Unfortunately we can't just overwrite it with the original path, since it's possible the underlying FS *has to* return a different path (ie. because of `external-names: true`).

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121423