Cache submodule into git db #16246

avnyu · 2025-11-12T16:33:56Z

What does this PR try to resolve?

My attempt to continue #10279.

This:

Moves the code for creating git db into GitSource::update_db
Creates GitSource for each submodules
Replaces fetch inside update_submodule by GitSource::update_db and db.copy_to
Removes recursive update_submodules calls cos db.copy_to already recursive.

How to test and review this PR?

I tested using the original pull method:

~/.cargo/target/debug/cargo update -p boring --precise 46787b7b6909cadf81cf3a8cd9dc351c9efdfdbd
~/.cargo/target/debug/cargo update -p boring --precise c037a438f8d7b91533524570237afcfeffffe496

and confirmed that the time to do the second update is negligible.
Also test if it can fetch submodule offline using the downloaded git db

git clone https://github.com/pop-os/cosmic-files && cd cosmic-files
~/.cargo/target/debug/cargo fetch --locked --target=$(rustc --print host-tuple)
rm -rf ~/.cargo/git/checkout ~/.cargo/target/debug/cargo fetch --locked --target=$(rustc --print host-tuple) --offline

rustbot · 2025-11-12T16:34:02Z

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

epage · 2025-11-12T16:50:16Z

It would help reviewers if you break this down into atomic commits. For example, by making a commit for extracting update_db, it becomes clear whether it was purely an extraction or there are also other changes.

See https://doc.crates.io/contrib/process/working-on-cargo.html#submitting-a-pull-request

- creates GitSource for each submodules - replaces fetch inside update_submodule by GitSource::update_db and db.copy_to - removes recursive update_submodules calls cos db.copy_to already recursive. - adjust git testsuite

avnyu · 2025-11-12T17:59:26Z

Done, the "extract code into update_db" part is a single commit now. I think the remains part needs to be in the same commit

epage · 2025-11-12T18:04:59Z

src/cargo/sources/git/utils.rs

            .with_context(|| {
                let name = child.name().unwrap_or("");
                format!("failed to fetch submodule `{name}` from {child_remote_url}",)
            })?;


Seems like we should have this also on update_db and copy_to

I tried and the testsuite failed. Should I also adjust testsuite more? I do try to keep it unchanged as much as possible.

Depends on the failure

How about put it only after db.copy_to? I tested and it do pass the testsuite without further modification. The original fetch was replaced by (gitsource create, update_db and db.copy_to), so put the error context to after db.copy_to seem enough?

aeda351
Done, and adjust the testsuite.

See an alternative in #16246 (comment)

epage · 2025-11-12T18:06:33Z

src/cargo/sources/git/utils.rs

+                SourceId::from_url(&format!("git+{child_remote_url}#{head}"))?,
                gctx,
-                RemoteKind::GitDependency,
            )


What impact does this have on our progress bars?

Mostly the same? The old fetch will have a single progress bar, which now changes into a progress bar for update_db and a brief progress bar after for db.copy. This progress bars behavior should be the same as when update git source. Most of the time I don't even notice the brief one.

ea4eb86 should fix when submodule database exist but still print "updating submodule"

epage · 2025-11-12T18:06:57Z

src/cargo/sources/git/utils.rs

+            db.copy_to(actual_rev, &repo.path(), gctx)?;

            let obj = repo.find_object(head, None)?;
-            reset(&repo, &obj, gctx)?;
-            update_submodules(&repo, gctx, &child_remote_url)
+            reset(&repo, &obj, gctx)


Would it be more proper to copy_to after reset?

If I not wrong, the original fetch create checkout?/source at the submodules path. update_db only create git db at CARGO_HOME/git/db, not at the checkout submodules path, so db.copy_to need to run first before we can git reset.

d4d3e6a
I break db.copy_to into small part, the recursive update_submodules part now keep at the original place

the original fetch is replace by (GitSource create, update_db, db.copy) so the error context should be put after db.copy

…ed-submodules

… have the commit let update_db handle the update status by passing quiet recursively

db.copy_to has recursive call for update submodules inside it, now clone the submodule first before reset and keep the recursive update submodules call at the original place.

also adjust the bad submodule git testsuite

rustbot · 2025-11-27T08:40:50Z

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

weihanglo

Thanks for the contribution!

We should have tests for this new caching behavior, especially for nested submodules. See https://doc.crates.io/contrib/process/working-on-cargo.html#making-a-change for how, and also mind the atomic commit pattern (adding test first capturing the existing behavior, and the fix commit showing behavior change through test diffs)

View changes since this review

weihanglo · 2025-11-29T17:09:50Z

src/cargo/sources/git/source.rs

        Ok(())
    }
