Re: [PATCH 00/11 GSoC] gitweb: Split gitweb into modules

From: Jakub Narebski <jnareb@gmail.com>
Date: Jun 23, 2010, 07:59 UTC
Message-ID: <201006230959.28098.jnareb@gmail.com>
In-Reply-To: <AANLkTimIDet95GgvjyiQQPHSU2FSkJcC7WKv7fILiz-y@mail.gmail.com>

On Tue, 22 June 2010, Pavan Kumar Sunkara wrote:

Show 13 quoted lines

> On Wed, Jun 23, 2010 at 1:59 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Tue, 22 June 2010, Pavan Kumar Sunkara wrote:
>>>
>>> Yes, due to either unment dependency or circular dependency problem.
>>> But no need to worry as they are very small.
>>
>> Could you _*LIST*_ those subroutines that you feel should belong in
>> Gitweb::Config but could not be put in it, and write _why_ they could not
>> be put (on what subroutines / variables do they depend that it makes
>> impossible to move them)?
> 
> feature_* subs. They can't be put in it because they use
> git_get_project_config from Gitweb::RepoConfig module

O.K., but that information should IMHO be in the commit message. But shouldn't feature_* subs (except perhaps utility feature_bool etc.) be in Gitweb::RepoConfig rather than Gitweb::Config anyway?

[...]

Show 6 quoted lines

>> Perhaps it would be better then to move _all_ validate_* subroutines to
>> separate Gitweb::Request::Validate module... unless they are used by some
>> subroutine from Gitweb::Request.
> 
> or even better if we leave them untouched for now. (in gitweb.perl script)
> and move them later along with the evaluate_validate_params function

That's good enough for me.

Show 28 quoted lines

>>>>> 6. gitweb: Create Gitweb::Escape module
>>>>>
>>>>> Create a Gitweb::Escape module in 'gitweb/lib/Gitweb/Escape.pm'
>>>>> to store all the quoting/unquoting and escaping subroutines
>>>>> regarding the gitweb.perl script.
>>>>>
>>>>> This module imports $fallback_encoding variable from
>>>>> Gitweb::Config module to use it in sub 'to_utf8'
>>>>>
>>>>> Subroutines moved:
>> [...]
>>>>>       unquote
>>>>
>>>> Shouldn't unquote be in Gitweb::Parse, as contrary to the rest of
>>>> subroutines is is not about gitweb output escaping/quoting, but
>>>> about processing (parsing) output of git commands?  Perhaps it
>>>> could even be provate to Gitweb::Parse (i.e. not exported by
>>>> default)...
>>>
>>> It would result in circular dependency. So, Escape module is best for
>>> it's place.
>>
>> Errr... how?  If unquote is used only by subroutines in Gitweb::Parse
>> (and I think it is), it could be local to Gitweb::Parse, not even
>> exported (by default).  Please explain.
> 
> Oh! I didn't know that.
> I will change it.

O.K.

Show 19 quoted lines

>>>>> 8. gitweb: Create Gitweb::View module
>>>>>
>>>>> Create Gitweb::View module in 'gitweb/lib/Gitweb/View.pm'
>>>>> to store the subroutines related to the HTML output
>>>>> for gitweb.
>>>>
>>>> I find that this module looks a bit like a collection of fairly unrelated
>>>> subroutines at very different levels of abstractions.
>>>>
>>>> I guess that you don't want to split gitweb into too many modules,
>>>> and splitting gitweb is more of one of steps to final goal of adding
>>>> write functionality to gitweb, rather than the goal in and of itself.
>>>> Nevertheless it would be good to be able to immediately know from the
>>>> description of module what kind of subroutines should be there.
>>
>> Any comment on this, if you don't mind?  Even "I don't want to think more
>> about better split" would be all right for me.
> 
> I don't want to think more about better split :)

NOTE that the goal of splitting gitweb was to make it possible to put
write functionalities for gitweb in separate module(s).  Would it be
possible with this proposed split?  If it is, then further enhancements
and refactoring to how gitweb is split into modules could be done 
"in tree", after this GSoC 2010 project is completed.
 
BTW. what write feature would be easiest to write?  Authentication
(only localhost / 127.0.0.1)?  Creating lightweight tags?  Scanning
for repositories and saving them in project index file?

Show 16 quoted lines

>>>>> This module depends on Git.pm, Config.pm, Request.pm,
>>>>> Escape.pm and RepoConfig.pm. Some subroutines which
>>>>> output HTML but are not included in this module due
>>>>> to unmet dependencies.
>>>>
>>>> Which subroutines and what unmet dependencies?
>>>
>>> action specific HTML divs. They include format_* and parse_* subs.
>>
>> Thanks for this info.  It should be, IMHO, in the comit message.
>>
>> But... Shouldn't all format_* subs be in Gitweb::Format anyway?
>> Shouldn't all parse_* subs be in Gitweb::Parse anyway?  Which of format_*
>> and parse_* do you feel that belong here?
> 
> Yes. I think you misunderstood me.

Ah, format_* and parse_* were "unment dependencies", not "not included subroutines".

So what were those "not includes subroutines"?

Show 6 quoted lines

>
> Gitweb::Parse and Gitweb::Formst depend on Gitweb::View.
> So, action specific HTML divs can't be placed in Gitweb::View because
> they depend on Gitweb::Format and Gitweb::Parse.
> 
> I think it's better if they are in GitwebAction::* modules

All right.

P.S. Unentangling interdependencies between gitweb subroutines can be left for later, but it looks like it would have to be necessary later.

-- 
Jakub Narebski
Poland