2010/6/7 Jakub Narebski <jnareb@xxxxxxxxx>:
> 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 <pavan.sss1991@xxxxxxxxx>
>> ---
>
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html