Avoid adding “drive-by formatting changes” to commits

Recently, when reviewing code, I saw a commit resembling the following:

- if (x) foo();
- if (y) bar();
+ if (y) {
+    bar();
+ }

It's not easy to see that of the two "if" statements, only one was actually deleted. The other had its formatting changed, but was otherwise not altered.

To prevent this, go through the "diff" before doing the commit. Revert any changes which have happened which haven't changed the functionality of the code.

Committing not only the things you meant to change, but a bunch of other changes that don't change the code's functionality, has the following negative consequences:

• The "blame" commands of the VCS will now show this new commit as the "last changer" of the "if (y)" line above. If you're trying to find out why that code was written, this formatting change isn't the commit you're interested in. (And the commit message "removed X" won't help you understand why the "if (y)" line is there.)
• Conflicts. We love feature branches. If two people do such a drive-by change, in inconsistent ways, in different feature branches, when it comes to merge, that will be a conflict, that some person is going to have to spend valuable time and energy fixing.
• The diff is more difficult to read than it needs to be. In the above commit, it takes longer to see that only the first line has been removed. Code reviews are important.

I think there are the following reasons why such changes might get introduced in the first place:

• The developer felt like they wanted to make the code "nicer". It's a good intention, but leave that for special "code cleanup" commits. Each commit should have one purpose.
• IDEs often auto-format code. In that case, the developer might not even realize they were making the formatting change? (But a "diff" before commit will show it to you and allow you to revert it.)
• Java IDEs have "organize imports automatically" differently. IntelliJ lays out the "import" statements differently to Eclipse. So, a trivial change to a piece of code, and "organize imports" will completely change the import section, for no functional difference. If you see such needless import changes in the diff before commit, revert them.
• Some text editors chop off trailing spaces in a line. This is a noble cause, but, again, it makes it difficult to see what the programmer has actually changed in the commit. In the most pathological case I've seen a commit with hundreds of files altered, for only a single line of actual change. This is going to be a nightmare to review and a nightmare to merge. Turn this feature of your editor off.
• Tabs vs Spaces inconsistency. IDEs often change the file to their preferred settings. You can't even see these changes! Again the "diff" tool before commit will make you aware of them.