Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add error message for github PR url in dep #15003

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use std::ffi::OsStr;
use std::path::{Path, PathBuf};
use std::rc::Rc;
use std::str::{self, FromStr};
use tracing_subscriber::fmt::format;

use crate::core::summary::MissingDependencyError;
use crate::AlreadyPrintedError;
Expand Down Expand Up @@ -2144,6 +2145,8 @@ fn to_dependency_source_id<P: ResolveToPath + Clone>(
.unwrap_or(GitReference::DefaultBranch);
let loc = git.into_url()?;

bail_if_github_pull_request(&name_in_toml, &loc)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit uncomfortable proactively erroring if the URL pattern matches to github, rather than providing context if what the user specified fails. I guess the worst that can happen is github allows you to clone PR URLs and we people report it and we have to remove it to allow its use. Not the worst outcome for something that is unclear if it would ever happen.

Copy link
Author

@joshka joshka Jan 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior here is the same as for how fragments are handled directly below this (though they are handled as a warning. The difference is that this causes an error later in the processing while the fragment does not. I think it's reasonable to avoid attempting to do cause an error by downloading. If this was wrong, then removing this code or changing it to a warning would be fairly low cost. How about we keep this as-is (in this PR)


if let Some(fragment) = loc.fragment() {
let msg = format!(
"URL fragment `#{fragment}` in git URL is ignored for dependency ({name_in_toml}). \
Expand Down Expand Up @@ -2182,6 +2185,27 @@ fn to_dependency_source_id<P: ResolveToPath + Clone>(
}
}

/// Checks if the URL is a GitHub pull request URL.
///
/// If the URL is a GitHub pull request URL, an error is returned with a message that explains
/// how to specify a specific git revision.
fn bail_if_github_pull_request(name_in_toml: &str, url: &Url) -> CargoResult<()> {
if url.host_str() != Some("github.com") {
return Ok(());
}
let path_components = url.path().split('/').collect::<Vec<_>>();
if let ["", owner, repo, "pull", pr_number, ..] = path_components[..] {
let repo_url = format!("https://github.com/{owner}/{repo}.git");
let rev = format!("refs/pull/{pr_number}/head");
bail!(
"dependency ({name_in_toml}) git url {url} is not a repository. \
The path looks like a pull request. Try replacing the dependency with: \
`git = \"{repo_url}\" rev = \"{rev}\"` in the dependency declaration.",
);
}
Ok(())
}

pub(crate) fn lookup_path_base<'a>(
base: &PathBaseName,
gctx: &GlobalContext,
Expand Down
31 changes: 31 additions & 0 deletions tests/testsuite/bad_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2151,6 +2151,37 @@ Caused by:
.run();
}

#[cargo_test]
fn github_pull_request_url() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted in #14941, this should cover patches as well and we should have a test for it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#15001 you mean?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in d743987

let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.0"
edition = "2015"
authors = []

[dependencies.bar]
git = "https://github.com/foo/bar/pull/123"
"#,
)
.file("src/lib.rs", "")
.build();

p.cargo("check -v")
.with_status(101)
.with_stderr_data(str![[r#"
[ERROR] failed to parse manifest at `[ROOT]/foo/Cargo.toml`

Caused by:
dependency (bar) git url https://github.com/foo/bar/pull/123 is not a repository. The path looks like a pull request. Try replacing the dependency with: `git = "https://github.com/foo/bar.git" rev = "refs/pull/123/head"` in the dependency declaration.

"#]])
.run();
}

#[cargo_test]
fn fragment_in_git_url() {
let p = project()
Expand Down
44 changes: 44 additions & 0 deletions tests/testsuite/patch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,50 @@ fn patch_to_git() {
.run();
}

#[cargo_test]
fn patch_to_git_pull_request() {
let bar = git::repo(&paths::root().join("override"))
.file("Cargo.toml", &basic_manifest("bar", "0.1.0"))
.file("src/lib.rs", "pub fn bar() {}")
.build();

Package::new("bar", "0.1.0").publish();

let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.1"
edition = "2015"
authors = []

[dependencies]
bar = "0.1"

[patch.crates-io]
bar = { git = 'https://github.com/foo/bar/pull/123' }
"#,
)
.file(
"src/lib.rs",
"extern crate bar; pub fn foo() { bar::bar(); }",
)
.build();

p.cargo("check -v")
.with_status(101)
.with_stderr_data(str![[r#"
[ERROR] failed to parse manifest at `[ROOT]/foo/Cargo.toml`

Caused by:
dependency (bar) git url https://github.com/foo/bar/pull/123 is not a repository. The path looks like a pull request. Try replacing the dependency with: `git = "https://github.com/foo/bar.git" rev = "refs/pull/123/head"` in the dependency declaration.

"#]])
.run();
}

#[cargo_test]
fn unused() {
Package::new("bar", "0.1.0").publish();
Expand Down
Loading