From ccbff6f4e9be44158842b338f640b4d1dbb74f64 Mon Sep 17 00:00:00 2001
From: Kornel <kornel@geekhood.net>
Date: Fri, 20 Jun 2025 17:42:47 +0100
Subject: [PATCH] Helpful error for confused cargo add arguments

---
 src/cargo/ops/cargo_add/crate_spec.rs |  12 ++-
 src/cargo/ops/cargo_add/mod.rs        | 102 ++++++++++++++++++++++++++
 2 files changed, 113 insertions(+), 1 deletion(-)

diff --git a/src/cargo/ops/cargo_add/crate_spec.rs b/src/cargo/ops/cargo_add/crate_spec.rs
index 9fb48fdb443..9a93e28d122 100644
--- a/src/cargo/ops/cargo_add/crate_spec.rs
+++ b/src/cargo/ops/cargo_add/crate_spec.rs
@@ -1,12 +1,17 @@
 //! Crate name parsing.
 
 use anyhow::Context as _;
+use thiserror::Error;
 
 use super::Dependency;
 use crate::util::toml_mut::dependency::RegistrySource;
 use crate::CargoResult;
 use cargo_util_schemas::manifest::PackageName;
 
+#[derive(Error, Debug)]
+#[error(transparent)]
+pub(crate) struct CrateSpecResolutionError(pub crate::Error);
+
 /// User-specified crate
 ///
 /// This can be a
@@ -22,7 +27,12 @@ pub struct CrateSpec {
 
 impl CrateSpec {
     /// Convert a string to a `Crate`
-    pub fn resolve(pkg_id: &str) -> CargoResult<Self> {
+    pub fn resolve(pkg_id: &str) -> Result<Self, CrateSpecResolutionError> {
+        // enables targeted add_spec_fix_suggestion
+        Self::resolve_inner(pkg_id).map_err(CrateSpecResolutionError)
+    }
+
+    fn resolve_inner(pkg_id: &str) -> CargoResult<Self> {
         let (name, version) = pkg_id
             .split_once('@')
             .map(|(n, v)| (n, Some(v)))
diff --git a/src/cargo/ops/cargo_add/mod.rs b/src/cargo/ops/cargo_add/mod.rs
index 6871f66f25a..530f90729a3 100644
--- a/src/cargo/ops/cargo_add/mod.rs
+++ b/src/cargo/ops/cargo_add/mod.rs
@@ -14,6 +14,7 @@ use cargo_util::paths;
 use cargo_util_schemas::core::PartialVersion;
 use cargo_util_schemas::manifest::PathBaseName;
 use cargo_util_schemas::manifest::RustVersion;
+use crate_spec::CrateSpecResolutionError;
 use indexmap::IndexSet;
 use itertools::Itertools;
 use toml_edit::Item as TomlItem;
@@ -104,6 +105,10 @@ pub fn add(workspace: &Workspace<'_>, options: &AddOptions<'_>) -> CargoResult<(
                     options.gctx,
                     &mut registry,
                 )
+                .map_err(|err| match err.downcast::<CrateSpecResolutionError>() {
+                    Ok(err) => add_spec_fix_suggestion(err, raw),
+                    Err(other) => other,
+                })
             })
             .collect::<CargoResult<Vec<_>>>()?
     };
@@ -1258,3 +1263,100 @@ fn precise_version(version_req: &semver::VersionReq) -> Option<String> {
         .max()
         .map(|v| v.to_string())
 }
+
+/// Help with invalid arguments to `cargo add`
+fn add_spec_fix_suggestion(err: CrateSpecResolutionError, arg: &DepOp) -> crate::Error {
+    if let Some(note) = spec_fix_suggestion_inner(arg) {
+        return anyhow::format_err!("{err}\nnote: {note}");
+    }
+    err.into()
+}
+
+fn spec_fix_suggestion_inner(arg: &DepOp) -> Option<&'static str> {
+    let spec = arg.crate_spec.as_deref()?;
+
+    let looks_like_git_url = spec.starts_with("git@")
+        || spec.starts_with("ssh:")
+        || spec.ends_with(".git")
+        || spec.starts_with("https://git"); // compromise between favoring a couple of top sites vs trying to list every git host
+
+    // check if the arg is present to avoid suggesting it redundantly
+    if arg.git.is_none() && looks_like_git_url {
+        Some("git URLs must be specified with --git <URL>")
+    } else if arg.registry.is_none()
+        && (spec.starts_with("registry+") || spec.starts_with("sparse+"))
+    {
+        Some("registy can be specified with --registry <NAME>")
+    } else if spec.contains("://") || looks_like_git_url {
+        Some("`cargo add` expects crates specified as 'name' or 'name@version', not URLs")
+    } else if arg.path.is_none() && spec.contains('/') {
+        Some("local crates can be added with --path <DIR>")
+    } else {
+        None
+    }
+}
+
+#[test]
+fn test_spec_fix_suggestion() {
+    fn dep(crate_spec: &str) -> DepOp {
+        DepOp {
+            crate_spec: Some(crate_spec.into()),
+            rename: None,
+            features: None,
+            default_features: None,
+            optional: None,
+            public: None,
+            registry: None,
+            path: None,
+            base: None,
+            git: None,
+            branch: None,
+            rev: None,
+            tag: None,
+        }
+    }
+
+    #[track_caller]
+    fn err_for(dep: &DepOp) -> String {
+        add_spec_fix_suggestion(
+            CrateSpec::resolve(dep.crate_spec.as_deref().unwrap()).unwrap_err(),
+            &dep,
+        )
+        .to_string()
+    }
+
+    for path in ["../some/path", "/absolute/path", "~/dir/Cargo.toml"] {
+        let mut dep = dep(path);
+        assert!(err_for(&dep).contains("note: local crates can be added with --path <DIR>"));
+
+        dep.path = Some(".".into());
+        assert!(!err_for(&dep).contains("--git"));
+        assert!(!err_for(&dep).contains("--registry"));
+        assert!(!err_for(&dep).contains("--path"));
+    }
+
+    assert!(err_for(&dep(
+        "registry+https://private.local:8000/index#crate@0.0.1"
+    ))
+    .contains("--registry <NAME>"));
+
+    for git_url in [
+        "git@host:dir/repo.git",
+        "https://gitlab.com/~user/crate.git",
+        "ssh://host/path",
+    ] {
+        let mut dep = dep(git_url);
+        let msg = err_for(&dep);
+        assert!(
+            msg.contains("note: git URLs must be specified with --git <URL>"),
+            "{msg} {dep:?}"
+        );
+        assert!(!err_for(&dep).contains("--path"));
+        assert!(!err_for(&dep).contains("--registry"));
+
+        dep.git = Some("true".into());
+        let msg = err_for(&dep);
+        assert!(!msg.contains("--git"));
+        assert!(msg.contains("'name@version', not URLs"), "{msg} {dep:?}");
+    }
+}