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

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

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

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

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?

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?

The fragment below was slightly edited to make it more clear.

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

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

 
 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.

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

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

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

Same with $git_dir -- it is request dependent, not configuration
dependent.

[very large cut, not necessary with exporting variables]

                                            ^^^^

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

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

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

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

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.

This comment is about commit a6d54b3 (gitweb: Create a perl module to
handle gitweb cgi params and vars, 2010-06-02) on 'master' branch of
repository shown at http://repo.or.cz/w/git/gsoc2010-gitweb.git

From a6d54b39cee3d8d39c7ffd993b23e7477d4b0eeb Mon Sep 17 00:00:00 2001
From: Pavan Kumar Sunkara <redacted>
Date: Wed, 2 Jun 2010 01:09:51 +0530
Subject: [PATCH] gitweb: Create a perl module to handle gitweb cgi params and vars

I would probably say

  Subject: [PATCH] gitweb: Create Gitweb::Request module to handle gitweb cgi params and vars

or even

  Subject: [PATCH] gitweb: Create Gitweb::Request module

so that _name_ of this module is in commit subject (summary).

Create a perl module in path gitweb/lib/Gitweb/Request.pm
to store and handle all the cgi params and global variables
regarding the gitweb.perl script and change the scope of those
variables in the main script.

Change Makefile accordingly to install the Perl Module.

Similar comment like for the previous patch.

You would probably want to list which subroutines were moved to
Gitweb::Request, I think.

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

This patch would be probably based on 'next', and this should be
mentioned in this area, in comment about patch.

Nice.

Nice.

The above context line changed wrt. original patch, to follow the idea
about exporting variables by default in Gitweb::Config, and not using
fully qualified variable names.

 
-our ($my_url, $my_uri, $base_url, $path_info, $home_link);
-sub evaluate_uri {
-	our $cgi;

I guess that $cgi is left for gitweb.perl, isn't it, because it
depends on FastCGI vs CGI?  [reads later part].  I guess not, only
$CGI is left in gitweb.perl.

[...]

-}

O.K. these variables, and evaluate_uri() is moved to Gitweb::Request.

[removed changes related to introducing fully qualified variable
 names; it would make patch smaller without those]

[...]

 }

[...]

 }

 sub evaluate_and_validate_params {

[...]

 }

Shouldn't evaluate_query_params(), evaluate_path_info(), and the
subroutine that ties them together evaluate_and_validate_params() be
in Gitweb::Request too?

 
 sub evaluate_git_dir {

[....]

 }

Ditto with evaluate_git_dir()?

BTW. as I said in comment to previous patch, vriables such as $project
should be put in Gitweb::Request in my opinion, not in Gitweb::Config.
Perhaps they are, but it is not obvious from this patch.

