Re: [RFC/PATCH 1/4] gitweb: Move subroutines to Gitweb::Config module

[Jakub Narebski <jnareb@gmail.com>]

A few generic comments about this whole series.


First, when you are sending larger patch series (and I think this series
of 4 patches qualify), you should provide cover letter, [RFC/PATCH 0/4]
in this case, describing the series.  git-format-patch has --cover-letter
option to make it easier to write such cover letters.  I could then write
generic comments about whole series as comments to cover letter...


Second, all those commits about splitting gitweb should perhaps be
reorganized (rebased).  Currently you have 'create Gitweb::Config',
'create Gitweb::Git', 'move subs to Gitweb::Config'; if Gitweb::Git
was created before Gitweb::Config, you would not need two commits
to create Gitweb::Config (to be exact even more than two, as not all
subroutines are moved, as you write yourself).


Third, and I think most important, is that the whole splitting gitweb into
modules series seems to alck direction, some underlying architecture
design.  For example Gitweb::HTML, Gitweb::HTML::Link, Gitweb::HTML::String
seems to me too detailed, too fine-grained modules.

It was not visible at first, because Gitweb::Config, Gitweb::Request and to
a bit lesser extent Gitweb::Git fell out naturally.  But should there be
for example Gitweb::Escape module, or should its functionality be a part of
Gitweb::Util?  Those issues needs to be addressed.  Perhaps they were
discussed with this GSoC project mentors (via IRC, private email, IM), but
we don't know what is the intended architecture design of gitweb.

Should we try for Model-Viewer-Controller pattern without backing MVC
(micro)framework?  (One of design decisions for gitweb was have it working
out of the box if Perl and git are installed, without requiring to install
extra modules; but now we can install extra Perl modules e.g. from CPAN
under lib/...).  How should we organize gitweb code into packages
(modules)?

Perhaps having gitweb.perl, Gitweb::Git, Gitweb::Config, Gitweb::Request,
Gitweb::Util and Gitweb would be enough?  Should it be Gitweb::HTML or
Gitweb::View?  Etc., etc.,...

That is a very important question.


On Mon, 7 June 2010, Pavan Kumar Sunkara wrote:

> Subject: [RFC/PATCH 1/4] gitweb: Move subroutines to Gitweb::Config module
>

This summary doesn't tell us much.  What subroutines, or what kind of
subroutines were moved to Gitweb::Config?  Why they were moved, and why
they weren't there from beginning (perhaps it would be better to simply
reorder patches, to have creation of Gitweb::Git before creation of
Gitweb::Config)?  Why can they be moved now?

Not all of those questions can be answered in summary (subject line for
email), but it should tell us what this commit is about.

> Subroutines moved:
> 	gitweb_get_feature
> 	gitweb_check_feature
> 	filter_snapshot_fmts
> 	configure_gitweb_features

I can find which subroutines were moved from the patch itself.  What
I cannot find without good commit message is _why_ do you feel they
belong in Gitweb::Config (unless it is obvious), and why they can be
moved now and why they could not be moved earlier.

> 
> Subroutines yet to move: (Contains not yet packaged subs & vars)
> 	feature_bool
> 	feature_avatar
> 	feature_snapshot
> 	feature_patches

Why they can't be moved; what is missing?  That is the question that commit
message should answer, or at least hint about / point the direction.

> 
> Signed-off-by: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>
> ---

> diff --git a/gitweb/lib/Gitweb/Config.pm b/gitweb/lib/Gitweb/Config.pm
> index fdab9f7..3810fda 100644
> --- a/gitweb/lib/Gitweb/Config.pm
> +++ b/gitweb/lib/Gitweb/Config.pm
> @@ -10,13 +10,16 @@ use strict;
>  use warnings;
>  use Exporter qw(import);
>  
> -our @EXPORT = qw(evaluate_gitweb_config $version $projectroot $project_maxdepth $mimetypes_file
> +our @EXPORT = qw(evaluate_gitweb_config gitweb_check_feature gitweb_get_feature configure_gitweb_features
> +                 filter_snapshot_fmts $version $projectroot $project_maxdepth $mimetypes_file $git_avatar
>                   $projects_list @git_base_url_list $export_ok $strict_export $home_link_str $site_name
>                   $site_header $site_footer $home_text @stylesheets $stylesheet $logo $favicon $javascript
>                   $GITWEB_CONFIG $GITWEB_CONFIG_SYSTEM $logo_url $logo_label $export_auth_hook
>                   $projects_list_description_width $default_projects_order $default_blob_plain_mimetype
>                   $default_text_plain_charset $fallback_encoding @diff_opts $prevent_xss $maxload
> -                 %avatar_size %known_snapshot_formats %known_snapshot_format_aliases %feature);
> +                 %avatar_size %known_snapshot_formats %feature @snapshot_fmts);

I think it would be better to have exported subroutines separated from
exported variables at least in that exported variables should begin on new
line.  This way it would be easier to see what was added, and (sometimes)
what was removed.

I had to look carefully to notice that you have removed the
%known_snapshot_format_aliases variable.  This was not mentioned in the
commit message.  There was no reason for removing it given in the commit
message (probably it was removed because it is internal to gitweb, and is
used only for backward compatibility with older configs... but it might be
not so internal, and might be useful and should be exported).

> +
> +use Gitweb::Git qw($git_dir);

O.K.


> +sub gitweb_get_feature {
[...]
> +	# project specific override is possible only if we have project
> +	if (!$override || !defined $git_dir) {
> +		return @defaults;
> +	}

Note that '!defined $git_dir' is a bit of hack, to check whether we are in
git repository and if we can run per-repository config.  This probably
could be solved better... but I don't mean that you would have to solve it.
It can be left for later.

-- 
Jakub Narebski
Poland