threads / patch / 23991

patch, 3 partsgitweb: Create Gitweb::Config module

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

## tl;dr

13 messages between Jun 3, 2010 and Jun 3, 2010. Diffs are folded; open one to read it.

replies: 12people: 4as markdown or json

Pavan Kumar Sunkara· Jun 3, 2010, 13:55 UTC · lore

Create Gitweb::Config module in 'gitweb/lib/Gitweb/Config.pm' to store all the configuration variables of the gitweb.perl script.

Subroutines moved:
	evaluate_gitweb_config
	evaluate_git_version
Subroutines yet to move: (Contains calls to not yet packaged subs)
	gitweb_get_feature
	gitweb_check_feature
Update gitweb/Makefile to install gitweb modules alongside gitweb
Signed-off-by: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>
---
This patch is based on branch 'pu' of alt-git.git
 gitweb/Makefile             |    6 +
 gitweb/gitweb.perl          |  421 +++----------------------------------------
 gitweb/lib/Gitweb/Config.pm |  413 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 447 insertions(+), 393 deletions(-)
 create mode 100644 gitweb/lib/Gitweb/Config.pm
Show changes to 3 files +447 −393

gitweb/Makefile, gitweb/gitweb.perl, gitweb/lib/Gitweb/Config.pm

diff --git a/gitweb/Makefile b/gitweb/Makefile
index d2584fe..45e176e 100644
--- a/gitweb/Makefile
+++ b/gitweb/Makefile
@@ -55,6 +55,7 @@ PERL_PATH  ?= /usr/bin/perl
 bindir_SQ = $(subst ','\'',$(bindir))#'
 gitwebdir_SQ = $(subst ','\'',$(gitwebdir))#'
 gitwebstaticdir_SQ = $(subst ','\'',$(gitwebdir)/static)#'
+gitweblibdir_SQ = $(subst ','\'',$(gitwebdir)/lib)#'
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))#'
 PERL_PATH_SQ  = $(subst ','\'',$(PERL_PATH))#'
 DESTDIR_SQ    = $(subst ','\'',$(DESTDIR))#'
@@ -110,6 +111,9 @@ endif
 
 GITWEB_FILES += static/git-logo.png static/git-favicon.png
 
+# Files: gitweb/lib/Gitweb
+GITWEB_LIB_GITWEB += lib/Gitweb/Config.pm
+
 GITWEB_REPLACE = \
 	-e 's|++GIT_VERSION++|$(GIT_VERSION)|g' \
 	-e 's|++GIT_BINDIR++|$(bindir)|g' \
@@ -150,6 +154,8 @@ install: all
 	$(INSTALL) -m 755 $(GITWEB_PROGRAMS) '$(DESTDIR_SQ)$(gitwebdir_SQ)'
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitwebstaticdir_SQ)'
 	$(INSTALL) -m 644 $(GITWEB_FILES) '$(DESTDIR_SQ)$(gitwebstaticdir_SQ)'
+	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitweblibdir_SQ)/Gitweb'
+	$(INSTALL) -m 644 $(GITWEB_LIB_GITWEB) '$(DESTDIR_SQ)$(gitweblibdir_SQ)/Gitweb'
 
 ### Cleaning rules
 
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 673e7a3..ef71656 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -16,19 +16,22 @@ use Encode;
 use Fcntl ':mode';
 use File::Find qw();
 use File::Basename qw(basename);
+use File::Spec;
 binmode STDOUT, ':utf8';
 
