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

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

[Matt McCutchen]

- Centralize knowledge about snapshot formats (mime types, extensions,
  commands) in %known_snapshot_formats and improve how some of that
  information is specified.  In particular, zip files are no longer a
  special case.

- Add support for offering multiple snapshot formats to the user so
  that he/she can download a snapshot in the format he/she prefers.
  The site-wide or project configuration now gives a list of formats
  to offer, and the "_snapshot_" link is replaced with, say,
  "snapshot (_tbz2_ _zip_)".

- Fix out-of-date "tarball" -> "archive" in comment.

Signed-off-by: Matt McCutchen <hashproduct@gmail.com>
---

I implemented this a while ago for my Web site.  You can see it in action at:

http://www.kepreon.com/~matt/mgear/mgear.git/

I thought I would submit it to the main project so you can adopt it if you like
it.

Matt

 gitweb/gitweb.perl |   89 +++++++++++++++++++++++++++++----------------------
 1 files changed, 51 insertions(+), 38 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index dbfb044..f36428e 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -101,6 +101,15 @@ our $mimetypes_file = undef;
 # could be even 'utf-8' for the old behavior)
 our $fallback_encoding = 'latin1';
 
+# information about snapshot formats that gitweb is capable of serving
+# name => [mime type, filename suffix, --format for git-archive,
+#          compressor command suffix]
+our %known_snapshot_formats = (
+	'tgz'  => ['application/x-gzip' , '.tar.gz' , 'tar', '| gzip' ],
+	'tbz2' => ['application/x-bzip2', '.tar.bz2', 'tar', '| bzip2'],
+	'zip'  => ['application/zip'    , '.zip'    , 'zip', ''       ],
+);
+
 # You define site-wide feature defaults here; override them with
 # $GITWEB_CONFIG as necessary.
 our %feature = (
@@ -131,20 +140,22 @@ our %feature = (
 		'override' => 0,
 		'default' => [0]},
 
-	# Enable the 'snapshot' link, providing a compressed tarball of any
+	# Enable the 'snapshot' link, providing a compressed archive of any
 	# tree. This can potentially generate high traffic if you have large
 	# project.
 
+	# Value is a list of formats defined in %known_snapshot_formats that
+	# you wish to offer.
 	# To disable system wide have in $GITWEB_CONFIG
-	# $feature{'snapshot'}{'default'} = [undef];
+	# $feature{'snapshot'}{'default'} = [];
 	# To have project specific config enable override in $GITWEB_CONFIG
 	# $feature{'snapshot'}{'override'} = 1;
-	# and in project config gitweb.snapshot = none|gzip|bzip2|zip;
+	# and in project config, a comma-separated list of formats or "none"
+	# to disable.  Example: gitweb.snapshot = tbz2,zip;
 	'snapshot' => {
 		'sub' => \&feature_snapshot,
 		'override' => 0,
-		#         => [content-encoding, suffix, program]
-		'default' => ['x-gzip', 'gz', 'gzip']},
+		'default' => ['tgz']},
 
 	# Enable text search, which will list the commits which match author,
 	# committer or commit text to a given string.  Enabled by default.