+
+    pub(crate) fn update_db(&self) -> CargoResult<(GitDatabase, git2::Oid)> {


update_db is an ambiguous name. Would love to see a doc comment of this with a clearer. A random suggestion:

Suggested change

pub(crate) fn update_db(&self) -> CargoResult<(GitDatabase, git2::Oid)> {

/// Ensures a local [`GitDatabase`] exists for this source,

/// and returns it with the resolved revision to use.

///

/// This won't fetch anything if the required revision is

/// already available locally.

pub(crate) fn ensure_db(&self) -> CargoResult<(GitDatabase, git2::Oid)> {

weihanglo · 2025-11-29T17:17:21Z

src/cargo/sources/git/utils.rs

-                &child_remote_url,
-                &reference,
+            let mut source = GitSource::new(
+                SourceId::from_url(&format!("git+{child_remote_url}#{head}"))?,


Instead of constructing URL manually, we have a SourceId::for_git for the purpose.

weihanglo · 2025-11-29T18:07:04Z

src/cargo/sources/git/utils.rs


            let obj = repo.find_object(head, None)?;
-            reset(&repo, &obj, gctx)
+            reset(&repo, &obj, gctx)?;


Why do we still need this reset, given GitCheckout::clone_into already reset it?

weihanglo · 2025-11-29T18:11:22Z

src/cargo/sources/git/utils.rs

                    let name = child.name().unwrap_or("");
                    format!("failed to fetch submodule `{name}` from {child_remote_url}",)
                })?;
+            guard.mark_ok()?;


The checkout is marked ready then we recurse into submodules. I feel like that means the checkout is not ready yet.

Would this situation happens? _User cancels the operation before recursing to nested submodules. They later they dependency on this submodule directly and Cargo assumes it is fresh so never recurse into nested submodules.

weihanglo · 2025-11-29T18:15:35Z

src/cargo/sources/git/utils.rs

+            .with_context(|| {
+                let name = child.name().unwrap_or("");
+                format!("failed to fetch submodule `{name}` from {child_remote_url}",)
+            })?;
            source.set_quiet(quiet);

-            let (db, actual_rev) = source.update_db(true)?;
+            let (db, actual_rev) = source.update_db(true).with_context(|| {
+                let name = child.name().unwrap_or("");
+                format!("failed to fetch submodule `{name}` from {child_remote_url}",)
+            })?;


Instead of this, I wonder if we should consolidate this in the error context in fn update_submodules()

weihanglo · 2025-11-29T18:18:17Z

src/cargo/sources/git/utils.rs

            })?;

-            // Clone and reset to the head commmit
+            // Clone and reset to the head commit


We are open to rebasing/reorganizing your commit history throughout the iterations of a pull request. And personally I think this kind of commit should be squashed into wherever made this typo. That would help get a more understandable/traceable git commit history

weihanglo · 2025-11-29T18:33:26Z

src/cargo/sources/git/source.rs

[not specific to this file]

[not hard blockers]

I noticed that in the commit history we have Merge branch 'master' of https://…. We encourage rebasing and having a well-structured git history for review/debugging/tracing purpose.

Also, chore: … were used in some commits, but chore should have not production code change (see https://stackoverflow.com/a/26944812). IMO they should either be refactor or fix.
(Some may be better being squashed into the entire fix ffd78e6, as those intermediates commits minimally help future review and code tracing)

rustbot assigned weihanglo Nov 12, 2025

rustbot added A-git Area: anything dealing with git S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 12, 2025

This comment has been minimized.

Sign in to view

avnyu added 2 commits November 13, 2025 00:41

extract code into GitSource::update_db

69eeb23

recursive cache git submodules

ffd78e6

- creates GitSource for each submodules - replaces fetch inside update_submodule by GitSource::update_db and db.copy_to - removes recursive update_submodules calls cos db.copy_to already recursive. - adjust git testsuite

avnyu force-pushed the cached-submodules branch from 70350b1 to ffd78e6 Compare November 12, 2025 17:52

This comment has been minimized.

Sign in to view

epage reviewed Nov 12, 2025

View reviewed changes

avnyu added 5 commits November 13, 2025 02:45

fix: error context position

65cb3b7

the original fetch is replace by (GitSource create, update_db, db.copy) so the error context should be put after db.copy

Merge branch 'master' of https://github.com/rust-lang/cargo into cach…

3530ca4

…ed-submodules

fix: print "updating git submodule" when database existed and already…

ea4eb86

… have the commit let update_db handle the update status by passing quiet recursively

chore: break db.copy_to

d4d3e6a

db.copy_to has recursive call for update submodules inside it, now clone the submodule first before reset and keep the recursive update submodules call at the original place.

chore: put fetch submodule error context to source and update_db

aeda351

also adjust the bad submodule git testsuite

chore: fix typo

8705c44

weihanglo reviewed Nov 29, 2025

View reviewed changes

-    pub(crate) fn update_db(&self) -> CargoResult<(GitDatabase, git2::Oid)> {
+    /// Ensures a local [`GitDatabase`] exists for this source,
+    /// and returns it with the resolved revision to use.
+    ///
+    /// This won't fetch anything if the required revision is
+    /// already available locally.
+    pub(crate) fn ensure_db(&self) -> CargoResult<(GitDatabase, git2::Oid)> {

Cache submodule into git db #16246

Are you sure you want to change the base?

Cache submodule into git db #16246

Conversation

avnyu commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR try to resolve?

How to test and review this PR?

Uh oh!

rustbot commented Nov 12, 2025

Uh oh!

epage commented Nov 12, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

avnyu commented Nov 12, 2025

Uh oh!

epage Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Nov 27, 2025

Uh oh!

weihanglo left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

avnyu commented Nov 12, 2025 •

edited

Loading

epage Nov 12, 2025 •

edited

Loading

weihanglo left a comment •

edited by rustbot

Loading