Re: About [PATCH] gitweb: Create a perl module to store gitweb configuration

2010/6/3 Jakub Narebski [off-list ref]:

This 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

quoted

From 9526ab8a12c8d4d875d08a6201c7eca963c7d9aa Mon Sep 17 00:00:00 2001
From: Pavan Kumar Sunkara <redacted>
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).

quoted

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

Ok.

quoted

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.

Ok.

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?

Nope. I actually wanted to say that any of the previous
gitweb_config.perl will work without the problems of scoping.

quoted

Change Makefile accordingly to install the Perl Module.

I would say:

 Update gitweb/Makefile to install gitweb modules alongside gitweb.

Or something like that.

Ok.

quoted

Signed-off-by: Pavan Kumar Sunkara <redacted>
---

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.

Ok.

quoted

 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))#'

It's great.

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?

Yeah. I have already written some of it in python before I applied for
gsoc. So, I have a good idea over it on the conceptual level.

quoted

 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?

Yes. Exactly.

quoted

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

quoted

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

Ok.

quoted

+# __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...

quoted

 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.

Yeah. I am currently removing them.

I would remove all such changes from discussion of this patch.

quoted

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

quoted

 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.

Ok. Will do it.

quoted

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

quoted

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

quoted

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

Sorry. That was a mistake. I probably ammended the commit while
working on Gitweb:Request Module.

quoted

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

quoted

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?

quoted

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

Ok. Sure.

quoted

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

quoted

+

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

Ok.

quoted

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

quoted

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

Will check it out and update README accoringly.

quoted

+
+1;

O.K.

Good work!
--
Jakub Narebski
Poland

Thanks,
Pavan.