fix(install): check if git url contains a rev by dawedawe · Pull Request #15489 · rust-lang/cargo

dawedawe · 2025-05-04T20:27:43Z

Check if the git url contains a '#' which is likely a sign for the user's desire to specify a rev. Give hint to use --rev

Fixes #15336

rustbot · 2025-05-04T20:27:48Z

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

epage · 2025-05-05T17:00:02Z

+        if url.contains('#') {
+            return Err(anyhow::format_err!(
+                "invalid git url to install from\n\n\
+                    help: use `--rev <SHA>` to specify a commit"
+            )
+            .into());
+        }
+


Is this definitively a bad URL or should this be a hint on the existing error?

I think it's definitely a bad URL, yes.
From the github page:

I can't think of a valid git url with a # besides the rev.

Cargo supports more than Github. The question would be more about the git protocol and what is expected around it as we could be turning a success case into an error case.

From what I'm seeing here, other git servers should be the same.

I might be missing it but I'm not seeing anything at that link saying servers will reject fragments.

Sorry, I don't remember what I thought back then.

0xPoe

I did some digging through the history to understand why we have this issue.

Cargo has long used # internally to serialize the resolved git revision in SourceId (e465ace, 2014), which is reasonable for lockfile/source tracking.

cargo install --git was added later in PR #2026 / commit bc60f64 (2015). It parsed the user-provided git URL but did not reject URL fragments. Install tracking then serialized the package source as: original URL + Cargo's internal #<resolved_commit>.

So https://host/repo.git#foo became git+https://host/repo.git#foo#actual_commit.

Thanks for working on this. Not sure if you still want to revisit this one, but I think the check fragment seems reasonable here.

View changes since this review

0xPoe · 2026-06-30T20:42:44Z

@rustbot author

rustbot · 2026-06-30T20:42:48Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

dawedawe · 2026-06-30T22:56:39Z

@rustbot ready

0xPoe

@epage I'm not sure whether we need to consider this a breaking change or just a bug. It does look like a bug, but I'm not sure if someone would really rely on this. What are your thoughts on this?

View changes since this review

0xPoe · 2026-07-01T21:21:29Z

