From: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>
Date: Thu, 03 Jun 2010 16:11:00 GMT
Subject: Re: [PATCH GSoC 1/3] gitweb: Create Gitweb::Config module
Message-ID: <AANLkTimoA95U0vivTzrc0XZ8i6q-SfCFA6RgMWK67OWl@mail.gmail.com>
In-Reply-To: <201006031755.29814.jnareb@gmail.com>

2010/6/3 Jakub Narebski <jnareb@gmail.com>:
> On Tue, 3 Jun 2010, Petr Baudis wrote:
>>
>>   I have couple of concerns; maybe they were addressed in the previous
>> discussion which I admit I did not read completely, but in that case
>> they ought to be addressed in the commit message as well.
>>
>> On Thu, Jun 03, 2010 at 07:25:54PM +0530, Pavan Kumar Sunkara wrote:
>> > -our $t0;
>> > -if (eval { require Time::HiRes; 1; }) {
>> > -   $t0 = [Time::HiRes::gettimeofday()];
>>
>> Why is this moved to Gitweb::Config? Shouldn't this be rather part of
>> Gitweb::Request?
>
> I also think that this should be either part of Gitweb::Request, oe
> even be left in gitweb.perl.  I think having it in Gitweb::Request
> would be a better idea, because it is about time (and number of git
> commands) it took to process request.

Ok. It will be done.


>> > +
>> > +use Gitweb::Config;
>> >
>> >  BEGIN {
>> >     CGI->compile() if $ENV{'MOD_PERL'};
>> >  }
>> >
>> > -our $version = "++GIT_VERSION++";
>> > +$version = "++GIT_VERSION++";
>
> This change is not necessary.
>
>  our $version = "++GIT_VERSION++";
>
> would keep working even if '$version' is declared in other module and
> exported by this module (is imported into current scope).

Ok. Will change it.

>> >
>> >  our ($my_url, $my_uri, $base_url, $path_info, $home_link);
>> >  sub evaluate_uri {
>> > @@ -68,402 +71,58 @@ sub evaluate_uri {
>> >
>> >  # core git executable to use
>> >  # this can just be "git" if your webserver has a sensible PATH
>> > -our $GIT = "++GIT_BINDIR++/git";
>> > +$GIT = "++GIT_BINDIR++/git";
>>
>> I dislike the new schema in one aspect - the list of configuration
>> variables together with their description is not at a single place
>> anymore: the build-time overridable variables have their descriptions
>> still in gitweb.pl and only very brief mentions in Gitweb::Config, while
>> the rest has moved fully to Gitweb::Config. I think it would be best to
>> move all descriptions to Gitweb::Config and keep only the override
>> assignments in gitweb.pl. So, Gitweb::Config would have
>>
>>       # core git executable to use
>>       # this can just be "git" if your webserver has a sensible PATH
>>       our $GIT;
>
> Good idea.
>
> Perhaps we should provide some sane default fallback values, like for
> example
>
>        our $GIT = "git";
>
>>
>> and gitweb.pl would have _just_
>>
>>       $GIT = "++GIT_BINDIR++/git";
>
> I would say
>
>        our $GIT = "++GIT_BINDIR++/git";

But, I think when we start reading the code, it would seem that 'our
$GIT' implies that it is a variable created locally rather than an
exported variable from Gitweb::Config module.

Even though it increases the patch size, I don't think it will be much
of a concern when it comes to good redability of code.

Jakub: Can you reply, what you think about this argument ?

Thanks,
Pavan.