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

[Jakub Narebski <jnareb@gmail.com>]

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 <pavan.sss1991@gmail.com>
> 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 <pavan.sss1991@gmail.com>
> ---

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

>  gitweb/Makefile              |    1 +
>  gitweb/gitweb.perl           | 1245 ++++++++++++++++++++----------------------
>  gitweb/lib/Gitweb/Request.pm |   58 ++
>  3 files changed, 662 insertions(+), 642 deletions(-)
>  create mode 100644 gitweb/lib/Gitweb/Request.pm
> 
> diff --git a/gitweb/Makefile b/gitweb/Makefile
> index 45e176e..15646b2 100644
> --- a/gitweb/Makefile
> +++ b/gitweb/Makefile
> @@ -113,6 +113,7 @@ GITWEB_FILES += static/git-logo.png static/git-favicon.png
>  
>  # Files: gitweb/lib/Gitweb
>  GITWEB_LIB_GITWEB += lib/Gitweb/Config.pm
> +GITWEB_LIB_GITWEB += lib/Gitweb/Request.pm

Nice.

> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 98a85f4..263af48 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -26,6 +26,7 @@ sub __DIR__ () {
>  use lib __DIR__ . "/lib";
>  
>  use Gitweb::Config;
> +use Gitweb::Request;

Nice.

> @@ -33,42 +34,6 @@ BEGIN {
>  
>  our $version = "++GIT_VERSION++";

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]

> @@ -347,13 +312,11 @@ our %allowed_options = (
>  # build an array of parameters that can be multi-valued, but since for the time
>  # being it's only this one, we just single it out
>  sub evaluate_query_params {
> -	our $cgi;
[...]
>  }

> @@ -361,12 +324,12 @@ sub evaluate_query_params {
>  # now read PATH_INFO and update the parameter list for missing parameters
>  sub evaluate_path_info {
[...]
>  }

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

>  
>  our (@snapshot_fmts, $git_avatar);
> @@ -643,32 +605,32 @@ set_message(\&handle_errors_html);
>  
>  # dispatch
>  sub dispatch {
[...]
>  }
>  
>  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.

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

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

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