Merge "Revert "libfetchers: make attribute / URL query handling consistent"" into main

6 files changed, 94 insertions, 135 deletions

diff --git a/src/libfetchers/fetchers.hh b/src/libfetchers/fetchers.hh
index 40f2b6294..2bb4248be 100644
--- a/src/libfetchers/fetchers.hh
+++ b/src/libfetchers/fetchers.hh
@@ -159,37 +159,6 @@ struct InputScheme
         std::optional<std::string> commitMsg) const;
 
     virtual std::pair<StorePath, Input> fetch(ref<Store> store, const Input & input) = 0;
-
-protected:
-    void emplaceURLQueryIntoAttrs(
-        const ParsedURL & parsedURL,
-        Attrs & attrs,
-        const StringSet & numericParams,
-        const StringSet & booleanParams) const
-    {
-        for (auto &[name, value] : parsedURL.query) {
-            if (name == "url") {
-                throw BadURL(
-                    "URL '%s' must not override url via query param!",
-                    parsedURL.to_string()
-                );
-            } else if (numericParams.count(name) != 0) {
-                if (auto n = string2Int<uint64_t>(value)) {
-                    attrs.insert_or_assign(name, *n);
-                } else {
-                    throw BadURL(
-                        "URL '%s' has non-numeric parameter '%s'",
-                        parsedURL.to_string(),
-                        name
-                    );
-                }
-            } else if (booleanParams.count(name) != 0) {
-                attrs.emplace(name, Explicit<bool> { value == "1" });
-            } else {
-                attrs.emplace(name, value);
-            }
-        }
-    }
 };
 
 void registerInputScheme(std::shared_ptr<InputScheme> && fetcher);
diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc
index a06bc86b2..b33d893b7 100644
--- a/src/libfetchers/git.cc
+++ b/src/libfetchers/git.cc
@@ -273,14 +273,17 @@ struct GitInputScheme : InputScheme
 
         Attrs attrs;
         attrs.emplace("type", "git");
-        attrs.emplace("url", url2.to_string());
 
-        emplaceURLQueryIntoAttrs(
-            url,
-            attrs,
-            {"lastModified", "revCount"},
-            {"shallow", "submodules", "allRefs"}
-        );
+        for (auto & [name, value] : url.query) {
+            if (name == "rev" || name == "ref")
+                attrs.emplace(name, value);
+            else if (name == "shallow" || name == "submodules" || name == "allRefs")
+                attrs.emplace(name, Explicit<bool> { value == "1" });
+            else
+                url2.query.emplace(name, value);
+        }
+
+        attrs.emplace("url", url2.to_string());
 
         return inputFromAttrs(attrs);
     }
diff --git a/src/libfetchers/github.cc b/src/libfetchers/github.cc
index b971781ae..60fefd1f3 100644
--- a/src/libfetchers/github.cc
+++ b/src/libfetchers/github.cc
@@ -1,4 +1,3 @@
-#include "attrs.hh"
 #include "filetransfer.hh"
 #include "cache.hh"
 #include "globals.hh"
@@ -37,11 +36,18 @@ struct GitArchiveInputScheme : InputScheme
 
         auto path = tokenizeString<std::vector<std::string>>(url.path, "/");
 
-        std::optional<std::string> refOrRev;
+        std::optional<Hash> rev;
+        std::optional<std::string> ref;
+        std::optional<std::string> host_url;
 
         auto size = path.size();
         if (size == 3) {
-            refOrRev = path[2];
+            if (std::regex_match(path[2], revRegex))
+                rev = Hash::parseAny(path[2], htSHA1);
+            else if (std::regex_match(path[2], refRegex))
+                ref = path[2];
+            else
+                throw BadURL("in URL '%s', '%s' is not a commit hash or branch/tag name", url.url, path[2]);
         } else if (size > 3) {
             std::string rs;
             for (auto i = std::next(path.begin(), 2); i != path.end(); i++) {
@@ -52,91 +58,61 @@ struct GitArchiveInputScheme : InputScheme
             }
 
             if (std::regex_match(rs, refRegex)) {
-                refOrRev = rs;
+                ref = rs;
             } else {
                 throw BadURL("in URL '%s', '%s' is not a branch/tag name", url.url, rs);
             }
         } else if (size < 2)
             throw BadURL("URL '%s' is invalid", url.url);
 
