{"thread":{"id":"16590","subject":"[RFCv3 0/2] gitweb: patch view","startedAt":"2008-12-03T22:59:46Z","lastAt":"2008-12-06T15:25:53Z","messageCount":20,"participants":["Giuseppe Bilotta","Junio C Hamano","Jakub Narebski","Petr Baudis"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"97106","messageId":"1228345188-15125-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"16590","inReplyTo":null,"subject":"[RFCv3 0/2] gitweb: patch view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-03T22:59:46Z","receivedAt":"2008-12-03T22:59:46Z","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 gitweb/gitweb.perl |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 files changed, 58 insertions(+), 1 deletions(-)\n"},{"id":"97107","messageId":"1228345188-15125-2-git-send-email-giuseppe.bilotta@gmail.com","threadId":"16590","inReplyTo":"1228345188-15125-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[RFCv3 1/2] gitweb: add patch view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-03T22:59:47Z","receivedAt":"2008-12-03T22:59:47Z","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.\n\nA configurable limit is imposed on the number of commits which will be\nincluded in a patchset, to prevent DoS attacks on the server. Setting\nthe limit to 0 will disable the patch view, setting it to a negative\nnumber will remove the limit.\n\nSigned-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>\n---\n gitweb/gitweb.perl |   41 ++++++++++++++++++++++++++++++++++++++++-\n 1 files changed, 40 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 2738643..c9abfcf 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -329,6 +329,13 @@ our %feature = (\n \t'ctags' => {\n \t\t'override' => 0,\n \t\t'default' => [0]},\n+\n+\t# The maximum number of patches in a patchset generated in patch\n+\t# view. Set this to 0 or undef to disable patch view, or to a\n+\t# negative number to remove any limit.\n+\t'patches' => {\n+\t\t'override' => 1,\n+\t\t'default' => [16]},\n );\n \n sub gitweb_get_feature {\n@@ -503,6 +510,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@@ -5386,6 +5394,12 @@ sub git_blobdiff_plain {\n \n sub git_commitdiff {\n \tmy $format = shift || 'html';\n+\n+\tmy $patch_max = gitweb_check_feature('patches');\n+\tif ($format eq 'patch') {\n+\t\tdie_error(403, \"Patch view not allowed\") unless $patch_max;\n+\t}\n+\n \t$hash ||= $hash_base || \"HEAD\";\n \tmy %co = parse_commit($hash)\n \t    or die_error(404, \"Unknown commit object\");\n@@ -5396,6 +5410,7 @@ sub git_commitdiff {\n \t}\n \t# we need to prepare $formats_nav before almost any parameter munging\n \tmy $formats_nav;\n+\n \tif ($format eq 'html') {\n \t\t$formats_nav =\n \t\t\t$cgi->a({-href => href(action=>\"commitdiff_plain\", -replay=>1)},\n@@ -5483,7 +5498,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 > 0 ? \"-$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 +5552,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 +5582,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 +5594,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":"97109","messageId":"1228345188-15125-3-git-send-email-giuseppe.bilotta@gmail.com","threadId":"16590","inReplyTo":"1228345188-15125-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"[RFCv3 2/2] gitweb: links to patch action in commitdiff and shortlog view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-03T22:59:48Z","receivedAt":"2008-12-03T22:59:48Z","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 <giuseppe.bilotta@gmail.com>\n---\n gitweb/gitweb.perl |   18 ++++++++++++++++++\n 1 files changed, 18 insertions(+), 0 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex c9abfcf..ec8fc7d 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5083,6 +5083,11 @@ sub git_commit {\n \t\t\t} @$parents ) .\n \t\t\t')';\n \t}\n+\tif (gitweb_check_feature('patches')) {\n+\t\t$formats_nav .= \" | \" .\n+\t\t\t$cgi->a({-href => href(action=>\"patch\", -replay=>1)},\n+\t\t\t\t\"patch\");\n+\t}\n \n \tif (!defined $parent) {\n \t\t$parent = \"--root\";\n@@ -5415,6 +5420,11 @@ sub git_commitdiff {\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\tif ($patch_max) {\n+\t\t\t$formats_nav .= \" | \" .\n+\t\t\t\t$cgi->a({-href => href(action=>\"patch\", -replay=>1)},\n+\t\t\t\t\t\"patch\");\n+\t\t}\n \n \t\tif (defined $hash_parent &&\n \t\t    $hash_parent ne '-c' && $hash_parent ne '--cc') {\n@@ -5949,6 +5959,14 @@ 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+\tmy $patch_max = gitweb_check_feature('patches');\n+\tif ($patch_max) {\n+\t\tif ($patch_max < 0 || @commitlist <= $patch_max) {\n+\t\t\t$paging_nav .= \" &sdot; \" .\n+\t\t\t\t$cgi->a({-href => href(action=>\"patch\", -replay=>1)},\n+\t\t\t\t\t@commitlist > 1 ? \"patchset\" : \"patch\");\n+\t\t}\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":"97114","messageId":"7vy6yw95ln.fsf@gitster.siamese.dyndns.org","threadId":"16590","inReplyTo":"1228345188-15125-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [RFCv3 1/2] gitweb: add patch view","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-03T23:55:16Z","receivedAt":"2008-12-03T23:55:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n\n> The manually-built email format in commitdiff_plain output is not\n> appropriate 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>\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.\n>\n> A configurable limit is imposed on the number of commits which will be\n> included in a patchset, to prevent DoS attacks on the server. Setting\n> the limit to 0 will disable the patch view, setting it to a negative\n> number will remove the limit.\n>\n> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>\n> ---\n>  gitweb/gitweb.perl |   41 ++++++++++++++++++++++++++++++++++++++++-\n>  1 files changed, 40 insertions(+), 1 deletions(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 2738643..c9abfcf 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -329,6 +329,13 @@ our %feature = (\n>  \t'ctags' => {\n>  \t\t'override' => 0,\n>  \t\t'default' => [0]},\n> +\n> +\t# The maximum number of patches in a patchset generated in patch\n> +\t# view. Set this to 0 or undef to disable patch view, or to a\n> +\t# negative number to remove any limit.\n> +\t'patches' => {\n> +\t\t'override' => 1,\n> +\t\t'default' => [16]},\n>  );\n\nLooking at the existing entries in the %feature hash, it seems that it is\nour tradition that a new feature starts as disabled and not overridable\n(see 'ctags' in the context above).\n\n>  sub git_commitdiff {\n>  \tmy $format = shift || 'html';\n> +\n> +\tmy $patch_max = gitweb_check_feature('patches');\n> +\tif ($format eq 'patch') {\n> +\t\tdie_error(403, \"Patch view not allowed\") unless $patch_max;\n> +\t}\n> +\n\nShould you have to pay overhead for the check-feature call even when\nthe $format is not \"patch\"?\n\n> @@ -5396,6 +5410,7 @@ sub git_commitdiff {\n>  \t}\n>  \t# we need to prepare $formats_nav before almost any parameter munging\n>  \tmy $formats_nav;\n> +\n\nNoise.\n\n> @@ -5532,6 +5552,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\nA stupid question.  Are you talking about sending these X-Foo as extra\nHTTP headers?  What good would they do (iow what will they be used for by\nthe receiving browser/wget)?\n\nOther than that the patch seems quite straightforward and was a pleasant\nread.  Thanks.\n"},{"id":"97115","messageId":"cb7bb73a0812031620s2459f773q3db33971e3507b2f@mail.gmail.com","threadId":"16590","inReplyTo":"7vy6yw95ln.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFCv3 1/2] gitweb: add patch view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-04T00:20:08Z","receivedAt":"2008-12-04T00:20:08Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Thu, Dec 4, 2008 at 12:55 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n>> +\n>> +     # The maximum number of patches in a patchset generated in patch\n>> +     # view. Set this to 0 or undef to disable patch view, or to a\n>> +     # negative number to remove any limit.\n>> +     'patches' => {\n>> +             'override' => 1,\n>> +             'default' => [16]},\n>>  );\n>\n> Looking at the existing entries in the %feature hash, it seems that it is\n> our tradition that a new feature starts as disabled and not overridable\n> (see 'ctags' in the context above).\n\nI always assumed that the disabled default was related to how invasive\nthe changes would be (to the UI or computationally-wise). As for the\noverridability, that's actually the only reason why it would make\nsense to put in the %feature hash ... otherwise a conf-settable\n$patch_max (as in v2) would have been enough.\n\n>>  sub git_commitdiff {\n>>       my $format = shift || 'html';\n>> +\n>> +     my $patch_max = gitweb_check_feature('patches');\n>> +     if ($format eq 'patch') {\n>> +             die_error(403, \"Patch view not allowed\") unless $patch_max;\n>> +     }\n>> +\n>\n> Should you have to pay overhead for the check-feature call even when\n> the $format is not \"patch\"?\n\nActually I wasn't sure if I could use my within the if block, and have\nthe value visible outside (it's used further down when picking the\noptions to pass to format-patch). And since it was used in the second\npatch anyway to choose whether to add the 'patch' link in html view or\nnot, I just put it outside the block.\n\n>> @@ -5396,6 +5410,7 @@ sub git_commitdiff {\n>>       }\n>>       # we need to prepare $formats_nav before almost any parameter munging\n>>       my $formats_nav;\n>> +\n>\n> Noise.\n\nOopsie.\n\n>> @@ -5532,6 +5552,15 @@ sub git_commitdiff {\n>>                       print to_utf8($line) . \"\\n\";\n>>               }\n>>               print \"---\\n\\n\";\n>> +     } elsif ($format eq 'patch') {\n>> +             my $filename = basename($project) . \"-$hash.patch\";\n>> +\n>> +             print $cgi->header(\n>> +                     -type => 'text/plain',\n>> +                     -charset => 'utf-8',\n>> +                     -expires => $expires,\n>> +                     -content_disposition => 'inline; filename=\"' . \"$filename\" . '\"');\n>> +             # TODO add X-Git-Tag/X-Git-Url headers in a sensible way\n>\n> A stupid question.  Are you talking about sending these X-Foo as extra\n> HTTP headers?  What good would they do (iow what will they be used for by\n> the receiving browser/wget)?\n\nNo, as extra 'email' headers, similarly to what commitdiff_plain does.\nIt might or might not make sense, and might or might not be worth the\neffort. For sure the best way to enable these things would have to\ngive git format-patch some support for extra headers specified on the\ncommand line.\n\n> Other than that the patch seems quite straightforward and was a pleasant\n> read.  Thanks.\n\nThank you.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"97118","messageId":"7v4p1k93ht.fsf@gitster.siamese.dyndns.org","threadId":"16590","inReplyTo":"cb7bb73a0812031620s2459f773q3db33971e3507b2f@mail.gmail.com","subject":"Re: [RFCv3 1/2] gitweb: add patch view","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-04T00:40:46Z","receivedAt":"2008-12-04T00:40:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Giuseppe Bilotta\" <giuseppe.bilotta@gmail.com> writes:\n\n> No, as extra 'email' headers, similarly to what commitdiff_plain does.\n> It might or might not make sense, and might or might not be worth the\n> effort.\n\nI tend to agree; the point of this 'patch' feature is to give something\nyou can feed \"am\" with, and \"am\" certainly would discard such extra\ngarbage headers as uninteresting, so there is no point to add such\nheaders. \n"},{"id":"97124","messageId":"200812040249.01374.jnareb@gmail.com","threadId":"16590","inReplyTo":"cb7bb73a0812031620s2459f773q3db33971e3507b2f@mail.gmail.com","subject":"Re: [RFCv3 1/2] gitweb: add patch view","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-04T01:48:59Z","receivedAt":"2008-12-04T01:48:59Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, Dec 4, 2008 at 01:20, Giuseppe Bilotta wrote:\n> On Thu, Dec 4, 2008 at 12:55 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n>>> +\n>>> +     # The maximum number of patches in a patchset generated in patch\n>>> +     # view. Set this to 0 or undef to disable patch view, or to a\n>>> +     # negative number to remove any limit.\n>>> +     'patches' => {\n>>> +             'override' => 1,\n>>> +             'default' => [16]},\n\nErrr... you need something like 'sub' => \\&feature_patches for override\nto actually work, I think.\n\n>>>  );\n>>\n>> Looking at the existing entries in the %feature hash, it seems that it is\n>> our tradition that a new feature starts as disabled and not overridable\n>> (see 'ctags' in the context above).\n> \n> I always assumed that the disabled default was related to how invasive\n> the changes would be (to the UI or computationally-wise). As for the\n> overridability, that's actually the only reason why it would make\n> sense to put in the %feature hash ... otherwise a conf-settable\n> $patch_max (as in v2) would have been enough.\n\nAdd to that the fact that this patch just adds the new view, like for\nexample in the case of 'snapshot' link, which was turned on... but fact,\nit was by default not overridable. I would agree that it can be turned\non with low limit but not overridable in introductory patch.\n\n>>>  sub git_commitdiff {\n>>>       my $format = shift || 'html';\n>>> +\n>>> +     my $patch_max = gitweb_check_feature('patches');\n>>> +     if ($format eq 'patch') {\n>>> +             die_error(403, \"Patch view not allowed\") unless $patch_max;\n>>> +     }\n>>> +\n>>\n>> Should you have to pay overhead for the check-feature call even when\n>> the $format is not \"patch\"?\n> \n> Actually I wasn't sure if I could use my within the if block, and have\n> the value visible outside (it's used further down when picking the\n> options to pass to format-patch). And since it was used in the second\n> patch anyway to choose whether to add the 'patch' link in html view or\n> not, I just put it outside the block.\n\nYou have to use _declaration_ ourside block, but assignment can happen\ninside:\n\n+     my $patch_max;\n+     if ($format eq 'patch') {\n+             $patch_max = gitweb_check_feature('patches');\n+             die_error(403, \"Patch view not allowed\") unless $patch_max;\n+     }\n\n(Side note: doesn't it uses spaces instead of tabs for align?)\n \n[...]\n>> Other than that the patch seems quite straightforward and was a pleasant\n>> read.  Thanks.\n\nBTW. I'll try to review patch (and probably Ack) soon.\n-- \nJakub Narebski\nPoland\n"},{"id":"97134","messageId":"cb7bb73a0812032324m24991c5cydec41203e738fa89@mail.gmail.com","threadId":"16590","inReplyTo":"200812040249.01374.jnareb@gmail.com","subject":"Re: [RFCv3 1/2] gitweb: add patch view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-04T07:24:52Z","receivedAt":"2008-12-04T07:24:52Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Thu, Dec 4, 2008 at 2:48 AM, Jakub Narebski <jnareb@gmail.com> wrote:\n> On Thu, Dec 4, 2008 at 01:20, Giuseppe Bilotta wrote:\n>> On Thu, Dec 4, 2008 at 12:55 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n>>>> +\n>>>> +     # The maximum number of patches in a patchset generated in patch\n>>>> +     # view. Set this to 0 or undef to disable patch view, or to a\n>>>> +     # negative number to remove any limit.\n>>>> +     'patches' => {\n>>>> +             'override' => 1,\n>>>> +             'default' => [16]},\n>\n> Errr... you need something like 'sub' => \\&feature_patches for override\n> to actually work, I think.\n\nOops, right.\n\n>> I always assumed that the disabled default was related to how invasive\n>> the changes would be (to the UI or computationally-wise). As for the\n>> overridability, that's actually the only reason why it would make\n>> sense to put in the %feature hash ... otherwise a conf-settable\n>> $patch_max (as in v2) would have been enough.\n>\n> Add to that the fact that this patch just adds the new view, like for\n> example in the case of 'snapshot' link, which was turned on... but fact,\n> it was by default not overridable. I would agree that it can be turned\n> on with low limit but not overridable in introductory patch.\n\nOk, I'll make it non-overridable, and keep this 16 limit for starters.\nOr would you suggest even lower?\n\n>>>>  sub git_commitdiff {\n>>>>       my $format = shift || 'html';\n>>>> +\n>>>> +     my $patch_max = gitweb_check_feature('patches');\n>>>> +     if ($format eq 'patch') {\n>>>> +             die_error(403, \"Patch view not allowed\") unless $patch_max;\n>>>> +     }\n>>>> +\n>>>\n>>> Should you have to pay overhead for the check-feature call even when\n>>> the $format is not \"patch\"?\n>>\n>> Actually I wasn't sure if I could use my within the if block, and have\n>> the value visible outside (it's used further down when picking the\n>> options to pass to format-patch). And since it was used in the second\n>> patch anyway to choose whether to add the 'patch' link in html view or\n>> not, I just put it outside the block.\n>\n> You have to use _declaration_ ourside block, but assignment can happen\n> inside:\n>\n> +     my $patch_max;\n> +     if ($format eq 'patch') {\n> +             $patch_max = gitweb_check_feature('patches');\n> +             die_error(403, \"Patch view not allowed\") unless $patch_max;\n> +     }\n\nRight, stupid me.\n\n> (Side note: doesn't it uses spaces instead of tabs for align?)\n\nI'll check.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"97229","messageId":"200812060134.22959.jnareb@gmail.com","threadId":"16590","inReplyTo":"1228345188-15125-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [RFCv3 1/2] gitweb: add patch view","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-06T00:34:21Z","receivedAt":"2008-12-06T00:34:21Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"I'm sorry for the delay reviewing this patch series...\n\nOn Wed, 3 Dec 2008, Giuseppe Bilotta wrote:\n\n> The manually-built email format in commitdiff_plain output is not\n> appropriate 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\nThis is because 'commitdiff_plain' wasn't _meant_ as patch series view,\nto be fed to git-am. Actually it is a bit cross between \"git show\"\nresult with '--pretty=email' format, and \"git diff\" between two commits,\nto be fed to git-apply or GNU patch.\n\nNevertheless the above reasoning doesn't need to be put in a commit\nmessage. But it explains why new 'patch' / 'patchset' view is needed:\nbecause there was no equivalent.\n\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\nThis is independent issue which is I think worth correcting anyway,\nunless we decide to scrap 'commitdiff_plain' view altogether.\nBut I think we would want some text/plain patch view to be applied\nby GNU patch (for example in RPM .spec file).\n\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.\n\nWhich is in my opinion a very good idea.\n\n> \n> A configurable limit is imposed on the number of commits which will be\n> included in a patchset, to prevent DoS attacks on the server. Setting\n> the limit to 0 will disable the patch view, setting it to a negative\n> number will remove the limit.\n\nNote that this limit doesn't need to be lower than page length limit,\ni.e. currently 100 commits, as new 'patch' view doesn't generate\ngreater load than 'log' view (which is split into 100 commits long\npages).\n\n> \n> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>\n> ---\n>  gitweb/gitweb.perl |   41 ++++++++++++++++++++++++++++++++++++++++-\n>  1 files changed, 40 insertions(+), 1 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 2738643..c9abfcf 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -329,6 +329,13 @@ our %feature = (\n>  \t'ctags' => {\n>  \t\t'override' => 0,\n>  \t\t'default' => [0]},\n> +\n> +\t# The maximum number of patches in a patchset generated in patch\n> +\t# view. Set this to 0 or undef to disable patch view, or to a\n> +\t# negative number to remove any limit.\n> +\t'patches' => {\n> +\t\t'override' => 1,\n> +\t\t'default' => [16]},\n>  );\n\nYou need to set \"'sub' => \\&feature_patches\" for feature to be \noverride-able at all.  Also features are usually not overridable\nby default, which reduces load a tiny bit (by _possibly_ not reading\nconfig, although that shouldn't matter much now with reading whole\ncommit using single call to git-config, and not one call per variable).\nAnd I think the default might be set larger: 'log' view generates\nas big if not bigger load, and it is split into 100 commits long\npages.\n\n>  \n>  sub gitweb_get_feature {\n> @@ -503,6 +510,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> @@ -5386,6 +5394,12 @@ sub git_blobdiff_plain {\n>  \n>  sub git_commitdiff {\n>  \tmy $format = shift || 'html';\n> +\n> +\tmy $patch_max = gitweb_check_feature('patches');\n\nWouldn't it be better to name this variable $max_patchset_size, or\nsomething like that? I'm not saying that this name is bad, but I'm\nwondering if it could be better...\n\n> +\tif ($format eq 'patch') {\n> +\t\tdie_error(403, \"Patch view not allowed\") unless $patch_max;\n\nSo undef and '' means \"not allowed\", beside '0'? I think it is a good\nidea.\n\nAnd it would be better (although now it is not as much performance hit)\nto move \"gitweb_check_feature('patches');\" inside conditional:\n\n+\tmy $patch_max;\n+\tif ($format eq 'patch') {\n+\t\t$patch_max = gitweb_check_feature('patches');\n+\t\tdie_error(403, \"Patch view not allowed\") unless $patch_max;\n+\t}\n+\n\n> +\t}\n> +\n>  \t$hash ||= $hash_base || \"HEAD\";\n>  \tmy %co = parse_commit($hash)\n>  \t    or die_error(404, \"Unknown commit object\");\n> @@ -5396,6 +5410,7 @@ sub git_commitdiff {\n>  \t}\n>  \t# we need to prepare $formats_nav before almost any parameter munging\n>  \tmy $formats_nav;\n> +\n\nHmmm... some accidental change, it looks like.\n\n>  \tif ($format eq 'html') {\n>  \t\t$formats_nav =\n>  \t\t\t$cgi->a({-href => href(action=>\"commitdiff_plain\", -replay=>1)},\n> @@ -5483,7 +5498,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 > 0 ? \"-$patch_max\" : (),\n\n> +\t\t\t$hash_parent ? ('-n', \"$hash_parent..$hash\") :\n> +\t\t\t('--root', '-1', $hash)\n\nThis bit makes 'patch' view behave bit differently than git-format-patch,\nwhich I think is a good idea: namely it shows single patch if there is\nno cutoff.  This should be mentioned in commit message, and perhaps\neven in a comment in code.\n\nBeside, if you show only single patch because $hash_parent is not set,\nyou don't need to examine $patch_max nor set limit, because you use '-1'.\nCurrently if $patch_max > 0 and !$hash_parent, you pass limit for the\nnumber of patches twice.  This I think is harmless but...\n\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 +5552,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\nGood.\n\n> +\t\t# TODO add X-Git-Tag/X-Git-Url headers in a sensible way\n\nSensible way would mean modifying git-format-patch to be able to add\nextra headers via command option, just like it is now possible via\nconfig variable format.headers, I think.  Because there are no surefire\nmarkers of where one patch ends and another begins: commit message is\nfree text, and can contain diff... although if it contains diff\nseparator '---' then git-am would have problem with patch; or at least\neven assuming sane commit messages it is not easy.\n\nAlso I think that only X-Git-Url makes sense, and it is per whole\npatchset (whole 'patch' view output) and not for each individual\npatch.\n\n>  \t}\n>  \n>  \t# write patch\n> @@ -5553,6 +5582,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 +5594,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\nO.K.\n\n>  sub git_history {\n>  \tif (!defined $hash_base) {\n>  \t\t$hash_base = git_get_head_hash($project);\n> -- \n> 1.5.6.5\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"97230","messageId":"7vabbaxh8y.fsf@gitster.siamese.dyndns.org","threadId":"16590","inReplyTo":"200812060134.22959.jnareb@gmail.com","subject":"Re: [RFCv3 1/2] gitweb: add patch view","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-06T00:46:37Z","receivedAt":"2008-12-06T00:46:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n>> +\t# The maximum number of patches in a patchset generated in patch\n>> +\t# view. Set this to 0 or undef to disable patch view, or to a\n>> +\t# negative number to remove any limit.\n>> +\t'patches' => {\n>> +\t\t'override' => 1,\n>> +\t\t'default' => [16]},\n>>  );\n>\n> You need to set \"'sub' => \\&feature_patches\" for feature to be \n> override-able at all.  Also features are usually not overridable\n> by default, which reduces load a tiny bit (by _possibly_ not reading\n> config, although that shouldn't matter much now with reading whole\n> commit using single call to git-config, and not one call per variable).\n> And I think the default might be set larger: 'log' view generates\n> as big if not bigger load, and it is split into 100 commits long\n> pages.\n\nI do not think defaulting to 'no' for overridability nor defaulting a new\nfeature to 'disabled' have much to do with the load, but they are more\nabout the principle of least surprise.  Somebody who runs gitweb in the\nplaypen he was given on the server shouldn't be getting a phone call from\nhis users late at night complaining that the page his gitweb serves look\ndifferent and has one extra link per each line, only because the sysadmin\nof the server decided to update git to 1.6.1 without telling him.\n\nOnce a new version capable of serving a new feature is introduced, he can\nplan, announce and deploy by switching the feature on in his gitweb\nconfiguration file.\n\nSome things, like sitewide default css changes, cannot be made disabled\nby default.  But a new feature can easily be kept disabled by default not\nto cause needless surprises.\n"},{"id":"97231","messageId":"200812060153.52947.jnareb@gmail.com","threadId":"16590","inReplyTo":"1228345188-15125-3-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [RFCv3 2/2] gitweb: links to patch action in commitdiff and shortlog view","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-06T00:53:52Z","receivedAt":"2008-12-06T00:53:52Z","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\n> In shortlog view, a link to the patchset is only offered when the number\n> of commits shown is less than the allowed maximum number of patches.\n> \n> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>\n> ---\n>  gitweb/gitweb.perl |   18 ++++++++++++++++++\n>  1 files changed, 18 insertions(+), 0 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index c9abfcf..ec8fc7d 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -5083,6 +5083,11 @@ sub git_commit {\n>  \t\t\t} @$parents ) .\n>  \t\t\t')';\n>  \t}\n> +\tif (gitweb_check_feature('patches')) {\n> +\t\t$formats_nav .= \" | \" .\n> +\t\t\t$cgi->a({-href => href(action=>\"patch\", -replay=>1)},\n> +\t\t\t\t\"patch\");\n> +\t}\n>  \n>  \tif (!defined $parent) {\n>  \t\t$parent = \"--root\";\n> @@ -5415,6 +5420,11 @@ sub git_commitdiff {\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\tif ($patch_max) {\n> +\t\t\t$formats_nav .= \" | \" .\n> +\t\t\t\t$cgi->a({-href => href(action=>\"patch\", -replay=>1)},\n> +\t\t\t\t\t\"patch\");\n> +\t\t}\n>  \n>  \t\tif (defined $hash_parent &&\n>  \t\t    $hash_parent ne '-c' && $hash_parent ne '--cc') {\n\nIn the above two hunks 'patch' view functions as \"git show --pretty=email\"\ntext/plain equivalent, but this duplicates a bit 'commitdiff_plain'\nfunctionality.  Well, 'commitdiff_plain' has currently some errors,\nbut...\n\n> @@ -5949,6 +5959,14 @@ 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> +\tmy $patch_max = gitweb_check_feature('patches');\n> +\tif ($patch_max) {\n> +\t\tif ($patch_max < 0 || @commitlist <= $patch_max) {\n> +\t\t\t$paging_nav .= \" &sdot; \" .\n> +\t\t\t\t$cgi->a({-href => href(action=>\"patch\", -replay=>1)},\n> +\t\t\t\t\t@commitlist > 1 ? \"patchset\" : \"patch\");\n> +\t\t}\n> +\t}\n\nHere 'patch' view functions as \"git format-patch\", able to be downloaded\nand fed to git-am.  Perhaps the action should also be named 'patches'\nhere?; it could lead to the same function.\n\nBy the way, there is subtle bug in above link. If shortlog view is less\nthan $patch_max commits long, but it is because the history for a given\nbranch (or starting from given commit) is so short, and not because\nthere is cutoff $hash_parent set, the 'patchset' view wouldn't display\nplain text equivalent view, but only patch for top commit.\n\nI assume that the link is only for 'shortlog' view, and not also for\n'log' and 'history' views because 'shortlog' is the only log-like view\nwhich support $hash_parent?\n\n>  \n>  \tgit_header_html();\n>  \tgit_print_page_nav('shortlog','', $hash,$hash,$hash, $paging_nav);\n> -- \n> 1.5.6.5\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"97232","messageId":"200812060209.42906.jnareb@gmail.com","threadId":"16590","inReplyTo":"7vabbaxh8y.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFCv3 1/2] gitweb: add patch view","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-06T01:09:42Z","receivedAt":"2008-12-06T01:09:42Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Dnia sobota 6. grudnia 2008 01:46, Junio C Hamano napisał:\n> Jakub Narebski <jnareb@gmail.com> writes:\n> \n>>> +\t# The maximum number of patches in a patchset generated in patch\n>>> +\t# view. Set this to 0 or undef to disable patch view, or to a\n>>> +\t# negative number to remove any limit.\n>>> +\t'patches' => {\n>>> +\t\t'override' => 1,\n>>> +\t\t'default' => [16]},\n>>>  );\n> >\n> > [...]  Also features are usually not overridable\n> > by default, which reduces load a tiny bit (by _possibly_ not reading\n> > config, although that shouldn't matter much now with reading whole\n> > commit using single call to git-config, and not one call per variable).\n> > And I think the default might be set larger: 'log' view generates\n> > as big if not bigger load, and it is split into 100 commits long\n> > pages.\n> \n> I do not think defaulting to 'no' for overridability nor defaulting a new\n> feature to 'disabled' have much to do with the load, but they are more\n> about the principle of least surprise.  Somebody who runs gitweb in the\n> playpen he was given on the server shouldn't be getting a phone call from\n> his users late at night complaining that the page his gitweb serves look\n> different and has one extra link per each line, only because the sysadmin\n> of the server decided to update git to 1.6.1 without telling him.\n> \n> Once a new version capable of serving a new feature is introduced, he can\n> plan, announce and deploy by switching the feature on in his gitweb\n> configuration file.\n> \n> Some things, like sitewide default css changes, cannot be made disabled\n> by default.  But a new feature can easily be kept disabled by default not\n> to cause needless surprises.\n\nWell, 'search', 'grep' and 'pickaxe' features are enabled by default,\nbut I think it is cause by the fact that they predate %features.\n\nBut we have also 'snapshot' feature, which like 'patches' is not simply\non/off but is configurable feature, like 'patches' adds new action and\ndoes modify existing actions only by adding extra links... and which is\nenabled by default.\n\nSo there... ;-)\n-- \nJakub Narebski\nPoland\n"},{"id":"97233","messageId":"7vtz9iw0uv.fsf@gitster.siamese.dyndns.org","threadId":"16590","inReplyTo":"200812060209.42906.jnareb@gmail.com","subject":"Re: [RFCv3 1/2] gitweb: add patch view","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-06T01:26:00Z","receivedAt":"2008-12-06T01:26:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> But we have also 'snapshot' feature, which like 'patches' is not simply\n> on/off but is configurable feature, like 'patches' adds new action and\n> does modify existing actions only by adding extra links... and which is\n> enabled by default.\n\nIf my \"git log\" is telling the true story, \"snapshot\" was introduced in\ncb9c6e5 (gitweb: Support for snapshot, 2006-08-17) and it was disabled by\ndefault.\n\nThe \"feature\" mechanism was introduced after that, with ddb8d90 (gitweb:\nMake blame and snapshot a feature., 2006-08-20).  To avoid surprises,\n\"blame\" were marked disabled by default to match the then-current default.\n\"snapshot\" somehow ended up enabled with that change, though, which might\nhave been a mistake.\n\nBut \"we erred before\" is not a good excuse to deliberately err again, is\nit?\n"},{"id":"97253","messageId":"cb7bb73a0812060434r2b48fd1ev359020fb1e0d1603@mail.gmail.com","threadId":"16590","inReplyTo":"200812060134.22959.jnareb@gmail.com","subject":"Re: [RFCv3 1/2] gitweb: add patch view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-06T12:34:57Z","receivedAt":"2008-12-06T12:34:57Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Sat, Dec 6, 2008 at 1:34 AM, Jakub Narebski <jnareb@gmail.com> wrote:\n> On Wed, 3 Dec 2008, Giuseppe Bilotta wrote:\n>\n>> The manually-built email format in commitdiff_plain output is not\n>> appropriate 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>\n> This is because 'commitdiff_plain' wasn't _meant_ as patch series view,\n> to be fed to git-am. Actually it is a bit cross between \"git show\"\n> result with '--pretty=email' format, and \"git diff\" between two commits,\n> to be fed to git-apply or GNU patch.\n>\n> Nevertheless the above reasoning doesn't need to be put in a commit\n> message. But it explains why new 'patch' / 'patchset' view is needed:\n> because there was no equivalent.\n\nI'll remove it.\n\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>\n> This is independent issue which is I think worth correcting anyway,\n> unless we decide to scrap 'commitdiff_plain' view altogether.\n> But I think we would want some text/plain patch view to be applied\n> by GNU patch (for example in RPM .spec file).\n\nI don't think we should scrap commitdiff either, but the subject\nreplication is not really an issue if the view is not fed to git am.\n\n>> +     # The maximum number of patches in a patchset generated in patch\n>> +     # view. Set this to 0 or undef to disable patch view, or to a\n>> +     # negative number to remove any limit.\n>> +     'patches' => {\n>> +             'override' => 1,\n>> +             'default' => [16]},\n>>  );\n>\n> You need to set \"'sub' => \\&feature_patches\" for feature to be\n> override-able at all.  Also features are usually not overridable\n> by default, which reduces load a tiny bit (by _possibly_ not reading\n> config, although that shouldn't matter much now with reading whole\n> commit using single call to git-config, and not one call per variable).\n\nI think I'll make the feature non-overridable. I'll also make it\ndefault to disabled, although I'm not particularly happy with the\nchoice.\n\n> And I think the default might be set larger: 'log' view generates\n> as big if not bigger load, and it is split into 100 commits long\n> pages.\n\nHm, I would say the load of patch view is much higher than the load of\nlog view, both in terms of bandwidth and in terms of load on the\nserver, because of the diffs.\n\n>>  sub git_commitdiff {\n>>       my $format = shift || 'html';\n>> +\n>> +     my $patch_max = gitweb_check_feature('patches');\n>\n> Wouldn't it be better to name this variable $max_patchset_size, or\n> something like that? I'm not saying that this name is bad, but I'm\n> wondering if it could be better...\n\nmax_patchset_size sounds much worse than patch_max to me, which is why\nI went for the latter 8-)\n\n>>       if ($format eq 'html') {\n>>               $formats_nav =\n>>                       $cgi->a({-href => href(action=>\"commitdiff_plain\", -replay=>1)},\n>> @@ -5483,7 +5498,12 @@ sub git_commitdiff {\n>>               open $fd, \"-|\", git_cmd(), \"diff-tree\", '-r', @diff_opts,\n>>                       '-p', $hash_parent_param, $hash, \"--\"\n>>                       or die_error(500, \"Open git-diff-tree failed\");\n>> -\n>> +     } elsif ($format eq 'patch') {\n>> +             open $fd, \"-|\", git_cmd(), \"format-patch\", '--encoding=utf8',\n>> +                     '--stdout', $patch_max > 0 ? \"-$patch_max\" : (),\n>\n>> +                     $hash_parent ? ('-n', \"$hash_parent..$hash\") :\n>> +                     ('--root', '-1', $hash)\n>\n> This bit makes 'patch' view behave bit differently than git-format-patch,\n> which I think is a good idea: namely it shows single patch if there is\n> no cutoff.  This should be mentioned in commit message, and perhaps\n> even in a comment in code.\n>\n> Beside, if you show only single patch because $hash_parent is not set,\n> you don't need to examine $patch_max nor set limit, because you use '-1'.\n> Currently if $patch_max > 0 and !$hash_parent, you pass limit for the\n> number of patches twice.  This I think is harmless but...\n\nI've reworked the code a bit, making the commit spec an array computed\nbefore passing it to the command line. The code is cleaner but\nobviously longer 8-)\n\nThe double limit worked properly, btw.\n\n>> +             # TODO add X-Git-Tag/X-Git-Url headers in a sensible way\n>\n> Sensible way would mean modifying git-format-patch to be able to add\n> extra headers via command option, just like it is now possible via\n> config variable format.headers, I think.  Because there are no surefire\n> markers of where one patch ends and another begins: commit message is\n> free text, and can contain diff... although if it contains diff\n> separator '---' then git-am would have problem with patch; or at least\n> even assuming sane commit messages it is not easy.\n>\n> Also I think that only X-Git-Url makes sense, and it is per whole\n> patchset (whole 'patch' view output) and not for each individual\n> patch.\n\nI've stripped this commet for the time being. I'm not sure even\nX-Git-Url makes sense, and the fact that it should only attached to\nthe first email makes it an oddball.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"97256","messageId":"cb7bb73a0812060501s6a3faea7l74e81c2a591ba2a7@mail.gmail.com","threadId":"16590","inReplyTo":"7vabbaxh8y.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFCv3 1/2] gitweb: add patch view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-06T13:01:09Z","receivedAt":"2008-12-06T13:01:09Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Sat, Dec 6, 2008 at 1:46 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n>\n>>> +    # The maximum number of patches in a patchset generated in patch\n>>> +    # view. Set this to 0 or undef to disable patch view, or to a\n>>> +    # negative number to remove any limit.\n>>> +    'patches' => {\n>>> +            'override' => 1,\n>>> +            'default' => [16]},\n>>>  );\n>>\n>> You need to set \"'sub' => \\&feature_patches\" for feature to be\n>> override-able at all.  Also features are usually not overridable\n>> by default, which reduces load a tiny bit (by _possibly_ not reading\n>> config, although that shouldn't matter much now with reading whole\n>> commit using single call to git-config, and not one call per variable).\n>> And I think the default might be set larger: 'log' view generates\n>> as big if not bigger load, and it is split into 100 commits long\n>> pages.\n>\n> I do not think defaulting to 'no' for overridability nor defaulting a new\n> feature to 'disabled' have much to do with the load, but they are more\n> about the principle of least surprise.  Somebody who runs gitweb in the\n> playpen he was given on the server shouldn't be getting a phone call from\n> his users late at night complaining that the page his gitweb serves look\n> different and has one extra link per each line, only because the sysadmin\n> of the server decided to update git to 1.6.1 without telling him.\n>\n> Once a new version capable of serving a new feature is introduced, he can\n> plan, announce and deploy by switching the feature on in his gitweb\n> configuration file.\n>\n> Some things, like sitewide default css changes, cannot be made disabled\n> by default.  But a new feature can easily be kept disabled by default not\n> to cause needless surprises.\n\nIn the eternal war between making feature easily available and not\ndisturbing the user too much between upgrades, I'm more on the 'making\navailable' field, especially when the features are not particularly\ninvasive (e.g., the patch view only adds one single link, on the\nnavigation bar, and only in some views).\n\nIt should also be noted that if the sysadmin deploys a new software\nversion without telling its users, there's an obvious communication\nproblem between the sysadmin and its users, but that's not something\nthe tool developers should try to work around. Otherwise we'd still be\noffering the dash version of the commands by default.\n\n(Plus, weren't you the one suggesting that the remote heads feature\nshould be enabled by default? And that's an even more invasive change,\nif you ask me.)\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"97257","messageId":"200812061409.02730.jnareb@gmail.com","threadId":"16590","inReplyTo":"cb7bb73a0812060434r2b48fd1ev359020fb1e0d1603@mail.gmail.com","subject":"Re: [RFCv3 1/2] gitweb: add patch view","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-06T13:09:01Z","receivedAt":"2008-12-06T13:09:01Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, Dec 6, 2008 at 13:34, Giuseppe Bilotta wrote:\n> On Sat, Dec 6, 2008 at 1:34 AM, Jakub Narebski <jnareb@gmail.com> wrote:\n>> On Wed, 3 Dec 2008, Giuseppe Bilotta wrote:\n>>\n>>> The manually-built email format in commitdiff_plain output is not\n>>> appropriate 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>>\n>> This is because 'commitdiff_plain' wasn't _meant_ as patch series view,\n>> to be fed to git-am. Actually it is a bit cross between \"git show\"\n>> result with '--pretty=email' format, and \"git diff\" between two commits,\n>> to be fed to git-apply or GNU patch.\n>>\n>> Nevertheless the above reasoning doesn't need to be put in a commit\n>> message. But it explains why new 'patch' / 'patchset' view is needed:\n>> because there was no equivalent.\n> \n> I'll remove it.\n\nErrr... sorry, I haven't made myself clear. I meant here that *my*\ncomment should not be added; your explanation about adding 'patch'\nview should IMHO stay, perhaps reworked a bit: commitdiff is not\nabout generating patch series.\n\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>>\n>> This is independent issue which is I think worth correcting anyway,\n>> unless we decide to scrap 'commitdiff_plain' view altogether.\n>> But I think we would want some text/plain patch view to be applied\n>> by GNU patch (for example in RPM .spec file).\n> \n> I don't think we should scrap commitdiff either, but the subject\n> replication is not really an issue if the view is not fed to git am.\n\nWell, there is it.\n \n>>> +     # The maximum number of patches in a patchset generated in patch\n>>> +     # view. Set this to 0 or undef to disable patch view, or to a\n>>> +     # negative number to remove any limit.\n>>> +     'patches' => {\n>>> +             'override' => 1,\n>>> +             'default' => [16]},\n>>>  );\n>>\n>> You need to set \"'sub' => \\&feature_patches\" for feature to be\n>> override-able at all.  Also features are usually not overridable\n>> by default, which reduces load a tiny bit (by _possibly_ not reading\n>> config, although that shouldn't matter much now with reading whole\n>> commit using single call to git-config, and not one call per variable).\n> \n> I think I'll make the feature non-overridable. I'll also make it\n> default to disabled, although I'm not particularly happy with the\n> choice.\n\nI think that having 'patches' feature to be able to override is a good\nidea, as limit on number of patches might depend on _repository_, for\nexample being lower for repository with large commits (large in sense\nof diff size).\n\nDefault with override unset seems to be precedence... as to having it\ndisabled by default: having feature which require configuration enabled\nby default serves as an example of configuration; on the other hand the\nexample might be in comments, like for 'actions' %feature.\n \n>> And I think the default might be set larger: 'log' view generates\n>> as big if not bigger load, and it is split into 100 commits long\n>> pages.\n> \n> Hm, I would say the load of patch view is much higher than the load of\n> log view, both in terms of bandwidth and in terms of load on the\n> server, because of the diffs.\n\nAh, I forgot about that. Limit to 16-25 seems to be reasonable, then.\n \n>>> @@ -5483,7 +5498,12 @@ sub git_commitdiff {\n>>>               open $fd, \"-|\", git_cmd(), \"diff-tree\", '-r', @diff_opts,\n>>>                       '-p', $hash_parent_param, $hash, \"--\"\n>>>                       or die_error(500, \"Open git-diff-tree failed\");\n>>> -\n>>> +     } elsif ($format eq 'patch') {\n>>> +             open $fd, \"-|\", git_cmd(), \"format-patch\", '--encoding=utf8',\n>>> +                     '--stdout', $patch_max > 0 ? \"-$patch_max\" : (),\n>>> +                     $hash_parent ? ('-n', \"$hash_parent..$hash\") :\n>>> +                     ('--root', '-1', $hash)\n>>\n>> This bit makes 'patch' view behave bit differently than git-format-patch,\n>> which I think is a good idea: namely it shows single patch if there is\n>> no cutoff.  This should be mentioned in commit message, and perhaps\n>> even in a comment in code.\n>>\n>> Beside, if you show only single patch because $hash_parent is not set,\n>> you don't need to examine $patch_max nor set limit, because you use '-1'.\n>> Currently if $patch_max > 0 and !$hash_parent, you pass limit for the\n>> number of patches twice.  This I think is harmless but...\n> \n> I've reworked the code a bit, making the commit spec an array computed\n> before passing it to the command line. The code is cleaner but\n> obviously longer 8-)\n\nCleaner is good.\n \n> The double limit worked properly, btw.\n\nNice to know. But should we rely on corner cases handling?\n\n-- \nJakub Narebski\nPoland\n"},{"id":"97258","messageId":"20081206131058.GB10491@machine.or.cz","threadId":"16590","inReplyTo":"cb7bb73a0812060501s6a3faea7l74e81c2a591ba2a7@mail.gmail.com","subject":"Re: [RFCv3 1/2] gitweb: add patch view","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-12-06T13:10:58Z","receivedAt":"2008-12-06T13:10:58Z","isPatch":false,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"On Sat, Dec 06, 2008 at 02:01:09PM +0100, Giuseppe Bilotta wrote:\n> On Sat, Dec 6, 2008 at 1:46 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> > Some things, like sitewide default css changes, cannot be made disabled\n> > by default.  But a new feature can easily be kept disabled by default not\n> > to cause needless surprises.\n> \n> In the eternal war between making feature easily available and not\n> disturbing the user too much between upgrades, I'm more on the 'making\n> available' field, especially when the features are not particularly\n> invasive (e.g., the patch view only adds one single link, on the\n> navigation bar, and only in some views).\n> \n> It should also be noted that if the sysadmin deploys a new software\n> version without telling its users, there's an obvious communication\n> problem between the sysadmin and its users, but that's not something\n> the tool developers should try to work around. Otherwise we'd still be\n> offering the dash version of the commands by default.\n\nI feel exactly the same.\n\nAcked-by: Petr Baudis <pasky@suse.cz>\n\nSame goes for the patches, I haven't had time to examine them in detail\nbut the general shape looks fine.\n"},{"id":"97259","messageId":"cb7bb73a0812060525k65a7f549l2cce5f0dae9fc76c@mail.gmail.com","threadId":"16590","inReplyTo":"200812060153.52947.jnareb@gmail.com","subject":"Re: [RFCv3 2/2] gitweb: links to patch action in commitdiff and shortlog view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-06T13:25:20Z","receivedAt":"2008-12-06T13:25:20Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Sat, Dec 6, 2008 at 1:53 AM, Jakub Narebski <jnareb@gmail.com> wrote:\n> On Wed, 3 Dec 2008, Giuseppe Bilotta wrote:\n>\n>> In shortlog view, a link to the patchset is only offered when the number\n>> of commits shown is less than the allowed maximum number of patches.\n>>\n>> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>\n\n>> +     if (gitweb_check_feature('patches')) {\n>> +             $formats_nav .= \" | \" .\n>> +                     $cgi->a({-href => href(action=>\"patch\", -replay=>1)},\n>> +                             \"patch\");\n>> +     }\n>>\n>>       if (!defined $parent) {\n>>               $parent = \"--root\";\n>> @@ -5415,6 +5420,11 @@ sub git_commitdiff {\n>>               $formats_nav =\n>>                       $cgi->a({-href => href(action=>\"commitdiff_plain\", -replay=>1)},\n>>                               \"raw\");\n>> +             if ($patch_max) {\n>> +                     $formats_nav .= \" | \" .\n>> +                             $cgi->a({-href => href(action=>\"patch\", -replay=>1)},\n>> +                                     \"patch\");\n>> +             }\n>>\n>>               if (defined $hash_parent &&\n>>                   $hash_parent ne '-c' && $hash_parent ne '--cc') {\n>\n> In the above two hunks 'patch' view functions as \"git show --pretty=email\"\n> text/plain equivalent, but this duplicates a bit 'commitdiff_plain'\n> functionality.  Well, 'commitdiff_plain' has currently some errors,\n> but...\n\nAll things considered, for single commit view there is (modulo bugs)\nno factual difference between plain diff and patch view.\n\nAlthough we could merge them, I'm thinking that the plain diff view\nhas somewhat too much information in it. For backwards compatibility\nit's probably not wise to change it, but we should consider it for the\nnext major version, honestly.\n\n>> @@ -5949,6 +5959,14 @@ sub git_shortlog {\n>>                       $cgi->a({-href => href(-replay=>1, page=>$page+1),\n>>                                -accesskey => \"n\", -title => \"Alt-n\"}, \"next\");\n>>       }\n>> +     my $patch_max = gitweb_check_feature('patches');\n>> +     if ($patch_max) {\n>> +             if ($patch_max < 0 || @commitlist <= $patch_max) {\n>> +                     $paging_nav .= \" &sdot; \" .\n>> +                             $cgi->a({-href => href(action=>\"patch\", -replay=>1)},\n>> +                                     @commitlist > 1 ? \"patchset\" : \"patch\");\n>> +             }\n>> +     }\n>\n> Here 'patch' view functions as \"git format-patch\", able to be downloaded\n> and fed to git-am.  Perhaps the action should also be named 'patches'\n> here?; it could lead to the same function.\n\nI had half an idea to do so. 'patches' or 'patchset'?\n\n> By the way, there is subtle bug in above link. If shortlog view is less\n> than $patch_max commits long, but it is because the history for a given\n> branch (or starting from given commit) is so short, and not because\n> there is cutoff $hash_parent set, the 'patchset' view wouldn't display\n> plain text equivalent view, but only patch for top commit.\n\nAh, good point.\n\nHm, not easy to solve. One way could be to add the hash_parent\nmanually. Or we could make the 'patches' view different from the\n'patch' view in the way it handles refspecs without ranges. I'm\nleaning towards the latter. What's your opinion?\n\n> I assume that the link is only for 'shortlog' view, and not also for\n> 'log' and 'history' views because 'shortlog' is the only log-like view\n> which support $hash_parent?\n\nThe actual reason is that I never use log nor history view, but since\nthey don't support hash_parent because of this (I was the one who sent\nthe patch to support hash_parent in shortlog view) you could\nparalogistically say that's the reason ;-)\n\nI'm not sure about history view, but for log view I'm considering\naddiong also a 'patch' link next to each commit. I'll think about it.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"97261","messageId":"cb7bb73a0812060546l23b39154q27c636220f6aae81@mail.gmail.com","threadId":"16590","inReplyTo":"200812061409.02730.jnareb@gmail.com","subject":"Re: [RFCv3 1/2] gitweb: add patch view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-06T13:46:30Z","receivedAt":"2008-12-06T13:46:30Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Sat, Dec 6, 2008 at 2:09 PM, Jakub Narebski <jnareb@gmail.com> wrote:\n> On Sat, Dec 6, 2008 at 13:34, Giuseppe Bilotta wrote:\n>> On Sat, Dec 6, 2008 at 1:34 AM, Jakub Narebski <jnareb@gmail.com> wrote:\n>>> On Wed, 3 Dec 2008, Giuseppe Bilotta wrote:\n>>>\n>>>> The manually-built email format in commitdiff_plain output is not\n>>>> appropriate 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>>>\n>>> This is because 'commitdiff_plain' wasn't _meant_ as patch series view,\n>>> to be fed to git-am. Actually it is a bit cross between \"git show\"\n>>> result with '--pretty=email' format, and \"git diff\" between two commits,\n>>> to be fed to git-apply or GNU patch.\n>>>\n>>> Nevertheless the above reasoning doesn't need to be put in a commit\n>>> message. But it explains why new 'patch' / 'patchset' view is needed:\n>>> because there was no equivalent.\n>>\n>> I'll remove it.\n>\n> Errr... sorry, I haven't made myself clear. I meant here that *my*\n> comment should not be added; your explanation about adding 'patch'\n> view should IMHO stay, perhaps reworked a bit: commitdiff is not\n> about generating patch series.\n\nOops, ok, I'll just rewrite it better 8-)\n\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>>>\n>>> This is independent issue which is I think worth correcting anyway,\n>>> unless we decide to scrap 'commitdiff_plain' view altogether.\n>>> But I think we would want some text/plain patch view to be applied\n>>> by GNU patch (for example in RPM .spec file).\n>>\n>> I don't think we should scrap commitdiff either, but the subject\n>> replication is not really an issue if the view is not fed to git am.\n>\n> Well, there is it.\n\nConsidering the comments on the second patch too, I've been thinking\nthat it might worth it to merge commitdiff_plain and patch view for\nsingle commits. Some changes for multi-commits commitdiff_plain can be\nconsidered too, but as I mentioned this is a major changes and should\nbe dealt with on its own.\n\n>>>> +     # The maximum number of patches in a patchset generated in patch\n>>>> +     # view. Set this to 0 or undef to disable patch view, or to a\n>>>> +     # negative number to remove any limit.\n>>>> +     'patches' => {\n>>>> +             'override' => 1,\n>>>> +             'default' => [16]},\n>>>>  );\n>>>\n>>> You need to set \"'sub' => \\&feature_patches\" for feature to be\n>>> override-able at all.  Also features are usually not overridable\n>>> by default, which reduces load a tiny bit (by _possibly_ not reading\n>>> config, although that shouldn't matter much now with reading whole\n>>> commit using single call to git-config, and not one call per variable).\n>>\n>> I think I'll make the feature non-overridable. I'll also make it\n>> default to disabled, although I'm not particularly happy with the\n>> choice.\n>\n> I think that having 'patches' feature to be able to override is a good\n> idea, as limit on number of patches might depend on _repository_, for\n> example being lower for repository with large commits (large in sense\n> of diff size).\n>\n> Default with override unset seems to be precedence... as to having it\n> disabled by default: having feature which require configuration enabled\n> by default serves as an example of configuration; on the other hand the\n> example might be in comments, like for 'actions' %feature.\n\nI've added the feature_patches sub. Also, considering that there are\nnow basically 3 votes against 1 for enabling the feature by default,\nI'll keep it enabled.\n\n>>> And I think the default might be set larger: 'log' view generates\n>>> as big if not bigger load, and it is split into 100 commits long\n>>> pages.\n>>\n>> Hm, I would say the load of patch view is much higher than the load of\n>> log view, both in terms of bandwidth and in terms of load on the\n>> server, because of the diffs.\n>\n> Ah, I forgot about that. Limit to 16-25 seems to be reasonable, then.\n\nI'll go with 16 for the time being, I think it's large enough to\naccomodate most patchsets.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"97267","messageId":"200812061625.53527.jnareb@gmail.com","threadId":"16590","inReplyTo":"cb7bb73a0812060525k65a7f549l2cce5f0dae9fc76c@mail.gmail.com","subject":"Re: [RFCv3 2/2] gitweb: links to patch action in commitdiff and shortlog view","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-06T15:25:53Z","receivedAt":"2008-12-06T15:25:53Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Dnia sobota 6. grudnia 2008 14:25, Giuseppe Bilotta napisał:\n> On Sat, Dec 6, 2008 at 1:53 AM, Jakub Narebski <jnareb@gmail.com> wrote:\n>> On Wed, 3 Dec 2008, Giuseppe Bilotta wrote:\n>>\n>>> In shortlog view, a link to the patchset is only offered when the number\n>>> of commits shown is less than the allowed maximum number of patches.\n>>>\n>>> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>\n \n>>> +     if (gitweb_check_feature('patches')) {\n>>> +             $formats_nav .= \" | \" .\n>>> +                     $cgi->a({-href => href(action=>\"patch\", -replay=>1)},\n>>> +                             \"patch\");\n>>> +     }\n>>>\n>>>       if (!defined $parent) {\n>>>               $parent = \"--root\";\n>>> @@ -5415,6 +5420,11 @@ sub git_commitdiff {\n>>>               $formats_nav =\n>>>                       $cgi->a({-href => href(action=>\"commitdiff_plain\", -replay=>1)},\n>>>                               \"raw\");\n>>> +             if ($patch_max) {\n>>> +                     $formats_nav .= \" | \" .\n>>> +                             $cgi->a({-href => href(action=>\"patch\", -replay=>1)},\n>>> +                                     \"patch\");\n>>> +             }\n>>>\n>>>               if (defined $hash_parent &&\n>>>                   $hash_parent ne '-c' && $hash_parent ne '--cc') {\n>>\n>> In the above two hunks 'patch' view functions as \"git show --pretty=email\"\n>> text/plain equivalent, but this duplicates a bit 'commitdiff_plain'\n>> functionality.  Well, 'commitdiff_plain' has currently some errors,\n>> but...\n> \n> All things considered, for single commit view there is (modulo bugs)\n> no factual difference between plain diff and patch view.\n> \n> Although we could merge them, I'm thinking that the plain diff view\n> has somewhat too much information in it. For backwards compatibility\n> it's probably not wise to change it, but we should consider it for the\n> next major version, honestly.\n\nI'm just wondering if we should add 'patch' link to 'commit' and\n'commitdiff' views (as alternate view) at all...\n \n>>> @@ -5949,6 +5959,14 @@ sub git_shortlog {\n>>>                       $cgi->a({-href => href(-replay=>1, page=>$page+1),\n>>>                                -accesskey => \"n\", -title => \"Alt-n\"}, \"next\");\n>>>       }\n>>> +     my $patch_max = gitweb_check_feature('patches');\n>>> +     if ($patch_max) {\n>>> +             if ($patch_max < 0 || @commitlist <= $patch_max) {\n>>> +                     $paging_nav .= \" &sdot; \" .\n>>> +                             $cgi->a({-href => href(action=>\"patch\", -replay=>1)},\n>>> +                                     @commitlist> 1 ? \"patchset\" : \"patch\");\n>>> +             }\n>>> +     }\n>>\n>> Here 'patch' view functions as \"git format-patch\", able to be downloaded\n>> and fed to git-am.  Perhaps the action should also be named 'patches'\n>> here?; it could lead to the same function.\n> \n> I had half an idea to do so. 'patches' or 'patchset'?\n\nHmmm... I think 'patches'.\n\n>> By the way, there is subtle bug in above link. If shortlog view is less\n>> than $patch_max commits long, but it is because the history for a given\n>> branch (or starting from given commit) is so short, and not because\n>> there is cutoff $hash_parent set, the 'patchset' view wouldn't display\n>> plain text equivalent view, but only patch for top commit.\n> \n> Ah, good point.\n> \n> Hm, not easy to solve. One way could be to add the hash_parent\n> manually. Or we could make the 'patches' view different from the\n> 'patch' view in the way it handles refspecs without ranges. I'm\n> leaning towards the latter. What's your opinion?\n\nI think simplest solution would be to add $hash_parent if it is not\nset from the last commit, i.e. $commitlist[-1]{'id'}\n\n>> I assume that the link is only for 'shortlog' view, and not also for\n>> 'log' and 'history' views because 'shortlog' is the only log-like view\n>> which support $hash_parent?\n> \n> The actual reason is that I never use log nor history view, but since\n> they don't support hash_parent because of this (I was the one who sent\n> the patch to support hash_parent in shortlog view) you could\n> paralogistically say that's the reason ;-)\n> \n> I'm not sure about history view, but for log view I'm considering\n> addiong also a 'patch' link next to each commit. I'll think about it.\n\nWell, you can add it only for 'shortlog' view, and when the code for\nall log-like views would get consolidated, you will get link to 'patches'\nview automatically :-)\n\n-- \nJakub Narebski\nPoland\n"}]}