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

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

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

Actually FindBin is in core since 5.4, and we require Perl 5.8+ for
gitweb anyway (for Encode and proper Unicode support).  

But from what I've heard FindBin is not recommended anymore, although
perhaps the disadvantages of FindBin doesn't matter in our situation.


>From FindBin(3pm) manpage:

  KNOWN ISSUES

  If there are two modules using `FindBin` from different directories under
  the same interpreter, this won't work. Since `FindBin` uses a `BEGIN` block,
  it'll be executed only once, and only the first caller will get it
  right.  **This is a problem under mod_perl** and other persistent Perl
  environments, where you shouldn't use this module.

>From what I remember there might also be problems with symlinks.


>From Dir::Self(3pm) manpage:

  DESCRIPTION

  Perl has two pseudo-constants describing the current location in your
  source code, __FILE__ and __LINE__.  This module adds __DIR__, which
  expands to the directory your source file is in, as an absolute pathname.

  This is useful if your code wants to access files in the same directory,
  like helper modules or configuration data.  This is a bit like `FindBin`
  except it's not limited to the main program, i.e. you can also use it in
  modules.  **And it actually works.**


Is there on git mailing list a Perl hacker that can tell us one way or
another?  (Not that this matter much, if it works).

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

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

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

Right.  I wholehartily agree.

> >  # 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!)

No, it isn't.  And without 'our $var = VALUE' -> '$var = VALUE' change,
which is not necessary and artifically inflates the size of patch, this
chunk wouldn't even be present.

-- 
Jakub Narebski
Poland