Re: [PATCH] gitweb: snapshot cleanups & support for offering multiple formats

Jakub Narebski [off-list ref] writes:

On Tue, 17 July 2007, Matt McCutchen napisał:
...

quoted

Alert for gitweb site administrators: This patch changes the format of
$feature{'snapshot'}{'default'} in gitweb_config.perl from a list of
three pieces of information about a single format to a list of one or
more formats you wish to offer from the set ('tgz', 'tbz2', 'zip').
Update your gitweb_config.perl appropriately.  The preferred names for
gitweb.snapshot in repository configuration have also changed from
'gzip' and 'bzip2' to 'tgz' and 'tbz2', but the old names are still
recognized for compatibility.

This alert/warning should probably be put in RelNotes for when it would
be in git.git

Does anybody else worry about the backward imcompatibility, I
wonder...  List?

I really hate to having to say something like that in the
RelNotes.  I do not think this is a good enough reason to break
existing configurations; I would not want to be defending that
change.

quoted

I thought of another incompatibility: previously bookmarked snapshot
URLs will no longer work because they lack the new "sf" parameter.  I
don't care about this; do any of you?

I think either having good error message, or using first format avaiable
would be good enough.

I doubt bookmarked snapshot URL would make sense to begin with,
so this would be Ok.

I am wondering if something like this patch (totally untested,
mind you) to convert the old style %feature in configuration at
the site at runtime would be sufficient.

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index f17c983..cdec4d0 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -236,9 +236,39 @@ our %feature = (
 		'default' => [0]},
 );
 
