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

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

[Pavan Kumar Sunkara]

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 <pavan.sss1991@gmail.com>
---
 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

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

[Jakub Narebski]

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

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

[Pavan Kumar Sunkara]

2010/6/7 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.

Ok.

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

Ok.

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

Yeah. Exactly.

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

Ok.

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

Yes because it is regarding git rather than gitweb.

Thanks,
Pavan.

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

[Petr Baudis]

On Mon, Jun 07, 2010 at 02:54:11AM +0530, Pavan Kumar Sunkara wrote:
> @@ -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++";

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

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

				Petr "Pasky" Baudis

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

[Jakub Narebski]

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

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