Merge pull request #10009 from obsidiansystems/ca-type-names

Proper `parse` and `render` functions for `FileIngestionMethod` and `ContentAddressMethod`

src/libfetchers/fetch-to-store.cc

@@ -21,23 +21,9 @@ StorePath fetchToStore(
         cacheKey = fetchers::Attrs{
             {"_what", "fetchToStore"},
             {"store", store.storeDir},
-            {"name", std::string(name)},
+            {"name", std::string{name}},
             {"fingerprint", *path.accessor->fingerprint},
-            {
-                "method",
-                std::visit(overloaded {
-                    [](const TextIngestionMethod &) {
-                        return "text";
-                    },
-                    [](const FileIngestionMethod & fim) {
-                        switch (fim) {
-                        case FileIngestionMethod::Flat: return "flat";
-                        case FileIngestionMethod::Recursive: return "nar";
-                        default: assert(false);
-                        }
-                    },
-                }, method.raw),
-            },
+            {"method", std::string{method.render()}},
             {"path", path.path.abs()}
         };
         if (auto res = fetchers::getCache()->lookup(store, *cacheKey)) {

src/libstore/content-address.cc

@@ -4,7 +4,7 @@
 
 namespace nix {
 
-std::string makeFileIngestionPrefix(FileIngestionMethod m)
+std::string_view makeFileIngestionPrefix(FileIngestionMethod m)
 {
     switch (m) {
     case FileIngestionMethod::Flat:
@@ -16,10 +16,29 @@ std::string makeFileIngestionPrefix(FileIngestionMethod m)
     }
 }
 
-std::string ContentAddressMethod::renderPrefix() const
+std::string_view ContentAddressMethod::render() const
 {
     return std::visit(overloaded {
-        [](TextIngestionMethod) -> std::string { return "text:"; },
+        [](TextIngestionMethod) -> std::string_view { return "text"; },
+        [](FileIngestionMethod m2) {
+             /* Not prefixed for back compat with things that couldn't produce text before. */
+            return renderFileIngestionMethod(m2);
+        },
+    }, raw);
+}
+
+ContentAddressMethod ContentAddressMethod::parse(std::string_view m)
+{
+    if (m == "text")
+        return TextIngestionMethod {};
+    else
+        return parseFileIngestionMethod(m);
+}
+
+std::string_view ContentAddressMethod::renderPrefix() const
+{
+    return std::visit(overloaded {
+        [](TextIngestionMethod) -> std::string_view { return "text:"; },
         [](FileIngestionMethod m2) {
              /* Not prefixed for back compat with things that couldn't produce text before. */
             return makeFileIngestionPrefix(m2);
@@ -38,7 +57,7 @@ ContentAddressMethod ContentAddressMethod::parsePrefix(std::string_view & m)
     return FileIngestionMethod::Flat;
 }
 
-std::string ContentAddressMethod::render(HashAlgorithm ha) const
+std::string ContentAddressMethod::renderWithAlgo(HashAlgorithm ha) const
 {
     return std::visit(overloaded {
         [&](const TextIngestionMethod & th) {
@@ -133,7 +152,7 @@ ContentAddress ContentAddress::parse(std::string_view rawCa)
     };
 }
 
-std::pair<ContentAddressMethod, HashAlgorithm> ContentAddressMethod::parse(std::string_view caMethod)
+std::pair<ContentAddressMethod, HashAlgorithm> ContentAddressMethod::parseWithAlgo(std::string_view caMethod)
 {
     std::string asPrefix = std::string{caMethod} + ":";
     // parseContentAddressMethodPrefix takes its argument by reference
@@ -155,7 +174,7 @@ std::string renderContentAddress(std::optional<ContentAddress> ca)
 
 std::string ContentAddress::printMethodAlgo() const
 {
-    return method.renderPrefix()
+    return std::string { method.renderPrefix() }
         + printHashAlgo(hash.algo);
 }
 

src/libstore/content-address.hh

@@ -36,7 +36,7 @@ struct TextIngestionMethod : std::monostate { };
  * Compute the prefix to the hash algorithm which indicates how the
  * files were ingested.
  */
-std::string makeFileIngestionPrefix(FileIngestionMethod m);
+std::string_view makeFileIngestionPrefix(FileIngestionMethod m);
 
 /**
  * An enumeration of all the ways we can content-address store objects.
@@ -59,6 +59,20 @@ struct ContentAddressMethod
 
     MAKE_WRAPPER_CONSTRUCTOR(ContentAddressMethod);
 
+    /**
+     * Parse a content addressing method (name).
+     *
+     * The inverse of `render`.
+     */
+    static ContentAddressMethod parse(std::string_view rawCaMethod);
+
+    /**
+     * Render a content addressing method (name).
+     *
+     * The inverse of `parse`.
+     */
+    std::string_view render() const;
+
     /**
      * Parse the prefix tag which indicates how the files
      * were ingested, with the fixed output case not prefixed for back
@@ -74,12 +88,12 @@ struct ContentAddressMethod
      *
      * The rough inverse of `parsePrefix()`.
      */
-    std::string renderPrefix() const;
+    std::string_view renderPrefix() const;
 
     /**
      * Parse a content addressing method and hash type.
      */
-    static std::pair<ContentAddressMethod, HashAlgorithm> parse(std::string_view rawCaMethod);
+    static std::pair<ContentAddressMethod, HashAlgorithm> parseWithAlgo(std::string_view rawCaMethod);
 
     /**
      * Render a content addressing method and hash type in a
@@ -87,7 +101,7 @@ struct ContentAddressMethod
      *
      * The rough inverse of `parse()`.
      */
-    std::string render(HashAlgorithm ht) const;
+    std::string renderWithAlgo(HashAlgorithm ht) const;
 
     /**
      * Get the underlying way to content-address file system objects.

src/libstore/daemon.cc

@@ -400,7 +400,7 @@ static void performOp(TunnelLogger * logger, ref<Store> store,
             logger->startWork();
             auto pathInfo = [&]() {
                 // NB: FramedSource must be out of scope before logger->stopWork();
-                auto [contentAddressMethod, hashAlgo_] = ContentAddressMethod::parse(camStr);
+                auto [contentAddressMethod, hashAlgo_] = ContentAddressMethod::parseWithAlgo(camStr);
                 auto hashAlgo = hashAlgo_; // work around clang bug
                 FramedSource source(from);
                 // TODO these two steps are essentially RemoteStore::addCAToStore. Move it up to Store.

src/libstore/derivations.cc

@@ -601,7 +601,7 @@ std::string Derivation::unparse(const StoreDirConfig & store, bool maskOutputs,
             },
             [&](const DerivationOutput::CAFloating & dof) {
                 s += ','; printUnquotedString(s, "");
-                s += ','; printUnquotedString(s, dof.method.renderPrefix() + printHashAlgo(dof.hashAlgo));
+                s += ','; printUnquotedString(s, std::string { dof.method.renderPrefix() } + printHashAlgo(dof.hashAlgo));
                 s += ','; printUnquotedString(s, "");
             },
             [&](const DerivationOutput::Deferred &) {
@@ -612,7 +612,7 @@ std::string Derivation::unparse(const StoreDirConfig & store, bool maskOutputs,
             [&](const DerivationOutput::Impure & doi) {
                 // FIXME
                 s += ','; printUnquotedString(s, "");
-                s += ','; printUnquotedString(s, doi.method.renderPrefix() + printHashAlgo(doi.hashAlgo));
+                s += ','; printUnquotedString(s, std::string { doi.method.renderPrefix() } + printHashAlgo(doi.hashAlgo));
                 s += ','; printUnquotedString(s, "impure");
             }
         }, i.second.raw);
@@ -984,7 +984,7 @@ void writeDerivation(Sink & out, const StoreDirConfig & store, const BasicDeriva
             },
             [&](const DerivationOutput::CAFloating & dof) {
                 out << ""
-                    << (dof.method.renderPrefix() + printHashAlgo(dof.hashAlgo))
+                    << (std::string { dof.method.renderPrefix() } + printHashAlgo(dof.hashAlgo))
                     << "";
             },
             [&](const DerivationOutput::Deferred &) {
@@ -994,7 +994,7 @@ void writeDerivation(Sink & out, const StoreDirConfig & store, const BasicDeriva
             },
             [&](const DerivationOutput::Impure & doi) {
                 out << ""
-                    << (doi.method.renderPrefix() + printHashAlgo(doi.hashAlgo))
+                    << (std::string { doi.method.renderPrefix() } + printHashAlgo(doi.hashAlgo))
                     << "impure";
             },
         }, i.second.raw);
@@ -1221,11 +1221,11 @@ nlohmann::json DerivationOutput::toJSON(
             // FIXME print refs?
         },
         [&](const DerivationOutput::CAFloating & dof) {
-            res["hashAlgo"] = dof.method.renderPrefix() + printHashAlgo(dof.hashAlgo);
+            res["hashAlgo"] = std::string { dof.method.renderPrefix() } + printHashAlgo(dof.hashAlgo);
         },
         [&](const DerivationOutput::Deferred &) {},
         [&](const DerivationOutput::Impure & doi) {
-            res["hashAlgo"] = doi.method.renderPrefix() + printHashAlgo(doi.hashAlgo);
+            res["hashAlgo"] = std::string { doi.method.renderPrefix() } + printHashAlgo(doi.hashAlgo);
             res["impure"] = true;
         },
     }, raw);

src/libstore/remote-store.cc

@@ -435,7 +435,7 @@ ref<const ValidPathInfo> RemoteStore::addCAToStore(
         conn->to
             << WorkerProto::Op::AddToStore
             << name
-            << caMethod.render(hashAlgo);
+            << caMethod.renderWithAlgo(hashAlgo);
         WorkerProto::write(*this, *conn, references);
         conn->to << repair;
 

src/libutil/file-content-address.cc

@@ -3,6 +3,31 @@
 
 namespace nix {
 
+FileIngestionMethod parseFileIngestionMethod(std::string_view input)
+{
+    if (input == "flat") {
+        return FileIngestionMethod::Flat;
+    } else if (input == "nar") {
+        return FileIngestionMethod::Recursive;
+    } else {
+        throw UsageError("Unknown file ingestion method '%s', expect `flat` or `nar`");
+    }
+}
+
+
+std::string_view renderFileIngestionMethod(FileIngestionMethod method)
+{
+    switch (method) {
+    case FileIngestionMethod::Flat:
+        return "flat";
+    case FileIngestionMethod::Recursive:
+        return "nar";
+    default:
+        abort();
+    }
+}
+
+
 void dumpPath(
     SourceAccessor & accessor, const CanonPath & path,
     Sink & sink,

src/libutil/file-content-address.hh

@@ -23,6 +23,23 @@ enum struct FileIngestionMethod : uint8_t {
     Recursive = 1,
 };
 
+/**
+ * Parse a `FileIngestionMethod` by name. Choice of:
+ *
+ *  - `flat`: `FileIngestionMethod::Flat`
+ *  - `nar`: `FileIngestionMethod::Recursive`
+ *
+ * Oppostite of `renderFileIngestionMethod`.
+ */
+FileIngestionMethod parseFileIngestionMethod(std::string_view input);
+
+/**
+ * Render a `FileIngestionMethod` by name.
+ *
+ * Oppostite of `parseFileIngestionMethod`.
+ */
+std::string_view renderFileIngestionMethod(FileIngestionMethod method);
+
 /**
  * Dump a serialization of the given file system object.
  */

src/nix/add-to-store.cc

@@ -6,17 +6,6 @@
 
 using namespace nix;
 
-static FileIngestionMethod parseIngestionMethod(std::string_view input)
-{
-    if (input == "flat") {
-        return FileIngestionMethod::Flat;
-    } else if (input == "nar") {
-        return FileIngestionMethod::Recursive;
-    } else {
-        throw UsageError("Unknown hash mode '%s', expect `flat` or `nar`");
-    }
-}
-
 struct CmdAddToStore : MixDryRun, StoreCommand
 {
     Path path;
@@ -49,7 +38,7 @@ struct CmdAddToStore : MixDryRun, StoreCommand
             )",
             .labels = {"hash-mode"},
             .handler = {[this](std::string s) {
-                this->caMethod = parseIngestionMethod(s);
+                this->caMethod = parseFileIngestionMethod(s);
             }},
         });
 

tests/unit/libstore/content-address.cc

@@ -0,0 +1,35 @@
+#include <gtest/gtest.h>
+
+#include "content-address.hh"
+
+namespace nix {
+
+/* ----------------------------------------------------------------------------
+ * ContentAddressMethod::parse, ContentAddressMethod::render
+ * --------------------------------------------------------------------------*/
+
+TEST(ContentAddressMethod, testRoundTripPrintParse_1) {
+    for (const ContentAddressMethod & cam : {
+        ContentAddressMethod { TextIngestionMethod {} },
+        ContentAddressMethod { FileIngestionMethod::Flat },
+        ContentAddressMethod { FileIngestionMethod::Recursive },
+    }) {
+        EXPECT_EQ(ContentAddressMethod::parse(cam.render()), cam);
+    }
+}
+
+TEST(ContentAddressMethod, testRoundTripPrintParse_2) {
+    for (const std::string_view camS : {
+        "text",
+        "flat",
+        "nar",
+    }) {
+        EXPECT_EQ(ContentAddressMethod::parse(camS).render(), camS);
+    }
+}
+
+TEST(ContentAddressMethod, testParseContentAddressMethodOptException) {
+    EXPECT_THROW(ContentAddressMethod::parse("narwhal"), UsageError);
+}
+
+}

tests/unit/libutil/file-content-address.cc

@@ -0,0 +1,33 @@
+#include <gtest/gtest.h>
+
+#include "file-content-address.hh"
+
+namespace nix {
+
+/* ----------------------------------------------------------------------------
+ * parseFileIngestionMethod, renderFileIngestionMethod
+ * --------------------------------------------------------------------------*/
+
+TEST(FileIngestionMethod, testRoundTripPrintParse_1) {
+    for (const FileIngestionMethod fim : {
+        FileIngestionMethod::Flat,
+        FileIngestionMethod::Recursive,
+    }) {
+        EXPECT_EQ(parseFileIngestionMethod(renderFileIngestionMethod(fim)), fim);
+    }
+}
+
+TEST(FileIngestionMethod, testRoundTripPrintParse_2) {
+    for (const std::string_view fimS : {
+        "flat",
+        "nar",
+    }) {
+        EXPECT_EQ(renderFileIngestionMethod(parseFileIngestionMethod(fimS)), fimS);
+    }
+}
+
+TEST(FileIngestionMethod, testParseFileIngestionMethodOptException) {
+    EXPECT_THROW(parseFileIngestionMethod("narwhal"), UsageError);
+}
+
+}