Re: [PATCH 0/5] gitweb: git_patchset_body fixes

[Jakub Narebski <jnareb@gmail.com>]

Dnia poniedziałek 8. stycznia 2007 04:50, Luben Tuikov napisał:
> --- Jakub Narebski <jnareb@gmail.com> 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