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

From: Jakub Narebski <jnareb@gmail.com>
Date: Jun 8, 2010, 12:46 UTC
Message-ID: <201006081446.22587.jnareb@gmail.com>
In-Reply-To: <1275943844-24991-1-git-send-email-pavan.sss1991@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.

Show 5 quoted lines

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

Show 6 quoted lines

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

Show 18 quoted lines

> 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