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