[PATHv3 0/3] PATH_INFO snapshot formats

[PATHv3 0/3] PATH_INFO snapshot formats

[Giuseppe Bilotta]

The following three patches allow gitweb to read (and emit) PATH_INFO
URLs with embedded snapshot_format information. This eliminates one
for CGI parameter when PATH_INFO is enabled, and also offers a more
user-friendly URL for snapshots.

The first patch is actually rather independent from the other two: it
collects the definition (and calculation) of the array of allowed
snapshot into a global variable.

Giuseppe Bilotta (3):
  gitweb: make the supported snapshot formats array global
  gitweb: retrieve snapshot format from PATH_INFO
  gitweb: embed snapshot format parameter in PATH_INFO

 gitweb/gitweb.perl |   70 ++++++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 62 insertions(+), 8 deletions(-)

[PATHv3 1/3] gitweb: make the supported snapshot formats array global

[Giuseppe Bilotta]

The array of supported snapshot format is used and defined (with two
different names) in two routines, one of which (format_snapshot_links)
is often called multiple times per page.

Simplify code and speed up page generation by making the array global.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 9d1af7e..8441912 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -721,6 +721,10 @@ if (defined $searchtext) {
 our $git_dir;
 $git_dir = "$projectroot/$project" if $project;
 
+# list of supported snapshot formats
+our @snapshot_fmts = gitweb_check_feature('snapshot');
+@snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);
+
 # dispatch
 if (!defined $action) {
 	if (defined $hash) {
@@ -1647,8 +1651,6 @@ sub format_diff_line {
 # linked.  Pass the hash of the tree/commit to snapshot.
 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.
@@ -4850,20 +4852,17 @@ sub git_tree {
 }
 
 sub git_snapshot {
-	my @supported_fmts = gitweb_check_feature('snapshot');
-	@supported_fmts = filter_snapshot_fmts(@supported_fmts);
-
 	my $format = $input_params{'snapshot_format'};
-	if (!@supported_fmts) {
+	if (!@snapshot_fmts) {
 		die_error(403, "Snapshots not allowed");
 	}
 	# default to first supported snapshot format
-	$format ||= $supported_fmts[0];
+	$format ||= $snapshot_fmts[0];
 	if ($format !~ m/^[a-z0-9]+$/) {
 		die_error(400, "Invalid snapshot format parameter");
 	} elsif (!exists($known_snapshot_formats{$format})) {
 		die_error(400, "Unknown snapshot format");
-	} elsif (!grep($_ eq $format, @supported_fmts)) {
+	} elsif (!grep($_ eq $format, @snapshot_fmts)) {
 		die_error(403, "Unsupported snapshot format");
 	}
 
-- 
1.5.6.5

[PATHv3 2/3] gitweb: retrieve snapshot format from PATH_INFO

[Giuseppe Bilotta]

We parse requests for $project/snapshot/$head.$sfx as equivalent to
$project/snapshot/$head?sf=$sfx, where $sfx is any of the known
(although not necessarily supported) snapshot formats (or its default
suffix).

The filename for the resulting package preserves the requested
extensions (so asking for a .tgz gives a .tgz, and asking for a .tar.gz
gives a .tar.gz), although for obvious reasons it doesn't preserve the
basename (git/snapshot/next.tgz returns a file names git-next.tgz).

This introduces a potential case for ambiguity if a project has a head
that ends with a snapshot-like suffix (.zip, .tgz, .tar.gz, etc) and the
sf CGI parameter is not present; however, gitweb only produces URLs with
the sf parameter currently, so this is only a potential issue for
hand-coded URLs for extremely unusual project.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   38 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 8441912..0a41be5 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -616,6 +616,44 @@ sub evaluate_path_info {
 			$input_params{'hash_parent'} ||= $parentrefname;
 		}
 	}
+
+	# for the snapshot action, we allow URLs in the form
+	# $project/snapshot/$hash.ext
+	# where .ext determines the snapshot and gets removed from the
+	# passed $refname to provide the $hash.
+	#
+	# To be able to tell that $refname includes the format extension, we
+	# require the following two conditions to be satisfied:
+	# - the hash input parameter MUST have been set from the $refname part
+	#   of the URL (i.e. they must be equal)
+	# - the snapshot format MUST NOT have been defined already (e.g. from
+	#   CGI parameter sf)
+	# It's also useless to try any matching unless $refname has a dot,
+	# so we check for that too
+	if ($input_params{'action'} eq 'snapshot' &&
+		defined $refname && index($refname, '.') != -1 &&
+		$refname eq $input_params{'hash'} &&
+		!defined $input_params{'snapshot_format'}) {
+		# We loop over the known snapshot formats, checking for
+		# extensions. Allowed extensions are both the defined suffix
+		# (which includes the initial dot already) and the snapshot
+		# format key itself, with a prepended dot
+		while (my ($fmt, %opt) = each %known_snapshot_formats) {
+			my $hash = $refname;
+			my $sfx;
+			$hash =~ s/(\Q$opt{'suffix'}\E|\Q.$fmt\E)$//;
+			next unless $sfx = $1;
+			# a valid suffix was found, so set the snapshot format
+			# and reset the hash parameter
+			$input_params{'snapshot_format'} = $fmt;
+			$input_params{'hash'} = $hash;
+			# we also set the format suffix to the one requested
+			# in the URL: this way a request for e.g. .tgz returns
+			# a .tgz instead of a .tar.gz
+			$known_snapshot_formats{$fmt}{'suffix'} = $sfx;
+			last;
+		}
+	}
 }
 evaluate_path_info();
 
-- 
1.5.6.5

[PATHv3 3/3] gitweb: embed snapshot format parameter in PATH_INFO

[Giuseppe Bilotta]

When PATH_INFO is active, get rid of the sf CGI parameter by embedding
the snapshot format information in the PATH_INFO URL, in the form of an
appropriate extension.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 0a41be5..d2484ab 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -810,6 +810,7 @@ sub href (%) {
 		#   - action
 		#   - hash_parent or hash_parent_base:/file_parent
 		#   - hash or hash_base:/filename
+		#   - the snapshot_format as an appropriate suffix
 
 		# When the script is the root DirectoryIndex for the domain,
 		# $href here would be something like http://gitweb.example.com/
@@ -821,6 +822,10 @@ sub href (%) {
 		$href .= "/".esc_url($params{'project'}) if defined $params{'project'};
 		delete $params{'project'};
 
+		# since we destructively absorb parameters, we keep this
+		# boolean that remembers if we're handling a snapshot
+		my $is_snapshot = $params{'action'} eq 'snapshot';
+
 		# Summary just uses the project path URL, any other action is
 		# added to the URL
 		if (defined $params{'action'}) {
@@ -860,6 +865,18 @@ sub href (%) {
 			$href .= esc_url($params{'hash'});
 			delete $params{'hash'};
 		}
+
+		# If the action was a snapshot, we can absorb the
+		# snapshot_format parameter too
+		if ($is_snapshot) {
+			my $fmt = $params{'snapshot_format'};
+			# snapshot_format should always be defined when href()
+			# is called, but just in case some code forgets, we
+			# fall back to the default
+			$fmt ||= $snapshot_fmts[0];
+			$href .= $known_snapshot_formats{$fmt}{'suffix'};
+			delete $params{'snapshot_format'};
+		}
 	}
 
 	# now encode the parameters explicitly
-- 
1.5.6.5

Re: [PATHv3 2/3] gitweb: retrieve snapshot format from PATH_INFO

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> +	# It's also useless to try any matching unless $refname has a dot,
> +	# so we check for that too
> +	if ($input_params{'action'} eq 'snapshot' &&

This broke t9500 because $input_params{'action'} can be undef.  What I'll
be pushing out on 'pu' has a fixup on it (I am rewinding and rebuilding
'pu' right now so it will be a while).

Next time, please make sure that your patches pass the tests before sending
them out.

Thanks.