fix(git): download each locked rev from its own checkout by weihanglo · Pull Request #17275 · rust-lang/cargo

weihanglo · 2026-07-30T05:24:45Z

What does this PR try to resolve?

Fixes #8101
Fixes #12233
Fixes #13641
Fixes #14230
Fixes #16076

This fixes a bug existing for a long time in Cargo.

The bug is like SourceId Hash impl ignores precise ¹
so when a lockfile pins packages from one git URL at several revisions,
SourceMap in PackageSet collapses them into a single GitSource,
and checks out at one of those revisions.

Later when Cargo starts downloading for the other revisions
then either
fail with "failed to find ... in path source",
or serve the wrong revision source.

Here we fix this by
making one GitSource instance able to check out multiple revisions.
GitSource's fingerprint prevoiusly also used package's lockec_rev,
so a package didn't rebuild if a sibling revision was loaded last.

How to test and review this PR?

The first three commits pin the current behavior,
followed by a series of refactors to build the foundation of the last fix commit.

Note

This is not an ideal solution. I am not happy with it.
However, changing the Hash impl for SourceId is a bit risky,
as SourceId rignt now serves three different purposes:

At declaration time (Cargo.toml): what user asked
At resolution time (the actual fetched revision): what user actually got
At record time (Cargo.lock): what user remembered

In the future, we should probably reconsider
SourceMap and SourceId identity and equality issues.

https://github.com/rust-lang/cargo/blob/0d83fa61d55f/src/workspace/source_id.rs#L675-L680 ↩

Cargo can generate a lockfile can pining packages from one git URL at different revisions. However, it may lead to donwload failures ``` fails to download with "failed to find ... in path source". ``` The test asserts the current buggy behavior on both * the fetch that creates the state * a fresh fetch with a cold git cache.

cargo still silently serves new checkout's source code Even a dep is locked at old revision in the lock file,

Like lockfile_with_multiple_revisions_change_code_content but with a branch ref that moves forward

The two fields are always set together and describe one checkout. Let make them together

Intended to reuse in next fix commit

This turns single checkout slot into a map from revision to checkout. The `GitSource::locked_rev` then selects the entry in use. This prepares `GitSource` for serving packages pinned at multiple revisions of one git URL.

This is a preparation for the actual fix that we will fetch a revision not from `GitSource::locked_rev`.

This fixes a bug existing for a long time in Cargo. The bug is like `SourceId` Hash impl ignores `precise` [^1] so when a lockfile pins packages from one git URL at several revisions, `SourceMap` in `PackageSet` collapses them into a single `GitSource`, and checks out at one of those revisions. Later when Cargo starts downloading for the other revisions then either fail with "failed to find ... in path source", or serve the wrong revision source. Here we fix this by making one `GitSource` instance able to check out multiple revisions. GitSource's fingerprint prevoiusly also used package's `lockec_rev`, so a package didn't rebuild if a sibling revision was loaded last. This is not an ideal solution. I am not happy with it. However, changing the `Hash` impl for `SourceId` is a bit risky, as `SourceId` rignt now latest serves thre different purposes: * At declaration time (Cargo.toml): what user asked * At resolution time (the actual fetched revision): what user actually got * At record time (Cargo.lock): what user remembered In the future, we should probably reconsider `SourceMap` and `SourceId` identity and equality issues. [^1]: https://github.com/rust-lang/cargo/blob/0d83fa61d55f/src/workspace/source_id.rs#L675-L680

rustbot · 2026-07-30T05:24:49Z

r? @epage

rustbot has assigned @epage.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

Owners of files modified in this PR: @epage, @weihanglo
@epage, @weihanglo expanded to epage, weihanglo

weihanglo · 2026-07-30T05:34:10Z

    }

