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

Add note to process::arg[s] that args shouldn't be escaped or quoted #78599

Merged
merged 2 commits into from
Nov 1, 2020

Conversation

panstromek
Copy link
Contributor

@panstromek panstromek commented Oct 31, 2020

This came out of discussion on forum, where I recently asked a question and it turned out that the problem was redundant quotation:

 Command::new("rg")
        .arg("\"pattern\"") // this will look for "pattern" with quotes included

This is something that has bitten me few times already (in multiple languages actually), so It'd be grateful to have it in the docs, even though it's not sctrictly Rust specific problem. Other users also agreed.

This can be really annoying to debug, because in many cases (inluding mine), quotes can be legal part of the argument, so the command doesn't fail, it just behaves unexpectedly. Not everybody (including me) knows that quotes around arguments are part of the shell and not part of the called program. Coincidentally, somoene had the same problem yesterday on reddit.

I am not a native speaker, so I welcome any corrections or better formulation, I don't expect this to be merged as is. I was also reminded that this is platform/shell specific behaviour, but I didn't find a good way to formulate that briefly, any ideas welcome.

It's also my first PR here, so I am not sure I did everything correctly, I did this just from Github UI.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @m-ou-se (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 31, 2020
Copy link
Member

@m-ou-se m-ou-se left a comment

Choose a reason for hiding this comment

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

Thanks! I added a suggestion that mentions that it's specifically shell syntax that has no effect.

library/std/src/process.rs Outdated Show resolved Hide resolved
library/std/src/process.rs Outdated Show resolved Hide resolved
@panstromek
Copy link
Contributor Author

Thanks. I applied the suggestions, it sounds better to me.

It's in separate commit, though, Do I need to squash it, is it squashed during merge?

@m-ou-se
Copy link
Member

m-ou-se commented Oct 31, 2020

I think it's fine to keep the git history as it is. Squashing might be nice if there's a large amount of commits making small (or opposite) changes to the same code.

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 31, 2020

📌 Commit db416b2 has been approved by m-ou-se

@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 Oct 31, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 1, 2020
Rollup of 6 pull requests

Successful merges:

 - rust-lang#78073 (Add #[inline] to some functions in core::str.)
 - rust-lang#78596 (Fix doc links to std::fmt)
 - rust-lang#78599 (Add note to process::arg[s] that args shouldn't be escaped or quoted)
 - rust-lang#78602 (fix various aliasing issues in the standard library)
 - rust-lang#78603 (expand: Tweak a comment in implementation of `macro_rules`)
 - rust-lang#78621 (Inline Default::default() for atomics)

Failed merges:

r? `@ghost`
@bors bors merged commit f281a76 into rust-lang:master Nov 1, 2020
@rustbot rustbot added this to the 1.49.0 milestone Nov 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants