Re: [PATCH GSoC 1/3] gitweb: Create Gitweb::Config module

2010/6/3 Jakub Narebski [off-list ref]:

On Tue, 3 Jun 2010, Petr Baudis wrote:

quoted

  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:

quoted

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

quoted

quoted

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

quoted

quoted

 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";

quoted

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.