4

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?

  • Usually one should ignore whitespace when comparing files for changes... Also some IDE reformat code when closing a } for example so I would not split a commit in such case. Otherwise, it is a good idea to use distinct commits for unrelated changes. – Phil1970 Jun 11 '17 at 4:07
  • 1
    Possible duplicate of Reformatting and version control – gnat Jun 11 '17 at 5:48
  • 1
    it is worth keeping in mind that even if you split commits git gives you an option to squash them – gnat Jun 11 '17 at 6:06
  • 1
    Note that you can eliminate the "whitespace" change caused by the indentation using the -w switch. Whenever you see a huge block of deletions followed by a block of additions you may quickly check with that switch to see the changes with whitespace (indentation) changes removed. With the knowledge about this Git feature it's no longer necessary to commit awkward histories for better readability in the future. – try-catch-finally Jul 26 '17 at 5:13
8

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).

4

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.

  • 1
    Sorry for the confusion, I does not mean tabs or whitespaces or that. I mean that when I wrap a section of html inside a <div> , everything inside gets 1 more indent. – golopot Jun 11 '17 at 3:05
  • @q4w56 -- In that case, that's bordering on a change in semantics. It still might be a good idea to break your commit into two parts, one where you wrap the section inside a <div> (but now improperly indented), and the next where you change 100 lines of code to make the indentation be correct. – David Hammen Jun 11 '17 at 3:23
0

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.