{"thread":{"id":"23985","subject":"About [PATCH] gitweb: Create a perl module to store gitweb configuration","startedAt":"2010-06-02T20:29:30Z","lastAt":"2010-06-03T10:23:31Z","messageCount":6,"participants":["Jakub Narebski","Pavan Kumar Sunkara"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"142837","messageId":"201006022229.31593.jnareb@gmail.com","threadId":"23985","inReplyTo":null,"subject":"About [PATCH] gitweb: Create a perl module to store gitweb configuration","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-06-02T20:29:30Z","receivedAt":"2010-06-02T20:29:30Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"This comment is about commit 9526ab8 (gitweb: Create a perl module to\nstore gitweb configuration, 2010-06-01) on 'master' branch of\nrepository shown at http://repo.or.cz/w/git/gsoc2010-gitweb.git\n\n> From 9526ab8a12c8d4d875d08a6201c7eca963c7d9aa Mon Sep 17 00:00:00 2001\n> From: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>\n> Date: Tue, 1 Jun 2010 02:36:48 +0530\n> Subject: [PATCH] gitweb: Create a perl module to store gitweb configuration\n\nI would probably say\n\n  Subject: [PATCH] gitweb: Create Gitweb::Config module to store gitweb configuration\n\nor even\n\n  Subject: [PATCH] gitweb: Create Gitweb::Config module\n\nso that _name_ of this module is in commit subject (summary).\n\n> \n> Create a perl module in path gitweb/lib/Gitweb/Config.pm\n> to store all the configuration variables of the gitweb.perl\n> script and change the scope of those variables in main script.\n\nA bit overlong sentence, isn't it?  Perhaps\n\n  Create a Gitweb::Config module in 'gitweb/lib/Gitweb/Config.pm'\n  to store all the configuration variables of the gitweb.perl\n  script.\n\n(Assuming that I can convince you to avoid fully qualified names).\n\n> \n> Move subroutine evaluate_gitweb_config() into the same module\n> so that the statement 'do GITWEB_CONFIG' or 'do GITWEB_CONFIG_SYSTEM'\n> works similiar to the working before the split.\n\ns/similiar/similar/\n\nI agree that you should list which subroutines you do move to\nGitweb::Config package, though this list would be probably longer that\nthe evaluate_gitweb_config() alone... or perhaps not.\n\nBut I do not understand the statement about \"do $GITWEB_CONFIG\"\netc. here.  Did you wanted to say that setting $GITWEB_CONFIG etc. was\nmoved out of evaluate_gitweb_config(), and is left in gitweb.perl?\n\n> \n> Change Makefile accordingly to install the Perl Module.\n\nI would say:\n\n  Update gitweb/Makefile to install gitweb modules alongside gitweb.\n\nOr something like that.\n\n> \n> Signed-off-by: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>\n> ---\n\nWhen sending this patch to git mailing list you should mention that it\nis based on\n  gitweb: Put all per-connection code in run() subroutine\nand perhaps also on\n  gitweb: Add support for FastCGI, using CGI::Fast\nunless those two commits made it into 'master' till then.\n\n>  gitweb/Makefile             |    6 +\n>  gitweb/gitweb.perl          |  692 ++++++++++---------------------------------\n>  gitweb/lib/Gitweb/Config.pm |  389 ++++++++++++++++++++++++\n>  3 files changed, 558 insertions(+), 529 deletions(-)\n>  create mode 100644 gitweb/lib/Gitweb/Config.pm\n> \n> diff --git a/gitweb/Makefile b/gitweb/Makefile\n> index d2584fe..45e176e 100644\n> --- a/gitweb/Makefile\n> +++ b/gitweb/Makefile\n> @@ -55,6 +55,7 @@ PERL_PATH  ?= /usr/bin/perl\n>  bindir_SQ = $(subst ','\\'',$(bindir))#'\n>  gitwebdir_SQ = $(subst ','\\'',$(gitwebdir))#'\n>  gitwebstaticdir_SQ = $(subst ','\\'',$(gitwebdir)/static)#'\n> +gitweblibdir_SQ = $(subst ','\\'',$(gitwebdir)/lib)#'\n\nI think it would be good idea to have 'gitweblibdir' as a separate\nvariable, alongside 'gitwebdir', and which would default to\n\n   gitweblibdir = $(gitwebdir)/lib\n\nto make it possible to install gitweb modules not alongside gitweb,\nbut somewhere else, for example together with other Perl modules.\n\nThen you would have:\n\n  +gitweblibdir_SQ = $(subst ','\\'',$(gitweblibdir))#'\n\nBut I think this change can be left for a separate commit.  It is not\nsomething terribly important, something blocking accepting the patch.\n\n\nBTW. did you start working on the 'write' part yet, at least on the\nconceptual (specification / architecture) level?\n\n>  SHELL_PATH_SQ = $(subst ','\\'',$(SHELL_PATH))#'\n>  PERL_PATH_SQ  = $(subst ','\\'',$(PERL_PATH))#'\n>  DESTDIR_SQ    = $(subst ','\\'',$(DESTDIR))#'\n> @@ -110,6 +111,9 @@ endif\n>  \n>  GITWEB_FILES += static/git-logo.png static/git-favicon.png\n>  \n> +# Files: gitweb/lib/Gitweb\n> +GITWEB_LIB_GITWEB += lib/Gitweb/Config.pm\n> +\n>  GITWEB_REPLACE = \\\n>  \t-e 's|++GIT_VERSION++|$(GIT_VERSION)|g' \\\n>  \t-e 's|++GIT_BINDIR++|$(bindir)|g' \\\n> @@ -150,6 +154,8 @@ install: all\n>  \t$(INSTALL) -m 755 $(GITWEB_PROGRAMS) '$(DESTDIR_SQ)$(gitwebdir_SQ)'\n>  \t$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitwebstaticdir_SQ)'\n>  \t$(INSTALL) -m 644 $(GITWEB_FILES) '$(DESTDIR_SQ)$(gitwebstaticdir_SQ)'\n> +\t$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitweblibdir_SQ)/Gitweb'\n> +\t$(INSTALL) -m 644 $(GITWEB_LIB_GITWEB) '$(DESTDIR_SQ)$(gitweblibdir_SQ)/Gitweb'\n\nUhhh... this would probably get more complicated *if* there would be\nmore complicated hierarchy, e.g. if there would be Gitweb module in\nlib/Gitweb.pm.  I guess that is the reason behind $(GITWEB_LIB_GITWEB)\nname, isn't it?\n\n>  \n>  ### Cleaning rules\n>  \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 673e7a3..98a85f4 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -16,19 +16,22 @@ use Encode;\n>  use Fcntl ':mode';\n>  use File::Find qw();\n>  use File::Basename qw(basename);\n> +use File::Spec;\n>  binmode STDOUT, ':utf8';\n> \n\nThe fragment below was slightly edited to make it more clear.\n\n> -our $t0;\n> -if (eval { require Time::HiRes; 1; }) {\n> -\t$t0 = [Time::HiRes::gettimeofday()];\n> -}\n> -our $number_of_git_cmds = 0;\n\nTheoretically neither $t0 nor $number_of_git_cmds belong to\nGitweb::Config, as those are about runtime timing, not about gitweb\nconfiguration.\n\nBut perhaps for the time being you can put it in Gitweb::Config.\n\n> +# __DIR__ is taken from Dir::Self __DIR__ fragment\n> +sub __DIR__ () {\n> +\tFile::Spec->rel2abs(join '', (File::Spec->splitpath(__FILE__))[0, 1]);\n> +}\n> +use lib __DIR__ . \"/lib\";\n> +\n> +use Gitweb::Config;\n\nO.K.\n\nIt's a pity that Dir::Self is not a core Perl module since Perl 5.8,\nand that FindBin which is in core since 5.4 has its quirks and\nnowadays is not recommended to use...\n\n>  \n>  BEGIN {\n>  \tCGI->compile() if $ENV{'MOD_PERL'};\n>  }\n>  \n> -our $version = \"++GIT_VERSION++\";\n> +$Gitweb::Config::version = \"++GIT_VERSION++\";\n\nIf '$version' was exported by Gitweb::Config this change would be not\nnecessary.\n\nI would remove all such changes from discussion of this patch.\n\n> -# URI and label (title) of GIT logo link\n> -#our $logo_url = \"http://www.kernel.org/pub/software/scm/git/docs/\";\n> -#our $logo_label = \"git documentation\";\n> -our $logo_url = \"http://git-scm.com/\";\n> -our $logo_label = \"git homepage\";\n\nAll right, those variables that do not need initialization using\nbuild-time substitutions are put together with their description in\nGitweb::Config.\n\nI would remove all such changes from discussion of this patch.\n\n>  sub gitweb_get_feature {\n>  \tmy ($name) = @_;\n[...]\n\nI wonder if this subroutine, and its companion 'gitweb_check_feature'\nshould be not moved to Gitweb::Config module.\n\n> -our ($GITWEB_CONFIG, $GITWEB_CONFIG_SYSTEM);\n> +our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || \"++GITWEB_CONFIG++\";\n> +our $GITWEB_CONFIG_SYSTEM = $ENV{'GITWEB_CONFIG_SYSTEM'} || \"++GITWEB_CONFIG_SYSTEM++\";\n\nThis would probably look like this, because $GITWEB_CONFIG must be set\nin gitweb.perl -> gitweb.cgi, and not in Gitweb::Config, where\nevaluate_gitweb_config() would be.\n\n> -sub evaluate_gitweb_config {\n> -\tour $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || \"++GITWEB_CONFIG++\";\n> -\tour $GITWEB_CONFIG_SYSTEM = $ENV{'GITWEB_CONFIG_SYSTEM'} || \"++GITWEB_CONFIG_SYSTEM++\";\n> -\t# die if there are errors parsing config file\n> -\tif (-e $GITWEB_CONFIG) {\n> -\t\tdo $GITWEB_CONFIG;\n> -\t\tdie $@ if $@;\n> -\t} elsif (-e $GITWEB_CONFIG_SYSTEM) {\n> -\t\tdo $GITWEB_CONFIG_SYSTEM;\n> -\t\tdie $@ if $@;\n> -\t}\n> -}\n\nRight, this got moved to Gitweb::Config.\n\n> -our ($action, $project, $file_name, $file_parent, $hash, $hash_parent, $hash_base,\n> -     $hash_parent_base, @extra_options, $page, $searchtype, $search_use_regexp,\n> -     $searchtext, $search_regexp);\n>  sub evaluate_and_validate_params {\n>  \tour $action = $input_params{'action'};\n>  \tif (defined $action) {\n\nHmmm... it looks like those got also moved to Gitweb::Config, even if\nthose variables have nothing to do with gitweb configuration, but are\nabout request, and therefore belong to Gitweb::Request.  Thus this\ncommit should probably not touch this.\n\n> @@ -963,10 +602,8 @@ sub evaluate_and_validate_params {\n>  \t}\n>  }\n>  \n> -# path to the current git repository\n> -our $git_dir;\n>  sub evaluate_git_dir {\n>  \tour $git_dir = \"$projectroot/$project\" if $project;\n>  }\n\nSame with $git_dir -- it is request dependent, not configuration\ndependent.\n\n[very large cut, not necessary with exporting variables]\n\n> diff --git a/gitweb/lib/Gitweb/Config.pm b/gitweb/lib/Gitweb/Config.pm\n> new file mode 100644\n> index 0000000..e1bd06f\n> --- /dev/null\n> +++ b/gitweb/lib/Gitweb/Config.pm\n> @@ -0,0 +1,389 @@\n> +#!/usr/bin/perl\n> +#\n> +# Gitweb::Config -- gitweb configuration package\n> +#\n> +# This program is licensed under the GPLv2\n> +\n> +package Gitweb::Config;\n> +\n> +use strict;\n> +\n> +BEGIN {\n> +\tuse Exporter();\n> +\n> +\t@Gitweb::Config::ISA = qw(Exporter);\t\n                                            ^^^^\n\nTrailing whitespace.  Didn't git warn you about this?\n\n> +\t@Gitweb::Config::EXPORT = qw();\n> +}\n\nWhy BEGIN block?  Why 'use Exporter();' and not 'use Exporter;'?\n\nWhy not\n\n   use Exporter;\n   use base 'Exporter';\n\nor even\n\n   use Exporter qw(import);\n\n(we can use the last version, because gitweb requires high enough Perl\nversion itself, so that this form can be used).\n\n> +\n> +our $t0;\n> +if (eval { require Time::HiRes; 1; }) {\n> +\t$t0 = [Time::HiRes::gettimeofday()];\n> +}\n> +our $number_of_git_cmds = 0;\n\nThis does not strictly speaking belong in Gitweb::Config, and probably\nneither in Gitweb::Request, but Gitweb::request is a better place for\nit.\n\nNever-mind, we can move it later.\n\n> +\n\nHere it would be good place for comment that those are variables that\nare affected by build-time configuration, and therefore their\ninitialization is put in gitweb.perl (together with comments with\ntheir description).\n\n> +our ($GIT, $version, $git_version);\n> +our ($projectroot, $project_maxdepth, $projects_list, @git_base_url_list);\n> +our ($export_ok, $strict_export);\n> +our ($home_link_str, $site_name, $site_header, $site_footer, $home_text);\n> +our (@stylesheets, $stylesheet, $logo, $favicon, $javascript);\n> +our ($GITWEB_CONFIG, $GITWEB_CONFIG_SYSTEM);\n> +\n> +# URI and label (title) of GIT logo link\n> +#our $logo_url = \"http://www.kernel.org/pub/software/scm/git/docs/\";\n> +#our $logo_label = \"git documentation\";\n> +our $logo_url = \"http://git-scm.com/\";\n> +our $logo_label = \"git homepage\";\n\n[cut]\n\nNothing especially interesting here.  'git blame -C -C' should detect\ncode movement.\n\n> +\n> +sub evaluate_gitweb_config {\n> +\t# die if there are errors parsing config file\n> +\tif (-e $GITWEB_CONFIG) {\n> +\t\tdo $GITWEB_CONFIG;\n> +\t\tdie $@ if $@;\n> +\t} elsif (-e $GITWEB_CONFIG_SYSTEM) {\n> +\t\tdo $GITWEB_CONFIG_SYSTEM;\n> +\t\tdie $@ if $@;\n> +\t}\n> +}\n\nHere the question is if this would affect interpretation of\n$GITWEB_CONFIG etc. if it is a relative path.  Perhaps nothing will\nchange, because paths are relative to the directory the script is run,\nnot relative to where module resides.\n\nNot that is matter much either way, as at most it would require\nstating in gitweb/README that one should use absolute pathnames.\n\nNote that tests use absolute pathname, so passing test does not answer\nthis.\n\n> +\n> +1;\n\nO.K.\n\nGood work!\n-- \nJakub Narebski\nPoland\n"},{"id":"142857","messageId":"AANLkTin9ZHij7ldIlSHyCqOd8NmU9DQA5q-tyvIImtdD@mail.gmail.com","threadId":"23985","inReplyTo":"201006022229.31593.jnareb@gmail.com","subject":"Re: About [PATCH] gitweb: Create a perl module to store gitweb configuration","fromName":"Pavan Kumar Sunkara","fromEmail":"pavan.sss1991@gmail.com","sentAt":"2010-06-03T05:07:10Z","receivedAt":"2010-06-03T05:07:10Z","isPatch":true,"sender":{"key":"pavan.sss1991@gmail.com","avatar":"https://avatars.githubusercontent.com/u/174703?v=4"},"body":"2010/6/3 Jakub Narebski <jnareb@gmail.com>:\n> This comment is about commit 9526ab8 (gitweb: Create a perl module to\n> store gitweb configuration, 2010-06-01) on 'master' branch of\n> repository shown at http://repo.or.cz/w/git/gsoc2010-gitweb.git\n>\n>> From 9526ab8a12c8d4d875d08a6201c7eca963c7d9aa Mon Sep 17 00:00:00 2001\n>> From: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>\n>> Date: Tue, 1 Jun 2010 02:36:48 +0530\n>> Subject: [PATCH] gitweb: Create a perl module to store gitweb configuration\n>\n> I would probably say\n>\n>  Subject: [PATCH] gitweb: Create Gitweb::Config module to store gitweb configuration\n>\n> or even\n>\n>  Subject: [PATCH] gitweb: Create Gitweb::Config module\n>\n> so that _name_ of this module is in commit subject (summary).\n>\n>>\n>> Create a perl module in path gitweb/lib/Gitweb/Config.pm\n>> to store all the configuration variables of the gitweb.perl\n>> script and change the scope of those variables in main script.\n>\n> A bit overlong sentence, isn't it?  Perhaps\n>\n>  Create a Gitweb::Config module in 'gitweb/lib/Gitweb/Config.pm'\n>  to store all the configuration variables of the gitweb.perl\n>  script.\n>\n> (Assuming that I can convince you to avoid fully qualified names).\n\nOk.\n\n>>\n>> Move subroutine evaluate_gitweb_config() into the same module\n>> so that the statement 'do GITWEB_CONFIG' or 'do GITWEB_CONFIG_SYSTEM'\n>> works similiar to the working before the split.\n>\n> s/similiar/similar/\n>\n> I agree that you should list which subroutines you do move to\n> Gitweb::Config package, though this list would be probably longer that\n> the evaluate_gitweb_config() alone... or perhaps not.\n\nOk.\n\n> But I do not understand the statement about \"do $GITWEB_CONFIG\"\n> etc. here.  Did you wanted to say that setting $GITWEB_CONFIG etc. was\n> moved out of evaluate_gitweb_config(), and is left in gitweb.perl?\n\nNope. I actually wanted to say that any of the previous\ngitweb_config.perl will work without the problems of scoping.\n\n>>\n>> Change Makefile accordingly to install the Perl Module.\n>\n> I would say:\n>\n>  Update gitweb/Makefile to install gitweb modules alongside gitweb.\n>\n> Or something like that.\n\nOk.\n\n>>\n>> Signed-off-by: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>\n>> ---\n>\n> When sending this patch to git mailing list you should mention that it\n> is based on\n>  gitweb: Put all per-connection code in run() subroutine\n> and perhaps also on\n>  gitweb: Add support for FastCGI, using CGI::Fast\n> unless those two commits made it into 'master' till then.\n\nOk.\n\n>>  gitweb/Makefile             |    6 +\n>>  gitweb/gitweb.perl          |  692 ++++++++++---------------------------------\n>>  gitweb/lib/Gitweb/Config.pm |  389 ++++++++++++++++++++++++\n>>  3 files changed, 558 insertions(+), 529 deletions(-)\n>>  create mode 100644 gitweb/lib/Gitweb/Config.pm\n>>\n>> diff --git a/gitweb/Makefile b/gitweb/Makefile\n>> index d2584fe..45e176e 100644\n>> --- a/gitweb/Makefile\n>> +++ b/gitweb/Makefile\n>> @@ -55,6 +55,7 @@ PERL_PATH  ?= /usr/bin/perl\n>>  bindir_SQ = $(subst ','\\'',$(bindir))#'\n>>  gitwebdir_SQ = $(subst ','\\'',$(gitwebdir))#'\n>>  gitwebstaticdir_SQ = $(subst ','\\'',$(gitwebdir)/static)#'\n>> +gitweblibdir_SQ = $(subst ','\\'',$(gitwebdir)/lib)#'\n>\n> I think it would be good idea to have 'gitweblibdir' as a separate\n> variable, alongside 'gitwebdir', and which would default to\n>\n>   gitweblibdir = $(gitwebdir)/lib\n>\n> to make it possible to install gitweb modules not alongside gitweb,\n> but somewhere else, for example together with other Perl modules.\n>\n> Then you would have:\n>\n>  +gitweblibdir_SQ = $(subst ','\\'',$(gitweblibdir))#'\n\nIt's great.\n\n> But I think this change can be left for a separate commit.  It is not\n> something terribly important, something blocking accepting the patch.\n>\n>\n> BTW. did you start working on the 'write' part yet, at least on the\n> conceptual (specification / architecture) level?\n\nYeah. I have already written some of it in python before I applied for\ngsoc. So, I have a good idea over it on the conceptual level.\n\n>>  SHELL_PATH_SQ = $(subst ','\\'',$(SHELL_PATH))#'\n>>  PERL_PATH_SQ  = $(subst ','\\'',$(PERL_PATH))#'\n>>  DESTDIR_SQ    = $(subst ','\\'',$(DESTDIR))#'\n>> @@ -110,6 +111,9 @@ endif\n>>\n>>  GITWEB_FILES += static/git-logo.png static/git-favicon.png\n>>\n>> +# Files: gitweb/lib/Gitweb\n>> +GITWEB_LIB_GITWEB += lib/Gitweb/Config.pm\n>> +\n>>  GITWEB_REPLACE = \\\n>>       -e 's|++GIT_VERSION++|$(GIT_VERSION)|g' \\\n>>       -e 's|++GIT_BINDIR++|$(bindir)|g' \\\n>> @@ -150,6 +154,8 @@ install: all\n>>       $(INSTALL) -m 755 $(GITWEB_PROGRAMS) '$(DESTDIR_SQ)$(gitwebdir_SQ)'\n>>       $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitwebstaticdir_SQ)'\n>>       $(INSTALL) -m 644 $(GITWEB_FILES) '$(DESTDIR_SQ)$(gitwebstaticdir_SQ)'\n>> +     $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitweblibdir_SQ)/Gitweb'\n>> +     $(INSTALL) -m 644 $(GITWEB_LIB_GITWEB) '$(DESTDIR_SQ)$(gitweblibdir_SQ)/Gitweb'\n>\n> Uhhh... this would probably get more complicated *if* there would be\n> more complicated hierarchy, e.g. if there would be Gitweb module in\n> lib/Gitweb.pm.  I guess that is the reason behind $(GITWEB_LIB_GITWEB)\n> name, isn't it?\n\nYes. Exactly.\n\n>>\n>>  ### Cleaning rules\n>>\n>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>> index 673e7a3..98a85f4 100755\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -16,19 +16,22 @@ use Encode;\n>>  use Fcntl ':mode';\n>>  use File::Find qw();\n>>  use File::Basename qw(basename);\n>> +use File::Spec;\n>>  binmode STDOUT, ':utf8';\n>>\n>\n> The fragment below was slightly edited to make it more clear.\n>\n>> -our $t0;\n>> -if (eval { require Time::HiRes; 1; }) {\n>> -     $t0 = [Time::HiRes::gettimeofday()];\n>> -}\n>> -our $number_of_git_cmds = 0;\n>\n> Theoretically neither $t0 nor $number_of_git_cmds belong to\n> Gitweb::Config, as those are about runtime timing, not about gitweb\n> configuration.\n>\n> But perhaps for the time being you can put it in Gitweb::Config.\n\nOk.\n\n>> +# __DIR__ is taken from Dir::Self __DIR__ fragment\n>> +sub __DIR__ () {\n>> +     File::Spec->rel2abs(join '', (File::Spec->splitpath(__FILE__))[0, 1]);\n>> +}\n>> +use lib __DIR__ . \"/lib\";\n>> +\n>> +use Gitweb::Config;\n>\n> O.K.\n>\n> It's a pity that Dir::Self is not a core Perl module since Perl 5.8,\n> and that FindBin which is in core since 5.4 has its quirks and\n> nowadays is not recommended to use...\n>\n>>\n>>  BEGIN {\n>>       CGI->compile() if $ENV{'MOD_PERL'};\n>>  }\n>>\n>> -our $version = \"++GIT_VERSION++\";\n>> +$Gitweb::Config::version = \"++GIT_VERSION++\";\n>\n> If '$version' was exported by Gitweb::Config this change would be not\n> necessary.\n\nYeah. I am currently removing them.\n\n> I would remove all such changes from discussion of this patch.\n>\n>> -# URI and label (title) of GIT logo link\n>> -#our $logo_url = \"http://www.kernel.org/pub/software/scm/git/docs/\";\n>> -#our $logo_label = \"git documentation\";\n>> -our $logo_url = \"http://git-scm.com/\";\n>> -our $logo_label = \"git homepage\";\n>\n> All right, those variables that do not need initialization using\n> build-time substitutions are put together with their description in\n> Gitweb::Config.\n>\n> I would remove all such changes from discussion of this patch.\n>\n>>  sub gitweb_get_feature {\n>>       my ($name) = @_;\n> [...]\n>\n> I wonder if this subroutine, and its companion 'gitweb_check_feature'\n> should be not moved to Gitweb::Config module.\n\nOk. Will do it.\n\n>> -our ($GITWEB_CONFIG, $GITWEB_CONFIG_SYSTEM);\n>> +our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || \"++GITWEB_CONFIG++\";\n>> +our $GITWEB_CONFIG_SYSTEM = $ENV{'GITWEB_CONFIG_SYSTEM'} || \"++GITWEB_CONFIG_SYSTEM++\";\n>\n> This would probably look like this, because $GITWEB_CONFIG must be set\n> in gitweb.perl -> gitweb.cgi, and not in Gitweb::Config, where\n> evaluate_gitweb_config() would be.\n>\n>> -sub evaluate_gitweb_config {\n>> -     our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || \"++GITWEB_CONFIG++\";\n>> -     our $GITWEB_CONFIG_SYSTEM = $ENV{'GITWEB_CONFIG_SYSTEM'} || \"++GITWEB_CONFIG_SYSTEM++\";\n>> -     # die if there are errors parsing config file\n>> -     if (-e $GITWEB_CONFIG) {\n>> -             do $GITWEB_CONFIG;\n>> -             die $@ if $@;\n>> -     } elsif (-e $GITWEB_CONFIG_SYSTEM) {\n>> -             do $GITWEB_CONFIG_SYSTEM;\n>> -             die $@ if $@;\n>> -     }\n>> -}\n>\n> Right, this got moved to Gitweb::Config.\n>\n>> -our ($action, $project, $file_name, $file_parent, $hash, $hash_parent, $hash_base,\n>> -     $hash_parent_base, @extra_options, $page, $searchtype, $search_use_regexp,\n>> -     $searchtext, $search_regexp);\n>>  sub evaluate_and_validate_params {\n>>       our $action = $input_params{'action'};\n>>       if (defined $action) {\n>\n> Hmmm... it looks like those got also moved to Gitweb::Config, even if\n> those variables have nothing to do with gitweb configuration, but are\n> about request, and therefore belong to Gitweb::Request.  Thus this\n> commit should probably not touch this.\n\nSorry. That was a mistake. I probably ammended the commit while\nworking on Gitweb:Request Module.\n\n>> @@ -963,10 +602,8 @@ sub evaluate_and_validate_params {\n>>       }\n>>  }\n>>\n>> -# path to the current git repository\n>> -our $git_dir;\n>>  sub evaluate_git_dir {\n>>       our $git_dir = \"$projectroot/$project\" if $project;\n>>  }\n>\n> Same with $git_dir -- it is request dependent, not configuration\n> dependent.\n>\n> [very large cut, not necessary with exporting variables]\n>\n>> diff --git a/gitweb/lib/Gitweb/Config.pm b/gitweb/lib/Gitweb/Config.pm\n>> new file mode 100644\n>> index 0000000..e1bd06f\n>> --- /dev/null\n>> +++ b/gitweb/lib/Gitweb/Config.pm\n>> @@ -0,0 +1,389 @@\n>> +#!/usr/bin/perl\n>> +#\n>> +# Gitweb::Config -- gitweb configuration package\n>> +#\n>> +# This program is licensed under the GPLv2\n>> +\n>> +package Gitweb::Config;\n>> +\n>> +use strict;\n>> +\n>> +BEGIN {\n>> +     use Exporter();\n>> +\n>> +     @Gitweb::Config::ISA = qw(Exporter);\n>                                            ^^^^\n>\n> Trailing whitespace.  Didn't git warn you about this?\n>\n>> +     @Gitweb::Config::EXPORT = qw();\n>> +}\n>\n> Why BEGIN block?  Why 'use Exporter();' and not 'use Exporter;'?\n>\n> Why not\n>\n>   use Exporter;\n>   use base 'Exporter';\n>\n> or even\n>\n>   use Exporter qw(import);\n>\n> (we can use the last version, because gitweb requires high enough Perl\n> version itself, so that this form can be used).\n\nOk. Sure.\n\n>> +\n>> +our $t0;\n>> +if (eval { require Time::HiRes; 1; }) {\n>> +     $t0 = [Time::HiRes::gettimeofday()];\n>> +}\n>> +our $number_of_git_cmds = 0;\n>\n> This does not strictly speaking belong in Gitweb::Config, and probably\n> neither in Gitweb::Request, but Gitweb::request is a better place for\n> it.\n>\n> Never-mind, we can move it later.\n>\n>> +\n>\n> Here it would be good place for comment that those are variables that\n> are affected by build-time configuration, and therefore their\n> initialization is put in gitweb.perl (together with comments with\n> their description).\n\nOk.\n\n>> +our ($GIT, $version, $git_version);\n>> +our ($projectroot, $project_maxdepth, $projects_list, @git_base_url_list);\n>> +our ($export_ok, $strict_export);\n>> +our ($home_link_str, $site_name, $site_header, $site_footer, $home_text);\n>> +our (@stylesheets, $stylesheet, $logo, $favicon, $javascript);\n>> +our ($GITWEB_CONFIG, $GITWEB_CONFIG_SYSTEM);\n>> +\n>> +# URI and label (title) of GIT logo link\n>> +#our $logo_url = \"http://www.kernel.org/pub/software/scm/git/docs/\";\n>> +#our $logo_label = \"git documentation\";\n>> +our $logo_url = \"http://git-scm.com/\";\n>> +our $logo_label = \"git homepage\";\n>\n> [cut]\n>\n> Nothing especially interesting here.  'git blame -C -C' should detect\n> code movement.\n>\n>> +\n>> +sub evaluate_gitweb_config {\n>> +     # die if there are errors parsing config file\n>> +     if (-e $GITWEB_CONFIG) {\n>> +             do $GITWEB_CONFIG;\n>> +             die $@ if $@;\n>> +     } elsif (-e $GITWEB_CONFIG_SYSTEM) {\n>> +             do $GITWEB_CONFIG_SYSTEM;\n>> +             die $@ if $@;\n>> +     }\n>> +}\n>\n> Here the question is if this would affect interpretation of\n> $GITWEB_CONFIG etc. if it is a relative path.  Perhaps nothing will\n> change, because paths are relative to the directory the script is run,\n> not relative to where module resides.\n>\n> Not that is matter much either way, as at most it would require\n> stating in gitweb/README that one should use absolute pathnames.\n>\n> Note that tests use absolute pathname, so passing test does not answer\n> this.\n\nWill check it out and update README accoringly.\n\n>> +\n>> +1;\n>\n> O.K.\n>\n> Good work!\n> --\n> Jakub Narebski\n> Poland\n>\n\nThanks,\nPavan.\n"},{"id":"142865","messageId":"201006031055.28966.jnareb@gmail.com","threadId":"23985","inReplyTo":"201006022229.31593.jnareb@gmail.com","subject":"About [PATCH 2/2] gitweb: Create a perl module to handle gitweb cgi params and vars","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-06-03T08:55:28Z","receivedAt":"2010-06-03T08:55:28Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"This comment is about commit a6d54b3 (gitweb: Create a perl module to\nhandle gitweb cgi params and vars, 2010-06-02) on 'master' branch of\nrepository shown at http://repo.or.cz/w/git/gsoc2010-gitweb.git\n\n> From a6d54b39cee3d8d39c7ffd993b23e7477d4b0eeb Mon Sep 17 00:00:00 2001\n> From: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>\n> Date: Wed, 2 Jun 2010 01:09:51 +0530\n> Subject: [PATCH] gitweb: Create a perl module to handle gitweb cgi params and vars\n\nI would probably say\n\n  Subject: [PATCH] gitweb: Create Gitweb::Request module to handle gitweb cgi params and vars\n\nor even\n\n  Subject: [PATCH] gitweb: Create Gitweb::Request module\n\nso that _name_ of this module is in commit subject (summary).\n\n> \n> Create a perl module in path gitweb/lib/Gitweb/Request.pm\n> to store and handle all the cgi params and global variables\n> regarding the gitweb.perl script and change the scope of those\n> variables in the main script.\n> \n> Change Makefile accordingly to install the Perl Module.\n\nSimilar comment like for the previous patch.\n\nYou would probably want to list which subroutines were moved to\nGitweb::Request, I think.\n\n> \n> Signed-off-by: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>\n> ---\n\nThis patch would be probably based on 'next', and this should be\nmentioned in this area, in comment about patch.\n\n>  gitweb/Makefile              |    1 +\n>  gitweb/gitweb.perl           | 1245 ++++++++++++++++++++----------------------\n>  gitweb/lib/Gitweb/Request.pm |   58 ++\n>  3 files changed, 662 insertions(+), 642 deletions(-)\n>  create mode 100644 gitweb/lib/Gitweb/Request.pm\n> \n> diff --git a/gitweb/Makefile b/gitweb/Makefile\n> index 45e176e..15646b2 100644\n> --- a/gitweb/Makefile\n> +++ b/gitweb/Makefile\n> @@ -113,6 +113,7 @@ GITWEB_FILES += static/git-logo.png static/git-favicon.png\n>  \n>  # Files: gitweb/lib/Gitweb\n>  GITWEB_LIB_GITWEB += lib/Gitweb/Config.pm\n> +GITWEB_LIB_GITWEB += lib/Gitweb/Request.pm\n\nNice.\n\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 98a85f4..263af48 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -26,6 +26,7 @@ sub __DIR__ () {\n>  use lib __DIR__ . \"/lib\";\n>  \n>  use Gitweb::Config;\n> +use Gitweb::Request;\n\nNice.\n\n> @@ -33,42 +34,6 @@ BEGIN {\n>  \n>  our $version = \"++GIT_VERSION++\";\n\nThe above context line changed wrt. original patch, to follow the idea\nabout exporting variables by default in Gitweb::Config, and not using\nfully qualified variable names.\n\n>  \n> -our ($my_url, $my_uri, $base_url, $path_info, $home_link);\n> -sub evaluate_uri {\n> -\tour $cgi;\n\nI guess that $cgi is left for gitweb.perl, isn't it, because it\ndepends on FastCGI vs CGI?  [reads later part].  I guess not, only\n$CGI is left in gitweb.perl.\n\n[...]\n> -}\n\nO.K. these variables, and evaluate_uri() is moved to Gitweb::Request.\n\n[removed changes related to introducing fully qualified variable\n names; it would make patch smaller without those]\n\n> @@ -347,13 +312,11 @@ our %allowed_options = (\n>  # build an array of parameters that can be multi-valued, but since for the time\n>  # being it's only this one, we just single it out\n>  sub evaluate_query_params {\n> -\tour $cgi;\n[...]\n>  }\n\n> @@ -361,12 +324,12 @@ sub evaluate_query_params {\n>  # now read PATH_INFO and update the parameter list for missing parameters\n>  sub evaluate_path_info {\n[...]\n>  }\n\n>  sub evaluate_and_validate_params {\n[...]\n>  }\n\nShouldn't evaluate_query_params(), evaluate_path_info(), and the\nsubroutine that ties them together evaluate_and_validate_params() be\nin Gitweb::Request too?\n\n>  \n>  sub evaluate_git_dir {\n[....]\n>  }\n\nDitto with evaluate_git_dir()?\n\nBTW. as I said in comment to previous patch, vriables such as $project\nshould be put in Gitweb::Request in my opinion, not in Gitweb::Config.\nPerhaps they are, but it is not obvious from this patch.\n\n>  \n>  our (@snapshot_fmts, $git_avatar);\n> @@ -643,32 +605,32 @@ set_message(\\&handle_errors_html);\n>  \n>  # dispatch\n>  sub dispatch {\n[...]\n>  }\n>  \n>  sub run_request {\n\nThose two subroutines should not, I guess, be put in Gitweb::Request.\nThey would be in catch-all Gitweb module, I guess, or perhaps in the\nlater post-GSoC future in Gitweb::Dispatch or something.\n\n> @@ -689,7 +651,6 @@ sub run_request {\n>  our $is_last_request = sub { 1 };\n>  our ($pre_dispatch_hook, $post_dispatch_hook, $pre_listen_hook);\n>  our $CGI = 'CGI';\n> -our $cgi;\n>  sub evaluate_argv {\n>  \treturn unless (@ARGV);\n\nAh, so $CGI is left in gitweb.perl, $cgi is moved to Gitweb::Request.\n  \n[cut more than half of patch, which sould not be needed with exporting\n variables and not using fully qualified variable names]\n\n> diff --git a/gitweb/lib/Gitweb/Request.pm b/gitweb/lib/Gitweb/Request.pm\n> new file mode 100644\n> index 0000000..91cc492\n> --- /dev/null\n> +++ b/gitweb/lib/Gitweb/Request.pm\n> @@ -0,0 +1,58 @@\n> +#!/usr/bin/perl\n> +#\n> +# Gitweb::Request -- gitweb request(cgi) package\n> +#\n> +# This program is licensed under the GPLv2\n\nI don't remember what is git policy on copyright statements, and on\nGPLv2 vs GPLv2+...\n\n> +\n> +package Gitweb::Request;\n> +\n> +use strict;\n> +\n> +BEGIN {\n> +\tuse Exporter();\n> +\n> +\t@Gitweb::Request::ISA = qw(Exporter);\t\n> +\t@Gitweb::Request::EXPORT = qw();\n> +}\n\nSame comment as for previous patch.\n\n   use Exporter qw(import);\n   our @EXPORT = qw($cgi, $my_url, $my_uri, $base_url, ...);\n\nBTW with exporting variables you can easily see here that\nGitweb::Request does not use any variable from Gitweb::Config.\n\n> +\n> +our ($cgi, $my_url, $my_uri, $base_url, $path_info, $home_link);\n> +our ($action, $project, $file_name, $file_parent, $hash, $hash_parent, $hash_base,\n> +     $hash_parent_base, @extra_options, $page, $git_dir);\n> +our ($searchtype, $search_use_regexp, $searchtext, $search_regexp);\n> +\n> +sub evaluate_uri {\n[...]\n> +}\n\nStraightforward code movement.  But see comment above on which\nsubroutines I feel should be also put in Gitweb::Request.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"142867","messageId":"AANLkTilYSAj_byhZ1WDEBrYOAj6OZVsyDmp59vuK0tQz@mail.gmail.com","threadId":"23985","inReplyTo":"201006031055.28966.jnareb@gmail.com","subject":"Re: About [PATCH 2/2] gitweb: Create a perl module to handle gitweb cgi params and vars","fromName":"Pavan Kumar Sunkara","fromEmail":"pavan.sss1991@gmail.com","sentAt":"2010-06-03T09:52:32Z","receivedAt":"2010-06-03T09:52:32Z","isPatch":true,"sender":{"key":"pavan.sss1991@gmail.com","avatar":"https://avatars.githubusercontent.com/u/174703?v=4"},"body":"> Shouldn't evaluate_query_params(), evaluate_path_info(), and the\n> subroutine that ties them together evaluate_and_validate_params() be\n> in Gitweb::Request too?\n>\n>>\n>>  sub evaluate_git_dir {\n> [....]\n>>  }\n>\n> Ditto with evaluate_git_dir()?\n\nWell, evaluate_and_validate_params() and evaluate_path_info() contains\ncalls to subroutines which are not yet moved into any package. So,\nwhat do you want to in such a case ?\n\n> BTW. as I said in comment to previous patch, vriables such as $project\n> should be put in Gitweb::Request in my opinion, not in Gitweb::Config.\n> Perhaps they are, but it is not obvious from this patch.\n>\n>>\n>>  our (@snapshot_fmts, $git_avatar);\n>> @@ -643,32 +605,32 @@ set_message(\\&handle_errors_html);\n>>\n>>  # dispatch\n>>  sub dispatch {\n> [...]\n>>  }\n>>\n>>  sub run_request {\n>\n> Those two subroutines should not, I guess, be put in Gitweb::Request.\n> They would be in catch-all Gitweb module, I guess, or perhaps in the\n> later post-GSoC future in Gitweb::Dispatch or something.\n\nYes.\n\n>> @@ -689,7 +651,6 @@ sub run_request {\n>>  our $is_last_request = sub { 1 };\n>>  our ($pre_dispatch_hook, $post_dispatch_hook, $pre_listen_hook);\n>>  our $CGI = 'CGI';\n>> -our $cgi;\n>>  sub evaluate_argv {\n>>       return unless (@ARGV);\n>\n> Ah, so $CGI is left in gitweb.perl, $cgi is moved to Gitweb::Request.\n>\n> [cut more than half of patch, which sould not be needed with exporting\n>  variables and not using fully qualified variable names]\n>\n>> diff --git a/gitweb/lib/Gitweb/Request.pm b/gitweb/lib/Gitweb/Request.pm\n>> new file mode 100644\n>> index 0000000..91cc492\n>> --- /dev/null\n>> +++ b/gitweb/lib/Gitweb/Request.pm\n>> @@ -0,0 +1,58 @@\n>> +#!/usr/bin/perl\n>> +#\n>> +# Gitweb::Request -- gitweb request(cgi) package\n>> +#\n>> +# This program is licensed under the GPLv2\n>\n> I don't remember what is git policy on copyright statements, and on\n> GPLv2 vs GPLv2+...\n>\n>> +\n>> +package Gitweb::Request;\n>> +\n>> +use strict;\n>> +\n>> +BEGIN {\n>> +     use Exporter();\n>> +\n>> +     @Gitweb::Request::ISA = qw(Exporter);\n>> +     @Gitweb::Request::EXPORT = qw();\n>> +}\n>\n> Same comment as for previous patch.\n>\n>   use Exporter qw(import);\n>   our @EXPORT = qw($cgi, $my_url, $my_uri, $base_url, ...);\n>\n> BTW with exporting variables you can easily see here that\n> Gitweb::Request does not use any variable from Gitweb::Config.\n>\n>> +\n>> +our ($cgi, $my_url, $my_uri, $base_url, $path_info, $home_link);\n>> +our ($action, $project, $file_name, $file_parent, $hash, $hash_parent, $hash_base,\n>> +     $hash_parent_base, @extra_options, $page, $git_dir);\n>> +our ($searchtype, $search_use_regexp, $searchtext, $search_regexp);\n>> +\n>> +sub evaluate_uri {\n> [...]\n>> +}\n>\n> Straightforward code movement.  But see comment above on which\n> subroutines I feel should be also put in Gitweb::Request.\n\nPlease answer the question regarding calls to not-yet-packaged subroutines.\n\nThanks,\nPavan.\n"},{"id":"142868","messageId":"201006031217.07525.jnareb@gmail.com","threadId":"23985","inReplyTo":"AANLkTilYSAj_byhZ1WDEBrYOAj6OZVsyDmp59vuK0tQz@mail.gmail.com","subject":"Re: About [PATCH 2/2] gitweb: Create a perl module to handle gitweb cgi params and vars","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-06-03T10:17:06Z","receivedAt":"2010-06-03T10:17:06Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, 3 Jun 2010, Pavan Kumar Sunkara wrote:\n> Jakub Narebski wrote:\n\n>> Shouldn't evaluate_query_params(), evaluate_path_info(), and the\n>> subroutine that ties them together evaluate_and_validate_params() be\n>> in Gitweb::Request too?\n>>\n>> Ditto with evaluate_git_dir()?\n> \n> Well, evaluate_and_validate_params() and evaluate_path_info() contains\n> calls to subroutines which are not yet moved into any package. So,\n> what do you want to in such a case ?\n\nO.K., in this case you should mention in the commit message that\nthe subroutines evaluate_query_params(), evaluate_path_info(),\nevaluate_and_validate_params() and evaluate_git_dir() didn't get\nmoved to Gitweb::Request because evaluate_and_validate_params()\nand evaluate_path_info() contain calls to check_head_link(),\ndie_error() and validate_*() subroutines which are not yet moved\ninto any package.  And it doesn't make sense to put only some\nof them in Gitweb::Request.\n\nWe can always move them later.\n\n\nP.S. The validate_*() subroutines could also be moved to\nGitweb::Request.  The check_head_link() subroutine looks like\ncandidate for Gitweb::Util / Gitweb::Utils; I am not sure where\none should put die_error() and friends: git_header_html(),\ngit_footer_html(), get_page_title(), and other subroutines\nthey use.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"142869","messageId":"201006031223.31675.jnareb@gmail.com","threadId":"23985","inReplyTo":"AANLkTin9ZHij7ldIlSHyCqOd8NmU9DQA5q-tyvIImtdD@mail.gmail.com","subject":"Re: About [PATCH] gitweb: Create a perl module to store gitweb configuration","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-06-03T10:23:31Z","receivedAt":"2010-06-03T10:23:31Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 3 Jun 2010, Pavan Kumar Sunkarawrote:\n> 2010/6/3 Jakub Narebski <jnareb@gmail.com>:\n>>\n>> This comment is about commit 9526ab8 (gitweb: Create a perl module to\n>> store gitweb configuration, 2010-06-01) on 'master' branch of\n>> repository shown at http://repo.or.cz/w/git/gsoc2010-gitweb.git\n\n>>> diff --git a/gitweb/Makefile b/gitweb/Makefile\n>>> index d2584fe..45e176e 100644\n>>> --- a/gitweb/Makefile\n>>> +++ b/gitweb/Makefile\n>>> @@ -55,6 +55,7 @@ PERL_PATH  ?= /usr/bin/perl\n>>>  bindir_SQ = $(subst ','\\'',$(bindir))#'\n>>>  gitwebdir_SQ = $(subst ','\\'',$(gitwebdir))#'\n>>>  gitwebstaticdir_SQ = $(subst ','\\'',$(gitwebdir)/static)#'\n>>> +gitweblibdir_SQ = $(subst ','\\'',$(gitwebdir)/lib)#'\n>>\n>> I think it would be good idea to have 'gitweblibdir' as a separate\n>> variable, alongside 'gitwebdir', and which would default to\n>>\n>>   gitweblibdir = $(gitwebdir)/lib\n>>\n>> to make it possible to install gitweb modules not alongside gitweb,\n>> but somewhere else, for example together with other Perl modules.\n>>\n>> Then you would have:\n>>\n>>  +gitweblibdir_SQ = $(subst ','\\'',$(gitweblibdir))#'\n> \n> It's great.\n> \n>> But I think this change can be left for a separate commit.  It is not\n>> something terribly important, something blocking accepting the patch.\n\nActually to really be able to put gitweb packages somewhere else,\ngitweb.perl should *probably* contain\n\n  use lib '++GITWEBLIBDIR++';\n\nOtherwise 'gitweblibdir' would have to be either left default to\ninstall packages alingside gitweb.cgi script, or be directory that is\nin PERL5LIB for web server.\n\nSo let's leave it for a later commit.\n\n-- \nJakub Narebski\nPoland\n"}]}