-our $t0;
-if (eval { require Time::HiRes; 1; }) {
-	$t0 = [Time::HiRes::gettimeofday()];
+# __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";
+
+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";
 
 # absolute fs-path which will be prepended to the project path
 #our $projectroot = "/pub/scm";
-our $projectroot = "++GITWEB_PROJECTROOT++";
+$projectroot = "++GITWEB_PROJECTROOT++";
 
 # fs traversing limit for getting project list
 # the number is relative to the projectroot
-our $project_maxdepth = "++GITWEB_PROJECT_MAXDEPTH++";
+$project_maxdepth = "++GITWEB_PROJECT_MAXDEPTH++";
 
 # string of the home link on top of all pages
-our $home_link_str = "++GITWEB_HOME_LINK_STR++";
+$home_link_str = "++GITWEB_HOME_LINK_STR++";
 
 # 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";
 
 # filename of html text to include at top of each page
-our $site_header = "++GITWEB_SITE_HEADER++";
+$site_header = "++GITWEB_SITE_HEADER++";
 # html text to include at home page
-our $home_text = "++GITWEB_HOMETEXT++";
+$home_text = "++GITWEB_HOMETEXT++";
 # filename of html text to include at bottom of each page
-our $site_footer = "++GITWEB_SITE_FOOTER++";
+$site_footer = "++GITWEB_SITE_FOOTER++";
 
 # URI of stylesheets
-our @stylesheets = ("++GITWEB_CSS++");
+@stylesheets = ("++GITWEB_CSS++");
 # URI of a single stylesheet, which can be overridden in GITWEB_CONFIG.
-our $stylesheet = undef;
+$stylesheet = undef;
 # URI of GIT logo (72x27 size)
-our $logo = "++GITWEB_LOGO++";
+$logo = "++GITWEB_LOGO++";
 # URI of GIT favicon, assumed to be image/png type
-our $favicon = "++GITWEB_FAVICON++";
+$favicon = "++GITWEB_FAVICON++";
 # URI of gitweb.js (JavaScript code for gitweb)
-our $javascript = "++GITWEB_JS++";
-
-# URI and label (title) of GIT logo link
-#our $logo_url = "http://www.kernel.org/pub/software/scm/git/docs/";
-#our $logo_label = "git documentation";
-our $logo_url = "http://git-scm.com/";
-our $logo_label = "git homepage";
+$javascript = "++GITWEB_JS++";
 
 # source of projects list
-our $projects_list = "++GITWEB_LIST++";
-
-# the width (in characters) of the projects list "Description" column
-our $projects_list_description_width = 25;
-
-# default order of projects list
-# valid values are none, project, descr, owner, and age
-our $default_projects_order = "project";
+$projects_list = "++GITWEB_LIST++";
 
 # show repository only if this file exists
 # (only effective if this variable evaluates to true)
-our $export_ok = "++GITWEB_EXPORT_OK++";
-
-# show repository only if this subroutine returns true
-# when given the path to the project, for example:
-#    sub { return -e "$_[0]/git-daemon-export-ok"; }
-our $export_auth_hook = undef;
+$export_ok = "++GITWEB_EXPORT_OK++";
 
 # only allow viewing of repositories also shown on the overview page
-our $strict_export = "++GITWEB_STRICT_EXPORT++";
+$strict_export = "++GITWEB_STRICT_EXPORT++";
 
 # list of git base URLs used for URL to where fetch project from,
 # i.e. full URL is "$git_base_url/$project"
-our @git_base_url_list = grep { $_ ne '' } ("++GITWEB_BASE_URL++");
-
-# default blob_plain mimetype and default charset for text/plain blob
-our $default_blob_plain_mimetype = 'text/plain';
-our $default_text_plain_charset  = undef;
-
-# file to use for guessing MIME types before trying /etc/mime.types
-# (relative to the current git repository)
-our $mimetypes_file = undef;
-
-# assume this charset if line contains non-UTF-8 characters;
-# it should be valid encoding (see Encoding::Supported(3pm) for list),
-# for which encoding all byte sequences are valid, for example
-# 'iso-8859-1' aka 'latin1' (it is decoded without checking, so it
-# could be even 'utf-8' for the old behavior)
-our $fallback_encoding = 'latin1';
-
-# rename detection options for git-diff and git-diff-tree
-# - default is '-M', with the cost proportional to
-#   (number of removed files) * (number of new files).
-# - more costly is '-C' (which implies '-M'), with the cost proportional to
-#   (number of changed files + number of removed files) * (number of new files)
-# - even more costly is '-C', '--find-copies-harder' with cost
-#   (number of files in the original tree) * (number of new files)
-# - one might want to include '-B' option, e.g. '-B', '-M'
-our @diff_opts = ('-M'); # taken from git_commit
-
-# Disables features that would allow repository owners to inject script into
-# the gitweb domain.
-our $prevent_xss = 0;
-
-# information about snapshot formats that gitweb is capable of serving
-our %known_snapshot_formats = (
-	# name => {
-	# 	'display' => display name,
-	# 	'type' => mime type,
-	# 	'suffix' => filename suffix,
-	# 	'format' => --format for git-archive,
-	# 	'compressor' => [compressor command and arguments]
-	# 	                (array reference, optional)
-	# 	'disabled' => boolean (optional)}
-	#
-	'tgz' => {
-		'display' => 'tar.gz',
-		'type' => 'application/x-gzip',
-		'suffix' => '.tar.gz',
-		'format' => 'tar',
-		'compressor' => ['gzip']},
-
-	'tbz2' => {
-		'display' => 'tar.bz2',
-		'type' => 'application/x-bzip2',
-		'suffix' => '.tar.bz2',
-		'format' => 'tar',
-		'compressor' => ['bzip2']},
-
-	'txz' => {
-		'display' => 'tar.xz',
-		'type' => 'application/x-xz',
-		'suffix' => '.tar.xz',
-		'format' => 'tar',
-		'compressor' => ['xz'],
-		'disabled' => 1},
-
-	'zip' => {
-		'display' => 'zip',
-		'type' => 'application/x-zip',
-		'suffix' => '.zip',
-		'format' => 'zip'},
-);
+@git_base_url_list = grep { $_ ne '' } ("++GITWEB_BASE_URL++");
 
-# Aliases so we understand old gitweb.snapshot values in repository
-# configuration.
-our %known_snapshot_format_aliases = (
-	'gzip'  => 'tgz',
-	'bzip2' => 'tbz2',
-	'xz'    => 'txz',
-
-	# backward compatibility: legacy gitweb config support
-	'x-gzip' => undef, 'gz' => undef,
-	'x-bzip2' => undef, 'bz2' => undef,
-	'x-zip' => undef, '' => undef,
-);
-
-# Pixel sizes for icons and avatars. If the default font sizes or lineheights
-# are changed, it may be appropriate to change these values too via
-# $GITWEB_CONFIG.
-our %avatar_size = (
-	'default' => 16,
-	'double'  => 32
-);
-
-# Used to set the maximum load that we will still respond to gitweb queries.
-# If server load exceed this value then return "503 server busy" error.
-# If gitweb cannot determined server load, it is taken to be 0.
-# Leave it undefined (or set to 'undef') to turn off load checking.
-our $maxload = 300;
-
-# You define site-wide feature defaults here; override them with
-# $GITWEB_CONFIG as necessary.
-our %feature = (
-	# feature => {
-	# 	'sub' => feature-sub (subroutine),
-	# 	'override' => allow-override (boolean),
-	# 	'default' => [ default options...] (array reference)}
-	#
-	# if feature is overridable (it means that allow-override has true value),
-	# then feature-sub will be called with default options as parameters;
-	# return value of feature-sub indicates if to enable specified feature
-	#
-	# if there is no 'sub' key (no feature-sub), then feature cannot be
-	# overriden
-	#
-	# use gitweb_get_feature(<feature>) to retrieve the <feature> value
-	# (an array) or gitweb_check_feature(<feature>) to check if <feature>
-	# is enabled
-
-	# Enable the 'blame' blob view, showing the last commit that modified
-	# each line in the file. This can be very CPU-intensive.
-
-	# To enable system wide have in $GITWEB_CONFIG
-	# $feature{'blame'}{'default'} = [1];
-	# To have project specific config enable override in $GITWEB_CONFIG
-	# $feature{'blame'}{'override'} = 1;
-	# and in project config gitweb.blame = 0|1;
-	'blame' => {
-		'sub' => sub { feature_bool('blame', @_) },
-		'override' => 0,
-		'default' => [0]},
-
-	# Enable the 'snapshot' link, providing a compressed archive of any
-	# tree. This can potentially generate high traffic if you have large
-	# project.
-
-	# Value is a list of formats defined in %known_snapshot_formats that
-	# you wish to offer.
-	# To disable system wide have in $GITWEB_CONFIG
-	# $feature{'snapshot'}{'default'} = [];
-	# To have project specific config enable override in $GITWEB_CONFIG
-	# $feature{'snapshot'}{'override'} = 1;
-	# and in project config, a comma-separated list of formats or "none"
-	# to disable.  Example: gitweb.snapshot = tbz2,zip;
-	'snapshot' => {
-		'sub' => \&feature_snapshot,
-		'override' => 0,
-		'default' => ['tgz']},
-
-	# Enable text search, which will list the commits which match author,
-	# committer or commit text to a given string.  Enabled by default.
-	# Project specific override is not supported.
-	'search' => {
-		'override' => 0,
-		'default' => [1]},
-
-	# Enable grep search, which will list the files in currently selected
-	# tree containing the given string. Enabled by default. This can be
-	# potentially CPU-intensive, of course.
-
-	# To enable system wide have in $GITWEB_CONFIG
-	# $feature{'grep'}{'default'} = [1];
-	# To have project specific config enable override in $GITWEB_CONFIG
-	# $feature{'grep'}{'override'} = 1;
-	# and in project config gitweb.grep = 0|1;
-	'grep' => {
-		'sub' => sub { feature_bool('grep', @_) },
-		'override' => 0,
-		'default' => [1]},
-
-	# Enable the pickaxe search, which will list the commits that modified
-	# a given string in a file. This can be practical and quite faster
-	# alternative to 'blame', but still potentially CPU-intensive.
-
-	# To enable system wide have in $GITWEB_CONFIG
-	# $feature{'pickaxe'}{'default'} = [1];
-	# To have project specific config enable override in $GITWEB_CONFIG
-	# $feature{'pickaxe'}{'override'} = 1;
-	# and in project config gitweb.pickaxe = 0|1;
-	'pickaxe' => {
-		'sub' => sub { feature_bool('pickaxe', @_) },
-		'override' => 0,
-		'default' => [1]},
-
-	# Enable showing size of blobs in a 'tree' view, in a separate
-	# column, similar to what 'ls -l' does.  This cost a bit of IO.
-
-	# To disable system wide have in $GITWEB_CONFIG
-	# $feature{'show-sizes'}{'default'} = [0];
-	# To have project specific config enable override in $GITWEB_CONFIG
-	# $feature{'show-sizes'}{'override'} = 1;
-	# and in project config gitweb.showsizes = 0|1;
-	'show-sizes' => {
-		'sub' => sub { feature_bool('showsizes', @_) },
-		'override' => 0,
-		'default' => [1]},
-
-	# Make gitweb use an alternative format of the URLs which can be
-	# more readable and natural-looking: project name is embedded
-	# directly in the path and the query string contains other
-	# auxiliary information. All gitweb installations recognize
-	# URL in either format; this configures in which formats gitweb
-	# generates links.
-
-	# To enable system wide have in $GITWEB_CONFIG
-	# $feature{'pathinfo'}{'default'} = [1];
-	# Project specific override is not supported.
-
-	# Note that you will need to change the default location of CSS,
-	# favicon, logo and possibly other files to an absolute URL. Also,
-	# if gitweb.cgi serves as your indexfile, you will need to force
-	# $my_uri to contain the script name in your $GITWEB_CONFIG.
-	'pathinfo' => {
-		'override' => 0,
-		'default' => [0]},
-
-	# Make gitweb consider projects in project root subdirectories
-	# to be forks of existing projects. Given project $projname.git,
-	# projects matching $projname/*.git will not be shown in the main
-	# projects list, instead a '+' mark will be added to $projname
-	# there and a 'forks' view will be enabled for the project, listing
-	# all the forks. If project list is taken from a file, forks have
-	# to be listed after the main project.
-
-	# To enable system wide have in $GITWEB_CONFIG
-	# $feature{'forks'}{'default'} = [1];
-	# Project specific override is not supported.
-	'forks' => {
-		'override' => 0,
-		'default' => [0]},
-
-	# Insert custom links to the action bar of all project pages.
-	# This enables you mainly to link to third-party scripts integrating
-	# into gitweb; e.g. git-browser for graphical history representation
-	# or custom web-based repository administration interface.
-
-	# The 'default' value consists of a list of triplets in the form
-	# (label, link, position) where position is the label after which
-	# to insert the link and link is a format string where %n expands
-	# to the project name, %f to the project path within the filesystem,
-	# %h to the current hash (h gitweb parameter) and %b to the current
-	# hash base (hb gitweb parameter); %% expands to %.
-
-	# To enable system wide have in $GITWEB_CONFIG e.g.
-	# $feature{'actions'}{'default'} = [('graphiclog',
-	# 	'/git-browser/by-commit.html?r=%n', 'summary')];
-	# Project specific override is not supported.
-	'actions' => {
-		'override' => 0,
-		'default' => []},
-
-	# Allow gitweb scan project content tags described in ctags/
-	# of project repository, and display the popular Web 2.0-ish
-	# "tag cloud" near the project list. Note that this is something
-	# COMPLETELY different from the normal Git tags.
-
-	# gitweb by itself can show existing tags, but it does not handle
-	# tagging itself; you need an external application for that.
-	# For an example script, check Girocco's cgi/tagproj.cgi.
-	# You may want to install the HTML::TagCloud Perl module to get
-	# a pretty tag cloud instead of just a list of tags.
-
-	# To enable system wide have in $GITWEB_CONFIG
-	# $feature{'ctags'}{'default'} = ['path_to_tag_script'];
-	# Project specific override is not supported.
-	'ctags' => {
-		'override' => 0,
-		'default' => [0]},
-
-	# The maximum number of patches in a patchset generated in patch
-	# view. Set this to 0 or undef to disable patch view, or to a
-	# negative number to remove any limit.
-
-	# To disable system wide have in $GITWEB_CONFIG
-	# $feature{'patches'}{'default'} = [0];
-	# To have project specific config enable override in $GITWEB_CONFIG
-	# $feature{'patches'}{'override'} = 1;
-	# and in project config gitweb.patches = 0|n;
-	# where n is the maximum number of patches allowed in a patchset.
-	'patches' => {
-		'sub' => \&feature_patches,
-		'override' => 0,
-		'default' => [16]},
-
-	# Avatar support. When this feature is enabled, views such as
-	# shortlog or commit will display an avatar associated with
-	# the email of the committer(s) and/or author(s).
-
-	# Currently available providers are gravatar and picon.
-	# If an unknown provider is specified, the feature is disabled.
-
-	# Gravatar depends on Digest::MD5.
-	# Picon currently relies on the indiana.edu database.
-
-	# To enable system wide have in $GITWEB_CONFIG
-	# $feature{'avatar'}{'default'} = ['<provider>'];
-	# where <provider> is either gravatar or picon.
-	# To have project specific config enable override in $GITWEB_CONFIG
-	# $feature{'avatar'}{'override'} = 1;
-	# and in project config gitweb.avatar = <provider>;
-	'avatar' => {
-		'sub' => \&feature_avatar,
-		'override' => 0,
-		'default' => ['']},
-
-	# Enable displaying how much time and how many git commands
-	# it took to generate and display page.  Disabled by default.
-	# Project specific override is not supported.
-	'timed' => {
-		'override' => 0,
-		'default' => [0]},
-
-	# Enable turning some links into links to actions which require
-	# JavaScript to run (like 'blame_incremental').  Not enabled by
-	# default.  Project specific override is currently not supported.
-	'javascript-actions' => {
-		'override' => 0,
-		'default' => [0]},
-
-	# Syntax highlighting support. This is based on Daniel Svensson's
-	# and Sham Chukoury's work in gitweb-xmms2.git.
-	# It requires the 'highlight' program present in $PATH,
-	# and therefore is disabled by default.
-
-	# To enable system wide have in $GITWEB_CONFIG
-	# $feature{'highlight'}{'default'} = [1];
-
-	'highlight' => {
-		'sub' => sub { feature_bool('highlight', @_) },
-		'override' => 0,
-		'default' => [0]},
-);
+$GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || "++GITWEB_CONFIG++";
+$GITWEB_CONFIG_SYSTEM = $ENV{'GITWEB_CONFIG_SYSTEM'} || "++GITWEB_CONFIG_SYSTEM++";
 
 sub gitweb_get_feature {
 	my ($name) = @_;
@@ -571,20 +230,6 @@ sub filter_snapshot_fmts {
 		!$known_snapshot_formats{$_}{'disabled'}} @fmts;
 }
 
-our ($GITWEB_CONFIG, $GITWEB_CONFIG_SYSTEM);
-sub evaluate_gitweb_config {
-	our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || "++GITWEB_CONFIG++";
-	our $GITWEB_CONFIG_SYSTEM = $ENV{'GITWEB_CONFIG_SYSTEM'} || "++GITWEB_CONFIG_SYSTEM++";
-	# die if there are errors parsing config file
-	if (-e $GITWEB_CONFIG) {
-		do $GITWEB_CONFIG;
-		die $@ if $@;
-	} elsif (-e $GITWEB_CONFIG_SYSTEM) {
-		do $GITWEB_CONFIG_SYSTEM;
-		die $@ if $@;
-	}
-}
-
 # Get loadavg of system, to compare against $maxload.
 # Currently it requires '/proc/loadavg' present to get loadavg;
 # if it is not present it returns 0, which means no load checking.
@@ -607,13 +252,6 @@ sub get_loadavg {
 	return 0;
 }
 
-# version of the core git binary
-our $git_version;
-sub evaluate_git_version {
-	our $git_version = qx("$GIT" --version) =~ m/git version (.*)$/ ? $1 : "unknown";
-	$number_of_git_cmds++;
-}
-
 sub check_loadavg {
 	if (defined $maxload && get_loadavg() > $maxload) {
 		die_error(503, "The load average on the server is too high");
@@ -1028,7 +666,7 @@ sub dispatch {
 }
 
 sub run_request {
-	our $t0 = [Time::HiRes::gettimeofday()]
+	$t0 = [Time::HiRes::gettimeofday()]
 		if defined $t0;
 
 	evaluate_uri();
@@ -2555,9 +2193,6 @@ sub git_get_projects_list {
 			follow_skip => 2, # ignore duplicates
 			dangling_symlinks => 0, # ignore dangling symlinks, silently
 			wanted => sub {
-				# global variables
-				our $project_maxdepth;
-				our $projectroot;
 				# skip project-list toplevel, if we get it.
 				return if (m!^[/.]$!);
 				# only directories can be git repositories
diff --git a/gitweb/lib/Gitweb/Config.pm b/gitweb/lib/Gitweb/Config.pm
new file mode 100644
index 0000000..cb8efa5
--- /dev/null
+++ b/gitweb/lib/Gitweb/Config.pm
@@ -0,0 +1,413 @@
+#!/usr/bin/perl
+#
+# Gitweb::Config -- gitweb configuration package
+#
+# This program is licensed under the GPLv2
+
+package Gitweb::Config;
+
+use strict;
+use warnings;
+use Exporter qw(import);
+
+our @ISA = qw(Exporter);
+our @EXPORT = qw($t0 $number_of_git_cmds $GIT $version $git_version $projectroot $project_maxdepth
+                 $projects_list @git_base_url_list $export_ok $strict_export $home_link_str $site_name
+                 $site_header $site_footer $home_text @stylesheets $stylesheet $logo $favicon $javascript
+                 $GITWEB_CONFIG $GITWEB_CONFIG_SYSTEM $logo_url $logo_label $default_projects_order
+                 $projects_list_description_width $export_auth_hook $default_blob_plain_mimetype $mimetypes_file
+                 $default_text_plain_charset $fallback_encoding @diff_opts $prevent_xss $maxload %avatar_size
+                 %known_snapshot_formats %known_snapshot_format_aliases %feature &evaluate_gitweb_config
+                 &evaluate_git_version);
+
+# Runtime variables
+our $t0;
+if (eval { require Time::HiRes; 1; }) {
+	$t0 = [Time::HiRes::gettimeofday()];
+}
+our $number_of_git_cmds = 0;
+
+# The following variables are affected by build-time configuration
+# and hence their initialisation is put in gitweb.perl script
+
+# core git executable path and versions
+our ($GIT, $version, $git_version);
+
+# fs-path, fs traversing limit, source of projects list and git base URL
+our ($projectroot, $project_maxdepth, $projects_list, @git_base_url_list);
+
+our ($export_ok, $strict_export);
+
+# Site configurations
+our ($home_link_str, $site_name, $site_header, $site_footer, $home_text);
+
+# Stylesheet, logo and javascript
+our (@stylesheets, $stylesheet, $logo, $favicon, $javascript);
+
+# gitweb config
+our ($GITWEB_CONFIG, $GITWEB_CONFIG_SYSTEM);
+
+# URI and label (title) of GIT logo link
+#our $logo_url = "http://www.kernel.org/pub/software/scm/git/docs/";
+#our $logo_label = "git documentation";
+our $logo_url = "http://git-scm.com/";
+our $logo_label = "git homepage";
+
+# the width (in characters) of the projects list "Description" column
+our $projects_list_description_width = 25;
+
+# default order of projects list
+# valid values are none, project, descr, owner, and age
+our $default_projects_order = "project";
+
+# show repository only if this subroutine returns true
+# when given the path to the project, for example:
+#    sub { return -e "$_[0]/git-daemon-export-ok"; }
+our $export_auth_hook = undef;
+
+# default blob_plain mimetype and default charset for text/plain blob
+our $default_blob_plain_mimetype = 'text/plain';
+our $default_text_plain_charset  = undef;
+
+# file to use for guessing MIME types before trying /etc/mime.types
+# (relative to the current git repository)
+our $mimetypes_file = undef;
+
+# assume this charset if line contains non-UTF-8 characters;
+# it should be valid encoding (see Encoding::Supported(3pm) for list),
+# for which encoding all byte sequences are valid, for example
+# 'iso-8859-1' aka 'latin1' (it is decoded without checking, so it
+# could be even 'utf-8' for the old behavior)
+our $fallback_encoding = 'latin1';
+
+# rename detection options for git-diff and git-diff-tree
+# - default is '-M', with the cost proportional to
+#   (number of removed files) * (number of new files).
+# - more costly is '-C' (which implies '-M'), with the cost proportional to
+#   (number of changed files + number of removed files) * (number of new files)
+# - even more costly is '-C', '--find-copies-harder' with cost
+#   (number of files in the original tree) * (number of new files)
+# - one might want to include '-B' option, e.g. '-B', '-M'
+our @diff_opts = ('-M'); # taken from git_commit
+
+# Disables features that would allow repository owners to inject script into
+# the gitweb domain.
+our $prevent_xss = 0;
+
+# information about snapshot formats that gitweb is capable of serving
+our %known_snapshot_formats = (
+	# name => {
+	# 	'display' => display name,
+	# 	'type' => mime type,
+	# 	'suffix' => filename suffix,
+	# 	'format' => --format for git-archive,
+	# 	'compressor' => [compressor command and arguments]
+	# 	                (array reference, optional)
+	# 	'disabled' => boolean (optional)}
+	#
+	'tgz' => {
+		'display' => 'tar.gz',
+		'type' => 'application/x-gzip',
+		'suffix' => '.tar.gz',
+		'format' => 'tar',
+		'compressor' => ['gzip']},
+
+	'tbz2' => {
+		'display' => 'tar.bz2',
+		'type' => 'application/x-bzip2',
+		'suffix' => '.tar.bz2',
+		'format' => 'tar',
+		'compressor' => ['bzip2']},
+
+	'txz' => {
+		'display' => 'tar.xz',
+		'type' => 'application/x-xz',
+		'suffix' => '.tar.xz',
+		'format' => 'tar',
+		'compressor' => ['xz'],
+		'disabled' => 1},
+
+	'zip' => {
+		'display' => 'zip',
+		'type' => 'application/x-zip',
+		'suffix' => '.zip',
+		'format' => 'zip'},
+);
+
+# Aliases so we understand old gitweb.snapshot values in repository
+# configuration.
+our %known_snapshot_format_aliases = (
+	'gzip'  => 'tgz',
+	'bzip2' => 'tbz2',
+	'xz'    => 'txz',
+
+	# backward compatibility: legacy gitweb config support
+	'x-gzip' => undef, 'gz' => undef,
+	'x-bzip2' => undef, 'bz2' => undef,
+	'x-zip' => undef, '' => undef,
+);
+
+# Pixel sizes for icons and avatars. If the default font sizes or lineheights
+# are changed, it may be appropriate to change these values too via
+# $GITWEB_CONFIG.
+our %avatar_size = (
+	'default' => 16,
+	'double'  => 32
+);
+
+# Used to set the maximum load that we will still respond to gitweb queries.
+# If server load exceed this value then return "503 server busy" error.
+# If gitweb cannot determined server load, it is taken to be 0.
+# Leave it undefined (or set to 'undef') to turn off load checking.
+our $maxload = 300;
+
+# You define site-wide feature defaults here; override them with
+# $GITWEB_CONFIG as necessary.
+our %feature = (
+	# feature => {
+	# 	'sub' => feature-sub (subroutine),
+	# 	'override' => allow-override (boolean),
+	# 	'default' => [ default options...] (array reference)}
+	#
+	# if feature is overridable (it means that allow-override has true value),
+	# then feature-sub will be called with default options as parameters;
+	# return value of feature-sub indicates if to enable specified feature
+	#
+	# if there is no 'sub' key (no feature-sub), then feature cannot be
+	# overriden
+	#
+	# use gitweb_get_feature(<feature>) to retrieve the <feature> value
+	# (an array) or gitweb_check_feature(<feature>) to check if <feature>
+	# is enabled
+
+	# Enable the 'blame' blob view, showing the last commit that modified
+	# each line in the file. This can be very CPU-intensive.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'blame'}{'default'} = [1];
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'blame'}{'override'} = 1;
+	# and in project config gitweb.blame = 0|1;
+	'blame' => {
+		'sub' => sub { feature_bool('blame', @_) },
+		'override' => 0,
+		'default' => [0]},
+
+	# Enable the 'snapshot' link, providing a compressed archive of any
+	# tree. This can potentially generate high traffic if you have large
+	# project.
+
+	# Value is a list of formats defined in %Gitweb::Config::known_snapshot_formats that
+	# you wish to offer.
+	# To disable system wide have in $GITWEB_CONFIG
+	# $feature{'snapshot'}{'default'} = [];
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'snapshot'}{'override'} = 1;
+	# and in project config, a comma-separated list of formats or "none"
+	# to disable.  Example: gitweb.snapshot = tbz2,zip;
+	'snapshot' => {
+		'sub' => \&feature_snapshot,
+		'override' => 0,
+		'default' => ['tgz']},
+
+	# Enable text search, which will list the commits which match author,
+	# committer or commit text to a given string.  Enabled by default.
+	# Project specific override is not supported.
+	'search' => {
+		'override' => 0,
+		'default' => [1]},
+
+	# Enable grep search, which will list the files in currently selected
+	# tree containing the given string. Enabled by default. This can be
+	# potentially CPU-intensive, of course.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'grep'}{'default'} = [1];
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'grep'}{'override'} = 1;
+	# and in project config gitweb.grep = 0|1;
+	'grep' => {
+		'sub' => sub { feature_bool('grep', @_) },
+		'override' => 0,
+		'default' => [1]},
+
+	# Enable the pickaxe search, which will list the commits that modified
+	# a given string in a file. This can be practical and quite faster
+	# alternative to 'blame', but still potentially CPU-intensive.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'pickaxe'}{'default'} = [1];
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'pickaxe'}{'override'} = 1;
+	# and in project config gitweb.pickaxe = 0|1;
+	'pickaxe' => {
+		'sub' => sub { feature_bool('pickaxe', @_) },
+		'override' => 0,
+		'default' => [1]},
+
+	# Enable showing size of blobs in a 'tree' view, in a separate
+	# column, similar to what 'ls -l' does.  This cost a bit of IO.
+
+	# To disable system wide have in $GITWEB_CONFIG
+	# $feature{'show-sizes'}{'default'} = [0];
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'show-sizes'}{'override'} = 1;
+	# and in project config gitweb.showsizes = 0|1;
+	'show-sizes' => {
+		'sub' => sub { feature_bool('showsizes', @_) },
+		'override' => 0,
+		'default' => [1]},
+
+	# Make gitweb use an alternative format of the URLs which can be
+	# more readable and natural-looking: project name is embedded
+	# directly in the path and the query string contains other
+	# auxiliary information. All gitweb installations recognize
+	# URL in either format; this configures in which formats gitweb
+	# generates links.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'pathinfo'}{'default'} = [1];
+	# Project specific override is not supported.
+
+	# Note that you will need to change the default location of CSS,
+	# favicon, logo and possibly other files to an absolute URL. Also,
+	# if gitweb.cgi serves as your indexfile, you will need to force
+	# $my_uri to contain the script name in your $GITWEB_CONFIG.
+	'pathinfo' => {
+		'override' => 0,
+		'default' => [0]},
+
+	# Make gitweb consider projects in project root subdirectories
+	# to be forks of existing projects. Given project $projname.git,
+	# projects matching $projname/*.git will not be shown in the main
+	# projects list, instead a '+' mark will be added to $projname
+	# there and a 'forks' view will be enabled for the project, listing
+	# all the forks. If project list is taken from a file, forks have
+	# to be listed after the main project.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'forks'}{'default'} = [1];
+	# Project specific override is not supported.
+	'forks' => {
+		'override' => 0,
+		'default' => [0]},
+
+	# Insert custom links to the action bar of all project pages.
+	# This enables you mainly to link to third-party scripts integrating
+	# into gitweb; e.g. git-browser for graphical history representation
+	# or custom web-based repository administration interface.
+
+	# The 'default' value consists of a list of triplets in the form
+	# (label, link, position) where position is the label after which
+	# to insert the link and link is a format string where %n expands
+	# to the project name, %f to the project path within the filesystem,
+	# %h to the current hash (h gitweb parameter) and %b to the current
+	# hash base (hb gitweb parameter); %% expands to %.
+
+	# To enable system wide have in $GITWEB_CONFIG e.g.
+	# $feature{'actions'}{'default'} = [('graphiclog',
+	# 	'/git-browser/by-commit.html?r=%n', 'summary')];
+	# Project specific override is not supported.
+	'actions' => {
+		'override' => 0,
+		'default' => []},
+
+	# Allow gitweb scan project content tags described in ctags/
+	# of project repository, and display the popular Web 2.0-ish
+	# "tag cloud" near the project list. Note that this is something
+	# COMPLETELY different from the normal Git tags.
+
+	# gitweb by itself can show existing tags, but it does not handle
+	# tagging itself; you need an external application for that.
+	# For an example script, check Girocco's cgi/tagproj.cgi.
+	# You may want to install the HTML::TagCloud Perl module to get
+	# a pretty tag cloud instead of just a list of tags.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'ctags'}{'default'} = ['path_to_tag_script'];
+	# Project specific override is not supported.
+	'ctags' => {
+		'override' => 0,
+		'default' => [0]},
+
+	# The maximum number of patches in a patchset generated in patch
+	# view. Set this to 0 or undef to disable patch view, or to a
+	# negative number to remove any limit.
+
+	# To disable system wide have in $GITWEB_CONFIG
+	# $feature{'patches'}{'default'} = [0];
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'patches'}{'override'} = 1;
+	# and in project config gitweb.patches = 0|n;
+	# where n is the maximum number of patches allowed in a patchset.
+	'patches' => {
+		'sub' => \&feature_patches,
+		'override' => 0,
+		'default' => [16]},
+
+	# Avatar support. When this feature is enabled, views such as
+	# shortlog or commit will display an avatar associated with
+	# the email of the committer(s) and/or author(s).
+
+	# Currently available providers are gravatar and picon.
+	# If an unknown provider is specified, the feature is disabled.
+
+	# Gravatar depends on Digest::MD5.
+	# Picon currently relies on the indiana.edu database.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'avatar'}{'default'} = ['<provider>'];
+	# where <provider> is either gravatar or picon.
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'avatar'}{'override'} = 1;
+	# and in project config gitweb.avatar = <provider>;
+	'avatar' => {
+		'sub' => \&feature_avatar,
+		'override' => 0,
+		'default' => ['']},
+
+	# Enable displaying how much time and how many git commands
+	# it took to generate and display page.  Disabled by default.
+	# Project specific override is not supported.
+	'timed' => {
+		'override' => 0,
+		'default' => [0]},
+
+	# Enable turning some links into links to actions which require
+	# JavaScript to run (like 'blame_incremental').  Not enabled by
+	# default.  Project specific override is currently not supported.
+	'javascript-actions' => {
+		'override' => 0,
+		'default' => [0]},
+
+	# Syntax highlighting support. This is based on Daniel Svensson's
+	# and Sham Chukoury's work in gitweb-xmms2.git.
+	# It requires the 'highlight' program present in $PATH,
+	# and therefore is disabled by default.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'highlight'}{'default'} = [1];
+
+	'highlight' => {
+		'sub' => sub { feature_bool('highlight', @_) },
+		'override' => 0,
+		'default' => [0]},
+);
+
+sub evaluate_gitweb_config {
+	# die if there are errors parsing config file
+	if (-e $GITWEB_CONFIG) {
+		do $GITWEB_CONFIG;
+		die $@ if $@;
+	} elsif (-e $GITWEB_CONFIG_SYSTEM) {
+		do $GITWEB_CONFIG_SYSTEM;
+		die $@ if $@;
+	}
+}
+
+sub evaluate_git_version {
+	$git_version = qx("$GIT" --version) =~ m/git version (.*)$/ ? $1 : "unknown";
+	$number_of_git_cmds++;
+}
+
+1;
-- 
1.7.1.447.gfddfb
Pavan Kumar Sunkara· Jun 3, 2010, 13:55 UTC · re: Pavan Kumar Sunkara · lore

