Re: [PATCH/RFC] gitweb: Create Gitweb::Git module

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

>>
>> 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 <pavan.sss1991@gmail.com>
>> ---
>
>> 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?

>> @@ -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?

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

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