Dnia poniedziałek 8. stycznia 2007 04:50, Luben Tuikov napisał: > --- Jakub Narebski <jnareb@xxxxxxxxx> wrote: >> Luben Tuikov wrote: >>> "And while at it" can you please actually *do* "refactor" >>> git_patchset_body *into smaller functions each one doing >>> a single particular task*. >>> >>> It is sad to see git_patchset_body in such despicable state >>> all the while seeing words like "refactor" in the commit logs >>> of that function. >> >> (Perhaps I overuse word "refactor"). > > Perhaps. Or maybe it is used incorrectly. That is also possible. That said git_patchset_body was rewritten from AWK-like event-driven single loop, with lots of state variables, to few loops following the structure of patchset, and of git diff. >>> git_patchset_body is grossly overloaded for what it is >>> supposed to do to, and being one single huge blob, it is >>> hard to maintain. >> >> I'm not sure if splitting git_patchset_body into smaller functions >> would be worth doing, as 1) such functions would be used only >> by git_patchset_body, 2) quite a bit of info has to be passed. > > When things get too complex, they should be done out of > principle, not out of "how I feel about it", since principles > can be proved and what they define can be easily tracked. > > Yes, it is worth doing. It doesn't matter that those smaller > functions would be used only by git_patchset_body, when > (by principle) the workings, logic and justification would > be clearly exposed by those smaller units of function. Thus > it would be possible to prove that the function is correct > or not, and bugs would be able to be isolated easily. > > Smaller units of function should do one thing and do it well. > If you cannot isolate them "because quite a bit of info has > to be passed" then the logic of git_patchset_body is faulty, > by _definition_, and needs to be scrapped and re-engineered > (from scratch). On first glance it would be easy to separate (refactor) individual patch parsing and output into separate subroutine; perhaps also separate parsing and output of diff header. The problems with this approach are two: empty patches (e.g. pure rename patch) and split patches (e.g. file to symlink patch). And we are dealing with two sources of information: "raw" difftree part, and the patch(set) part. So it is not that easy. But I can certainly try... -- Jakub Narebski Poland - 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