Jakub NarebskiThis comment is about commit 9526ab8 (gitweb: Create a perl module to store gitweb configuration, 2010-06-01) on 'master' branch of repository shown at http://repo.or.cz/w/git/gsoc2010-gitweb.git
> From 9526ab8a12c8d4d875d08a6201c7eca963c7d9aa Mon Sep 17 00:00:00 2001 > From: Pavan Kumar Sunkara <pavan.sss1991@gmail.com> > Date: Tue, 1 Jun 2010 02:36:48 +0530 > Subject: [PATCH] gitweb: Create a perl module to store gitweb configuration
I would probably say
Subject: [PATCH] gitweb: Create Gitweb::Config module to store gitweb configuration
or even
Subject: [PATCH] gitweb: Create Gitweb::Config module
so that _name_ of this module is in commit subject (summary).
> > Create a perl module in path gitweb/lib/Gitweb/Config.pm > to store all the configuration variables of the gitweb.perl > script and change the scope of those variables in main script.
A bit overlong sentence, isn't it? Perhaps
Create a Gitweb::Config module in 'gitweb/lib/Gitweb/Config.pm' to store all the configuration variables of the gitweb.perl script.
(Assuming that I can convince you to avoid fully qualified names).
> > Move subroutine evaluate_gitweb_config() into the same module > so that the statement 'do GITWEB_CONFIG' or 'do GITWEB_CONFIG_SYSTEM' > works similiar to the working before the split.
s/similiar/similar/
I agree that you should list which subroutines you do move to Gitweb::Config package, though this list would be probably longer that the evaluate_gitweb_config() alone... or perhaps not.
But I do not understand the statement about "do $GITWEB_CONFIG" etc. here. Did you wanted to say that setting $GITWEB_CONFIG etc. was moved out of evaluate_gitweb_config(), and is left in gitweb.perl?
> > Change Makefile accordingly to install the Perl Module.
I would say:
Update gitweb/Makefile to install gitweb modules alongside gitweb.
Or something like that.
> > Signed-off-by: Pavan Kumar Sunkara <pavan.sss1991@gmail.com> > ---
When sending this patch to git mailing list you should mention that it is based on gitweb: Put all per-connection code in run() subroutine and perhaps also on gitweb: Add support for FastCGI, using CGI::Fast unless those two commits made it into 'master' till then.
Show 15 quoted lines
> gitweb/Makefile | 6 + > gitweb/gitweb.perl | 692 ++++++++++--------------------------------- > gitweb/lib/Gitweb/Config.pm | 389 ++++++++++++++++++++++++ > 3 files changed, 558 insertions(+), 529 deletions(-) > create mode 100644 gitweb/lib/Gitweb/Config.pm > > diff --git a/gitweb/Makefile b/gitweb/Makefile > index d2584fe..45e176e 100644 > --- a/gitweb/Makefile > +++ b/gitweb/Makefile > @@ -55,6 +55,7 @@ PERL_PATH ?= /usr/bin/perl > bindir_SQ = $(subst ','\'',$(bindir))#' > gitwebdir_SQ = $(subst ','\'',$(gitwebdir))#' > gitwebstaticdir_SQ = $(subst ','\'',$(gitwebdir)/static)#' > +gitweblibdir_SQ = $(subst ','\'',$(gitwebdir)/lib)#'
I think it would be good idea to have 'gitweblibdir' as a separate variable, alongside 'gitwebdir', and which would default to
gitweblibdir = $(gitwebdir)/lib
to make it possible to install gitweb modules not alongside gitweb, but somewhere else, for example together with other Perl modules.
Then you would have:
+gitweblibdir_SQ = $(subst ','\'',$(gitweblibdir))#'
But I think this change can be left for a separate commit. It is not something terribly important, something blocking accepting the patch.
BTW. did you start working on the 'write' part yet, at least on the conceptual (specification / architecture) level?
Show 19 quoted lines
> SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))#' > PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))#' > DESTDIR_SQ = $(subst ','\'',$(DESTDIR))#' > @@ -110,6 +111,9 @@ endif > > GITWEB_FILES += static/git-logo.png static/git-favicon.png > > +# Files: gitweb/lib/Gitweb > +GITWEB_LIB_GITWEB += lib/Gitweb/Config.pm > + > GITWEB_REPLACE = \ > -e 's|++GIT_VERSION++|$(GIT_VERSION)|g' \ > -e 's|++GIT_BINDIR++|$(bindir)|g' \ > @@ -150,6 +154,8 @@ install: all > $(INSTALL) -m 755 $(GITWEB_PROGRAMS) '$(DESTDIR_SQ)$(gitwebdir_SQ)' > $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitwebstaticdir_SQ)' > $(INSTALL) -m 644 $(GITWEB_FILES) '$(DESTDIR_SQ)$(gitwebstaticdir_SQ)' > + $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitweblibdir_SQ)/Gitweb' > + $(INSTALL) -m 644 $(GITWEB_LIB_GITWEB) '$(DESTDIR_SQ)$(gitweblibdir_SQ)/Gitweb'
Uhhh... this would probably get more complicated *if* there would be more complicated hierarchy, e.g. if there would be Gitweb module in lib/Gitweb.pm. I guess that is the reason behind $(GITWEB_LIB_GITWEB) name, isn't it?
Show 14 quoted lines
> > ### Cleaning rules > > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl > index 673e7a3..98a85f4 100755 > --- a/gitweb/gitweb.perl > +++ b/gitweb/gitweb.perl > @@ -16,19 +16,22 @@ use Encode; > use Fcntl ':mode'; > use File::Find qw(); > use File::Basename qw(basename); > +use File::Spec; > binmode STDOUT, ':utf8'; >
The fragment below was slightly edited to make it more clear.
Show 5 quoted lines
> -our $t0;
> -if (eval { require Time::HiRes; 1; }) {
> - $t0 = [Time::HiRes::gettimeofday()];
> -}
> -our $number_of_git_cmds = 0;Theoretically neither $t0 nor $number_of_git_cmds belong to Gitweb::Config, as those are about runtime timing, not about gitweb configuration.
But perhaps for the time being you can put it in Gitweb::Config.
Show 7 quoted lines
> +# __DIR__ is taken from Dir::Self __DIR__ fragment
> +sub __DIR__ () {
> + File::Spec->rel2abs(join '', (File::Spec->splitpath(__FILE__))[0, 1]);
> +}
> +use lib __DIR__ . "/lib";
> +
> +use Gitweb::Config;O.K.
It's a pity that Dir::Self is not a core Perl module since Perl 5.8, and that FindBin which is in core since 5.4 has its quirks and nowadays is not recommended to use...
Show 7 quoted lines
>
> BEGIN {
> CGI->compile() if $ENV{'MOD_PERL'};
> }
>
> -our $version = "++GIT_VERSION++";
> +$Gitweb::Config::version = "++GIT_VERSION++";If '$version' was exported by Gitweb::Config this change would be not necessary.
I would remove all such changes from discussion of this patch.
Show 5 quoted lines
> -# URI and label (title) of GIT logo link > -#our $logo_url = "http://www.kernel.org/pub/software/scm/git/docs/"; > -#our $logo_label = "git documentation"; > -our $logo_url = "http://git-scm.com/"; > -our $logo_label = "git homepage";
All right, those variables that do not need initialization using build-time substitutions are put together with their description in Gitweb::Config.
I would remove all such changes from discussion of this patch.
> sub gitweb_get_feature {
> my ($name) = @_;[...]
I wonder if this subroutine, and its companion 'gitweb_check_feature' should be not moved to Gitweb::Config module.
> -our ($GITWEB_CONFIG, $GITWEB_CONFIG_SYSTEM);
> +our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || "++GITWEB_CONFIG++";
> +our $GITWEB_CONFIG_SYSTEM = $ENV{'GITWEB_CONFIG_SYSTEM'} || "++GITWEB_CONFIG_SYSTEM++";This would probably look like this, because $GITWEB_CONFIG must be set in gitweb.perl -> gitweb.cgi, and not in Gitweb::Config, where evaluate_gitweb_config() would be.
Show 12 quoted lines
> -sub evaluate_gitweb_config {
> - our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || "++GITWEB_CONFIG++";
> - our $GITWEB_CONFIG_SYSTEM = $ENV{'GITWEB_CONFIG_SYSTEM'} || "++GITWEB_CONFIG_SYSTEM++";
> - # die if there are errors parsing config file
> - if (-e $GITWEB_CONFIG) {
> - do $GITWEB_CONFIG;
> - die $@ if $@;
> - } elsif (-e $GITWEB_CONFIG_SYSTEM) {
> - do $GITWEB_CONFIG_SYSTEM;
> - die $@ if $@;
> - }
> -}Right, this got moved to Gitweb::Config.
Show 6 quoted lines
> -our ($action, $project, $file_name, $file_parent, $hash, $hash_parent, $hash_base,
> - $hash_parent_base, @extra_options, $page, $searchtype, $search_use_regexp,
> - $searchtext, $search_regexp);
> sub evaluate_and_validate_params {
> our $action = $input_params{'action'};
> if (defined $action) {Hmmm... it looks like those got also moved to Gitweb::Config, even if those variables have nothing to do with gitweb configuration, but are about request, and therefore belong to Gitweb::Request. Thus this commit should probably not touch this.
Show 9 quoted lines
> @@ -963,10 +602,8 @@ sub evaluate_and_validate_params {
> }
> }
>
> -# path to the current git repository
> -our $git_dir;
> sub evaluate_git_dir {
> our $git_dir = "$projectroot/$project" if $project;
> }Same with $git_dir -- it is request dependent, not configuration dependent.
[very large cut, not necessary with exporting variables]
Show 20 quoted lines
> diff --git a/gitweb/lib/Gitweb/Config.pm b/gitweb/lib/Gitweb/Config.pm
> new file mode 100644
> index 0000000..e1bd06f
> --- /dev/null
> +++ b/gitweb/lib/Gitweb/Config.pm
> @@ -0,0 +1,389 @@
> +#!/usr/bin/perl
> +#
> +# Gitweb::Config -- gitweb configuration package
> +#
> +# This program is licensed under the GPLv2
> +
> +package Gitweb::Config;
> +
> +use strict;
> +
> +BEGIN {
> + use Exporter();
> +
> + @Gitweb::Config::ISA = qw(Exporter); ^^^^
Trailing whitespace. Didn't git warn you about this?
> + @Gitweb::Config::EXPORT = qw(); > +}
Why BEGIN block? Why 'use Exporter();' and not 'use Exporter;'?
Why not
use Exporter; use base 'Exporter';
or even
use Exporter qw(import);
(we can use the last version, because gitweb requires high enough Perl version itself, so that this form can be used).
Show 6 quoted lines
> +
> +our $t0;
> +if (eval { require Time::HiRes; 1; }) {
> + $t0 = [Time::HiRes::gettimeofday()];
> +}
> +our $number_of_git_cmds = 0;This does not strictly speaking belong in Gitweb::Config, and probably neither in Gitweb::Request, but Gitweb::request is a better place for it.
Never-mind, we can move it later.
> +
Here it would be good place for comment that those are variables that are affected by build-time configuration, and therefore their initialization is put in gitweb.perl (together with comments with their description).
Show 12 quoted lines
> +our ($GIT, $version, $git_version); > +our ($projectroot, $project_maxdepth, $projects_list, @git_base_url_list); > +our ($export_ok, $strict_export); > +our ($home_link_str, $site_name, $site_header, $site_footer, $home_text); > +our (@stylesheets, $stylesheet, $logo, $favicon, $javascript); > +our ($GITWEB_CONFIG, $GITWEB_CONFIG_SYSTEM); > + > +# URI and label (title) of GIT logo link > +#our $logo_url = "http://www.kernel.org/pub/software/scm/git/docs/"; > +#our $logo_label = "git documentation"; > +our $logo_url = "http://git-scm.com/"; > +our $logo_label = "git homepage";
[cut]
Nothing especially interesting here. 'git blame -C -C' should detect code movement.
Show 11 quoted lines
> +
> +sub evaluate_gitweb_config {
> + # die if there are errors parsing config file
> + if (-e $GITWEB_CONFIG) {
> + do $GITWEB_CONFIG;
> + die $@ if $@;
> + } elsif (-e $GITWEB_CONFIG_SYSTEM) {
> + do $GITWEB_CONFIG_SYSTEM;
> + die $@ if $@;
> + }
> +}Here the question is if this would affect interpretation of $GITWEB_CONFIG etc. if it is a relative path. Perhaps nothing will change, because paths are relative to the directory the script is run, not relative to where module resides.
Not that is matter much either way, as at most it would require stating in gitweb/README that one should use absolute pathnames.
Note that tests use absolute pathname, so passing test does not answer this.
> + > +1;
O.K.
Good work!
-- Jakub Narebski Poland