@@ -243,28 +254,15 @@ sub feature_blame {
 }
 
 sub feature_snapshot {
-	my ($ctype, $suffix, $command) = @_;
+	my (@fmts) = @_;
 
 	my ($val) = git_get_project_config('snapshot');
 
-	if ($val eq 'gzip') {
-		return ('x-gzip', 'gz', 'gzip');
-	} elsif ($val eq 'bzip2') {
-		return ('x-bzip2', 'bz2', 'bzip2');
-	} elsif ($val eq 'zip') {
-		return ('x-zip', 'zip', '');
-	} elsif ($val eq 'none') {
-		return ();
+	if ($val) {
+		@fmts = ($val eq 'none' ? () : split /,/, $val);
 	}
 
-	return ($ctype, $suffix, $command);
-}
-
-sub gitweb_have_snapshot {
-	my ($ctype, $suffix, $command) = gitweb_check_feature('snapshot');
-	my $have_snapshot = (defined $ctype && defined $suffix);
-
-	return $have_snapshot;
+	return grep exists $known_snapshot_formats{$_}, @fmts;
 }
 
 sub feature_grep {
@@ -542,6 +540,7 @@ sub href(%) {
 		order => "o",
 		searchtext => "s",
 		searchtype => "st",
+		snapshot_format => "sf",
 	);
 	my %mapping = @mapping;
 
@@ -1236,6 +1235,21 @@ sub format_diff_line {
 	return "<div class=\"diff$diff_class\">" . esc_html($line, -nbsp=>1) . "</div>\n";
 }
 
+# Generates undef or something like "snapshot (tbz2 zip)", linked.
+# Pass the hash.
+sub format_snapshot_links {
+	my ($hash) = @_;
+	my @snapshot_fmts = gitweb_check_feature('snapshot');
+	if (@snapshot_fmts) {
+		return "snapshot (" . join(' ', map $cgi->a(
+			{-href => href(action=>"snapshot", hash=>$hash, snapshot_format=>$_)}, "$_"),
+			@snapshot_fmts)
+		. ")";
+	} else {
+		return undef;
+	}
+}
+
 ## ----------------------------------------------------------------------
 ## git utility subroutines, invoking git commands
 
@@ -3280,8 +3294,6 @@ sub git_shortlog_body {
 	# uses global variable $project
 	my ($commitlist, $from, $to, $refs, $extra) = @_;
 
-	my $have_snapshot = gitweb_have_snapshot();
-
 	$from = 0 unless defined $from;
 	$to = $#{$commitlist} if (!defined $to || $#{$commitlist} < $to);
 
@@ -3308,8 +3320,9 @@ sub git_shortlog_body {
 		      $cgi->a({-href => href(action=>"commit", hash=>$commit)}, "commit") . " | " .
 		      $cgi->a({-href => href(action=>"commitdiff", hash=>$commit)}, "commitdiff") . " | " .
 		      $cgi->a({-href => href(action=>"tree", hash=>$commit, hash_base=>$commit)}, "tree");
-		if ($have_snapshot) {
-			print " | " . $cgi->a({-href => href(action=>"snapshot", hash=>$commit)}, "snapshot");
+		my $snapshot_links = format_snapshot_links($commit);
+		if (defined $snapshot_links) {
+			print " | " . $snapshot_links;
 		}
 		print "</td>\n" .
 		      "</tr>\n";
@@ -4194,11 +4207,16 @@ sub git_tree {
 }
 
 sub git_snapshot {
-	my ($ctype, $suffix, $command) = gitweb_check_feature('snapshot');
-	my $have_snapshot = (defined $ctype && defined $suffix);
-	if (!$have_snapshot) {
-		die_error('403 Permission denied', "Permission denied");
+	my @supported_fmts = gitweb_check_feature('snapshot');
+
+	my $format = $cgi->param('sf');
+	unless ($format =~ m/[a-z0-9]+/
+		&& exists($known_snapshot_formats{$format})
+		&& grep($_ eq $format, @supported_fmts)) {
+		die_error(undef, "Unsupported snapshot format");
 	}
+	my ($ctype, $suffix, $ga_format, $pipe_compressor) =
+		@{$known_snapshot_formats{$format}};
 
 	if (!defined $hash) {
 		$hash = git_get_head_hash($project);
@@ -4211,16 +4229,11 @@ sub git_snapshot {
 	my $filename = to_utf8($name);
 	$name =~ s/\047/\047\\\047\047/g;
 	my $cmd;
-	if ($suffix eq 'zip') {
-		$filename .= "-$hash.$suffix";
-		$cmd = "$git archive --format=zip --prefix=\'$name\'/ $hash";
-	} else {
-		$filename .= "-$hash.tar.$suffix";
-		$cmd = "$git archive --format=tar --prefix=\'$name\'/ $hash | $command";
-	}
+	$filename .= "-$hash$suffix";
+	$cmd = "$git archive --format=$ga_format --prefix=\'$name\'/ $hash $pipe_compressor";
 
 	print $cgi->header(
-		-type => "application/$ctype",
+		-type => "$ctype",
 		-content_disposition => 'inline; filename="' . "$filename" . '"',
 		-status => '200 OK');
 
-- 
1.5.2.2.552.gc32f

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

[Junio C Hamano]

Matt McCutchen <hashproduct@gmail.com> writes:

> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index dbfb044..f36428e 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -101,6 +101,15 @@ our $mimetypes_file = undef;
>  # could be even 'utf-8' for the old behavior)
>  our $fallback_encoding = 'latin1';
>  
> +# information about snapshot formats that gitweb is capable of serving
> +# name => [mime type, filename suffix, --format for git-archive,
> +#          compressor command suffix]
> +our %known_snapshot_formats = (
> +	'tgz'  => ['application/x-gzip' , '.tar.gz' , 'tar', '| gzip' ],
> +	'tbz2' => ['application/x-bzip2', '.tar.bz2', 'tar', '| bzip2'],
> +	'zip'  => ['application/zip'    , '.zip'    , 'zip', ''       ],
> +);
> +
>  # You define site-wide feature defaults here; override them with
>  # $GITWEB_CONFIG as necessary.
>  our %feature = (
> @@ -131,20 +140,22 @@ our %feature = (
> ...
>  	'snapshot' => {
>  		'sub' => \&feature_snapshot,
>  		'override' => 0,
> -		#         => [content-encoding, suffix, program]
> -		'default' => ['x-gzip', 'gz', 'gzip']},
> +		'default' => ['tgz']},
>  
>  	# Enable text search, which will list the commits which match author,
>  	# committer or commit text to a given string.  Enabled by default.

This is a very nice clean-up, and I agree we should go this
route in the longer term.

This however will break people's existing gitweb configuration,
so if we were to do this it should be post 1.5.3, I would say.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> This however will break people's existing gitweb configuration,
> so if we were to do this it should be post 1.5.3, I would say.

We have swallowed some changes that breaks details of user
experience so far, and compared to one of them, the
incompatibility this brings in is much more benign.  We haven't
declared -rc1 when the command set and features for the next
release is cast in stone.

I am tempted to change my mind and am inclined to apply this.

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

[Junio C Hamano]

Matt McCutchen <hashproduct@gmail.com> writes:

> -sub gitweb_have_snapshot {
> -	my ($ctype, $suffix, $command) = gitweb_check_feature('snapshot');
> -	my $have_snapshot = (defined $ctype && defined $suffix);
> -
> -	return $have_snapshot;

Although you are removing this function, you still have a couple
of callers left in the code.

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

[Matt McCutchen]

On 7/8/07, Junio C Hamano <gitster@pobox.com> wrote:
> Matt McCutchen <hashproduct@gmail.com> writes:
>
> > -sub gitweb_have_snapshot {
> > -     my ($ctype, $suffix, $command) = gitweb_check_feature('snapshot');
> > -     my $have_snapshot = (defined $ctype && defined $suffix);
> > -
> > -     return $have_snapshot;
>
> Although you are removing this function, you still have a couple
> of callers left in the code.

OK, I will revise the patch, submit it and see if I can get it to
appear as a reply to this thread.  Incidentally, when only one format
is offered, would you prefer the snapshot link to appear as
"_snapshot_" (the same as before) or "_snapshot (tgz)_" instead of the
"snapshot (_tgz_)" that the current patch does?

Matt

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

[Matt McCutchen]

- Centralize knowledge about snapshot formats (mime types, extensions,
  commands) in %known_snapshot_formats and improve how some of that
  information is specified.  In particular, zip files are no longer a
  special case.

- Add support for offering multiple snapshot formats to the user so
  that he/she can download a snapshot in the format he/she prefers.
  The site-wide or project configuration now gives a list of formats
  to offer, and the "_snapshot_" link is replaced with, say,
  "snapshot (_tbz2_ _zip_)".

- Fix out-of-date "tarball" -> "archive" in comment.

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.

Signed-off-by: Matt McCutchen <hashproduct@gmail.com>
---
This revised patch converts the two remaining gitweb_have_snapshot callers and
adds the alert in the commit message.

 gitweb/gitweb.perl |  106 ++++++++++++++++++++++++++++------------------------
 1 files changed, 57 insertions(+), 49 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index dc609f4..4313671 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -101,6 +101,15 @@ our $mimetypes_file = undef;
 # could be even 'utf-8' for the old behavior)
 our $fallback_encoding = 'latin1';
 
+# information about snapshot formats that gitweb is capable of serving
+# name => [mime type, filename suffix, --format for git-archive,
+#          compressor command suffix]
+our %known_snapshot_formats = (
+	'tgz'  => ['application/x-gzip' , '.tar.gz' , 'tar', '| gzip' ],
+	'tbz2' => ['application/x-bzip2', '.tar.bz2', 'tar', '| bzip2'],
+	'zip'  => ['application/zip'    , '.zip'    , 'zip', ''       ],
+);
+
 # You define site-wide feature defaults here; override them with
 # $GITWEB_CONFIG as necessary.
 our %feature = (
@@ -131,20 +140,22 @@ our %feature = (
 		'override' => 0,
 		'default' => [0]},
 
-	# Enable the 'snapshot' link, providing a compressed tarball of any
+	# Enable the 'snapshot' link, providing a compressed archive of any
 	# tree. This can potentially generate high traffic if you have large
 	# project.
 
+	# Value is a list of formats defined in %known_snapshot_formats that
+	# you wish to offer.
 	# To disable system wide have in $GITWEB_CONFIG
-	# $feature{'snapshot'}{'default'} = [undef];
+	# $feature{'snapshot'}{'default'} = [];
 	# To have project specific config enable override in $GITWEB_CONFIG
 	# $feature{'snapshot'}{'override'} = 1;
-	# and in project config gitweb.snapshot = none|gzip|bzip2|zip;
+	# and in project config, a comma-separated list of formats or "none"
+	# to disable.  Example: gitweb.snapshot = tbz2,zip;
 	'snapshot' => {
 		'sub' => \&feature_snapshot,
 		'override' => 0,
-		#         => [content-encoding, suffix, program]
-		'default' => ['x-gzip', 'gz', 'gzip']},
+		'default' => ['tgz']},
 
 	# Enable text search, which will list the commits which match author,
 	# committer or commit text to a given string.  Enabled by default.
@@ -243,28 +254,15 @@ sub feature_blame {
 }
 
 sub feature_snapshot {
-	my ($ctype, $suffix, $command) = @_;
+	my (@fmts) = @_;
 
 	my ($val) = git_get_project_config('snapshot');
 
-	if ($val eq 'gzip') {
-		return ('x-gzip', 'gz', 'gzip');
-	} elsif ($val eq 'bzip2') {
-		return ('x-bzip2', 'bz2', 'bzip2');
-	} elsif ($val eq 'zip') {
-		return ('x-zip', 'zip', '');
-	} elsif ($val eq 'none') {
-		return ();
+	if ($val) {
+		@fmts = ($val eq 'none' ? () : split /,/, $val);
 	}
 
-	return ($ctype, $suffix, $command);
-}
-
-sub gitweb_have_snapshot {
-	my ($ctype, $suffix, $command) = gitweb_check_feature('snapshot');
-	my $have_snapshot = (defined $ctype && defined $suffix);
-
-	return $have_snapshot;
+	return grep exists $known_snapshot_formats{$_}, @fmts;
 }
 
 sub feature_grep {
@@ -542,6 +540,7 @@ sub href(%) {
 		order => "o",
 		searchtext => "s",
 		searchtype => "st",
+		snapshot_format => "sf",
 	);
 	my %mapping = @mapping;
 
@@ -1236,6 +1235,21 @@ sub format_diff_line {
 	return "<div class=\"diff$diff_class\">" . esc_html($line, -nbsp=>1) . "</div>\n";
 }
 
+# Generates undef or something like "snapshot (tbz2 zip)", linked.
+# Pass the hash.
+sub format_snapshot_links {
+	my ($hash) = @_;
+	my @snapshot_fmts = gitweb_check_feature('snapshot');
+	if (@snapshot_fmts) {
+		return "snapshot (" . join(' ', map $cgi->a(
+			{-href => href(action=>"snapshot", hash=>$hash, snapshot_format=>$_)}, "$_"),
+			@snapshot_fmts)
+		. ")";
+	} else {
+		return undef;
+	}
+}
+
 ## ----------------------------------------------------------------------
 ## git utility subroutines, invoking git commands
 
@@ -3299,8 +3313,6 @@ sub git_shortlog_body {
 	# uses global variable $project
 	my ($commitlist, $from, $to, $refs, $extra) = @_;
 
-	my $have_snapshot = gitweb_have_snapshot();
-
 	$from = 0 unless defined $from;
 	$to = $#{$commitlist} if (!defined $to || $#{$commitlist} < $to);
 
@@ -3327,8 +3339,9 @@ sub git_shortlog_body {
 		      $cgi->a({-href => href(action=>"commit", hash=>$commit)}, "commit") . " | " .
 		      $cgi->a({-href => href(action=>"commitdiff", hash=>$commit)}, "commitdiff") . " | " .
 		      $cgi->a({-href => href(action=>"tree", hash=>$commit, hash_base=>$commit)}, "tree");
-		if ($have_snapshot) {
-			print " | " . $cgi->a({-href => href(action=>"snapshot", hash=>$commit)}, "snapshot");
+		my $snapshot_links = format_snapshot_links($commit);
+		if (defined $snapshot_links) {
+			print " | " . $snapshot_links;
 		}
 		print "</td>\n" .
 		      "</tr>\n";
@@ -4110,8 +4123,6 @@ sub git_blob {
 }
 
 sub git_tree {
-	my $have_snapshot = gitweb_have_snapshot();
-
 	if (!defined $hash_base) {
 		$hash_base = "HEAD";
 	}
@@ -4145,11 +4156,10 @@ sub git_tree {
 				                       hash_base=>"HEAD", file_name=>$file_name)},
 				        "HEAD"),
 		}
-		if ($have_snapshot) {
+		my $snapshot_links = format_snapshot_links($hash);
+		if (defined $snapshot_links) {
 			# FIXME: Should be available when we have no hash base as well.
-			push @views_nav,
-				$cgi->a({-href => href(action=>"snapshot", hash=>$hash)},
-				        "snapshot");
+			push @views_nav, $snapshot_links;
 		}
 		git_print_page_nav('tree','', $hash_base, undef, undef, join(' | ', @views_nav));
 		git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash_base);
@@ -4213,11 +4223,16 @@ sub git_tree {
 }
 
 sub git_snapshot {
-	my ($ctype, $suffix, $command) = gitweb_check_feature('snapshot');
-	my $have_snapshot = (defined $ctype && defined $suffix);
-	if (!$have_snapshot) {
-		die_error('403 Permission denied', "Permission denied");
+	my @supported_fmts = gitweb_check_feature('snapshot');
+
+	my $format = $cgi->param('sf');
+	unless ($format =~ m/[a-z0-9]+/
+		&& exists($known_snapshot_formats{$format})
+		&& grep($_ eq $format, @supported_fmts)) {
+		die_error(undef, "Unsupported snapshot format");
 	}
+	my ($ctype, $suffix, $ga_format, $pipe_compressor) =
+		@{$known_snapshot_formats{$format}};
 
 	if (!defined $hash) {
 		$hash = git_get_head_hash($project);
@@ -4230,16 +4245,11 @@ sub git_snapshot {
 	my $filename = to_utf8($name);
 	$name =~ s/\047/\047\\\047\047/g;
 	my $cmd;
-	if ($suffix eq 'zip') {
-		$filename .= "-$hash.$suffix";
-		$cmd = "$git archive --format=zip --prefix=\'$name\'/ $hash";
-	} else {
-		$filename .= "-$hash.tar.$suffix";
-		$cmd = "$git archive --format=tar --prefix=\'$name\'/ $hash | $command";
-	}
+	$filename .= "-$hash$suffix";
+	$cmd = "$git archive --format=$ga_format --prefix=\'$name\'/ $hash $pipe_compressor";
 
 	print $cgi->header(
-		-type => "application/$ctype",
+		-type => "$ctype",
 		-content_disposition => 'inline; filename="' . "$filename" . '"',
 		-status => '200 OK');
 
@@ -4368,8 +4378,6 @@ sub git_commit {
 	my $refs = git_get_references();
 	my $ref = format_ref_marker($refs, $co{'id'});
 
-	my $have_snapshot = gitweb_have_snapshot();
-
 	git_header_html(undef, $expires);
 	git_print_page_nav('commit', '',
 	                   $hash, $co{'tree'}, $hash,
@@ -4408,9 +4416,9 @@ sub git_commit {
 	      "<td class=\"link\">" .
 	      $cgi->a({-href => href(action=>"tree", hash=>$co{'tree'}, hash_base=>$hash)},
 	              "tree");
-	if ($have_snapshot) {
-		print " | " .
-		      $cgi->a({-href => href(action=>"snapshot", hash=>$hash)}, "snapshot");
+	my $snapshot_links = format_snapshot_links($hash);
+	if (defined $snapshot_links) {
+		print " | " . $snapshot_links;
 	}
 	print "</td>" .
 	      "</tr>\n";
-- 
1.5.3.rc0.82.g2bad2-dirty

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

[Junio C Hamano]

"Matt McCutchen" <hashproduct@gmail.com> writes:

> On 7/8/07, Junio C Hamano <gitster@pobox.com> wrote:
>> Matt McCutchen <hashproduct@gmail.com> writes:
>>
>> > -sub gitweb_have_snapshot {
>> > -     my ($ctype, $suffix, $command) = gitweb_check_feature('snapshot');
>> > -     my $have_snapshot = (defined $ctype && defined $suffix);
>> > -
>> > -     return $have_snapshot;
>>
>> Although you are removing this function, you still have a couple
>> of callers left in the code.
>
> OK, I will revise the patch, submit it and see if I can get it to
> appear as a reply to this thread.

Thanks.  For future reference, I caught it not by code
inspection, but by running one of the tests (t9500).

> Incidentally, when only one format
> is offered, would you prefer the snapshot link to appear as
> "_snapshot_" (the same as before) or "_snapshot (tgz)_" instead of the
> "snapshot (_tgz_)" that the current patch does?

When only one format is offerred by the site, it is not like the
end user has any choice, so "_snapshot_" is probably the most
appropriate from the screen real-estate point-of-view.

The end user _might_ complain "Geez, I cannot grok zip, I can
only expand tar.  I would not have clicked the link if it said
'snapshot (_zip_)', but the stupid gitweb said '_snapshot_' and
nothing else."  So in that sense, we are robbing one choice the
user has (i.e. "to decide not to click the link, based on the
format of the data that would be given"), but I do not think
that is something we would seriously want to worry about.

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

[Matt McCutchen]

- Centralize knowledge about snapshot formats (mime types, extensions,
  commands) in %known_snapshot_formats and improve how some of that
  information is specified.  In particular, zip files are no longer a
  special case.

- Add support for offering multiple snapshot formats to the user so
  that he/she can download a snapshot in the format he/she prefers.
  The site-wide or project configuration now gives a list of formats
  to offer, and if more than one format is offered, the "_snapshot_"
  link becomes something like "snapshot (_tbz2_ _zip_)".

- Fix out-of-date "tarball" -> "archive" in comment.

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.

Signed-off-by: Matt McCutchen <hashproduct@gmail.com>
---
This third revision of the patch keeps the link as "_snapshot_" when only one
format is offered.

 gitweb/gitweb.perl |  110 +++++++++++++++++++++++++++++-----------------------
 1 files changed, 61 insertions(+), 49 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index dc609f4..b520342 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -101,6 +101,15 @@ our $mimetypes_file = undef;
 # could be even 'utf-8' for the old behavior)
 our $fallback_encoding = 'latin1';
 
+# information about snapshot formats that gitweb is capable of serving
+# name => [mime type, filename suffix, --format for git-archive,
+#          compressor command suffix]
+our %known_snapshot_formats = (
+	'tgz'  => ['application/x-gzip' , '.tar.gz' , 'tar', '| gzip' ],
+	'tbz2' => ['application/x-bzip2', '.tar.bz2', 'tar', '| bzip2'],
+	'zip'  => ['application/zip'    , '.zip'    , 'zip', ''       ],
+);
+
 # You define site-wide feature defaults here; override them with
 # $GITWEB_CONFIG as necessary.
 our %feature = (
@@ -131,20 +140,22 @@ our %feature = (
 		'override' => 0,
 		'default' => [0]},
 
-	# Enable the 'snapshot' link, providing a compressed tarball of any
+	# Enable the 'snapshot' link, providing a compressed archive of any
 	# tree. This can potentially generate high traffic if you have large
 	# project.
 
+	# Value is a list of formats defined in %known_snapshot_formats that
+	# you wish to offer.
 	# To disable system wide have in $GITWEB_CONFIG
-	# $feature{'snapshot'}{'default'} = [undef];
+	# $feature{'snapshot'}{'default'} = [];
 	# To have project specific config enable override in $GITWEB_CONFIG
 	# $feature{'snapshot'}{'override'} = 1;
-	# and in project config gitweb.snapshot = none|gzip|bzip2|zip;
+	# and in project config, a comma-separated list of formats or "none"
+	# to disable.  Example: gitweb.snapshot = tbz2,zip;
 	'snapshot' => {
 		'sub' => \&feature_snapshot,
 		'override' => 0,
-		#         => [content-encoding, suffix, program]
-		'default' => ['x-gzip', 'gz', 'gzip']},
+		'default' => ['tgz']},
 
 	# Enable text search, which will list the commits which match author,
 	# committer or commit text to a given string.  Enabled by default.
@@ -243,28 +254,15 @@ sub feature_blame {
 }
 
 sub feature_snapshot {
-	my ($ctype, $suffix, $command) = @_;
+	my (@fmts) = @_;
 
 	my ($val) = git_get_project_config('snapshot');
 
-	if ($val eq 'gzip') {
-		return ('x-gzip', 'gz', 'gzip');
-	} elsif ($val eq 'bzip2') {
-		return ('x-bzip2', 'bz2', 'bzip2');
-	} elsif ($val eq 'zip') {
-		return ('x-zip', 'zip', '');
-	} elsif ($val eq 'none') {
-		return ();
+	if ($val) {
+		@fmts = ($val eq 'none' ? () : split /,/, $val);
 	}
 
-	return ($ctype, $suffix, $command);
-}
-
-sub gitweb_have_snapshot {
-	my ($ctype, $suffix, $command) = gitweb_check_feature('snapshot');
-	my $have_snapshot = (defined $ctype && defined $suffix);
-
-	return $have_snapshot;
+	return grep exists $known_snapshot_formats{$_}, @fmts;
 }
 
 sub feature_grep {
@@ -542,6 +540,7 @@ sub href(%) {
 		order => "o",
 		searchtext => "s",
 		searchtype => "st",
+		snapshot_format => "sf",
 	);
 	my %mapping = @mapping;
 
@@ -1236,6 +1235,25 @@ sub format_diff_line {
 	return "<div class=\"diff$diff_class\">" . esc_html($line, -nbsp=>1) . "</div>\n";
 }
 
+# Generates undef or something like "snapshot (tbz2 zip)", linked.
+# Pass the hash.
+sub format_snapshot_links {
+	my ($hash) = @_;
+	my @snapshot_fmts = gitweb_check_feature('snapshot');
+	my $num_fmts = @snapshot_fmts;
+	if ($num_fmts > 1) {
+		return "snapshot (" . join(' ', map $cgi->a(
+			{-href => href(action=>"snapshot", hash=>$hash, snapshot_format=>$_)}, "$_"),
+			@snapshot_fmts)
+		. ")";
+	} elsif ($num_fmts == 1) {
+		return $cgi->a({-href => href(action=>"snapshot", hash=>$hash,
+			snapshot_format=>$snapshot_fmts[0])}, "snapshot");
+	} else { # $num_fmts == 0
+		return undef;
+	}
+}
+
 ## ----------------------------------------------------------------------
 ## git utility subroutines, invoking git commands
 
@@ -3299,8 +3317,6 @@ sub git_shortlog_body {
 	# uses global variable $project
 	my ($commitlist, $from, $to, $refs, $extra) = @_;
 
-	my $have_snapshot = gitweb_have_snapshot();
-
 	$from = 0 unless defined $from;
 	$to = $#{$commitlist} if (!defined $to || $#{$commitlist} < $to);
 
@@ -3327,8 +3343,9 @@ sub git_shortlog_body {
 		      $cgi->a({-href => href(action=>"commit", hash=>$commit)}, "commit") . " | " .
 		      $cgi->a({-href => href(action=>"commitdiff", hash=>$commit)}, "commitdiff") . " | " .
 		      $cgi->a({-href => href(action=>"tree", hash=>$commit, hash_base=>$commit)}, "tree");
-		if ($have_snapshot) {
-			print " | " . $cgi->a({-href => href(action=>"snapshot", hash=>$commit)}, "snapshot");
+		my $snapshot_links = format_snapshot_links($commit);
+		if (defined $snapshot_links) {
+			print " | " . $snapshot_links;
 		}
 		print "</td>\n" .
 		      "</tr>\n";
@@ -4110,8 +4127,6 @@ sub git_blob {
 }
 
 sub git_tree {
-	my $have_snapshot = gitweb_have_snapshot();
-
 	if (!defined $hash_base) {
 		$hash_base = "HEAD";
 	}
@@ -4145,11 +4160,10 @@ sub git_tree {
 				                       hash_base=>"HEAD", file_name=>$file_name)},
 				        "HEAD"),
 		}
-		if ($have_snapshot) {
+		my $snapshot_links = format_snapshot_links($hash);
+		if (defined $snapshot_links) {
 			# FIXME: Should be available when we have no hash base as well.
-			push @views_nav,
-				$cgi->a({-href => href(action=>"snapshot", hash=>$hash)},
-				        "snapshot");
+			push @views_nav, $snapshot_links;
 		}
 		git_print_page_nav('tree','', $hash_base, undef, undef, join(' | ', @views_nav));
 		git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash_base);
@@ -4213,11 +4227,16 @@ sub git_tree {
 }
 
 sub git_snapshot {
-	my ($ctype, $suffix, $command) = gitweb_check_feature('snapshot');
-	my $have_snapshot = (defined $ctype && defined $suffix);
-	if (!$have_snapshot) {
-		die_error('403 Permission denied', "Permission denied");
+	my @supported_fmts = gitweb_check_feature('snapshot');
+
+	my $format = $cgi->param('sf');
+	unless ($format =~ m/[a-z0-9]+/
+		&& exists($known_snapshot_formats{$format})
+		&& grep($_ eq $format, @supported_fmts)) {
+		die_error(undef, "Unsupported snapshot format");
 	}
+	my ($ctype, $suffix, $ga_format, $pipe_compressor) =
+		@{$known_snapshot_formats{$format}};
 
 	if (!defined $hash) {
 		$hash = git_get_head_hash($project);
@@ -4230,16 +4249,11 @@ sub git_snapshot {
 	my $filename = to_utf8($name);
 	$name =~ s/\047/\047\\\047\047/g;
 	my $cmd;
-	if ($suffix eq 'zip') {
-		$filename .= "-$hash.$suffix";
-		$cmd = "$git archive --format=zip --prefix=\'$name\'/ $hash";
-	} else {
-		$filename .= "-$hash.tar.$suffix";
-		$cmd = "$git archive --format=tar --prefix=\'$name\'/ $hash | $command";
-	}
+	$filename .= "-$hash$suffix";
+	$cmd = "$git archive --format=$ga_format --prefix=\'$name\'/ $hash $pipe_compressor";
 
 	print $cgi->header(
-		-type => "application/$ctype",
+		-type => "$ctype",
 		-content_disposition => 'inline; filename="' . "$filename" . '"',
 		-status => '200 OK');
 
@@ -4368,8 +4382,6 @@ sub git_commit {
 	my $refs = git_get_references();
 	my $ref = format_ref_marker($refs, $co{'id'});
 
-	my $have_snapshot = gitweb_have_snapshot();
-
 	git_header_html(undef, $expires);
 	git_print_page_nav('commit', '',
 	                   $hash, $co{'tree'}, $hash,
@@ -4408,9 +4420,9 @@ sub git_commit {
 	      "<td class=\"link\">" .
 	      $cgi->a({-href => href(action=>"tree", hash=>$co{'tree'}, hash_base=>$hash)},
 	              "tree");
-	if ($have_snapshot) {
-		print " | " .
-		      $cgi->a({-href => href(action=>"snapshot", hash=>$hash)}, "snapshot");
+	my $snapshot_links = format_snapshot_links($hash);
+	if (defined $snapshot_links) {
+		print " | " . $snapshot_links;
 	}
 	print "</td>" .
 	      "</tr>\n";
-- 
1.5.3.rc0.83.gb18a6

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

[Matt McCutchen]

- Centralize knowledge about snapshot formats (mime types, extensions,
  commands) in %known_snapshot_formats and improve how some of that
  information is specified.  In particular, zip files are no longer a
  special case.

- Add support for offering multiple snapshot formats to the user so
  that he/she can download a snapshot in the format he/she prefers.
  The site-wide or project configuration now gives a list of formats
  to offer, and if more than one format is offered, the "_snapshot_"
  link becomes something like "snapshot (_tbz2_ _zip_)".

- Fix out-of-date "tarball" -> "archive" in comment.

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.

Signed-off-by: Matt McCutchen <hashproduct@gmail.com>
---
This third revision of the patch keeps the link as "_snapshot_" when only one
format is offered.

 gitweb/gitweb.perl |  110 +++++++++++++++++++++++++++++-----------------------
 1 files changed, 61 insertions(+), 49 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index dc609f4..b520342 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -101,6 +101,15 @@ our $mimetypes_file = undef;
 # could be even 'utf-8' for the old behavior)
 our $fallback_encoding = 'latin1';
 
+# information about snapshot formats that gitweb is capable of serving
+# name => [mime type, filename suffix, --format for git-archive,
+#          compressor command suffix]
+our %known_snapshot_formats = (
+	'tgz'  => ['application/x-gzip' , '.tar.gz' , 'tar', '| gzip' ],
+	'tbz2' => ['application/x-bzip2', '.tar.bz2', 'tar', '| bzip2'],
+	'zip'  => ['application/zip'    , '.zip'    , 'zip', ''       ],
+);
+
 # You define site-wide feature defaults here; override them with
 # $GITWEB_CONFIG as necessary.
 our %feature = (
@@ -131,20 +140,22 @@ our %feature = (
 		'override' => 0,
 		'default' => [0]},
 
-	# Enable the 'snapshot' link, providing a compressed tarball of any
+	# Enable the 'snapshot' link, providing a compressed archive of any
 	# tree. This can potentially generate high traffic if you have large
 	# project.
 
+	# Value is a list of formats defined in %known_snapshot_formats that
+	# you wish to offer.
 	# To disable system wide have in $GITWEB_CONFIG
-	# $feature{'snapshot'}{'default'} = [undef];
+	# $feature{'snapshot'}{'default'} = [];
 	# To have project specific config enable override in $GITWEB_CONFIG
 	# $feature{'snapshot'}{'override'} = 1;
-	# and in project config gitweb.snapshot = none|gzip|bzip2|zip;
+	# and in project config, a comma-separated list of formats or "none"
+	# to disable.  Example: gitweb.snapshot = tbz2,zip;
 	'snapshot' => {
 		'sub' => \&feature_snapshot,
 		'override' => 0,
-		#         => [content-encoding, suffix, program]
-		'default' => ['x-gzip', 'gz', 'gzip']},
+		'default' => ['tgz']},
 
 	# Enable text search, which will list the commits which match author,
 	# committer or commit text to a given string.  Enabled by default.
@@ -243,28 +254,15 @@ sub feature_blame {
 }
 
 sub feature_snapshot {
-	my ($ctype, $suffix, $command) = @_;
+	my (@fmts) = @_;
 
 	my ($val) = git_get_project_config('snapshot');
 
-	if ($val eq 'gzip') {
-		return ('x-gzip', 'gz', 'gzip');
-	} elsif ($val eq 'bzip2') {
-		return ('x-bzip2', 'bz2', 'bzip2');
-	} elsif ($val eq 'zip') {
-		return ('x-zip', 'zip', '');
-	} elsif ($val eq 'none') {
-		return ();
+	if ($val) {
+		@fmts = ($val eq 'none' ? () : split /,/, $val);
 	}
 
-	return ($ctype, $suffix, $command);
-}
-
-sub gitweb_have_snapshot {
-	my ($ctype, $suffix, $command) = gitweb_check_feature('snapshot');
-	my $have_snapshot = (defined $ctype && defined $suffix);
-
-	return $have_snapshot;
+	return grep exists $known_snapshot_formats{$_}, @fmts;
 }
 
 sub feature_grep {
@@ -542,6 +540,7 @@ sub href(%) {
 		order => "o",
 		searchtext => "s",
 		searchtype => "st",
+		snapshot_format => "sf",
 	);
 	my %mapping = @mapping;
 
@@ -1236,6 +1235,25 @@ sub format_diff_line {
 	return "<div class=\"diff$diff_class\">" . esc_html($line, -nbsp=>1) . "</div>\n";
 }
 
+# Generates undef or something like "snapshot (tbz2 zip)", linked.
+# Pass the hash.
+sub format_snapshot_links {
+	my ($hash) = @_;
+	my @snapshot_fmts = gitweb_check_feature('snapshot');
+	my $num_fmts = @snapshot_fmts;
+	if ($num_fmts > 1) {
+		return "snapshot (" . join(' ', map $cgi->a(
+			{-href => href(action=>"snapshot", hash=>$hash, snapshot_format=>$_)}, "$_"),
+			@snapshot_fmts)
+		. ")";
+	} elsif ($num_fmts == 1) {
+		return $cgi->a({-href => href(action=>"snapshot", hash=>$hash,
+			snapshot_format=>$snapshot_fmts[0])}, "snapshot");
+	} else { # $num_fmts == 0
+		return undef;
+	}
+}
+
 ## ----------------------------------------------------------------------
 ## git utility subroutines, invoking git commands
 
@@ -3299,8 +3317,6 @@ sub git_shortlog_body {
 	# uses global variable $project
 	my ($commitlist, $from, $to, $refs, $extra) = @_;
 
-	my $have_snapshot = gitweb_have_snapshot();
-
 	$from = 0 unless defined $from;
 	$to = $#{$commitlist} if (!defined $to || $#{$commitlist} < $to);
 
@@ -3327,8 +3343,9 @@ sub git_shortlog_body {
 		      $cgi->a({-href => href(action=>"commit", hash=>$commit)}, "commit") . " | " .
 		      $cgi->a({-href => href(action=>"commitdiff", hash=>$commit)}, "commitdiff") . " | " .
 		      $cgi->a({-href => href(action=>"tree", hash=>$commit, hash_base=>$commit)}, "tree");
-		if ($have_snapshot) {
-			print " | " . $cgi->a({-href => href(action=>"snapshot", hash=>$commit)}, "snapshot");
+		my $snapshot_links = format_snapshot_links($commit);
+		if (defined $snapshot_links) {
+			print " | " . $snapshot_links;
 		}
 		print "</td>\n" .
 		      "</tr>\n";
@@ -4110,8 +4127,6 @@ sub git_blob {
 }
 
 sub git_tree {
-	my $have_snapshot = gitweb_have_snapshot();
-
 	if (!defined $hash_base) {
 		$hash_base = "HEAD";
 	}
@@ -4145,11 +4160,10 @@ sub git_tree {
 				                       hash_base=>"HEAD", file_name=>$file_name)},
 				        "HEAD"),
 		}
-		if ($have_snapshot) {
+		my $snapshot_links = format_snapshot_links($hash);
+		if (defined $snapshot_links) {
 			# FIXME: Should be available when we have no hash base as well.
-			push @views_nav,
-				$cgi->a({-href => href(action=>"snapshot", hash=>$hash)},
-				        "snapshot");
+			push @views_nav, $snapshot_links;
 		}
 		git_print_page_nav('tree','', $hash_base, undef, undef, join(' | ', @views_nav));
 		git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash_base);
@@ -4213,11 +4227,16 @@ sub git_tree {
 }
 
 sub git_snapshot {
-	my ($ctype, $suffix, $command) = gitweb_check_feature('snapshot');
-	my $have_snapshot = (defined $ctype && defined $suffix);
-	if (!$have_snapshot) {
-		die_error('403 Permission denied', "Permission denied");
+	my @supported_fmts = gitweb_check_feature('snapshot');
+
+	my $format = $cgi->param('sf');
+	unless ($format =~ m/[a-z0-9]+/
+		&& exists($known_snapshot_formats{$format})
+		&& grep($_ eq $format, @supported_fmts)) {
+		die_error(undef, "Unsupported snapshot format");
 	}
+	my ($ctype, $suffix, $ga_format, $pipe_compressor) =
+		@{$known_snapshot_formats{$format}};
 
 	if (!defined $hash) {
 		$hash = git_get_head_hash($project);
@@ -4230,16 +4249,11 @@ sub git_snapshot {
 	my $filename = to_utf8($name);
 	$name =~ s/\047/\047\\\047\047/g;
 	my $cmd;
-	if ($suffix eq 'zip') {
-		$filename .= "-$hash.$suffix";
-		$cmd = "$git archive --format=zip --prefix=\'$name\'/ $hash";
-	} else {
-		$filename .= "-$hash.tar.$suffix";
-		$cmd = "$git archive --format=tar --prefix=\'$name\'/ $hash | $command";
-	}
+	$filename .= "-$hash$suffix";
+	$cmd = "$git archive --format=$ga_format --prefix=\'$name\'/ $hash $pipe_compressor";
 
 	print $cgi->header(
-		-type => "application/$ctype",
+		-type => "$ctype",
 		-content_disposition => 'inline; filename="' . "$filename" . '"',
 		-status => '200 OK');
 
@@ -4368,8 +4382,6 @@ sub git_commit {
 	my $refs = git_get_references();
 	my $ref = format_ref_marker($refs, $co{'id'});
 
-	my $have_snapshot = gitweb_have_snapshot();
-
 	git_header_html(undef, $expires);
 	git_print_page_nav('commit', '',
 	                   $hash, $co{'tree'}, $hash,
@@ -4408,9 +4420,9 @@ sub git_commit {
 	      "<td class=\"link\">" .
 	      $cgi->a({-href => href(action=>"tree", hash=>$co{'tree'}, hash_base=>$hash)},
 	              "tree");
-	if ($have_snapshot) {
-		print " | " .
-		      $cgi->a({-href => href(action=>"snapshot", hash=>$hash)}, "snapshot");
+	my $snapshot_links = format_snapshot_links($hash);
+	if (defined $snapshot_links) {
+		print " | " . $snapshot_links;
 	}
 	print "</td>" .
 	      "</tr>\n";
-- 
1.5.3.rc0.83.gb18a6

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

[Jakub Narebski]

Matt McCutchen wrote:

> - Centralize knowledge about snapshot formats (mime types, extensions,
>   commands) in %known_snapshot_formats and improve how some of that
>   information is specified.  In particular, zip files are no longer a
>   special case.
> 
> - Add support for offering multiple snapshot formats to the user so
>   that he/she can download a snapshot in the format he/she prefers.
>   The site-wide or project configuration now gives a list of formats
>   to offer, and the "_snapshot_" link is replaced with, say,
>   "snapshot (_tbz2_ _zip_)".
> 
> - Fix out-of-date "tarball" -> "archive" in comment.
> 
> 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.

Quite nice and I think needed refactoring of a snapshot code. Nevertheless
I have some comments on the changes introduced by this patch; not only
change to gitweb_config.perl is needed (which gitweb admin has control
over), but also repo config for individual repositories might need to
be changed (which gitweb admin might not have control over, and which is
much harder to do).

> +# information about snapshot formats that gitweb is capable of serving
> +# name => [mime type, filename suffix, --format for git-archive,
> +#          compressor command suffix]
> +our %known_snapshot_formats = (
> +     'tgz'  => ['application/x-gzip' , '.tar.gz' , 'tar', '| gzip' ],
> +     'tbz2' => ['application/x-bzip2', '.tar.bz2', 'tar', '| bzip2'],
> +     'zip'  => ['application/zip'    , '.zip'    , 'zip', ''       ],
> +);

First, is full mimetype really needed? Earlier code assumed that mimetype
for snapshot is of the form of application/<something>, and it provided
only <something>.

Second, I'd rather have 'gzip' and 'bzip2' aliases to 'tgz' and 'tbz2',
so the old config continues to work. I can see that it would be hard
to do without special-casing code, or changing the assumption that list
of default available snapshot formats is keys of above hash.

> -     # and in project config gitweb.snapshot = none|gzip|bzip2|zip;
> +     # and in project config, a comma-separated list of formats or "none"
> +     # to disable.  Example: gitweb.snapshot = tbz2,zip;

I would relax the syntax, so "tbz2, zip" would also work, or even
"tbz2 zip". I'd like for old config to also work, meaning that "gzip"
would be the same as "tgz" and "bzip2" as "tbz2".

> -     if ($val eq 'gzip') {
> -             return ('x-gzip', 'gz', 'gzip');
> -     } elsif ($val eq 'bzip2') {
> -             return ('x-bzip2', 'bz2', 'bzip2');
> -     } elsif ($val eq 'zip') {
> -             return ('x-zip', 'zip', '');
> -     } elsif ($val eq 'none') {
> -             return ();

Very nice getting rid of this swith-like statement...

> +     if ($val) {
> +             @fmts = ($val eq 'none' ? () : split /,/, $val);

... but I would relax this regexp.

> +# Generates undef or something like "snapshot (tbz2 zip)", linked.
> +# Pass the hash.
> +sub format_snapshot_links {
> +     my ($hash) = @_;
> +     my @snapshot_fmts = gitweb_check_feature('snapshot');
> +     if (@snapshot_fmts) {
> +             return "snapshot (" . join(' ', map $cgi->a(
> +                     {-href => href(action=>"snapshot", hash=>$hash, snapshot_format=>$_)}, "$_"),
> +                     @snapshot_fmts)
> +             . ")";
> +     } else {
> +             return undef;
> +     }
> +}

Nice separation into subroutine.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

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

[Jakub Narebski]

On Sat, 7 Jul 2007, Junio C Hamano wrote:
> Matt McCutchen <hashproduct@gmail.com> writes:

>> +# information about snapshot formats that gitweb is capable of serving
>> +# name => [mime type, filename suffix, --format for git-archive,
>> +#          compressor command suffix]
>> +our %known_snapshot_formats = (
>> +	'tgz'  => ['application/x-gzip' , '.tar.gz' , 'tar', '| gzip' ],
>> +	'tbz2' => ['application/x-bzip2', '.tar.bz2', 'tar', '| bzip2'],
>> +	'zip'  => ['application/zip'    , '.zip'    , 'zip', ''       ],
>> +);

> This is a very nice clean-up, and I agree we should go this
> route in the longer term.

I agree that is a nice cleanup.

I'm not sure if we want to store whole 'application/x-gzip' or only
'x-gzip' part of mime type, and if we want to store compressor as
'| gzip' or simply as 'gzip'.
 
> This however will break people's existing gitweb configuration,
> so if we were to do this it should be post 1.5.3, I would say.

This would break not only existing _gitweb_ configuration (when
gitweb admin installs new gitweb it isn't that hard to correct
gitweb config), but also git _repositories_ config: gitweb.snapshot
no longer work as it worked before, for example neither 'gzip'
nor 'bzip2' values work anymore ('zip' doesn't stop working).

-- 
Jakub Narebski
Poland

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> I'm not sure if we want to store whole 'application/x-gzip' or only
> 'x-gzip' part of mime type, and if we want to store compressor as
> '| gzip' or simply as 'gzip'.
>  
>> This however will break people's existing gitweb configuration,
>> so if we were to do this it should be post 1.5.3, I would say.
>
> This would break not only existing _gitweb_ configuration (when
> gitweb admin installs new gitweb it isn't that hard to correct
> gitweb config), but also git _repositories_ config: gitweb.snapshot
> no longer work as it worked before, for example neither 'gzip'
> nor 'bzip2' values work anymore ('zip' doesn't stop working).

I realized after seeing your other message on this patch that
this can be done while retaining backward compatibility, as you
suggested.  Matt, does Jakub's suggestion make sense to you?

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

[Matt McCutchen]

On 7/11/07, Junio C Hamano <gitster@pobox.com> wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
> > I'm not sure if we want to store whole 'application/x-gzip' or only
> > 'x-gzip' part of mime type, and if we want to store compressor as
> > '| gzip' or simply as 'gzip'.

Storing only 'x-gzip' assumes that all archive formats have MIME types
beginning with 'application/'.  Even if this assumption is justified
by the MIME specification, I felt it was inappropriate to code it into
gitweb.  The advantage of '| gzip' is that the lack of a compressor is
not a special case.  This is why I wrote %known_snapshot_formats the
way I did, but of course you all are welcome to overrule me.

> > This would break not only existing _gitweb_ configuration (when
> > gitweb admin installs new gitweb it isn't that hard to correct
> > gitweb config), but also git _repositories_ config: gitweb.snapshot
> > no longer work as it worked before, for example neither 'gzip'
> > nor 'bzip2' values work anymore ('zip' doesn't stop working).
>
> I realized after seeing your other message on this patch that
> this can be done while retaining backward compatibility, as you
> suggested.  Matt, does Jakub's suggestion make sense to you?

It's not clear to me what the suggestion is: offer format names 'gzip'
and 'bzip2' instead of 'tgz' and 'tbz2', or in addition to them, or
what?  I prefer 'tgz' and 'tbz2' because they carry more information
and are properly analogous to 'zip', so I don't want to offer 'gzip'
and 'bzip2' instead of them.  Furthermore, I would like the user to
see 'snapshot (tgz tbz2)' even if the repository owner wrote 'gzip
bzip2', so just adding two rows to %known_snapshot_formats is
insufficient.  Either an additional column could be added to
%known_snapshot_formats for the display name, or 'gzip' and 'bzip2'
could be specified as aliases in %known_snapshot_formats and
feature_snapshot could be taught to resolve them.  I would prefer the
second option; shall I implement it?

It would be possible to make the gitweb site configuration
backward-compatible too; here's one possible approach.  On startup,
gitweb would check whether $feature{'snapshot'}{'default'} is a
three-element list that appears to be in the old format.  If so, it
would save the settings in $known_snapshot_formats{'default'} and then
set $feature{'snapshot'}{'default'} = 'default' .  This is a hack; is
it justified by the compatibility benefit?

Matt

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

[Jakub Narebski]

On Thu, 12 July 2007, Matt McCutchen wrote:
> On 7/11/07, Junio C Hamano <gitster@pobox.com> wrote:
>> Jakub Narebski <jnareb@gmail.com> writes:
>>
>>> I'm not sure if we want to store whole 'application/x-gzip' or only
>>> 'x-gzip' part of mime type, and if we want to store compressor as
>>> '| gzip' or simply as 'gzip'.
> 
> Storing only 'x-gzip' assumes that all archive formats have MIME types
> beginning with 'application/'.  Even if this assumption is justified
> by the MIME specification, I felt it was inappropriate to code it into
> gitweb.  

Good argument. Besides, now we use it only in one place, but if we
would in the future use it in other place having full mimetype would
make it easier.

> The advantage of '| gzip' is that the lack of a compressor is 
> not a special case.  This is why I wrote %known_snapshot_formats the
> way I did, but of course you all are welcome to overrule me.

I wrote about this because I'm thinking about replacing the few 
pipelines we use in gitweb[*1*], including the snapshot one, with
the list form, which has the advantage of avoiding spawning the shell 
(performance) and not dealing with shell quoting of arguments (security 
and errors), like we did for simple calling of git commands to read 
from in the commit b9182987a80f7e820cbe1f8c7c4dc26f8586e8cd
  "gitweb: Use list for of open for running git commands, thorougly"

Thus I'd rather have list of extra commands and arguments instead of
pipe as a string, i.e. 'gzip' instead of '| gzip', and 'gzip', '-9'
instead of '| gzip -9'.

[*1*] We currently use pipelines for snapshots which need external 
compressor, like tgz and tbz2, and for pickaxe search.

>>> This would break not only existing _gitweb_ configuration (when
>>> gitweb admin installs new gitweb it isn't that hard to correct
>>> gitweb config), but also git _repositories_ config: gitweb.snapshot
>>> no longer work as it worked before, for example neither 'gzip'
>>> nor 'bzip2' values work anymore ('zip' doesn't stop working).
>>
>> I realized after seeing your other message on this patch that
>> this can be done while retaining backward compatibility, as you
>> suggested.  Matt, does Jakub's suggestion make sense to you?
> 
> It's not clear to me what the suggestion is: offer format names 'gzip'
> and 'bzip2' instead of 'tgz' and 'tbz2', or in addition to them, or
> what?  I prefer 'tgz' and 'tbz2' because they carry more information
> and are properly analogous to 'zip', so I don't want to offer 'gzip'
> and 'bzip2' instead of them.  Furthermore, I would like the user to
> see 'snapshot (tgz tbz2)' even if the repository owner wrote 'gzip
> bzip2', so just adding two rows to %known_snapshot_formats is
> insufficient.  Either an additional column could be added to
> %known_snapshot_formats for the display name, or 'gzip' and 'bzip2'
> could be specified as aliases in %known_snapshot_formats and
> feature_snapshot could be taught to resolve them.  I would prefer the
> second option; shall I implement it?

I also prefer the second option, perhaps as simple as 'gzip' => 'tbz'
and 'bzip2' => 'tbz2', and of course accompaning code to deal with this.

As to "display name" column: I'm not sure if for example 'tar.gz' 
instead of 'tgz' would be not easier to understand.

> It would be possible to make the gitweb site configuration
> backward-compatible too; here's one possible approach.  On startup,
> gitweb would check whether $feature{'snapshot'}{'default'} is a
> three-element list that appears to be in the old format.  If so, it
> would save the settings in $known_snapshot_formats{'default'} and then
> set $feature{'snapshot'}{'default'} = 'default' .  This is a hack; is
> it justified by the compatibility benefit?

If you implement 'gzip' and 'bzip2' as aliases, it could be as simple
as just taking last non-false element of array if the array has more
than one element. But I'm not sure if it is worth it.

But we should probably error out with some error message on 
incompatibile gitweb site configuration; I'm not sure...

-- 
Jakub Narebski
Poland

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

[Matt McCutchen]

On 7/12/07, Jakub Narebski <jnareb@gmail.com> wrote:
> > The advantage of '| gzip' is that the lack of a compressor is
> > not a special case.  This is why I wrote %known_snapshot_formats the
> > way I did, but of course you all are welcome to overrule me.
>
> I wrote about this because I'm thinking about replacing the few
> pipelines we use in gitweb[*1*], including the snapshot one, with
> the list form, which has the advantage of avoiding spawning the shell
> (performance) and not dealing with shell quoting of arguments (security
> and errors), like we did for simple calling of git commands to read
> from in the commit b9182987a80f7e820cbe1f8c7c4dc26f8586e8cd
>   "gitweb: Use list for of open for running git commands, thorougly"
>
> Thus I'd rather have list of extra commands and arguments instead of
> pipe as a string, i.e. 'gzip' instead of '| gzip', and 'gzip', '-9'
> instead of '| gzip -9'.
>
> [*1*] We currently use pipelines for snapshots which need external
> compressor, like tgz and tbz2, and for pickaxe search.

OK, I changed the extra commands to list form.  The field is now a
reference to the compressor argv like ['gzip'] or undef if there is no
compressor.

> I also prefer the second option, perhaps as simple as 'gzip' => 'tbz'
> and 'bzip2' => 'tbz2', and of course accompaning code to deal with this.
>
> As to "display name" column: I'm not sure if for example 'tar.gz'
> instead of 'tgz' would be not easier to understand.

I went ahead and added both aliases and display names (currently
'tar.gz', 'tar.bz2', and 'zip').  Aliases are resolved in
&feature_snapshot for repository configuration but not for side-wide
configuration because then aliases in side-wide configuration would be
resolved only if override is on, which would be weird.  For the same
reason, I changed the filtering out of unknown formats in
&feature_snapshot to apply only to repository configuration.

> > It would be possible to make the gitweb site configuration
> > backward-compatible too; here's one possible approach.  On startup,
> > gitweb would check whether $feature{'snapshot'}{'default'} is a
> > three-element list that appears to be in the old format.  If so, it
> > would save the settings in $known_snapshot_formats{'default'} and then
> > set $feature{'snapshot'}{'default'} = 'default' .  This is a hack; is
> > it justified by the compatibility benefit?
>
> If you implement 'gzip' and 'bzip2' as aliases, it could be as simple
> as just taking last non-false element of array if the array has more
> than one element.

No, because it must be possible for the site default to consist of
multiple formats.  It would be possible to take all elements that are
recognized as snapshot formats and ignore the others, but I would like
to be able to distinguish between "formats" that are part of a legacy
specification and completely bogus formats so that we can issue a
warning or error for the latter.

> But I'm not sure if it is worth it.

At this point I am leaning against backward compatibility for the site
configuration, and if I do implement it, I would just recognize the
three exact specifications that currently appear in feature_snapshot.
Recognizing other legacy specifications is iffy in the first place,
and handling them would require the hack I described.

I will send the revised patch soon.

Matt

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

[Matt McCutchen]

- Centralize knowledge about snapshot formats (mime types, extensions,
  commands) in %known_snapshot_formats and improve how some of that
  information is specified.  In particular, zip files are no longer a
  special case.

- Add support for offering multiple snapshot formats to the user so
  that he/she can download a snapshot in the format he/she prefers.
  The site-wide or project configuration now gives a list of formats
  to offer, and if more than one format is offered, the "_snapshot_"
  link becomes something like "snapshot (_tar.bz2_ _zip_)".

- If only one format is offered, a tooltip on the "_snapshot_" link
  tells the user what it is.

- Fix out-of-date "tarball" -> "archive" in comment.

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.

Signed-off-by: Matt McCutchen <hashproduct@gmail.com>
---

Changes since the previous revision of the patch:

- Added display names.
- Changed compressor command line to list form.
- Added compatibility format aliases for repository configuration.
- Tweaked filtering of unknown formats to apply only to repository
  configuration.
- Reformatted format_snapshot_links and added/modified comments to make it much
  easier to understand.
- When a single snapshot format is offered, added a tooltip showing the format
  to the "snapshot" link.  This helps Junio's hypothetical end user without
  using additional screen real estate.

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?

Is there anything else I need to do to the patch?  If not, how soon is
it likely to be committed?  I'm guessing I missed the boat on git 1.5.3.

Matt

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index c8ba3a2..f17c983 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -104,6 +104,22 @@ our $mimetypes_file = undef;
 # could be even 'utf-8' for the old behavior)
 our $fallback_encoding = 'latin1';
 
+# information about snapshot formats that gitweb is capable of serving
+# name => [display name, mime type, filename suffix, --format for git-archive,
+#          [compressor command and arguments] | undef]
+our %known_snapshot_formats = (
+	'tgz'  => ['tar.gz' , 'application/x-gzip' , '.tar.gz' , 'tar', ['gzip' ]],
+	'tbz2' => ['tar.bz2', 'application/x-bzip2', '.tar.bz2', 'tar', ['bzip2']],
+	'zip'  => ['zip',     'application/x-zip'  , '.zip'    , 'zip', undef    ],
+);
+
+# Aliases so we understand old gitweb.snapshot values in repository
+# configuration.
+our %known_snapshot_format_aliases = (
+	'gzip'  => 'tgz' ,
+	'bzip2' => 'tbz2',
+);
+
 # You define site-wide feature defaults here; override them with
 # $GITWEB_CONFIG as necessary.
 our %feature = (
@@ -134,20 +150,22 @@ our %feature = (
 		'override' => 0,
 		'default' => [0]},
 
-	# Enable the 'snapshot' link, providing a compressed tarball of any
+	# Enable the 'snapshot' link, providing a compressed archive of any
 	# tree. This can potentially generate high traffic if you have large
 	# project.
 
+	# Value is a list of formats defined in %known_snapshot_formats that
+	# you wish to offer.
 	# To disable system wide have in $GITWEB_CONFIG
-	# $feature{'snapshot'}{'default'} = [undef];
+	# $feature{'snapshot'}{'default'} = [];
 	# To have project specific config enable override in $GITWEB_CONFIG
 	# $feature{'snapshot'}{'override'} = 1;
-	# and in project config gitweb.snapshot = none|gzip|bzip2|zip;
+	# and in project config, a comma-separated list of formats or "none"
+	# to disable.  Example: gitweb.snapshot = tbz2,zip;
 	'snapshot' => {
 		'sub' => \&feature_snapshot,
 		'override' => 0,
-		#         => [content-encoding, suffix, program]
-		'default' => ['x-gzip', 'gz', 'gzip']},
+		'default' => ['tgz']},
 
 	# Enable text search, which will list the commits which match author,
 	# committer or commit text to a given string.  Enabled by default.
@@ -246,28 +264,17 @@ sub feature_blame {
 }
 
 sub feature_snapshot {
-	my ($ctype, $suffix, $command) = @_;
+	my (@fmts) = @_;
 
 	my ($val) = git_get_project_config('snapshot');
 
-	if ($val eq 'gzip') {
-		return ('x-gzip', 'gz', 'gzip');
-	} elsif ($val eq 'bzip2') {
-		return ('x-bzip2', 'bz2', 'bzip2');
-	} elsif ($val eq 'zip') {
-		return ('x-zip', 'zip', '');
-	} elsif ($val eq 'none') {
-		return ();
+	if ($val) {
+		@fmts = ($val eq 'none' ? () : split /,/, $val);
+		@fmts = map $known_snapshot_format_aliases{$_} || $_, @fmts;
+		@fmts = grep exists $known_snapshot_formats{$_}, @fmts;
 	}
 
-	return ($ctype, $suffix, $command);
-}
-
-sub gitweb_have_snapshot {
-	my ($ctype, $suffix, $command) = gitweb_check_feature('snapshot');
-	my $have_snapshot = (defined $ctype && defined $suffix);
-
-	return $have_snapshot;
+	return @fmts;
 }
 
 sub feature_grep {
@@ -563,6 +570,7 @@ sub href(%) {
 		order => "o",
 		searchtext => "s",
 		searchtype => "st",
+		snapshot_format => "sf",
 	);
 	my %mapping = @mapping;
 
@@ -1257,6 +1265,39 @@ sub format_diff_line {
 	return "<div class=\"diff$diff_class\">" . esc_html($line, -nbsp=>1) . "</div>\n";
 }
 
+# Generates undef or something like "_snapshot_" or "snapshot (_tbz2_ _zip_)",
+# linked.  Pass the hash of the tree/commit to snapshot.
+sub format_snapshot_links {
+	my ($hash) = @_;
+	my @snapshot_fmts = gitweb_check_feature('snapshot');
+	my $num_fmts = @snapshot_fmts;
+	if ($num_fmts > 1) {
+		# A parenthesized list of links bearing format names.
+		return "snapshot (" . join(' ', map
+			$cgi->a({
+				-href => href(
+					action=>"snapshot",
+					hash=>$hash,
+					snapshot_format=>$_
+				)
+			}, $known_snapshot_formats{$_}[0]) # the display name
+		, @snapshot_fmts) . ")";
+	} elsif ($num_fmts == 1) {
+		# A single "snapshot" link whose tooltip bears the format name.
+		my ($fmt) = @snapshot_fmts;
+		return $cgi->a({
+				-href => href(
+					action=>"snapshot",
+					hash=>$hash,
+					snapshot_format=>$fmt
+				),
+				-title => "in format: $known_snapshot_formats{$fmt}[0]" # the display name
+			}, "snapshot");
+	} else { # $num_fmts == 0
+		return undef;
+	}
+}
+
 ## ----------------------------------------------------------------------
 ## git utility subroutines, invoking git commands
 
@@ -3321,8 +3362,6 @@ sub git_shortlog_body {
 	# uses global variable $project
 	my ($commitlist, $from, $to, $refs, $extra) = @_;
 
-	my $have_snapshot = gitweb_have_snapshot();
-
 	$from = 0 unless defined $from;
 	$to = $#{$commitlist} if (!defined $to || $#{$commitlist} < $to);
 
@@ -3349,8 +3388,9 @@ sub git_shortlog_body {
 		      $cgi->a({-href => href(action=>"commit", hash=>$commit)}, "commit") . " | " .
 		      $cgi->a({-href => href(action=>"commitdiff", hash=>$commit)}, "commitdiff") . " | " .
 		      $cgi->a({-href => href(action=>"tree", hash=>$commit, hash_base=>$commit)}, "tree");
-		if ($have_snapshot) {
-			print " | " . $cgi->a({-href => href(action=>"snapshot", hash=>$commit)}, "snapshot");
+		my $snapshot_links = format_snapshot_links($commit);
+		if (defined $snapshot_links) {
+			print " | " . $snapshot_links;
 		}
 		print "</td>\n" .
 		      "</tr>\n";
@@ -4132,8 +4172,6 @@ sub git_blob {
 }
 
 sub git_tree {
-	my $have_snapshot = gitweb_have_snapshot();
-
 	if (!defined $hash_base) {
 		$hash_base = "HEAD";
 	}
@@ -4167,11 +4205,10 @@ sub git_tree {
 				                       hash_base=>"HEAD", file_name=>$file_name)},
 				        "HEAD"),
 		}
-		if ($have_snapshot) {
+		my $snapshot_links = format_snapshot_links($hash);
+		if (defined $snapshot_links) {
 			# FIXME: Should be available when we have no hash base as well.
-			push @views_nav,
-				$cgi->a({-href => href(action=>"snapshot", hash=>$hash)},
-				        "snapshot");
+			push @views_nav, $snapshot_links;
 		}
 		git_print_page_nav('tree','', $hash_base, undef, undef, join(' | ', @views_nav));
 		git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash_base);
@@ -4235,11 +4272,16 @@ sub git_tree {
 }
 
 sub git_snapshot {
-	my ($ctype, $suffix, $command) = gitweb_check_feature('snapshot');
-	my $have_snapshot = (defined $ctype && defined $suffix);
-	if (!$have_snapshot) {
-		die_error('403 Permission denied', "Permission denied");
+	my @supported_fmts = gitweb_check_feature('snapshot');
+
+	my $format = $cgi->param('sf');
+	unless ($format =~ m/[a-z0-9]+/
+		&& exists($known_snapshot_formats{$format})
+		&& grep($_ eq $format, @supported_fmts)) {
+		die_error(undef, "Unsupported snapshot format");
 	}
+	my ($dispname, $ctype, $suffix, $ga_format, $compressor_argv) =
+		@{$known_snapshot_formats{$format}};
 
 	if (!defined $hash) {
 		$hash = git_get_head_hash($project);
@@ -4252,16 +4294,14 @@ sub git_snapshot {
 	my $filename = to_utf8($name);
 	$name =~ s/\047/\047\\\047\047/g;
 	my $cmd;
-	if ($suffix eq 'zip') {
-		$filename .= "-$hash.$suffix";
-		$cmd = "$git archive --format=zip --prefix=\'$name\'/ $hash";
-	} else {
-		$filename .= "-$hash.tar.$suffix";
-		$cmd = "$git archive --format=tar --prefix=\'$name\'/ $hash | $command";
+	$filename .= "-$hash$suffix";
+	$cmd = "$git archive --format=$ga_format --prefix=\'$name\'/ $hash";
+	if (defined $compressor_argv) {
+		$cmd .= ' | ' . join ' ', @$compressor_argv;
 	}
 
 	print $cgi->header(
-		-type => "application/$ctype",
+		-type => "$ctype",
 		-content_disposition => 'inline; filename="' . "$filename" . '"',
 		-status => '200 OK');
 
@@ -4390,8 +4430,6 @@ sub git_commit {
 	my $refs = git_get_references();
 	my $ref = format_ref_marker($refs, $co{'id'});
 
-	my $have_snapshot = gitweb_have_snapshot();
-
 	git_header_html(undef, $expires);
 	git_print_page_nav('commit', '',
 	                   $hash, $co{'tree'}, $hash,
@@ -4430,9 +4468,9 @@ sub git_commit {
 	      "<td class=\"link\">" .
 	      $cgi->a({-href => href(action=>"tree", hash=>$co{'tree'}, hash_base=>$hash)},
 	              "tree");
-	if ($have_snapshot) {
-		print " | " .
-		      $cgi->a({-href => href(action=>"snapshot", hash=>$hash)}, "snapshot");
+	my $snapshot_links = format_snapshot_links($hash);
+	if (defined $snapshot_links) {
+		print " | " . $snapshot_links;
 	}
 	print "</td>" .
 	      "</tr>\n";
-- 
1.5.3.rc2.6.gf09b

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

[Jakub Narebski]

On Tue, 17 July 2007, Matt McCutchen napisał:
> - Centralize knowledge about snapshot formats (mime types, extensions,
>   commands) in %known_snapshot_formats and improve how some of that
>   information is specified.  In particular, zip files are no longer a
>   special case.
> 
> - Add support for offering multiple snapshot formats to the user so
>   that he/she can download a snapshot in the format he/she prefers.
>   The site-wide or project configuration now gives a list of formats
>   to offer, and if more than one format is offered, the "_snapshot_"
>   link becomes something like "snapshot (_tar.bz2_ _zip_)".
> 
> - If only one format is offered, a tooltip on the "_snapshot_" link
>   tells the user what it is.

Nice idea.

> - Fix out-of-date "tarball" -> "archive" in comment.
> 
> 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

> Signed-off-by: Matt McCutchen <hashproduct@gmail.com>
> ---
> 
> Changes since the previous revision of the patch:
> 
> - Added display names.
> - Changed compressor command line to list form.
> - Added compatibility format aliases for repository configuration.
> - Tweaked filtering of unknown formats to apply only to repository
>   configuration.
> - Reformatted format_snapshot_links and added/modified comments to make it much
>   easier to understand.
> - When a single snapshot format is offered, added a tooltip showing the format
>   to the "snapshot" link.  This helps Junio's hypothetical end user without
>   using additional screen real estate.
> 
> 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.

> +# information about snapshot formats that gitweb is capable of serving
> +# name => [display name, mime type, filename suffix, --format for git-archive,
> +#          [compressor command and arguments] | undef]
> +our %known_snapshot_formats = (
> +	'tgz'  => ['tar.gz' , 'application/x-gzip' , '.tar.gz' , 'tar', ['gzip' ]],
> +	'tbz2' => ['tar.bz2', 'application/x-bzip2', '.tar.bz2', 'tar', ['bzip2']],
> +	'zip'  => ['zip',     'application/x-zip'  , '.zip'    , 'zip', undef    ],
> +);

First I'm not sure if we want to do the way it had to be done when
those info was in the subfield of %feature hash, or to imitate %feature
hash using instead:

+our %known_snapshot_formats = (
+	'tgz'  => {
+		'display'  => 'tar.gz',
+		'mimetype' => 'application/x-gzip',
+		'suffix'   => '.tar.gz',
+		'format'   => 'tar',
+		'compressor' => ['gzip' ]},
... 

which means that when using %known_snapshot_formats we don't have
to remember for example which of the elements in array is mimetype,
which display name, and which format to be passed to git-archive.


Second, I have thought that we might want to simply use the rest of
array for the compressor and it's arguments instead of adding it as
anonymous array reference (inner array). But this way we could in
princile add more pipelines... although I think it would be not useful.
I'd rather have first option implemented, even if it does not allow for
multiple pipelines.

Third, I think we don't need to say "undef" explicitely, I think.
"defined ('a')[1]" returns the same as "defined ('a', undef)[1]".

> +# Aliases so we understand old gitweb.snapshot values in repository
> +# configuration.
> +our %known_snapshot_format_aliases = (
> +	'gzip'  => 'tgz' ,
> +	'bzip2' => 'tbz2',
> +);

Good idea, better than tring to fit it in %known_snapshot_formats.

-- 
Jakub Narebski
Poland

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> On Tue, 17 July 2007, Matt McCutchen napisał:
> ...
>> 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 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'},

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

[Luben Tuikov]

--- Junio C Hamano <gitster@pobox.com> wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > On Tue, 17 July 2007, Matt McCutchen napisał:
> > ...
> >> 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.
> 
> >> 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

> 
> 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'},
> 
> 

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

[Jakub Narebski]

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

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

[Luben Tuikov]

--- Jakub Narebski <jnareb@gmail.com> wrote:
> 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.

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

Thanks for the fix.

   Luben

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

[Jakub Narebski]

On Thu, 19 July 2007, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> On Tue, 17 July 2007, Matt McCutchen napisał:
>> ...
>>> 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

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

[Jakub Narebski]

On Tue, 17 July 2007, Matt McCutchen wrote:

>  sub feature_snapshot {
> -       my ($ctype, $suffix, $command) = @_;
> +       my (@fmts) = @_;
>  
>         my ($val) = git_get_project_config('snapshot');
>  
> -       if ($val eq 'gzip') {
> -               return ('x-gzip', 'gz', 'gzip');
> -       } elsif ($val eq 'bzip2') {
> -               return ('x-bzip2', 'bz2', 'bzip2');
> -       } elsif ($val eq 'zip') {
> -               return ('x-zip', 'zip', '');
> -       } elsif ($val eq 'none') {
> -               return ();
> +       if ($val) {
> +               @fmts = ($val eq 'none' ? () : split /,/, $val);
> +               @fmts = map $known_snapshot_format_aliases{$_} || $_, @fmts;
> +               @fmts = grep exists $known_snapshot_formats{$_}, @fmts;
>         }
>  
> -       return ($ctype, $suffix, $command);
> -}

I would use more permissive (be forbidding in what you accept) regexp
to split gitweb.snapshot value into list of snapshot formats, so one
could use "tgz, zip", or perhaps even "tgz zip", and not only "tgz,zip"
(no whitespace possible). For example

+               @fmts = ($val eq 'none' ? () : split(/\s*,\s*/, $val));

or even

+               @fmts = ($val eq 'none' ? () : split(/\s*[,\s]\s*/, $val));

to allow "tgz zip".


Your regexp for "tgz, zip" would get 'tgz', ' zip' (with leading space)
as snapshot formats to use.

-- 
Jakub Narebski
Poland

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> On Thu, 19 July 2007, Junio C Hamano wrote:
> ...
>> 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.
>
> 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).

That's true.  I'd rather see this polished a bit more before
applying.

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

[Jakub Narebski]

From: Matt McCutchen <hashproduct@gmail.com>
Subject: [PATCH] gitweb: snapshot cleanups & support for offering multiple formats

- Centralize knowledge about snapshot formats (mime types, extensions,
  commands) in %known_snapshot_formats and improve how some of that
  information is specified.  In particular, zip files are no longer a
  special case.

- Add support for offering multiple snapshot formats to the user so
  that he/she can download a snapshot in the format he/she prefers.
  The site-wide or project configuration now gives a list of formats
  to offer, and if more than one format is offered, the "_snapshot_"
  link becomes something like "snapshot (_tar.bz2_ _zip_)".

- If only one format is offered, a tooltip on the "_snapshot_" link
  tells the user what it is.

- Fix out-of-date "tarball" -> "archive" in comment.

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.  There was taken care
for old-style gitweb configuration to work as it used to, but this
backward compatibility works only for the values which correspond to
gitweb.snapshot values of 'gzip', 'bzip2' and 'zip', i.e.
  ['x-gzip', 'gz', 'gzip']
  ['x-bzip2', 'bz2', 'bzip2']
  ['x-zip', 'zip', '']

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.

Signed-off-by: Matt McCutchen <hashproduct@gmail.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This tries to address concerns that was raised in this thread.
Namely:
 - hash is used instead of fixed order of mixed arguments array
 - it tries to be backward compatibile wrt. gitweb config

 gitweb/gitweb.perl |  166 ++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 116 insertions(+), 50 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 6754e26..c4f8824 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -114,6 +114,49 @@ our $fallback_encoding = 'latin1';
 # - one might want to include '-B' option, e.g. '-B', '-M'
 our @diff_opts = ('-M'); # taken from git_commit
 
+# information about snapshot formats that gitweb is capable of serving
+our %known_snapshot_formats = (
+	# name => {
+	# 	'display' => display name,
+	# 	'type' => mime type,
+	# 	'suffix' => filename suffix,
+	# 	'format' => --format for git-archive,
+	# 	'compressor' => [compressor command and arguments]
+	# 	                (array reference, optional)}
+	#
+	'tgz' => {
+		'display' => 'tar.gz',
+		'type' => 'application/x-gzip',
+		'suffix' => '.tar.gz',
+		'format' => 'tar',
+		'compressor' => ['gzip']},
+
+	'tbz2' => {
+		'display' => 'tar.bz2',
+		'type' => 'application/x-bzip2',
+		'suffix' => '.tar.bz2',
+		'format' => 'tar',
+		'compressor' => ['bzip2']},
+
+	'zip' => {
+		'display' => 'zip',
+		'type' => 'application/x-zip',
+		'suffix' => '.zip',
+		'format' => 'zip'},
+);
+
+# Aliases so we understand old gitweb.snapshot values in repository
+# configuration.
+our %known_snapshot_format_aliases = (
+	'gzip'  => 'tgz',
+	'bzip2' => 'tbz2',
+
+	# backward compatibility: legacy gitweb config support
+	'x-gzip' => undef, 'gz' => undef,
+	'x-bzip2' => undef, 'bz2' => undef,
+	'x-zip' => undef, '' => undef,
+);
+
 # You define site-wide feature defaults here; override them with
 # $GITWEB_CONFIG as necessary.
 our %feature = (
@@ -144,20 +187,22 @@ our %feature = (
 		'override' => 0,
 		'default' => [0]},
 
-	# Enable the 'snapshot' link, providing a compressed tarball of any
+	# Enable the 'snapshot' link, providing a compressed archive of any
 	# tree. This can potentially generate high traffic if you have large
 	# project.
 
+	# Value is a list of formats defined in %known_snapshot_formats that
+	# you wish to offer.
 	# To disable system wide have in $GITWEB_CONFIG
-	# $feature{'snapshot'}{'default'} = [undef];
+	# $feature{'snapshot'}{'default'} = [];
 	# To have project specific config enable override in $GITWEB_CONFIG
 	# $feature{'snapshot'}{'override'} = 1;
-	# and in project config gitweb.snapshot = none|gzip|bzip2|zip;
+	# and in project config, a comma-separated list of formats or "none"
+	# to disable.  Example: gitweb.snapshot = tbz2,zip;
 	'snapshot' => {
 		'sub' => \&feature_snapshot,
 		'override' => 0,
-		#         => [content-encoding, suffix, program]
-		'default' => ['x-gzip', 'gz', 'gzip']},
+		'default' => ['tgz']},
 
 	# Enable text search, which will list the commits which match author,
 	# committer or commit text to a given string.  Enabled by default.
@@ -256,28 +301,19 @@ sub feature_blame {
 }
 
 sub feature_snapshot {
-	my ($ctype, $suffix, $command) = @_;
+	my (@fmts) = @_;
 
 	my ($val) = git_get_project_config('snapshot');
 
-	if ($val eq 'gzip') {
-		return ('x-gzip', 'gz', 'gzip');
-	} elsif ($val eq 'bzip2') {
-		return ('x-bzip2', 'bz2', 'bzip2');
-	} elsif ($val eq 'zip') {
-		return ('x-zip', 'zip', '');
-	} elsif ($val eq 'none') {
-		return ();
+	if ($val) {
+		@fmts = ($val eq 'none' ? () : split /\s*[,\s]\s*/, $val);
+		@fmts = grep { defined } map {
+			exists $known_snapshot_format_aliases{$_} ?
+				$known_snapshot_format_aliases{$_} : $_ } @fmts;
+		@fmts = grep(exists $known_snapshot_formats{$_}, @fmts);
 	}
 
-	return ($ctype, $suffix, $command);
-}
-
-sub gitweb_have_snapshot {
-	my ($ctype, $suffix, $command) = gitweb_check_feature('snapshot');
-	my $have_snapshot = (defined $ctype && defined $suffix);
-
-	return $have_snapshot;
+	return @fmts;
 }
 
 sub feature_grep {
@@ -563,6 +599,7 @@ sub href(%) {
 		order => "o",
 		searchtext => "s",
 		searchtype => "st",
+		snapshot_format => "sf",
 	);
 	my %mapping = @mapping;
 
@@ -1257,6 +1294,39 @@ sub format_diff_line {
 	return "<div class=\"diff$diff_class\">" . esc_html($line, -nbsp=>1) . "</div>\n";
 }
 
+# Generates undef or something like "_snapshot_" or "snapshot (_tbz2_ _zip_)",
+# linked.  Pass the hash of the tree/commit to snapshot.
+sub format_snapshot_links {
+	my ($hash) = @_;
+	my @snapshot_fmts = gitweb_check_feature('snapshot');
+	my $num_fmts = @snapshot_fmts;
+	if ($num_fmts > 1) {
+		# A parenthesized list of links bearing format names.
+		return "snapshot (" . join(' ', map
+			$cgi->a({
+				-href => href(
+					action=>"snapshot",
+					hash=>$hash,
+					snapshot_format=>$_
+				)
+			}, $known_snapshot_formats{$_}{'display'})
+		, @snapshot_fmts) . ")";
+	} elsif ($num_fmts == 1) {
+		# A single "snapshot" link whose tooltip bears the format name.
+		my ($fmt) = @snapshot_fmts;
+		return $cgi->a({
+				-href => href(
+					action=>"snapshot",
+					hash=>$hash,
+					snapshot_format=>$fmt
+				),
+				-title => "in format: $known_snapshot_formats{$fmt}{'display'}"
+			}, "snapshot");
+	} else { # $num_fmts == 0
+		return undef;
+	}
+}
+
 ## ----------------------------------------------------------------------
 ## git utility subroutines, invoking git commands
 
@@ -3321,8 +3391,6 @@ sub git_shortlog_body {
 	# uses global variable $project
 	my ($commitlist, $from, $to, $refs, $extra) = @_;
 
-	my $have_snapshot = gitweb_have_snapshot();
-
 	$from = 0 unless defined $from;
 	$to = $#{$commitlist} if (!defined $to || $#{$commitlist} < $to);
 
@@ -3349,8 +3417,9 @@ sub git_shortlog_body {
 		      $cgi->a({-href => href(action=>"commit", hash=>$commit)}, "commit") . " | " .
 		      $cgi->a({-href => href(action=>"commitdiff", hash=>$commit)}, "commitdiff") . " | " .
 		      $cgi->a({-href => href(action=>"tree", hash=>$commit, hash_base=>$commit)}, "tree");
-		if ($have_snapshot) {
-			print " | " . $cgi->a({-href => href(action=>"snapshot", hash=>$commit)}, "snapshot");
+		my $snapshot_links = format_snapshot_links($commit);
+		if (defined $snapshot_links) {
+			print " | " . $snapshot_links;
 		}
 		print "</td>\n" .
 		      "</tr>\n";
@@ -4132,8 +4201,6 @@ sub git_blob {
 }
 
 sub git_tree {
-	my $have_snapshot = gitweb_have_snapshot();
-
 	if (!defined $hash_base) {
 		$hash_base = "HEAD";
 	}
@@ -4167,11 +4234,10 @@ sub git_tree {
 				                       hash_base=>"HEAD", file_name=>$file_name)},
 				        "HEAD"),
 		}
-		if ($have_snapshot) {
+		my $snapshot_links = format_snapshot_links($hash);
+		if (defined $snapshot_links) {
 			# FIXME: Should be available when we have no hash base as well.
-			push @views_nav,
-				$cgi->a({-href => href(action=>"snapshot", hash=>$hash)},
-				        "snapshot");
+			push @views_nav, $snapshot_links;
 		}
 		git_print_page_nav('tree','', $hash_base, undef, undef, join(' | ', @views_nav));
 		git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash_base);
@@ -4235,33 +4301,36 @@ sub git_tree {
 }
 
 sub git_snapshot {
-	my ($ctype, $suffix, $command) = gitweb_check_feature('snapshot');
-	my $have_snapshot = (defined $ctype && defined $suffix);
-	if (!$have_snapshot) {
-		die_error('403 Permission denied', "Permission denied");
+	my @supported_fmts = gitweb_check_feature('snapshot');
+
+	my $format = $cgi->param('sf');
+	unless ($format =~ m/[a-z0-9]+/
+		&& exists($known_snapshot_formats{$format})
+		&& grep($_ eq $format, @supported_fmts)) {
+		die_error(undef, "Unsupported snapshot format");
 	}
 
 	if (!defined $hash) {
 		$hash = git_get_head_hash($project);
 	}
 
-	my $git = git_cmd_str();
+	my $git_command = git_cmd_str();
 	my $name = $project;
 	$name =~ s,([^/])/*\.git$,$1,;
 	$name = basename($name);
 	my $filename = to_utf8($name);
 	$name =~ s/\047/\047\\\047\047/g;
 	my $cmd;
-	if ($suffix eq 'zip') {
-		$filename .= "-$hash.$suffix";
-		$cmd = "$git archive --format=zip --prefix=\'$name\'/ $hash";
-	} else {
-		$filename .= "-$hash.tar.$suffix";
-		$cmd = "$git archive --format=tar --prefix=\'$name\'/ $hash | $command";
+	$filename .= "-$hash$known_snapshot_formats{$format}{'suffix'}";
+	$cmd = "$git_command archive " .
+		"--format=$known_snapshot_formats{$format}{'format'}" .
+		"--prefix=\'$name\'/ $hash";
+	if (exists $known_snapshot_formats{$format}{'compressor'}) {
+		$cmd .= ' | ' . join ' ', @{$known_snapshot_formats{$format}{'compressor'}};
 	}
 
 	print $cgi->header(
-		-type => "application/$ctype",
+		-type => $known_snapshot_formats{$format}{'type'},
 		-content_disposition => 'inline; filename="' . "$filename" . '"',
 		-status => '200 OK');
 
@@ -4271,7 +4340,6 @@ sub git_snapshot {
 	print <$fd>;
 	binmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi
 	close $fd;
-
 }
 
 sub git_log {
@@ -4390,8 +4458,6 @@ sub git_commit {
 	my $refs = git_get_references();
 	my $ref = format_ref_marker($refs, $co{'id'});
 
-	my $have_snapshot = gitweb_have_snapshot();
-
 	git_header_html(undef, $expires);
 	git_print_page_nav('commit', '',
 	                   $hash, $co{'tree'}, $hash,
@@ -4430,9 +4496,9 @@ sub git_commit {
 	      "<td class=\"link\">" .
 	      $cgi->a({-href => href(action=>"tree", hash=>$co{'tree'}, hash_base=>$hash)},
 	              "tree");
-	if ($have_snapshot) {
-		print " | " .
-		      $cgi->a({-href => href(action=>"snapshot", hash=>$hash)}, "snapshot");
+	my $snapshot_links = format_snapshot_links($hash);
+	if (defined $snapshot_links) {
+		print " | " . $snapshot_links;
 	}
 	print "</td>" .
 	      "</tr>\n";
-- 
1.5.2.4

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

[Junio C Hamano]

Thanks.  Will apply.

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

[Matt McCutchen]

On 7/22/07, Junio C Hamano <gitster@pobox.com> wrote:
> Thanks.  Will apply.

Jakub, thanks for picking this up.  I was running out of energy to
push what began as an offer of a possibly useful local customization
any further toward adoption.

That said: the backward compatibility code for gitweb _site_
configuration is broken because it is inside an if statement that only
runs for gitweb _repository_ configuration.  I tested it with a
gitweb_config.perl with

$feature{'snapshot'}{'default'} = ['x-gzip', 'gz', 'gzip'];
$feature{'snapshot'}{'override'} = 0;

and gitweb generated "snapshot ( )" (no visible link).  Inspection of
the source revealed links to "sf=x-gzip", "sf=gz", and "sf=gzip" with
empty strings for the link text.  The expected behavior is
"_snapshot_", linking to "sf=tgz", with a tooltip indicating "tar.gz"
format.

Moving the code out of the `if' wouldn't solve the problem by itself
because feature_snapshot is only reached if override is enabled, but
the site configuration compatibility needs to work whether or not
override is enabled.  That's why Junio was considering adding a
separate function gitweb_bc_feature_snapshot.  I think a nicer
alternative would be to change gitweb_check_feature to call the sub
(if any) whether or not override is enabled and let the sub check
$feature{'foo'}{'override'} itself to decide whether to look for an
override.  Then each feature_* sub is the central point for both
override processing and backward compatibility for that feature.

Matt

[PATCH] gitweb: Fix support for legacy gitweb config for snapshots

[Jakub Narebski]

Earlier commit which cleaned up snapshot support and introduced
support for multiple snapshot formats changed the format of
$feature{'snapshot'}{'default'} (gitweb configuration) and
gitweb.snapshot configuration variable (repository configuration).
It supported old gitweb.snapshot values of 'gzip', 'bzip2' and 'zip'
and tried to support, but failed to do that, old values of
$feature{'snapshot'}{'default'}; at least those corresponding to
old gitweb.snapshot values of 'gzip', 'bzip2' and 'zip', i.e.
  ['x-gzip', 'gz', 'gzip']
  ['x-bzip2', 'bz2', 'bzip2']
  ['x-zip', 'zip', '']

This commit moves legacy configuration support out of feature_snapshot
subroutine to separate filter_snapshot_fmts subroutine. The
filter_snapshot_fmts is used on result on result of
gitweb_check_feature('snapshot').  This way feature_snapshot deals
_only_ with repository config.

As a byproduct you can now use 'gzip' and 'bzip2' as aliases to 'tgz'
and 'tbz2' also in $feature{'snapshot'}{'default'}, not only in
gitweb.snapshot.


While at it do some whitespace cleanup: use tabs for indent, but
spaces for align.

Noticed-by: Matt McCutchen <hashproduct@gmail.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
On Sun, 22 July 2007, Matt McCutchen wrote:

> That said: the backward compatibility code for gitweb _site_
> configuration is broken because it is inside an if statement that only
> runs for gitweb _repository_ configuration.

Sorry for sending not fully tested code. This should fix that.
This commit _is_ rudimentally tested.

 gitweb/gitweb.perl |   27 ++++++++++++++++++++-------
 1 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index c4f8824..fdfce31 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -307,10 +307,6 @@ sub feature_snapshot {
 
 	if ($val) {
 		@fmts = ($val eq 'none' ? () : split /\s*[,\s]\s*/, $val);
-		@fmts = grep { defined } map {
-			exists $known_snapshot_format_aliases{$_} ?
-				$known_snapshot_format_aliases{$_} : $_ } @fmts;
-		@fmts = grep(exists $known_snapshot_formats{$_}, @fmts);
 	}
 
 	return @fmts;
@@ -356,6 +352,18 @@ sub check_export_ok {
 		(!$export_ok || -e "$dir/$export_ok"));
 }
 
+# process alternate names for backward compatibility
+# filter out unsupported (unknown) snapshot formats
+sub filter_snapshot_fmts {
+	my @fmts = @_;
+
+	@fmts = map {
+		exists $known_snapshot_format_aliases{$_} ?
+		       $known_snapshot_format_aliases{$_} : $_} @fmts;
+	@fmts = grep(exists $known_snapshot_formats{$_}, @fmts);
+
+}
+
 our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || "++GITWEB_CONFIG++";
 do $GITWEB_CONFIG if -e $GITWEB_CONFIG;
 
@@ -1299,9 +1307,11 @@ sub format_diff_line {
 sub format_snapshot_links {
 	my ($hash) = @_;
 	my @snapshot_fmts = gitweb_check_feature('snapshot');
+	@snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);
 	my $num_fmts = @snapshot_fmts;
 	if ($num_fmts > 1) {
 		# A parenthesized list of links bearing format names.
+		# e.g. "snapshot (_tar.gz_ _zip_)"
 		return "snapshot (" . join(' ', map
 			$cgi->a({
 				-href => href(
@@ -1313,8 +1323,10 @@ sub format_snapshot_links {
 		, @snapshot_fmts) . ")";
 	} elsif ($num_fmts == 1) {
 		# A single "snapshot" link whose tooltip bears the format name.
+		# i.e. "_snapshot_"
 		my ($fmt) = @snapshot_fmts;
-		return $cgi->a({
+		return
+			$cgi->a({
 				-href => href(
 					action=>"snapshot",
 					hash=>$hash,
@@ -4302,11 +4314,12 @@ sub git_tree {
 
 sub git_snapshot {
 	my @supported_fmts = gitweb_check_feature('snapshot');
+	@supported_fmts = filter_snapshot_fmts(@supported_fmts);
 
 	my $format = $cgi->param('sf');
 	unless ($format =~ m/[a-z0-9]+/
-		&& exists($known_snapshot_formats{$format})
-		&& grep($_ eq $format, @supported_fmts)) {
+	        && exists($known_snapshot_formats{$format})
+	        && grep($_ eq $format, @supported_fmts)) {
 		die_error(undef, "Unsupported snapshot format");
 	}
 
-- 
1.5.2.4

Re: [PATCH] gitweb: Fix support for legacy gitweb config for snapshots

[Matt McCutchen]

On 7/22/07, Jakub Narebski <jnareb@gmail.com> wrote:
> This commit moves legacy configuration support out of feature_snapshot
> subroutine to separate filter_snapshot_fmts subroutine. The
> filter_snapshot_fmts is used on result on result of

There's a typo: "on result" appears twice.

> On Sun, 22 July 2007, Matt McCutchen wrote:
>
> > That said: the backward compatibility code for gitweb _site_
> > configuration is broken because it is inside an if statement that only
> > runs for gitweb _repository_ configuration.
>
> Sorry for sending not fully tested code. This should fix that.
> This commit _is_ rudimentally tested.

I tested it too and it worked.  I like the approach of using
filter_snapshot_fmts.

Matt

Re: [PATCH] gitweb: Fix support for legacy gitweb config for snapshots

[Junio C Hamano]

Thanks, both.

[RFC/PATCH] gitweb: Enable transparent compression form HTTP output

[Jakub Narebski]

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 <jnareb@gmail.com>
---
On Thu, 19 July 2007, Luben Tuikov wrote:
> --- Jakub Narebski <jnareb@gmail.com> wrote:
> > 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.
> 
> Yeah, that's what I suspected, since there was nothing obviously
> wrong with the code.

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

Re: [RFC/PATCH] gitweb: Enable transparent compression form HTTP output

[Petr Baudis]

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

Acked-by: Petr Baudis <pasky@suse.cz>

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

Re: [RFC/PATCH] gitweb: Enable transparent compression form HTTP output

[Jakub Narebski]

On Sat, Aug 25, 2007, Petr Baudis wrote:
> 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.

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;
 
>> 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 <jnareb@gmail.com>
> 
> Acked-by: Petr Baudis <pasky@suse.cz>

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

Re: [RFC/PATCH] gitweb: Enable transparent compression form HTTP output

[Petr Baudis]

On Sun, Aug 26, 2007 at 12:09:29AM CEST, Jakub Narebski wrote:
> On Sat, Aug 25, 2007, Petr Baudis wrote:
> > 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.
> 
> 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

Re: [RFC/PATCH] gitweb: Enable transparent compression form HTTP output

[Jakub Narebski]

On Sunday, 26 August 2007, Petr "Pasky" Baudis wrote:
> On Sun, Aug 26, 2007 at 12:09:29AM CEST, Jakub Narebski wrote:
>> On Sat, Aug 25, 2007, Petr Baudis wrote:
>>> 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.
>> 
>> 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.

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