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

Comments

Projects
None yet
4 participants
@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 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 rust-lang-nursery/rustfix#127

@killercup

This comment has been minimized.

Copy link
Member

killercup commented Aug 14, 2018

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

This comment has been minimized.

Copy link

JoshBrudnak commented Aug 18, 2018

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

@jljusten

This comment has been minimized.

Copy link
Contributor

jljusten commented Aug 21, 2018

@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

This comment has been minimized.

Copy link

JoshBrudnak commented Aug 21, 2018

@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

This comment has been minimized.

Copy link
Contributor

jljusten commented Aug 21, 2018

@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

This comment has been minimized.

Copy link
Member

killercup commented Aug 21, 2018

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

This comment has been minimized.

Copy link

JoshBrudnak commented Aug 21, 2018

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

@jljusten

This comment has been minimized.

Copy link
Contributor

jljusten commented Aug 22, 2018

@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

Auto merge of #5943 - dwijnand:fix-allow-staged, r=alexcrichton
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.

@bors bors closed this in #5943 Aug 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.