Skip to content

Display registry name instead of registry URL when possible #9632

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

Merged
merged 6 commits into from
Jul 21, 2021
Merged
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
8 changes: 5 additions & 3 deletions src/cargo/core/compiler/rustdoc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ use std::fmt;
use std::hash;
use url::Url;

const DOCS_RS_URL: &'static str = "https://docs.rs/";

/// Mode used for `std`.
#[derive(Debug, Hash)]
pub enum RustdocExternMode {
Expand Down Expand Up @@ -63,7 +65,7 @@ pub struct RustdocExternMap {
impl Default for RustdocExternMap {
fn default() -> Self {
let mut registries = HashMap::new();
registries.insert("crates-io".into(), "https://docs.rs/".into());
registries.insert(CRATES_IO_REGISTRY.into(), DOCS_RS_URL.into());
Self {
registries,
std: None,
Expand All @@ -76,8 +78,8 @@ fn default_crates_io_to_docs_rs<'de, D: serde::Deserializer<'de>>(
) -> Result<HashMap<String, String>, D::Error> {
use serde::Deserialize;
let mut registries = HashMap::deserialize(de)?;
if !registries.contains_key("crates-io") {
registries.insert("crates-io".into(), "https://docs.rs/".into());
if !registries.contains_key(CRATES_IO_REGISTRY) {
registries.insert(CRATES_IO_REGISTRY.into(), DOCS_RS_URL.into());
}
Ok(registries)
}
Expand Down
6 changes: 3 additions & 3 deletions src/cargo/core/package_id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,15 +252,15 @@ mod tests {
let loc = CRATES_IO_INDEX.into_url().unwrap();
let pkg_id = PackageId::new("foo", "1.0.0", SourceId::for_registry(&loc).unwrap()).unwrap();
assert_eq!(
r#"PackageId { name: "foo", version: "1.0.0", source: "registry `https://github.com/rust-lang/crates.io-index`" }"#,
r#"PackageId { name: "foo", version: "1.0.0", source: "registry `crates-io`" }"#,
format!("{:?}", pkg_id)
);

let expected = r#"
PackageId {
name: "foo",
version: "1.0.0",
source: "registry `https://github.com/rust-lang/crates.io-index`",
source: "registry `crates-io`",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not keep the original registry url instead of using a short alias?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the intent of #6691, which make it less mess in terminal (maybe?). And yes it gets some rooms for deciding whether URL should be hidden.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it isn't an issue since it's less than 80 characters which terminal will usually expect to be the lowest, of course it can go lower but most stuff will look ugly.

    Updating `https://gitlab.com/xxxxxxxxxx/cargo/crates-index` index

If a short alias is used some amount of information is hidden which could be useful in the logs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. Maybe we can preserve the original behaviour of crates.io, and shorten other custom registries, since we have no idea which contains sensitive information. Is this more reasonable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the url sensitive? If it is sensitive shouldn't the user hide it themselves? I think it is not easily missed as it is at the top line, and it does not contain any credentials.

Copy link
Member Author

Choose a reason for hiding this comment

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

I got the point and totally agreed. It's very reasonable not using short alias. Let's await the opinion from others. Maybe we can come up with better solution.

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 confused about the question here. Are you just talking about the debug display? That is almost never shown anywhere, so I'm not sure it really matters. It'll probably be safer to keep the short name (like if asked to share CARGO_LOG output). If I'm misunderstanding, can you clarify?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not just debug display, it affects the default cargo display as well. Rather than showing the crates url it shows crates-io.

}
"#
.trim();
Expand All @@ -271,7 +271,7 @@ PackageId {
PackageId {
name: "foo",
version: "1.0.0",
source: "registry `https://github.com/rust-lang/crates.io-index`"
source: "registry `crates-io`"
}
"#
.trim();
Expand Down
81 changes: 59 additions & 22 deletions src/cargo/core/source/source_id.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::core::PackageId;
use crate::sources::DirectorySource;
use crate::sources::{GitSource, PathSource, RegistrySource, CRATES_IO_INDEX};
use crate::sources::{DirectorySource, CRATES_IO_DOMAIN, CRATES_IO_INDEX, CRATES_IO_REGISTRY};
use crate::sources::{GitSource, PathSource, RegistrySource};
use crate::util::{CanonicalUrl, CargoResult, Config, IntoUrl};
use log::trace;
use serde::de;
Expand All @@ -24,7 +24,7 @@ pub struct SourceId {
inner: &'static SourceIdInner,
}

#[derive(PartialEq, Eq, Clone, Debug, Hash)]
#[derive(Eq, Clone, Debug)]
struct SourceIdInner {
/// The source URL.
url: Url,
Expand Down Expand Up @@ -73,13 +73,13 @@ impl SourceId {
/// Creates a `SourceId` object from the kind and URL.
///
/// The canonical url will be calculated, but the precise field will not
fn new(kind: SourceKind, url: Url) -> CargoResult<SourceId> {
fn new(kind: SourceKind, url: Url, name: Option<&str>) -> CargoResult<SourceId> {
let source_id = SourceId::wrap(SourceIdInner {
kind,
canonical_url: CanonicalUrl::new(&url)?,
url,
precise: None,
name: None,
name: name.map(|n| n.into()),
});
Ok(source_id)
}
Expand Down Expand Up @@ -132,12 +132,12 @@ impl SourceId {
}
"registry" => {
let url = url.into_url()?;
Ok(SourceId::new(SourceKind::Registry, url)?
Ok(SourceId::new(SourceKind::Registry, url, None)?
.with_precise(Some("locked".to_string())))
}
"path" => {
let url = url.into_url()?;
SourceId::new(SourceKind::Path, url)
SourceId::new(SourceKind::Path, url, None)
}
kind => Err(anyhow::format_err!("unsupported source protocol: {}", kind)),
}
Expand All @@ -155,43 +155,53 @@ impl SourceId {
/// `path`: an absolute path.
pub fn for_path(path: &Path) -> CargoResult<SourceId> {
let url = path.into_url()?;
SourceId::new(SourceKind::Path, url)
SourceId::new(SourceKind::Path, url, None)
}

/// Creates a `SourceId` from a Git reference.
pub fn for_git(url: &Url, reference: GitReference) -> CargoResult<SourceId> {
SourceId::new(SourceKind::Git(reference), url.clone())
SourceId::new(SourceKind::Git(reference), url.clone(), None)
}

/// Creates a SourceId from a registry URL.
/// Creates a SourceId from a remote registry URL when the registry name
/// cannot be determined, e.g. an user passes `--index` directly from CLI.
///
/// Use [`SourceId::for_alt_registry`] if a name can provided, which
/// generates better messages for cargo.
pub fn for_registry(url: &Url) -> CargoResult<SourceId> {
SourceId::new(SourceKind::Registry, url.clone())
SourceId::new(SourceKind::Registry, url.clone(), None)
}

/// Creates a `SourceId` from a remote registry URL with given name.
pub fn for_alt_registry(url: &Url, name: &str) -> CargoResult<SourceId> {
SourceId::new(SourceKind::Registry, url.clone(), Some(name))
}

/// Creates a SourceId from a local registry path.
pub fn for_local_registry(path: &Path) -> CargoResult<SourceId> {
let url = path.into_url()?;
SourceId::new(SourceKind::LocalRegistry, url)
SourceId::new(SourceKind::LocalRegistry, url, None)
}

/// Creates a `SourceId` from a directory path.
pub fn for_directory(path: &Path) -> CargoResult<SourceId> {
let url = path.into_url()?;
SourceId::new(SourceKind::Directory, url)
SourceId::new(SourceKind::Directory, url, None)
}

/// Returns the `SourceId` corresponding to the main repository.
///
/// This is the main cargo registry by default, but it can be overridden in
/// a `.cargo/config`.
/// a `.cargo/config.toml`.
pub fn crates_io(config: &Config) -> CargoResult<SourceId> {
config.crates_io_source_id(|| {
config.check_registry_index_not_set()?;
let url = CRATES_IO_INDEX.into_url().unwrap();
SourceId::for_registry(&url)
SourceId::new(SourceKind::Registry, url, Some(CRATES_IO_REGISTRY))
})
}

/// Gets the `SourceId` associated with given name of the remote regsitry.
pub fn alt_registry(config: &Config, key: &str) -> CargoResult<SourceId> {
let url = config.get_registry_index(key)?;
Ok(SourceId::wrap(SourceIdInner {
Expand All @@ -216,17 +226,21 @@ impl SourceId {

pub fn display_index(self) -> String {
if self.is_default_registry() {
"crates.io index".to_string()
format!("{} index", CRATES_IO_DOMAIN)
} else {
format!("`{}` index", url_display(self.url()))
format!("`{}` index", self.display_registry_name())
}
}

pub fn display_registry_name(self) -> String {
if self.is_default_registry() {
"crates.io".to_string()
CRATES_IO_REGISTRY.to_string()
} else if let Some(name) = &self.inner.name {
name.clone()
} else if self.precise().is_some() {
// We remove `precise` here to retrieve an permissive version of
// `SourceIdInner`, which may contain the registry name.
self.with_precise(None).display_registry_name()
} else {
url_display(self.url())
}
Expand Down Expand Up @@ -463,7 +477,7 @@ impl fmt::Display for SourceId {
Ok(())
}
SourceKind::Path => write!(f, "{}", url_display(&self.inner.url)),
SourceKind::Registry => write!(f, "registry `{}`", url_display(&self.inner.url)),
SourceKind::Registry => write!(f, "registry `{}`", self.display_registry_name()),
SourceKind::LocalRegistry => write!(f, "registry `{}`", url_display(&self.inner.url)),
SourceKind::Directory => write!(f, "dir {}", url_display(&self.inner.url)),
}
Expand All @@ -483,6 +497,29 @@ impl Hash for SourceId {
}
}

impl Hash for SourceIdInner {
/// The hash of `SourceIdInner` is used to retrieve its interned value. We
/// only care about fields that make `SourceIdInner` unique, which are:
///
/// - `kind`
/// - `precise`
/// - `canonical_url`
fn hash<S: hash::Hasher>(&self, into: &mut S) {
self.kind.hash(into);
self.precise.hash(into);
self.canonical_url.hash(into);
}
}

impl PartialEq for SourceIdInner {
/// This implementation must be synced with [`SourceIdInner::hash`].
fn eq(&self, other: &Self) -> bool {
self.kind == other.kind
&& self.precise == other.precise
&& self.canonical_url == other.canonical_url
}
}

// forward to `Ord`
impl PartialOrd for SourceKind {
fn partial_cmp(&self, other: &SourceKind) -> Option<Ordering> {
Expand Down Expand Up @@ -670,15 +707,15 @@ mod tests {
fn github_sources_equal() {
let loc = "https://github.com/foo/bar".into_url().unwrap();
let default = SourceKind::Git(GitReference::DefaultBranch);
let s1 = SourceId::new(default.clone(), loc).unwrap();
let s1 = SourceId::new(default.clone(), loc, None).unwrap();

let loc = "git://github.com/foo/bar".into_url().unwrap();
let s2 = SourceId::new(default, loc.clone()).unwrap();
let s2 = SourceId::new(default, loc.clone(), None).unwrap();

assert_eq!(s1, s2);

let foo = SourceKind::Git(GitReference::Branch("foo".to_string()));
let s3 = SourceId::new(foo, loc).unwrap();
let s3 = SourceId::new(foo, loc, None).unwrap();
assert_ne!(s1, s3);
}
}
4 changes: 3 additions & 1 deletion src/cargo/ops/mod.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use crate::sources::CRATES_IO_DOMAIN;

pub use self::cargo_clean::{clean, CleanOptions};
pub use self::cargo_compile::{
compile, compile_with_exec, compile_ws, create_bcx, print, resolve_all_features, CompileOptions,
Expand Down Expand Up @@ -66,7 +68,7 @@ fn check_dep_has_version(dep: &crate::core::Dependency, publish: bool) -> crate:

if !dep.specified_req() && dep.is_transitive() {
let dep_version_source = dep.registry_id().map_or_else(
|| "crates.io".to_string(),
|| CRATES_IO_DOMAIN.to_string(),
|registry_id| registry_id.display_registry_name(),
);
anyhow::bail!(
Expand Down
6 changes: 3 additions & 3 deletions src/cargo/ops/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use crate::core::resolver::CliFeatures;
use crate::core::source::Source;
use crate::core::{Package, SourceId, Workspace};
use crate::ops;
use crate::sources::{RegistrySource, SourceConfigMap, CRATES_IO_REGISTRY};
use crate::sources::{RegistrySource, SourceConfigMap, CRATES_IO_DOMAIN, CRATES_IO_REGISTRY};
use crate::util::config::{self, Config, SslVersionConfig, SslVersionConfigRange};
use crate::util::errors::CargoResult;
use crate::util::important_paths::find_root_manifest_for_wd;
Expand Down Expand Up @@ -730,15 +730,15 @@ pub fn registry_login(
"Login",
format!(
"token for `{}` saved",
reg.as_ref().map_or("crates.io", String::as_str)
reg.as_ref().map_or(CRATES_IO_DOMAIN, String::as_str)
),
)?;
Ok(())
}

pub fn registry_logout(config: &Config, reg: Option<String>) -> CargoResult<()> {
let (registry, reg_cfg, _) = registry(config, None, None, reg.clone(), false, false)?;
let reg_name = reg.as_deref().unwrap_or("crates.io");
let reg_name = reg.as_deref().unwrap_or(CRATES_IO_DOMAIN);
if reg_cfg.credential_process.is_none() && reg_cfg.token.is_none() {
config.shell().status(
"Logout",
Expand Down
3 changes: 2 additions & 1 deletion src/cargo/ops/vendor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use crate::core::shell::Verbosity;
use crate::core::{GitReference, Workspace};
use crate::ops;
use crate::sources::path::PathSource;
use crate::sources::CRATES_IO_REGISTRY;
use crate::util::{CargoResult, Config};
use anyhow::{bail, Context as _};
use cargo_util::{paths, Sha256};
Expand Down Expand Up @@ -248,7 +249,7 @@ fn sync(
// replace original sources with vendor
for source_id in sources {
let name = if source_id.is_default_registry() {
"crates-io".to_string()
CRATES_IO_REGISTRY.to_string()
} else {
source_id.url().to_string()
};
Expand Down
4 changes: 2 additions & 2 deletions src/cargo/sources/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ restore the source replacement configuration to continue the build
let mut srcs = Vec::new();
if let Some(registry) = def.registry {
let url = url(&registry, &format!("source.{}.registry", name))?;
srcs.push(SourceId::for_registry(&url)?);
srcs.push(SourceId::for_alt_registry(&url, &name)?);
}
if let Some(local_registry) = def.local_registry {
let path = local_registry.resolve_path(self.config);
Expand Down Expand Up @@ -247,7 +247,7 @@ restore the source replacement configuration to continue the build
check_not_set("tag", def.tag)?;
check_not_set("rev", def.rev)?;
}
if name == "crates-io" && srcs.is_empty() {
if name == CRATES_IO_REGISTRY && srcs.is_empty() {
srcs.push(SourceId::crates_io(self.config)?);
}

Expand Down
2 changes: 1 addition & 1 deletion src/cargo/sources/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ pub use self::config::SourceConfigMap;
pub use self::directory::DirectorySource;
pub use self::git::GitSource;
pub use self::path::PathSource;
pub use self::registry::{RegistrySource, CRATES_IO_INDEX, CRATES_IO_REGISTRY};
pub use self::registry::{RegistrySource, CRATES_IO_DOMAIN, CRATES_IO_INDEX, CRATES_IO_REGISTRY};
pub use self::replaced::ReplacedSource;

pub mod config;
Expand Down
1 change: 1 addition & 0 deletions src/cargo/sources/registry/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ use crate::util::{restricted_names, CargoResult, Config, Filesystem, OptVersionR
const PACKAGE_SOURCE_LOCK: &str = ".cargo-ok";
pub const CRATES_IO_INDEX: &str = "https://github.com/rust-lang/crates.io-index";
pub const CRATES_IO_REGISTRY: &str = "crates-io";
pub const CRATES_IO_DOMAIN: &str = "crates.io";
const CRATE_TEMPLATE: &str = "{crate}";
const VERSION_TEMPLATE: &str = "{version}";
const PREFIX_TEMPLATE: &str = "{prefix}";
Expand Down
Loading