Creating a Proper Path for Clean-up Patches [LWN.net]

Creating a Proper Path for Clean-up Patches

Posted May 29, 2008 6:01 UTC (Thu) by midg3t (guest, #30998)
In reply to: Creating a Proper Path for Clean-up Patches by AnswerGuy
Parent article: Getting the right kind of contributions

I think the point is that whitespace-only changes at any time are noise. Trying to understand annotated code (a la git blame) that is punctuated with whitespace-only changes is a waste of time. The preferable method is to fix the whitespace when you are making a code change on the same line (or block). That way every change has a purpose as a code change and doesn't get in the way when looking at the code history.

Keeping whitespace changes in a separate tree sounds OK on paper, but in practice it means someone has to spend all day resolving conflicts between code changes and whitespace changes. Not fun.

to post comments

Creating a Proper Path for Clean-up Patches

Posted May 29, 2008 8:38 UTC (Thu) by ekj (guest, #1524) [Link] (2 responses)

Perhaps.

But there's also the point that sometimes two patches:

1) This patch does NOTHING other than reformat function foo()
2) This patch adds a check for $ERROR_CONDITION to foo()

Is easier to read and understand the patches than it would've been if they where rolled into
one.

Don't do it then

Posted May 29, 2008 9:24 UTC (Thu) by khim (subscriber, #9252) [Link] (1 responses)

If reformatting of the patch is such a big part of it that it's hard to understand what the patch is actually doing - then drop the reformatting part. It's as simple as that. Formatting patches are bad. We once hired some guy and asked him to add small functionality to the program. It was supposed to be week or two of work. He took month and at the end we've got requested functionality and... patch which changes 90% of code to make it adhere to coding standards. It was the last month this guy worked for us: the work was redode by someone else.

The rule of thumb is simple: you must be ready to answer for all lines your patch touches. You are "owner" of said lines after patch is applied (most VCS will allow you to find such owners). And answer like "I've only moved this two whitespaces right" does not count: you should genuinely understand this code. If you can do it - feel free to reformat code. If you don't have interest or time - leave the code as is!

Don't do it then

Posted May 29, 2008 11:09 UTC (Thu) by ekj (guest, #1524) [Link]

It's not that it's hard. It's just that -any- noise subtracts from readability.

Which means a patch that -only- does the minimum required for adding the error-check will
always be more readable than one that does that, plus fix the formatting of the surrounding 5
lines.

So your advice adds up to, essentially: NEVER FIX STYLE-ISSUES.

Which is overdoing it. I agree that it's *often* a bad thing to do style-change. But there's
two important readability-issues here:

One is the readability of the patch. That is always better with zero style-changes.

The second is the readability of the code after the patch is applied. If that is sufficiently
improved by doing style-fixes, those may be worthwhile even though it hurts the patch.