[PATCH GSoC 2/3] gitweb: Create Gitweb::Request module

Create a Gitweb::Request module in 'gitweb/lib/Gitweb/Request.pm' to store and handle all the cgi params and related variables regarding the gitweb.perl script.

Subroutines moved:
	evaluate_uri
	evaluate_query_params
	evaluate_git_dir
	validate_pathname
	validate_refname
Subroutines yet to move: (Contains calls to not yet packages subs)
	evaluate_path_info
	evaluate_and _validate_params
	validate_action
	validate_project
Update gitweb/Makefile to install gitweb modules alongside gitweb
Signed-off-by: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>
---
This patch is based on branch 'pu' and previous patch.
 gitweb/Makefile              |    1 +
 gitweb/gitweb.perl           |  165 ++++--------------------------------------
 gitweb/lib/Gitweb/Request.pm |  156 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 171 insertions(+), 151 deletions(-)
 create mode 100644 gitweb/lib/Gitweb/Request.pm
Show changes to 3 files +171 −151

gitweb/Makefile, gitweb/gitweb.perl, gitweb/lib/Gitweb/Request.pm

diff --git a/gitweb/Makefile b/gitweb/Makefile
index 45e176e..15646b2 100644
--- a/gitweb/Makefile
+++ b/gitweb/Makefile
@@ -113,6 +113,7 @@ GITWEB_FILES += static/git-logo.png static/git-favicon.png
 
 # Files: gitweb/lib/Gitweb
 GITWEB_LIB_GITWEB += lib/Gitweb/Config.pm
