Improve forge auto-detection and URL handling

CHANGELOG.md

@@ -3,6 +3,7 @@
 ## Unreleased
 
 - `npins show` now accepts a list of pin entries to show instead of always showing the complete list (https://github.com/andir/npins/pull/190)
+- Tweaked forge auto-detection for `add git` (https://github.com/andir/npins/pull/202)
 
 ## 0.4.0
 

libnpins/src/git.rs

@@ -136,40 +136,65 @@ impl Repository {
     pub async fn git_auto(url: Url) -> Self {
         let url2 = url.clone();
         match (url.scheme(), url.domain()) {
-            ("http" | "https", Some("github.com")) => Self::github_from_url(url),
-            ("http" | "https", Some("gitlab.com")) => Self::gitlab_from_url(url),
-            ("http" | "https", Some("codeberg.org")) => Self::forgejo_from_url(url),
+            ("http" | "https", Some("github.com")) => {
+                log::debug!("Trying to parse URL as GitHub repository based on the domain name (github.com)");
+                Self::github_from_url(url)
+                    .inspect(|_| log::info!("Auto-detected GitHub repository (github.com)"))
+            },
+            ("http" | "https", Some("gitlab.com")) => {
+                log::debug!("Trying to parse URL as GitLab repository based on the domain name (gitlab.com)");
+                Self::gitlab_from_url(url)
+                    .inspect(|_| log::info!("Auto-detected GitLab repository (gitlab.com)"))
+            },
+            ("http" | "https", Some("codeberg.org")) => {
+                log::debug!("Trying to parse URL as Forgejo repository based on the domain name (codeberg.org)");
+                Self::forgejo_from_url(url)
+                    .inspect(|_| log::info!("Auto-detected Forgejo repository (codeberg.org)"))
+            },
             ("http" | "https", _) => Self::probe_forge(url).await,
             _ => None,
         }
-        .unwrap_or(Self::git(url2))
+        .unwrap_or_else(|| {
+            log::info!("No forge was auto-detected, treating as plain git repository");
+            Self::git(url2)
+        })
     }
 
     ///
     /// Takes in a URL of unknown forge and tries to determine which forge the hoster is
     /// And then parse the url into the according Repository Variant
     async fn probe_forge(url: Url) -> Option<Self> {
-        async fn probe(mut test_url: Url, path: &str) -> Result<()> {
-            test_url.set_path(path);
-            let _: serde_json::Value = get_and_deserialize(test_url).await?;
-            Ok(())
-        }
+        log::debug!("Probing {url} for Forgejo and GitLab API endpoints");
 
         /* We probe some known endpoints unique to the respective GitLab and Forgejo APIs to determine if a corresponding server is running */
         let distinct_api_endpoints = [
             (
+                "GitLab",
                 "/api/v4/projects",
                 Self::gitlab_from_url as fn(Url) -> Option<Self>,
             ),
             (
+                "Forgejo",
                 "/api/v1/settings/api",
                 Self::forgejo_from_url as fn(Url) -> Option<Self>,
             ),
         ];
 
-        for (path, func) in distinct_api_endpoints {
-            if probe(url.clone(), path).await.is_ok() {
-                return func(url);
+        for (forge_type, path, func) in distinct_api_endpoints {
+            let probe = |mut test_url: Url| async {
+                test_url.set_path(path);
+                log::debug!("Probing {test_url} to check for {forge_type}");
+                let _: serde_json::Value = get_and_deserialize(test_url).await?;
+                Ok::<(), anyhow::Error>(())
+            };
+
+            if probe(url.clone()).await.is_ok() {
+                return func(url.clone()).inspect(|_| {
+                    log::info!(
+                        "Auto-detected {forge_type} repository ({})",
+                        url.domain().unwrap()
+                    )
+                });
             }
         }
         None
@@ -196,10 +221,12 @@ impl Repository {
                 .map(|split| split.collect::<Vec<_>>())
                 .as_deref(),
         ) {
-            ("http" | "https", Some("github.com"), Some([owner, repo])) => Some(Self::github(
-                owner.to_string(),
-                repo.strip_suffix(".git").unwrap_or(repo).to_string(),
-            )),
+            ("http" | "https", Some("github.com"), Some([owner, repo] | [owner, repo, ""])) => {
+                Some(Self::github(
+                    owner.to_string(),
+                    repo.strip_suffix(".git").unwrap_or(repo).to_string(),
+                ))
+            },
             _ => None,
         }
     }
@@ -220,11 +247,13 @@ impl Repository {
                 .map(|split| split.collect::<Vec<_>>())
                 .as_deref(),
         ) {
-            ("http" | "https", Some(_domain), Some([owner, repo])) => Some(Self::forgejo(
-                strip_url(url.clone()),
-                owner.to_string(),
-                repo.to_string(),
-            )),
+            ("http" | "https", Some(_domain), Some([owner, repo] | [owner, repo, ""])) => {
+                Some(Self::forgejo(
+                    strip_url(url.clone()),
+                    owner.to_string(),
+                    repo.strip_suffix(".git").unwrap_or(repo).to_string(),
+                ))
+            },
             _ => None,
         }
     }
@@ -243,11 +272,12 @@ impl Repository {
     pub fn gitlab_from_url(url: Url) -> Option<Self> {
         let authority = strip_url(url.clone());
         match (url.scheme(), url.domain(), url.path()) {
-            ("http" | "https", Some(_domain), repo_path) => Some(Self::gitlab(
-                repo_path
-                    .strip_suffix(".git")
-                    .unwrap_or(repo_path)
-                    .to_string(),
+            ("http" | "https", Some(_domain), mut repo_path) => Some(Self::gitlab(
+                {
+                    repo_path = repo_path.strip_suffix("/").unwrap_or(repo_path);
+                    repo_path = repo_path.strip_suffix(".git").unwrap_or(repo_path);
+                    repo_path.to_string()
+                },
                 Some(authority),
                 None,
             )),
@@ -554,8 +584,7 @@ impl Updatable for GitReleasePin {
             self.pre_releases,
             version_upper_bound.as_ref(),
             self.release_prefix.as_deref(),
-        )
-            .ok_or_else(|| anyhow::format_err!("Repository has no matching release tags"))?;
+        ).context("Repository has no matching release tags")?;
 
         // If we have a release prefix strip it from the previous version for semver comparison.
         // If the old version didn't have a prefix we keep it as is.
@@ -677,7 +706,7 @@ async fn fetch_remote(url: &str, args: &[&str]) -> Result<Vec<RemoteInfo>> {
             .map(|line| {
                 let (revision, ref_) = line
                     .split_once('\t')
-                    .ok_or_else(|| anyhow::format_err!("Output line contains no '\\t'"))?;
+                    .context("Output line contains no '\\t'")?;
                 anyhow::ensure!(
                     !ref_.contains('\t'),
                     "Output line contains more than one '\\t'"
@@ -710,9 +739,8 @@ pub async fn fetch_ref(repo: &Url, ref_: impl AsRef<str>) -> Result<RemoteInfo>
     /* git ls-remote always postfix-matches the ref like a glob, but we want an exact match.
      * See https://github.com/andir/npins/issues/142
      */
-    remotes.into_iter().find(|r| r.ref_ == ref_).ok_or_else(
-        || anyhow::format_err!("git ls-remote output does not contain the requested remote '{}'. This should not have happened!", ref_)
-    )
+    remotes.into_iter().find(|r| r.ref_ == ref_)
+        .with_context(|| "git ls-remote output does not contain the requested remote '{ref_}'. This should not have happened!")
 }
 
 /// Get the revision for a branch
@@ -1337,6 +1365,13 @@ mod test {
                 repo: "Nixpkgs".into()
             },
         );
+        assert_eq!(
+            Repository::git_auto("https://github.com/NixOS/Nixpkgs/".parse().unwrap()).await,
+            Repository::GitHub {
+                owner: "NixOS".into(),
+                repo: "Nixpkgs".into()
+            },
+        );
         assert_eq!(
             Repository::git_auto("https://github.com/NixOS/Nixpkgs.git".parse().unwrap()).await,
             Repository::GitHub {
@@ -1366,7 +1401,20 @@ mod test {
             .await,
             Repository::GitLab {
                 server: "https://gitlab.gnome.org".parse().unwrap(),
-                repo_path: "/GNOME/gnome-control-center/".to_string(),
+                repo_path: "/GNOME/gnome-control-center".to_string(),
+                private_token: None
+            },
+        );
+        assert_eq!(
+            Repository::git_auto(
+                "https://gitlab.gnome.org/GNOME/gnome-control-center.git"
+                    .parse()
+                    .unwrap()
+            )
+            .await,
+            Repository::GitLab {
+                server: "https://gitlab.gnome.org".parse().unwrap(),
+                repo_path: "/GNOME/gnome-control-center".to_string(),
                 private_token: None
             },
         );
@@ -1384,5 +1432,26 @@ mod test {
                 repo: "lix".to_string()
             },
         );
+        assert_eq!(
+            Repository::git_auto("https://git.lix.systems/lix-project/lix/".parse().unwrap()).await,
+            Repository::Forgejo {
+                server: "https://git.lix.systems".parse().unwrap(),
+                owner: "lix-project".to_string(),
+                repo: "lix".to_string()
+            },
+        );
+        assert_eq!(
+            Repository::git_auto(
+                "https://git.lix.systems/lix-project/lix.git"
+                    .parse()
+                    .unwrap()
+            )
+            .await,
+            Repository::Forgejo {
+                server: "https://git.lix.systems".parse().unwrap(),
+                owner: "lix-project".to_string(),
+                repo: "lix".to_string()
+            },
+        );
     }
 }

libnpins/src/lib.rs

@@ -3,6 +3,7 @@
 //! Currently, it pretty much exposes the internals of the CLI 1:1, but in the future
 //! this is supposed to evolve into a more standalone library.
 
+use anyhow::Context;
 use diff::{Diff, OptionExt};
 use nix_compat::nixhash::NixHash;
 use reqwest::IntoUrl;
@@ -194,7 +195,7 @@ macro_rules! mkPin {
                 Ok(match self {
                     $(Self::$name { input, version, hashes, .. } => {
                         let version = version.as_ref()
-                            .ok_or_else(|| anyhow::format_err!("No version information available, call `update` first or manually set one"))?;
+                            .context("No version information available, call `update` first or manually set one")?;
                         /* Use very explicit syntax to force the correct types and get good compile errors */
                         let new_hashes = <$input_name as Updatable>::fetch(input, &version).await?;
                         hashes.insert_diffed(new_hashes)

libnpins/src/pypi.rs

@@ -75,7 +75,7 @@ impl Updatable for Pin {
                     .filter(|version| version < &version_upper_bound)
                     /* Get the latest version */
                     .max()
-                    .ok_or_else(|| anyhow::format_err!("No matching versions found"))?
+                    .context("No matching versions found")?
                     .to_string()
             },
             /* Simply take latest */
@@ -119,24 +119,21 @@ impl Updatable for Pin {
         let mut latest_source: PyPiUrlMetadata = metadata
             .releases
             .remove(&version.version)
-            .ok_or_else(|| {
+            .with_context(|| {
                 anyhow::format_err!("Could not find requested version {}", version.version)
             })?
             .into_iter()
             /* Of all files for the latest release, we only care about source tarballs */
             .find(|file_meta| file_meta.python_version == "source")
-            .ok_or_else(|| {
-                anyhow::format_err!("Unsupported package: must contain some \"source\" download",)
-            })?;
+            .context("Unsupported package: must contain some \"source\" download")?;
 
-        let hash_str = latest_source.digests.remove("sha256").ok_or_else(|| {
-            anyhow::format_err!(
-                "JSON metadata is invalid: must contain a `sha256` entry within `digests`",
-            )
-        })?;
+        let hash_str = latest_source
+            .digests
+            .remove("sha256")
+            .context("JSON metadata is invalid: must contain a `sha256` entry within `digests`")?;
 
         let hash = NixHash::from_str(&hash_str, Some(nixhash::HashAlgo::Sha256))
-            .with_context(|| "failed to convert to NixHash")?;
+            .context("failed to convert to NixHash")?;
 
         Ok(GenericUrlHashes {
             hash,

libnpins/src/versions.rs

@@ -52,7 +52,7 @@ pub fn upgrade(mut pins_raw: Map<String, Value>, path: &Path) -> Result<Value> {
     let version = pins_raw
         .get("version")
         .and_then(Value::as_u64)
-        .ok_or_else(|| {
+        .with_context(|| {
             anyhow::format_err!(
                 "{} must contain a numeric version field at the top level",
                 path.display()
@@ -71,14 +71,14 @@ pub fn upgrade(mut pins_raw: Map<String, Value>, path: &Path) -> Result<Value> {
         let pins = pins_raw
             .get_mut("pins")
             .and_then(Value::as_object_mut)
-            .ok_or_else(|| {
+            .with_context(|| {
                 anyhow::format_err!("'{}' must contain a `pins` object", path.display())
             })?;
         for (name, pin) in pins.iter_mut() {
             update_pin_fn(
                 name,
                 pin.as_object_mut()
-                    .ok_or_else(|| anyhow::format_err!("Pin {} must be an object", name))?,
+                    .with_context(|| anyhow::format_err!("Pin {} must be an object", name))?,
             )
             .context(anyhow::format_err!("Pin {} could not be upgraded", name))?;
         }

src/main.rs

@@ -212,7 +212,7 @@ impl GitLabAddOpts {
         Ok((
             Some(self.repo_path
                 .last()
-                .ok_or_else(|| anyhow::format_err!("GitLab repository path must at least have one element (usually two: owner, repo)"))?
+                .context("GitLab repository path must at least have one element (usually two: owner, repo)")?
                 .clone()),
             self.more.add(repository)?,
         ))
@@ -280,15 +280,16 @@ impl GitAddOpts {
         let name = name.strip_suffix(".git").unwrap_or(&name);
 
         use git::Repository;
-        let url2 = url.clone();
         let repository = match self.forge {
-            GitForgeOpts::Auto => Some(Repository::git_auto(url).await),
-            GitForgeOpts::None => Some(Repository::git(url)),
-            GitForgeOpts::Github => Repository::github_from_url(url),
-            GitForgeOpts::Gitlab => Repository::gitlab_from_url(url),
-            GitForgeOpts::Forgejo => Repository::forgejo_from_url(url),
-        }
-        .unwrap_or(Repository::git(url2));
+            GitForgeOpts::Auto => Repository::git_auto(url).await,
+            GitForgeOpts::None => Repository::git(url),
+            GitForgeOpts::Github => Repository::github_from_url(url)
+                .context("Could not parse the URL as GitHub repository")?,
+            GitForgeOpts::Gitlab => Repository::gitlab_from_url(url)
+                .context("Could not parse the URL as GitLab repository")?,
+            GitForgeOpts::Forgejo => Repository::forgejo_from_url(url)
+                .context("Could not parse the URL as Forgejo repository")?,
+        };
 
         Ok((Some(name.to_owned()), self.more.add(repository)?))
     }
@@ -1069,7 +1070,7 @@ impl Opts {
         ) -> Result<()> {
             let pin = pin
                 .or_else(|| niv.get(name))
-                .ok_or_else(|| anyhow::format_err!("Pin '{}' not found in sources.json", name))?;
+                .with_context(|| anyhow::format_err!("Pin '{name}' not found in sources.json"))?;
             anyhow::ensure!(
                 !npins.pins.contains_key(name),
                 "Pin '{}' exists in both files, this is a collision. Please delete the entry in one of the files.",
@@ -1155,7 +1156,7 @@ impl Opts {
         ) -> Result<()> {
             let pin = nodes
                 .get(name)
-                .ok_or_else(|| anyhow::format_err!("Pin '{}' not found in flake.lock", name))?;
+                .with_context(|| anyhow::format_err!("Pin '{name}' not found in flake.lock"))?;
             anyhow::ensure!(
                 !npins.pins.contains_key(name),
                 "Pin '{}' exists in both files, this is a collision. Please delete the entry in one of the files.",