+# Functions to convert values from older gitweb configuration
+# into the current data format
+sub gitweb_bc_feature_snapshot {
+	my $def = $feature{'snapshot'}{'default'};
+	# Older definition was to have either undef (to disable), or
+	# a three-element array whose first element was content encoding
+	# without leading "application/".
+	return if (ref $def ne 'ARRAY');
+	if (!defined $def->[0] && @$def == 1) {
+		# Disabled -- the new way to spell it is to have an empty
+		# arrayref.
+		$feature{'snapshot'}{'default'} = [];
+		return;
+	}
+	return if (@$def != 3);
+	for ($def->[0]) {
+		if (/x-gzip/) {
+			$feature{'snapshot'}{'default'} = ['tgz'];
+		}
+		if (/x-bz2/) {
+			$feature{'snapshot'}{'default'} = ['tbz2'];
+		}
+		if (/x-zip/) {
+			$feature{'snapshot'}{'default'} = ['zip'];
+		}
+	}
+}
+
 sub gitweb_check_feature {
 	my ($name) = @_;
 	return unless exists $feature{$name};
+	eval "gitweb_bc_feature_$name()";
+
 	my ($sub, $override, @defaults) = (
 		$feature{$name}{'sub'},
 		$feature{$name}{'override'},

--- Junio C Hamano <gitster@pobox.com> wrote:

Jakub Narebski [off-list ref] writes:

quoted

On Tue, 17 July 2007, Matt McCutchen napisał:
...

quoted

Alert for gitweb site administrators: This patch changes the format of
$feature{'snapshot'}{'default'} in gitweb_config.perl from a list of
three pieces of information about a single format to a list of one or
more formats you wish to offer from the set ('tgz', 'tbz2', 'zip').
Update your gitweb_config.perl appropriately.  The preferred names for
gitweb.snapshot in repository configuration have also changed from
'gzip' and 'bzip2' to 'tgz' and 'tbz2', but the old names are still
recognized for compatibility.

This alert/warning should probably be put in RelNotes for when it would
be in git.git

Does anybody else worry about the backward imcompatibility, I
wonder...  List?

I wouldn't mind an improvement in the snapshot area of gitweb.
I wasn't really happy with the snapshot feature as it was originally
implemented, as it would generate a tar file with ".tar.bz2"
name extension, but the file was NOT bz2, and I had to always
manually rename, bz2, and rename back.

I really hate to having to say something like that in the
RelNotes.  I do not think this is a good enough reason to break
existing configurations; I would not want to be defending that
change.

quoted

quoted

I thought of another incompatibility: previously bookmarked snapshot
URLs will no longer work because they lack the new "sf" parameter.  I
don't care about this; do any of you?

I think either having good error message, or using first format avaiable
would be good enough.

I doubt bookmarked snapshot URL would make sense to begin with,
so this would be Ok.

I am wondering if something like this patch (totally untested,
mind you) to convert the old style %feature in configuration at
the site at runtime would be sufficient.

"totally untested" is a problem.  Anything going into gitweb for
public consumption (master branch, next ok), should be completely
and exhaustively tested.

   Luben

Luben Tuikov wrote:

I wouldn't mind an improvement in the snapshot area of gitweb.
I wasn't really happy with the snapshot feature as it was originally
implemented, as it would generate a tar file with ".tar.bz2"
name extension, but the file was NOT bz2, and I had to always
manually rename, bz2, and rename back.

This was a *bug*, but it is now corrected (in 9aa17573). Gitweb used 
Content-Encoding, which is meant for _transparent_ compression.

-- 
Jakub Narebski
Poland

--- Jakub Narebski <jnareb@gmail.com> wrote:

Luben Tuikov wrote:

quoted

I wouldn't mind an improvement in the snapshot area of gitweb.
I wasn't really happy with the snapshot feature as it was originally
implemented, as it would generate a tar file with ".tar.bz2"
name extension, but the file was NOT bz2, and I had to always
manually rename, bz2, and rename back.

This was a *bug*, but it is now corrected (in 9aa17573). Gitweb used 
Content-Encoding, which is meant for _transparent_ compression.

Yeah, that's what I suspected, since there was nothing obviously
wrong with the code.

Thanks for the fix.

   Luben

On Thu, 19 July 2007, Junio C Hamano wrote:

Jakub Narebski [off-list ref] writes:

quoted

On Tue, 17 July 2007, Matt McCutchen napisał:
...

quoted

Alert for gitweb site administrators: This patch changes the format of
$feature{'snapshot'}{'default'} in gitweb_config.perl from a list of
three pieces of information about a single format to a list of one or
more formats you wish to offer from the set ('tgz', 'tbz2', 'zip').
Update your gitweb_config.perl appropriately.  The preferred names for
gitweb.snapshot in repository configuration have also changed from
'gzip' and 'bzip2' to 'tgz' and 'tbz2', but the old names are still
recognized for compatibility.

This alert/warning should probably be put in RelNotes for when it would
be in git.git

Does anybody else worry about the backward imcompatibility, I
wonder...  List?

I really hate to having to say something like that in the
RelNotes.  I do not think this is a good enough reason to break
existing configurations; I would not want to be defending that
change.

[...]

I am wondering if something like this patch (totally untested,
mind you) to convert the old style %feature in configuration at
the site at runtime would be sufficient.

Would it be sufficient to put above alert/warning in commit message,
RelNotes and gitweb/INSTALL (or gitweb/README), and add rule to Makefile
to convert old configuration, or at least check if GITWEB_CONFIG uses
old snapshot configuration? This way if somebody is installing/upgrading
gitweb by hand he/she would know what needs possibly to be changes, and
if somebody uses "make gitweb/gitweb.cgi" he would get big fat warning,
and info how to convert gitweb config.

By the way, I think it was a mistake to use different syntax in the
%feature hash ([content-encoding, suffix, program]) than in repo config
override (name).


Besides the proposed patch incurs performance penalty for all feature
checks, not only for snapshot. I think it could be solved by using
a hack of providing more aliases, so that 'gzip' (repo config) but
also 'x-gzip', 'gz' and 'gzip' (gitweb config) would be aliases to
'tgz' snapshot, and we would perform "uniq" on the list of snapshot
formats (assuming it is sorted). Or make 'x-gzip' and 'gz' aliases
into undef, so 'gzip' from old configuration would be aliased to the
new format name 'tgz'. What do you think about this?

Ooops, this has disadvantage of having to guess what could be put
in the gitweb config regarding snapshot configuration, but I think we
could assume that only the values enumerated in the old feature_snapshot
would be used.


All said, I think it is a good change. I guess that gitweb admins would
want to provide both tgz/tar.gz archives for the Unix crowd, and zip
archives for MS Windows users...


P.S. I wonder why git-archive does not support tgz format. Git is linked
to zlib, so...
-- 
Jakub Narebski
Poland

Check if PerlIO::gzip is available, and if it is make it possible to
enable (via 'compression' %feature) transparent compression of HTML
output.  Error messages and any non-HTML output are excluded from
transparent compression.

Signed-off-by: Jakub Narebski <redacted>
---
On Thu, 19 July 2007, Luben Tuikov wrote:

And _this_ patch adds support for true, intentional transparent
compression.

 gitweb/gitweb.perl |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 0acd0ca..d48a193 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -20,8 +20,12 @@ binmode STDOUT, ':utf8';
 
 BEGIN {
 	CGI->compile() if $ENV{'MOD_PERL'};
+
+	eval { require PerlIO::gzip; }; # needed for transparent compression
 }
 
+our $enable_transparent_compression = !! $PerlIO::gzip::VERSION;
+
 our $cgi = new CGI;
 our $version = "++GIT_VERSION++";
 our $my_url = $cgi->url();

@@ -238,6 +242,22 @@ our %feature = (
 		'override' => 0,
 		'default' => [1]},
 
+	# Enable transparent compression, for now only for HTML output;
+	# this reduces network bandwidth at the cost of CPU usage.
+	# You need to have PerlIO::gzip for that, and browser has to accept
+	# (via Accept-Encoding: HTTP request header) 'gzip' encoding.
+	# Transparent compression is not used for error messages.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'compression'}{'default'} = [1];
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'compression'}{'override'} = 1;
+	# and in project config gitweb.compression = 0|1;
+	'compression' => {
+		'sub' => \&feature_compression,
+		'override' => 0,
+		'default' => [0]},
+
 	# Make gitweb use an alternative format of the URLs which can be
 	# more readable and natural-looking: project name is embedded
 	# directly in the path and the query string contains other

@@ -336,6 +356,18 @@ sub feature_pickaxe {
 	return ($_[0]);
 }
 
+sub feature_compression {
+	my ($val) = git_get_project_config('compression', '--bool');
+
+	if ($val eq 'true') {
+		return (1);
+	} elsif ($val eq 'false') {
+		return (0);
+	}
+
+	return ($_[0]);
+}
+
 # checking HEAD file with -e is fragile if the repository was
 # initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed
 # and then pruned.

@@ -2238,9 +2270,24 @@ sub git_header_html {
 	} else {
 		$content_type = 'text/html';
 	}
+	# transparent compression has to be supported, enabled, and accepted
+	# explicitely by UA; note that qvalue of 0 means "not acceptable."
+	my %content_encoding = ();
+	if ($enable_transparent_compression &&
+	    gitweb_check_feature('compression') &&
+	    defined $cgi->http('HTTP_ACCEPT_ENCODING') &&
+	    $cgi->http('HTTP_ACCEPT_ENCODING') =~ m/(^|,|;|\s)gzip(,|;|\s|$)/ &&
+	    $cgi->http('HTTP_ACCEPT_ENCODING') !~ m/(^|,|;|\s)gzip\s*;q=0(,|\s|$)/) {
+		%content_encoding = (-content_encoding => 'gzip');
+	}
 	print $cgi->header(-type=>$content_type, -charset => 'utf-8',
+	                   %content_encoding,
 	                   -status=> $status, -expires => $expires);
 	my $mod_perl_version = $ENV{'MOD_PERL'} ? " $ENV{'MOD_PERL'}" : '';
+	if (%content_encoding) {
+		# implies $enable_transparent_compression
+		binmode STDOUT, ':gzip';
+	}
 	print <<EOF;
 <?xml version="1.0" encoding="utf-8"?>
 <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">

@@ -2375,6 +2422,7 @@ sub die_error {
 	my $status = shift || "403 Forbidden";
 	my $error = shift || "Malformed query, file missing or permission denied";
 
+	$enable_transparent_compression = 0;
 	git_header_html($status);
 	print <<EOF;
 <div class="page_body">

-- 
1.5.2.4

On Wed, Jul 25, 2007 at 08:39:43PM CEST, Jakub Narebski wrote:

Check if PerlIO::gzip is available, and if it is make it possible to

It doesn't really check if the require succeeded. Either the description
or (preferrably, but not a showstopper, IMO) the code should be
adjusted.

enable (via 'compression' %feature) transparent compression of HTML
output.  Error messages and any non-HTML output are excluded from
transparent compression.

Signed-off-by: Jakub Narebski <redacted>

Acked-by: Petr Baudis <redacted>

I'd put it on repo.or.cz... too bad that there I value CPU much more
than the bandwidth. ;-)

Why did you exclude non-HTML output from transparent compression? Me and
I guess other people too sometimes download rather large chunks of raw
data over gitweb.

-- 
				Petr "Pasky" Baudis
Ever try. Ever fail. No matter. // Try again. Fail again. Fail better.
		-- Samuel Beckett

On Sat, Aug 25, 2007, Petr Baudis wrote:

On Wed, Jul 25, 2007 at 08:39:43PM CEST, Jakub Narebski wrote:

quoted

Check if PerlIO::gzip is available, and if it is make it possible to

It doesn't really check if the require succeeded. Either the description
or (preferrably, but not a showstopper, IMO) the code should be
adjusted.

It does not check if require succeeded (I could do that this way),
but instead checks if $PerlIO::gzip::VERSION is defined (if it is true).

our $enable_transparent_compression = !! $PerlIO::gzip::VERSION;
 

quoted

enable (via 'compression' %feature) transparent compression of HTML
output.  Error messages and any non-HTML output are excluded from
transparent compression.

Signed-off-by: Jakub Narebski <redacted>

Acked-by: Petr Baudis <redacted>

By the way, this was more "proof of concept" than solution of an itch.
 

I'd put it on repo.or.cz... too bad that there I value CPU much more
than the bandwidth. ;-)

