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

[Luben Tuikov <ltuikov@yahoo.com>]

--- Jakub Narebski <jnareb@gmail.com> wrote:
> Luben Tuikov wrote:
> 
> > gitweb has two bugs in git_patchset_body (now you know who
> > you are without data-mining with git-blame).
> > 
> > The first bug is a missing "</div>" on commitdiff.
> > Saw this one a couple of days ago, but was swamped
> > and had no time to post.
> 
> Could you please provide either minimal example from
> scratch, or parameters for git.git repository view, or
> an URL? I could not reproduce this...

I cannot post a URL to the repo for many reasons.
I don't have time to allocate to trying to define a test
case from scratch, nor am I certain that I can attain that,
since that would be a trivial bug to fix and am sure that
it would've been caught.  But as a heavy git user in a complex
(git) environment, the limits and corner cases are sometimes hit.

I cannot suggest anything other than using git professionally
in a complex environment, as opposed to being a pastime.

I also understand that it is a fine line of separating the two,
and it would appear that the former group not always has the time
and the latter not always has the complex environment to hit
the corner cases.

> > The second bug is displaying a renamed file with the
> > same name as the one of the preceding "patch". Saw
> > this one today when I pulled.
> > 
> > To exhibit these, you need to "commitdiff" a commit
> > which has at least one rename and at least one regular
> > diff preceding the rename.
> 
> Haven't found this one, but I hope one of those patches
> would fix this.

I hope so too.

> > "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.

> > 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 I 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).

   Luben