From: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>
Date: Thu, 03 Jun 2010 09:52:32 GMT
Subject: Re: About [PATCH 2/2] gitweb: Create a perl module to handle gitweb cgi params and vars
Message-ID: <AANLkTilYSAj_byhZ1WDEBrYOAj6OZVsyDmp59vuK0tQz@mail.gmail.com>
In-Reply-To: <201006031055.28966.jnareb@gmail.com>

> 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()?

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

Yes.

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

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

Thanks,
Pavan.