[RFCv2 0/2] gitweb: patch view

[RFCv2 0/2] gitweb: patch view

[Giuseppe Bilotta]

Another go at the patch view feature. I'm marking this as RFC because
there are still a couple of points that need to be agreed on, esp. wrt
to the patch limiting and the insertion of extra X-Git-* email headers.

Giuseppe Bilotta (2):
  gitweb: add patch view
  gitweb: links to patch action in commitdiff and shortlog view

 Makefile           |    2 ++
 gitweb/gitweb.perl |   39 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 39 insertions(+), 2 deletions(-)

[RFCv2 1/2] gitweb: add patch view

[Giuseppe Bilotta]

The manually-built email format in commitdiff_plain output is not
appropriate for feeding git-am, because of two limitations:
 * when a range of commits is specified, commitdiff_plain publishes a
   single patch with the message from the first commit, instead of a
   patchset,
 * in either case, the patch summary is replicated both as email subject
   and as first line of the email itself, resulting in a doubled summary
   if the output is fed to git-am.

We thus create a new view that can be fed to git-am directly by exposing
the output of git format-patch directly. This allows patch exchange and
submission via gitweb. A hard limit (configurable, defaults to 100) is
imposed on the number of commits which will be included in a patchset,
to prevent DoS attacks on the server.

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

diff --git a/Makefile b/Makefile
index 5a69a41..dbf414c 100644
--- a/Makefile
+++ b/Makefile
@@ -220,6 +220,7 @@ GITWEB_LOGO = git-logo.png
 GITWEB_FAVICON = git-favicon.png
 GITWEB_SITE_HEADER =
 GITWEB_SITE_FOOTER =
+GITWEB_PATCH_MAX = 100
 
 export prefix bindir sharedir htmldir sysconfdir
 
@@ -1210,6 +1211,7 @@ gitweb/gitweb.cgi: gitweb/gitweb.perl
 	    -e 's|++GITWEB_FAVICON++|$(GITWEB_FAVICON)|g' \
 	    -e 's|++GITWEB_SITE_HEADER++|$(GITWEB_SITE_HEADER)|g' \
 	    -e 's|++GITWEB_SITE_FOOTER++|$(GITWEB_SITE_FOOTER)|g' \
+	    -e 's|++GITWEB_PATCH_MAX++|$(GITWEB_PATCH_MAX)|g' \
 	    $< >$@+ && \
 	chmod +x $@+ && \
 	mv $@+ $@
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 2738643..10cbe93 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -87,6 +87,9 @@ our $projects_list = "++GITWEB_LIST++";
 # the width (in characters) of the projects list "Description" column
 our $projects_list_description_width = 25;
 
+# the maximum number of patches allowed in patch view
+our $patch_max = "++GITWEB_PATCH_MAX++";
+
 # default order of projects list
 # valid values are none, project, descr, owner, and age
 our $default_projects_order = "project";
@@ -503,6 +506,7 @@ our %actions = (
 	"heads" => \&git_heads,
 	"history" => \&git_history,
 	"log" => \&git_log,
+	"patch" => \&git_patch,
 	"rss" => \&git_rss,
 	"atom" => \&git_atom,
 	"search" => \&git_search,
@@ -5483,7 +5487,12 @@ 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') {
+		open $fd, "-|", git_cmd(), "format-patch", '--encoding=utf8',
+			'--stdout', "-$patch_max",
+			$hash_parent ? ('-n', "$hash_parent..$hash") :
+			('--root', '-1', $hash)
+			or die_error(500, "Open git-format-patch failed");
 	} else {
 		die_error(400, "Unknown commitdiff format");
 	}
@@ -5532,6 +5541,15 @@ 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" . '"');
+		# TODO add X-Git-Tag/X-Git-Url headers in a sensible way
 	}
 
 	# write patch
@@ -5553,6 +5571,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 +5583,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

[RFCv2 2/2] gitweb: links to patch action in commitdiff and shortlog view

[Giuseppe Bilotta]

