Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
111 changes: 59 additions & 52 deletions src/cargo/sources/git/source.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use crate::core::global_cache_tracker;
use crate::core::{Dependency, Package, PackageId};
use crate::sources::IndexSummary;
use crate::sources::RecursivePathSource;
use crate::sources::git::utils::GitDatabase;
use crate::sources::git::utils::GitRemote;
use crate::sources::git::utils::rev_to_oid;
use crate::sources::source::MaybePackage;
Expand Down Expand Up @@ -155,6 +156,63 @@ impl<'gctx> GitSource<'gctx> {
});
Ok(())
}

pub(crate) fn update_db(&self) -> CargoResult<(GitDatabase, git2::Oid)> {
let db_path = self.gctx.git_db_path().join(&self.ident);
let db_path = db_path.into_path_unlocked();

let db = self.remote.db_at(&db_path).ok();

let (db, actual_rev) = match (&self.locked_rev, db) {
// If we have a locked revision, and we have a preexisting database
// which has that revision, then no update needs to happen.
(Revision::Locked(oid), Some(db)) if db.contains(*oid) => (db, *oid),

// If we're in offline mode, we're not locked, and we have a
// database, then try to resolve our reference with the preexisting
// repository.
(Revision::Deferred(git_ref), Some(db)) if !self.gctx.network_allowed() => {
let offline_flag = self
.gctx
.offline_flag()
.expect("always present when `!network_allowed`");
let rev = db.resolve(&git_ref).with_context(|| {
format!(
"failed to lookup reference in preexisting repository, and \
can't check for updates in offline mode ({offline_flag})"
)
})?;
(db, rev)
}

// ... otherwise we use this state to update the git database. Note
// that we still check for being offline here, for example in the
// situation that we have a locked revision but the database
// doesn't have it.
(locked_rev, db) => {
if let Some(offline_flag) = self.gctx.offline_flag() {
anyhow::bail!(
"can't checkout from '{}': you are in the offline mode ({offline_flag})",
self.remote.url()
);
}

if !self.quiet {
self.gctx.shell().status(
"Updating",
format!("git repository `{}`", self.remote.url()),
)?;
}

trace!("updating git source `{:?}`", self.remote);

let locked_rev = locked_rev.clone().into();
self.remote.checkout(&db_path, db, &locked_rev, self.gctx)?
}
};

Ok((db, actual_rev))
}
}

/// Indicates a [Git revision] that might be locked or deferred to be resolved.
Expand Down Expand Up @@ -286,58 +344,7 @@ impl<'gctx> Source for GitSource<'gctx> {
// exists.
exclude_from_backups_and_indexing(&git_path);

let db_path = self.gctx.git_db_path().join(&self.ident);
let db_path = db_path.into_path_unlocked();

let db = self.remote.db_at(&db_path).ok();

let (db, actual_rev) = match (&self.locked_rev, db) {
// If we have a locked revision, and we have a preexisting database
// which has that revision, then no update needs to happen.
(Revision::Locked(oid), Some(db)) if db.contains(*oid) => (db, *oid),

// If we're in offline mode, we're not locked, and we have a
// database, then try to resolve our reference with the preexisting
// repository.
(Revision::Deferred(git_ref), Some(db)) if !self.gctx.network_allowed() => {
let offline_flag = self
.gctx
.offline_flag()
.expect("always present when `!network_allowed`");
let rev = db.resolve(&git_ref).with_context(|| {
format!(
"failed to lookup reference in preexisting repository, and \
can't check for updates in offline mode ({offline_flag})"
)
})?;
(db, rev)
}

// ... otherwise we use this state to update the git database. Note
// that we still check for being offline here, for example in the
// situation that we have a locked revision but the database
// doesn't have it.
(locked_rev, db) => {
if let Some(offline_flag) = self.gctx.offline_flag() {
anyhow::bail!(
"can't checkout from '{}': you are in the offline mode ({offline_flag})",
self.remote.url()
);
}

if !self.quiet {
self.gctx.shell().status(
"Updating",
format!("git repository `{}`", self.remote.url()),
)?;
}

trace!("updating git source `{:?}`", self.remote);

let locked_rev = locked_rev.clone().into();
self.remote.checkout(&db_path, db, &locked_rev, self.gctx)?
}
};
let (db, actual_rev) = self.update_db()?;

// Don’t use the full hash, in order to contribute less to reaching the
// path length limit on Windows. See
Expand Down
21 changes: 11 additions & 10 deletions src/cargo/sources/git/utils.rs
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

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

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

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

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

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

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

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

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