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

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

On Wed, Jun 23, 2010 at 1:59 AM, Jakub Narebski [off-list ref] wrote:

quoted

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

quoted

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?

[...]

quoted

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.

quoted

quoted

quoted

quoted

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:

[...]

quoted

quoted

quoted

      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.
 

quoted

quoted

quoted

quoted

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?

quoted

quoted

quoted

quoted

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

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