Display registry name instead of registry URL when possible #9632

weihanglo · 2021-06-27T21:04:33Z

Fixes #6691

This PR can be divided into several parts:

c5be3de: Try to display registry names instead of URLs for impl Dipslay for SourceId. This benefits almost all kinds of messages usingSourceId directly or indrectly.
9394d48: This fixes Updating <name> index part of [source] replacement, which previously didn't preserve the registry name information.
4c2f9b5: This makes its best effort to show registry names for deps from Cargo.lock. Since current lockfile format does not serialize any registry name. We here try the best effort to restore registry name from either [registries] table or [source] replacement table. This is done by manually implementing Hash and PartialEq for SourceIdInner, of which two traits previously are simply derived.
To make SourceIdInner generate the same hash no matter it contains name field or not, here we remove name field from hashing and only concern about kind, precise and canonical_url.

Feel free to ask me for adding more tests, though I am not sure what tests should be added 😅

rust-highfive · 2021-06-27T21:04:36Z

r? @Eh2406

(rust-highfive has picked a reviewer for you, use r? to override)

pickfire · 2021-06-28T00:56:24Z

src/cargo/core/package_id.rs

            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`",


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

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.

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.

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?

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.

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.

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?

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

bors · 2021-07-14T16:13:58Z

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

ehuss · 2021-07-21T15:47:36Z

Thanks! I'm always a bit nervous changing the behavior of SourceId since it is used in so many places with subtle behavior. However, I can't immediately think of any issues here. I guess there might be some edge cases where the URL will still be displayed, but I imagine they should be rare.

Overall I think the names look cleaner, so 👍 from me.

@bors r+

bors · 2021-07-21T15:47:37Z

📌 Commit b7135644b3a39db3477927e9cd4cb249c9f2e963 has been approved by ehuss

bors · 2021-07-21T15:47:41Z

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

git checkout issue-6691 (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
git push self issue-6691 --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

Auto-merging tests/testsuite/registry.rs
Auto-merging tests/testsuite/publish_lockfile.rs
Auto-merging tests/testsuite/patch.rs
Auto-merging tests/testsuite/offline.rs
Auto-merging tests/testsuite/install.rs
Auto-merging tests/testsuite/directory.rs
CONFLICT (content): Merge conflict in tests/testsuite/directory.rs
Auto-merging tests/testsuite/build.rs
Auto-merging tests/testsuite/bad_config.rs
Auto-merging src/cargo/ops/registry.rs
Automatic merge failed; fix conflicts and then commit the result.

ehuss · 2021-07-21T15:47:53Z

Oops, didn't see the conflicting file. Can you rebase?

@bors r-

This fixes most messages to display registry names instead of URLs.

Since current lockfile does not serialize any registry names. We here try best effort to restore registry name from either `[registries]` table or `[source]` replacement table. This is done by manually implementing `Hash` and `PartialEq` for `SourceIdInner`, of which two traits previously are simply `derive`d. To make `SourceIdInner` generate the same hash whether contains `name` field or not, here we remove `name` field from hashing and only concern about `kind`, `precise` and `canonical_url`.

weihanglo · 2021-07-21T17:12:26Z

Rebased. Thanks for your review @ehuss 😄

ehuss · 2021-07-21T17:14:15Z

@bors r+

bors · 2021-07-21T17:14:16Z

📌 Commit 8c75e2f has been approved by ehuss

bors · 2021-07-21T17:14:24Z

⌛ Testing commit 8c75e2f with merge 706c291...

bors · 2021-07-21T17:40:47Z

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 706c291 to master...

Update cargo 2 commits in 4e143fd131e0c16cefd008456e974236ca54e62e..cebef2951ee69617852844894164b54ed478a7da 2021-07-20 21:55:45 +0000 to 2021-07-22 13:01:52 +0000 - Changes rustc argument from `--force-warns` to `--force-warn` (rust-lang/cargo#9714) - Display registry name instead of registry URL when possible (rust-lang/cargo#9632)

rust-highfive assigned Eh2406 Jun 27, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 27, 2021

pickfire reviewed Jun 28, 2021

View reviewed changes

weihanglo force-pushed the issue-6691 branch from 53eff61 to b713564 Compare June 28, 2021 16:47

weihanglo mentioned this pull request Jul 2, 2021

Using alternative registries names in text output #6691

Closed

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 21, 2021

bors added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 21, 2021

weihanglo added 6 commits July 22, 2021 00:18

Reuse crate-io constant

9244a42

Show registry name in messages if possible

71f5e92

This fixes most messages to display registry names instead of URLs.

Show registry name when using source replacment

b8ddbd2

Use crates-io instead of crates.io as registry name

4efaa4e

Update tests to use registry names

8c75e2f

weihanglo force-pushed the issue-6691 branch from b713564 to 8c75e2f Compare July 21, 2021 16:51

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Jul 21, 2021

bors merged commit 706c291 into rust-lang:master Jul 21, 2021

bors mentioned this pull request Jul 21, 2021

Packages can inherit fields from their root workspace #9684

Closed

weihanglo deleted the issue-6691 branch July 22, 2021 01:24

ehuss mentioned this pull request Jul 22, 2021

Update cargo rust-lang/rust#87371

Merged

ehuss added this to the 1.55.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display registry name instead of registry URL when possible #9632

Display registry name instead of registry URL when possible #9632

weihanglo commented Jun 27, 2021

rust-highfive commented Jun 27, 2021

pickfire Jun 28, 2021

weihanglo Jun 28, 2021

pickfire Jun 29, 2021

weihanglo Jul 3, 2021

pickfire Jul 3, 2021

weihanglo Jul 5, 2021

ehuss Jul 6, 2021

pickfire Jul 6, 2021

bors commented Jul 14, 2021

ehuss commented Jul 21, 2021

bors commented Jul 21, 2021

bors commented Jul 21, 2021

ehuss commented Jul 21, 2021

weihanglo commented Jul 21, 2021

ehuss commented Jul 21, 2021

bors commented Jul 21, 2021

bors commented Jul 21, 2021

bors commented Jul 21, 2021

Display registry name instead of registry URL when possible #9632

Display registry name instead of registry URL when possible #9632

Conversation

weihanglo commented Jun 27, 2021

rust-highfive commented Jun 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jul 14, 2021

ehuss commented Jul 21, 2021

bors commented Jul 21, 2021

bors commented Jul 21, 2021

ehuss commented Jul 21, 2021

weihanglo commented Jul 21, 2021

ehuss commented Jul 21, 2021

bors commented Jul 21, 2021

bors commented Jul 21, 2021

bors commented Jul 21, 2021