+    /// Ensures a path source exists and is fetched for `rev`.
+    fn ensure_checkout(&self, rev: git2::Oid, source_id: SourceId) -> CargoResult<()> {


Looking at this issue from a different angle:

We could teach Cargo to stop producing multi-rev lockfiles, and maybe Cargo should try to resolve to a single rev as possible as it could, which is usually what users want unless explicitly specified. However, multi-rev lockfiles already exist today. I think we should support this kinda of lockfile anyways.

View changes since the review

epage · 2026-07-30T14:14:39Z

Yay, more reasons for me to be against hashes/equality that lie!

One thing that isn't clear from this is the relationship between having multiple revisions in a GitSource and GitSource::locked_rev / GitSource::checked_out_rev.

Are we using one source to fetch everything and then later multiple sources for checking out?

If so, then that really needs to be called out in // HACK comments in the code.

weihanglo · 2026-07-30T15:24:58Z

Yay, more reasons for me to be against hashes/equality that lie!

One thing that isn't clear from this is the relationship between having multiple revisions in a GitSource and GitSource::locked_rev / GitSource::checked_out_rev.

Are we using one source to fetch everything and then later multiple sources for checking out?

If so, then that really needs to be called out in // HACK comments in the code.

It is more like this:

Multiple GitSource instances fetch the same repo with different revision during resolution time
The last wins and occpuies the slot in SourceMap
During download time:
- Before this PR: since there is only one GitSource left, Cargo knows the one and only PathSource associated with that GitSource on the locked_rev.
- With this PR:
  - GitSource still has only one revision before download() is called (which is the locked_rev one). The difference is that it checks if the requested package's SourceId has a precise, and uses that to check out instead (that revision should have already been fetched during resolution / update()).
  - locked_rev stays the same as before. It is the revision this GitSource resolved at update() time, and serve queries with a source id precise (e.g., from manifest)

I am trying to find a place documenting this hack, though I feel like there are a pile of hacks over lies 😞.

epage · 2026-07-30T15:29:03Z

To me, this feels too obscure of an interaction that this seems more likely to trip us up in the future. I don't feel comfortable moving forward with this.

weihanglo · 2026-07-30T18:05:53Z

To me, this feels too obscure of an interaction that this seems more likely to trip us up in the future. I don't feel comfortable moving forward with this.

Don't disagree with that, though in what way it would trip us do you see? Is it like with this fix we settle a "working" behavior that we might not want to offer in the future. Or it is more like the Cargo internal implementation that may hard to refactor in the future?

We know that the entire story is bad. My thought was in #17275 (comment). The hack commit be9f5e7 is minimal IMO (plus 69ae9c1 which is meaingless outside the fix commit). Probably we should look at what one-way door we are closing if merging this. Or any suggestion to make this less obscure? I had a hard time figuring out how to change SourceId identify, as it is partially exposed in public interface.

Regardless, do we want those tests checked in?

epage · 2026-07-30T18:23:39Z

Don't disagree with that, though in what way it would trip us do you see? Is it like with this fix we settle a "working" behavior that we might not want to offer in the future. Or it is more like the Cargo internal implementation that may hard to refactor in the future?

Future contributors are unlikely to take this into account when making changes, either not having problems surface until running tests, after the investgation and development, wasting their time, or breaking things outside of how we test.

weihanglo · 2026-07-30T19:37:55Z

Close in favor of #17279. See the description in #17279 for the reason.

### What does this PR try to resolve? Testes extracted from <rust-lang#17275>. ### How to test and review this PR? CI passes. We adds tests instead of merging the fix because * We are not satisfied with the solution presented in rust-lang#17275. It adds more obscure interactions. The ideal solution should fix the "root cause" — SourceId identity issue. * For the network issue (lockfile_with_multiple_revisions_bump_pkg_version), people can `cargo update` to make revision aligned (which is usually what people want) * For the bug that silently compiling to wrong revision (the other two tests), that is bad and harder to detect. However, `cargo update` should fix and we could probably have a lint to ensure a single rev in a dependency graph. See <rust-lang#6921>.

weihanglo added 9 commits July 30, 2026 01:12

test(git): multi-rev git lockfile building wrong revision

09fb46f

cargo still silently serves new checkout's source code Even a dep is locked at old revision in the lock file,

test(git): multi-rev git lockfile with branch ref

5b09057

Like lockfile_with_multiple_revisions_change_code_content but with a branch ref that moves forward

refactor(git): store checkout short id as GitShortID

a24668f

refactor(git): pair path source with its checkout short id

cc66543

The two fields are always set together and describe one checkout. Let make them together

refactor(git): extract checkout() from update()

323b36a

Intended to reuse in next fix commit

refactor(git): key checkouts by revision

69ae9c1

This turns single checkout slot into a map from revision to checkout. The `GitSource::locked_rev` then selects the entry in use. This prepares `GitSource` for serving packages pinned at multiple revisions of one git URL.

refactor(git): parrameterize fetch_db revision

202b8e3

This is a preparation for the actual fix that we will fetch a revision not from `GitSource::locked_rev`.

rustbot assigned epage Jul 30, 2026

rustbot added A-git Area: anything dealing with git S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 30, 2026

weihanglo commented Jul 30, 2026

View reviewed changes

weihanglo mentioned this pull request Jul 30, 2026

test(git): exercise multi git revision lockfile #17279

Merged

weihanglo closed this Jul 30, 2026

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 30, 2026

weihanglo deleted the git-dup branch July 30, 2026 19:38

Navigation Menu

Uh oh!

Uh oh!

fix(git): download each locked rev from its own checkout - #17275

fix(git): download each locked rev from its own checkout#17275
weihanglo wants to merge 9 commits into
rust-lang:masterfrom
weihanglo:git-dup

weihanglo commented Jul 30, 2026 •

edited

Loading

Uh oh!

rustbot commented Jul 30, 2026

Uh oh!

weihanglo Jul 30, 2026 •

edited by rustbot

Loading

Uh oh!

epage commented Jul 30, 2026

Uh oh!

weihanglo commented Jul 30, 2026 •

edited

Loading

Uh oh!

epage commented Jul 30, 2026

Uh oh!

weihanglo commented Jul 30, 2026

Uh oh!

epage commented Jul 30, 2026

Uh oh!

weihanglo commented Jul 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

weihanglo commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR try to resolve?

How to test and review this PR?

Note

Footnotes

Uh oh!

rustbot commented Jul 30, 2026

Uh oh!

weihanglo Jul 30, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epage commented Jul 30, 2026

Uh oh!

weihanglo commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

epage commented Jul 30, 2026

Uh oh!

weihanglo commented Jul 30, 2026

Uh oh!

epage commented Jul 30, 2026

Uh oh!

weihanglo commented Jul 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

weihanglo commented Jul 30, 2026 •

edited

Loading

weihanglo Jul 30, 2026 •

edited by rustbot

Loading

weihanglo commented Jul 30, 2026 •

edited

Loading