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