From f1895819bc5ed09171b0aa5b8e211854fd103054 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 11 Nov 2024 16:24:23 -0600 Subject: [PATCH 1/5] test(git): Clarify checkout path --- tests/testsuite/git.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/testsuite/git.rs b/tests/testsuite/git.rs index 6bc72423c72..c22d3db88db 100644 --- a/tests/testsuite/git.rs +++ b/tests/testsuite/git.rs @@ -3873,17 +3873,17 @@ 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"); // Deleting this file simulates an interrupted checkout. - t!(fs::remove_file(&ok)); + t!(fs::remove_file(&dep1_ok)); // This should refresh the checkout. let mut e = p.cargo("fetch"); @@ -3891,7 +3891,7 @@ fn _corrupted_checkout(with_cli: bool) { e.env("CARGO_NET_GIT_FETCH_WITH_CLI", "true"); } e.run(); - assert!(ok.exists()); + assert!(dep1_ok.exists()); } #[cargo_test] From e8ad597ed475f5d5a5360063a907fb2d320039a0 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 11 Nov 2024 16:25:22 -0600 Subject: [PATCH 2/5] test(git): Verify corrupted checkout is fixed --- tests/testsuite/git.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/testsuite/git.rs b/tests/testsuite/git.rs index c22d3db88db..010cfc6de9a 100644 --- a/tests/testsuite/git.rs +++ b/tests/testsuite/git.rs @@ -3881,9 +3881,11 @@ fn _corrupted_checkout(with_cli: bool) { )); 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"); // Deleting this file simulates an interrupted checkout. t!(fs::remove_file(&dep1_ok)); + t!(fs::remove_file(&dep1_manifest)); // This should refresh the checkout. let mut e = p.cargo("fetch"); @@ -3892,6 +3894,7 @@ fn _corrupted_checkout(with_cli: bool) { } e.run(); assert!(dep1_ok.exists()); + assert!(dep1_manifest.exists()); } #[cargo_test] From 63c01d8dd0cfd646de1fdd8af4b0a05ab953f43e Mon Sep 17 00:00:00 2001 From: Piotr Osiewicz <24362066+osiewicz@users.noreply.github.com> Date: Tue, 1 Oct 2024 10:23:56 +0200 Subject: [PATCH 3/5] test(git): Add submodule to _corrupted_checkout tests --- tests/testsuite/git.rs | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/tests/testsuite/git.rs b/tests/testsuite/git.rs index 010cfc6de9a..a0650bc0646 100644 --- a/tests/testsuite/git.rs +++ b/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", @@ -3882,10 +3891,12 @@ fn _corrupted_checkout(with_cli: bool) { 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(&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"); @@ -3895,6 +3906,7 @@ fn _corrupted_checkout(with_cli: bool) { e.run(); assert!(dep1_ok.exists()); assert!(dep1_manifest.exists()); + assert!(dep2_readme.exists()); } #[cargo_test] From ecb63986881a66020fbe599b8775485e58607b7b Mon Sep 17 00:00:00 2001 From: Piotr Osiewicz <24362066+osiewicz@users.noreply.github.com> Date: Tue, 1 Oct 2024 11:03:52 +0200 Subject: [PATCH 4/5] refactor(git): Abstract `.cargo-ok` handling --- src/cargo/sources/git/utils.rs | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/src/cargo/sources/git/utils.rs b/src/cargo/sources/git/utils.rs index d28931f8642..39cd5616a5f 100644 --- a/src/cargo/sources/git/utils.rs +++ b/src/cargo/sources/git/utils.rs @@ -359,8 +359,7 @@ impl<'a> GitCheckout<'a> { /// /// [`.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); + 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,7 +369,8 @@ impl<'a> GitCheckout<'a> { let object = self.repo.find_object(self.revision, None)?; reset(&self.repo, &object, gctx)?; - paths::create(ok_file)?; + + guard.mark_ok()?; Ok(()) } @@ -479,6 +479,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 `./` From e82ad5fcf03fbb6049df19c5666d038e456a489c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 11 Nov 2024 16:21:15 -0600 Subject: [PATCH 5/5] perf(git): Skip submodules update on existing checkouts Fixes #14603 --- src/cargo/sources/git/utils.rs | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/cargo/sources/git/utils.rs b/src/cargo/sources/git/utils.rs index 39cd5616a5f..4f1f873bd8b 100644 --- a/src/cargo/sources/git/utils.rs +++ b/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> { + ) -> 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,10 +360,11 @@ 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<()> { + fn reset(&self, gctx: &GlobalContext) -> CargoResult { let guard = CheckoutGuard::guard(&self.path); info!("reset {} to {}", self.repo.path().display(), self.revision); @@ -370,8 +376,7 @@ impl<'a> GitCheckout<'a> { let object = self.repo.find_object(self.revision, None)?; reset(&self.repo, &object, gctx)?; - guard.mark_ok()?; - Ok(()) + Ok(guard) } /// Like `git submodule update --recursive` but for this git checkout.