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

2010/6/7 Jakub Narebski [off-list ref]:

Summary: minor complaints, mainly about _descriptions_.

On Sun, 6 June 2010, Pavan Kumar Sunkara wrote:

quoted

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.

quoted

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.

quoted

Signed-off-by: Pavan Kumar Sunkara <redacted>
---

quoted

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index e95aaf7..59a65a8 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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.

quoted

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

quoted

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

quoted

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