[PATCHv5 0/3] gitweb: patch view

[PATCHv5 0/3] gitweb: patch view

[Giuseppe Bilotta]

Fifth iteration of the patch view in gitweb, that exposes the
git-format-patch output directly, allowing patchset exchange via gitweb.

No new functionality, just cleanups suggested by Jakub.

Giuseppe Bilotta (3):
  gitweb: add patch view
  gitweb: add patches view
  gitweb: link to patch(es) view in commit(diff) and (short)log view

 gitweb/gitweb.perl |  109 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 108 insertions(+), 1 deletions(-)

[PATCHv5 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 defaulting to 16 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 |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 68 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 6eb370d..1006dfe 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -330,6 +330,21 @@ 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.
+
+	# To disable system wide have in $GITWEB_CONFIG
+	# $feature{'patches'}{'default'} = [0];
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'patches'}{'override'} = 1;
+	# and in project config gitweb.patches = 0|n;
+	# where n is the maximum number of patches allowed in a patchset.
+	'patches' => {
+		'sub' => \&feature_patches,
+		'override' => 0,
+		'default' => [16]},
 );
 
 sub gitweb_get_feature {
@@ -411,6 +426,16 @@ sub feature_pickaxe {
 	return ($_[0]);
 }
 
+sub feature_patches {
+	my @val = (git_get_project_config('patches', '--int'));
+
+	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.
@@ -504,6 +529,7 @@ our %actions = (
 	"heads" => \&git_heads,
 	"history" => \&git_history,
 	"log" => \&git_log,
+	"patch" => \&git_patch,
 	"rss" => \&git_rss,
 	"atom" => \&git_atom,
 	"search" => \&git_search,
@@ -5387,6 +5413,13 @@ sub git_blobdiff_plain {
 
 sub git_commitdiff {
 	my $format = shift || 'html';
+
+	my $patch_max;
+	if ($format eq 'patch') {
+		($patch_max) = gitweb_get_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");
@@ -5484,7 +5517,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");
 	}
@@ -5533,6 +5582,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
@@ -5554,6 +5611,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";
 	}
 }
 
@@ -5561,6 +5623,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

[PATCHv5 2/3] gitweb: add patches view

[Giuseppe Bilotta]

The only difference between patch and patches view is in the treatement
of single commits: the former only displays a single patch, whereas the
latter displays a patchset leading to the specified commit.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 1006dfe..e7a6477 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -530,6 +530,7 @@ our %actions = (
 	"history" => \&git_history,
 	"log" => \&git_log,
 	"patch" => \&git_patch,
+	"patches" => \&git_patches,
 	"rss" => \&git_rss,
 	"atom" => \&git_atom,
 	"search" => \&git_search,
@@ -5413,6 +5414,7 @@ sub git_blobdiff_plain {
 
 sub git_commitdiff {
 	my $format = shift || 'html';
+	my %params = @_;
 
 	my $patch_max;
 	if ($format eq 'patch') {
@@ -5529,7 +5531,15 @@ sub git_commitdiff {
 			}
 			push @commit_spec, '-n', "$hash_parent..$hash";
 		} else {
-			push @commit_spec, '-1', '--root', $hash;
+			if ($params{-single}) {
+				push @commit_spec, '-1';
+			} else {
+				if ($patch_max > 0) {
+					push @commit_spec, "-$patch_max";
+				}
+				push @commit_spec, "-n";
+			}
+			push @commit_spec, '--root', $hash;
 		}
 		open $fd, "-|", git_cmd(), "format-patch", '--encoding=utf8',
 			'--stdout', @commit_spec
@@ -5625,6 +5635,10 @@ sub git_commitdiff_plain {
 
 # format-patch-style patches
 sub git_patch {
+	git_commitdiff('patch', -single=> 1);
+}
+
+sub git_patches {
 	git_commitdiff('patch');
 }
 
-- 
1.5.6.5

[PATCHv5 3/3] gitweb: link to patch(es) view in commit(diff) and (short)log view

[Giuseppe Bilotta]

We link to patch view in commit and commitdiff view, and to patches view
in log and shortlog view.

In (short)log view, the link is only offered when the number of commits
shown is no more than the allowed maximum number of patches.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index e7a6477..168deb7 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5024,6 +5024,15 @@ sub git_log {
 
 	my $paging_nav = format_paging_nav('log', $hash, $head, $page, $#commitlist >= 100);
 
+	my ($patch_max) = gitweb_get_feature('patches');
+	if ($patch_max) {
+		if ($patch_max < 0 || @commitlist <= $patch_max) {
+			$paging_nav .= " &sdot; " .
+				$cgi->a({-href => href(action=>"patches", -replay=>1)},
+					"patches");
+		}
+	}
+
 	git_header_html();
 	git_print_page_nav('log','', $hash,undef,undef, $paging_nav);
 
@@ -5103,6 +5112,11 @@ sub git_commit {
 			} @$parents ) .
 			')';
 	}
+	if (gitweb_check_feature('patches')) {
+		$formats_nav .= " | " .
+			$cgi->a({-href => href(action=>"patch", -replay=>1)},
+				"patch");
+	}
 
 	if (!defined $parent) {
 		$parent = "--root";
@@ -5416,9 +5430,8 @@ sub git_commitdiff {
 	my $format = shift || 'html';
 	my %params = @_;
 
-	my $patch_max;
+	my ($patch_max) = gitweb_get_feature('patches');
 	if ($format eq 'patch') {
-		($patch_max) = gitweb_get_feature('patches');
 		die_error(403, "Patch view not allowed") unless $patch_max;
 	}
 
@@ -5436,6 +5449,11 @@ sub git_commitdiff {
 		$formats_nav =
 			$cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
 			        "raw");
+		if ($patch_max) {
+			$formats_nav .= " | " .
+				$cgi->a({-href => href(action=>"patch", -replay=>1)},
+					"patch");
+		}
 
 		if (defined $hash_parent &&
 		    $hash_parent ne '-c' && $hash_parent ne '--cc') {
@@ -5992,6 +6010,14 @@ sub git_shortlog {
 			$cgi->a({-href => href(-replay=>1, page=>$page+1),
 			         -accesskey => "n", -title => "Alt-n"}, "next");
 	}
+	my $patch_max = gitweb_check_feature('patches');
+	if ($patch_max) {
+		if ($patch_max < 0 || @commitlist <= $patch_max) {
+			$paging_nav .= " &sdot; " .
+				$cgi->a({-href => href(action=>"patches", -replay=>1)},
+					"patches");
+		}
+	}
 
 	git_header_html();
 	git_print_page_nav('shortlog','', $hash,$hash,$hash, $paging_nav);
-- 
1.5.6.5

Re: [PATCHv5 2/3] gitweb: add patches view

[Junio C Hamano]

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

>  sub git_commitdiff {
>  	my $format = shift || 'html';
> +	my %params = @_;
> ...  
> +			if ($params{-single}) {
> +				push @commit_spec, '-1';
> +			} else {
> +				if ($patch_max > 0) {
> ...
> +			}
> @@ -5625,6 +5635,10 @@ sub git_commitdiff_plain {
>  
>  # format-patch-style patches
>  sub git_patch {
> +	git_commitdiff('patch', -single=> 1);
> +}

Hmm, if you are changing the interface this way, wouldn't it make more
sense to also do this?

	git_commitdiff(--format => 'patch', --single => 1);
	git_commitdiff(--format => 'html');

Re: [PATCHv5 2/3] gitweb: add patches view

[Jakub Narebski]

On Thu, 18 Dec 2008, Junio C Hamano wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
> 
> >  sub git_commitdiff {
> >  	my $format = shift || 'html';
> > +	my %params = @_;
> > ...  
> > +			if ($params{-single}) {
> > +				push @commit_spec, '-1';
> > +			} else {
> > +				if ($patch_max > 0) {
> > ...
> > +			}
> > @@ -5625,6 +5635,10 @@ sub git_commitdiff_plain {
> >  
> >  # format-patch-style patches
> >  sub git_patch {
> > +	git_commitdiff('patch', -single=> 1);
> > +}
> 
> Hmm, if you are changing the interface this way, wouldn't it make more
> sense to also do this?
> 
> 	git_commitdiff(--format => 'patch', --single => 1);
> 	git_commitdiff(--format => 'html');

The first argument (format) is _required_, second is _optional_;
I'd rather use named parameters trick only for optional parameters.
Because with more than one optional parameter function call begins
to be cryptic; also flag (boolean) parameters are more readable
when used as named parameters.

-- 
Jakub Narebski
Poland

Re: [PATCHv5 2/3] gitweb: add patches view

[Giuseppe Bilotta]

On Thu, Dec 18, 2008 at 10:33 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Thu, 18 Dec 2008, Junio C Hamano wrote:
>> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>>
>> >  sub git_commitdiff {
>> >     my $format = shift || 'html';
>> > +   my %params = @_;
>> > ...
>> > +                   if ($params{-single}) {
>> > +                           push @commit_spec, '-1';
>> > +                   } else {
>> > +                           if ($patch_max > 0) {
>> > ...
>> > +                   }
>> > @@ -5625,6 +5635,10 @@ sub git_commitdiff_plain {
>> >
>> >  # format-patch-style patches
>> >  sub git_patch {
>> > +   git_commitdiff('patch', -single=> 1);
>> > +}
>>
>> Hmm, if you are changing the interface this way, wouldn't it make more
>> sense to also do this?
>>
>>       git_commitdiff(--format => 'patch', --single => 1);
>>       git_commitdiff(--format => 'html');
>
> The first argument (format) is _required_, second is _optional_;
> I'd rather use named parameters trick only for optional parameters.
> Because with more than one optional parameter function call begins
> to be cryptic; also flag (boolean) parameters are more readable
> when used as named parameters.

I have mixed feelings about this: on the one hand we have href() (say)
that takes all its params from a has, but on the other hand we have
esc_html() (say) that takes only additional options from a hash. I'm
personally more inclined towards the latter usage for git_commitdiff
(i.e. this patch) but since the other alternative is straightforward I
sent a v6 of the patchset which implements it.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv5 2/3] gitweb: add patches view

[Jakub Narebski]

On Thu, Dec 18, 2008, Giuseppe Bilotta wrote:
> On Thu, Dec 18, 2008, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Thu, 18 Dec 2008, Junio C Hamano wrote:

>>> [...] wouldn't it make more sense to also do this?
>>>
>>>       git_commitdiff(--format => 'patch', --single => 1);
>>>       git_commitdiff(--format => 'html');
>>
>> The first argument (format) is _required_, second is _optional_;
>> I'd rather use named parameters trick only for optional parameters.
>> Because with more than one optional parameter function call begins
>> to be cryptic; also flag (boolean) parameters are more readable
>> when used as named parameters.
> 
> I have mixed feelings about this: on the one hand we have href() (say)
> that takes all its params from a has, but on the other hand we have
> esc_html() (say) that takes only additional options from a hash. [...]

It is not "on one hand".  href() is a bit special case. It has _all_
parameters optional, especially now that -replay=>1 was introduced.
It has no required parameters. Therefore all parameters are named.

Beside, href() mimics a bit CGI interface...

-- 
Jakub Narebski
Poland

Re: [PATCHv5 2/3] gitweb: add patches view

[Jakub Narebski]

On Thu, 18 Dec 2008, Junio C Hamano wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
> 
> >  sub git_commitdiff {
> >  	my $format = shift || 'html';
> > +	my %params = @_;
> > ...  
> > +			if ($params{-single}) {
> > +				push @commit_spec, '-1';
> > +			} else {
> > +				if ($patch_max > 0) {
> > ...
> > +			}
> > @@ -5625,6 +5635,10 @@ sub git_commitdiff_plain {
> >  
> >  # format-patch-style patches
> >  sub git_patch {
> > +	git_commitdiff('patch', -single=> 1);
> > +}
> 
> Hmm, if you are changing the interface this way, wouldn't it make more
> sense to also do this?
> 
> 	git_commitdiff(--format => 'patch', --single => 1);
> 	git_commitdiff(--format => 'html');

Just a though, but does it really take much sense to have "patch" and
"patches" views handled in git_commitdiff? I can understand in the
first version, where it was more about better 'commitdiff_plain', but
I wonder about now, where patch(es) view is something between git show
and log_plain format... Wouldn't it be simpler to use separate 
subroutine, for example git_format_patch or something?

-- 
Jakub Narebski
Poland

Re: [PATCHv5 2/3] gitweb: add patches view

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> On Thu, 18 Dec 2008, Junio C Hamano wrote:
>> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>> 
>> >  sub git_commitdiff {
>> >  	my $format = shift || 'html';
>> > +	my %params = @_;
>> > ...  
>> > +			if ($params{-single}) {
>> > +				push @commit_spec, '-1';
>> > +			} else {
>> > +				if ($patch_max > 0) {
>> > ...
>> > +			}
>> > @@ -5625,6 +5635,10 @@ sub git_commitdiff_plain {
>> >  
>> >  # format-patch-style patches
>> >  sub git_patch {
>> > +	git_commitdiff('patch', -single=> 1);
>> > +}
>> 
>> Hmm, if you are changing the interface this way, wouldn't it make more
>> sense to also do this?
>> 
>> 	git_commitdiff(--format => 'patch', --single => 1);
>> 	git_commitdiff(--format => 'html');
>
> The first argument (format) is _required_, second is _optional_;

My reading of the first line of the sub seems to contradict with your
statement.  "If the first argument is empty, we use 'html' instead" sounds
pretty optional to me.

Re: [PATCHv5 2/3] gitweb: add patches view

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>> On Thu, 18 Dec 2008, Junio C Hamano wrote:
>>> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>>> 
>>>>  sub git_commitdiff {
>>>>  	my $format = shift || 'html';
>>>> +	my %params = @_;
>>>> ...  
>>>> +			if ($params{-single}) {
>>>> +				push @commit_spec, '-1';
>>>> +			} else {
>>>> +				if ($patch_max> 0) {
>>>> ...
>>>> +			}
>>>> @@ -5625,6 +5635,10 @@ sub git_commitdiff_plain {
>>>>  
>>>>  # format-patch-style patches
>>>>  sub git_patch {
>>>> +	git_commitdiff('patch', -single=> 1);
>>>> +}
>>> 
>>> Hmm, if you are changing the interface this way, wouldn't it make more
>>> sense to also do this?
>>> 
>>> 	git_commitdiff(--format => 'patch', --single => 1);
>>> 	git_commitdiff(--format => 'html');
>>
>> The first argument (format) is _required_, second is _optional_;
> 
> My reading of the first line of the sub seems to contradict with your
> statement.  "If the first argument is empty, we use 'html' instead" sounds
> pretty optional to me.
 
Well, for me it is more of required argument, but with default value.
But now we argue semantics... :-)

-- 
Jakub Narebski
Poland

Re: [PATCHv5 2/3] gitweb: add patches view

[Giuseppe Bilotta]

On Thu, Dec 18, 2008 at 7:15 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>
> Just a though, but does it really take much sense to have "patch" and
> "patches" views handled in git_commitdiff? I can understand in the
> first version, where it was more about better 'commitdiff_plain', but
> I wonder about now, where patch(es) view is something between git show
> and log_plain format... Wouldn't it be simpler to use separate
> subroutine, for example git_format_patch or something?

I don't feel like much has changed since the beginning when IYO it
made sense to have it part of commitdiff, honestly

-- 
Giuseppe "Oblomov" Bilotta