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

[Pavan Kumar Sunkara <pavan.sss1991@gmail.com>]

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

>>>
>>> What unment dependencies?  Can it be worked around by changing their
>>> signatures to use additional parameters, e.g.:
>>>
>>>  sub validate_action {
>>>        my $input = shift || return undef;
>>>        my $actions = shift || return undef;
>>>
>>>        return undef unless exists $actions->{$input};
>>>        return $input;
>>>  }
>>
>> Yes. I think we can but don't you think that we need to import
>> %actions everytime we use validate_action.
>> So, I figured it would be better If i leave it untouched for now.
>
> Well, changing subroutines could be thought as being ot of scope of
> current series, which is about splitting gitweb and moving subroutines to
> appripriate modules... unless it would be not possible because of
> circular dependencies problem (but I guess it is not the case,
> ultimately).
>
>>
>>> But I guess it would be very hard to do the same with validate_project.
>>> Also such change might be out of scope for _this_ series.
>
> 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

>>>>
>>>> Subroutines moved:
>>>>       evaluate_uri
>>>>       evaluate_query_params
>>>>       validate_pathname
>>>>       validate_refname
>>>
>>> I'm not completly sure if validate_* subroutines should be not separate,
>>> as they do require more knowledge about setup, and about git repositories
>>> served, than the rest of Gitweb::Request subroutines...
>>
>> But they are being used in Gitweb::Request subroutines, so I included
>> them in there.
>
> Which Gitweb::Request subroutine uses validate_* subroutines?  Neither
> evaluate_uri, nor evaluate_query_params, nor evaluate_path_info uses
> them.  And evaluate_and_validate_params can be in Gitweb::Request::Validate.
>
> If there is circular dependency in reasoning given above, please explain
> it.
>
>>>> 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.

>>>> 7. gitweb: Create Gitweb::RepoConfig module
>>>>
>>>> Create a Gitweb::RepoConfig module in 'gitweb/lib/Gitweb/RepoConfig.pm'
>>>> to store and handle all the configuration and subroutines
>>>> related to a single repository regarding the gitweb.perl script.
>>>>
>>>> This module depend on several other modules like Git.pm,
>>>> Config.pm, Request.pm and Escape.pm.
>>>>
>>>> It also include subroutines regarding project_list and
>>>> it's handling.
>>>
>>> Why?  Is it to not have too many tiny modules, or is simply there no
>>> single good place where to put this subroutine?
>>
>> There is no single good place. Anyhow, as they are regarding project
>> configuration, I thought it would be better place for it.
>
> O.K.
>
>>>> 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 :)

>>>> 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 u misunderstood me.
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

>>>> 9. gitweb: Create Gitweb::Util module
>>>>
>>>> Create Gitweb::Util module in 'gitweb/lib/Gitweb/Util.pm'
>>>> to store the git utility subroutines related to gitweb.
>>>>
>>>> This module include subroutines in various categories
>>>> such as git utility subs invoking git commands, git
>>>> utility subs accessing git repository, mimetype related
>>>> subs and HTML output utility subs.
>>>>
>>>> Subroutines moved:
>>>>       git_get_head_hash
> [...]
>>>>       is_patch_split
>>>
>>> O.K.
>>>
>>>>       run_highlighter
>>>
>>> _Perhaps_ with exception of this subroutine.
>>
>> Well. It was in the utility category in gitweb.perl script. So, I
>> added it in here without giving much thought.
>
> Well, it is good explanation.  All right, then.
>
>>>> 10. gitweb: Create Gitweb::Format module
>>>>
>>>> Create Gitweb::Format module in 'gitweb/lib/Gitweb/Format.pm'
>>>> to store the subroutines related to formatting of HTML
>>>> fragments required for gitweb.
>>>>
>>>> This module depends on Config.pm, View.pm, Escape.pm,
>>>> Util.pm and Request.pm. It mainly contain functions returning
>>>> short HTML fragments or transforming HTML fragments. Also
>>>> include subroutines regarding avatar formatting.
>>>
>>> Why it depends on Gitweb::View, and not the reverse?  I understand why
>>> it depends on Gitweb::Config and Gitweb::Escape, and I guess it needs
>>> $cgi from Gitweb::Request (or is it something more?).
>>
>> $hash, $hash_parent also.
>
> Why it depends on Gitweb::View?  Is it because of ubiquitous and
> troublesome :-) die_error?
>
>>>> 11. gitweb: Create Gitweb::Parse module
>>>>
>>>> Create Gitweb::Parse module in 'gitweb/lib/Gitweb/Parse.pm'
>>>> to store the subroutines which related to parsing functions
>>>> required for gitweb.
>>>>
>>>> This module depends on Git.pm, Escape.pm, View.pm and Util.pm.
>>>
>>> Why it depends on Gitweb::View?  It is a strange dependency.
>>> I understand depending on Gitweb::Git to some extent, I'm not sure
>>> if we shouldn't simply move unescape to it instead of requiring
>>> Gitweb::Escape (unless there is more: to_utf8 perhaps?), and
>>> I understand that Gitweb::Util has some required subroutines.
>>
>> for die_error.
>
> Ah, I se that any run of git command is protected with
> 'or die_error(...)'.  Hmmm, not very good.
>
> Well, in the future we could think about decoupling Gitweb::Parse from
> other modules, but for now it should be enough to mention in commit
> message that Gitweb::Parse depends on Gitweb::View because of die_error.
> Good for now.  O.K.
>
> Perhaps die_error should be in Gitweb::Carp?  But it neds git_header_html
> etc. anyway.  Ugh.
>
> --
> Jakub Narebski
> Poland
>

Thanks,
Pavan.