Don't do it then

Don't do it then

Posted May 29, 2008 9:24 UTC (Thu) by khim (subscriber, #9252)
In reply to: Creating a Proper Path for Clean-up Patches by ekj
Parent article: Getting the right kind of contributions

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!

---

(Log in to post comments)

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.