Ensure package IDs are unique when cleaning. #12708
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
Command-clean
S-waiting-on-review
src/cargo/ops/cargo_clean.rs
Outdated
| @@ -104,7 +105,7 @@ pub fn clean(ws: &Workspace<'_>, opts: &CleanOptions<'_>) -> CargoResult<()> { | |||
| // Doc tests produce no output. | |||
|
|
|||
| // Get Packages for the specified specs. | |||
| let mut pkg_ids = Vec::new(); | |||
| let mut pkg_ids = BTreeSet::new(); | |||
There was a problem hiding this comment.
Can you add a test for this? Ideally the first commit would have the test with a #[should_panic] and then the second commit would include this with the test updated. Using these two commits will make it easier to ensure the test is testing the right thing and to show to reviewers what the intended behavior change is (especially since this is a PR without an issue)
There was a problem hiding this comment.
I took a quick look at this, and adding a test seems fairly complicated. There are not any tests in cargo_clean.rs, and most of it consists of a monolithic clean() function, in which this code change is buried. I would need to construct an appropriate Workspace object, which is fairly complicated.
It looks like there are some cursory integration tests, but adding something would require figuring out exactly how to trigger a duplicate ID, which might be tricky.
Overall, it doesn't look at all straightforward to test. :( I will keep poking at it, though, and see if I can figure anything out.
There was a problem hiding this comment.
Look in tests/testsuite/clean.rs for tests.
|
☔ The latest upstream changes (presumably #12638) made this pull request unmergeable. Please resolve the merge conflicts. |
get_many will panic if there are duplicate IDs.
|
After pulling in #12638 I am no longer getting a panic, so I am going to abandon this. |
get_many will panic if there are duplicate IDs.