fix(cargo-update): --precise accept arbitrary git revisions
#13250
Conversation
This demonstrates the bug described in 13188
`cargo update --precise` might pass in any arbitrary Git reference, and `git2::Oid::from_str` would always zero-pad the given str if it is not a full SHA hex string. This introduces an enum `Revision` to represent `locked_rev` that is either deferred or resolved to an actual object ID. When `locked_rev` is a short ID or any other Git reference, Cargo first performs a Git fetch to resolve it (without --offline), and then locks it to an actual commit object ID.
|
r? @ehuss (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
A-git
| fn new(rev: &str) -> Revision { | ||
| let oid = git2::Oid::from_str(rev).ok(); | ||
| match oid { | ||
| // Git object ID is supposed to be a hex string of 20 (SHA1) or 32 (SHA256) bytes. | ||
| // Its length must be double to the underlying bytes (40 or 64), | ||
| // otherwise libgit2 would happily zero-pad the returned oid. | ||
| // See rust-lang/cargo#13188 | ||
| Some(oid) if oid.as_bytes().len() * 2 == rev.len() => Revision::Locked(oid), | ||
| _ => Revision::Deferred(GitReference::Rev(rev.to_string())), |
There was a problem hiding this comment.
Could you add a test for when a tag looks like an Oid?
| [UPDATING] git repository [..] | ||
| [ERROR] Unable to update [..] | ||
|
|
||
| Caused by: | ||
| precise value for git is not a git revision: 0.1.2 | ||
|
|
||
| Caused by: | ||
| unable to parse OID - contains invalid characters; class=Invalid (3) | ||
| revspec '0.1.2' not found; class=Reference (4); code=NotFound (-3) |
There was a problem hiding this comment.
When used with spec, allows you to specify a specific version number to set the package to. If the package comes from a git repository, this can be a git revision (such as a SHA hash or tag).
The documentation only says SHA and doesn't specify if shorts are allowed.
I almost wonder if the reference to tag is the bug, especially when I see errors like this...
There was a problem hiding this comment.
It worked in 1.45.0 before #8363 merged and was documented since then. The doc mentions git revisions, which could be like anything. This could be considered as an old regression that nobody reported until recent.
There was a problem hiding this comment.
BTW the test was added to prevent panic when revision is missing: 1ad6f78.
| @@ -167,7 +167,7 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes | |||
| format!( | |||
| "{} -> #{}", | |||
| removed[0], | |||
| &added[0].source_id().precise_git_fragment().unwrap() | |||
| &added[0].source_id().precise_git_fragment().unwrap()[..8], | |||
There was a problem hiding this comment.
Just a side comment, to be pedantically correct this should probably be using Object::short_id to avoid collisions, but that seems pretty unlikely.
|
Thanks! @bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Update cargo 10 commits in 84976cd699f4aea56cb3a90ce3eedeed9e20d5a5..1cff2ee6b92e0ad3f87c44b70b28f788b2528b3c 2024-01-12 15:55:43 +0000 to 2024-01-16 16:56:57 +0000 - doc: add a heading to highlight "How to find features enabled on dependencies" (rust-lang/cargo#13305) - fix(cargo-update): `--precise` accept arbitrary git revisions (rust-lang/cargo#13250) - Strip debuginfo when debuginfo is not requested (rust-lang/cargo#13257) - Update ahash dependency to 0.8.7 (rust-lang/cargo#13301) - docs: add more links to pkgid spec chapter (rust-lang/cargo#13298) - fix(metadata): Stabilize id format as PackageIDSpec (rust-lang/cargo#12914) - Introduce `-Zprecise-pre-release` unstable flag (rust-lang/cargo#13296) - Delete sentence about parentheses being unsupported in license (rust-lang/cargo#13292) - Add guidance on setting homepage in manifest (rust-lang/cargo#13293) - Clarify the function of the test options (rust-lang/cargo#13236) r? ghost
What does this PR try to resolve?
cargo update --precisemight pass in any arbitrary Git reference,and
git2::Oid::from_strwould always zero-pad the given str if it isnot a full SHA hex string.
This introduces an enum
Revisionto representlocked_revthat is either deferred or resolved to an actual object ID.
When
locked_revis a short ID or any other Git reference,Cargo first performs a Git fetch to resolve it (without --offline),
and then locks it to an actual commit object ID.
This behavior is documented:
How should we test and review this PR?
Review it commit by commit.
Run the repro steps in #13188.
Additional information
Resolves #13188