Re: [PATCH GSoC 1/3] gitweb: Create Gitweb::Config module
- From
Petr Baudis <pasky@suse.cz>
- Date
- Jun 3, 2010, 16:06 UTC
- Message-ID
- <20100603160605.GF20775@machine.or.cz>
- In-Reply-To
- <201006031755.29814.jnareb@gmail.com>
On Thu, Jun 03, 2010 at 05:55:28PM +0200, Jakub Narebski wrote:
> But from what I've heard FindBin is not recommended anymore, although > perhaps the disadvantages of FindBin doesn't matter in our situation.
Ah, never mind then. It's probably more trouble than it's worth, even if it's not problematic for us right at the moment.
Show 16 quoted lines
> > > +
> > > +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).Hmm, that's right, but it feels dirty since it strongly suggests that $version is then a variable local to this package. It would reduce the diff size, but I perosnally don't think it's worth it. I'll leave this up to Pavan personally, though.
> Perhaps we should provide some sane default fallback values, like for > example > > our $GIT = "git";
I considered that, but I'm not too fond of this within this patch, I'd rather keep the pieces simple and stupid.
Show 13 quoted lines
> > > # 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.I'm sorry, I was just blind.
-- Petr "Pasky" Baudis The true meaning of life is to plant a tree under whose shade you will never sit.