Giuseppe Bilotta <giuseppe.bilotta@xxxxxxxxx> writes: > I've done the refactoring in a separate patch, to have a better (IMO) > view of what I was doing. I can squash them together if needed. I personally think squashing won't add much value in this case, but I think swapping would. The recommended order is that refactor and clean-up come first and then a new feature is built on the base that is made more solid, thanks to the refactoring. That allows the reviewers to see and judge the extent of damage to the codebase caused by the new feature more fairly. For example, adding a new feature to an unclean codebase may have to touch ten different places to add the same code, which would make the feature look expensive and intrusive. But if you first refactor the codebase cleanly, these ten places will be calling a single helper function, and you may be able to add the new feature by touching only one place, the helper function. It would be clear that the change is not intrusive if presented in such an order. This is a tangent, but the key to a better design is to try to resist the templation to think that yours will be the last cool feature. If you think "with addition of gravatar, the authorship display is complete", then there is no reason to name the subroutine that is called by "git_print_authorship" anything but "get_gravatar". If you resist the temptation, however, you would realize that other people may want to add support for different sources of person-label sources, and try to come up with a more generic name to call it [*1*]. I would not change the name of variable $git_gravatar_enabled, though. That way, people who would want other kinds of personal-icons can add an else-if to the sub, without changing other things. [Footnote] *1* To put it differently, you can tell where one's imagination ends by looking at the way that person refactors a piece of code and gives names. A concrete name (e.g. "gravatar") shows where the boundary of abstraction lies in that person's imagination. The patch shows that it was done by somebody who never thought of the possibility that git_print_authorship might be able to use service other than gravatar to show small icons associated with an email if other people build on top of his work. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html