From 78639187e8596f3f6bc597c0a4c10eb175d47140 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 21 Jan 2025 14:26:58 -0500 Subject: [PATCH] Remove the `FullCommit` variant from `GitReference` (#10803) ## Summary It's not quite correct for this to be on `GitReference`. It's not a variant that can be created by users, or by us. --- crates/uv-git/src/git.rs | 205 +++++++++++++----------- crates/uv-git/src/lib.rs | 4 +- crates/uv-pypi-types/src/requirement.rs | 12 +- crates/uv-resolver/src/lock/mod.rs | 13 +- crates/uv-resolver/src/redirect.rs | 14 +- crates/uv-workspace/src/pyproject.rs | 1 - 6 files changed, 127 insertions(+), 122 deletions(-) diff --git a/crates/uv-git/src/git.rs b/crates/uv-git/src/git.rs index d793a3db4..81297cd3f 100644 --- a/crates/uv-git/src/git.rs +++ b/crates/uv-git/src/git.rs @@ -4,7 +4,7 @@ use std::env; use std::fmt::Display; use std::path::{Path, PathBuf}; -use std::str::{self, FromStr}; +use std::str::{self}; use std::sync::LazyLock; use anyhow::{Context, Result}; @@ -40,6 +40,14 @@ pub static GIT: LazyLock> = LazyLock::new(|| { }) }); +/// Strategy when fetching refspecs for a [`GitReference`] +enum RefspecStrategy { + /// All refspecs should be fetched, if any fail then the fetch will fail. + All, + /// Stop after the first successful fetch, if none succeed then the fetch will fail. + First, +} + /// A reference to commit or commit-ish. #[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] pub enum GitReference { @@ -49,24 +57,14 @@ pub enum GitReference { Tag(String), /// From a reference that's ambiguously a branch or tag. BranchOrTag(String), - /// From a reference that's ambiguously a short commit, a branch, or a tag. + /// From a reference that's ambiguously a commit, branch, or tag. BranchOrTagOrCommit(String), /// From a named reference, like `refs/pull/493/head`. NamedRef(String), - /// From a specific revision, using a full 40-character commit hash. - FullCommit(String), /// The default branch of the repository, the reference named `HEAD`. DefaultBranch, } -/// Strategy when fetching refspecs for a [`GitReference`] -enum RefspecStrategy { - // All refspecs should be fetched, if any fail then the fetch will fail - All, - // Stop after the first successful fetch, if none succeed then the fetch will fail - First, -} - impl GitReference { /// Creates a [`GitReference`] from an arbitrary revision string, which could represent a /// branch, tag, commit, or named ref. @@ -87,7 +85,6 @@ impl GitReference { Self::Branch(rev) => Some(rev), Self::BranchOrTag(rev) => Some(rev), Self::BranchOrTagOrCommit(rev) => Some(rev), - Self::FullCommit(rev) => Some(rev), Self::NamedRef(rev) => Some(rev), Self::DefaultBranch => None, } @@ -100,7 +97,6 @@ impl GitReference { Self::Branch(rev) => rev, Self::BranchOrTag(rev) => rev, Self::BranchOrTagOrCommit(rev) => rev, - Self::FullCommit(rev) => rev, Self::NamedRef(rev) => rev, Self::DefaultBranch => "HEAD", } @@ -112,21 +108,11 @@ impl GitReference { Self::Branch(_) => "branch", Self::Tag(_) => "tag", Self::BranchOrTag(_) => "branch or tag", - Self::FullCommit(_) => "commit", Self::BranchOrTagOrCommit(_) => "branch, tag, or commit", Self::NamedRef(_) => "ref", Self::DefaultBranch => "default branch", } } - - /// Returns the precise [`GitOid`] of this reference, if it's a full commit. - pub(crate) fn as_sha(&self) -> Option { - if let Self::FullCommit(rev) = self { - Some(GitOid::from_str(rev).expect("Full commit should be exactly 40 characters")) - } else { - None - } - } } impl Display for GitReference { @@ -135,6 +121,83 @@ impl Display for GitReference { } } +/// A Git reference (like a tag or branch) or a specific commit. +#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] +enum ReferenceOrOid<'reference> { + /// A Git reference, like a tag or branch. + Reference(&'reference GitReference), + /// A specific commit. + Oid(GitOid), +} + +impl ReferenceOrOid<'_> { + /// Resolves the [`ReferenceOrOid`] to an object ID with objects the `repo` currently has. + fn resolve(&self, repo: &GitRepository) -> Result { + let refkind = self.kind_str(); + let result = match self { + // Resolve the commit pointed to by the tag. + // + // `^0` recursively peels away from the revision to the underlying commit object. + // This also verifies that the tag indeed refers to a commit. + Self::Reference(GitReference::Tag(s)) => { + repo.rev_parse(&format!("refs/remotes/origin/tags/{s}^0")) + } + + // Resolve the commit pointed to by the branch. + Self::Reference(GitReference::Branch(s)) => repo.rev_parse(&format!("origin/{s}^0")), + + // Attempt to resolve the branch, then the tag. + Self::Reference(GitReference::BranchOrTag(s)) => repo + .rev_parse(&format!("origin/{s}^0")) + .or_else(|_| repo.rev_parse(&format!("refs/remotes/origin/tags/{s}^0"))), + + // Attempt to resolve the branch, then the tag, then the commit. + Self::Reference(GitReference::BranchOrTagOrCommit(s)) => repo + .rev_parse(&format!("origin/{s}^0")) + .or_else(|_| repo.rev_parse(&format!("refs/remotes/origin/tags/{s}^0"))) + .or_else(|_| repo.rev_parse(&format!("{s}^0"))), + + // We'll be using the HEAD commit. + Self::Reference(GitReference::DefaultBranch) => { + repo.rev_parse("refs/remotes/origin/HEAD") + } + + // Resolve a named reference. + Self::Reference(GitReference::NamedRef(s)) => repo.rev_parse(&format!("{s}^0")), + + // Resolve a specific commit. + Self::Oid(s) => repo.rev_parse(&format!("{s}^0")), + }; + + result.with_context(|| anyhow::format_err!("failed to find {refkind} `{self}`")) + } + + /// Returns the kind of this [`ReferenceOrOid`]. + fn kind_str(&self) -> &str { + match self { + Self::Reference(reference) => reference.kind_str(), + Self::Oid(_) => "commit", + } + } + + /// Converts the [`ReferenceOrOid`] to a `str` that can be used as a revision. + fn as_rev(&self) -> &str { + match self { + Self::Reference(r) => r.as_rev(), + Self::Oid(rev) => rev.as_str(), + } + } +} + +impl Display for ReferenceOrOid<'_> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Reference(reference) => write!(f, "{reference}"), + Self::Oid(oid) => write!(f, "{oid}"), + } + } +} + /// A remote repository. It gets cloned into a local [`GitDatabase`]. #[derive(PartialEq, Clone, Debug)] pub(crate) struct GitRemote { @@ -241,8 +304,9 @@ impl GitRemote { locked_rev: Option, client: &ClientWithMiddleware, ) -> Result<(GitDatabase, GitOid)> { - let locked_ref = locked_rev.map(|oid| GitReference::FullCommit(oid.to_string())); - let reference = locked_ref.as_ref().unwrap_or(reference); + let reference = locked_rev + .map(ReferenceOrOid::Oid) + .unwrap_or(ReferenceOrOid::Reference(reference)); let enable_lfs_fetch = env::var(EnvVars::UV_GIT_LFS).is_ok(); if let Some(mut db) = db { @@ -334,42 +398,6 @@ impl GitDatabase { } } -impl GitReference { - /// Resolves self to an object ID with objects the `repo` currently has. - pub(crate) fn resolve(&self, repo: &GitRepository) -> Result { - let refkind = self.kind_str(); - let result = match self { - // Resolve the commit pointed to by the tag. - // - // `^0` recursively peels away from the revision to the underlying commit object. - // This also verifies that the tag indeed refers to a commit. - Self::Tag(s) => repo.rev_parse(&format!("refs/remotes/origin/tags/{s}^0")), - - // Resolve the commit pointed to by the branch. - Self::Branch(s) => repo.rev_parse(&format!("origin/{s}^0")), - - // Attempt to resolve the branch, then the tag. - Self::BranchOrTag(s) => repo - .rev_parse(&format!("origin/{s}^0")) - .or_else(|_| repo.rev_parse(&format!("refs/remotes/origin/tags/{s}^0"))), - - // Attempt to resolve the branch, then the tag, then the commit. - Self::BranchOrTagOrCommit(s) => repo - .rev_parse(&format!("origin/{s}^0")) - .or_else(|_| repo.rev_parse(&format!("refs/remotes/origin/tags/{s}^0"))) - .or_else(|_| repo.rev_parse(&format!("{s}^0"))), - - // We'll be using the HEAD commit. - Self::DefaultBranch => repo.rev_parse("refs/remotes/origin/HEAD"), - - // Resolve a direct commit reference. - Self::FullCommit(s) | Self::NamedRef(s) => repo.rev_parse(&format!("{s}^0")), - }; - - result.with_context(|| anyhow::format_err!("failed to find {refkind} `{self}`")) - } -} - impl GitCheckout { /// Creates an instance of [`GitCheckout`]. This doesn't imply the checkout /// is done. Use [`GitCheckout::is_fresh`] to check. @@ -469,10 +497,10 @@ impl GitCheckout { /// * Dispatches `git fetch` using the git CLI. /// /// The `remote_url` argument is the git remote URL where we want to fetch from. -pub(crate) fn fetch( +fn fetch( repo: &mut GitRepository, remote_url: &Url, - reference: &GitReference, + reference: ReferenceOrOid<'_>, client: &ClientWithMiddleware, ) -> Result<()> { let oid_to_fetch = match github_fast_path(repo, remote_url, reference, client) { @@ -496,15 +524,15 @@ pub(crate) fn fetch( match reference { // For branches and tags we can fetch simply one reference and copy it // locally, no need to fetch other branches/tags. - GitReference::Branch(branch) => { + ReferenceOrOid::Reference(GitReference::Branch(branch)) => { refspecs.push(format!("+refs/heads/{branch}:refs/remotes/origin/{branch}")); } - GitReference::Tag(tag) => { + ReferenceOrOid::Reference(GitReference::Tag(tag)) => { refspecs.push(format!("+refs/tags/{tag}:refs/remotes/origin/tags/{tag}")); } - GitReference::BranchOrTag(branch_or_tag) => { + ReferenceOrOid::Reference(GitReference::BranchOrTag(branch_or_tag)) => { refspecs.push(format!( "+refs/heads/{branch_or_tag}:refs/remotes/origin/{branch_or_tag}" )); @@ -516,7 +544,7 @@ pub(crate) fn fetch( // For ambiguous references, we can fetch the exact commit (if known); otherwise, // we fetch all branches and tags. - GitReference::BranchOrTagOrCommit(branch_or_tag_or_commit) => { + ReferenceOrOid::Reference(GitReference::BranchOrTagOrCommit(branch_or_tag_or_commit)) => { // The `oid_to_fetch` is the exact commit we want to fetch. But it could be the exact // commit of a branch or tag. We should only fetch it directly if it's the exact commit // of a short commit hash. @@ -534,26 +562,16 @@ pub(crate) fn fetch( } } - GitReference::DefaultBranch => { + ReferenceOrOid::Reference(GitReference::DefaultBranch) => { refspecs.push(String::from("+HEAD:refs/remotes/origin/HEAD")); } - GitReference::NamedRef(rev) => { + ReferenceOrOid::Reference(GitReference::NamedRef(rev)) => { refspecs.push(format!("+{rev}:{rev}")); } - GitReference::FullCommit(rev) => { - if let Some(oid_to_fetch) = oid_to_fetch { - refspecs.push(format!("+{oid_to_fetch}:refs/commit/{oid_to_fetch}")); - } else { - // There is a specific commit to fetch and we will do so in shallow-mode only - // to not disturb the previous logic. - // Note that with typical settings for shallowing, we will just fetch a single `rev` - // as single commit. - // The reason we write to `refs/remotes/origin/HEAD` is that it's of special significance - // when during `GitReference::resolve()`, but otherwise it shouldn't matter. - refspecs.push(format!("+{rev}:refs/remotes/origin/HEAD")); - } + ReferenceOrOid::Oid(rev) => { + refspecs.push(format!("+{rev}:refs/commit/{rev}")); } } @@ -594,7 +612,7 @@ pub(crate) fn fetch( }; match reference { // With the default branch, adding context is confusing - GitReference::DefaultBranch => result, + ReferenceOrOid::Reference(GitReference::DefaultBranch) => result, _ => result.with_context(|| { format!( "failed to fetch {} `{}`", @@ -710,7 +728,7 @@ enum FastPathRev { fn github_fast_path( git: &mut GitRepository, url: &Url, - reference: &GitReference, + reference: ReferenceOrOid<'_>, client: &ClientWithMiddleware, ) -> Result { let Some(GitHubRepository { owner, repo }) = GitHubRepository::parse(url) else { @@ -720,12 +738,12 @@ fn github_fast_path( let local_object = reference.resolve(git).ok(); let github_branch_name = match reference { - GitReference::Branch(branch) => branch, - GitReference::Tag(tag) => tag, - GitReference::BranchOrTag(branch_or_tag) => branch_or_tag, - GitReference::DefaultBranch => "HEAD", - GitReference::NamedRef(rev) => rev, - GitReference::BranchOrTagOrCommit(rev) => { + ReferenceOrOid::Reference(GitReference::DefaultBranch) => "HEAD", + ReferenceOrOid::Reference(GitReference::Branch(branch)) => branch, + ReferenceOrOid::Reference(GitReference::Tag(tag)) => tag, + ReferenceOrOid::Reference(GitReference::BranchOrTag(branch_or_tag)) => branch_or_tag, + ReferenceOrOid::Reference(GitReference::NamedRef(rev)) => rev, + ReferenceOrOid::Reference(GitReference::BranchOrTagOrCommit(rev)) => { // `revparse_single` (used by `resolve`) is the only way to turn // short hash -> long hash, but it also parses other things, // like branch and tag names, which might coincidentally be @@ -750,12 +768,11 @@ fn github_fast_path( } rev } - GitReference::FullCommit(rev) => { + ReferenceOrOid::Oid(rev) => { debug!("Skipping GitHub fast path; full commit hash provided: {rev}"); - let rev = GitOid::from_str(rev)?; - if let Some(ref local_object) = local_object { - if rev == *local_object { + if let Some(local_object) = local_object { + if rev == local_object { return Ok(FastPathRev::UpToDate); } } diff --git a/crates/uv-git/src/lib.rs b/crates/uv-git/src/lib.rs index 44eaa582e..9609b9116 100644 --- a/crates/uv-git/src/lib.rs +++ b/crates/uv-git/src/lib.rs @@ -31,11 +31,10 @@ pub struct GitUrl { impl GitUrl { /// Create a new [`GitUrl`] from a repository URL and a reference. pub fn from_reference(repository: Url, reference: GitReference) -> Self { - let precise = reference.as_sha(); Self { repository, reference, - precise, + precise: None, } } @@ -117,7 +116,6 @@ impl From for Url { | GitReference::Tag(rev) | GitReference::BranchOrTag(rev) | GitReference::NamedRef(rev) - | GitReference::FullCommit(rev) | GitReference::BranchOrTagOrCommit(rev) => { url.set_path(&format!("{}@{}", url.path(), rev)); } diff --git a/crates/uv-pypi-types/src/requirement.rs b/crates/uv-pypi-types/src/requirement.rs index 29dc9a3ac..3a9dee6c8 100644 --- a/crates/uv-pypi-types/src/requirement.rs +++ b/crates/uv-pypi-types/src/requirement.rs @@ -735,19 +735,15 @@ impl From for RequirementSourceWire { // Put the requested reference in the query. match reference { GitReference::Branch(branch) => { - url.query_pairs_mut() - .append_pair("branch", branch.to_string().as_str()); + url.query_pairs_mut().append_pair("branch", branch.as_str()); } GitReference::Tag(tag) => { - url.query_pairs_mut() - .append_pair("tag", tag.to_string().as_str()); + url.query_pairs_mut().append_pair("tag", tag.as_str()); } GitReference::BranchOrTag(rev) | GitReference::BranchOrTagOrCommit(rev) - | GitReference::NamedRef(rev) - | GitReference::FullCommit(rev) => { - url.query_pairs_mut() - .append_pair("rev", rev.to_string().as_str()); + | GitReference::NamedRef(rev) => { + url.query_pairs_mut().append_pair("rev", rev.as_str()); } GitReference::DefaultBranch => {} } diff --git a/crates/uv-resolver/src/lock/mod.rs b/crates/uv-resolver/src/lock/mod.rs index c35261ae0..d7dc9dab0 100644 --- a/crates/uv-resolver/src/lock/mod.rs +++ b/crates/uv-resolver/src/lock/mod.rs @@ -3511,7 +3511,6 @@ impl From for GitSourceKind { GitReference::BranchOrTag(rev) => GitSourceKind::Rev(rev.to_string()), GitReference::BranchOrTagOrCommit(rev) => GitSourceKind::Rev(rev.to_string()), GitReference::NamedRef(rev) => GitSourceKind::Rev(rev.to_string()), - GitReference::FullCommit(rev) => GitSourceKind::Rev(rev.to_string()), GitReference::DefaultBranch => GitSourceKind::DefaultBranch, } } @@ -3554,19 +3553,15 @@ fn locked_git_url(git_dist: &GitSourceDist) -> Url { // Put the requested reference in the query. match git_dist.git.reference() { GitReference::Branch(branch) => { - url.query_pairs_mut() - .append_pair("branch", branch.to_string().as_str()); + url.query_pairs_mut().append_pair("branch", branch.as_str()); } GitReference::Tag(tag) => { - url.query_pairs_mut() - .append_pair("tag", tag.to_string().as_str()); + url.query_pairs_mut().append_pair("tag", tag.as_str()); } GitReference::BranchOrTag(rev) | GitReference::BranchOrTagOrCommit(rev) - | GitReference::NamedRef(rev) - | GitReference::FullCommit(rev) => { - url.query_pairs_mut() - .append_pair("rev", rev.to_string().as_str()); + | GitReference::NamedRef(rev) => { + url.query_pairs_mut().append_pair("rev", rev.as_str()); } GitReference::DefaultBranch => {} } diff --git a/crates/uv-resolver/src/redirect.rs b/crates/uv-resolver/src/redirect.rs index 49c654861..3542dbf36 100644 --- a/crates/uv-resolver/src/redirect.rs +++ b/crates/uv-resolver/src/redirect.rs @@ -1,5 +1,6 @@ use url::Url; -use uv_git::{GitReference, GitResolver}; + +use uv_git::GitResolver; use uv_pep508::VerbatimUrl; use uv_pypi_types::{ParsedGitUrl, ParsedUrl, VerbatimParsedUrl}; @@ -14,12 +15,11 @@ pub(crate) fn url_to_precise(url: VerbatimParsedUrl, git: &GitResolver) -> Verba }; let Some(new_git_url) = git.precise(git_url.clone()) else { - debug_assert!( - matches!(git_url.reference(), GitReference::FullCommit(_)), - "Unseen Git URL: {}, {git_url:?}", - url.verbatim, - ); - return url; + if cfg!(debug_assertions) { + panic!("Unresolved Git URL: {}, {git_url:?}", url.verbatim,); + } else { + return url; + } }; let new_parsed_url = ParsedGitUrl { diff --git a/crates/uv-workspace/src/pyproject.rs b/crates/uv-workspace/src/pyproject.rs index 43d2d2b09..e6ae3a0b2 100644 --- a/crates/uv-workspace/src/pyproject.rs +++ b/crates/uv-workspace/src/pyproject.rs @@ -1423,7 +1423,6 @@ impl Source { } => { if rev.is_none() && tag.is_none() && branch.is_none() { let rev = match reference { - GitReference::FullCommit(ref mut rev) => Some(mem::take(rev)), GitReference::Branch(ref mut rev) => Some(mem::take(rev)), GitReference::Tag(ref mut rev) => Some(mem::take(rev)), GitReference::BranchOrTag(ref mut rev) => Some(mem::take(rev)),