[...]

 }
 
 sub run_request {

Those two subroutines should not, I guess, be put in Gitweb::Request.
They would be in catch-all Gitweb module, I guess, or perhaps in the
later post-GSoC future in Gitweb::Dispatch or something.

Ah, so $CGI is left in gitweb.perl, $cgi is moved to Gitweb::Request.
  
[cut more than half of patch, which sould not be needed with exporting
 variables and not using fully qualified variable names]

I don't remember what is git policy on copyright statements, and on
GPLv2 vs GPLv2+...

+
+package Gitweb::Request;
+
+use strict;
+
+BEGIN {
+	use Exporter();
+
+	@Gitweb::Request::ISA = qw(Exporter);	
+	@Gitweb::Request::EXPORT = qw();
+}

Same comment as for previous patch.

   use Exporter qw(import);
   our @EXPORT = qw($cgi, $my_url, $my_uri, $base_url, ...);

BTW with exporting variables you can easily see here that
Gitweb::Request does not use any variable from Gitweb::Config.

+
+our ($cgi, $my_url, $my_uri, $base_url, $path_info, $home_link);
+our ($action, $project, $file_name, $file_parent, $hash, $hash_parent, $hash_base,
+     $hash_parent_base, @extra_options, $page, $git_dir);
+our ($searchtype, $search_use_regexp, $searchtext, $search_regexp);
+
+sub evaluate_uri {

[...]

+}

Straightforward code movement.  But see comment above on which
subroutines I feel should be also put in Gitweb::Request.

-- 
Jakub Narebski
Poland

Shouldn't evaluate_query_params(), evaluate_path_info(), and the
subroutine that ties them together evaluate_and_validate_params() be
in Gitweb::Request too?

quoted

 sub evaluate_git_dir {

[....]

quoted

 }

Ditto with evaluate_git_dir()?

Well, evaluate_and_validate_params() and evaluate_path_info() contains
calls to subroutines which are not yet moved into any package. So,
what do you want to in such a case ?

BTW. as I said in comment to previous patch, vriables such as $project
should be put in Gitweb::Request in my opinion, not in Gitweb::Config.
Perhaps they are, but it is not obvious from this patch.

quoted

 our (@snapshot_fmts, $git_avatar);

@@ -643,32 +605,32 @@ set_message(\&handle_errors_html);

 # dispatch
 sub dispatch {

[...]

quoted

 }

 sub run_request {

Those two subroutines should not, I guess, be put in Gitweb::Request.
They would be in catch-all Gitweb module, I guess, or perhaps in the
later post-GSoC future in Gitweb::Dispatch or something.

Yes.

quoted

@@ -689,7 +651,6 @@ sub run_request {

 our $is_last_request = sub { 1 };
 our ($pre_dispatch_hook, $post_dispatch_hook, $pre_listen_hook);
 our $CGI = 'CGI';
-our $cgi;
 sub evaluate_argv {
      return unless (@ARGV);

Ah, so $CGI is left in gitweb.perl, $cgi is moved to Gitweb::Request.

[cut more than half of patch, which sould not be needed with exporting
 variables and not using fully qualified variable names]

quoted

diff --git a/gitweb/lib/Gitweb/Request.pm b/gitweb/lib/Gitweb/Request.pm
new file mode 100644
index 0000000..91cc492
--- /dev/null
+++ b/gitweb/lib/Gitweb/Request.pm

@@ -0,0 +1,58 @@
+#!/usr/bin/perl
+#
+# Gitweb::Request -- gitweb request(cgi) package
+#
+# This program is licensed under the GPLv2

I don't remember what is git policy on copyright statements, and on
GPLv2 vs GPLv2+...

quoted

+
+package Gitweb::Request;
+
+use strict;
+
+BEGIN {
+     use Exporter();
+
+     @Gitweb::Request::ISA = qw(Exporter);
+     @Gitweb::Request::EXPORT = qw();
+}

Same comment as for previous patch.

  use Exporter qw(import);
  our @EXPORT = qw($cgi, $my_url, $my_uri, $base_url, ...);

BTW with exporting variables you can easily see here that
Gitweb::Request does not use any variable from Gitweb::Config.

quoted

+
+our ($cgi, $my_url, $my_uri, $base_url, $path_info, $home_link);
+our ($action, $project, $file_name, $file_parent, $hash, $hash_parent, $hash_base,
+     $hash_parent_base, @extra_options, $page, $git_dir);
+our ($searchtype, $search_use_regexp, $searchtext, $search_regexp);
+
+sub evaluate_uri {

[...]

quoted

+}

Straightforward code movement.  But see comment above on which
subroutines I feel should be also put in Gitweb::Request.

Please answer the question regarding calls to not-yet-packaged subroutines.

Thanks,
Pavan.

On Tue, 3 Jun 2010, Pavan Kumar Sunkara wrote:

Jakub Narebski wrote:

quoted

Shouldn't evaluate_query_params(), evaluate_path_info(), and the
subroutine that ties them together evaluate_and_validate_params() be
in Gitweb::Request too?

Ditto with evaluate_git_dir()?

Well, evaluate_and_validate_params() and evaluate_path_info() contains
calls to subroutines which are not yet moved into any package. So,
what do you want to in such a case ?

O.K., in this case you should mention in the commit message that
the subroutines evaluate_query_params(), evaluate_path_info(),
evaluate_and_validate_params() and evaluate_git_dir() didn't get
moved to Gitweb::Request because evaluate_and_validate_params()
and evaluate_path_info() contain calls to check_head_link(),
die_error() and validate_*() subroutines which are not yet moved
into any package.  And it doesn't make sense to put only some
of them in Gitweb::Request.

We can always move them later.


P.S. The validate_*() subroutines could also be moved to
Gitweb::Request.  The check_head_link() subroutine looks like
candidate for Gitweb::Util / Gitweb::Utils; I am not sure where
one should put die_error() and friends: git_header_html(),
git_footer_html(), get_page_title(), and other subroutines
they use.

-- 
Jakub Narebski
Poland

On Thu, 3 Jun 2010, Pavan Kumar Sunkarawrote:

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

quoted

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

quoted

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.

quoted

But I think this change can be left for a separate commit.  It is not
something terribly important, something blocking accepting the patch.

Actually to really be able to put gitweb packages somewhere else,
gitweb.perl should *probably* contain

  use lib '++GITWEBLIBDIR++';

Otherwise 'gitweblibdir' would have to be either left default to
install packages alingside gitweb.cgi script, or be directory that is
in PERL5LIB for web server.

So let's leave it for a later commit.

-- 
Jakub Narebski
Poland