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

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

Nice side-effect.

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?

O.K.

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