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@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?
> @@ -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?
> +
> +# path to the current git repository
> +our $git_dir;
> +
> +# returns path to the core git executable and the --git-dir parameter as list
> +sub git_cmd {
> + $number_of_git_cmds++;
> + return $GIT, '--git-dir='.$git_dir;
> +}
O.K.
> +
> +# quote the given arguments for passing them to the shell
> +# quote_command("command", "arg 1", "arg with ' and ! characters")
> +# => "'command' 'arg 1' 'arg with '\'' and '\!' characters'"
> +# Try to avoid using this function wherever possible.
> +sub quote_command {
> + return join(' ',
> + map { my $a = $_; $a =~ s/(['!])/'\\$1'/g; "'$a'" } @_ );
> +}
O.K.
> +
> +sub evaluate_git_version {
> + $git_version = qx("$GIT" --version) =~ m/git version (.*)$/ ? $1 : "unknown";
> + $number_of_git_cmds++;
> +}
> +
> +1;
> --
--
Jakub Narębski
Poland
--
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