User: Password:
|
|
Subscribe / Log in / New account

Creating a Proper Path for Clean-up Patches

Creating a Proper Path for Clean-up Patches

Posted May 28, 2008 20:09 UTC (Wed) by AnswerGuy (subscriber, #1256)
Parent article: Getting the right kind of contributions

It seems like the best approach would be to have a path (through kernel newbies and/or kernel
janitors and thence through a small group of volunteer liaisons to the main kernel list.

Then all the clean-up patches can be consolidated, reviewed tested and merged in a streamlined
manner.

Perhaps one technique could be for new (substantive) patches to go through LKML -- then to
testing -- then through the janitorial process (which adds the stylistic and cosmetic
clean-up) --- then back through testing (-mm or similar kernels) and finally back through LKML
on its way to main.

In that model the janitorial team keeps their clean-up patches in their own repository, in
holding, and merges the clean-ups with substantive patches as those arise.  Occassionally they
inject a separate patch to a messy but long  untouched area of the sources --- but it's all
coming from them and done in a way that doesn't burn out the LKML participants.  (So such
clean-up only injections can be done opportunistically when the list has been relatively quiet
for a few days).

Just a thought.

JimD


(Log in to post comments)

Creating a Proper Path for Clean-up Patches

Posted May 29, 2008 5:18 UTC (Thu) by cpeterso (guest, #305) [Link]

I think that is the best way to manage contributions (of all sizes). I think the Kernel
Janitors and the "Trivial Patch Monkey" maintain some patchsets, but perhaps there should be
more official git trees. For example:

linux-whitespace
linux-trivial
linux-warnings
linux-janitors

could be maintained separately and merged into linux-testing or -mm at convenient merge times.

Creating a Proper Path for Clean-up Patches

Posted May 29, 2008 6:01 UTC (Thu) by midg3t (guest, #30998) [Link]

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.

Creating a Proper Path for Clean-up Patches

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

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]

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.



Copyright © 2017, Eklektix, Inc.
Comments and public postings are copyrighted by their creators.
Linux is a registered trademark of Linus Torvalds