Apply GitHub fast path even for partial hashes by dtolnay · Pull Request #10807 · rust-lang/cargo (original) (raw)

What does this PR try to resolve?

As flagged in #10079 (comment), it's not great to assume that git SHAs would always be 40-character hex strings. In the future they will be longer.

Git is on a long-term trajectory to move to SHA256 hashes (current status). I suppose when that becomes available/the default it's possible for a 40-digit hex-encoded hash not to be the full hash. Will this fail for that case?

The implementation from #10079 fails in that situation because it turns dependencies of the form { git = "…", rev = "[…40 hex…]" } into fetches with a refspec +[…40 hex…]:refs/commit/[…40 hex…]. That only works if the 40 hex digits are the full long hash of your commit. If it's really a prefix ("short hash") of a 64-hex-digit SHA-256 commit hash, you'd get a failure that resembles:

error: failed to get dependency as a dependency of package repro v0.0.0

Caused by: failed to load source for dependency dependency

Caused by: Unable to update https://github.com/rust-lang/dependency?rev=b30694b4d9b29141298870b7993e9aee10940524

Caused by: revspec 'b30694b4d9b29141298870b7993e9aee10940524' not found; class=Reference (4); code=NotFound (-3)

This PR updates the implementation so that Cargo will curl GitHub to get a resolved long commit hash even if the rev specified for the git dependency in Cargo.toml already looks like a SHA-1 long hash.

Performance considerations

⛔ This reverses a (questionable, negligible) benefit of #10079 of skipping the curl when rev is a long hash and is not already present in the local clone. These curls take 200-250ms on my machine.

🟰 We retain the much larger benefit of #10079 which comes from being able to precisely fetch a single rev, instead of fetching all branches and tags in the upstream repo and hoping to find the rev somewhere in there. This accounts for the entire performance difference explained in the summary of that PR.

🟰 We still skip the curl when rev is a long hash of a commit that is already previously fetched.

🥳 After this PR, we also curl and hit fast path when rev is a short hash of some upstream commit. For example { git = "https://github.com/rust-lang/cargo", rev = "b30694b4d9" } would previously have done the download-all-branches-and-tags codepath because b30694b4d9 is not a long hash. After this PR, the curl to GitHub informs us that b30694b4d9 resolves to the long hash b30694b4d9b29141298870b7993e9aee10940524, and we download just that commit instead of all-branches-and-tags.

How should we test and review this PR?

I tested with the following dependency specs, using /path/to/target/release/cargo generate-lockfile.

Before: slow path (fetch all branches and tags; 70K git objects)

After: fast path (20K git objects)

cargo = { git = "https://github.com/rust-lang/cargo", rev = "b30694b4d9b2914129" }

Before and after: fast path

cargo = { git = "https://github.com/rust-lang/cargo", rev = "b30694b4d9b29141298870b7993e9aee10940524" }

Before and after: fast path

cargo = { git = "https://github.com/rust-lang/cargo", rev = "refs/heads/rust-1.14.0" }

Before and after: same error "revspec 'rust-1.14.0' not found"

You are supposed to use branch = "rust-1.14.0", this is not considered a rev

cargo = { git = "https://github.com/rust-lang/cargo", rev = "rust-1.14.0" }

I made sure these all work both with and without rm -rf ~/.cargo/git/db/cargo-* ~/.cargo/git/checkouts/cargo-* in between each cargo invocation.

FYI @djc