Re: [PATCH GSoC 1/3] gitweb: Create Gitweb::Config module
From: Petr Baudis <hidden>
Date: 2016-06-15 22:48:54
Hi! I think this is a good start! 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?
+# __DIR__ is taken from Dir::Self __DIR__ fragment
+sub __DIR__ () {
+ File::Spec->rel2abs(join '', (File::Spec->splitpath(__FILE__))[0, 1]);
}
-our $number_of_git_cmds = 0;
+use lib __DIR__ . "/lib";Wouldn't it be more elegant to use FindBin? I'm just not sure how long is it part of core Perl.
quoted hunk ↗ jump to hunk
+ +use Gitweb::Config; BEGIN { CGI->compile() if $ENV{'MOD_PERL'}; } -our $version = "++GIT_VERSION++"; +$version = "++GIT_VERSION++"; 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; and gitweb.pl would have _just_ $GIT = "++GIT_BINDIR++/git"; How does that sound? I think you ought to add a comment in front of this section explaining that not all configuration variables are listed here anymore. Something like # Only configuration variables with build-time overridable # defaults are listed below. The complete set of variables # with their descriptions is listed in Gitweb::Config.
# name of your site or organization to appear in page titles
# replace this with something more descriptive for clearer bookmarks
-our $site_name = "++GITWEB_SITENAME++"
+$site_name = "++GITWEB_SITENAME++"
|| ($ENV{'SERVER_NAME'} || "Untitled") . " Git";This looks like some new feature; please do that in a separate patch. (BTW, I assume that there are no other changes like this in the rest of the moved code blocks!) -- Petr "Pasky" Baudis The true meaning of life is to plant a tree under whose shade you will never sit.