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 --allow-staged to cargo fix #5737

Closed
alexcrichton opened this issue Jul 17, 2018 · 8 comments · Fixed by #5943
Closed

Add --allow-staged to cargo fix #5737

alexcrichton opened this issue Jul 17, 2018 · 8 comments · Fixed by #5943
Assignees
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-fix
Milestone

Comments

@alexcrichton
Copy link
Member

alexcrichton commented Jul 17, 2018

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 changes1.

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.


Originally reported as rust-lang/rustfix#127

Footnotes

  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

@alexcrichton alexcrichton added the C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` label Jul 17, 2018
@killercup killercup added this to the Edition RC 1 milestone Aug 14, 2018
@killercup
Copy link
Member

This should be a pretty isolated change in this function:

fn check_version_control(opts: &FixOptions) -> CargoResult<()> {

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
Copy link

@alexcrichton I would like to help out with this issue. 👍

@jljusten
Copy link
Contributor

@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
Copy link

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

@jljusten
Copy link
Contributor

@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
Copy link
Member

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 jljusten@2bbbca3 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
Copy link

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

@jljusten
Copy link
Contributor

@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.

bors added a commit that referenced this issue Aug 29, 2018
Add `--allow-staged` to `cargo fix`

Fixes #5737

This is @jljusten's branch, adapted to the testsuite changes in master.
Submitted as an alternative to #5910 to expedite #5737 resolution in time for Edition RC 1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-fix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants