Giuseppe BilottaAnother 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(-)
Giuseppe Bilotta[RFCv2 1/2] gitweb: add patch view
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
Giuseppe Bilotta[RFCv2 2/2] gitweb: links to patch action in commitdiff and shortlog view
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 .= " ⋅ " . + $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
Junio C HamanoRe: [RFCv2 1/2] gitweb: add patch view
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.
Giuseppe BilottaRe: [RFCv2 1/2] gitweb: add patch view
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
Jakub NarebskiRe: [RFCv2 1/2] gitweb: add patch view
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
Giuseppe BilottaRe: [RFCv2 1/2] gitweb: add patch view
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
Jakub NarebskiRe: [RFCv2 1/2] gitweb: add patch view
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
Giuseppe BilottaRe: [RFCv2 1/2] gitweb: add patch view
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