[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

[Kousik Kumar via Phabricator via cfe-commits]

kousikk added a comment.

In D68193#1692293 <https://reviews.llvm.org/D68193#1692293>, @arphaman wrote:

> I don't quite understand the specific issue you're hitting, as it sounds that the logic right now should handle it:
>
> > It seems that when the CachingFileSystem is first given a file to open that is actually a directory, it incorrectly
> > caches that path to be errenous and throws an error when subsequently a directory open >call is made for the same
> > path.
>
> In this case, right now (without this patch) `createFileEntry` will return `std::errc::is_a_directory`. `DependencyScanningWorkerFilesystem::openFileForRead` will then invalidate the entry in the global cache:


`createFileEntry` does return std::errc::is_a_directory, but the cache entry is NOT invalidated from the global cache. It stays on in the global cache, as an error entry for that directory path. I think the current version is slightly better because the first time we see a path, we determine if its a file / directory and store the appropriate CachedFileSystemEntry in the global cache.

> 
> 
>   if (CacheEntry.getError() == std::errc::is_a_directory) {
>       // Reset the CacheEntry to avoid setting an error entry in the
>       // cache for the given directory path.
>       CacheEntry = CachedFileSystemEntry();
>    
> 
> Which means that when the call to `stat` happens, it should be fine as the global cache doesn't have a valid entry, so it will be able to recognize a directory and won't return an error.
> 
> Does this not happen in your case?

I think the difference is the global cache does have the entry (but its set to an Error object). The test-case I added should currently fail without the changes in this CL.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68193