Make narHash in ValidPathInfo not optional #3899
Conversation
We did this in the same spirit of the dummy value that's present in libstore/path.hh
meditans
changed the title
|
We realized we need to remove |
meditans
changed the title
Since the hash is not optional anymore
meditans
changed the title
src/libstore/path-info.hh
Outdated
| ValidPathInfo(StorePath && path) : path(std::move(path)) { }; | ||
| ValidPathInfo(const StorePath & path) : path(path) { }; | ||
| ValidPathInfo(StorePath && path) : path(std::move(path)), narHash(Hash::dummy) { }; | ||
| ValidPathInfo(const StorePath & path) : path(path), narHash(Hash::dummy) { }; |
There was a problem hiding this comment.
Ideally we'd require a narHash at construction time. Do you know if there are any places where we construct a ValidPathInfo and we don't know the narHash?
There was a problem hiding this comment.
Agreed. We should make this an argument, and at the very least callers would opt into using Hash::dummy and then initialize later, though better yet Hash::dummy can go away altogether.
|
@edolstra OK the |
| /* No longer support missing NAR hash */ | ||
| assert(GET_PROTOCOL_MINOR(conn->remoteVersion) >= 4); |
There was a problem hiding this comment.
One of the more interesting changes
| if (s == "") | ||
| throw Error("NAR hash is now mandatory"); |
| ValidPathInfo info(makeFixedOutputPath(method, *h, name)); | ||
| ValidPathInfo info { | ||
| makeFixedOutputPath(method, *h, name), | ||
| Hash::dummy, // Will be fixed in addToStore, which recomputes nar hash |
There was a problem hiding this comment.
This is rather tricky. Because the other one calculates the Nar hash, I think we should eventually make an addToStoreFromDump out of it, and then this tricky bit can go away.
| ValidPathInfo info(computeStorePathForText(name, s, references)); | ||
| ValidPathInfo info { | ||
| computeStorePathForText(name, s, references), | ||
| Hash::dummy, // Will be fixed in addToStore, which recomputes nar hash |
There was a problem hiding this comment.
To get rid of this like we do above, we would need addToStoreFromDump to also support references, but I think that's a good idea anyways.
No description provided.