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

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

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)> {

c6c0f76 will fetch_db work?

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.

a474768, I notice the GitSource created by for_git always has precise=None, result to locked_rev being Deferred, so fetch_db always try to fetch if online. Is it appropriate to use with_git_precise for this?

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?

26ffef4 Fixed

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.

I overlook when inline db.copy_to, my bad. But now the code almost the same as db.copy_to, with db.copy has additional check for freshness, should I just use db.copy_to instead for reduce code dups?

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()

26ffef4 There already exists a "failed to update submodule" error context in the fn update_submodules, so I think this fetch error context should only apply to fetch_db?

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)

Fixed, I won't merge unless needed

weihanglo · 2025-12-09T14:06:30Z

@rustbot author

rustbot · 2025-12-09T14:06:36Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

fetch each submodules using git database then checkout using the database

rustbot · 2025-12-10T03:54:35Z

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.

avnyu · 2025-12-10T04:46:39Z

Sorry for the late response, I force-pushed the commits. About new tests, if I am not mistaking, it should be only about the git db cache, not the correctness of git repo checkouts, as it should be already covered? Currently I can't find api for manipulating the cached git db, or example about testing the git cache db

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 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

This comment has been minimized.

Sign in to view

weihanglo reviewed Nov 29, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 9, 2025

avnyu and others added 5 commits December 10, 2025 04:47

refactor: extract into GitSource::fetch_db

c6c0f76

feat: recursive cache git submodules

26ffef4

fetch each submodules using git database then checkout using the database

fix(test): reflect cache git submodules changes

3de3173

test: add case for cached git submodule

c863e3d

fix: remove extra 'update git submodule' when the submodule is cached

a474768

avnyu force-pushed the cached-submodules branch from 8705c44 to a474768 Compare December 10, 2025 03:54

-    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!

This comment has been minimized.

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!

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!

weihanglo commented Dec 9, 2025

Uh oh!

rustbot commented Dec 9, 2025

Uh oh!

avnyu commented Nov 12, 2025 •

edited

Loading

epage Nov 12, 2025 •

edited

Loading

weihanglo left a comment •

edited by rustbot

Loading