Auto merge of #12244 - krs98:fix-relative-git-submodules, r=weihanglo

fix: git submodules with relative urls

### What does this PR try to resolve?

Fixes #12151.

When recursing submodules, the url of the parent remote was being passed to `update_submodules` instead of the child remote url. This caused Cargo to try to add the wrong submodule.

### How should we test and review this PR?

A test case is added in the first commit. The second one renames the `url` variable as suggested in the issue. The third includes the changes to fix the issue. The last one includes a minor refactor where a redundant `match` expr is removed.

Author: bors

src/cargo/sources/git/utils.rs

@@ -444,7 +444,9 @@ impl<'a> GitCheckout<'a> {
             // See [`git submodule add`] documentation.
             //
             // [`git submodule add`]: https://git-scm.com/docs/git-submodule
-            let url = if child_url_str.starts_with("./") || child_url_str.starts_with("../") {
+            let child_remote_url = if child_url_str.starts_with("./")
+                || child_url_str.starts_with("../")
+            {
                 let mut new_parent_remote_url = parent_remote_url.clone();
 
                 let mut new_path = Cow::from(parent_remote_url.path());
@@ -453,17 +455,19 @@ impl<'a> GitCheckout<'a> {
                 }
                 new_parent_remote_url.set_path(&new_path);
 
-                match new_parent_remote_url.join(child_url_str) {
-                    Ok(x) => x.to_string(),
-                    Err(err) => Err(err).with_context(|| {
+                new_parent_remote_url.join(child_url_str).with_context(|| {
+                    format!(
+                        "failed to parse relative child submodule url `{child_url_str}` \
+                        using parent base url `{new_parent_remote_url}`"
+                    )
+                })?
+            } else {
+                Url::parse(child_url_str).with_context(|| {
+                        let child_module_name = child.name().unwrap_or("");
                         format!(
-                            "failed to parse relative child submodule url `{}` using parent base url `{}`",
-                            child_url_str, new_parent_remote_url
+                            "failed to parse url for submodule `{child_module_name}`: `{child_url_str}`"
                         )
-                    })?,
-                }
-            } else {
-                child_url_str.to_string()
+                    })?
             };
 
             // A submodule which is listed in .gitmodules but not actually
@@ -484,7 +488,7 @@ impl<'a> GitCheckout<'a> {
             let mut repo = match head_and_repo {
                 Ok((head, repo)) => {
                     if child.head_id() == head {
-                        return update_submodules(&repo, cargo_config, parent_remote_url);
+                        return update_submodules(&repo, cargo_config, &child_remote_url);
                     }
                     repo
                 }
@@ -498,10 +502,10 @@ impl<'a> GitCheckout<'a> {
             let reference = GitReference::Rev(head.to_string());
             cargo_config
                 .shell()
-                .status("Updating", format!("git submodule `{}`", url))?;
+                .status("Updating", format!("git submodule `{}`", child_remote_url))?;
             fetch(
                 &mut repo,
-                &url,
+                child_remote_url.as_str(),
                 &reference,
                 cargo_config,
                 RemoteKind::GitDependency,
@@ -510,13 +514,13 @@ impl<'a> GitCheckout<'a> {
                 format!(
                     "failed to fetch submodule `{}` from {}",
                     child.name().unwrap_or(""),
-                    url
+                    child_remote_url
                 )
             })?;
 
             let obj = repo.find_object(head, None)?;
             reset(&repo, &obj, cargo_config)?;
-            update_submodules(&repo, cargo_config, parent_remote_url)
+            update_submodules(&repo, cargo_config, &child_remote_url)
         }
     }
 }

tests/testsuite/git.rs

@@ -3619,3 +3619,71 @@ fn cleans_temp_pack_files() {
     p.cargo("generate-lockfile").run();
     assert!(!tmp_path.exists());
 }
+
+#[cargo_test]
+fn different_user_relative_submodules() {
+    let user1_git_project = git::new("user1/dep1", |project| {
+        project
+            .file("Cargo.toml", &basic_lib_manifest("dep1"))
+            .file("src/lib.rs", "")
+    });
+
+    let user2_git_project = git::new("user2/dep1", |project| {
+        project
+            .file("Cargo.toml", &basic_lib_manifest("dep1"))
+            .file("src/lib.rs", "")
+    });
+    let user2_git_project2 = git::new("user2/dep2", |project| {
+        project
+            .file("Cargo.toml", &basic_lib_manifest("dep1"))
+            .file("src/lib.rs", "")
+    });
+
+    let user2_repo = git2::Repository::open(&user2_git_project.root()).unwrap();
+    let url = "../dep2";
+    git::add_submodule(&user2_repo, url, Path::new("dep2"));
+    git::commit(&user2_repo);
+
+    let user1_repo = git2::Repository::open(&user1_git_project.root()).unwrap();
+    let url = user2_git_project.url();
+    git::add_submodule(&user1_repo, url.as_str(), Path::new("user2/dep1"));
+    git::commit(&user1_repo);
+
+    let project = project()
+        .file(
+            "Cargo.toml",
+            &format!(
+                r#"
+                    [package]
+                    name = "foo" 
+                    version = "0.5.0"
+
+                    [dependencies.dep1]
+                    git = '{}'
+                "#,
+                user1_git_project.url()
+            ),
+        )
+        .file("src/main.rs", &main_file(r#""hello""#, &[]))
+        .build();
+
+    project
+        .cargo("build")
+        .with_stderr(&format!(
+            "\
+[UPDATING] git repository `{}`
+[UPDATING] git submodule `{}`
+[UPDATING] git submodule `{}`
+[COMPILING] dep1 v0.5.0 ({}#[..])
+[COMPILING] foo v0.5.0 ([CWD])
+[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]
+",
+            path2url(&user1_git_project.root()),
+            path2url(&user2_git_project.root()),
+            path2url(&user2_git_project2.root()),
+            path2url(&user1_git_project.root()),
+        ))
+        .run();
+
+    assert!(project.bin("foo").is_file());
+}