From 9c9dc577f015605356ba4158f8b6dc1223a8b36c Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 2 Apr 2024 22:55:03 -0400 Subject: [PATCH] Fix fetch --- crates/uv-git/src/git.rs | 137 ++++++++++++++++----------- crates/uv-git/src/lib.rs | 6 +- crates/uv/tests/pip_compile.rs | 163 ++++++++++++--------------------- 3 files changed, 150 insertions(+), 156 deletions(-) diff --git a/crates/uv-git/src/git.rs b/crates/uv-git/src/git.rs index 9ad898e2de36..cd37c9fc2a0c 100644 --- a/crates/uv-git/src/git.rs +++ b/crates/uv-git/src/git.rs @@ -32,7 +32,7 @@ pub enum GitReference { /// From a named reference, like `refs/pull/493/head`. NamedRef(String), /// From a specific revision, using a full 40-character commit hash. - Commit(String), + FullCommit(String), /// The default branch of the repository, the reference named `HEAD`. DefaultBranch, } @@ -52,7 +52,7 @@ impl GitReference { Self::NamedRef(rev.to_owned()) } else if looks_like_commit_hash(rev) { if rev.len() == 40 { - Self::Commit(rev.to_owned()) + Self::FullCommit(rev.to_owned()) } else { Self::BranchOrTagOrCommit(rev.to_owned()) } @@ -65,7 +65,7 @@ impl GitReference { pub fn as_str(&self) -> Option<&str> { match self { Self::BranchOrTag(rev) => Some(rev), - Self::Commit(rev) => Some(rev), + Self::FullCommit(rev) => Some(rev), Self::BranchOrTagOrCommit(rev) => Some(rev), Self::NamedRef(rev) => Some(rev), Self::DefaultBranch => None, @@ -76,7 +76,7 @@ impl GitReference { pub(crate) fn as_rev(&self) -> &str { match self { Self::BranchOrTag(rev) - | Self::Commit(rev) + | Self::FullCommit(rev) | Self::BranchOrTagOrCommit(rev) | Self::NamedRef(rev) => rev, Self::DefaultBranch => "HEAD", @@ -87,7 +87,7 @@ impl GitReference { pub(crate) fn kind_str(&self) -> &str { match self { Self::BranchOrTag(_) => "branch or tag", - Self::Commit(_) => "commit", + Self::FullCommit(_) => "commit", Self::BranchOrTagOrCommit(_) => "branch, tag, or commit", Self::NamedRef(_) => "ref", Self::DefaultBranch => "default branch", @@ -169,7 +169,7 @@ impl GitRemote { strategy: FetchStrategy, client: &Client, ) -> Result<(GitDatabase, git2::Oid)> { - let locked_ref = locked_rev.map(|oid| GitReference::Commit(oid.to_string())); + let locked_ref = locked_rev.map(|oid| GitReference::FullCommit(oid.to_string())); let reference = locked_ref.as_ref().unwrap_or(reference); if let Some(mut db) = db { fetch(&mut db.repo, self.url.as_str(), reference, strategy, client) @@ -283,6 +283,35 @@ impl GitReference { .ok_or_else(|| anyhow::format_err!("failed to find {refkind} `{s}`"))? } + // Attempt to resolve the branch, then the tag, then the commit. + Self::BranchOrTagOrCommit(s) => { + let name = format!("origin/{s}"); + + // Resolve the remote name since that's all we're configuring in + // `fetch` below. + repo.find_branch(&name, git2::BranchType::Remote) + .ok() + .and_then(|b| b.get().target()) + .or_else(|| { + // Note that we resolve the named tag here in sync with where it's + // fetched into via `fetch` below. + let refname = format!("refs/remotes/origin/tags/{s}"); + let id = repo.refname_to_id(&refname).ok()?; + let obj = repo.find_object(id, None).ok()?; + let obj = obj.peel(ObjectType::Commit).ok()?; + Some(obj.id()) + }) + .or_else(|| { + // Resolve the commit. + let obj = repo.revparse_single(s).ok()?; + match obj.as_tag() { + Some(tag) => Some(tag.target_id()), + None => Some(obj.id()), + } + }) + .ok_or_else(|| anyhow::format_err!("failed to find {refkind} `{s}`"))? + } + // We'll be using the HEAD commit Self::DefaultBranch => { let head_id = repo.refname_to_id("refs/remotes/origin/HEAD")?; @@ -290,7 +319,7 @@ impl GitReference { head.peel(ObjectType::Commit)?.id() } - Self::Commit(s) | Self::BranchOrTagOrCommit(s) | Self::NamedRef(s) => { + Self::FullCommit(s) | Self::NamedRef(s) => { let obj = repo.revparse_single(s)?; match obj.as_tag() { Some(tag) => tag.target_id(), @@ -511,7 +540,7 @@ impl<'a> GitCheckout<'a> { // Fetch data from origin and reset to the head commit debug!("Updating Git submodule: {}", child_remote_url); - let reference = GitReference::Commit(head.to_string()); + let reference = GitReference::FullCommit(head.to_string()); fetch(&mut repo, &child_remote_url, &reference, strategy, client).with_context( || { format!( @@ -930,6 +959,26 @@ pub(crate) fn fetch( refspec_strategy = RefspecStrategy::First; } + // 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) => { + // 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. + if let Some(oid_to_fetch) = + oid_to_fetch.filter(|oid| is_short_hash_of(branch_or_tag_or_commit, *oid)) + { + refspecs.push(format!("+{oid_to_fetch}:refs/commit/{oid_to_fetch}")); + } else { + // We don't know what the rev will point to. To handle this + // situation we fetch all branches and tags, and then we pray + // it's somewhere in there. + refspecs.push(String::from("+refs/heads/*:refs/remotes/origin/*")); + refspecs.push(String::from("+HEAD:refs/remotes/origin/HEAD")); + tags = true; + } + } + GitReference::DefaultBranch => { refspecs.push(String::from("+HEAD:refs/remotes/origin/HEAD")); } @@ -938,7 +987,7 @@ pub(crate) fn fetch( refspecs.push(format!("+{rev}:{rev}")); } - GitReference::Commit(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 { @@ -951,19 +1000,6 @@ pub(crate) fn fetch( refspecs.push(format!("+{rev}:refs/remotes/origin/HEAD")); } } - - GitReference::BranchOrTagOrCommit(_) => { - if let Some(oid_to_fetch) = oid_to_fetch { - refspecs.push(format!("+{oid_to_fetch}:refs/commit/{oid_to_fetch}")); - } else { - // We don't know what the rev will point to. To handle this - // situation we fetch all branches and tags, and then we pray - // it's somewhere in there. - refspecs.push(String::from("+refs/heads/*:refs/remotes/origin/*")); - refspecs.push(String::from("+HEAD:refs/remotes/origin/HEAD")); - tags = true; - } - } } debug!("Performing a Git fetch for: {remote_url}"); @@ -976,8 +1012,12 @@ pub(crate) fn fetch( let mut errors = refspecs .iter() .map_while(|refspec| { - let fetch_result = - fetch_with_cli(repo, remote_url, &[refspec.clone()], tags); + let fetch_result = fetch_with_cli( + repo, + remote_url, + std::slice::from_ref(refspec), + tags, + ); // Stop after the first success and log failures match fetch_result { @@ -1292,35 +1332,30 @@ fn github_fast_path( GitReference::BranchOrTag(branch_or_tag) => branch_or_tag, GitReference::DefaultBranch => "HEAD", GitReference::NamedRef(rev) => rev, - GitReference::Commit(rev) | GitReference::BranchOrTagOrCommit(rev) => { - if looks_like_commit_hash(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 - // valid hex. - // - // We only return early if `rev` is a prefix of the object found - // by `revparse_single`. Don't bother talking to GitHub in that - // case, since commit hashes are permanent. If a commit with the - // requested hash is already present in the local clone, its - // contents must be the same as what is on the server for that - // hash. - // - // If `rev` is not found locally by `revparse_single`, we'll - // need GitHub to resolve it and get a hash. If `rev` is found - // but is not a short hash of the found object, it's probably a - // branch and we also need to get a hash from GitHub, in case - // the branch has moved. - if let Some(local_object) = local_object { - if is_short_hash_of(rev, local_object) { - return Ok(FastPathRev::UpToDate); - } + GitReference::FullCommit(rev) | 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 + // valid hex. + // + // We only return early if `rev` is a prefix of the object found + // by `revparse_single`. Don't bother talking to GitHub in that + // case, since commit hashes are permanent. If a commit with the + // requested hash is already present in the local clone, its + // contents must be the same as what is on the server for that + // hash. + // + // If `rev` is not found locally by `revparse_single`, we'll + // need GitHub to resolve it and get a hash. If `rev` is found + // but is not a short hash of the found object, it's probably a + // branch and we also need to get a hash from GitHub, in case + // the branch has moved. + if let Some(local_object) = local_object { + if is_short_hash_of(rev, local_object) { + return Ok(FastPathRev::UpToDate); } - rev - } else { - debug!("can't use github fast path with `rev = \"{}\"`", rev); - return Ok(FastPathRev::Indeterminate); } + rev } }; diff --git a/crates/uv-git/src/lib.rs b/crates/uv-git/src/lib.rs index b7a98fb9d390..7198fd071832 100644 --- a/crates/uv-git/src/lib.rs +++ b/crates/uv-git/src/lib.rs @@ -41,7 +41,7 @@ impl GitUrl { /// Returns `true` if the reference is a full commit. pub fn is_full_commit(&self) -> bool { - matches!(self.reference, GitReference::Commit(_)) + matches!(self.reference, GitReference::FullCommit(_)) } /// Return the precise commit, if known. @@ -71,7 +71,7 @@ impl TryFrom for GitUrl { url.set_path(&prefix); } - let precise = if let GitReference::Commit(rev) = &reference { + let precise = if let GitReference::FullCommit(rev) = &reference { Some(GitSha::from_str(rev)?) } else { None @@ -97,7 +97,7 @@ impl From for Url { match git.reference { GitReference::BranchOrTag(rev) | GitReference::NamedRef(rev) - | GitReference::Commit(rev) + | GitReference::FullCommit(rev) | GitReference::BranchOrTagOrCommit(rev) => { url.set_path(&format!("{}@{}", url.path(), rev)); } diff --git a/crates/uv/tests/pip_compile.rs b/crates/uv/tests/pip_compile.rs index e86c8a971db0..6a8e26f08a7f 100644 --- a/crates/uv/tests/pip_compile.rs +++ b/crates/uv/tests/pip_compile.rs @@ -1066,13 +1066,15 @@ fn compile_sdist_url_dependency() -> Result<()> { Ok(()) } -/// Resolve a specific Flask source distribution via a Git HTTPS dependency. +/// Resolve a specific source distribution via a Git HTTPS dependency. #[test] #[cfg(feature = "git")] fn compile_git_https_dependency() -> Result<()> { let context = TestContext::new("3.12"); let requirements_in = context.temp_dir.child("requirements.in"); - requirements_in.write_str("flask @ git+https://github.com/pallets/flask.git")?; + requirements_in.write_str( + "uv-public-pypackage @ git+https://github.com/astral-test/uv-public-pypackage", + )?; // In addition to the standard filters, remove the `main` commit, which will change frequently. let filters: Vec<_> = [(r"@(\d|\w){40}", "@[COMMIT]")] @@ -1087,36 +1089,24 @@ fn compile_git_https_dependency() -> Result<()> { ----- stdout ----- # This file was autogenerated by uv via the following command: # uv pip compile --cache-dir [CACHE_DIR] --exclude-newer 2024-03-25T00:00:00Z requirements.in - blinker==1.7.0 - # via flask - click==8.1.7 - # via flask - flask @ git+https://github.com/pallets/flask.git@[COMMIT] - itsdangerous==2.1.2 - # via flask - jinja2==3.1.3 - # via flask - markupsafe==2.1.5 - # via - # jinja2 - # werkzeug - werkzeug==3.0.1 - # via flask + uv-public-pypackage @ git+https://github.com/astral-test/uv-public-pypackage@[COMMIT] ----- stderr ----- - Resolved 7 packages in [TIME] + Resolved 1 package in [TIME] "###); Ok(()) } -/// Resolve a specific Flask branch via a Git HTTPS dependency. +/// Resolve a specific branch via a Git HTTPS dependency. #[test] #[cfg(feature = "git")] fn compile_git_branch_https_dependency() -> Result<()> { let context = TestContext::new("3.12"); let requirements_in = context.temp_dir.child("requirements.in"); - requirements_in.write_str("flask @ git+https://github.com/pallets/flask.git@1.0.x")?; + requirements_in.write_str( + "uv-public-pypackage @ git+https://github.com/astral-test/uv-public-pypackage@test-branch", + )?; uv_snapshot!(context.compile() .arg("requirements.in"), @r###" @@ -1125,35 +1115,25 @@ fn compile_git_branch_https_dependency() -> Result<()> { ----- stdout ----- # This file was autogenerated by uv via the following command: # uv pip compile --cache-dir [CACHE_DIR] --exclude-newer 2024-03-25T00:00:00Z requirements.in - click==8.1.7 - # via flask - flask @ git+https://github.com/pallets/flask.git@d92b64aa275841b0c9aea3903aba72fbc4275d91 - itsdangerous==2.1.2 - # via flask - jinja2==3.1.3 - # via flask - markupsafe==2.1.5 - # via - # jinja2 - # werkzeug - werkzeug==3.0.1 - # via flask + uv-public-pypackage @ git+https://github.com/astral-test/uv-public-pypackage@0dacfd662c64cb4ceb16e6cf65a157a8b715b979 ----- stderr ----- - Resolved 6 packages in [TIME] + Resolved 1 package in [TIME] "### ); Ok(()) } -/// Resolve a specific Flask tag via a Git HTTPS dependency. +/// Resolve a specific tag via a Git HTTPS dependency. #[test] #[cfg(feature = "git")] fn compile_git_tag_https_dependency() -> Result<()> { let context = TestContext::new("3.12"); let requirements_in = context.temp_dir.child("requirements.in"); - requirements_in.write_str("flask @ git+https://github.com/pallets/flask.git@3.0.0")?; + requirements_in.write_str( + "uv-public-pypackage @ git+https://github.com/astral-test/uv-public-pypackage@test-tag", + )?; uv_snapshot!(context.compile() .arg("requirements.in"), @r###" @@ -1162,38 +1142,53 @@ fn compile_git_tag_https_dependency() -> Result<()> { ----- stdout ----- # This file was autogenerated by uv via the following command: # uv pip compile --cache-dir [CACHE_DIR] --exclude-newer 2024-03-25T00:00:00Z requirements.in - blinker==1.7.0 - # via flask - click==8.1.7 - # via flask - flask @ git+https://github.com/pallets/flask.git@735a4701d6d5e848241e7d7535db898efb62d400 - itsdangerous==2.1.2 - # via flask - jinja2==3.1.3 - # via flask - markupsafe==2.1.5 - # via - # jinja2 - # werkzeug - werkzeug==3.0.1 - # via flask + uv-public-pypackage @ git+https://github.com/astral-test/uv-public-pypackage@0dacfd662c64cb4ceb16e6cf65a157a8b715b979 ----- stderr ----- - Resolved 7 packages in [TIME] + Resolved 1 package in [TIME] + "### + ); + + Ok(()) +} + +/// Resolve a specific tag via a Git HTTPS dependency. +/// +/// In this case, the tag is a date, and thus could feasibly refer to a short commit hash. +#[test] +#[cfg(feature = "git")] +fn compile_git_date_tag_https_dependency() -> Result<()> { + let context = TestContext::new("3.12"); + let requirements_in = context.temp_dir.child("requirements.in"); + requirements_in.write_str( + "uv-public-pypackage @ git+https://github.com/astral-test/uv-public-pypackage@20240402", + )?; + + uv_snapshot!(context.compile() + .arg("requirements.in"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + # This file was autogenerated by uv via the following command: + # uv pip compile --cache-dir [CACHE_DIR] --exclude-newer 2024-03-25T00:00:00Z requirements.in + uv-public-pypackage @ git+https://github.com/astral-test/uv-public-pypackage@0dacfd662c64cb4ceb16e6cf65a157a8b715b979 + + ----- stderr ----- + Resolved 1 package in [TIME] "### ); Ok(()) } -/// Resolve a specific Flask commit via a Git HTTPS dependency. +/// Resolve a specific commit via a Git HTTPS dependency. #[test] #[cfg(feature = "git")] fn compile_git_long_commit_https_dependency() -> Result<()> { let context = TestContext::new("3.12"); let requirements_in = context.temp_dir.child("requirements.in"); requirements_in.write_str( - "flask @ git+https://github.com/pallets/flask.git@d92b64aa275841b0c9aea3903aba72fbc4275d91", + "uv-public-pypackage @ git+https://github.com/astral-test/uv-public-pypackage@0dacfd662c64cb4ceb16e6cf65a157a8b715b979", )?; uv_snapshot!(context.compile() @@ -1203,35 +1198,25 @@ fn compile_git_long_commit_https_dependency() -> Result<()> { ----- stdout ----- # This file was autogenerated by uv via the following command: # uv pip compile --cache-dir [CACHE_DIR] --exclude-newer 2024-03-25T00:00:00Z requirements.in - click==8.1.7 - # via flask - flask @ git+https://github.com/pallets/flask.git@d92b64aa275841b0c9aea3903aba72fbc4275d91 - itsdangerous==2.1.2 - # via flask - jinja2==3.1.3 - # via flask - markupsafe==2.1.5 - # via - # jinja2 - # werkzeug - werkzeug==3.0.1 - # via flask + uv-public-pypackage @ git+https://github.com/astral-test/uv-public-pypackage@0dacfd662c64cb4ceb16e6cf65a157a8b715b979 ----- stderr ----- - Resolved 6 packages in [TIME] + Resolved 1 package in [TIME] "### ); Ok(()) } -/// Resolve a specific Flask commit via a Git HTTPS dependency. +/// Resolve a specific commit via a Git HTTPS dependency. #[test] #[cfg(feature = "git")] fn compile_git_short_commit_https_dependency() -> Result<()> { let context = TestContext::new("3.12"); let requirements_in = context.temp_dir.child("requirements.in"); - requirements_in.write_str("flask @ git+https://github.com/pallets/flask.git@d92b64a")?; + requirements_in.write_str( + "uv-public-pypackage @ git+https://github.com/astral-test/uv-public-pypackage@0dacfd6", + )?; uv_snapshot!(context.compile() .arg("requirements.in"), @r###" @@ -1240,36 +1225,24 @@ fn compile_git_short_commit_https_dependency() -> Result<()> { ----- stdout ----- # This file was autogenerated by uv via the following command: # uv pip compile --cache-dir [CACHE_DIR] --exclude-newer 2024-03-25T00:00:00Z requirements.in - click==8.1.7 - # via flask - flask @ git+https://github.com/pallets/flask.git@d92b64aa275841b0c9aea3903aba72fbc4275d91 - itsdangerous==2.1.2 - # via flask - jinja2==3.1.3 - # via flask - markupsafe==2.1.5 - # via - # jinja2 - # werkzeug - werkzeug==3.0.1 - # via flask + uv-public-pypackage @ git+https://github.com/astral-test/uv-public-pypackage@0dacfd662c64cb4ceb16e6cf65a157a8b715b979 ----- stderr ----- - Resolved 6 packages in [TIME] + Resolved 1 package in [TIME] "### ); Ok(()) } -/// Resolve a specific Flask ref via a Git HTTPS dependency. +/// Resolve a specific ref via a Git HTTPS dependency. #[test] #[cfg(feature = "git")] fn compile_git_refs_https_dependency() -> Result<()> { let context = TestContext::new("3.12"); let requirements_in = context.temp_dir.child("requirements.in"); requirements_in - .write_str("flask @ git+https://github.com/pallets/flask.git@refs/pull/5313/head")?; + .write_str("uv-public-pypackage @ git+https://github.com/astral-test/uv-public-pypackage@refs/pull/4/head")?; uv_snapshot!(context.compile() .arg("requirements.in"), @r###" @@ -1278,24 +1251,10 @@ fn compile_git_refs_https_dependency() -> Result<()> { ----- stdout ----- # This file was autogenerated by uv via the following command: # uv pip compile --cache-dir [CACHE_DIR] --exclude-newer 2024-03-25T00:00:00Z requirements.in - blinker==1.7.0 - # via flask - click==8.1.7 - # via flask - flask @ git+https://github.com/pallets/flask.git@7af0271f4703a71beef8e26d1f5f6f8da04100e6 - itsdangerous==2.1.2 - # via flask - jinja2==3.1.3 - # via flask - markupsafe==2.1.5 - # via - # jinja2 - # werkzeug - werkzeug==3.0.1 - # via flask + uv-public-pypackage @ git+https://github.com/astral-test/uv-public-pypackage@9d01a806f17ddacb9c7b66b1b68574adf790b63f ----- stderr ----- - Resolved 7 packages in [TIME] + Resolved 1 package in [TIME] "### );