Why did you exclude non-HTML output from transparent compression? Me and
I guess other people too sometimes download rather large chunks of raw
data over gitweb.

Because it was easiest. We have single point of entry for HTML output
(the git_header_html subroutine), but we don't have anything similar for
non-HTML output. And we most certainly wouldn't want to enable transparent
compression for snapshots and 'blob_plain' view for compressed files,
including png, gif, jpeg, zip, mp3, ogg,...

-- 
Jakub Narebski
Poland

On Sun, Aug 26, 2007 at 12:09:29AM CEST, Jakub Narebski wrote:

On Sat, Aug 25, 2007, Petr Baudis wrote:

quoted

On Wed, Jul 25, 2007 at 08:39:43PM CEST, Jakub Narebski wrote:

quoted

quoted

Check if PerlIO::gzip is available, and if it is make it possible to

It doesn't really check if the require succeeded. Either the description
or (preferrably, but not a showstopper, IMO) the code should be
adjusted.

It does not check if require succeeded (I could do that this way),
but instead checks if $PerlIO::gzip::VERSION is defined (if it is true).

our $enable_transparent_compression = !! $PerlIO::gzip::VERSION;

Whoops, I completely missed this chunk.

 Bareword "PerlIO::gzip::VERSION" not allowed while "strict subs" in use at /home/pasky/WWW/repo/gitweb.cgi line 26.

