cargo-fix: --allow-dirty to imply --allow-staged?

[kornelski]

Problem

cargo fix/clippy --fix don't modify git's staging area, so the requirement to specify --allow-staged if there are any staged files is very cautious. Technically the staged files are not stored directly in the working directory, and can't be overwritten by the fix commands.

OTOH "dirty" state is much more volatile, and potentially has file changes in the working directory that haven't been saved anywhere, and which could be irreversibly lost when files are modified. This makes --allow-dirty a much stronger option that implies the local state is unimportant and can be lost.

This makes use of --allow-dirty without --allow-staged a weird edge case. It says the local changes are unimportant and can be overwritten even if they're unrecoverable, except being extra cautious about existence of local changes that are safe, and won't be overwritten. It's like having a --force-overwrite flag, but requiring --allow-not-forced-non-overwrite flag too.

Proposed Solution

Having to add both --allow-dirty --allow-staged flags to the fix commands is verbose and a bit tedious to type. The extra --allow-staged flag could be avoided if the more destructive --allow-dirty also implied the lesser --allow-staged. Alternatively, maybe there could be a single option like --force/-f that implies both.

Notes

No response

[epage]

I can't speak too much to this as personally I'm not thrilled with the --allow-<something> workflow for the "safe" fixes (which is all we do atm) and so I can't fully speak towards its intent.

[kornelski]

I still think it's worthwhile to make this simple change, and I don't have anything to add to the arguments for it.

I think it's now up to you to drive the acceptance process forward. If you don't want to accept this change, then please close the issue instead of leaving it hanging indefinitely.

[epage]

Keep in mind that we have over 500 issues that need triage and over 100 asking for team input.

[weihanglo]

To myself I understand and agree that --allow-dirty should be treated like "allow whatever VCS reports as dirty". cargo package --allow-dirty does like that. And cargo fix --allow-dirty before --allow-staged added did that as well.

If I were there in the discussion, I would suggest people to create a temporary commit and run cargo fix. Generally checking in code is cheap and safer than working in staging area. The addition of --allow-staged was also technically a breaking change at that time for workflow that only passed --allow-dirty. I didn't see many discussions about the flag itself and alternatives in #5737 though.

One concern is that the proposal is, again, a breaking change for a pretty odd use case: Only allowing dirty unstaged changes but not staged. I cannot tell who ever wants that though.

[weihanglo]

To @kornelski: I am still trying to find a time reading your mtime hack thoroughly again. I personally think that is more urgent and useful than this one. Yes we overlooked this and lots of others. Sorry about that. I don't mine if you give me a ping here or on Zulip or wherever. I'll try my best prioritizing if reasonable. Can't speak for others.

Self-hidden because off-topic

[weihanglo]

Keep in mind that we have over 500 issues that need triage and over 100 asking for team input.

While we do have problems with the huge backlog, I think whoever comes and contributes reasonably should get some rewards. To me, it is a way to prevent the project from drowning in issue backlog and still get helps from old and new friends.

I may be wrong but this proposal seems small enough, so I'll give it a nudge and start an async FCP. No pressure though.

---

@rfcbot fcp merge

This proposes cargo fix --allow-dirty implies cargo fix --allow-staged.

Arguments:

• The current behavior is that you need to pass both flags if you have changes in both staging area and working directory in a Git repo. Inconvenient.
• Before Add --allow-staged to cargo fix #5943, --allow-dirty in cargo fix implies --allow-staged.
• In cargo package, --allow-dirty implies all changes including in staging area.
• --allow-dirty without --allow-staged is an odd usecase. See the issue description.

Counterarguments:

• It may break somebody's workflow that specifically wants to fix changes in working directory only (the --allow-dirty without --allow-staged use case).
  • cargo fix, especially with --allow-* flags, as a destructive command, is generally used in interactive sessions. It should be extremely rare to break somebody's CI with this change.

[rfcbot]

Team member @weihanglo has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Eh2406
• @Muscraft
• @Rustin170506
• @arlosi
• @ehuss
• @epage
• @joshtriplett
• @weihanglo

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.