Skip to content

Cache submodule into git db #16246

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+76 −68
Open

Cache submodule into git db #16246

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
69eeb23
extract code into GitSource::update_db
avnyu Nov 12, 2025
ffd78e6
recursive cache git submodules
avnyu Nov 12, 2025
65cb3b7
fix: error context position
avnyu Nov 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 11 additions & 10 deletions src/cargo/sources/git/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
//! Utilities for handling git repositories, mainly around
//! authentication/cloning.

use crate::core::{GitReference, Verbosity};
use crate::core::{GitReference, SourceId, Verbosity};
use crate::sources::git::fetch::RemoteKind;
use crate::sources::git::oxide;
use crate::sources::git::oxide::cargo_config_to_gitoxide_overrides;
use crate::sources::git::source::GitSource;
use crate::sources::source::Source as _;
use crate::util::HumanBytes;
use crate::util::errors::{CargoResult, GitCliError};
use crate::util::{GlobalContext, IntoUrl, MetricsCounter, Progress, network};
Expand Down Expand Up @@ -447,7 +449,7 @@ impl<'a> GitCheckout<'a> {
let target = repo.head()?.target();
Ok((target, repo))
});
let mut repo = match head_and_repo {
let repo = match head_and_repo {
Ok((head, repo)) => {
if child.head_id() == head {
return update_submodules(&repo, gctx, &child_remote_url);
Expand All @@ -461,24 +463,23 @@ impl<'a> GitCheckout<'a> {
}
};
// Fetch data from origin and reset to the head commit
let reference = GitReference::Rev(head.to_string());
gctx.shell()
.status("Updating", format!("git submodule `{child_remote_url}`"))?;
fetch(
&mut repo,
&child_remote_url,
&reference,
let mut source = GitSource::new(
SourceId::from_url(&format!("git+{child_remote_url}#{head}"))?,
gctx,
RemoteKind::GitDependency,
)
Copy link
Contributor

@epage epage Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What impact does this have on our progress bars?

Copy link
Author

@avnyu avnyu Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

@epage epage Nov 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we should have this also on update_db and copy_to

Copy link
Author

@avnyu avnyu Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@epage epage Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends on the failure

Copy link
Author

@avnyu avnyu Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

source.set_quiet(true);

let (db, actual_rev) = source.update_db()?;
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)
Copy link
Contributor

@epage epage Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be more proper to copy_to after reset?

Copy link
Author

@avnyu avnyu Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
}
Expand Down
2 changes: 1 addition & 1 deletion tests/testsuite/git.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1060,7 +1060,7 @@ Caused by:
failed to update submodule `src`

Caused by:
object not found - no match for id ([..]); class=Odb (9); code=NotFound (-3)
revspec '[..]' not found; class=Reference (4); code=NotFound (-3)

"#]];

Expand Down