Skip to content

Commit

Permalink
Skip GitHub fast path when full commit is already known (#10800)
Browse files Browse the repository at this point in the history
## Summary

If we have a `GitReference::FullCommit`, we don't need to go to GitHub
to resolve the SHA. We already have it!
  • Loading branch information
charliermarsh authored Jan 21, 2025
1 parent 9552c0a commit 29226b0
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 20 deletions.
27 changes: 16 additions & 11 deletions crates/uv-git/src/git.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,7 @@ impl GitReference {
if rev.starts_with("refs/") {
Self::NamedRef(rev)
} else if looks_like_commit_hash(&rev) {
if rev.len() == 40 {
Self::FullCommit(rev)
} else {
Self::BranchOrTagOrCommit(rev)
}
Self::BranchOrTagOrCommit(rev)
} else {
Self::BranchOrTag(rev)
}
Expand Down Expand Up @@ -729,7 +725,7 @@ fn github_fast_path(
GitReference::BranchOrTag(branch_or_tag) => branch_or_tag,
GitReference::DefaultBranch => "HEAD",
GitReference::NamedRef(rev) => rev,
GitReference::FullCommit(rev) | GitReference::BranchOrTagOrCommit(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
Expand All @@ -754,12 +750,21 @@ fn github_fast_path(
}
rev
}
};
GitReference::FullCommit(rev) => {
debug!("Skipping GitHub fast path; full commit hash provided: {rev}");

// TODO(charlie): If we _know_ that we have a full commit SHA, there's no need to perform this
// request. We can just return `FastPathRev::NeedsFetch`. However, we need to audit all uses of
// `GitReference::FullCommit` to ensure that we _know_ it's a SHA, as opposed to (e.g.) a Git
// tag that just "looks like" a commit (i.e., a tag composed of 40 hex characters).
let rev = GitOid::from_str(rev)?;
if let Some(ref local_object) = local_object {
if rev == *local_object {
return Ok(FastPathRev::UpToDate);
}
}

// If we know the reference is a full commit hash, we can just return it without
// querying GitHub.
return Ok(FastPathRev::NeedsFetch(rev));
}
};

let url = format!("https://api.github.com/repos/{owner}/{repo}/commits/{github_branch_name}");

Expand Down
2 changes: 1 addition & 1 deletion crates/uv/tests/it/branching_urls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -667,7 +667,7 @@ fn branching_urls_of_different_sources_disjoint() -> Result<()> {
[package.metadata]
requires-dist = [
{ name = "iniconfig", marker = "python_full_version < '3.12'", url = "https://files.pythonhosted.org/packages/9b/dd/b3c12c6d707058fa947864b67f0c4e0c39ef8610988d7baea9578f3c48f3/iniconfig-1.1.1-py2.py3-none-any.whl" },
{ name = "iniconfig", marker = "python_full_version >= '3.12'", git = "https://github.com/pytest-dev/iniconfig?rev=93f5930e668c0d1ddf4597e38dd0dea4e2665e7a#93f5930e668c0d1ddf4597e38dd0dea4e2665e7a" },
{ name = "iniconfig", marker = "python_full_version >= '3.12'", git = "https://github.com/pytest-dev/iniconfig?rev=93f5930e668c0d1ddf4597e38dd0dea4e2665e7a" },
]
[[package]]
Expand Down
6 changes: 3 additions & 3 deletions crates/uv/tests/it/lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -582,7 +582,7 @@ fn lock_sdist_git_subdirectory() -> Result<()> {
]

[package.metadata]
requires-dist = [{ name = "example-pkg-a", git = "https://github.com/pypa/sample-namespace-packages.git?subdirectory=pkg_resources%2Fpkg_a&rev=df7530eeb8fa0cb7dbb8ecb28363e8e36bfa2f45#df7530eeb8fa0cb7dbb8ecb28363e8e36bfa2f45" }]
requires-dist = [{ name = "example-pkg-a", git = "https://github.com/pypa/sample-namespace-packages.git?subdirectory=pkg_resources%2Fpkg_a&rev=df7530eeb8fa0cb7dbb8ecb28363e8e36bfa2f45" }]
"###
);
});
Expand Down Expand Up @@ -742,7 +742,7 @@ fn lock_sdist_git_pep508() -> Result<()> {
]

[package.metadata]
requires-dist = [{ name = "uv-public-pypackage", git = "https://github.com/astral-test/uv-public-pypackage.git?rev=0dacfd662c64cb4ceb16e6cf65a157a8b715b979#0dacfd662c64cb4ceb16e6cf65a157a8b715b979" }]
requires-dist = [{ name = "uv-public-pypackage", git = "https://github.com/astral-test/uv-public-pypackage.git?rev=0dacfd662c64cb4ceb16e6cf65a157a8b715b979" }]

[[package]]
name = "uv-public-pypackage"
Expand Down Expand Up @@ -799,7 +799,7 @@ fn lock_sdist_git_pep508() -> Result<()> {
]

[package.metadata]
requires-dist = [{ name = "uv-public-pypackage", git = "https://github.com/astral-test/uv-public-pypackage.git?rev=b270df1a2fb5d012294e9aaf05e7e0bab1e6a389#b270df1a2fb5d012294e9aaf05e7e0bab1e6a389" }]
requires-dist = [{ name = "uv-public-pypackage", git = "https://github.com/astral-test/uv-public-pypackage.git?rev=b270df1a2fb5d012294e9aaf05e7e0bab1e6a389" }]

[[package]]
name = "uv-public-pypackage"
Expand Down
14 changes: 9 additions & 5 deletions crates/uv/tests/it/pip_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1725,7 +1725,7 @@ fn install_git_public_https_missing_commit() {

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(.exe)? fetch .*`", "`git fetch [...]`"));
filters.push(("`.*/git(.exe)? rev-parse .*`", "`git rev-parse [...]`"));
filters.push(("exit status", "exit code"));

// There are flakes on Windows where this irrelevant error is appended
Expand All @@ -1745,11 +1745,15 @@ fn install_git_public_https_missing_commit() {
----- stderr -----
× Failed to download and build `uv-public-pypackage @ git+https://github.com/astral-test/uv-public-pypackage@79a935a7a1a0ad6d0bdf72dce0e16cb0a24a1b3b`
├─▶ Git operation failed
├─▶ failed to clone into: [CACHE_DIR]/git-v0/db/8dab139913c4b566
├─▶ failed to fetch commit `79a935a7a1a0ad6d0bdf72dce0e16cb0a24a1b3b`
╰─▶ process didn't exit successfully: `git fetch [...]` (exit code: 128)
├─▶ failed to find branch, tag, or commit `79a935a7a1a0ad6d0bdf72dce0e16cb0a24a1b3b`
╰─▶ process didn't exit successfully: `git rev-parse [...]` (exit code: 128)
--- stdout
79a935a7a1a0ad6d0bdf72dce0e16cb0a24a1b3b^0
--- stderr
fatal: remote error: upload-pack: not our ref 79a935a7a1a0ad6d0bdf72dce0e16cb0a24a1b3b
fatal: ambiguous argument '79a935a7a1a0ad6d0bdf72dce0e16cb0a24a1b3b^0': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
"###);
}

Expand Down

0 comments on commit 29226b0

Please sign in to comment.