From f3edba9197ee0075edd29d1096195158267977b0 Mon Sep 17 00:00:00 2001 From: Dacian Pascu Date: Sun, 17 Nov 2024 21:14:48 +0200 Subject: [PATCH 1/3] test(git): added tests for issue rust-lang#14621 --- tests/testsuite/git.rs | 78 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/tests/testsuite/git.rs b/tests/testsuite/git.rs index 6bc72423c72..96fb941a587 100644 --- a/tests/testsuite/git.rs +++ b/tests/testsuite/git.rs @@ -4052,6 +4052,84 @@ src/lib.rs .run(); } +#[cargo_test(public_network_test, requires_git)] +fn github_fastpath_error_message() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + edition = "2015" + + [dependencies] + bitflags = { git = "https://github.com/rust-lang/bitflags.git", rev="11111b376b93484341c68fbca3ca110ae5cd2790" } + "#, + ) + .file("src/lib.rs", "") + .build(); + p.cargo("fetch") + .env("CARGO_NET_GIT_FETCH_WITH_CLI", "true") + .with_status(101) + .with_stderr_data(str![[r#" +[UPDATING] git repository `https://github.com/rust-lang/bitflags.git` +fatal: remote [ERROR] upload-pack: not our ref 11111b376b93484341c68fbca3ca110ae5cd2790 +[ERROR] failed to get `bitflags` as a dependency of package `foo v0.1.0 ([ROOT]/foo)` + +Caused by: + failed to load source for dependency `bitflags` + +Caused by: + Unable to update https://github.com/rust-lang/bitflags.git?rev=11111b376b93484341c68fbca3ca110ae5cd2790 + +Caused by: + failed to clone into: [ROOT]/home/.cargo/git/db/bitflags-[HASH] + +Caused by: + process didn't exit successfully: `git fetch --no-tags --force --update-head-ok 'https://github.com/rust-lang/bitflags.git' '+11111b376b93484341c68fbca3ca110ae5cd2790:refs/commit/11111b376b93484341c68fbca3ca110ae5cd2790'` ([EXIT_STATUS]: 128) + +"#]]) + .run(); +} + +#[cargo_test(public_network_test)] +fn git_fetch_libgit2_error_message() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + edition = "2015" + + [dependencies] + bitflags = { git = "https://github.com/rust-lang/bitflags.git", rev="11111b376b93484341c68fbca3ca110ae5cd2790" } + "#, + ) + .file("src/lib.rs", "") + .build(); + p.cargo("fetch") + .with_status(101) + .with_stderr_data(str![[r#" +[UPDATING] git repository `https://github.com/rust-lang/bitflags.git` +... +[ERROR] failed to get `bitflags` as a dependency of package `foo v0.1.0 ([ROOT]/foo)` + +Caused by: + failed to load source for dependency `bitflags` + +Caused by: + Unable to update https://github.com/rust-lang/bitflags.git?rev=11111b376b93484341c68fbca3ca110ae5cd2790 + +Caused by: + failed to clone into: [ROOT]/home/.cargo/git/db/bitflags-[HASH] +... +"#]]) + .run(); +} + #[cargo_test] fn git_worktree_with_bare_original_repo() { let project = project().build(); From 309b2ce828c51d8e7faed4088a73acc9f93c6b59 Mon Sep 17 00:00:00 2001 From: Dacian Pascu Date: Sun, 17 Nov 2024 21:15:33 +0200 Subject: [PATCH 2/3] refactor(git): extracted fetch logic into different functions --- src/cargo/sources/git/utils.rs | 318 ++++++++++++++++++--------------- 1 file changed, 169 insertions(+), 149 deletions(-) diff --git a/src/cargo/sources/git/utils.rs b/src/cargo/sources/git/utils.rs index d28931f8642..fb8cf366e60 100644 --- a/src/cargo/sources/git/utils.rs +++ b/src/cargo/sources/git/utils.rs @@ -1007,156 +1007,11 @@ pub fn fetch( } if let Some(true) = gctx.net_config()?.git_fetch_with_cli { - return fetch_with_cli(repo, remote_url, &refspecs, tags, gctx); - } - - if gctx.cli_unstable().gitoxide.map_or(false, |git| git.fetch) { - let git2_repo = repo; - let config_overrides = cargo_config_to_gitoxide_overrides(gctx)?; - let repo_reinitialized = AtomicBool::default(); - let res = oxide::with_retry_and_progress( - &git2_repo.path().to_owned(), - gctx, - &|repo_path, - should_interrupt, - mut progress, - url_for_authentication: &mut dyn FnMut(&gix::bstr::BStr)| { - // The `fetch` operation here may fail spuriously due to a corrupt - // repository. It could also fail, however, for a whole slew of other - // reasons (aka network related reasons). We want Cargo to automatically - // recover from corrupt repositories, but we don't want Cargo to stomp - // over other legitimate errors. - // - // Consequently we save off the error of the `fetch` operation and if it - // looks like a "corrupt repo" error then we blow away the repo and try - // again. If it looks like any other kind of error, or if we've already - // blown away the repository, then we want to return the error as-is. - loop { - let res = oxide::open_repo( - repo_path, - config_overrides.clone(), - oxide::OpenMode::ForFetch, - ) - .map_err(crate::sources::git::fetch::Error::from) - .and_then(|repo| { - debug!("initiating fetch of {refspecs:?} from {remote_url}"); - let url_for_authentication = &mut *url_for_authentication; - let remote = repo - .remote_at(remote_url)? - .with_fetch_tags(if tags { - gix::remote::fetch::Tags::All - } else { - gix::remote::fetch::Tags::Included - }) - .with_refspecs( - refspecs.iter().map(|s| s.as_str()), - gix::remote::Direction::Fetch, - ) - .map_err(crate::sources::git::fetch::Error::Other)?; - let url = remote - .url(gix::remote::Direction::Fetch) - .expect("set at init") - .to_owned(); - let connection = remote.connect(gix::remote::Direction::Fetch)?; - let mut authenticate = connection.configured_credentials(url)?; - let connection = connection.with_credentials( - move |action: gix::protocol::credentials::helper::Action| { - if let Some(url) = action.context().and_then(|gctx| { - gctx.url.as_ref().filter(|url| *url != remote_url) - }) { - url_for_authentication(url.as_ref()); - } - authenticate(action) - }, - ); - let outcome = connection - .prepare_fetch(&mut progress, gix::remote::ref_map::Options::default())? - .with_shallow(shallow.clone().into()) - .receive(&mut progress, should_interrupt)?; - Ok(outcome) - }); - let err = match res { - Ok(_) => break, - Err(e) => e, - }; - debug!("fetch failed: {}", err); - - if !repo_reinitialized.load(Ordering::Relaxed) - // We check for errors that could occur if the configuration, refs or odb files are corrupted. - // We don't check for errors related to writing as `gitoxide` is expected to create missing leading - // folder before writing files into it, or else not even open a directory as git repository (which is - // also handled here). - && err.is_corrupted() - || has_shallow_lock_file(&err) - { - repo_reinitialized.store(true, Ordering::Relaxed); - debug!( - "looks like this is a corrupt repository, reinitializing \ - and trying again" - ); - if oxide::reinitialize(repo_path).is_ok() { - continue; - } - } - - return Err(err.into()); - } - Ok(()) - }, - ); - if repo_reinitialized.load(Ordering::Relaxed) { - *git2_repo = git2::Repository::open(git2_repo.path())?; - } - res + fetch_with_cli(repo, remote_url, &refspecs, tags, gctx) + } else if gctx.cli_unstable().gitoxide.map_or(false, |git| git.fetch) { + fetch_with_gitoxide(repo, remote_url, refspecs, tags, shallow, gctx) } else { - debug!("doing a fetch for {remote_url}"); - let git_config = git2::Config::open_default()?; - with_fetch_options(&git_config, remote_url, gctx, &mut |mut opts| { - if tags { - opts.download_tags(git2::AutotagOption::All); - } - if let gix::remote::fetch::Shallow::DepthAtRemote(depth) = shallow { - opts.depth(0i32.saturating_add_unsigned(depth.get())); - } - // The `fetch` operation here may fail spuriously due to a corrupt - // repository. It could also fail, however, for a whole slew of other - // reasons (aka network related reasons). We want Cargo to automatically - // recover from corrupt repositories, but we don't want Cargo to stomp - // over other legitimate errors. - // - // Consequently we save off the error of the `fetch` operation and if it - // looks like a "corrupt repo" error then we blow away the repo and try - // again. If it looks like any other kind of error, or if we've already - // blown away the repository, then we want to return the error as-is. - let mut repo_reinitialized = false; - loop { - debug!("initiating fetch of {refspecs:?} from {remote_url}"); - let res = - repo.remote_anonymous(remote_url)? - .fetch(&refspecs, Some(&mut opts), None); - let err = match res { - Ok(()) => break, - Err(e) => e, - }; - debug!("fetch failed: {}", err); - - if !repo_reinitialized - && matches!(err.class(), ErrorClass::Reference | ErrorClass::Odb) - { - repo_reinitialized = true; - debug!( - "looks like this is a corrupt repository, reinitializing \ - and trying again" - ); - if reinitialize(repo).is_ok() { - continue; - } - } - - return Err(err.into()); - } - Ok(()) - }) + fetch_with_libgit2(repo, remote_url, refspecs, tags, shallow, gctx) } } @@ -1227,6 +1082,171 @@ fn fetch_with_cli( Ok(()) } +fn fetch_with_gitoxide( + repo: &mut git2::Repository, + remote_url: &str, + refspecs: Vec, + tags: bool, + shallow: gix::remote::fetch::Shallow, + gctx: &GlobalContext, +) -> CargoResult<()> { + let git2_repo = repo; + let config_overrides = cargo_config_to_gitoxide_overrides(gctx)?; + let repo_reinitialized = AtomicBool::default(); + let res = oxide::with_retry_and_progress( + &git2_repo.path().to_owned(), + gctx, + &|repo_path, + should_interrupt, + mut progress, + url_for_authentication: &mut dyn FnMut(&gix::bstr::BStr)| { + // The `fetch` operation here may fail spuriously due to a corrupt + // repository. It could also fail, however, for a whole slew of other + // reasons (aka network related reasons). We want Cargo to automatically + // recover from corrupt repositories, but we don't want Cargo to stomp + // over other legitimate errors. + // + // Consequently we save off the error of the `fetch` operation and if it + // looks like a "corrupt repo" error then we blow away the repo and try + // again. If it looks like any other kind of error, or if we've already + // blown away the repository, then we want to return the error as-is. + loop { + let res = oxide::open_repo( + repo_path, + config_overrides.clone(), + oxide::OpenMode::ForFetch, + ) + .map_err(crate::sources::git::fetch::Error::from) + .and_then(|repo| { + debug!("initiating fetch of {refspecs:?} from {remote_url}"); + let url_for_authentication = &mut *url_for_authentication; + let remote = repo + .remote_at(remote_url)? + .with_fetch_tags(if tags { + gix::remote::fetch::Tags::All + } else { + gix::remote::fetch::Tags::Included + }) + .with_refspecs( + refspecs.iter().map(|s| s.as_str()), + gix::remote::Direction::Fetch, + ) + .map_err(crate::sources::git::fetch::Error::Other)?; + let url = remote + .url(gix::remote::Direction::Fetch) + .expect("set at init") + .to_owned(); + let connection = remote.connect(gix::remote::Direction::Fetch)?; + let mut authenticate = connection.configured_credentials(url)?; + let connection = connection.with_credentials( + move |action: gix::protocol::credentials::helper::Action| { + if let Some(url) = action + .context() + .and_then(|gctx| gctx.url.as_ref().filter(|url| *url != remote_url)) + { + url_for_authentication(url.as_ref()); + } + authenticate(action) + }, + ); + let outcome = connection + .prepare_fetch(&mut progress, gix::remote::ref_map::Options::default())? + .with_shallow(shallow.clone().into()) + .receive(&mut progress, should_interrupt)?; + Ok(outcome) + }); + let err = match res { + Ok(_) => break, + Err(e) => e, + }; + debug!("fetch failed: {}", err); + + if !repo_reinitialized.load(Ordering::Relaxed) + // We check for errors that could occur if the configuration, refs or odb files are corrupted. + // We don't check for errors related to writing as `gitoxide` is expected to create missing leading + // folder before writing files into it, or else not even open a directory as git repository (which is + // also handled here). + && err.is_corrupted() + || has_shallow_lock_file(&err) + { + repo_reinitialized.store(true, Ordering::Relaxed); + debug!( + "looks like this is a corrupt repository, reinitializing \ + and trying again" + ); + if oxide::reinitialize(repo_path).is_ok() { + continue; + } + } + + return Err(err.into()); + } + Ok(()) + }, + ); + if repo_reinitialized.load(Ordering::Relaxed) { + *git2_repo = git2::Repository::open(git2_repo.path())?; + } + res +} + +fn fetch_with_libgit2( + repo: &mut git2::Repository, + remote_url: &str, + refspecs: Vec, + tags: bool, + shallow: gix::remote::fetch::Shallow, + gctx: &GlobalContext, +) -> CargoResult<()> { + debug!("doing a fetch for {remote_url}"); + let git_config = git2::Config::open_default()?; + with_fetch_options(&git_config, remote_url, gctx, &mut |mut opts| { + if tags { + opts.download_tags(git2::AutotagOption::All); + } + if let gix::remote::fetch::Shallow::DepthAtRemote(depth) = shallow { + opts.depth(0i32.saturating_add_unsigned(depth.get())); + } + // The `fetch` operation here may fail spuriously due to a corrupt + // repository. It could also fail, however, for a whole slew of other + // reasons (aka network related reasons). We want Cargo to automatically + // recover from corrupt repositories, but we don't want Cargo to stomp + // over other legitimate errors. + // + // Consequently we save off the error of the `fetch` operation and if it + // looks like a "corrupt repo" error then we blow away the repo and try + // again. If it looks like any other kind of error, or if we've already + // blown away the repository, then we want to return the error as-is. + let mut repo_reinitialized = false; + loop { + debug!("initiating fetch of {refspecs:?} from {remote_url}"); + let res = repo + .remote_anonymous(remote_url)? + .fetch(&refspecs, Some(&mut opts), None); + let err = match res { + Ok(()) => break, + Err(e) => e, + }; + debug!("fetch failed: {}", err); + + if !repo_reinitialized && matches!(err.class(), ErrorClass::Reference | ErrorClass::Odb) + { + repo_reinitialized = true; + debug!( + "looks like this is a corrupt repository, reinitializing \ + and trying again" + ); + if reinitialize(repo).is_ok() { + continue; + } + } + + return Err(err.into()); + } + Ok(()) + }) +} + /// Attempts to `git gc` a repository. /// /// Cargo has a bunch of long-lived git repositories in its global cache and From 43a5dee2a8cff584550c565b3025e1b3fab66c5c Mon Sep 17 00:00:00 2001 From: Dacian Pascu Date: Sun, 17 Nov 2024 22:49:36 +0200 Subject: [PATCH 3/3] fix(git): added extra error context for a revision that doesn't exist See issue rust-lang#14621 --- src/cargo/sources/git/utils.rs | 13 ++++++++++++- tests/testsuite/git.rs | 8 +++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/cargo/sources/git/utils.rs b/src/cargo/sources/git/utils.rs index fb8cf366e60..ed2d759982e 100644 --- a/src/cargo/sources/git/utils.rs +++ b/src/cargo/sources/git/utils.rs @@ -944,6 +944,9 @@ pub fn fetch( let shallow = remote_kind.to_shallow_setting(repo.is_shallow(), gctx); + // Flag to keep track if the rev is a full commit hash + let mut fast_path_rev: bool = false; + let oid_to_fetch = match github_fast_path(repo, remote_url, reference, gctx) { Ok(FastPathRev::UpToDate) => return Ok(()), Ok(FastPathRev::NeedsFetch(rev)) => Some(rev), @@ -984,6 +987,7 @@ pub fn fetch( if rev.starts_with("refs/") { refspecs.push(format!("+{0}:{0}", rev)); } else if let Some(oid_to_fetch) = oid_to_fetch { + fast_path_rev = true; refspecs.push(format!("+{0}:refs/commit/{0}", oid_to_fetch)); } else if !matches!(shallow, gix::remote::fetch::Shallow::NoChange) && rev.parse::().is_ok() @@ -1006,13 +1010,20 @@ pub fn fetch( } } - if let Some(true) = gctx.net_config()?.git_fetch_with_cli { + let result = if let Some(true) = gctx.net_config()?.git_fetch_with_cli { fetch_with_cli(repo, remote_url, &refspecs, tags, gctx) } else if gctx.cli_unstable().gitoxide.map_or(false, |git| git.fetch) { fetch_with_gitoxide(repo, remote_url, refspecs, tags, shallow, gctx) } else { fetch_with_libgit2(repo, remote_url, refspecs, tags, shallow, gctx) + }; + + if fast_path_rev { + if let Some(oid) = oid_to_fetch { + return result.with_context(|| format!("revision {} not found", oid)); + } } + result } /// `gitoxide` uses shallow locks to assure consistency when fetching to and to avoid races, and to write diff --git a/tests/testsuite/git.rs b/tests/testsuite/git.rs index 96fb941a587..8ff5510df97 100644 --- a/tests/testsuite/git.rs +++ b/tests/testsuite/git.rs @@ -4087,7 +4087,10 @@ Caused by: failed to clone into: [ROOT]/home/.cargo/git/db/bitflags-[HASH] Caused by: - process didn't exit successfully: `git fetch --no-tags --force --update-head-ok 'https://github.com/rust-lang/bitflags.git' '+11111b376b93484341c68fbca3ca110ae5cd2790:refs/commit/11111b376b93484341c68fbca3ca110ae5cd2790'` ([EXIT_STATUS]: 128) + revision 11111b376b93484341c68fbca3ca110ae5cd2790 not found + +Caused by: + process didn't exit successfully: `git fetch --no-tags --force --update-head-ok [..] "#]]) .run(); @@ -4125,6 +4128,9 @@ Caused by: Caused by: failed to clone into: [ROOT]/home/.cargo/git/db/bitflags-[HASH] + +Caused by: + revision 11111b376b93484341c68fbca3ca110ae5cd2790 not found ... "#]]) .run();