-        Attrs attrs;
-        attrs.emplace("type", type());
-        attrs.emplace("owner", path[0]);
-        attrs.emplace("repo", path[1]);
-
         for (auto &[name, value] : url.query) {
-            if (name == "rev" || name == "ref") {
-                if (refOrRev) {
-                    throw BadURL("URL '%s' already contains a ref or rev", url.url);
-                } else {
-                    refOrRev = value;
-                }
-            } else if (name == "lastModified") {
-                if (auto n = string2Int<uint64_t>(value)) {
-                    attrs.emplace(name, *n);
-                } else {
-                    throw Error(
-                        "Attribute 'lastModified' in URL '%s' must be an integer",
-                        url.to_string()
-                    );
-                }
-            } else {
-                attrs.emplace(name, value);
+            if (name == "rev") {
+                if (rev)
+                    throw BadURL("URL '%s' contains multiple commit hashes", url.url);
+                rev = Hash::parseAny(value, htSHA1);
             }
+            else if (name == "ref") {
+                if (!std::regex_match(value, refRegex))
+                    throw BadURL("URL '%s' contains an invalid branch/tag name", url.url);
+                if (ref)
+                    throw BadURL("URL '%s' contains multiple branch/tag names", url.url);
+                ref = value;
+            }
+            else if (name == "host") {
+                if (!std::regex_match(value, hostRegex))
+                    throw BadURL("URL '%s' contains an invalid instance host", url.url);
+                host_url = value;
+            }
+            // FIXME: barf on unsupported attributes
         }
 
-        if (refOrRev) attrs.emplace("refOrRev", *refOrRev);
+        if (ref && rev)
+            throw BadURL("URL '%s' contains both a commit hash and a branch/tag name %s %s", url.url, *ref, rev->gitRev());
 
-        return inputFromAttrs(attrs);
+        Input input;
+        input.attrs.insert_or_assign("type", type());
+        input.attrs.insert_or_assign("owner", path[0]);
+        input.attrs.insert_or_assign("repo", path[1]);
+        if (rev) input.attrs.insert_or_assign("rev", rev->gitRev());
+        if (ref) input.attrs.insert_or_assign("ref", *ref);
+        if (host_url) input.attrs.insert_or_assign("host", *host_url);
+
+        return input;
     }
 
     std::optional<Input> inputFromAttrs(const Attrs & attrs) const override
     {
-        // Attributes can contain refOrRev and it needs to be figured out
-        // which one it is (see inputFromURL for when that may happen).
-        // The correct one (ref or rev) will be written into finalAttrs and
-        // it needs to be mutable for that.
-        Attrs finalAttrs(attrs);
-        auto type_ = maybeGetStrAttr(finalAttrs, "type");
-        if (type_ != type()) return {};
-
-        auto owner = getStrAttr(finalAttrs, "owner");
-        auto repo = getStrAttr(finalAttrs, "repo");
-
-        auto url = fmt("%s:%s/%s", *type_, owner, repo);
-        if (auto host = maybeGetStrAttr(finalAttrs, "host")) {
-            if (!std::regex_match(*host, hostRegex)) {
-                throw BadURL("URL '%s' contains an invalid instance host", url);
-            }
-        }
-
-        if (auto refOrRev = maybeGetStrAttr(finalAttrs, "refOrRev")) {
-            finalAttrs.erase("refOrRev");
-            if (std::regex_match(*refOrRev, revRegex)) {
-                finalAttrs.emplace("rev", *refOrRev);
-            } else if (std::regex_match(*refOrRev, refRegex)) {
-                finalAttrs.emplace("ref", *refOrRev);
-            } else {
-                throw Error(
-                    "in URL '%s', '%s' is not a commit hash or a branch/tag name",
-                    url,
-                    *refOrRev
-                );
-            }
-        } else if (auto ref = maybeGetStrAttr(finalAttrs, "ref")) {
-            if (!std::regex_match(*ref, refRegex)) {
-                throw BadURL("URL '%s' contains an invalid branch/tag name", url);
-            }
-        }
+        if (maybeGetStrAttr(attrs, "type") != type()) return {};
 
-        for (auto & [name, value] : finalAttrs) {
-            if (name != "type" && name != "owner" && name != "repo" && name != "ref" && name != "rev" && name != "narHash" && name != "lastModified" && name != "host") {
+        for (auto & [name, value] : attrs)
+            if (name != "type" && name != "owner" && name != "repo" && name != "ref" && name != "rev" && name != "narHash" && name != "lastModified" && name != "host")
                 throw Error("unsupported input attribute '%s'", name);
-            }
-        }
+
+        getStrAttr(attrs, "owner");
+        getStrAttr(attrs, "repo");
 
         Input input;
-        input.attrs = finalAttrs;
+        input.attrs = attrs;
         return input;
     }
 
diff --git a/src/libfetchers/indirect.cc b/src/libfetchers/indirect.cc
index 8c0176e84..c73505b31 100644
--- a/src/libfetchers/indirect.cc
+++ b/src/libfetchers/indirect.cc
@@ -17,8 +17,6 @@ struct IndirectInputScheme : InputScheme
         std::optional<Hash> rev;
         std::optional<std::string> ref;
 
-        Attrs attrs;
-
         if (path.size() == 1) {
         } else if (path.size() == 2) {
             if (std::regex_match(path[1], revRegex))
@@ -28,21 +26,29 @@ struct IndirectInputScheme : InputScheme
             else
                 throw BadURL("in flake URL '%s', '%s' is not a commit hash or branch/tag name", url.url, path[1]);
         } else if (path.size() == 3) {
+            if (!std::regex_match(path[1], refRegex))
+                throw BadURL("in flake URL '%s', '%s' is not a branch/tag name", url.url, path[1]);
             ref = path[1];
+            if (!std::regex_match(path[2], revRegex))
+                throw BadURL("in flake URL '%s', '%s' is not a commit hash", url.url, path[2]);
             rev = Hash::parseAny(path[2], htSHA1);
         } else
             throw BadURL("GitHub URL '%s' is invalid", url.url);
 
         std::string id = path[0];
