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

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.

quoted

quoted

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

quoted

quoted

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