+fn install_git_with_rev_in_url() {
+    let p = git::repo(&paths::root().join("foo"))
+        .file("Cargo.toml", &basic_manifest("foo", "0.1.0"))
+        .file("src/main.rs", "fn main() {}")
+        .build();
+
+    let mut url = p.url().to_string();
+    url.push_str("#0e8d88b5cfc173c5f5a6a0fe0ce1d4e6018600d4");
+
+    cargo_process("install --locked --git")
+        .arg(url.to_string())
+        .with_stderr_data(str![[r#"
+[UPDATING] git repository `[ROOTURL]/foo#0e8d88b5cfc173c5f5a6a0fe0ce1d4e6018600d4`
+[WARNING] spurious network error (3 tries remaining): failed to resolve path '[ROOT]/foo#0e8d88b5cfc173c5f5a6a0fe0ce1d4e6018600d4': No such file or directory; class=Os (2)
+[WARNING] spurious network error (2 tries remaining): failed to resolve path '[ROOT]/foo#0e8d88b5cfc173c5f5a6a0fe0ce1d4e6018600d4': No such file or directory; class=Os (2)
+[WARNING] spurious network error (1 try remaining): failed to resolve path '[ROOT]/foo#0e8d88b5cfc173c5f5a6a0fe0ce1d4e6018600d4': No such file or directory; class=Os (2)
+[ERROR] failed to clone into: [ROOT]/home/.cargo/git/db/foo-[HASH]
+
+Caused by:
+  failed to resolve path '[ROOT]/foo#0e8d88b5cfc173c5f5a6a0fe0ce1d4e6018600d4': No such file or directory; class=Os (2)
+
+"#]])
+        .with_status(101)
+        .run();
+}


#[cargo_test] fn install_git_with_rev_in_url() { let p = git::repo(&paths::root().join("foo")) .file("Cargo.toml", &basic_manifest("foo", "0.1.0")) .file("src/main.rs", "fn main() {}") .build(); let rev = p.revparse_head(); // The malformed URL is passed to libgit2 as a path containing `#`. // Create that path so the clone succeeds and the install can be tracked. let fragment_path = paths::root().join(format!("foo#{rev}")); git2::Repository::clone(p.url().as_str(), &fragment_path).unwrap(); let mut url = p.url().to_string(); url.push('#'); url.push_str(&rev); cargo_process("install --locked --git").arg(url).run(); assert_has_installed_exe(paths::cargo_home(), "foo"); let crates_toml = fs::read_to_string(paths::home().join(".cargo/.crates.toml")).unwrap(); assert!( crates_toml.contains(&format!("git+{}#{rev}#{rev}", p.url())), "{crates_toml}" ); }

This would make this case really catch the buggy behavior from the issue.

@0xPoe , thanks, that's a nice repro indeed.

dawedawe · 2026-07-10T11:12:36Z

Hey @0xPoe, @epage, is there anything more I can do to drive this forward?

weihanglo · 2026-07-10T11:56:25Z

I'm not sure whether we need to consider this a breaking change or just a bug. It does look like a bug, but I'm not sure if someone would really rely on this. What are your thoughts on this?

It is more like a bug indeed, and I sympathize who ever relying on this. Though to be a bit cautious we can give a transition period and turn it a hard warning I guess?

0xPoe · 2026-07-12T20:35:18Z

I'm not sure whether we need to consider this a breaking change or just a bug. It does look like a bug, but I'm not sure if someone would really rely on this. What are your thoughts on this?

It is more like a bug indeed, and I sympathize who ever relying on this. Though to be a bit cautious we can give a transition period and turn it a hard warning I guess?

I tried to check whether that change would have any edge cases. But it seems there are no bad cases, as Git’s standard behavior never allows fragments in the URL: https://git-scm.com/docs/git-clone#_git_urls

https://git-scm.com/docs/http-protocol And from this documentation, Git seems to use only the path and query, and no fragment is used at all. And for most Git platforms, the fragment is used to create the permalink.

But I did notice that npm would accept git+https://github.com/org/foo.git#v1.0.0 as a valid URL to pick version 1.0.0, and so would other JavaScript tools like Yarn and Bun. On the other hand, would their code prove that it is safe to assume there is no # in the repository name at all? Otherwise, they would hit the bug caused by having too many # characters in the URL. https://github.com/pnpm/pnpm/blob/fbf78bd7d26a3da41b384f1ee35539992b4f799b/pnpm11/resolving/git-resolver/src/parseBareSpecifier.ts#L51

So I guess going with the direct error would be OK?

0xPoe · 2026-07-12T20:38:23Z

Hey @0xPoe, @epage, is there anything more I can do to drive this forward?

The changes look good to me. I would like to wait for @weihanglo to check my response and decide whether we should add a transition period before turning it into a hard error.

weihanglo · 2026-07-12T21:12:23Z

Sounds reasonable. Can go either way.

0xPoe

Thanks!

View changes since this review

0xPoe · 2026-07-14T19:56:51Z

@epage Just want to double-check with you before I merge it. Do you think a hard error would be OK here? Or do you prefer we go with a transition period here?

epage · 2026-07-14T20:44:41Z


    let source = if let Some(url) = args.get_one::<String>("git") {
        let url = url.into_url()?;
+        if url.fragment().is_some() {


This change is directly in cargo install.

Does this mean use of fragments in dependencies gets the old behavior?

Does cargo add --git error before we write to the manifest?

View changes since the review

This change is directly in cargo install.

Does this mean use of fragments in dependencies gets the old behavior?

Yes, that behavior isn't touched.

Does cargo add --git error before we write to the manifest?

No, it doesn't error and doesn't warn from what I see.

Yes, that behavior isn't touched.

is there a reason we are putting it in this layer so that it only affects one code path, rather than lower down so it affects all? For example, if we keep the error as is, what if we moved this into SourceId? That doesn't cover git add but it does cover quite a good number of cases.

Mmh, maybe I misunderstand you but wouldn't disallowing a rev in SourceId make all the Cargo.toml files out there that already have such a git url fail to parse?

Not disallowing it in the SourceId URL but in the URL we pass to SourceId::for_git.

Having the error inside of SourceId::for_git escalates a warning to an error if there's a malformed git dependency in a Cargo.toml. I'm afraid this could affect lots of people.
Checking the argument at all callsides or changing the return of SourceId::for_git to keep it as a potential warning is, of course, against the idea to catch all code paths with a central check.

…it rev is in url

rustbot · 2026-07-18T09:24:11Z

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

return error if the git url passed to `SourceId::for_git` contains a # and hint user to remove the fragment or use a flag

rustbot · 2026-10-14T03:29:54Z

☔ The latest upstream changes (possibly #17506) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot assigned epage May 4, 2025

rustbot added A-cli Area: Command-line interface, option parsing, etc. Command-install S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 4, 2025

epage reviewed May 5, 2025

View reviewed changes

Comment thread src/bin/cargo/commands/install.rs Resolved

epage reviewed May 5, 2025

View reviewed changes

dawedawe force-pushed the check_rev_in_git_url branch from 552440b to dcc8cef Compare May 5, 2025 20:51

0xPoe reviewed Jun 30, 2026

View reviewed changes

Comment thread src/bin/cargo/commands/install.rs Resolved

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

rustbot added the S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label Jun 30, 2026

dawedawe force-pushed the check_rev_in_git_url branch from dcc8cef to f9b37b9 Compare June 30, 2026 22:17

This comment has been minimized.

Sign in to view

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Jun 30, 2026

0xPoe reviewed Jul 1, 2026

View reviewed changes

dawedawe force-pushed the check_rev_in_git_url branch from f9b37b9 to b6d0cee Compare July 2, 2026 22:24

0xPoe approved these changes Jul 14, 2026

View reviewed changes

epage reviewed Jul 14, 2026

View reviewed changes

dawedawe force-pushed the check_rev_in_git_url branch from b6d0cee to 8251237 Compare July 18, 2026 09:18

This comment has been minimized.

Sign in to view

test(add): add test to show current behaviour of cargo install when g…

6297887

…it rev is in url

dawedawe force-pushed the check_rev_in_git_url branch from 8251237 to 70f08b0 Compare July 18, 2026 09:24

fix: return error when rev is in git url

5d813a6

return error if the git url passed to `SourceId::for_git` contains a # and hint user to remove the fragment or use a flag

dawedawe force-pushed the check_rev_in_git_url branch from 70f08b0 to 5d813a6 Compare July 18, 2026 09:32

rustbot added the A-manifest Area: Cargo.toml issues label Jul 18, 2026

Conversation

dawedawe commented May 4, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented May 4, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xPoe left a comment • 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!

Uh oh!

0xPoe commented Jun 30, 2026

Uh oh!

rustbot commented Jun 30, 2026

Uh oh!

This comment has been minimized.

dawedawe commented Jun 30, 2026

Uh oh!

0xPoe left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dawedawe commented Jul 10, 2026

Uh oh!

weihanglo commented Jul 10, 2026

Uh oh!

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

Uh oh!

Uh oh!

0xPoe commented Jul 12, 2026

Uh oh!

weihanglo commented Jul 12, 2026

Uh oh!

0xPoe left a comment • 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!

0xPoe commented Jul 14, 2026

Uh oh!

epage Jul 14, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

rustbot commented Jul 18, 2026

Uh oh!

rustbot commented Oct 14, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

dawedawe commented May 4, 2025 •

edited by rustbot

Loading

0xPoe left a comment •

edited by rustbot

Loading

0xPoe left a comment •

edited by rustbot

Loading

0xPoe commented Jul 12, 2026 •

edited

Loading

0xPoe left a comment •

edited by rustbot

Loading

epage Jul 14, 2026 •

edited by rustbot

Loading