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

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

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

quoted

quoted

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

quoted

quoted

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

quoted

quoted

quoted

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.

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.

quoted

quoted

quoted

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.

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 :)

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

quoted

quoted

quoted

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

[...]

quoted

quoted

quoted

      is_patch_split

O.K.

quoted

      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.

quoted

quoted

quoted

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?

quoted

quoted

quoted

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.