From 942f7f9c5937a4518746e3439e8e293abd7444cd Mon Sep 17 00:00:00 2001 From: x-hgg-x <39058530+x-hgg-x@users.noreply.github.com> Date: Sat, 17 Sep 2022 23:09:11 +0200 Subject: [PATCH 1/4] Add support for relative git submodule paths --- src/cargo/sources/git/source.rs | 3 +- src/cargo/sources/git/utils.rs | 81 ++++++++++++++++++++++++++------- tests/testsuite/git.rs | 67 +++++++++++++++++++++++++++ 3 files changed, 134 insertions(+), 17 deletions(-) diff --git a/src/cargo/sources/git/source.rs b/src/cargo/sources/git/source.rs index 4644a3ac710..d09d5271627 100644 --- a/src/cargo/sources/git/source.rs +++ b/src/cargo/sources/git/source.rs @@ -191,7 +191,8 @@ impl<'cfg> Source for GitSource<'cfg> { .join("checkouts") .join(&self.ident) .join(short_id.as_str()); - db.copy_to(actual_rev, &checkout_path, self.config)?; + let parent_remote_url = self.url(); + db.copy_to(actual_rev, &checkout_path, self.config, parent_remote_url)?; let source_id = self.source_id.with_precise(Some(actual_rev.to_string())); let path_source = PathSource::new_recursive(&checkout_path, source_id, self.config); diff --git a/src/cargo/sources/git/utils.rs b/src/cargo/sources/git/utils.rs index 17fef7ca0f9..10d50e1c002 100644 --- a/src/cargo/sources/git/utils.rs +++ b/src/cargo/sources/git/utils.rs @@ -17,7 +17,7 @@ use std::path::{Path, PathBuf}; use std::process::Command; use std::str; use std::time::{Duration, Instant}; -use url::Url; +use url::{ParseError, Url}; fn serialize_str(t: &T, s: S) -> Result where @@ -151,6 +151,7 @@ impl GitDatabase { rev: git2::Oid, dest: &Path, cargo_config: &Config, + parent_remote_url: &Url, ) -> CargoResult> { // If the existing checkout exists, and it is fresh, use it. // A non-fresh checkout can happen if the checkout operation was @@ -164,7 +165,7 @@ impl GitDatabase { Some(co) => co, None => GitCheckout::clone_into(dest, self, rev, cargo_config)?, }; - checkout.update_submodules(cargo_config)?; + checkout.update_submodules(cargo_config, parent_remote_url)?; Ok(checkout) } @@ -322,19 +323,25 @@ impl<'a> GitCheckout<'a> { Ok(()) } - fn update_submodules(&self, cargo_config: &Config) -> CargoResult<()> { - return update_submodules(&self.repo, cargo_config); + fn update_submodules(&self, cargo_config: &Config, parent_remote_url: &Url) -> CargoResult<()> { + return update_submodules(&self.repo, cargo_config, parent_remote_url); - fn update_submodules(repo: &git2::Repository, cargo_config: &Config) -> CargoResult<()> { + fn update_submodules( + repo: &git2::Repository, + cargo_config: &Config, + parent_remote_url: &Url, + ) -> CargoResult<()> { debug!("update submodules for: {:?}", repo.workdir().unwrap()); for mut child in repo.submodules()? { - update_submodule(repo, &mut child, cargo_config).with_context(|| { - format!( - "failed to update submodule `{}`", - child.name().unwrap_or("") - ) - })?; + update_submodule(repo, &mut child, cargo_config, parent_remote_url).with_context( + || { + format!( + "failed to update submodule `{}`", + child.name().unwrap_or("") + ) + }, + )?; } Ok(()) } @@ -343,9 +350,11 @@ impl<'a> GitCheckout<'a> { parent: &git2::Repository, child: &mut git2::Submodule<'_>, cargo_config: &Config, + parent_remote_url: &Url, ) -> CargoResult<()> { child.init(false)?; - let url = child.url().ok_or_else(|| { + + let child_url_str = child.url().ok_or_else(|| { anyhow::format_err!("non-utf8 url for submodule {:?}?", child.path()) })?; @@ -355,12 +364,52 @@ impl<'a> GitCheckout<'a> { "Skipping", format!( "git submodule `{}` due to update strategy in .gitmodules", - url + child_url_str ), )?; return Ok(()); } + // There are a few possible cases here: + // 1. Submodule URL is not relative. + // Happy path, Url is just the submodule url. + // 2. Submodule URL is relative and does specify ssh/https/file/etc. + // We combine the parent path and relative path. + // 3. Submodule URL is relative and does not specify ssh/https/file/etc. + // In which case we fail to parse with the error ParseError::RelativeUrlWithoutBase. + // We then combine the relative url with the host/protocol from the parent url. + // 4. We fail to parse submodule url for some reason. + // Error. Gets passed up. + + let join_urls = || { + let path = parent_remote_url.path(); + let mut new_parent_remote_url = parent_remote_url.clone(); + new_parent_remote_url.set_path(&format!("{}/", path)); + match new_parent_remote_url.join(child_url_str) { + Ok(x) => Ok(x.to_string()), + Err(err) => { + Err(err).with_context(|| format!("Failed to parse child submodule url")) + } + } + }; + + let url = match Url::parse(child_url_str) { + Ok(child_url) => { + if Path::new(child_url.path()).is_relative() { + join_urls()? + } else { + child_url.to_string() + } + } + Err(ParseError::RelativeUrlWithoutBase) => join_urls()?, + Err(err) => { + return Err(anyhow::format_err!( + "Error parsing submodule url: {:?}?", + err + )); + } + }; + // A submodule which is listed in .gitmodules but not actually // checked out will not have a head id, so we should ignore it. let head = match child.head_id() { @@ -379,7 +428,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); + return update_submodules(&repo, cargo_config, parent_remote_url); } repo } @@ -394,7 +443,7 @@ impl<'a> GitCheckout<'a> { cargo_config .shell() .status("Updating", format!("git submodule `{}`", url))?; - fetch(&mut repo, url, &reference, cargo_config).with_context(|| { + fetch(&mut repo, &url, &reference, cargo_config).with_context(|| { format!( "failed to fetch submodule `{}` from {}", child.name().unwrap_or(""), @@ -404,7 +453,7 @@ impl<'a> GitCheckout<'a> { let obj = repo.find_object(head, None)?; reset(&repo, &obj, cargo_config)?; - update_submodules(&repo, cargo_config) + update_submodules(&repo, cargo_config, parent_remote_url) } } } diff --git a/tests/testsuite/git.rs b/tests/testsuite/git.rs index c7d72b04e1d..171cf9527b5 100644 --- a/tests/testsuite/git.rs +++ b/tests/testsuite/git.rs @@ -935,6 +935,73 @@ fn dep_with_submodule() { .run(); } +#[cargo_test] +fn dep_with_relative_submodule() { + let foo = project(); + let base = git::new("base", |project| { + project + .file( + "Cargo.toml", + r#" + [package] + name = "base" + version = "0.5.0" + + [dependencies] + deployment.path = "deployment" + "#, + ) + .file( + "src/lib.rs", + r#" + pub fn dep() { + deployment::deployment_func(); + } + "#, + ) + }); + let _deployment = git::new("deployment", |project| { + project + .file("src/lib.rs", "pub fn deployment_func() {}") + .file("Cargo.toml", &basic_lib_manifest("deployment")) + }); + + let base_repo = git2::Repository::open(&base.root()).unwrap(); + git::add_submodule(&base_repo, "../deployment", Path::new("deployment")); + git::commit(&base_repo); + + let project = foo + .file( + "Cargo.toml", + &format!( + r#" + [project] + name = "foo" + version = "0.5.0" + + [dependencies.base] + git = '{}' + "#, + base.url() + ), + ) + .file("src/lib.rs", "pub fn foo() { }") + .build(); + + project + .cargo("build") + .with_stderr( + "\ +[UPDATING] git repository [..] +[UPDATING] git submodule `file://[..]/deployment` +[COMPILING] deployment [..] +[COMPILING] base [..] +[COMPILING] foo [..] +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]\n", + ) + .run(); +} + #[cargo_test] fn dep_with_bad_submodule() { let project = project(); From c29f0b715fc1303cd04dc61192c029f564e8aa3d Mon Sep 17 00:00:00 2001 From: x-hgg-x <39058530+x-hgg-x@users.noreply.github.com> Date: Sun, 18 Sep 2022 11:34:55 +0200 Subject: [PATCH 2/4] Simplify relative submodule handling --- src/cargo/sources/git/utils.rs | 49 +++++++++++----------------------- 1 file changed, 15 insertions(+), 34 deletions(-) diff --git a/src/cargo/sources/git/utils.rs b/src/cargo/sources/git/utils.rs index 10d50e1c002..69c10c423e4 100644 --- a/src/cargo/sources/git/utils.rs +++ b/src/cargo/sources/git/utils.rs @@ -17,7 +17,7 @@ use std::path::{Path, PathBuf}; use std::process::Command; use std::str; use std::time::{Duration, Instant}; -use url::{ParseError, Url}; +use url::Url; fn serialize_str(t: &T, s: S) -> Result where @@ -370,44 +370,25 @@ impl<'a> GitCheckout<'a> { return Ok(()); } - // There are a few possible cases here: - // 1. Submodule URL is not relative. - // Happy path, Url is just the submodule url. - // 2. Submodule URL is relative and does specify ssh/https/file/etc. - // We combine the parent path and relative path. - // 3. Submodule URL is relative and does not specify ssh/https/file/etc. - // In which case we fail to parse with the error ParseError::RelativeUrlWithoutBase. - // We then combine the relative url with the host/protocol from the parent url. - // 4. We fail to parse submodule url for some reason. - // Error. Gets passed up. - - let join_urls = || { - let path = parent_remote_url.path(); + // See the `git submodule add` command on the documentation: + // https://git-scm.com/docs/git-submodule + let url = if child_url_str.starts_with("./") || child_url_str.starts_with("../") { + let mut new_path = parent_remote_url.path().to_string(); + if !new_path.ends_with('/') { + new_path.push('/'); + } + let mut new_parent_remote_url = parent_remote_url.clone(); - new_parent_remote_url.set_path(&format!("{}/", path)); + new_parent_remote_url.set_path(&new_path); + match new_parent_remote_url.join(child_url_str) { - Ok(x) => Ok(x.to_string()), + Ok(x) => x.to_string(), Err(err) => { - Err(err).with_context(|| format!("Failed to parse child submodule url")) + Err(err).with_context(|| format!("Failed to parse child submodule url"))? } } - }; - - let url = match Url::parse(child_url_str) { - Ok(child_url) => { - if Path::new(child_url.path()).is_relative() { - join_urls()? - } else { - child_url.to_string() - } - } - Err(ParseError::RelativeUrlWithoutBase) => join_urls()?, - Err(err) => { - return Err(anyhow::format_err!( - "Error parsing submodule url: {:?}?", - err - )); - } + } else { + child_url_str.to_string() }; // A submodule which is listed in .gitmodules but not actually From 37fdd714c409f9fb0bc57981e751e40899d9a2bb Mon Sep 17 00:00:00 2001 From: x-hgg-x <39058530+x-hgg-x@users.noreply.github.com> Date: Mon, 19 Sep 2022 13:16:46 +0200 Subject: [PATCH 3/4] Use Cow when normalizing parent url path --- src/cargo/sources/git/utils.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/cargo/sources/git/utils.rs b/src/cargo/sources/git/utils.rs index 69c10c423e4..f3a5a6dae5f 100644 --- a/src/cargo/sources/git/utils.rs +++ b/src/cargo/sources/git/utils.rs @@ -11,6 +11,7 @@ use git2::{self, ErrorClass, ObjectType, Oid}; use log::{debug, info}; use serde::ser; use serde::Serialize; +use std::borrow::Cow; use std::env; use std::fmt; use std::path::{Path, PathBuf}; @@ -373,9 +374,9 @@ impl<'a> GitCheckout<'a> { // See the `git submodule add` command on the documentation: // https://git-scm.com/docs/git-submodule let url = if child_url_str.starts_with("./") || child_url_str.starts_with("../") { - let mut new_path = parent_remote_url.path().to_string(); + let mut new_path = Cow::from(parent_remote_url.path()); if !new_path.ends_with('/') { - new_path.push('/'); + new_path.to_mut().push('/'); } let mut new_parent_remote_url = parent_remote_url.clone(); From 528a8b25afe5f81f7bc9b6ffe628e33c96dfd568 Mon Sep 17 00:00:00 2001 From: x-hgg-x <39058530+x-hgg-x@users.noreply.github.com> Date: Wed, 21 Sep 2022 13:00:17 +0200 Subject: [PATCH 4/4] Add fixes from review --- src/cargo/sources/git/utils.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/cargo/sources/git/utils.rs b/src/cargo/sources/git/utils.rs index f3a5a6dae5f..4629f91748f 100644 --- a/src/cargo/sources/git/utils.rs +++ b/src/cargo/sources/git/utils.rs @@ -371,22 +371,27 @@ impl<'a> GitCheckout<'a> { return Ok(()); } - // See the `git submodule add` command on the documentation: - // https://git-scm.com/docs/git-submodule + // Git only assumes a URL is a relative path if it starts with `./` or `../`. + // 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 mut new_parent_remote_url = parent_remote_url.clone(); + let mut new_path = Cow::from(parent_remote_url.path()); if !new_path.ends_with('/') { new_path.to_mut().push('/'); } - - let mut new_parent_remote_url = parent_remote_url.clone(); 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(|| format!("Failed to parse child submodule url"))? - } + Err(err) => Err(err).with_context(|| { + format!( + "failed to parse relative child submodule url `{}` using parent base url `{}`", + child_url_str, new_parent_remote_url + ) + })?, } } else { child_url_str.to_string()