2010/6/3 Jakub Narebski <jnareb@xxxxxxxxx>:
> 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.
--
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