From: Pavan Kumar Sunkara Date: Mon, 07 Jun 2010 15:31:35 GMT Subject: Re: [PATCH/RFC] gitweb: Create Gitweb::Git module Message-ID: In-Reply-To: <201006071042.42908.jnareb@gmail.com> 2010/6/7 Jakub Narebski : > Summary: minor complaints, mainly about _descriptions_. > > On Sun, 6 June 2010, Pavan Kumar Sunkara wrote: > >> Subject: [PATCH/RFC] gitweb: Create Gitweb::Git module >> >> Create a Gitweb::Git module in 'gitweb/lib/Gitweb/Git.pm' >> to store essential git variables and subs regarding the >> gitweb.perl script > > The pararaph above and the commit description (subject of this mail) do > not tell us what does this new module Gitweb::Git is for, what does it > contain.  The description of module in header comment is also a bit > lacking (see my comments below). > > I know I suggested, among other forms, the above short form of commit > description, but I think that in this case it is too short. > > Perhaps (this is only a proposal): > >  gitweb: Create Gitweb::Git module, to run git commands > >  Create a Gitweb::Git module in  'gitweb/lib/Gitweb/Git.pm' >  to deal with running git commands (and also processing output >  of git commands with external programs) from gitweb. > > I think you should also write why $GIT variable is moved to Gitweb::Git, > even though it is variable which is configured during build, and one > might think that it belongs to Gitweb::Config. > > Perhaps something like this (it is only a proposal): > >  This module is intended as standalone module, which does not require >  (include) other gitweb' modules to avoid circular dependencies.  That >  is why it includes $GIT variable, even though this variable is >  configured during building gitweb.  On the other hand $GIT is more >  about git configuration, than gitweb configuration. > > Or something like that. Ok. >> >> Subroutines moved: >>       evaluate_git_version >>       git_cmd >>       quote_command >> >> Subroutines yet to move: (Contains not yet packaged subs & vars) >>       None >> >> Update gitweb/Makefile to install gitweb modules alongside gitweb > > It is not 'gitweb modules', but single gitweb module. > >  Update gitweb/Makefile to install Gitweb::Git alongside gitweb. > >> >> Signed-off-by: Pavan Kumar Sunkara >> --- > >> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl >> index e95aaf7..59a65a8 100755 >> --- a/gitweb/gitweb.perl >> +++ b/gitweb/gitweb.perl > >> -# core git executable to use >> -# this can just be "git" if your webserver has a sensible PATH >> -our $GIT = "++GIT_BINDIR++/git"; >> +#only this variable has it's root in Gitweb::Git >> +$GIT = "++GIT_BINDIR++/git"; > > Hmmm... is this comment really needed?  It does not matter, at least not > much, where given subroutine comes from.  Only lack of 'our' indication > that it is defined in other package. > > Perhaps > >  +# $GIT is from Gitweb::Git > > or something like that? Ok. >> @@ -77,7 +75,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; >>       } > > Nice side-effect. > >> @@ -197,13 +194,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++; >> -} > > I guess that evaluate_git_version and $number_of_git_cmds are moved to > Gitweb::Git because of technical reasons (for module to be self > contained, and to avoid circular dependencies), isn't it? Yeah. Exactly. >> @@ -492,10 +482,8 @@ 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; >> +     $git_dir = "$projectroot/$project" if $project; >>  } > > O.K. > >> diff --git a/gitweb/lib/Gitweb/Git.pm b/gitweb/lib/Gitweb/Git.pm >> new file mode 100644 >> index 0000000..9961e6d >> --- /dev/null >> +++ b/gitweb/lib/Gitweb/Git.pm >> @@ -0,0 +1,48 @@ >> +#!/usr/bin/perl >> +# >> +# Gitweb::Git -- gitweb git package >> +# >> +# This program is licensed under the GPLv2 > > This description doesn't tell us much.  What does "git package" mean? > I would like to have description here what this package is for, and > whet it (should) include. > > Perhaps (this is only a proposal): > >  +# Gitweb::Git -- gitweb's package dealing with running git commands > > or something like that. Ok. >> + >> +package Gitweb::Git; >> + >> +use strict; >> +use warnings; >> +use Exporter qw(import); >> + >> +our @EXPORT = qw($GIT $number_of_git_cmds $git_version $git_dir >> +                 git_cmd quote_command evaluate_git_version); >> + >> +# core git executable to use >> +# this can just be "git" if your webserver has a sensible PATH >> +our $GIT; > > One could think that this should belong to Gitweb::Config, but it is > more about _git_ configuration than about _gitweb_ configuration. > And there are technical reasons for having it there. > >> + >> +our $number_of_git_cmds = 0; > > I guess that counting git commands belong there... > > By the way, can anyone check if it is correctly reset, and is counting > number of git commands it took to process _a request_, also when running > in FastCGI mode? > >> + >> +# version of the core git binary >> +our $git_version; > > Hmmm... wouldn't it be better to have this close to evaluate_git_version? > > Also, does $git_version and evaluate_git_version belong in Gitweb::Git? Yes because it is regarding git rather than gitweb. Thanks, Pavan.