Skip to content

Commit

Permalink
Improve error message when git ref cannot be fetched (#1826)
Browse files Browse the repository at this point in the history 
Follow-up to #1781 improving the error message when a ref cannot be
fetched
  • Loading branch information
@zanieb
zanieb authored Feb 22, 2024
1 parent f441f8f commit 10be62e
Show file tree
Hide file tree
Showing 3 changed files with 103 additions and 36 deletions.
77 changes: 52 additions & 25 deletions crates/uv-git/src/git.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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<git2::Oid> {
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.
Expand All @@ -255,18 +268,18 @@ 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.
GitReference::Branch(s) => {
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.
Expand All @@ -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
Expand Down Expand Up @@ -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::<Vec<_>>();

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 => {
Expand Down
16 changes: 8 additions & 8 deletions crates/uv/tests/common/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
}

Expand Down
46 changes: 43 additions & 3 deletions crates/uv/tests/pip_install.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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###"
Expand All @@ -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
"###);
}

Expand Down

0 comments on commit 10be62e

Please sign in to comment.