Re: [RFCv4 1/3] gitweb: add patch view

Re: [RFCv4 1/3] gitweb: add patch view

[Giuseppe Bilotta]

On Sun, Dec 7, 2008 at 7:04 AM, Matt Kraai <kraai@ftbfs.org> wrote:
> Howdy,
>
> Why do you check $patch_max at the start of git_commitdiff instead of
> at the start of hunk which opens git-format-patch?  It seems like it
> would be clearer to check it later, at some small loss of efficiency.

Subsequent patches use the check to add a link to the nav bar, so it
reduces code shift from patch to patch 8-)

-- 
Giuseppe "Oblomov" Bilotta

[RFCv4 0/3] gitweb: patch view

[Giuseppe Bilotta]

Fourth iteration of the patch view feature, that exposes git
format-patch output in gitweb. The most significant different from the
previous revision is the introduction of the 'patches' view, that only
differs from 'patch' view in the treatment of single commits: 'patch'
view only displays the patch for that specific commit, whereas 'patches'
follows the git format-patch M.O.

Giuseppe Bilotta (3):
  gitweb: add patch view
  gitweb: add patches view
  gitweb: link to patch(es) view from commit and log views

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

[RFCv4 1/3] gitweb: add patch view

[Giuseppe Bilotta]

The output of commitdiff_plain is not intended for git-am:
 * when given a range of commits, commitdiff_plain publishes a single
   patch with the message from the first commit, instead of a patchset
 * the hand-built email format replicates the commit summary both as
   email subject and as first line of the email itself, resulting in
   a duplication if the output is used with git-am.

We thus create a new view that can be fed to git-am directly, allowing
patch exchange via gitweb. The new view exposes the output of git
format-patch directly, limiting it to a single patch in the case of a
single commit.

