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.

Sign up for GitHub

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

Jump to bottom

fix(cli): Forward non-UTF8 arguments to external subcommands #11118

Merged
merged 1 commit into from
Sep 22, 2022
Merged

fix(cli): Forward non-UTF8 arguments to external subcommands #11118

merged 1 commit into from
Sep 22, 2022

Conversation

epage
Copy link
Contributor

@epage epage commented Sep 20, 2022

Whether we allow non-UTF-8 arguments or not, we shouldn't preclude external subcommands from deciding to do so.

I noticed this because clap v4 changed the default for external subcommands from String to OsString with the assumption that this would help people to "do the right thing" more often.

@rust-highfive
Copy link

r? @weihanglo

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 20, 2022
@bors
Copy link
Collaborator

bors commented Sep 21, 2022

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

weihanglo
weihanglo reviewed Sep 21, 2022
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Generally looks great!

Should we add a test to prevent any regression?

src/bin/cargo/commands/help.rs Outdated Show resolved Hide resolved
@epage
fix(cli): Forward non-UTF8 arguments to external subcommands
87fdf76 
I noticed this because clap v4 changed the default for external
subcommands from `String` to `OsString` with the assumption that this
would push people to "do the right thing" more often.
@epage
Copy link
Contributor Author

epage commented Sep 21, 2022

Should we add a test to prevent any regression?

I'm trying to weigh this out with updating the infrastructure so it can be tested.

As is, this is something the type system is verifying for us.

To test this, we need a platform-specific test that can generate a project capable of verifying the argument was passed through. The existing echo_subcommand would require platform-specific changes to allow it to sometimes not panic when echoing back out (windows could still panic) or a custom program that fails if expected values aren't received which requires updating the project builder to work with bytes instead of strings.

All of this can be done but weighing out, I'm unsure how worth it it is to do.

weihanglo
weihanglo approved these changes Sep 22, 2022
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Should we add a test to prevent any regression?

I'm trying to weigh this out with updating the infrastructure so it can be tested.

As is, this is something the type system is verifying for us.

Make sense. I'll merge it. Thank you for posting this!

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 22, 2022

📌 Commit 87fdf76 has been approved by weihanglo

It is now in the queue for this repository.

@bors 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 Sep 22, 2022
@bors
Copy link
Collaborator

bors commented Sep 22, 2022

⌛ Testing commit 87fdf76 with merge 7c8a5a6...

@bors
Copy link
Collaborator

bors commented Sep 22, 2022

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 7c8a5a6 to master...

@bors bors merged commit 7c8a5a6 into rust-lang:master Sep 22, 2022
@bors bors mentioned this pull request Sep 22, 2022
Merged
@epage epage deleted the external branch September 22, 2022 15:00
@weihanglo weihanglo mentioned this pull request Sep 27, 2022
Merged
bors pushed a commit to rust-lang-ci/rust that referenced this pull request Sep 27, 2022
@weihanglo
Update cargo
5095f54 
22 commits in 73ba3f35e0205844418260722c11602113179c4a..f5fed93ba24607980647962c59863bbabb03ce14
2022-09-18 06:38:16 +0000 to 2022-09-27 12:03:57 +0000

- build-scripts.md: Use em dash consistently. (rust-lang/cargo#11150)
- Indicate how Cargo locates the manifest (rust-lang/cargo#10770)
- Reduce references to `[project]` within cargo (rust-lang/cargo#11135)
- Iteratively construct target cfg (rust-lang/cargo#11114)
- update comment about `CARGO_BIN_EXE_` (rust-lang/cargo#11146)
- Call out that not all config values can be set via env vars (rust-lang/cargo#11139)
- Bump to 0.67.0, update changelog (rust-lang/cargo#11137)
- ci: update toolchain for building api doc (rust-lang/cargo#11134)
- Http publish not noop (rust-lang/cargo#11111)
- Improve errors for TOML fields that support workspace inheritance (rust-lang/cargo#11113)
- switch to `std::task::ready!()` where possible (rust-lang/cargo#11130)
- Report cmd aliasing failure with more contexts (rust-lang/cargo#11087)
- minor: remove unused mut (rust-lang/cargo#11127)
- fix(cli): Forward non-UTF8 arguments to external subcommands (rust-lang/cargo#11118)
- This change adds an example to the authors attribute in the manifest. (rust-lang/cargo#10938)
- Add support for relative git submodule paths (rust-lang/cargo#11106)
- make unknown features on `cargo add` more discoverable (rust-lang/cargo#11098)
- Unlink old final artifacts before compilation (rust-lang/cargo#11122)
- refactor(cli): Prepare for clap v4 (rust-lang/cargo#11116)
- fix(cli): Error trailing args rather than ignore (rust-lang/cargo#11119)
- Add a minor clarification (rust-lang/cargo#11093)
- doc(changelog): mention CVE fixes (rust-lang/cargo#11104)
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 27, 2022
@bors
Auto merge of rust-lang#102365 - weihanglo:update-cargo, r=ehuss
470e518 
Update cargo

22 commits in 73ba3f35e0205844418260722c11602113179c4a..f5fed93ba24607980647962c59863bbabb03ce14 2022-09-18 06:38:16 +0000 to 2022-09-27 12:03:57 +0000

- build-scripts.md: Use em dash consistently. (rust-lang/cargo#11150)
- Indicate how Cargo locates the manifest (rust-lang/cargo#10770)
- Reduce references to `[project]` within cargo (rust-lang/cargo#11135)
- Iteratively construct target cfg (rust-lang/cargo#11114)
- update comment about `CARGO_BIN_EXE_` (rust-lang/cargo#11146)
- Call out that not all config values can be set via env vars (rust-lang/cargo#11139)
- Bump to 0.67.0, update changelog (rust-lang/cargo#11137)
- ci: update toolchain for building api doc (rust-lang/cargo#11134)
- Http publish not noop (rust-lang/cargo#11111)
- Improve errors for TOML fields that support workspace inheritance (rust-lang/cargo#11113)
- switch to `std::task::ready!()` where possible (rust-lang/cargo#11130)
- Report cmd aliasing failure with more contexts (rust-lang/cargo#11087)
- minor: remove unused mut (rust-lang/cargo#11127)
- fix(cli): Forward non-UTF8 arguments to external subcommands (rust-lang/cargo#11118)
- This change adds an example to the authors attribute in the manifest. (rust-lang/cargo#10938)
- Add support for relative git submodule paths (rust-lang/cargo#11106)
- make unknown features on `cargo add` more discoverable (rust-lang/cargo#11098)
- Unlink old final artifacts before compilation (rust-lang/cargo#11122)
- refactor(cli): Prepare for clap v4 (rust-lang/cargo#11116)
- fix(cli): Error trailing args rather than ignore (rust-lang/cargo#11119)
- Add a minor clarification (rust-lang/cargo#11093)
- doc(changelog): mention CVE fixes (rust-lang/cargo#11104)
@ehuss ehuss added this to the 1.66.0 milestone Oct 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weihanglo weihanglo weihanglo approved these changes

Assignees

@weihanglo weihanglo

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.66.0
Development

Successfully merging this pull request may close these issues.
5 participants
@epage @rust-highfive @bors @weihanglo @ehuss