-- 
				Petr "Pasky" Baudis
Ever try. Ever fail. No matter. // Try again. Fail again. Fail better.
		-- Samuel Beckett

On Sunday, 26 August 2007, Petr "Pasky" Baudis wrote:

On Sun, Aug 26, 2007 at 12:09:29AM CEST, Jakub Narebski wrote:

quoted

On Sat, Aug 25, 2007, Petr Baudis wrote:

quoted

On Wed, Jul 25, 2007 at 08:39:43PM CEST, Jakub Narebski wrote:

quoted

Check if PerlIO::gzip is available, and if it is make it possible to

It doesn't really check if the require succeeded. Either the description
or (preferrably, but not a showstopper, IMO) the code should be
adjusted.

It does not check if require succeeded (I could do that this way),
but instead checks if $PerlIO::gzip::VERSION is defined (if it is true).

See below for alternate solution.

quoted

our $enable_transparent_compression = !! $PerlIO::gzip::VERSION;

Whoops, I completely missed this chunk.

 Bareword "PerlIO::gzip::VERSION" not allowed while "strict subs" in use at /home/pasky/WWW/repo/gitweb.cgi line 26.

Did you perchance forgot '$' in "$PerlIO::gzip::VERSION"?

But I agree that using

	BEGIN {
        	CGI->compile() if $ENV{'MOD_PERL'};

	        eval { require PerlIO::gzip; }; # needed for transparent compression
		our $enable_transparent_compression = ! $@;
	}
 

instead of

	BEGIN {
        	CGI->compile() if $ENV{'MOD_PERL'};

	        eval { require PerlIO::gzip; }; # needed for transparent compression
	}
 
	our $enable_transparent_compression = !! $PerlIO::gzip::VERSION;
 
is more sensible. I have tried to check the above code for the case when
PerlIO::gzip is not available by using non-existent module "PerlIO::gzp"
in eval, and non-existent variable "$PerlIO::gip::VERSION" in the
definition of $enable_transparent_compression variable, and Perl doesn't
give any errors nor warnings while running gitweb.

But what it is a bit strange, when I have chosen different name for
a variable to test, "$PerlIO::gzip::VERSON" (existing but not loaded
module, non-existent name), I have got the following strange warning:

  gitweb.perl: Name "PerlIO::gzip::VERSON" used only once: possible typo
  at /home/jnareb/git/t/trash/../../gitweb/gitweb.perl line 27.

Strange...

-- 
Jakub Narebski
Poland