Auto merge of #7238 - ehuss:git-ssh-submodule, r=alexcrichton

Allow git dependency with shorthand ssh submodules to work.

If a submodule is defined with a shorthand ssh url (like `git@github.com/user/repo.git`), then cargo was choking on it trying to convert it to a URL. The fix is to just pass around strings.

An alternate solution would be to try to detect shorthand git urls and automatically add `ssh://` to the path. I'm concerned about matching git's heuristics for this, though. I'm willing to try if you think this would be better, though.

I can't think of a good way to write a test for this, since we don't have any SSH test infrastructure. I verified running locally against github.

Closes #7202

src/cargo/sources/git/utils.rs

@@ -141,7 +141,7 @@ impl GitRemote {
     fn fetch_into(&self, dst: &mut git2::Repository, cargo_config: &Config) -> CargoResult<()> {
         // Create a local anonymous remote in the repository to fetch the url
         let refspec = "refs/heads/*:refs/heads/*";
-        fetch(dst, &self.url, refspec, cargo_config)
+        fetch(dst, &self.url.as_str(), refspec, cargo_config)
     }
 
     fn clone_into(&self, dst: &Path, cargo_config: &Config) -> CargoResult<git2::Repository> {
@@ -152,7 +152,7 @@ impl GitRemote {
         let mut repo = init(dst, true)?;
         fetch(
             &mut repo,
-            &self.url,
+            &self.url.as_str(),
             "refs/heads/*:refs/heads/*",
             cargo_config,
         )?;
@@ -276,7 +276,7 @@ impl<'a> GitCheckout<'a> {
         // need authentication information we may want progress bars and such.
         let url = database.path.into_url()?;
         let mut repo = None;
-        with_fetch_options(&git_config, &url, config, &mut |fopts| {
+        with_fetch_options(&git_config, url.as_str(), config, &mut |fopts| {
             let mut checkout = git2::build::CheckoutBuilder::new();
             checkout.dry_run(); // we'll do this below during a `reset`
 
@@ -312,7 +312,7 @@ impl<'a> GitCheckout<'a> {
         info!("fetch {}", self.repo.path().display());
         let url = self.database.path.into_url()?;
         let refspec = "refs/heads/*:refs/heads/*";
-        fetch(&mut self.repo, &url, refspec, cargo_config)?;
+        fetch(&mut self.repo, url.as_str(), refspec, cargo_config)?;
         Ok(())
     }
 
@@ -393,10 +393,8 @@ impl<'a> GitCheckout<'a> {
                     init(&path, false)?
                 }
             };
-
             // Fetch data from origin and reset to the head commit
             let refspec = "refs/heads/*:refs/heads/*";
-            let url = url.into_url()?;
             fetch(&mut repo, &url, refspec, cargo_config).chain_err(|| {
                 internal(format!(
                     "failed to fetch submodule `{}` from {}",
@@ -640,13 +638,13 @@ fn reset(repo: &git2::Repository, obj: &git2::Object<'_>, config: &Config) -> Ca
 
 pub fn with_fetch_options(
     git_config: &git2::Config,
-    url: &Url,
+    url: &str,
     config: &Config,
     cb: &mut dyn FnMut(git2::FetchOptions<'_>) -> CargoResult<()>,
 ) -> CargoResult<()> {
     let mut progress = Progress::new("Fetch", config);
     network::with_retry(config, || {
-        with_authentication(url.as_str(), git_config, |f| {
+        with_authentication(url, git_config, |f| {
             let mut rcb = git2::RemoteCallbacks::new();
             rcb.credentials(f);
 
@@ -669,7 +667,7 @@ pub fn with_fetch_options(
 
 pub fn fetch(
     repo: &mut git2::Repository,
-    url: &Url,
+    url: &str,
     refspec: &str,
     config: &Config,
 ) -> CargoResult<()> {
@@ -685,14 +683,17 @@ pub fn fetch(
 
     // If we're fetching from GitHub, attempt GitHub's special fast path for
     // testing if we've already got an up-to-date copy of the repository
-    if url.host_str() == Some("github.com") {
-        if let Ok(oid) = repo.refname_to_id("refs/remotes/origin/master") {
-            let mut handle = config.http()?.borrow_mut();
-            debug!("attempting GitHub fast path for {}", url);
-            if github_up_to_date(&mut handle, url, &oid) {
-                return Ok(());
-            } else {
-                debug!("fast path failed, falling back to a git fetch");
+
+    if let Ok(url) = Url::parse(url) {
+        if url.host_str() == Some("github.com") {
+            if let Ok(oid) = repo.refname_to_id("refs/remotes/origin/master") {
+                let mut handle = config.http()?.borrow_mut();
+                debug!("attempting GitHub fast path for {}", url);
+                if github_up_to_date(&mut handle, &url, &oid) {
+                    return Ok(());
+                } else {
+                    debug!("fast path failed, falling back to a git fetch");
+                }
             }
         }
     }
@@ -732,7 +733,7 @@ pub fn fetch(
         loop {
             debug!("initiating fetch of {} from {}", refspec, url);
             let res = repo
-                .remote_anonymous(url.as_str())?
+                .remote_anonymous(url)?
                 .fetch(&[refspec], Some(&mut opts), None);
             let err = match res {
                 Ok(()) => break,
@@ -759,7 +760,7 @@ pub fn fetch(
 
 fn fetch_with_cli(
     repo: &mut git2::Repository,
-    url: &Url,
+    url: &str,
     refspec: &str,
     config: &Config,
 ) -> CargoResult<()> {
@@ -768,7 +769,7 @@ fn fetch_with_cli(
         .arg("--tags") // fetch all tags
         .arg("--force") // handle force pushes
         .arg("--update-head-ok") // see discussion in #2078
-        .arg(url.to_string())
+        .arg(url)
         .arg(refspec)
         // If cargo is run by git (for example, the `exec` command in `git
         // rebase`), the GIT_DIR is set by git and will point to the wrong

src/cargo/sources/registry/remote.rs

@@ -230,7 +230,7 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> {
         let url = self.source_id.url();
         let refspec = "refs/heads/master:refs/remotes/origin/master";
         let repo = self.repo.borrow_mut().unwrap();
-        git::fetch(repo, url, refspec, self.config)
+        git::fetch(repo, url.as_str(), refspec, self.config)
             .chain_err(|| format!("failed to fetch `{}`", url))?;
         self.config.updated_sources().insert(self.source_id);