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

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

Open
kornelski opened this issue Jul 1, 2024 · 7 comments · May be fixed by #15013
Open

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

kornelski opened this issue Jul 1, 2024 · 7 comments · May be fixed by #15013
Labels
A-git Area: anything dealing with git C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-fix disposition-merge FCP with intent to merge proposed-final-comment-period An FCP proposal has started, but not yet signed off. S-triage Status: This issue is waiting on initial triage. T-cargo Team: Cargo

Comments

@kornelski
Copy link
Contributor

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

@kornelski kornelski added C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-triage Status: This issue is waiting on initial triage. labels Jul 1, 2024
@epage epage added Command-fix A-git Area: anything dealing with git labels Jul 2, 2024
@epage
Copy link
Contributor

epage commented Jul 2, 2024

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 kornelski linked a pull request Jan 4, 2025 that will close this issue
@kornelski
Copy link
Contributor Author

kornelski commented Jan 4, 2025

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

epage commented Jan 4, 2025

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

@weihanglo
Copy link
Member

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

This comment has been minimized.

@weihanglo weihanglo added the T-cargo Team: Cargo label Jan 4, 2025
@weihanglo
Copy link
Member

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

rfcbot commented Jan 4, 2025

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

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.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Jan 4, 2025
@weihanglo weihanglo changed the title --allow-dirty to imply --allow-staged? cargo-fix: --allow-dirty to imply --allow-staged? Jan 4, 2025
@ehuss ehuss moved this to FCP merge in Cargo status tracker Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-git Area: anything dealing with git C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-fix disposition-merge FCP with intent to merge proposed-final-comment-period An FCP proposal has started, but not yet signed off. S-triage Status: This issue is waiting on initial triage. T-cargo Team: Cargo
Projects
Status: FCP merge
Development

Successfully merging a pull request may close this issue.

4 participants