A configurable upper limit is imposed on the number of commits which
will be included in a patchset, to prevent DoS attacks on the server.
Setting the limit to 0 will disable the patch view, setting it to a
negative number will remove the limit.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 95988fb..71d5af4 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -329,6 +329,14 @@ our %feature = (
 	'ctags' => {
 		'override' => 0,
 		'default' => [0]},
+
+	# The maximum number of patches in a patchset generated in patch
+	# view. Set this to 0 or undef to disable patch view, or to a
+	# negative number to remove any limit.
+	'patches' => {
+		'sub' => \&feature_patches,
+		'override' => 0,
+		'default' => [16]},
 );
 
 sub gitweb_get_feature {
@@ -410,6 +418,19 @@ sub feature_pickaxe {
 	return ($_[0]);
 }
 
+sub feature_patches {
+	my @val = (git_get_project_config('patches', '--int'));
+
+	# if @val is empty, the config is not (properly)
+	# overriding the feature, so we return the default,
+	# otherwise we pick the override
+	if (@val) {
+		return @val;
+	}
+
+	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.
@@ -503,6 +524,7 @@ our %actions = (
 	"heads" => \&git_heads,
 	"history" => \&git_history,
 	"log" => \&git_log,
+	"patch" => \&git_patch,
 	"rss" => \&git_rss,
 	"atom" => \&git_atom,
 	"search" => \&git_search,
@@ -5386,6 +5408,13 @@ sub git_blobdiff_plain {
 
 sub git_commitdiff {
 	my $format = shift || 'html';
+
+	my $patch_max;
+	if ($format eq 'patch') {
+		$patch_max = gitweb_check_feature('patches');
+		die_error(403, "Patch view not allowed") unless $patch_max;
+	}
+
 	$hash ||= $hash_base || "HEAD";
 	my %co = parse_commit($hash)
 	    or die_error(404, "Unknown commit object");
@@ -5483,7 +5512,23 @@ sub git_commitdiff {
 		open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
 			'-p', $hash_parent_param, $hash, "--"
 			or die_error(500, "Open git-diff-tree failed");
-
+	} elsif ($format eq 'patch') {
+		# For commit ranges, we limit the output to the number of
+		# patches specified in the 'patches' feature.
+		# For single commits, we limit the output to a single patch,
+		# diverging from the git format-patch default.
+		my @commit_spec = ();
+		if ($hash_parent) {
+			if ($patch_max > 0) {
+				push @commit_spec, "-$patch_max";
+			}
+			push @commit_spec, '-n', "$hash_parent..$hash";
+		} else {
+			push @commit_spec, '-1', '--root', $hash;
+		}
+		open $fd, "-|", git_cmd(), "format-patch", '--encoding=utf8',
+			'--stdout', @commit_spec
+			or die_error(500, "Open git-format-patch failed");
 	} else {
 		die_error(400, "Unknown commitdiff format");
 	}
@@ -5532,6 +5577,14 @@ sub git_commitdiff {
 			print to_utf8($line) . "\n";
 		}
 		print "---\n\n";
+	} elsif ($format eq 'patch') {
+		my $filename = basename($project) . "-$hash.patch";
+
+		print $cgi->header(
+			-type => 'text/plain',
+			-charset => 'utf-8',
+			-expires => $expires,
+			-content_disposition => 'inline; filename="' . "$filename" . '"');
 	}
 
 	# write patch
@@ -5553,6 +5606,11 @@ sub git_commitdiff {
 		print <$fd>;
 		close $fd
 			or print "Reading git-diff-tree failed\n";
+	} elsif ($format eq 'patch') {
+		local $/ = undef;
+		print <$fd>;
+		close $fd
+			or print "Reading git-format-patch failed\n";
 	}
 }
 
@@ -5560,6 +5618,11 @@ sub git_commitdiff_plain {
 	git_commitdiff('plain');
 }
 
+# format-patch-style patches
+sub git_patch {
+	git_commitdiff('patch');
+}
+
 sub git_history {
 	if (!defined $hash_base) {
 		$hash_base = git_get_head_hash($project);
-- 
1.5.6.5

Re: [RFCv4 1/3] gitweb: add patch view

[Jakub Narebski]

On Sat, 6 Dec 2008, Giuseppe Bilotta wrote:

> The output of commitdiff_plain is not intended for git-am:
>  * when given a range of commits, commitdiff_plain publishes a single
>    patch with the message from the first commit, instead of a patchset
>  * the hand-built email format replicates the commit summary both as
>    email subject and as first line of the email itself, resulting in
>    a duplication if the output is used with git-am.
> 
> We thus create a new view that can be fed to git-am directly, allowing
> patch exchange via gitweb. The new view exposes the output of git
> format-patch directly, limiting it to a single patch in the case of a
> single commit.
> 
> A configurable upper limit is imposed on the number of commits which
> will be included in a patchset, to prevent DoS attacks on the server.
> Setting the limit to 0 will disable the patch view, setting it to a
> negative number will remove the limit.

It would be good to have in commit mesage what is the default upper
limit (or if we decide to have this feature turned off by default,
proposed limit to choose when enabling this feature).

Does limit of 16 patches have any numbers behind it? We use page size
of 100 commits for 'shortlog', 'log' and 'history' views, but for
those views we don't need to generate patches (which is slower). From
a few tests "git log -100" is faster than "git format-patch -100 --stdout"
about 30 times in warm cache case, and about 1.7 times in cold cache
case.

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

Other than that minor detail I like this patch

> ---
>  gitweb/gitweb.perl |   65 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 64 insertions(+), 1 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 95988fb..71d5af4 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -329,6 +329,14 @@ our %feature = (
>  	'ctags' => {
>  		'override' => 0,
>  		'default' => [0]},
> +
> +	# The maximum number of patches in a patchset generated in patch
> +	# view. Set this to 0 or undef to disable patch view, or to a
> +	# negative number to remove any limit.

I think it would be nice to have standard boilerplate explanation how
to change it, and how to override it, with the addition on how to
disable it from repository config, because it is not very obvious.

Something like:

+	# To disable system wide have in $GITWEB_CONFIG
+	# $feature{'patches'}{'default'} = [];
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'patches'}{'override'} = 1;
+	# and in project config, maximum number of patches in a patchset
+	# or 0 to disable.  Example: gitweb.patches = 0

> +	'patches' => {
> +		'sub' => \&feature_patches,
> +		'override' => 0,
> +		'default' => [16]},
>  );
>  
>  sub gitweb_get_feature {
> @@ -410,6 +418,19 @@ sub feature_pickaxe {
>  	return ($_[0]);
>  }
>  
> +sub feature_patches {
> +	my @val = (git_get_project_config('patches', '--int'));
> +
> +	# if @val is empty, the config is not (properly)
> +	# overriding the feature, so we return the default,
> +	# otherwise we pick the override

Very similar feature_snapshot subroutine doesn't have such comment.
I don't think it is necessary, and its wording might cause confusion.

> +	if (@val) {
> +		return @val;
> +	}
> +
> +	return ($_[0]);
> +}
> +

Nice.

>  # 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.
> @@ -503,6 +524,7 @@ our %actions = (
>  	"heads" => \&git_heads,
>  	"history" => \&git_history,
>  	"log" => \&git_log,
> +	"patch" => \&git_patch,
>  	"rss" => \&git_rss,
>  	"atom" => \&git_atom,
>  	"search" => \&git_search,
> @@ -5386,6 +5408,13 @@ sub git_blobdiff_plain {
>  
>  sub git_commitdiff {
>  	my $format = shift || 'html';
> +
> +	my $patch_max;
> +	if ($format eq 'patch') {
> +		$patch_max = gitweb_check_feature('patches');
> +		die_error(403, "Patch view not allowed") unless $patch_max;
> +	}
> +

Hmmm... gitweb_check_feature

>  	$hash ||= $hash_base || "HEAD";
>  	my %co = parse_commit($hash)
>  	    or die_error(404, "Unknown commit object");
> @@ -5483,7 +5512,23 @@ sub git_commitdiff {
>  		open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
>  			'-p', $hash_parent_param, $hash, "--"
>  			or die_error(500, "Open git-diff-tree failed");
> -
> +	} elsif ($format eq 'patch') {
> +		# For commit ranges, we limit the output to the number of
> +		# patches specified in the 'patches' feature.
> +		# For single commits, we limit the output to a single patch,
> +		# diverging from the git format-patch default.

I think it would be more clear to use

+		# diverging from the git-format-patch default.

> +		my @commit_spec = ();
> +		if ($hash_parent) {
> +			if ($patch_max > 0) {
> +				push @commit_spec, "-$patch_max";
> +			}
> +			push @commit_spec, '-n', "$hash_parent..$hash";
> +		} else {
> +			push @commit_spec, '-1', '--root', $hash;
> +		}

Nice solution. I like it.

> +		open $fd, "-|", git_cmd(), "format-patch", '--encoding=utf8',
> +			'--stdout', @commit_spec
> +			or die_error(500, "Open git-format-patch failed");
>  	} else {
>  		die_error(400, "Unknown commitdiff format");
>  	}
> @@ -5532,6 +5577,14 @@ sub git_commitdiff {
>  			print to_utf8($line) . "\n";
>  		}
>  		print "---\n\n";
> +	} elsif ($format eq 'patch') {
> +		my $filename = basename($project) . "-$hash.patch";

I am wondering if we could somehow mark (encode) either $hash_parent
or number of patches in proposed filename... but I don't think it is
worth it.

> +
> +		print $cgi->header(
> +			-type => 'text/plain',
> +			-charset => 'utf-8',
> +			-expires => $expires,
> +			-content_disposition => 'inline; filename="' . "$filename" . '"');
>  	}
>  
>  	# write patch
> @@ -5553,6 +5606,11 @@ sub git_commitdiff {
>  		print <$fd>;
>  		close $fd
>  			or print "Reading git-diff-tree failed\n";
> +	} elsif ($format eq 'patch') {
> +		local $/ = undef;
> +		print <$fd>;
> +		close $fd
> +			or print "Reading git-format-patch failed\n";

Nice, although... I'd prefer for Perl expert to say if it is better
to dump file as a whole in such way (it might be quite large), or
to do it line by line, i.e. without "local $/ = undef;", or using
"print while <$fd>;" also without "local $/ = undef;".


>  	}
>  }
>  
> @@ -5560,6 +5618,11 @@ sub git_commitdiff_plain {
>  	git_commitdiff('plain');
>  }
>  
> +# format-patch-style patches
> +sub git_patch {
> +	git_commitdiff('patch');
> +}
> +

Nice.

>  sub git_history {
>  	if (!defined $hash_base) {
>  		$hash_base = git_get_head_hash($project);
> -- 
> 1.5.6.5
> 
> 

-- 
Jakub Narebski
Poland

Re: [RFCv4 1/3] gitweb: add patch view

[Giuseppe Bilotta]

On Mon, Dec 15, 2008 at 2:17 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>> +     my $patch_max;
>> +     if ($format eq 'patch') {
>> +             $patch_max = gitweb_check_feature('patches');
>> +             die_error(403, "Patch view not allowed") unless $patch_max;
>> +     }
>> +
>
> Hmmm... gitweb_check_feature

You're right, it's an abuse. I'll make it gitweb_get_feature()[0]

> I am wondering if we could somehow mark (encode) either $hash_parent
> or number of patches in proposed filename... but I don't think it is
> worth it.

Including hash_parent if defined is quite possible. I'm not sure it's
really worth it considering that the typical usage would be to publish
a patchset for a particular feature (in which case the hash/branch
name would be enough).

>> +     } elsif ($format eq 'patch') {
>> +             local $/ = undef;
>> +             print <$fd>;
>> +             close $fd
>> +                     or print "Reading git-format-patch failed\n";
>
> Nice, although... I'd prefer for Perl expert to say if it is better
> to dump file as a whole in such way (it might be quite large), or
> to do it line by line, i.e. without "local $/ = undef;", or using
> "print while <$fd>;" also without "local $/ = undef;".

I'm just sticking to whatever the existing code does :-)

As soon as you finish the patchset review I'll have a new version
taking into consideration all the other suggestions and remarks I
snipped from this reply.

-- 
Giuseppe "Oblomov" Bilotta

Re: [RFCv4 1/3] gitweb: add patch view

[Jakub Narebski]

Giuseppe Bilotta wrote:
> On Mon, Dec 15, 2008 at 2:17 PM, Jakub Narebski <jnareb@gmail.com> wrote:

>>> +     my $patch_max;
>>> +     if ($format eq 'patch') {
>>> +             $patch_max = gitweb_check_feature('patches');
>>> +             die_error(403, "Patch view not allowed") unless $patch_max;
>>> +     }
>>> +
>>
>> Hmmm... gitweb_check_feature
> 
> You're right, it's an abuse. I'll make it gitweb_get_feature()[0]

Or better

+             ($patch_max) = gitweb_get_feature('patches');

[...]
> As soon as you finish the patchset review I'll have a new version
> taking into consideration all the other suggestions and remarks I
> snipped from this reply.

Thank you very much for keeping this patch series alive.

-- 
Jakub Narebski
Poland