[webkit-dev] Reformatting-only patches being applied to trunk

[Maciej Stachowiak]

On Jul 25, 2009, at 2:08 AM, Oliver Hunt wrote:

> I've just noticed that there have been a few purely style related  
> patches being landed in the tree recently, I don't believe these are  
> a good idea and that any further reformatting only patches be  
> rejected.
>
> Historically we have avoided purely style related changes as they  
> impact our ability to track code changes efficiently and make patch  
> merging more complex. The general approach to code cleanup is to  
> clean up regions of code as we work in them -- this means that more- 
> or-less the only code effected by reformatting is code that was  
> being modified anyway.
>
> Does anyone disagree with this approach?

My opinion:

I think pure style/formatting changes should be separated from  
substantive behavior changes. I like it when style changes come as  
separate patches but in coordination with other patches that make  
substantive changes to the area. It's true that we've never made a  
comprehensive effort to reformat all the code to match style  
guidelines. But we have had some pure style cleanup changes to  
sections of the code, and have made some big changes like the one- 
class-per-file split long ago, as well as renaming our top-level  
namespaces.

I would say for files that are already close to correct style, minor  
changes to fix the last few issues are likely a good idea. The cost is  
low and the benefit of making our code more consistent is worthwhile.

We prefer to fix style gradually, but doing some systematically is ok.  
If some of these changes end up being big, it may be worth broader  
discussion. Large-scale reformatting is something that has project- 
wide impact.

More generally though, I would say now is a good time to do code  
cleanup and refactoring. It seems like a good time for some  
consolidation and entropy removal, after a lengthy period of  
aggressive feature development.

Regards,
Maciej