{"thread":{"id":"16574","subject":"[RFCv2 0/2] gitweb: patch view","startedAt":"2008-12-03T10:07:40Z","lastAt":"2008-12-03T20:52:00Z","messageCount":9,"participants":["Giuseppe Bilotta","Junio C Hamano","Jakub Narebski"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"97042","messageId":"1228298862-28191-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"16574","inReplyTo":null,"subject":"[RFCv2 0/2] gitweb: patch view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-03T10:07:40Z","receivedAt":"2008-12-03T10:07:40Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Another go at the patch view feature. I'm marking this as RFC because\nthere are still a couple of points that need to be agreed on, esp. wrt\nto the patch limiting and the insertion of extra X-Git-* email headers.\n\nGiuseppe Bilotta (2):\n gitweb: add patch view\n gitweb: links to patch action in commitdiff and shortlog view\n\n Makefile | 2 ++\n gitweb/gitweb.perl | 39 +++++++++++++++++++++++++++++++++++++--\n 2 files changed, 39 insertions(+), 2 deletions(-)\n"},{"id":"97044","messageId":"1228298862-28191-2-git-send-email-giuseppe.bilotta@gmail.com","threadId":"16574","inReplyTo":"1228298862-28191-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[RFCv2 1/2] gitweb: add patch view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-03T10:07:41Z","receivedAt":"2008-12-03T10:07:41Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"The manually-built email format in commitdiff_plain output is not\nappropriate for feeding git-am, because of two limitations:\n * when a range of commits is specified, commitdiff_plain publishes a\n single patch with the message from the first commit, instead of a\n patchset,\n * in either case, the patch summary is replicated both as email subject\n and as first line of the email itself, resulting in a doubled summary\n if the output is fed to git-am.\n\nWe thus create a new view that can be fed to git-am directly by exposing\nthe output of git format-patch directly. This allows patch exchange and\nsubmission via gitweb. A hard limit (configurable, defaults to 100) is\nimposed on the number of commits which will be included in a patchset,\nto prevent DoS attacks on the server.\n\nSigned-off-by: Giuseppe Bilotta \n---\n Makefile | 2 ++\n gitweb/gitweb.perl | 30 +++++++++++++++++++++++++++++-\n 2 files changed, 31 insertions(+), 1 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 5a69a41..dbf414c 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -220,6 +220,7 @@ GITWEB_LOGO = git-logo.png\n GITWEB_FAVICON = git-favicon.png\n GITWEB_SITE_HEADER =\n GITWEB_SITE_FOOTER =\n+GITWEB_PATCH_MAX = 100\n \n export prefix bindir sharedir htmldir sysconfdir\n \n@@ -1210,6 +1211,7 @@ gitweb/gitweb.cgi: gitweb/gitweb.perl\n \t -e 's|++GITWEB_FAVICON++|$(GITWEB_FAVICON)|g' \\\n \t -e 's|++GITWEB_SITE_HEADER++|$(GITWEB_SITE_HEADER)|g' \\\n \t -e 's|++GITWEB_SITE_FOOTER++|$(GITWEB_SITE_FOOTER)|g' \\\n+\t -e 's|++GITWEB_PATCH_MAX++|$(GITWEB_PATCH_MAX)|g' \\\n \t $< >$@+ && \\\n \tchmod +x $@+ && \\\n \tmv $@+ $@\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 2738643..10cbe93 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -87,6 +87,9 @@ our $projects_list = \"++GITWEB_LIST++\";\n # the width (in characters) of the projects list \"Description\" column\n our $projects_list_description_width = 25;\n \n+# the maximum number of patches allowed in patch view\n+our $patch_max = \"++GITWEB_PATCH_MAX++\";\n+\n # default order of projects list\n # valid values are none, project, descr, owner, and age\n our $default_projects_order = \"project\";\n@@ -503,6 +506,7 @@ our %actions = (\n \t\"heads\" => \\&git_heads,\n \t\"history\" => \\&git_history,\n \t\"log\" => \\&git_log,\n+\t\"patch\" => \\&git_patch,\n \t\"rss\" => \\&git_rss,\n \t\"atom\" => \\&git_atom,\n \t\"search\" => \\&git_search,\n@@ -5483,7 +5487,12 @@ sub git_commitdiff {\n \t\topen $fd, \"-|\", git_cmd(), \"diff-tree\", '-r', @diff_opts,\n \t\t\t'-p', $hash_parent_param, $hash, \"--\"\n \t\t\tor die_error(500, \"Open git-diff-tree failed\");\n-\n+\t} elsif ($format eq 'patch') {\n+\t\topen $fd, \"-|\", git_cmd(), \"format-patch\", '--encoding=utf8',\n+\t\t\t'--stdout', \"-$patch_max\",\n+\t\t\t$hash_parent ? ('-n', \"$hash_parent..$hash\") :\n+\t\t\t('--root', '-1', $hash)\n+\t\t\tor die_error(500, \"Open git-format-patch failed\");\n \t} else {\n \t\tdie_error(400, \"Unknown commitdiff format\");\n \t}\n@@ -5532,6 +5541,15 @@ sub git_commitdiff {\n \t\t\tprint to_utf8($line) . \"\\n\";\n \t\t}\n \t\tprint \"---\\n\\n\";\n+\t} elsif ($format eq 'patch') {\n+\t\tmy $filename = basename($project) . \"-$hash.patch\";\n+\n+\t\tprint $cgi->header(\n+\t\t\t-type => 'text/plain',\n+\t\t\t-charset => 'utf-8',\n+\t\t\t-expires => $expires,\n+\t\t\t-content_disposition => 'inline; filename=\"' . \"$filename\" . '\"');\n+\t\t# TODO add X-Git-Tag/X-Git-Url headers in a sensible way\n \t}\n \n \t# write patch\n@@ -5553,6 +5571,11 @@ sub git_commitdiff {\n \t\tprint <$fd>;\n \t\tclose $fd\n \t\t\tor print \"Reading git-diff-tree failed\\n\";\n+\t} elsif ($format eq 'patch') {\n+\t\tlocal $/ = undef;\n+\t\tprint <$fd>;\n+\t\tclose $fd\n+\t\t\tor print \"Reading git-format-patch failed\\n\";\n \t}\n }\n \n@@ -5560,6 +5583,11 @@ sub git_commitdiff_plain {\n \tgit_commitdiff('plain');\n }\n \n+# format-patch-style patches\n+sub git_patch {\n+\tgit_commitdiff('patch');\n+}\n+\n sub git_history {\n \tif (!defined $hash_base) {\n \t\t$hash_base = git_get_head_hash($project);\n-- \n1.5.6.5\n"},{"id":"97043","messageId":"1228298862-28191-3-git-send-email-giuseppe.bilotta@gmail.com","threadId":"16574","inReplyTo":"1228298862-28191-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"[RFCv2 2/2] gitweb: links to patch action in commitdiff and shortlog view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-03T10:07:42Z","receivedAt":"2008-12-03T10:07:42Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"In shortlog view, a link to the patchset is only offered when the number\nof commits shown is less than the allowed maximum number of patches.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 9 ++++++++-\n 1 files changed, 8 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 10cbe93..aea0e07 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5403,7 +5403,9 @@ sub git_commitdiff {\n \tif ($format eq 'html') {\n \t\t$formats_nav =\n \t\t\t$cgi->a({-href => href(action=>\"commitdiff_plain\", -replay=>1)},\n-\t\t\t \"raw\");\n+\t\t\t \"raw\") . \" | \" .\n+\t\t\t$cgi->a({-href => href(action=>\"patch\", -replay=>1)},\n+\t\t\t \"patch\");\n \n \t\tif (defined $hash_parent &&\n \t\t $hash_parent ne '-c' && $hash_parent ne '--cc') {\n@@ -5938,6 +5940,11 @@ sub git_shortlog {\n \t\t\t$cgi->a({-href => href(-replay=>1, page=>$page+1),\n \t\t\t -accesskey => \"n\", -title => \"Alt-n\"}, \"next\");\n \t}\n+\tif ($#commitlist <= $patch_max) {\n+\t\t$paging_nav .= \" ⋅ \" .\n+\t\t\t$cgi->a({-href => href(action=>\"patch\", -replay=>1)},\n+\t\t\t $#commitlist > 1 ? \"patchset\" : \"patch\");\n+\t}\n \n \tgit_header_html();\n \tgit_print_page_nav('shortlog','', $hash,$hash,$hash, $paging_nav);\n-- \n1.5.6.5\n"},{"id":"97048","messageId":"7vbpvtecb6.fsf@gitster.siamese.dyndns.org","threadId":"16574","inReplyTo":"1228298862-28191-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [RFCv2 1/2] gitweb: add patch view","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-03T11:19:09Z","receivedAt":"2008-12-03T11:19:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta writes:\n\n> We thus create a new view that can be fed to git-am directly by exposing\n> the output of git format-patch directly. This allows patch exchange and\n> submission via gitweb. A hard limit (configurable, defaults to 100) is\n> imposed on the number of commits which will be included in a patchset,\n> to prevent DoS attacks on the server.\n\nHmm, I would imagine that \"snapshot\" would be a much more effective way to\ndo such an attack, and notice the way we prevent it is to selectively\nenable the feature per repository.\n\nPerhaps this configuration should also be a feature defined in %feature,\noverridable by each repository? If you default it to \"disabled\" (as any\nnew feature typically does), you do not have to yank a random number such\nas 100 out of thin air.\n"},{"id":"97052","messageId":"cb7bb73a0812030333n2cfdc745m3a910d2e8677e7fe@mail.gmail.com","threadId":"16574","inReplyTo":"7vbpvtecb6.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFCv2 1/2] gitweb: add patch view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-03T11:33:52Z","receivedAt":"2008-12-03T11:33:52Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Wed, Dec 3, 2008 at 12:19 PM, Junio C Hamano wrote:\n> Giuseppe Bilotta writes:\n>\n>> We thus create a new view that can be fed to git-am directly by exposing\n>> the output of git format-patch directly. This allows patch exchange and\n>> submission via gitweb. A hard limit (configurable, defaults to 100) is\n>> imposed on the number of commits which will be included in a patchset,\n>> to prevent DoS attacks on the server.\n>\n> Hmm, I would imagine that \"snapshot\" would be a much more effective way to\n> do such an attack, and notice the way we prevent it is to selectively\n> enable the feature per repository.\n>\n> Perhaps this configuration should also be a feature defined in %feature,\n> overridable by each repository? If you default it to \"disabled\" (as any\n> new feature typically does), you do not have to yank a random number such\n> as 100 out of thin air.\n\nI thought about it, but then I thought it was way too useful for\nsingle patches to disable the feature a priori. I'd rather make the\ndefault limit much smaller (like the original 16 commits I had in\nmind, or even less).\n\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"97056","messageId":"200812031400.50980.jnareb@gmail.com","threadId":"16574","inReplyTo":"cb7bb73a0812030333n2cfdc745m3a910d2e8677e7fe@mail.gmail.com","subject":"Re: [RFCv2 1/2] gitweb: add patch view","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-03T13:00:50Z","receivedAt":"2008-12-03T13:00:50Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, 3 Dec 2008, Giuseppe Bilotta wrote:\n> On Wed, Dec 3, 2008 at 12:19 PM, Junio C Hamano wrote:\n>> Giuseppe Bilotta writes:\n>>\n>>> We thus create a new view that can be fed to git-am directly by exposing\n>>> the output of git format-patch directly. This allows patch exchange and\n>>> submission via gitweb. A hard limit (configurable, defaults to 100) is\n>>> imposed on the number of commits which will be included in a patchset,\n>>> to prevent DoS attacks on the server.\n>>\n>> Hmm, I would imagine that \"snapshot\" would be a much more effective way to\n>> do such an attack, and notice the way we prevent it is to selectively\n>> enable the feature per repository.\n>>\n>> Perhaps this configuration should also be a feature defined in %feature,\n>> overridable by each repository? If you default it to \"disabled\" (as any\n>> new feature typically does), you do not have to yank a random number such\n>> as 100 out of thin air.\n> \n> I thought about it, but then I thought it was way too useful for\n> single patches to disable the feature a priori. I'd rather make the\n> default limit much smaller (like the original 16 commits I had in\n> mind, or even less).\n\nPerhaps %feature can be used to configure _maximum_ number of patches\nin 'patch' / 'format_patch' view (gitweb_get_feature... well, sort of\nas gitweb_check_feature would work too), rather than checking if it\nis enabled or disabled?\n\n-- \nJakub Narebski\nPoland\n"},{"id":"97058","messageId":"cb7bb73a0812030514u11e10bebue5a1451d54fc3f96@mail.gmail.com","threadId":"16574","inReplyTo":"200812031400.50980.jnareb@gmail.com","subject":"Re: [RFCv2 1/2] gitweb: add patch view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-03T13:14:15Z","receivedAt":"2008-12-03T13:14:15Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Wed, Dec 3, 2008 at 2:00 PM, Jakub Narebski wrote:\n> On Wed, 3 Dec 2008, Giuseppe Bilotta wrote:\n>> On Wed, Dec 3, 2008 at 12:19 PM, Junio C Hamano wrote:\n>>>\n>>> Perhaps this configuration should also be a feature defined in %feature,\n>>> overridable by each repository? If you default it to \"disabled\" (as any\n>>> new feature typically does), you do not have to yank a random number such\n>>> as 100 out of thin air.\n>>\n>> I thought about it, but then I thought it was way too useful for\n>> single patches to disable the feature a priori. I'd rather make the\n>> default limit much smaller (like the original 16 commits I had in\n>> mind, or even less).\n>\n> Perhaps %feature can be used to configure _maximum_ number of patches\n> in 'patch' / 'format_patch' view (gitweb_get_feature... well, sort of\n> as gitweb_check_feature would work too), rather than checking if it\n> is enabled or disabled?\n\nThe way it's implemented in v2, you just need to set $patch_max in\nyour local or system config file (e.g. /etc/gitweb.conf). I'm not sure\nabout the benefit we would gain in going through %feature.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"97078","messageId":"200812031808.01916.jnareb@gmail.com","threadId":"16574","inReplyTo":"cb7bb73a0812030514u11e10bebue5a1451d54fc3f96@mail.gmail.com","subject":"Re: [RFCv2 1/2] gitweb: add patch view","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-03T17:08:01Z","receivedAt":"2008-12-03T17:08:01Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, Dec 3, 2008 at 14:14, Giuseppe Bilotta wrote:\n> On Wed, Dec 3, 2008 at 2:00 PM, Jakub Narebski wrote:\n>> On Wed, 3 Dec 2008, Giuseppe Bilotta wrote:\n>>> On Wed, Dec 3, 2008 at 12:19 PM, Junio C Hamano wrote:\n>>>>\n>>>> Perhaps this configuration should also be a feature defined in %feature,\n>>>> overridable by each repository? If you default it to \"disabled\" (as any\n>>>> new feature typically does), you do not have to yank a random number such\n>>>> as 100 out of thin air.\n>>>\n>>> I thought about it, but then I thought it was way too useful for\n>>> single patches to disable the feature a priori. I'd rather make the\n>>> default limit much smaller (like the original 16 commits I had in\n>>> mind, or even less).\n>>\n>> Perhaps %feature can be used to configure _maximum_ number of patches\n>> in 'patch' / 'format_patch' view (gitweb_get_feature... well, sort of\n>> as gitweb_check_feature would work too), rather than checking if it\n>> is enabled or disabled?\n> \n> The way it's implemented in v2, you just need to set $patch_max in\n> your local or system config file (e.g. /etc/gitweb.conf). I'm not sure\n> about the benefit we would gain in going through %feature.\n\nAh, I haven't read patch in detail yet.\n\nThe (doubtful or not) benefit of going through %feature would be ability\nto set limits (with perhaps -1 / <0 / undef / '' meaning: unlimited) on\nper repository basis, with no limit for small repository, some limit for\nthe rest, and no 'patch' view or heavily limited for repository with\nlarge size commits.\n\nJust my 2 cents.\n-- \nJakub Narebski\nPoland\n"},{"id":"97093","messageId":"cb7bb73a0812031252t7f88cedfic266c218f707cee7@mail.gmail.com","threadId":"16574","inReplyTo":"200812031808.01916.jnareb@gmail.com","subject":"Re: [RFCv2 1/2] gitweb: add patch view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-03T20:52:00Z","receivedAt":"2008-12-03T20:52:00Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Wed, Dec 3, 2008 at 6:08 PM, Jakub Narebski wrote:\n> On Wed, Dec 3, 2008 at 14:14, Giuseppe Bilotta wrote:\n>>\n>> The way it's implemented in v2, you just need to set $patch_max in\n>> your local or system config file (e.g. /etc/gitweb.conf). I'm not sure\n>> about the benefit we would gain in going through %feature.\n>\n> Ah, I haven't read patch in detail yet.\n>\n> The (doubtful or not) benefit of going through %feature would be ability\n> to set limits (with perhaps -1 / <0 / undef / '' meaning: unlimited) on\n> per repository basis, with no limit for small repository, some limit for\n> the rest, and no 'patch' view or heavily limited for repository with\n> large size commits.\n\nHm. I'm not entirely sure it would be used at all, but I think this\ncould be done. Something like the following:\n * perl false meaning: feature disabled\n * > 0 maximum number of patches\n * does Perl have an 'infinity' value? if not, we can use <0 to mean unlimited\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"}]}