About [PATCH 2/2] gitweb: Create a perl module to handle gitweb cgi params and vars

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