+GITWEB_LIB_GITWEB += lib/Gitweb/Request.pm
 
 GITWEB_REPLACE = \
 	-e 's|++GIT_VERSION++|$(GIT_VERSION)|g' \
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index ef71656..2514b7a 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -26,6 +26,7 @@ sub __DIR__ () {
 use lib __DIR__ . "/lib";
 
 use Gitweb::Config;
+use Gitweb::Request;
 
 BEGIN {
 	CGI->compile() if $ENV{'MOD_PERL'};
@@ -33,42 +34,6 @@ BEGIN {
 
 $version = "++GIT_VERSION++";
 
-our ($my_url, $my_uri, $base_url, $path_info, $home_link);
-sub evaluate_uri {
-	our $cgi;
-
-	our $my_url = $cgi->url();
-	our $my_uri = $cgi->url(-absolute => 1);
-
-	# Base URL for relative URLs in gitweb ($logo, $favicon, ...),
-	# needed and used only for URLs with nonempty PATH_INFO
-	our $base_url = $my_url;
-
-	# When the script is used as DirectoryIndex, the URL does not contain the name
-	# of the script file itself, and $cgi->url() fails to strip PATH_INFO, so we
-	# have to do it ourselves. We make $path_info global because it's also used
-	# later on.
-	#
-	# Another issue with the script being the DirectoryIndex is that the resulting
-	# $my_url data is not the full script URL: this is good, because we want
-	# generated links to keep implying the script name if it wasn't explicitly
-	# indicated in the URL we're handling, but it means that $my_url cannot be used
-	# as base URL.
-	# Therefore, if we needed to strip PATH_INFO, then we know that we have
-	# to build the base URL ourselves:
-	our $path_info = $ENV{"PATH_INFO"};
-	if ($path_info) {
-		if ($my_url =~ s,\Q$path_info\E$,, &&
-		    $my_uri =~ s,\Q$path_info\E$,, &&
-		    defined $ENV{'SCRIPT_NAME'}) {
-			$base_url = $cgi->url(-base => 1) . $ENV{'SCRIPT_NAME'};
-		}
-	}
-
-	# target of the home link on top of all pages
-	our $home_link = $my_uri || "/";
-}
-
 # core git executable to use
 # this can just be "git" if your webserver has a sensible PATH
 $GIT = "++GIT_BINDIR++/git";
@@ -132,7 +97,6 @@ sub gitweb_get_feature {
 		$feature{$name}{'override'},
 		@{$feature{$name}{'default'}});
 	# project specific override is possible only if we have project
-	our $git_dir; # global variable, declared later
 	if (!$override || !defined $git_dir) {
 		return @defaults;
 	}
@@ -261,42 +225,6 @@ sub check_loadavg {
 # ======================================================================
 # input validation and dispatch
 
-# input parameters can be collected from a variety of sources (presently, CGI
-# and PATH_INFO), so we define an %input_params hash that collects them all
-# together during validation: this allows subsequent uses (e.g. href()) to be
-# agnostic of the parameter origin
-
-our %input_params = ();
-
-# input parameters are stored with the long parameter name as key. This will
-# also be used in the href subroutine to convert parameters to their CGI
-# equivalent, and since the href() usage is the most frequent one, we store
-# the name -> CGI key mapping here, instead of the reverse.
-#
-# XXX: Warning: If you touch this, check the search form for updating,
-# too.
-
-our @cgi_param_mapping = (
-	project => "p",
-	action => "a",
-	file_name => "f",
-	file_parent => "fp",
-	hash => "h",
-	hash_parent => "hp",
-	hash_base => "hb",
-	hash_parent_base => "hpb",
-	page => "pg",
-	order => "o",
-	searchtext => "s",
-	searchtype => "st",
-	snapshot_format => "sf",
-	extra_options => "opt",
-	search_use_regexp => "sr",
-	# this must be last entry (for manipulation from JavaScript)
-	javascript => "js"
-);
-our %cgi_param_mapping = @cgi_param_mapping;
-
 # we will also need to know the possible actions, for validation
 our %actions = (
 	"blame" => \&git_blame,
@@ -332,27 +260,6 @@ our %actions = (
 	"project_index" => \&git_project_index,
 );
 
-# finally, we have the hash of allowed extra_options for the commands that
-# allow them
-our %allowed_options = (
-	"--no-merges" => [ qw(rss atom log shortlog history) ],
-);
-
-# fill %input_params with the CGI parameters. All values except for 'opt'
-# should be single values, but opt can be an array. We should probably
-# build an array of parameters that can be multi-valued, but since for the time
-# being it's only this one, we just single it out
-sub evaluate_query_params {
-	our $cgi;
-
-	while (my ($name, $symbol) = each %cgi_param_mapping) {
-		if ($symbol eq 'opt') {
-			$input_params{$name} = [ $cgi->param($symbol) ];
-		} else {
-			$input_params{$name} = $cgi->param($symbol);
-		}
-	}
-}
 
 # now read PATH_INFO and update the parameter list for missing parameters
 sub evaluate_path_info {
@@ -498,11 +405,8 @@ sub evaluate_path_info {
 	}
 }
 
-our ($action, $project, $file_name, $file_parent, $hash, $hash_parent, $hash_base,
-     $hash_parent_base, @extra_options, $page, $searchtype, $search_use_regexp,
-     $searchtext, $search_regexp);
 sub evaluate_and_validate_params {
-	our $action = $input_params{'action'};
+	$action = $input_params{'action'};
 	if (defined $action) {
 		if (!validate_action($action)) {
 			die_error(400, "Invalid action parameter");
@@ -510,7 +414,7 @@ sub evaluate_and_validate_params {
 	}
 
 	# parameters which are pathnames
-	our $project = $input_params{'project'};
+	$project = $input_params{'project'};
 	if (defined $project) {
 		if (!validate_project($project)) {
 			undef $project;
@@ -518,14 +422,14 @@ sub evaluate_and_validate_params {
 		}
 	}
 
-	our $file_name = $input_params{'file_name'};
+	$file_name = $input_params{'file_name'};
 	if (defined $file_name) {
 		if (!validate_pathname($file_name)) {
 			die_error(400, "Invalid file parameter");
 		}
 	}
 
-	our $file_parent = $input_params{'file_parent'};
+	$file_parent = $input_params{'file_parent'};
 	if (defined $file_parent) {
 		if (!validate_pathname($file_parent)) {
 			die_error(400, "Invalid file parent parameter");
@@ -533,28 +437,28 @@ sub evaluate_and_validate_params {
 	}
 
 	# parameters which are refnames
-	our $hash = $input_params{'hash'};
+	$hash = $input_params{'hash'};
 	if (defined $hash) {
 		if (!validate_refname($hash)) {
 			die_error(400, "Invalid hash parameter");
 		}
 	}
 
-	our $hash_parent = $input_params{'hash_parent'};
+	$hash_parent = $input_params{'hash_parent'};
 	if (defined $hash_parent) {
 		if (!validate_refname($hash_parent)) {
 			die_error(400, "Invalid hash parent parameter");
 		}
 	}
 
-	our $hash_base = $input_params{'hash_base'};
+	$hash_base = $input_params{'hash_base'};
 	if (defined $hash_base) {
 		if (!validate_refname($hash_base)) {
 			die_error(400, "Invalid hash base parameter");
 		}
 	}
 
-	our @extra_options = @{$input_params{'extra_options'}};
+	@extra_options = @{$input_params{'extra_options'}};
 	# @extra_options is always defined, since it can only be (currently) set from
 	# CGI, and $cgi->param() returns the empty array in array context if the param
 	# is not set
@@ -567,7 +471,7 @@ sub evaluate_and_validate_params {
 		}
 	}
 
-	our $hash_parent_base = $input_params{'hash_parent_base'};
+	$hash_parent_base = $input_params{'hash_parent_base'};
 	if (defined $hash_parent_base) {
 		if (!validate_refname($hash_parent_base)) {
 			die_error(400, "Invalid hash parent base parameter");
@@ -575,24 +479,23 @@ sub evaluate_and_validate_params {
 	}
 
 	# other parameters
-	our $page = $input_params{'page'};
+	$page = $input_params{'page'};
 	if (defined $page) {
 		if ($page =~ m/[^0-9]/) {
 			die_error(400, "Invalid page parameter");
 		}
 	}
 
-	our $searchtype = $input_params{'searchtype'};
+	$searchtype = $input_params{'searchtype'};
 	if (defined $searchtype) {
 		if ($searchtype =~ m/[^a-z]/) {
 			die_error(400, "Invalid searchtype parameter");
 		}
 	}
 
-	our $search_use_regexp = $input_params{'search_use_regexp'};
+	$search_use_regexp = $input_params{'search_use_regexp'};
 
-	our $searchtext = $input_params{'searchtext'};
-	our $search_regexp;
+	$searchtext = $input_params{'searchtext'};
 	if (defined $searchtext) {
 		if (length($searchtext) < 2) {
 			die_error(403, "At least two characters are required for search parameter");
@@ -601,12 +504,6 @@ sub evaluate_and_validate_params {
 	}
 }
 
-# path to the current git repository
-our $git_dir;
-sub evaluate_git_dir {
-	our $git_dir = "$projectroot/$project" if $project;
-}
-
 our (@snapshot_fmts, $git_avatar);
 sub configure_gitweb_features {
 	# list of supported snapshot formats
@@ -690,7 +587,6 @@ sub run_request {
 our $is_last_request = sub { 1 };
 our ($pre_dispatch_hook, $post_dispatch_hook, $pre_listen_hook);
 our $CGI = 'CGI';
-our $cgi;
 sub evaluate_argv {
 	return unless (@ARGV);
 
@@ -885,39 +781,6 @@ sub validate_project {
 	}
 }
 
-sub validate_pathname {
-	my $input = shift || return undef;
-
-	# no '.' or '..' as elements of path, i.e. no '.' nor '..'
-	# at the beginning, at the end, and between slashes.
-	# also this catches doubled slashes
-	if ($input =~ m!(^|/)(|\.|\.\.)(/|$)!) {
-		return undef;
-	}
-	# no null characters
-	if ($input =~ m!\0!) {
-		return undef;
-	}
-	return $input;
-}
-
-sub validate_refname {
-	my $input = shift || return undef;
-
-	# textual hashes are O.K.
-	if ($input =~ m/^[0-9a-fA-F]{40}$/) {
-		return $input;
-	}
-	# it must be correct pathname
-	$input = validate_pathname($input)
-		or return undef;
-	# restrictions on ref name according to git-check-ref-format
-	if ($input =~ m!(/\.|\.\.|[\000-\040\177 ~^:?*\[]|/$)!) {
-		return undef;
-	}
-	return $input;
-}
-
 # decode sequences of octets in utf8 into Perl's internal form,
 # which is utf-8 with utf8 flag set if needed.  gitweb writes out
 # in utf-8 thanks to "binmode STDOUT, ':utf8'" at beginning
diff --git a/gitweb/lib/Gitweb/Request.pm b/gitweb/lib/Gitweb/Request.pm
new file mode 100644
index 0000000..474e89d
--- /dev/null
+++ b/gitweb/lib/Gitweb/Request.pm
@@ -0,0 +1,156 @@
+#!/usr/bin/perl
+#
+# Gitweb::Request -- gitweb request(cgi) package
+#
+# This program is licensed under the GPLv2
+
+package Gitweb::Request;
+
+use strict;
+use warnings;
+use Exporter qw(import);
+
+our @ISA = qw(Exporter);
+our @EXPORT = qw($cgi $my_url $my_uri $base_url $path_info $home_link $action $project $file_name
+                 $file_parent $hash $hash_parent $hash_base $hash_parent_base @extra_options $page
+                 $git_dir $searchtype $search_use_regexp $searchtext $search_regexp %input_params
+                 @cgi_param_mapping %cgi_param_mapping %allowed_options &evaluate_query_params
+                 &evaluate_uri &evaluate_git_dir &validate_pathname &validate_refname);
+
+use Gitweb::Config qw($projectroot);
+
+our ($cgi, $my_url, $my_uri, $base_url, $path_info, $home_link);
+our ($action, $project, $file_name, $file_parent, $hash, $hash_parent, $hash_base,
+     $hash_parent_base, @extra_options, $page, $git_dir);
+our ($searchtype, $search_use_regexp, $searchtext, $search_regexp);
+
+# input parameters can be collected from a variety of sources (presently, CGI
+# and PATH_INFO), so we define an %input_params hash that collects them all
+# together during validation: this allows subsequent uses (e.g. href()) to be
+# agnostic of the parameter origin
+
+our %input_params = ();
+
+# input parameters are stored with the long parameter name as key. This will
+# also be used in the href subroutine to convert parameters to their CGI
+# equivalent, and since the href() usage is the most frequent one, we store
+# the name -> CGI key mapping here, instead of the reverse.
+#
+# XXX: Warning: If you touch this, check the search form for updating,
+# too.
+
+our @cgi_param_mapping = (
+	project => "p",
+	action => "a",
+	file_name => "f",
+	file_parent => "fp",
+	hash => "h",
+	hash_parent => "hp",
+	hash_base => "hb",
+	hash_parent_base => "hpb",
+	page => "pg",
+	order => "o",
+	searchtext => "s",
+	searchtype => "st",
+	snapshot_format => "sf",
+	extra_options => "opt",
+	search_use_regexp => "sr",
+	# this must be last entry (for manipulation from JavaScript)
+	javascript => "js"
+);
+our %cgi_param_mapping = @cgi_param_mapping;
+
+# finally, we have the hash of allowed extra_options for the commands that
+# allow them
+our %allowed_options = (
+	"--no-merges" => [ qw(rss atom log shortlog history) ],
+);
+
+# fill %input_params with the CGI parameters. All values except for 'opt'
+# should be single values, but opt can be an array. We should probably
+# build an array of parameters that can be multi-valued, but since for the time
+# being it's only this one, we just single it out
+sub evaluate_query_params {
+	while (my ($name, $symbol) = each %cgi_param_mapping) {
+		if ($symbol eq 'opt') {
+			$input_params{$name} = [ $cgi->param($symbol) ];
+		} else {
+			$input_params{$name} = $cgi->param($symbol);
+		}
+	}
+}
+
+sub evaluate_uri {
+	our $cgi;
+
+	our $my_url = $cgi->url();
+	our $my_uri = $cgi->url(-absolute => 1);
+
+	# Base URL for relative URLs in gitweb ($Gitweb::Config::logo, $Gitweb::Config::favicon, ...),
+	# needed and used only for URLs with nonempty PATH_INFO
+	our $base_url = $my_url;
+
+	# When the script is used as DirectoryIndex, the URL does not contain the name
+	# of the script file itself, and $cgi->url() fails to strip PATH_INFO, so we
+	# have to do it ourselves. We make $path_info global because it's also used
+	# later on.
+	#
+	# Another issue with the script being the DirectoryIndex is that the resulting
+	# $my_url data is not the full script URL: this is good, because we want
+	# generated links to keep implying the script name if it wasn't explicitly
+	# indicated in the URL we're handling, but it means that $my_url cannot be used
+	# as base URL.
+	# Therefore, if we needed to strip PATH_INFO, then we know that we have
+	# to build the base URL ourselves:
+	our $path_info = $ENV{"PATH_INFO"};
+	if ($path_info) {
+		if ($my_url =~ s,\Q$path_info\E$,, &&
+		    $my_uri =~ s,\Q$path_info\E$,, &&
+		    defined $ENV{'SCRIPT_NAME'}) {
+			$base_url = $cgi->url(-base => 1) . $ENV{'SCRIPT_NAME'};
+		}
+	}
+
+	# target of the home link on top of all pages
+	our $home_link = $my_uri || "/";
+}
+
+sub evaluate_git_dir {
+	$git_dir = "$projectroot/$project" if $project;
+}
+
+
+sub validate_pathname {
+	my $input = shift || return undef;
+
+	# no '.' or '..' as elements of path, i.e. no '.' nor '..'
+	# at the beginning, at the end, and between slashes.
+	# also this catches doubled slashes
+	if ($input =~ m!(^|/)(|\.|\.\.)(/|$)!) {
+		return undef;
+	}
+	# no null characters
+	if ($input =~ m!\0!) {
+		return undef;
+	}
+	return $input;
+}
+
+sub validate_refname {
+	my $input = shift || return undef;
+
+	# textual hashes are O.K.
+	if ($input =~ m/^[0-9a-fA-F]{40}$/) {
+		return $input;
+	}
+	# it must be correct pathname
+	$input = validate_pathname($input)
+		or return undef;
+	# restrictions on ref name according to git-check-ref-format
+	if ($input =~ m!(/\.|\.\.|[\000-\040\177 ~^:?*\[]|/$)!) {
+		return undef;
+	}
+	return $input;
+}
+
+1;
-- 
1.7.1.447.gfddfb
Pavan Kumar Sunkara· Jun 3, 2010, 13:55 UTC · re: Pavan Kumar Sunkara · lore

[PATCH 3/3] git-instaweb: Add support for --reuse-config using gitconfig

Support instaweb's --reuse-config using instaweb.overwrite variable in gitconfig.

To use --reuse-config always
	[instaweb]
		overwrite = false
Signed-off-by: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>
---

This patch is based on branch 'pu' and my previous patch http://thread.gmane.org/gmane.comp.version-control.git/148161

 Documentation/git-instaweb.txt |    2 ++
 git-instaweb.sh                |    5 ++++-
 2 files changed, 6 insertions(+), 1 deletions(-)
Show changes to 2 files +6 −1

Documentation/git-instaweb.txt, git-instaweb.sh

diff --git a/Documentation/git-instaweb.txt b/Documentation/git-instaweb.txt
index 0e7e20b..12cbe1d 100644
--- a/Documentation/git-instaweb.txt
+++ b/Documentation/git-instaweb.txt
@@ -77,6 +77,8 @@ You may specify configuration in your .git/config
 	port = 4321
 	browser = konqueror
 	modulepath = /usr/lib/apache2/modules
+	gitwebdir = /usr/share/gitweb
+	overwrite = false
 
 -----------------------------------------------------------------------
 
diff --git a/git-instaweb.sh b/git-instaweb.sh
index 1c704a3..3635974 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -28,7 +28,7 @@ httpd="$(git config --get instaweb.httpd)"
 root="$(git config --get instaweb.gitwebdir)"
 port=$(git config --get instaweb.port)
 module_path="$(git config --get instaweb.modulepath)"
-no_reuse=true
+no_reuse="$(git config --bool --get instaweb.overwrite)"
 
 conf="$GIT_DIR/gitweb/httpd.conf"
 
@@ -43,6 +43,9 @@ test -z "$root" && root='@@GITWEBDIR@@'
 # any untaken local port will do...
 test -z "$port" && port=1234
 
+# Default is true -> overwrite gitweb_config.perl
+test -z "$no_reuse" && no_reuse=true
+
 resolve_full_httpd () {
 	case "$httpd" in
 	*apache2*|*lighttpd*)
-- 
1.7.1.447.gfddfb
Petr Baudis· Jun 3, 2010, 15:20 UTC · re: Pavan Kumar Sunkara · lore

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

  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?

Show 6 quoted lines
> +# __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.

Show 18 quoted lines
> +
> +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.
Show 5 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!)

-- 
				Petr "Pasky" Baudis
The true meaning of life is to plant a tree under whose shade
you will never sit.
Ævar Arnfjörð Bjarmason· Jun 3, 2010, 15:54 UTC · re: Petr Baudis · lore

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

On Thu, Jun 3, 2010 at 15:20, Petr Baudis <pasky@suse.cz> wrote:
> On Thu, Jun 03, 2010 at 07:25:54PM +0530, Pavan Kumar Sunkara wrote:
Show 6 quoted lines
>> -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?
>> +our @ISA = qw(Exporter);

This is also re-arranging deck chairs on the Titanic, but 'use base qw(Exporter)' is nicer.

Show 9 quoted lines
>> +# __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.

No, those don't do the same thing as discussed in previous reviews. FindBin finds the invoked binary, Dir::Self finds the the current file.

Jakub Narebski· Jun 3, 2010, 16:59 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason wrote:
Show 7 quoted lines
> On Thu, Jun 3, 2010 at 15:20, Petr Baudis <pasky@suse.cz> wrote:
>> On Thu, Jun 03, 2010 at 07:25:54PM +0530, Pavan Kumar Sunkara wrote:
>>>
>>> +our @ISA = qw(Exporter);
> 
> This is also re-arranging deck chairs on the Titanic, but 'use base
> qw(Exporter)' is nicer.

Or simply 'use Exporter qw(import);', as Perl 5.8+ supports 'use Module LIST' form.

-- 
Jakub Narebski
Poland
Ævar Arnfjörð Bjarmason· Jun 3, 2010, 17:04 UTC · re: Jakub Narebski · lore

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

On Thu, Jun 3, 2010 at 16:59, Jakub Narebski <jnareb@gmail.com> wrote:
Show 11 quoted lines
> Ævar Arnfjörð Bjarmason wrote:
>> On Thu, Jun 3, 2010 at 15:20, Petr Baudis <pasky@suse.cz> wrote:
>>> On Thu, Jun 03, 2010 at 07:25:54PM +0530, Pavan Kumar Sunkara wrote:
>>>>
>>>> +our @ISA = qw(Exporter);
>>
>> This is also re-arranging deck chairs on the Titanic, but 'use base
>> qw(Exporter)' is nicer.
>
> Or simply 'use Exporter qw(import);', as Perl 5.8+ supports
> 'use Module LIST' form.

Ah yes, if the code is lucky enough to require 5.8 in the first place. I didn't know whether gitweb was one of those things.

Jakub Narebski· Jun 3, 2010, 15:55 UTC · re: Petr Baudis · lore

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

On Tue, 3 Jun 2010, Petr Baudis wrote:
Show 12 quoted lines
>
>   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.

Show 9 quoted lines
> > +# __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).

Show 10 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).

Show 21 quoted lines
> >  
> >  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";
Show 7 quoted lines
> 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.
Show 9 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.

-- 
Jakub Narebski
Poland
Petr Baudis· Jun 3, 2010, 16:06 UTC · re: Jakub Narebski · lore

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.

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.
Pavan Kumar Sunkara· Jun 3, 2010, 16:11 UTC · re: Jakub Narebski · lore

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

2010/6/3 Jakub Narebski <jnareb@gmail.com>:
Show 18 quoted lines
> 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.
Ok. It will be done.
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).
Ok. Will change it.
Show 37 quoted lines
>> >
>> >  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";

But, I think when we start reading the code, it would seem that 'our $GIT' implies that it is a variable created locally rather than an exported variable from Gitweb::Config module.

Even though it increases the patch size, I don't think it will be much of a concern when it comes to good redability of code.

Jakub: Can you reply, what you think about this argument ?

Thanks, Pavan.

Jakub Narebski· Jun 3, 2010, 18:43 UTC · re: Pavan Kumar Sunkara · lore

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

Pavan Kumar Sunkara wrote:
> 2010/6/3 Jakub Narebski <jnareb@gmail.com>:
>> On Tue, 3 Jun 2010, Petr Baudis wrote:
Show 18 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).
> 
> Ok. Will change it.
[...]
Show 7 quoted lines
>> I would say
>>
>>        our $GIT = "++GIT_BINDIR++/git";
> 
> But, I think when we start reading the code, it would seem that 'our
> $GIT' implies that it is a variable created locally rather than an
> exported variable from Gitweb::Config module.
From `perldoc -f our`:
  An "our" declares the listed variables to be valid globals within the
  enclosing block, file, or "eval".  That is, it has the same scoping
  rules as a "my" declaration, but does not create a local variable.
  [...] The package in which the variable is entered is determined at
  the point of the declaration, [...]

So if the variable already exists in given scope, for example if the variable was imported from other package, the 'our' declaration would be a no-op.

Show 5 quoted lines
> 
> Even though it increases the patch size, I don't think it will be much
> of a concern when it comes to good redability of code.
> 
> Jakub: Can you reply, what you think about this argument ?

But I agree that first, 'our $var' seems to imply that it is _new_ variable declared in current scope, and second if we make a typo in variable name it wouldn't be detected as different from exported variable: 'our' will create new variable.

So I agree that removing 'our' is a good idea, especially together with putting all variables that should be there in Gitweb::Config together with comments, even if they are configured during build process.

Perhaps those declarations in Gitweb::Config should have in-line comment that they are defined in gitweb.cgi / gitweb.perl?

-- 
Jakub Narebski
Poland
Pavan Kumar Sunkara· Jun 3, 2010, 18:50 UTC · re: Jakub Narebski · lore

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

On Fri, Jun 4, 2010 at 12:13 AM, Jakub Narebski <jnareb@gmail.com> wrote:
Show 11 quoted lines
> Pavan Kumar Sunkara wrote:
>>
>> Even though it increases the patch size, I don't think it will be much
>> of a concern when it comes to good redability of code.
>>
>> Jakub: Can you reply, what you think about this argument ?
>
> But I agree that first, 'our $var' seems to imply that it is _new_
> variable declared in current scope, and second if we make a typo in
> variable name it wouldn't be detected as different from exported
> variable: 'our' will create new variable.
And it's hard to detect the typo while debugging.
Show 8 quoted lines
> So I agree that removing 'our' is a good idea, especially together
> with putting all variables that should be there in Gitweb::Config
> together with comments, even if they are configured during build
> process.
>
> Perhaps those declarations in Gitweb::Config should have in-line
> comment that they are defined in gitweb.cgi / gitweb.perl?
>
Yeah, Sure.

Thanks, Pavan.

Ævar Arnfjörð Bjarmason· Jun 3, 2010, 16:00 UTC · re: Pavan Kumar Sunkara · lore

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

On Thu, Jun 3, 2010 at 13:55, Pavan Kumar Sunkara <pavan.sss1991@gmail.com> wrote:

Show 10 quoted lines
> +sub evaluate_gitweb_config {
> +       # die if there are errors parsing config file
> +       if (-e $GITWEB_CONFIG) {
> +               do $GITWEB_CONFIG;
> +               die $@ if $@;
> +       } elsif (-e $GITWEB_CONFIG_SYSTEM) {
> +               do $GITWEB_CONFIG_SYSTEM;
> +               die $@ if $@;
> +       }
> +}

I think I mentioned this before, but why not *optionally* use Config::Any (or something similar) and if it doesn't exists fall back on do(), and document this, along with a way to disable Perl execution.

It'd be completely compatible, but admins could then allow someone to edit a gitweb config file without opening themselves up to that someone having permission to execute code as the webserver.

Check out Gitalist (the Catalyst rewrite of Gitweb) for some prior art.

← back to recent threads