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@xxxxxxxxx> > 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@xxxxxxxxx> > --- 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html