search

rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.64k stars 2.4k forks source link

Add `--allow-staged` to `cargo fix` #5737

Closed alexcrichton closed 6 years ago

alexcrichton commented 6 years ago

In addition to cargo fix --allow-dirty, a user should also be able to pass --allow-staged that lets them run cargo-fix even if there are no unstaged changes[^1].

The rationale is that content that has been git added isn't actually in danger of being permanently eaten by a poor suggestion application, because you can git checkout -- . to back out the changes.

[^1]: That's git terminology; it should behave similar similarly if Mercurial, Pijul, or Fossil have an analogue of Git's staging area—I'm not familiar

Originally reported as https://github.com/rust-lang-nursery/rustfix/issues/127

killercup commented 6 years ago

This should be a pretty isolated change in this function:

https://github.com/rust-lang/cargo/blob/6a7672ef5344c1bb570610f2574250fbee932355/src/cargo/ops/fix.rs#L79

It will require some, but not very intimate, knowledge of the supported version control systems. It does not require knowledge about cargo internals.

You should add a few tests similar to ones found here.

JoshBrudnak commented 6 years ago

@alexcrichton I would like to help out with this issue. :+1:

jljusten commented 6 years ago

@JoshBrudnak: Ah. I should have replied earlier. I worked on this a bit on Saturday, and was going to reply saying I would be interested in working on it. Any chance I could try looking at this, and you could take #5740? I see you mentioned you were interested in both.

JoshBrudnak commented 6 years ago

@jljusten I have already opened a pull request fixing this issue (#5910) but if you want to take #5740 that's fine with me. :100:

jljusten commented 6 years ago

@JoshBrudnak: Ah, ok.

FWIW, my changes are in this branch: https://github.com/jljusten/cargo/commits/fix-allow-staged

Maybe I'll re-work them as a follow-on to your change.

killercup commented 6 years ago

Hey, this is now assigned to me to remind me to check in with you how this is going :)

5910 looks pretty good so far, @JoshBrudnak, and @jljusten's https://github.com/jljusten/cargo/commit/2bbbca31265cc7fddee3ba2d4f14e66517a3aadc also contains a test -- would be great to combine the two! (For example: If you add each other to the forked repos as contributors, you can both push to each others PRs. Or you could cherry-pick the commits.)

Both approaches are specific to git as far as I can see. Did you check if the other VCSs have something similar? We don't need to add support for that at first but it'd be great to know what we might also want to do.

JoshBrudnak commented 6 years ago

I haven't checked other VCSs yet. @jljusten do you want me to cherry-pick jljusten@2bbbca3?

jljusten commented 6 years ago

@JoshBrudnak: If you cherry-pick just that patch, the test would fail. I changed the code to print out the list of staged files with (staged) after each staged filename. (I also added (dirty) for the non-staged files.) So, our implementations were a bit different. My thought was this would help the user to know if they needed --allow-dirty, --allow-staged, or both.