[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

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

Signed-off-by: Pavan Kumar Sunkara <redacted>
---
 gitweb/Makefile          |    1 +
 gitweb/gitweb.perl       |   35 +++-----------------------------
 gitweb/lib/Gitweb/Git.pm |   48 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 31 deletions(-)
 create mode 100644 gitweb/lib/Gitweb/Git.pm

diff --git a/gitweb/Makefile b/gitweb/Makefile
index 4343396..fcd4042 100644
--- a/gitweb/Makefile
+++ b/gitweb/Makefile

@@ -115,6 +115,7 @@ GITWEB_FILES += static/git-logo.png static/git-favicon.png
 GITWEB_LIB_GITWEB += lib/Gitweb/Config.pm
 GITWEB_LIB_GITWEB += lib/Gitweb/Request.pm
 GITWEB_LIB_GITWEB += lib/Gitweb/Escape.pm
+GITWEB_LIB_GITWEB += lib/Gitweb/Git.pm
 
 GITWEB_REPLACE = \
 	-e 's|++GIT_VERSION++|$(GIT_VERSION)|g' \

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

@@ -24,12 +24,11 @@ sub __DIR__ () {
 }
 use lib __DIR__ . "/lib";
 
+use Gitweb::Git;
 use Gitweb::Config;
 use Gitweb::Request;
 use Gitweb::Escape;
 
-our $number_of_git_cmds = 0;
-
 BEGIN {
 	CGI->compile() if $ENV{'MOD_PERL'};
 }

@@ -39,9 +38,8 @@ BEGIN {
 # with their descriptions is listed in Gitweb::Config.
 $version = "++GIT_VERSION++";
 
-# 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";
 
 $projectroot = "++GITWEB_PROJECTROOT++";
 $project_maxdepth = "++GITWEB_PROJECT_MAXDEPTH++";

@@ -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;
 	}

@@ -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++;
-}
-
 sub check_loadavg {
 	if (defined $maxload && get_loadavg() > $maxload) {
 		die_error(503, "The load average on the server is too high");

@@ -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;
 }
 
 our (@snapshot_fmts, $git_avatar);

@@ -1548,21 +1536,6 @@ sub get_feed_info {
 ## ----------------------------------------------------------------------
 ## git utility subroutines, invoking git commands
 
-# 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;
-}
-
-# 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'" } @_ );
-}
-
 # get HEAD ref of given project as hash
 sub git_get_head_hash {
 	return git_get_full_hash(shift, 'HEAD');

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
+
+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;
+
+our $number_of_git_cmds = 0;
+
+# version of the core git binary
+our $git_version;
+
+# 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;
+}
+
+# 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'" } @_ );
+}
+
+sub evaluate_git_version {
+	$git_version = qx("$GIT" --version) =~ m/git version (.*)$/ ? $1 : "unknown";
+	$number_of_git_cmds++;
+}
+
+1;

-- 
1.7.1.450.g21d56

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

On Mon, Jun 07, 2010 at 02:54:11AM +0530, Pavan Kumar Sunkara wrote:

That comment is super-cryptic, I'd suggest either rewording it or
dropping it completely.

Is there any reason why didn't you move the rest of the commands from
this section to Gitweb::Git as well?

				Petr "Pasky" Baudis

On Mon, Jun 07, 2010, Petr Baudis wrote:

On Mon, Jun 07, 2010 at 02:54:11AM +0530, Pavan Kumar Sunkara wrote:

quoted

@@ -1548,21 +1536,6 @@ sub get_feed_info {
 ## ----------------------------------------------------------------------
 ## git utility subroutines, invoking git commands

Is there any reason why didn't you move the rest of the commands from
this section to Gitweb::Git as well?

Probably because they are less clear about being about running (git)
commands, I guess?

Let's examine those subroutines in more detail:

* git_cmd - requires $GIT and $git_dir, also $number_of_git_cmds

* quote_command - helper command, not exactly about running comands,
  but about shell escaping / shell quoting.  Should it be in Gitweb::Git
  or in Gitweb::Escape?

* evaluate_git_version - requires $GIT, sets $number_of_git_cmds
  and $git_version.  Does it belong to Gitweb::Git, or Gitweb::Config,
  or perhaps Gitweb::Request?

* git_get_hash (and wrappers: git_get_head_hash, git_get_full_hash,
  git_get_short_hash) - requires $projectroot, something which other
  commands do not require, and $git_dir.  Uses git_cmd().

* git_get_type - uses git_cmd().

* git_get_hash_by_path - uses git_cmd()
* git_get_path_by_hash - uses git_cmd()

Subroutines related to parsing per-repository configuration should be
either in Gitweb::Config, or in a separate module, e.g. Gitweb::Git::Config
(or something like that, like Gitweb::RepoConfig, etc.).

Next there are 'git utility functions, directly accessing git repository'

-- 
Jakub Narebski
Poland

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.