Auto merge of #10829 - ehuss:corrupted-checkout, r=weihanglo

Fix corrupted git checkout recovery.

This fixes an issue where cargo would not recover from a corrupted git checkout correctly when using `net.git-fetch-with-cli`.

Git dependencies have two clones, the "db" and the "checkout". The "db" is shared amongst multiple checkout revisions from the same repository. The "checkout" is each individual revision. There was some code in `copy_to` which creates the "checkout" that tries to recover from an error. The "checkout" can be invalid if cargo was interrupted while cloning it, or if there is fs corruption. However, that code was failing when using the git CLI.  For reasons I did not dig into, the "db" does not have a HEAD ref, so that special-case fetch was failing with a `couldn't find remote ref HEAD` error from `git`.

This changes it so that if the "checkout" is invalid, it just gets blown away and a new clone is created (instead of calling `git fetch` from the "db").

I believe there is some long history for this `copy_to` code where it made more sense in the past. Previously, the "checkout" directories used the `GitReference` string as-is. So, for example, a branch would checkout into a directory with that branch name. At some point, it was changed so that each checkout uses a short hash of the actual revision.  Rebuilding the checkout made sense when it was possible for that checkout revision to change (like if new commits were pushed to a branch).  That recovery is no longer necessary since a checkout is only ever one revision.

Fixes #10826

Author: bors

src/cargo/sources/git/utils.rs

@@ -151,30 +151,16 @@ impl GitDatabase {
         dest: &Path,
         cargo_config: &Config,
     ) -> CargoResult<GitCheckout<'_>> {
-        let mut checkout = None;
-        if let Ok(repo) = git2::Repository::open(dest) {
-            let mut co = GitCheckout::new(dest, self, rev, repo);
-            if !co.is_fresh() {
-                // After a successful fetch operation the subsequent reset can
-                // fail sometimes for corrupt repositories where the fetch
-                // operation succeeds but the object isn't actually there in one
-                // way or another. In these situations just skip the error and
-                // try blowing away the whole repository and trying with a
-                // clone.
-                co.fetch(cargo_config)?;
-                match co.reset(cargo_config) {
-                    Ok(()) => {
-                        assert!(co.is_fresh());
-                        checkout = Some(co);
-                    }
-                    Err(e) => debug!("failed reset after fetch {:?}", e),
-                }
-            } else {
-                checkout = Some(co);
-            }
-        };
-        let checkout = match checkout {
-            Some(c) => c,
+        // If the existing checkout exists, and it is fresh, use it.
+        // A non-fresh checkout can happen if the checkout operation was
+        // interrupted. In that case, the checkout gets deleted and a new
+        // clone is created.
+        let checkout = match git2::Repository::open(dest)
+            .ok()
+            .map(|repo| GitCheckout::new(dest, self, rev, repo))
+            .filter(|co| co.is_fresh())
+        {
+            Some(co) => co,
             None => GitCheckout::clone_into(dest, self, rev, cargo_config)?,
         };
         checkout.update_submodules(cargo_config)?;
@@ -311,14 +297,6 @@ impl<'a> GitCheckout<'a> {
         }
     }
 
-    fn fetch(&mut self, cargo_config: &Config) -> CargoResult<()> {
-        info!("fetch {}", self.repo.path().display());
-        let url = self.database.path.into_url()?;
-        let reference = GitReference::Rev(self.revision.to_string());
-        fetch(&mut self.repo, url.as_str(), &reference, cargo_config)?;
-        Ok(())
-    }
-
     fn reset(&self, config: &Config) -> CargoResult<()> {
         // If we're interrupted while performing this reset (e.g., we die because
         // of a signal) Cargo needs to be sure to try to check out this repo

tests/testsuite/git.rs

@@ -3510,3 +3510,65 @@ fn git_with_force_push() {
     amend_commit("awesome-four");
     verify("awesome-four");
 }
+
+#[cargo_test]
+fn corrupted_checkout() {
+    // Test what happens if the checkout is corrupted somehow.
+    _corrupted_checkout(false);
+}
+
+#[cargo_test]
+fn corrupted_checkout_with_cli() {
+    // Test what happens if the checkout is corrupted somehow with git cli.
+    if disable_git_cli() {
+        return;
+    }
+    _corrupted_checkout(true);
+}
+
+fn _corrupted_checkout(with_cli: bool) {
+    let git_project = git::new("dep1", |project| {
+        project
+            .file("Cargo.toml", &basic_manifest("dep1", "0.5.0"))
+            .file("src/lib.rs", "")
+    });
+    let p = project()
+        .file(
+            "Cargo.toml",
+            &format!(
+                r#"
+                    [package]
+                    name = "foo"
+                    version = "0.1.0"
+
+                    [dependencies]
+                    dep1 = {{ git = "{}" }}
+                "#,
+                git_project.url()
+            ),
+        )
+        .file("src/lib.rs", "")
+        .build();
+
+    p.cargo("fetch").run();
+
+    let mut paths = t!(glob::glob(
+        paths::home()
+            .join(".cargo/git/checkouts/dep1-*/*")
+            .to_str()
+            .unwrap()
+    ));
+    let path = paths.next().unwrap().unwrap();
+    let ok = path.join(".cargo-ok");
+
+    // Deleting this file simulates an interrupted checkout.
+    t!(fs::remove_file(&ok));
+
+    // This should refresh the checkout.
+    let mut e = p.cargo("fetch");
+    if with_cli {
+        e.env("CARGO_NET_GIT_FETCH_WITH_CLI", "true");
+    }
+    e.run();
+    assert!(ok.exists());
+}