+        if (!std::regex_match(id, flakeRegex))
+            throw BadURL("'%s' is not a valid flake ID", id);
 
-        attrs.emplace("type", "indirect");
-        attrs.emplace("id", id);
-        if (rev) attrs.emplace("rev", rev->gitRev());
-        if (ref) attrs.emplace("ref", *ref);
+        // FIXME: forbid query params?
 
-        emplaceURLQueryIntoAttrs(url, attrs, {}, {});
+        Input input;
+        input.direct = false;
+        input.attrs.insert_or_assign("type", "indirect");
+        input.attrs.insert_or_assign("id", id);
+        if (rev) input.attrs.insert_or_assign("rev", rev->gitRev());
+        if (ref) input.attrs.insert_or_assign("ref", *ref);
 
-        return inputFromAttrs(attrs);
+        return input;
     }
 
     std::optional<Input> inputFromAttrs(const Attrs & attrs) const override
@@ -57,18 +63,6 @@ struct IndirectInputScheme : InputScheme
         if (!std::regex_match(id, flakeRegex))
             throw BadURL("'%s' is not a valid flake ID", id);
 
-        // TODO come up with a nicer error message for those two.
-        if (auto rev = maybeGetStrAttr(attrs, "rev")) {
-            if (!std::regex_match(*rev, revRegex)) {
-                throw BadURL("in flake '%s', '%s' is not a commit hash", id, *rev);
-            }
-        }
-        if (auto ref = maybeGetStrAttr(attrs, "ref")) {
-            if (!std::regex_match(*ref, refRegex)) {
-                throw BadURL("in flake '%s', '%s' is not a valid branch/tag name", id, *ref);
-            }
-        }
-
         Input input;
         input.direct = false;
         input.attrs = attrs;
diff --git a/src/libfetchers/mercurial.cc b/src/libfetchers/mercurial.cc
index b4150e9df..23cf7b51d 100644
--- a/src/libfetchers/mercurial.cc
+++ b/src/libfetchers/mercurial.cc
@@ -56,7 +56,12 @@ struct MercurialInputScheme : InputScheme
         Attrs attrs;
         attrs.emplace("type", "hg");
 
-        emplaceURLQueryIntoAttrs(url, attrs, {"revCount"}, {});
+        for (auto &[name, value] : url.query) {
+            if (name == "rev" || name == "ref")
+                attrs.emplace(name, value);
+            else
+                url2.query.emplace(name, value);
+        }
 
         attrs.emplace("url", url2.to_string());
 
diff --git a/src/libfetchers/tarball.cc b/src/libfetchers/tarball.cc
index 8dfdecda8..c903895e2 100644
--- a/src/libfetchers/tarball.cc
+++ b/src/libfetchers/tarball.cc
@@ -201,17 +201,29 @@ struct CurlInputScheme : InputScheme
         if (!isValidURL(_url, requireTree))
             return std::nullopt;
 
-        auto url = _url;
+        Input input;
 
-        Attrs attrs;
-        attrs.emplace("type", inputType());
+        auto url = _url;
 
         url.scheme = parseUrlScheme(url.scheme).transport;
 
-        emplaceURLQueryIntoAttrs(url, attrs, {"revCount"}, {});
+        auto narHash = url.query.find("narHash");
+        if (narHash != url.query.end())
+            input.attrs.insert_or_assign("narHash", narHash->second);
+
+        if (auto i = get(url.query, "rev"))
+            input.attrs.insert_or_assign("rev", *i);
 
-        attrs.emplace("url", url.to_string());
-        return inputFromAttrs(attrs);
+        if (auto i = get(url.query, "revCount"))
+            if (auto n = string2Int<uint64_t>(*i))
+                input.attrs.insert_or_assign("revCount", *n);
+
+        url.query.erase("rev");
+        url.query.erase("revCount");
+
+        input.attrs.insert_or_assign("type", inputType());
+        input.attrs.insert_or_assign("url", url.to_string());
+        return input;
     }
 
     std::optional<Input> inputFromAttrs(const Attrs & attrs) const override
@@ -223,7 +235,7 @@ struct CurlInputScheme : InputScheme
         std::set<std::string> allowedNames = {"type", "url", "narHash", "name", "unpack", "rev", "revCount", "lastModified"};
         for (auto & [name, value] : attrs)
             if (!allowedNames.count(name))
-                throw Error("unsupported %s input attribute '%s'. If you wanted to fetch a tarball with a query parameter, please use '{ type = \"tarball\"; url = \"...\"; }'", *type, name);
+                throw Error("unsupported %s input attribute '%s'", *type, name);
 
         Input input;
         input.attrs = attrs;