From 10be62e9d3537ca5555b4a3893eff63a814ec519 Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Wed, 21 Feb 2024 19:22:00 -0600 Subject: [PATCH] Improve error message when git ref cannot be fetched (#1826) Follow-up to #1781 improving the error message when a ref cannot be fetched --- crates/uv-git/src/git.rs | 77 +++++++++++++++++++++++----------- crates/uv/tests/common/mod.rs | 16 +++---- crates/uv/tests/pip_install.rs | 46 ++++++++++++++++++-- 3 files changed, 103 insertions(+), 36 deletions(-) diff --git a/crates/uv-git/src/git.rs b/crates/uv-git/src/git.rs index cc72ca7a4..2ea743be1 100644 --- a/crates/uv-git/src/git.rs +++ b/crates/uv-git/src/git.rs @@ -6,12 +6,12 @@ use std::path::{Path, PathBuf}; use std::process::Command; use std::{env, str}; -use anyhow::{anyhow, Context as _, Result}; +use anyhow::{anyhow, Context, Result}; use cargo_util::{paths, ProcessBuilder}; use git2::{self, ErrorClass, ObjectType}; use reqwest::Client; use reqwest::StatusCode; -use tracing::{debug, error, warn}; +use tracing::{debug, warn}; use url::Url; use uv_fs::Normalized; @@ -78,6 +78,18 @@ impl GitReference { GitReference::DefaultBranch => "HEAD", } } + + pub(crate) fn kind_str(&self) -> &str { + match self { + GitReference::Branch(_) => "branch", + GitReference::Tag(_) => "tag", + GitReference::BranchOrTag(_) => "branch or tag", + GitReference::FullCommit(_) => "commit", + GitReference::ShortCommit(_) => "short commit", + GitReference::Ref(_) => "ref", + GitReference::DefaultBranch => "default branch", + } + } } /// A short abbreviated OID. @@ -245,6 +257,7 @@ impl GitDatabase { impl GitReference { /// Resolves self to an object ID with objects the `repo` currently has. pub(crate) fn resolve(&self, repo: &git2::Repository) -> Result { + let refkind = self.kind_str(); let id = match self { // Note that we resolve the named tag here in sync with where it's // fetched into via `fetch` below. @@ -255,7 +268,7 @@ impl GitReference { let obj = obj.peel(ObjectType::Commit)?; Ok(obj.id()) })() - .with_context(|| format!("failed to find tag `{s}`"))?, + .with_context(|| format!("failed to find {refkind} `{s}`"))?, // Resolve the remote name since that's all we're configuring in // `fetch` below. @@ -263,10 +276,10 @@ impl GitReference { let name = format!("origin/{s}"); let b = repo .find_branch(&name, git2::BranchType::Remote) - .with_context(|| format!("failed to find branch `{s}`"))?; + .with_context(|| format!("failed to find {refkind} `{s}`"))?; b.get() .target() - .ok_or_else(|| anyhow::format_err!("branch `{s}` did not have a target"))? + .ok_or_else(|| anyhow::format_err!("{refkind} `{s}` did not have a target"))? } // Attempt to resolve the branch, then the tag. @@ -283,7 +296,7 @@ impl GitReference { let obj = obj.peel(ObjectType::Commit).ok()?; Some(obj.id()) }) - .ok_or_else(|| anyhow::format_err!("failed to find branch or tag `{s}`"))? + .ok_or_else(|| anyhow::format_err!("failed to find {refkind} `{s}`"))? } // We'll be using the HEAD commit @@ -980,36 +993,50 @@ pub(crate) fn fetch( debug!("Performing a Git fetch for: {remote_url}"); match strategy { FetchStrategy::Cli => { - match refspec_strategy { + let result = match refspec_strategy { RefspecStrategy::All => fetch_with_cli(repo, remote_url, refspecs.as_slice(), tags), RefspecStrategy::First => { - let num_refspecs = refspecs.len(); - // Try each refspec - let errors = refspecs - .into_iter() - .map(|refspec| { - ( - refspec.clone(), - fetch_with_cli(repo, remote_url, &[refspec], tags), - ) + let mut errors = refspecs + .iter() + .map_while(|refspec| { + let fetch_result = + fetch_with_cli(repo, remote_url, &[refspec.clone()], tags); + + // Stop after the first success and log failures + match fetch_result { + Err(ref err) => { + debug!("failed to fetch refspec `{refspec}`: {err}"); + Some(fetch_result) + } + Ok(()) => None, + } }) - // Stop after the first success - .take_while(|(_, result)| result.is_err()) .collect::>(); - if errors.len() == num_refspecs { - // If all of the fetches failed, report to the user - for (refspec, err) in errors { - if let Err(err) = err { - error!("failed to fetch refspec `{refspec}`: {err}"); - } + if errors.len() == refspecs.len() { + if let Some(result) = errors.pop() { + // Use the last error for the message + result + } else { + // Can only occur if there were no refspecs to fetch + Ok(()) } - Err(anyhow!("failed to fetch all refspecs")) } else { Ok(()) } } + }; + match reference { + // With the default branch, adding context is confusing + GitReference::DefaultBranch => result, + _ => result.with_context(|| { + format!( + "failed to fetch {} `{}`", + reference.kind_str(), + reference.as_str() + ) + }), } } FetchStrategy::Libgit2 => { diff --git a/crates/uv/tests/common/mod.rs b/crates/uv/tests/common/mod.rs index 12ff31b88..b80532b42 100644 --- a/crates/uv/tests/common/mod.rs +++ b/crates/uv/tests/common/mod.rs @@ -148,15 +148,15 @@ impl TestContext { ] } - /// Canonical snapshot filters for this test context. + /// Standard snapshot filters _plus_ those for this test context. pub fn filters(&self) -> Vec<(&str, &str)> { - let mut filters = INSTA_FILTERS.to_vec(); - - for (pattern, replacement) in &self.filters { - filters.push((pattern, replacement)); - } - - filters + // Put test context snapshots before the default filters + // This ensures we don't replace other patterns inside paths from the test context first + self.filters + .iter() + .map(|(p, r)| (p.as_str(), r.as_str())) + .chain(INSTA_FILTERS.iter().copied()) + .collect() } } diff --git a/crates/uv/tests/pip_install.rs b/crates/uv/tests/pip_install.rs index 890699b77..f24e9fee1 100644 --- a/crates/uv/tests/pip_install.rs +++ b/crates/uv/tests/pip_install.rs @@ -815,10 +815,15 @@ fn install_git_public_https() { /// Install a package from a public GitHub repository at a ref that does not exist #[test] #[cfg(feature = "git")] -fn install_git_public_https_missing_ref() { +fn install_git_public_https_missing_branch_or_tag() { let context = TestContext::new("3.8"); - uv_snapshot!(context.filters(), command(&context) + let mut filters = context.filters(); + // Windows does not style the command the same as Unix, so we must omit it from the snapshot + filters.push(("`git fetch .*`", "`git fetch [...]`")); + filters.push(("exit status", "exit code")); + + uv_snapshot!(filters, command(&context) // 2.0.0 does not exist .arg("uv-public-pypackage @ git+https://github.com/astral-test/uv-public-pypackage@2.0.0") , @r###" @@ -830,7 +835,42 @@ fn install_git_public_https_missing_ref() { error: Failed to download and build: uv-public-pypackage @ git+https://github.com/astral-test/uv-public-pypackage@2.0.0 Caused by: Git operation failed Caused by: failed to clone into: [CACHE_DIR]/git-v0/db/8dab139913c4b566 - Caused by: failed to fetch all refspecs + Caused by: failed to fetch branch or tag `2.0.0` + Caused by: process didn't exit successfully: `git fetch [...]` (exit code: 128) + --- stderr + fatal: couldn't find remote ref refs/tags/2.0.0 + + "###); +} + +/// Install a package from a public GitHub repository at a ref that does not exist +#[test] +#[cfg(feature = "git")] +fn install_git_public_https_missing_commit() { + let context = TestContext::new("3.8"); + + let mut filters = context.filters(); + // Windows does not style the command the same as Unix, so we must omit it from the snapshot + filters.push(("`git fetch .*`", "`git fetch [...]`")); + filters.push(("exit status", "exit code")); + + uv_snapshot!(filters, command(&context) + // 2.0.0 does not exist + .arg("uv-public-pypackage @ git+https://github.com/astral-test/uv-public-pypackage@79a935a7a1a0ad6d0bdf72dce0e16cb0a24a1b3b") + , @r###" + success: false + exit_code: 2 + ----- stdout ----- + + ----- stderr ----- + error: Failed to download and build: uv-public-pypackage @ git+https://github.com/astral-test/uv-public-pypackage@79a935a7a1a0ad6d0bdf72dce0e16cb0a24a1b3b + Caused by: Git operation failed + Caused by: failed to clone into: [CACHE_DIR]/git-v0/db/8dab139913c4b566 + Caused by: failed to fetch commit `79a935a7a1a0ad6d0bdf72dce0e16cb0a24a1b3b` + Caused by: process didn't exit successfully: `git fetch [...]` (exit code: 128) + --- stderr + fatal: remote error: upload-pack: not our ref 79a935a7a1a0ad6d0bdf72dce0e16cb0a24a1b3b + "###); }