remote-fs-accessor: return null for missing store objects - #16408
remote-fs-accessor: return null for missing store objects#16408domenkozar wants to merge 1 commit into
Conversation
883dd71 to
1fbf496
Compare
| /* FIXME: Correctly handle invalid names (return nullopt). */ | ||
| auto [storePath, restPath] = store->toStorePath(store->storeDir + path.abs()); | ||
| auto accessor = accessObject(storePath); | ||
| if (!accessor) | ||
| return std::nullopt; | ||
| return accessor->maybeLstat(restPath); |
There was a problem hiding this comment.
This makes me think we should have a
std::pair<std::shared_ptr<SourceAccessor>, CanonPath> RemoteFSAccessor::maybeFetch(const CanonPath & path);that fetch wraps
There was a problem hiding this comment.
Not sure it's needed - the only caller would be maybeLstat which is specific enough that inlining would yield cleaner code.
There was a problem hiding this comment.
Yes, it is only needed once, but I also think that the fetch wrapper around maybeFetch is very trivial. It is just throwing if the first thing is non-null, and then returning std::pair<ref<SourceAccessor>, CanonPath> to "prove" that that check was done.
There was a problem hiding this comment.
If we don't do this, then fetch version and the maybeLstat version can go out of sink --- arguably we are getting bit by that a bit already. Forcing them to stay in sync is what I am trying to accomplish here, not "DRY for DRY's sake".
|
|
||
| void RemoteFSAccessor::anchor() {} | ||
|
|
||
| RemoteFSAccessor::RemoteFSAccessor(ref<Store> store, bool requireValidPath, std::optional<AbsolutePath> cacheDir) |
There was a problem hiding this comment.
Why leave the signature unchanged if the argument is fully unused?
| return res.first->maybeLstat(res.second); | ||
| /* FIXME: Correctly handle invalid names (return nullopt). */ | ||
| auto [storePath, restPath] = store->toStorePath(store->storeDir + path.abs()); | ||
| auto accessor = accessObject(storePath); |
There was a problem hiding this comment.
Can we copy the fix for that in LocalFSStore accessor? The whole storeDir + path.abs() is pretty unnecessary. No idea why it was done this way
There was a problem hiding this comment.
Oh we should just share a CanonPath -> std::pair<StorePath, CanonPath> function for both, and maybe that obviates the need for my maybeFetch because that new function is the actual used-many-times nugget here.
There was a problem hiding this comment.
Store::toStorePath can also reuse that function in its own definition.
There was a problem hiding this comment.
Oh we should just share a CanonPath -> std::pair<StorePath, CanonPath> function for both
It's a bit more complicated though, because we also need to handle the case of "this CanonPath can't be a valid store path name, so return nullopt". See #16017
There was a problem hiding this comment.
I had previously fixed that stuff for the local store in #16072 and sprinkled around all these FIXMEs.
3 participants
Motivation
Store::getFSAccessor(path)promises to returnnullptrwhen the store object is missing.RemoteFSAccessor::accessObjectinstead propagatedInvalidPathfromqueryPathInfo.Context
Found while reviewing the profile-store work in #16395. Catch
InvalidPathat the path-info lookup boundary and returnnullptr, preserving other accessor errors. Includes a regression test using a local binary cache.