Merge pull request #3738 from obsidiansystems/hash-always-has-type

Hash always has a valid type

16 files changed, 54 insertions, 53 deletions

diff --git a/src/libstore/binary-cache-store.cc b/src/libstore/binary-cache-store.cc
index b791c125b..30150eeba 100644
--- a/src/libstore/binary-cache-store.cc
+++ b/src/libstore/binary-cache-store.cc
@@ -178,7 +178,7 @@ void BinaryCacheStore::addToStore(const ValidPathInfo & info, Source & narSource
     auto [fileHash, fileSize] = fileHashSink.finish();
     narInfo->fileHash = fileHash;
     narInfo->fileSize = fileSize;
-    narInfo->url = "nar/" + narInfo->fileHash.to_string(Base32, false) + ".nar"
+    narInfo->url = "nar/" + narInfo->fileHash->to_string(Base32, false) + ".nar"
         + (compression == "xz" ? ".xz" :
            compression == "bzip2" ? ".bz2" :
            compression == "br" ? ".br" :
@@ -372,7 +372,7 @@ StorePath BinaryCacheStore::addToStore(const string & name, const Path & srcPath
        method for very large paths, but `copyPath' is mainly used for
        small files. */
     StringSink sink;
-    Hash h;
+    std::optional<Hash> h;
     if (method == FileIngestionMethod::Recursive) {
         dumpPath(srcPath, sink, filter);
         h = hashString(hashAlgo, *sink.s);
@@ -382,7 +382,7 @@ StorePath BinaryCacheStore::addToStore(const string & name, const Path & srcPath
         h = hashString(hashAlgo, s);
     }
 
-    ValidPathInfo info(makeFixedOutputPath(method, h, name));
+    ValidPathInfo info(makeFixedOutputPath(method, *h, name));
 
     auto source = StringSource { *sink.s };
     addToStore(info, source, repair, CheckSigs);
diff --git a/src/libstore/build.cc b/src/libstore/build.cc
index 109368f8a..d26f96b74 100644
--- a/src/libstore/build.cc
+++ b/src/libstore/build.cc
@@ -3737,8 +3737,8 @@ void DerivationGoal::registerOutputs()
             /* Check the hash. In hash mode, move the path produced by
                the derivation to its content-addressed location. */
             Hash h2 = outputHash.method == FileIngestionMethod::Recursive
-                ? hashPath(*outputHash.hash.type, actualPath).first
-                : hashFile(*outputHash.hash.type, actualPath);
+                ? hashPath(outputHash.hash.type, actualPath).first
+                : hashFile(outputHash.hash.type, actualPath);
 
             auto dest = worker.store.makeFixedOutputPath(outputHash.method, h2, i.second.path(worker.store, drv->name).name());
 
@@ -3787,8 +3787,10 @@ void DerivationGoal::registerOutputs()
            time.  The hash is stored in the database so that we can
            verify later on whether nobody has messed with the store. */
         debug("scanning for references inside '%1%'", path);
-        HashResult hash;
-        auto references = worker.store.parseStorePathSet(scanForReferences(actualPath, worker.store.printStorePathSet(referenceablePaths), hash));
+        // HashResult hash;
+        auto pathSetAndHash = scanForReferences(actualPath, worker.store.printStorePathSet(referenceablePaths));
+        auto references = worker.store.parseStorePathSet(pathSetAndHash.first);
+        HashResult hash = pathSetAndHash.second;
 
         if (buildMode == bmCheck) {
             if (!worker.store.isValidPath(worker.store.parseStorePath(path))) continue;
@@ -5010,7 +5012,7 @@ bool Worker::pathContentsGood(const StorePath & path)
     if (!pathExists(store.printStorePath(path)))
         res = false;
     else {
-        HashResult current = hashPath(*info->narHash.type, store.printStorePath(path));
+        HashResult current = hashPath(info->narHash->type, store.printStorePath(path));
         Hash nullHash(htSHA256);
         res = info->narHash == nullHash || info->narHash == current.first;
     }
diff --git a/src/libstore/builtins/fetchurl.cc b/src/libstore/builtins/fetchurl.cc
index 017a633b5..03bb77488 100644
--- a/src/libstore/builtins/fetchurl.cc
+++ b/src/libstore/builtins/fetchurl.cc
@@ -68,7 +68,7 @@ void builtinFetchurl(const BasicDerivation & drv, const std::string & netrcData)
             for (auto hashedMirror : settings.hashedMirrors.get()) {
                 try {
                     if (!hasSuffix(hashedMirror, "/")) hashedMirror += '/';
-                    fetch(hashedMirror + printHashType(*hash->hash.hash.type) + "/" + hash->hash.hash.to_string(Base16, false));
+                    fetch(hashedMirror + printHashType(hash->hash.hash.type) + "/" + hash->hash.hash.to_string(Base16, false));
                     return;
                 } catch (Error & e) {
                     debug(e.what());
diff --git a/src/libstore/content-address.cc b/src/libstore/content-address.cc
index 6cb69d0a9..f83b98a98 100644
--- a/src/libstore/content-address.cc
+++ b/src/libstore/content-address.cc
@@ -3,7 +3,7 @@
 namespace nix {
 
 std::string FixedOutputHash::printMethodAlgo() const {
-    return makeFileIngestionPrefix(method) + printHashType(*hash.type);
+    return makeFileIngestionPrefix(method) + printHashType(hash.type);
 }
 
 std::string makeFileIngestionPrefix(const FileIngestionMethod m) {
@@ -46,7 +46,7 @@ ContentAddress parseContentAddress(std::string_view rawCa) {
         if (prefix == "text") {
             auto hashTypeAndHash = rawCa.substr(prefixSeparator+1, string::npos);
             Hash hash = Hash(string(hashTypeAndHash));
-            if (*hash.type != htSHA256) {
+            if (hash.type != htSHA256) {
                 throw Error("parseContentAddress: the text hash should have type SHA256");
             }
             return TextHash { hash };
diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc
index df295084a..503e04f92 100644
--- a/src/libstore/daemon.cc
+++ b/src/libstore/daemon.cc
@@ -289,7 +289,7 @@ static void performOp(TunnelLogger * logger, ref<Store> store,
         logger->startWork();
         auto hash = store->queryPathInfo(path)->narHash;
         logger->stopWork();
-        to << hash.to_string(Base16, false);
+        to << hash->to_string(Base16, false);
         break;
     }
 
@@ -632,7 +632,7 @@ static void performOp(TunnelLogger * logger, ref<Store> store,
             if (GET_PROTOCOL_MINOR(clientVersion) >= 17)
                 to << 1;
             to << (info->deriver ? store->printStorePath(*info->deriver) : "")
-               << info->narHash.to_string(Base16, false);
+               << info->narHash->to_string(Base16, false);
             writeStorePaths(*store, to, info->references);
             to << info->registrationTime << info->narSize;
             if (GET_PROTOCOL_MINOR(clientVersion) >= 16) {
diff --git a/src/libstore/export-import.cc b/src/libstore/export-import.cc
index b963d64d7..a0fc22264 100644
--- a/src/libstore/export-import.cc
+++ b/src/libstore/export-import.cc
@@ -38,9 +38,9 @@ void Store::exportPath(const StorePath & path, Sink & sink)
        filesystem corruption from spreading to other machines.
        Don't complain if the stored hash is zero (unknown). */
     Hash hash = hashSink.currentHash().first;
-    if (hash != info->narHash && info->narHash != Hash(*info->narHash.type))
+    if (hash != info->narHash && info->narHash != Hash(info->narHash->type))
         throw Error("hash of path '%s' has changed from '%s' to '%s'!",
-            printStorePath(path), info->narHash.to_string(Base32, true), hash.to_string(Base32, true));
+            printStorePath(path), info->narHash->to_string(Base32, true), hash.to_string(Base32, true));
 
     teeSink
         << exportMagic
diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc
index a8bd8a972..5d7566121 100644
--- a/src/libstore/legacy-ssh-store.cc
+++ b/src/libstore/legacy-ssh-store.cc
@@ -113,7 +113,7 @@ struct LegacySSHStore : public Store
 
             if (GET_PROTOCOL_MINOR(conn->remoteVersion) >= 4) {
                 auto s = readString(conn->from);
-                info->narHash = s.empty() ? Hash() : Hash(s);
+                info->narHash = s.empty() ? std::optional<Hash>{} : Hash{s};
                 info->ca = parseContentAddressOpt(readString(conn->from));
                 info->sigs = readStrings<StringSet>(conn->from);
             }
@@ -138,7 +138,7 @@ struct LegacySSHStore : public Store
                 << cmdAddToStoreNar
                 << printStorePath(info.path)
                 << (info.deriver ? printStorePath(*info.deriver) : "")
-                << info.narHash.to_string(Base16, false);
+                << info.narHash->to_string(Base16, false);
             writeStorePaths(*this, conn->to, info.references);
             conn->to
                 << info.registrationTime
diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc
index 1e6b34cd1..eb367d778 100644
--- a/src/libstore/local-store.cc
+++ b/src/libstore/local-store.cc
@@ -579,7 +579,7 @@ uint64_t LocalStore::addValidPath(State & state,
 
     state.stmtRegisterValidPath.use()
         (printStorePath(info.path))
-        (info.narHash.to_string(Base16, true))
+        (info.narHash->to_string(Base16, true))
         (info.registrationTime == 0 ? time(0) : info.registrationTime)
         (info.deriver ? printStorePath(*info.deriver) : "", (bool) info.deriver)
         (info.narSize, info.narSize != 0)
@@ -679,7 +679,7 @@ void LocalStore::updatePathInfo(State & state, const ValidPathInfo & info)
 {
     state.stmtUpdatePathInfo.use()
         (info.narSize, info.narSize != 0)
-        (info.narHash.to_string(Base16, true))
+        (info.narHash->to_string(Base16, true))
         (info.ultimate ? 1 : 0, info.ultimate)
         (concatStringsSep(" ", info.sigs), !info.sigs.empty())
         (renderContentAddress(info.ca), (bool) info.ca)
@@ -893,7 +893,7 @@ void LocalStore::registerValidPaths(const ValidPathInfos & infos)
         StorePathSet paths;
 
         for (auto & i : infos) {
-            assert(i.narHash.type == htSHA256);
+            assert(i.narHash && i.narHash->type == htSHA256);
             if (isValidPath_(*state, i.path))
                 updatePathInfo(*state, i);
             else
@@ -1006,7 +1006,7 @@ void LocalStore::addToStore(const ValidPathInfo & info, Source & source,
 
             if (hashResult.first != info.narHash)
                 throw Error("hash mismatch importing path '%s';\n  wanted: %s\n  got:    %s",
-                    printStorePath(info.path), info.narHash.to_string(Base32, true), hashResult.first.to_string(Base32, true));
+                    printStorePath(info.path), info.narHash->to_string(Base32, true), hashResult.first.to_string(Base32, true));
 
             if (hashResult.second != info.narSize)
                 throw Error("size mismatch importing path '%s';\n  wanted: %s\n  got:   %s",
@@ -1302,9 +1302,9 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair)
 
                 std::unique_ptr<AbstractHashSink> hashSink;
                 if (!info->ca || !info->references.count(info->path))
-                    hashSink = std::make_unique<HashSink>(*info->narHash.type);
+                    hashSink = std::make_unique<HashSink>(info->narHash->type);
                 else
-                    hashSink = std::make_unique<HashModuloSink>(*info->narHash.type, std::string(info->path.hashPart()));
+                    hashSink = std::make_unique<HashModuloSink>(info->narHash->type, std::string(info->path.hashPart()));
 
                 dumpPath(Store::toRealPath(i), *hashSink);
                 auto current = hashSink->finish();
@@ -1313,7 +1313,7 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair)
                     logError({
                         .name = "Invalid hash - path modified",
                         .hint = hintfmt("path '%s' was modified! expected hash '%s', got '%s'",
-                        printStorePath(i), info->narHash.to_string(Base32, true), current.first.to_string(Base32, true))
+                        printStorePath(i), info->narHash->to_string(Base32, true), current.first.to_string(Base32, true))
                     });
                     if (repair) repairPath(i); else errors = true;
                 } else {
diff --git a/src/libstore/nar-info-disk-cache.cc b/src/libstore/nar-info-disk-cache.cc
index 012dea6ea..9ddb9957f 100644
--- a/src/libstore/nar-info-disk-cache.cc
+++ b/src/libstore/nar-info-disk-cache.cc
@@ -230,9 +230,9 @@ public:
                     (std::string(info->path.name()))
                     (narInfo ? narInfo->url : "", narInfo != 0)
                     (narInfo ? narInfo->compression : "", narInfo != 0)
-                    (narInfo && narInfo->fileHash ? narInfo->fileHash.to_string(Base32, true) : "", narInfo && narInfo->fileHash)
+                    (narInfo && narInfo->fileHash ? narInfo->fileHash->to_string(Base32, true) : "", narInfo && narInfo->fileHash)
                     (narInfo ? narInfo->fileSize : 0, narInfo != 0 && narInfo->fileSize)
-                    (info->narHash.to_string(Base32, true))
+                    (info->narHash->to_string(Base32, true))
                     (info->narSize)
                     (concatStringsSep(" ", info->shortRefs()))
                     (info->deriver ? std::string(info->deriver->to_string()) : "", (bool) info->deriver)
diff --git a/src/libstore/nar-info.cc b/src/libstore/nar-info.cc
index 04550ed97..ca471463c 100644
--- a/src/libstore/nar-info.cc
+++ b/src/libstore/nar-info.cc
@@ -7,15 +7,14 @@ NarInfo::NarInfo(const Store & store, const std::string & s, const std::string &
     : ValidPathInfo(StorePath(StorePath::dummy)) // FIXME: hack
 {
     auto corrupt = [&]() {
-        throw Error("NAR info file '%1%' is corrupt", whence);
+        return Error("NAR info file '%1%' is corrupt", whence);
     };
 
     auto parseHashField = [&](const string & s) {
         try {
             return Hash(s);
         } catch (BadHash &) {
-            corrupt();
-            return Hash(); // never reached
+            throw corrupt();
         }
     };
 
@@ -25,12 +24,12 @@ NarInfo::NarInfo(const Store & store, const std::string & s, const std::string &
     while (pos < s.size()) {
 
         size_t colon = s.find(':', pos);
-        if (colon == std::string::npos) corrupt();
+        if (colon == std::string::npos) throw corrupt();
 
         std::string name(s, pos, colon - pos);
 
         size_t eol = s.find('\n', colon + 2);
-        if (eol == std::string::npos) corrupt();
+        if (eol == std::string::npos) throw corrupt();
 
         std::string value(s, colon + 2, eol - colon - 2);
 
@@ -45,16 +44,16 @@ NarInfo::NarInfo(const Store & store, const std::string & s, const std::string &
         else if (name == "FileHash")
             fileHash = parseHashField(value);
         else if (name == "FileSize") {
-            if (!string2Int(value, fileSize)) corrupt();
+            if (!string2Int(value, fileSize)) throw corrupt();
         }
         else if (name == "NarHash")
             narHash = parseHashField(value);
         else if (name == "NarSize") {
-            if (!string2Int(value, narSize)) corrupt();
+            if (!string2Int(value, narSize)) throw corrupt();
         }
         else if (name == "References") {
             auto refs = tokenizeString<Strings>(value, " ");
-            if (!references.empty()) corrupt();
+            if (!references.empty()) throw corrupt();
             for (auto & r : refs)
                 references.insert(StorePath(r));
         }
@@ -67,7 +66,7 @@ NarInfo::NarInfo(const Store & store, const std::string & s, const std::string &
         else if (name == "Sig")
             sigs.insert(value);
         else if (name == "CA") {
-            if (ca) corrupt();
+            if (ca) throw corrupt();
             // FIXME: allow blank ca or require skipping field?
             ca = parseContentAddressOpt(value);
         }
@@ -77,7 +76,7 @@ NarInfo::NarInfo(const Store & store, const std::string & s, const std::string &
 
     if (compression == "") compression = "bzip2";
 
-    if (!havePath || url.empty() || narSize == 0 || !narHash) corrupt();
+    if (!havePath || url.empty() || narSize == 0 || !narHash) throw corrupt();
 }
 
 std::string NarInfo::to_string(const Store & store) const
@@ -87,11 +86,11 @@ std::string NarInfo::to_string(const Store & store) const
     res += "URL: " + url + "\n";
     assert(compression != "");
     res += "Compression: " + compression + "\n";
-    assert(fileHash.type == htSHA256);
-    res += "FileHash: " + fileHash.to_string(Base32, true) + "\n";
+    assert(fileHash && fileHash->type == htSHA256);
+    res += "FileHash: " + fileHash->to_string(Base32, true) + "\n";
     res += "FileSize: " + std::to_string(fileSize) + "\n";
-    assert(narHash.type == htSHA256);
-    res += "NarHash: " + narHash.to_string(Base32, true) + "\n";
+    assert(narHash && narHash->type == htSHA256);
+    res += "NarHash: " + narHash->to_string(Base32, true) + "\n";
     res += "NarSize: " + std::to_string(narSize) + "\n";
 
     res += "References: " + concatStringsSep(" ", shortRefs()) + "\n";
diff --git a/src/libstore/nar-info.hh b/src/libstore/nar-info.hh
index 373c33427..eff19f0ef 100644
--- a/src/libstore/nar-info.hh
+++ b/src/libstore/nar-info.hh
@@ -10,7 +10,7 @@ struct NarInfo : ValidPathInfo
 {
     std::string url;
     std::string compression;
-    Hash fileHash;
+    std::optional<Hash> fileHash;
     uint64_t fileSize = 0;
     std::string system;
 
diff --git a/src/libstore/references.cc b/src/libstore/references.cc
index 968e2e425..62a3cda61 100644
--- a/src/libstore/references.cc
+++ b/src/libstore/references.cc
@@ -76,8 +76,8 @@ void RefScanSink::operator () (const unsigned char * data, size_t len)
 }
 
 
-PathSet scanForReferences(const string & path,
-    const PathSet & refs, HashResult & hash)
+std::pair<PathSet, HashResult> scanForReferences(const string & path,
+    const PathSet & refs)
 {
     RefScanSink refsSink;
     HashSink hashSink { htSHA256 };
@@ -111,9 +111,9 @@ PathSet scanForReferences(const string & path,
         found.insert(j->second);
     }
 
-    hash = hashSink.finish();
+    auto hash = hashSink.finish();
 
-    return found;
+    return std::pair<PathSet, HashResult>(found, hash);
 }
 
 
diff --git a/src/libstore/references.hh b/src/libstore/references.hh
index c38bdd720..598a3203a 100644
--- a/src/libstore/references.hh
+++ b/src/libstore/references.hh
@@ -5,8 +5,7 @@
 
 namespace nix {
 
-PathSet scanForReferences(const Path & path, const PathSet & refs,
-    HashResult & hash);
+std::pair<PathSet, HashResult> scanForReferences(const Path & path, const PathSet & refs);
 
 struct RewritingSink : Sink
 {
diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc
index 9af4364b7..8d01c6667 100644
--- a/src/libstore/remote-store.cc
+++ b/src/libstore/remote-store.cc
@@ -498,7 +498,7 @@ void RemoteStore::addToStore(const ValidPathInfo & info, Source & source,
         conn->to << wopAddToStoreNar
                  << printStorePath(info.path)
                  << (info.deriver ? printStorePath(*info.deriver) : "")
-                 << info.narHash.to_string(Base16, false);
+                 << info.narHash->to_string(Base16, false);
         writeStorePaths(*this, conn->to, info.references);
         conn->to << info.registrationTime << info.narSize
                  << info.ultimate << info.sigs << renderContentAddress(info.ca)
diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc
index ab21b0bd5..b6e6952b7 100644
--- a/src/libstore/store-api.cc
+++ b/src/libstore/store-api.cc
@@ -538,7 +538,7 @@ string Store::makeValidityRegistration(const StorePathSet & paths,
         auto info = queryPathInfo(i);
 
         if (showHash) {
-            s += info->narHash.to_string(Base16, false) + "\n";
+            s += info->narHash->to_string(Base16, false) + "\n";
             s += (format("%1%\n") % info->narSize).str();
         }
 
@@ -570,7 +570,7 @@ void Store::pathInfoToJSON(JSONPlaceholder & jsonOut, const StorePathSet & store
             auto info = queryPathInfo(storePath);
 
             jsonPath
-                .attr("narHash", info->narHash.to_string(hashBase, true))
+                .attr("narHash", info->narHash->to_string(hashBase, true))
                 .attr("narSize", info->narSize);
 
             {
@@ -613,7 +613,7 @@ void Store::pathInfoToJSON(JSONPlaceholder & jsonOut, const StorePathSet & store
                     if (!narInfo->url.empty())
                         jsonPath.attr("url", narInfo->url);
                     if (narInfo->fileHash)
-                        jsonPath.attr("downloadHash", narInfo->fileHash.to_string(hashBase, true));
+                        jsonPath.attr("downloadHash", narInfo->fileHash->to_string(hashBase, true));
                     if (narInfo->fileSize)
                         jsonPath.attr("downloadSize", narInfo->fileSize);
                     if (showClosureSize)
@@ -854,7 +854,7 @@ std::string ValidPathInfo::fingerprint(const Store & store) const
             store.printStorePath(path));
     return
         "1;" + store.printStorePath(path) + ";"
-        + narHash.to_string(Base32, true) + ";"
+        + narHash->to_string(Base32, true) + ";"
         + std::to_string(narSize) + ";"
         + concatStringsSep(",", store.printStorePathSet(references));
 }
diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh
index d1cb2035f..a7288d0cc 100644
--- a/src/libstore/store-api.hh
+++ b/src/libstore/store-api.hh
@@ -115,7 +115,8 @@ struct ValidPathInfo
 {
     StorePath path;
     std::optional<StorePath> deriver;
-    Hash narHash;
+    // TODO document this
+    std::optional<Hash> narHash;
     StorePathSet references;
     time_t registrationTime = 0;
     uint64_t narSize = 0; // 0 = unknown