git: do not validate submodules of fresh checkouts (#14605)

Fixes #14603

### What does this PR try to resolve?

As is, we unconditionally validate freshness of the submodules of a
checkout, even though we could assume that a fresh checkout has to have
up-to-date submodules as well.

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

N/A

### Additional information

N/A

Author: epage

src/cargo/sources/git/utils.rs

@@ -181,9 +181,14 @@ impl GitDatabase {
             .filter(|co| co.is_fresh())
         {
             Some(co) => co,
-            None => GitCheckout::clone_into(dest, self, rev, gctx)?,
+            None => {
+                let (checkout, guard) = GitCheckout::clone_into(dest, self, rev, gctx)?;
+                checkout.update_submodules(gctx)?;
+                guard.mark_ok()?;
+                checkout
+            }
         };
-        checkout.update_submodules(gctx)?;
+
         Ok(checkout)
     }
 
@@ -280,7 +285,7 @@ impl<'a> GitCheckout<'a> {
         database: &'a GitDatabase,
         revision: git2::Oid,
         gctx: &GlobalContext,
-    ) -> CargoResult<GitCheckout<'a>> {
+    ) -> CargoResult<(GitCheckout<'a>, CheckoutGuard)> {
         let dirname = into.parent().unwrap();
         paths::create_dir_all(&dirname)?;
         if into.exists() {
@@ -329,8 +334,8 @@ impl<'a> GitCheckout<'a> {
         let repo = repo.unwrap();
 
         let checkout = GitCheckout::new(database, revision, repo);
-        checkout.reset(gctx)?;
-        Ok(checkout)
+        let guard = checkout.reset(gctx)?;
+        Ok((checkout, guard))
     }
 
     /// Checks if the `HEAD` of this checkout points to the expected revision.
@@ -355,12 +360,12 @@ impl<'a> GitCheckout<'a> {
     /// To enable this we have a dummy file in our checkout, [`.cargo-ok`],
     /// which if present means that the repo has been successfully reset and is
     /// ready to go. Hence if we start to do a reset, we make sure this file
-    /// *doesn't* exist, and then once we're done we create the file.
+    /// *doesn't* exist. The caller of [`reset`] has an option to perform additional operations
+    /// (e.g. submodule update) before marking the check-out as ready.
     ///
     /// [`.cargo-ok`]: CHECKOUT_READY_LOCK
-    fn reset(&self, gctx: &GlobalContext) -> CargoResult<()> {
-        let ok_file = self.path.join(CHECKOUT_READY_LOCK);
-        let _ = paths::remove_file(&ok_file);
+    fn reset(&self, gctx: &GlobalContext) -> CargoResult<CheckoutGuard> {
+        let guard = CheckoutGuard::guard(&self.path);
         info!("reset {} to {}", self.repo.path().display(), self.revision);
 
         // Ensure libgit2 won't mess with newlines when we vendor.
@@ -370,8 +375,8 @@ impl<'a> GitCheckout<'a> {
 
         let object = self.repo.find_object(self.revision, None)?;
         reset(&self.repo, &object, gctx)?;
-        paths::create(ok_file)?;
-        Ok(())
+
+        Ok(guard)
     }
 
     /// Like `git submodule update --recursive` but for this git checkout.
@@ -479,6 +484,25 @@ impl<'a> GitCheckout<'a> {
     }
 }
 
+/// See [`GitCheckout::reset`] for rationale on this type.
+#[must_use]
+struct CheckoutGuard {
+    ok_file: PathBuf,
+}
+
+impl CheckoutGuard {
+    fn guard(path: &Path) -> Self {
+        let ok_file = path.join(CHECKOUT_READY_LOCK);
+        let _ = paths::remove_file(&ok_file);
+        Self { ok_file }
+    }
+
+    fn mark_ok(self) -> CargoResult<()> {
+        let _ = paths::create(self.ok_file)?;
+        Ok(())
+    }
+}
+
 /// Constructs an absolute URL for a child submodule URL with its parent base URL.
 ///
 /// Git only assumes a submodule URL is a relative path if it starts with `./`

tests/testsuite/git.rs

@@ -9,7 +9,7 @@ use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::Arc;
 use std::thread;
 
-use cargo_test_support::git::cargo_uses_gitoxide;
+use cargo_test_support::git::{add_submodule, cargo_uses_gitoxide};
 use cargo_test_support::paths;
 use cargo_test_support::prelude::IntoData;
 use cargo_test_support::prelude::*;
@@ -3847,11 +3847,20 @@ fn corrupted_checkout_with_cli() {
 }
 
 fn _corrupted_checkout(with_cli: bool) {
-    let git_project = git::new("dep1", |project| {
+    let (git_project, repository) = git::new_repo("dep1", |project| {
         project
             .file("Cargo.toml", &basic_manifest("dep1", "0.5.0"))
             .file("src/lib.rs", "")
     });
+
+    let project2 = git::new("dep2", |project| {
+        project.no_manifest().file("README.md", "")
+    });
+    let url = project2.root().to_url().to_string();
+    add_submodule(&repository, &url, Path::new("dep2"));
+    git::commit(&repository);
+    drop(repository);
+
     let p = project()
         .file(
             "Cargo.toml",
@@ -3873,25 +3882,31 @@ fn _corrupted_checkout(with_cli: bool) {
 
     p.cargo("fetch").run();
 
-    let mut paths = t!(glob::glob(
+    let mut dep1_co_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");
+    let dep1_co_path = dep1_co_paths.next().unwrap().unwrap();
+    let dep1_ok = dep1_co_path.join(".cargo-ok");
+    let dep1_manifest = dep1_co_path.join("Cargo.toml");
+    let dep2_readme = dep1_co_path.join("dep2/README.md");
 
     // Deleting this file simulates an interrupted checkout.
-    t!(fs::remove_file(&ok));
+    t!(fs::remove_file(&dep1_ok));
+    t!(fs::remove_file(&dep1_manifest));
+    t!(fs::remove_file(&dep2_readme));
 
     // 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());
+    assert!(dep1_ok.exists());
+    assert!(dep1_manifest.exists());
+    assert!(dep2_readme.exists());
 }
 
 #[cargo_test]