Is it a good practice to split one commit into two, where one is content change, the other is style(indent) change?

Question

Suppose I am to make a commit that wrapped a section of code inside another thing, so the section came to have 1 more level of indent. In diff it will show delete 100 lines, add 100 lines though all it changed is indent level, and this is very noisy noise.

If I split one commit into two, the first one is the real code change(deliberately not add indent), the second one consists of only indent changes. This scheme can greatly separate the signal and noise (content change and style change), the diff will be much easier to read. Is it a good practice?

Answer 1

I think that's taking it too far. Putting functional changes and stylistic changes in different commits is good practice as it keeps the commits focused and comprehensible. However, indentation is a quasi-functional aspect of code -- it signals what level of nesting we are at in any block, which says a lot about how the code is going to be run. So I would disagree that we have two changes "nest this block" and "indent this block", rather you have one change "nest this block" that by the conventions of code formatting requires "indent this block".

The block being part of the change is in my mind a good thing as it makes it clearer what changed. If I had two separate commits as you propose, one could easily look at the first commit and say "okay, they added an if statement, but what the heck is the body of that statement?" and then "okay, they indented this block of code, why on earth did they do that?" Whereas with one commit there's no confusion.

I think this is also a case where you might need to use your tools better. Where I work, our cr review software makes clear when part of a diff is caused by changes in whitespace, and one can use git to also make this distinction (e.g. with git diff -w).

Answer 2

Of course it's a good idea. The commit for a change in semantics is quite distinct from the commit for a change in indentation.

However, why are you changing the indentation? The general old school rule was that the maintainer of a body of code should live with the indentation scheme of the original developer. There are exceptions upon exceptions to this rule, but the intent of the rule is to keep (1) avoid edit wars, and (2) avoid commits that have zero semantic meaning.

Suppose you do find one file in the configuration managed code base that blatantly violates your organization's coding standards. It's most likely that that is not the only file. You should create a new change request (or whatever they are called in your organization) to fix all of those problems. That one commit might hit anything from one file to thousands of them, but if the reformatting is done by a well-tested tool, it will have zero semantic impact.

The new school rule is that commits / pushes will either (a) be automatically reformatted to an agreed-upon common style upon commit /push, or (b) rejected should the code in question fail to conform with that style. Either way, you will not have a problem with code that deviates from the style with which everyone has agreed to abide.

Answer 3

I think it is better to split the commits to two. It keeps the intent of the commits clear.

I try to do that but I also know that I don't stick to it religiously. If I notice the formatting issues before I start making content changes I try to fix the formatting issues first and check the changes in before I work on the content changes. If I notice the formatting issues after I make the content changes, I either ignore the formatting issues or submit both changes in one one commit.