Merge pull request #13888 from NixOS/old-busted-git-url-with-tests

Old busted git url with tests

src/libutil-tests/url.cc

@@ -14,8 +14,9 @@ using HostType = Authority::HostType;
 
 struct FixGitURLParam
 {
-    std::string_view input;
+    std::string input;
-    std::string_view expected;
+    std::string expected;
+    ParsedURL parsed;
 };
 
 std::ostream & operator<<(std::ostream & os, const FixGitURLParam & param)
@@ -32,42 +33,131 @@ INSTANTIATE_TEST_SUITE_P(
     ::testing::Values(
         // https://github.com/NixOS/nix/issues/5958
         // Already proper URL with git+ssh
-        FixGitURLParam{"git+ssh://user@domain:1234/path", "git+ssh://user@domain:1234/path"},
+        FixGitURLParam{
+            .input = "git+ssh://user@domain:1234/path",
+            .expected = "git+ssh://user@domain:1234/path",
+            .parsed =
+                ParsedURL{
+                    .scheme = "git+ssh",
+                    .authority =
+                        ParsedURL::Authority{
+                            .host = "domain",
+                            .user = "user",
+                            .port = 1234,
+                        },
+                    .path = {"", "path"},
+                },
+        },
         // SCP-like URL (rewritten to ssh://)
-        FixGitURLParam{"git@github.com:owner/repo.git", "ssh://git@github.com/owner/repo.git"},
+        FixGitURLParam{
-        // SCP-like URL (no user)
+            .input = "git@github.com:owner/repo.git",
-        FixGitURLParam{"github.com:owner/repo.git", "ssh://github.com/owner/repo.git"},
+            .expected = "ssh://git@github.com/owner/repo.git",
-        // SCP-like URL (leading slash)
+            .parsed =
-        FixGitURLParam{"github.com:/owner/repo.git", "ssh://github.com/owner/repo.git"},
+                ParsedURL{
+                    .scheme = "ssh",
+                    .authority =
+                        ParsedURL::Authority{
+                            .host = "github.com",
+                            .user = "git",
+                        },
+                    .path = {"", "owner", "repo.git"},
+                },
+        },
         // Absolute path (becomes file:)
-        FixGitURLParam{"/home/me/repo", "file:///home/me/repo"},
+        FixGitURLParam{
-        // Relative path (becomes file:// absolute)
+            .input = "/home/me/repo",
-        FixGitURLParam{"relative/repo", "file:///relative/repo"},
+            .expected = "file:///home/me/repo",
+            .parsed =
+                ParsedURL{
+                    .scheme = "file",
+                    .authority = ParsedURL::Authority{},
+                    .path = {"", "home", "me", "repo"},
+                },
+        },
         // Already file: scheme
-        // NOTE: This is not valid technically as it's not absolute
+        // NOTE: Git/SCP treat this as a `<hostname>:<path>`, so we are
-        FixGitURLParam{"file:/var/repos/x", "file:/var/repos/x"},
+        // failing to "fix up" this case.
+        FixGitURLParam{
+            .input = "file:/var/repos/x",
+            .expected = "file:/var/repos/x",
+            .parsed =
+                ParsedURL{
+                    .scheme = "file",
+                    .authority = std::nullopt,
+                    .path = {"", "var", "repos", "x"},
+                },
+        },
         // IPV6 test case
-        FixGitURLParam{"user@[2001:db8:1::2]:/home/file", "ssh://user@[2001:db8:1::2]/home/file"}));
+        FixGitURLParam{
+            .input = "user@[2001:db8:1::2]:/home/file",
+            .expected = "ssh://user@[2001:db8:1::2]//home/file",
+            .parsed =
+                ParsedURL{
+                    .scheme = "ssh",
+                    .authority =
+                        ParsedURL::Authority{
+                            .hostType = HostType::IPv6,
+                            .host = "2001:db8:1::2",
+                            .user = "user",
+                        },
+                    .path = {"", "", "home", "file"},
+                },
+        }));
 
 TEST_P(FixGitURLTestSuite, parsesVariedGitUrls)
 {
     auto & p = GetParam();
-    const auto actual = fixGitURL(p.input).to_string();
+    const auto actual = fixGitURL(p.input);
-    EXPECT_EQ(actual, p.expected);
+    EXPECT_EQ(actual, p.parsed);
+    EXPECT_EQ(actual.to_string(), p.expected);
 }
 
-TEST_P(FixGitURLTestSuite, fixGitIsIdempotent)
+TEST(FixGitURLTestSuite, scpLikeNoUserParsesPoorly)
 {
-    auto & p = GetParam();
+    // SCP-like URL (no user)
-    const auto actual = fixGitURL(p.expected).to_string();
+
-    EXPECT_EQ(actual, p.expected);
+    // Cannot "to_string" this because has illegal path not starting
+    // with `/`.
+    EXPECT_EQ(
+        fixGitURL("github.com:owner/repo.git"),
+        (ParsedURL{
+            .scheme = "file",
+            .authority = ParsedURL::Authority{},
+            .path = {"github.com:owner", "repo.git"},
+        }));
 }
 
