Cargo fmt should not run on a dirty repository by default

[dhardy]

Moved from rust-lang/cargo#7509

---

Problem

1. Hack on code, don't create a commit yet
2. Run cargo fmt; find formatting changes to old code + new code all mixed up

(Yes, I am fully aware that some people frequently do this on purpose. More than once I have received PRs with commits mixing up significant amounts of reformatting and new code.)

Possible Solution(s)
cargo fix, cargo publish etc. refuse to run on a dirty repository by default (but allow override with --allow-dirty). Please do the same for cargo fmt.

The above is only really a problem when significant amounts of old code were not previously formatted, so possibly Cargo+rustfmt could try to detect this first.

An alternative solution might be to make it possible to undo cargo fmt, but I don't see a clean way to do this.

[topecongiro]

Disallowing cargo fmt in a dirty workspace is a significant change to the default behavior of cargo-fmt, which I would prefer not to introduce without clear winning. We could add a command-line option, something like --no-dirty, which provides the required semantic. Alternatively, we could add a configuration option, something like allow_dirty, which is set to true by default. That way, you don't need to pass an extra command line option to cargo fmt, and enforce the rule to other contributors.

[dhardy]

I realise finding a solution which makes everyone happy here is tough! Possibly though there are a couple of options:

1. Create a git snapshot (i.e. a branch but known only by its commit hash) before formatting, and use this to enable revocation
2. Try to detect whether this project is usually formatted by rustfmt — if it is, there should be little/no reformatting needed to files which haven't been touched since the last git commit. Or, simpler, detect whether there is a rustfmt.toml file.

... but I'm also a realist, and recognise that maintaining the status quo may be the best feasible solution.

[calebcartwright]

Just wanted to add my two cents here, fully aware that this seems like a topic where reasonable folks will disagree.

IMHO formatting-related problems like the example tend to surface in certain contexts with certain user behaviors, and I think the most feasible solution is to address those behaviors/contexts.

For example:

• users could run cargo fmt -- --check instead of cargo fmt
• mechanisms could be enacted (CI checks, git hooks, etc.) to prevent misformatted code in the repo in the first place
• if parts of the project are known to be misformatted and should be left as-is (for example til the team is ready to reformat those), then a rustfmt.toml file could be included with the ignore setting to prevent any contributors from accidentally formatting those portions
• projects that don't want to have rustfmt-ing of the code could include a rustfmt.toml file that prevents rustfmt from running (with the disable_all_formatting option and/or ignore = [ "src/" ]), etc.

That being said, as it relates to the proposed solutions in the OP:

An alternative solution might be to make it possible to undo cargo fmt, but I don't see a clean way to do this.

I don't think that'd be viable either, and I also think that the responsibility of undo/rollback of changes belongs to the VCS. Once finished, rustfmt doesn't have a before/after picture of the changes, and I personally wouldn't want rustfmt to monkey around with my git repo.

refuse to run on a dirty repository by default

I personally wouldn't like this, and agree with @topecongiro that it would be a big change. Refusing to run on a dirty repo by default or adding a --no-dirty/allow_dirty config option also raises other questions about behavior:

• Would that apply to all emitters? Would I still be able to do a cargo fmt -- --check or cargo fmt -- --emit json if the repo was dirty or would I have to include a flag/config option?
• What would the expectation be when running cargo fmt --all in a repo workspace that contains local/path based dependencies that are themselves git repos (which may or may not be dirty)

[dhardy]

It seems the previous discussion got missed, so I'll copy it here.

From @ehuss:

Would you mind moving this to https://github.com/rust-lang/rustfmt? That's where cargo fmt lives.

Also, I think rust-lang/rustfmt#1324 might be close to what you want. I don't think aborting on a dirty repo is the right behavior, because that's the scenario it runs most often during development (that I've seen).

Some things that can help mitigate the issue:

• Enforce rustfmt on CI. This will ensure everything stays formatted all the time. Changes due to rustfmt versions are rare.
• Run rustfmt from an IDE/editor, where you can rustfmt only a region or single file. This also has the benefit of being easily undoable.

From myself:

rust-lang/rustfmt#1324 would help. But your mitigations don't:

• Enforce rustfmt on CI: not everyone wants to do this
• Run rustfmt from an IDE/editor: if your editor does this, then great, my proposal won't inconvenience you. But it doesn't fix my use case at all (which was in fact not intending to format the project but mistakenly calling the wrong command).

---

In reply to @calebcartwright:

projects that don't want to have rustfmt-ing of the code could include a rustfmt.toml file that prevents rustfmt from running (with the disable_all_formatting option and/or ignore = [ "src/" ]), etc.

So enable-by-default with an opt-out? I generally prefer the other way around, but this is partially workable. Either way though, auto-detection would be much easier (saves project owners remembering to configure rustfmt correctly, and in general, extra configuration makes software less easy to use). Something like:

$ cargo fmt
Rustfmt: dirty index and counted more than 20 formatting changes.
It looks like `cargo fmt` has not been run on this crate for a while.
Commit your changes first or run again with --allow-dirty to reformat code.

[calebcartwright]

I'm going to close this because the current behavior is expected, and desirable from the rustfmt perspective.

If you want to prevent rustfmt from being executed against your project, then you can add a rustfmt config file to your project as discussed above. The version of rustfmt that'll ship with rust v1.58 will have stabilized options you can use to turn rustfmt off, and/or you can consider making the rustfmt config file invalid/unparseable if you want to account for contributors on earlier stable versions.

[dhardy]

If you want to prevent rustfmt from being executed against your project, then you can add a rustfmt config file to your project as discussed above.

I don't want to disable rustfmt. I want to use it, but only when there are no changes between the working directory and the "git index", so that and changes made by rustfmt can be checked.

[calebcartwright]

Fair enough, but that's still not something I want to bake into rustfmt directly. For the interim, I think that use case would be best handled by a higher level script/task runner that makes the conditional check for whether or not to invoke rustfmt.

If such a feature is sufficiently widely desired then perhaps we'd consider incorporating it into a new binary we ship with the existing set, or perhaps integrating it with the existing git-rustfmt in some manner (even though it's current goal/feature is practically the opposite). It's just that we've explicitly decoupled the core rustfmt binary from version control system driven inspection and that's a pattern I'd like to maintain.

[dhardy]

Good point. This ought to do it.