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

[Jakub Narebski <jnareb@gmail.com>]

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?

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