-TEST_P(FixGitURLTestSuite, fixGitOutputParses)
+TEST(FixGitURLTestSuite, scpLikePathLeadingSlashParsesPoorly)
 {
-    auto & p = GetParam();
+    // SCP-like URL (no user)
-    const auto parsed = fixGitURL(p.expected);
+
-    EXPECT_EQ(parseURL(parsed.to_string()), parsed);
+    // Cannot "to_string" this because has illegal path not starting
+    // with `/`.
+    EXPECT_EQ(
+        fixGitURL("github.com:/owner/repo.git"),
+        (ParsedURL{
+            .scheme = "file",
+            .authority = ParsedURL::Authority{},
+            .path = {"github.com:", "owner", "repo.git"},
+        }));
+}
+
+TEST(FixGitURLTestSuite, relativePathParsesPoorly)
+{
+    // Relative path (becomes file:// absolute)
+
+    // Cannot "to_string" this because has illegal path not starting
+    // with `/`.
+    EXPECT_EQ(
+        fixGitURL("relative/repo"),
+        (ParsedURL{
+            .scheme = "file",
+            .authority =
+                ParsedURL::Authority{
+                    .hostType = ParsedURL::Authority::HostType::Name,
+                    .host = "",
+                },
+            .path = {"relative", "repo"}}));
 }
 
 TEST(parseURL, parsesSimpleHttpUrl)

src/libutil/include/nix/util/url.hh

@@ -327,23 +327,10 @@ struct ParsedUrlScheme
 
 ParsedUrlScheme parseUrlScheme(std::string_view scheme);
 
-/**
+/* Detects scp-style uris (e.g. git@github.com:NixOS/nix) and fixes
- * Normalize a Git remote string from various styles into a URL-like form.
+   them by removing the `:` and assuming a scheme of `ssh://`. Also
- * Input forms handled:
+   changes absolute paths into file:// URLs. */
- *   1) SCP-style SSH syntax:    "[user@]host:path"      -> "ssh://user@host/path"
+ParsedURL fixGitURL(const std::string & url);
- *   2) Already "file:" URLs:    "file:/abs/or/rel"      -> unchanged
- *   3) Bare paths / filenames:  "src/repo" or "/abs"    -> "file:src/repo" or "file:/abs"
- *   4) Anything with "://":     treated as a proper URL -> unchanged
- *
- *  Note: for the scp-style, as they are converted to ssh-form, all paths are assumed to
- *  then be absolute whereas in programs like git, they retain the scp form which allows
- *  relative paths.
- *
- *  Additionally, if no url can be determined, it is returned as a file:// URI.
- *  If the url does not start with a leading slash, one will be added since there are no
- *  relative path URIs.
- */
-ParsedURL fixGitURL(std::string_view url);
 
 /**
  * Whether a string is valid as RFC 3986 scheme name.

src/libutil/url.cc

@@ -408,78 +408,22 @@ ParsedUrlScheme parseUrlScheme(std::string_view scheme)
     };
 }
 
-struct ScpLike
+ParsedURL fixGitURL(const std::string & url)
 {
-    ParsedURL::Authority authority;
+    std::regex scpRegex("([^/]*)@(.*):(.*)");
-    std::string_view path;
+    if (!hasPrefix(url, "/") && std::regex_match(url, scpRegex))
-};
+        return parseURL(std::regex_replace(url, scpRegex, "ssh://$1@$2/$3"));
-
+    if (hasPrefix(url, "file:"))
-/**
+        return parseURL(url);
- * Parse a scp url. This is a helper struct for fixGitURL.
+    if (url.find("://") == std::string::npos) {
- * This is needed since we support scp-style urls for git urls.
- * https://git-scm.com/book/ms/v2/Git-on-the-Server-The-Protocols
- *
- * A good reference is libgit2 also allows scp style
- * https://github.com/libgit2/libgit2/blob/58d9363f02f1fa39e46d49b604f27008e75b72f2/src/util/net.c#L806
- */
-static std::optional<ScpLike> parseScp(const std::string_view s) noexcept
-{
-    if (s.empty() || s.front() == '/')
-        return std::nullopt;
-
-    // Find the colon that separates host from path.
-    // Find the right-most since ipv6 has colons
-    const auto colon = s.rfind(':');
-    if (colon == std::string_view::npos)
-        return std::nullopt;
-
-    // Split head:[path]
-    const auto head = s.substr(0, colon);
-    const auto path = s.substr(colon + 1);
-
-    if (head.empty())
-        return std::nullopt;
-
-    return ScpLike{
-        .authority = ParsedURL::Authority::parse(head),
-        .path = path,
-    };
-}
-
-ParsedURL fixGitURL(const std::string_view url)
-{
-    try {
-        if (auto parsed = parseURL(url); parsed.scheme == "file" || parsed.authority)
-            return parsed;
-    } catch (BadURL &) {
-    }
-
-    // if the url does not start with forward slash, add one
-    auto splitMakeAbs = [&](std::string_view pathS) {
-        std::vector<std::string> path;
-
-        if (!hasPrefix(pathS, "/")) {
-            path.emplace_back("");
-        }
-        splitStringInto(path, pathS, "/");
-
-        return path;
-    };
-
-    if (auto scp = parseScp(url)) {
-        return ParsedURL{
-            .scheme = "ssh",
-            .authority = std::move(scp->authority),
-            .path = splitMakeAbs(scp->path),
-        };
-    }
-
         return ParsedURL{
             .scheme = "file",
             .authority = ParsedURL::Authority{},
-        .path = splitMakeAbs(url),
+            .path = splitString<std::vector<std::string>>(url, "/"),
         };
     }
+    return parseURL(url);
+}
 
 // https://www.rfc-editor.org/rfc/rfc3986#section-3.1
 bool isValidSchemeName(std::string_view s)