In shortlog view, a link to the patchset is only offered when the number
of commits shown is less than the allowed maximum number of patches.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 10cbe93..aea0e07 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5403,7 +5403,9 @@ sub git_commitdiff {
 	if ($format eq 'html') {
 		$formats_nav =
 			$cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
-			        "raw");
+			        "raw") . " | " .
+			$cgi->a({-href => href(action=>"patch", -replay=>1)},
+			        "patch");
 
 		if (defined $hash_parent &&
 		    $hash_parent ne '-c' && $hash_parent ne '--cc') {
@@ -5938,6 +5940,11 @@ sub git_shortlog {
 			$cgi->a({-href => href(-replay=>1, page=>$page+1),
 			         -accesskey => "n", -title => "Alt-n"}, "next");
 	}
+	if ($#commitlist <= $patch_max) {
+		$paging_nav .= " &sdot; " .
+			$cgi->a({-href => href(action=>"patch", -replay=>1)},
+			        $#commitlist > 1 ? "patchset" : "patch");
+	}
 
 	git_header_html();
 	git_print_page_nav('shortlog','', $hash,$hash,$hash, $paging_nav);
-- 
1.5.6.5

Re: [RFCv2 1/2] gitweb: add patch view

[Junio C Hamano]

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

> We thus create a new view that can be fed to git-am directly by exposing
> the output of git format-patch directly. This allows patch exchange and
> submission via gitweb. A hard limit (configurable, defaults to 100) is
> imposed on the number of commits which will be included in a patchset,
> to prevent DoS attacks on the server.

Hmm, I would imagine that "snapshot" would be a much more effective way to
do such an attack, and notice the way we prevent it is to selectively
enable the feature per repository.

Perhaps this configuration should also be a feature defined in %feature,
overridable by each repository?  If you default it to "disabled" (as any
new feature typically does), you do not have to yank a random number such
as 100 out of thin air.

Re: [RFCv2 1/2] gitweb: add patch view

[Giuseppe Bilotta]

On Wed, Dec 3, 2008 at 12:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> We thus create a new view that can be fed to git-am directly by exposing
>> the output of git format-patch directly. This allows patch exchange and
>> submission via gitweb. A hard limit (configurable, defaults to 100) is
>> imposed on the number of commits which will be included in a patchset,
>> to prevent DoS attacks on the server.
>
> Hmm, I would imagine that "snapshot" would be a much more effective way to
> do such an attack, and notice the way we prevent it is to selectively
> enable the feature per repository.
>
> Perhaps this configuration should also be a feature defined in %feature,
> overridable by each repository?  If you default it to "disabled" (as any
> new feature typically does), you do not have to yank a random number such
> as 100 out of thin air.

I thought about it, but then I thought it was way too useful for
single patches to disable the feature a priori. I'd rather make the
default limit much smaller (like the original 16 commits I had in
mind, or even less).



-- 
Giuseppe "Oblomov" Bilotta

Re: [RFCv2 1/2] gitweb: add patch view

[Jakub Narebski]

On Wed, 3 Dec 2008, Giuseppe Bilotta wrote:
> On Wed, Dec 3, 2008 at 12:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>>
>>> We thus create a new view that can be fed to git-am directly by exposing
>>> the output of git format-patch directly. This allows patch exchange and
>>> submission via gitweb. A hard limit (configurable, defaults to 100) is
>>> imposed on the number of commits which will be included in a patchset,
>>> to prevent DoS attacks on the server.
>>
>> Hmm, I would imagine that "snapshot" would be a much more effective way to
>> do such an attack, and notice the way we prevent it is to selectively
>> enable the feature per repository.
>>
>> Perhaps this configuration should also be a feature defined in %feature,
>> overridable by each repository?  If you default it to "disabled" (as any
>> new feature typically does), you do not have to yank a random number such
>> as 100 out of thin air.
> 
> I thought about it, but then I thought it was way too useful for
> single patches to disable the feature a priori. I'd rather make the
> default limit much smaller (like the original 16 commits I had in
> mind, or even less).

Perhaps %feature can be used to configure _maximum_ number of patches
in 'patch' / 'format_patch' view (gitweb_get_feature... well, sort of
as gitweb_check_feature would work too), rather than checking if it
is enabled or disabled?

-- 
Jakub Narebski
Poland

Re: [RFCv2 1/2] gitweb: add patch view

[Giuseppe Bilotta]

On Wed, Dec 3, 2008 at 2:00 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Wed, 3 Dec 2008, Giuseppe Bilotta wrote:
>> On Wed, Dec 3, 2008 at 12:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>> Perhaps this configuration should also be a feature defined in %feature,
>>> overridable by each repository?  If you default it to "disabled" (as any
>>> new feature typically does), you do not have to yank a random number such
>>> as 100 out of thin air.
>>
>> I thought about it, but then I thought it was way too useful for
>> single patches to disable the feature a priori. I'd rather make the
>> default limit much smaller (like the original 16 commits I had in
>> mind, or even less).
>
> Perhaps %feature can be used to configure _maximum_ number of patches
> in 'patch' / 'format_patch' view (gitweb_get_feature... well, sort of
> as gitweb_check_feature would work too), rather than checking if it
> is enabled or disabled?

The way it's implemented in v2, you just need to set $patch_max in
your local or system config file (e.g. /etc/gitweb.conf). I'm not sure
about the benefit we would gain in going through %feature.

-- 
Giuseppe "Oblomov" Bilotta

Re: [RFCv2 1/2] gitweb: add patch view

[Jakub Narebski]

On Wed, Dec 3, 2008 at 14:14, Giuseppe Bilotta wrote:
> On Wed, Dec 3, 2008 at 2:00 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Wed, 3 Dec 2008, Giuseppe Bilotta wrote:
>>> On Wed, Dec 3, 2008 at 12:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>>
>>>> Perhaps this configuration should also be a feature defined in %feature,
>>>> overridable by each repository?  If you default it to "disabled" (as any
>>>> new feature typically does), you do not have to yank a random number such
>>>> as 100 out of thin air.
>>>
>>> I thought about it, but then I thought it was way too useful for
>>> single patches to disable the feature a priori. I'd rather make the
>>> default limit much smaller (like the original 16 commits I had in
>>> mind, or even less).
>>
>> Perhaps %feature can be used to configure _maximum_ number of patches
>> in 'patch' / 'format_patch' view (gitweb_get_feature... well, sort of
>> as gitweb_check_feature would work too), rather than checking if it
>> is enabled or disabled?
> 
> The way it's implemented in v2, you just need to set $patch_max in
> your local or system config file (e.g. /etc/gitweb.conf). I'm not sure
> about the benefit we would gain in going through %feature.

Ah, I haven't read patch in detail yet.

The (doubtful or not) benefit of going through %feature would be ability
to set limits (with perhaps -1 / <0 / undef / '' meaning: unlimited) on
per repository basis, with no limit for small repository, some limit for
the rest, and no 'patch' view or heavily limited for repository with
large size commits.

Just my 2 cents.
-- 
Jakub Narebski
Poland

Re: [RFCv2 1/2] gitweb: add patch view

[Giuseppe Bilotta]

On Wed, Dec 3, 2008 at 6:08 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Wed, Dec 3, 2008 at 14:14, Giuseppe Bilotta wrote:
>>
>> The way it's implemented in v2, you just need to set $patch_max in
>> your local or system config file (e.g. /etc/gitweb.conf). I'm not sure
>> about the benefit we would gain in going through %feature.
>
> Ah, I haven't read patch in detail yet.
>
> The (doubtful or not) benefit of going through %feature would be ability
> to set limits (with perhaps -1 / <0 / undef / '' meaning: unlimited) on
> per repository basis, with no limit for small repository, some limit for
> the rest, and no 'patch' view or heavily limited for repository with
> large size commits.

Hm. I'm not entirely sure it would be used at all, but I think this
could be done. Something like the following:
 * perl false meaning: feature disabled
 * > 0 maximum number of patches
 * does Perl have an 'infinity' value? if not, we can use <0 to mean unlimited

-- 
Giuseppe "Oblomov" Bilotta