{"thread":{"id":"16750","subject":"[PATCHv5 1/3] gitweb: add patch view","startedAt":"2008-12-16T10:11:27Z","lastAt":"2008-12-18T19:57:43Z","messageCount":12,"participants":["Giuseppe Bilotta","Junio C Hamano","Jakub Narebski"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"98058","messageId":"1229422290-6213-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"16750","inReplyTo":null,"subject":"[PATCHv5 0/3] gitweb: patch view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-16T10:11:27Z","receivedAt":"2008-12-16T10:11:27Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Fifth iteration of the patch view in gitweb, that exposes the\ngit-format-patch output directly, allowing patchset exchange via gitweb.\n\nNo new functionality, just cleanups suggested by Jakub.\n\nGiuseppe Bilotta (3):\n  gitweb: add patch view\n  gitweb: add patches view\n  gitweb: link to patch(es) view in commit(diff) and (short)log view\n\n gitweb/gitweb.perl |  109 +++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 files changed, 108 insertions(+), 1 deletions(-)\n"},{"id":"98057","messageId":"1229422290-6213-2-git-send-email-giuseppe.bilotta@gmail.com","threadId":"16750","inReplyTo":"1229422290-6213-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv5 1/3] gitweb: add patch view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-16T10:11:28Z","receivedAt":"2008-12-16T10:11:28Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"The output of commitdiff_plain is not intended for git-am:\n * when given a range of commits, commitdiff_plain publishes a single\n   patch with the message from the first commit, instead of a patchset\n * the hand-built email format replicates the commit summary both as\n   email subject and as first line of the email itself, resulting in\n   a duplication if the output is used with git-am.\n\nWe thus create a new view that can be fed to git-am directly, allowing\npatch exchange via gitweb. The new view exposes the output of git\nformat-patch directly, limiting it to a single patch in the case of a\nsingle commit.\n\nA configurable upper limit defaulting to 16 is imposed on the number of\ncommits which will be included in a patchset, to prevent DoS attacks on\nthe server. Setting the limit to 0 will disable the patch view, setting\nit to a negative number will remove the limit.\n\nSigned-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>\n---\n gitweb/gitweb.perl |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 files changed, 68 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 6eb370d..1006dfe 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -330,6 +330,21 @@ 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+\n+\t# To disable system wide have in $GITWEB_CONFIG\n+\t# $feature{'patches'}{'default'} = [0];\n+\t# To have project specific config enable override in $GITWEB_CONFIG\n+\t# $feature{'patches'}{'override'} = 1;\n+\t# and in project config gitweb.patches = 0|n;\n+\t# where n is the maximum number of patches allowed in a patchset.\n+\t'patches' => {\n+\t\t'sub' => \\&feature_patches,\n+\t\t'override' => 0,\n+\t\t'default' => [16]},\n );\n \n sub gitweb_get_feature {\n@@ -411,6 +426,16 @@ sub feature_pickaxe {\n \treturn ($_[0]);\n }\n \n+sub feature_patches {\n+\tmy @val = (git_get_project_config('patches', '--int'));\n+\n+\tif (@val) {\n+\t\treturn @val;\n+\t}\n+\n+\treturn ($_[0]);\n+}\n+\n # checking HEAD file with -e is fragile if the repository was\n # initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed\n # and then pruned.\n@@ -504,6 +529,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@@ -5387,6 +5413,13 @@ sub git_blobdiff_plain {\n \n sub git_commitdiff {\n \tmy $format = shift || 'html';\n+\n+\tmy $patch_max;\n+\tif ($format eq 'patch') {\n+\t\t($patch_max) = gitweb_get_feature('patches');\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@@ -5484,7 +5517,23 @@ 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\t# For commit ranges, we limit the output to the number of\n+\t\t# patches specified in the 'patches' feature.\n+\t\t# For single commits, we limit the output to a single patch,\n+\t\t# diverging from the git-format-patch default.\n+\t\tmy @commit_spec = ();\n+\t\tif ($hash_parent) {\n+\t\t\tif ($patch_max > 0) {\n+\t\t\t\tpush @commit_spec, \"-$patch_max\";\n+\t\t\t}\n+\t\t\tpush @commit_spec, '-n', \"$hash_parent..$hash\";\n+\t\t} else {\n+\t\t\tpush @commit_spec, '-1', '--root', $hash;\n+\t\t}\n+\t\topen $fd, \"-|\", git_cmd(), \"format-patch\", '--encoding=utf8',\n+\t\t\t'--stdout', @commit_spec\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@@ -5533,6 +5582,14 @@ 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}\n \n \t# write patch\n@@ -5554,6 +5611,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@@ -5561,6 +5623,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":"98059","messageId":"1229422290-6213-3-git-send-email-giuseppe.bilotta@gmail.com","threadId":"16750","inReplyTo":"1229422290-6213-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv5 2/3] gitweb: add patches view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-16T10:11:29Z","receivedAt":"2008-12-16T10:11:29Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"The only difference between patch and patches view is in the treatement\nof single commits: the former only displays a single patch, whereas the\nlatter displays a patchset leading to the specified commit.\n\nSigned-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>\n---\n gitweb/gitweb.perl |   16 +++++++++++++++-\n 1 files changed, 15 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 1006dfe..e7a6477 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -530,6 +530,7 @@ our %actions = (\n \t\"history\" => \\&git_history,\n \t\"log\" => \\&git_log,\n \t\"patch\" => \\&git_patch,\n+\t\"patches\" => \\&git_patches,\n \t\"rss\" => \\&git_rss,\n \t\"atom\" => \\&git_atom,\n \t\"search\" => \\&git_search,\n@@ -5413,6 +5414,7 @@ sub git_blobdiff_plain {\n \n sub git_commitdiff {\n \tmy $format = shift || 'html';\n+\tmy %params = @_;\n \n \tmy $patch_max;\n \tif ($format eq 'patch') {\n@@ -5529,7 +5531,15 @@ sub git_commitdiff {\n \t\t\t}\n \t\t\tpush @commit_spec, '-n', \"$hash_parent..$hash\";\n \t\t} else {\n-\t\t\tpush @commit_spec, '-1', '--root', $hash;\n+\t\t\tif ($params{-single}) {\n+\t\t\t\tpush @commit_spec, '-1';\n+\t\t\t} else {\n+\t\t\t\tif ($patch_max > 0) {\n+\t\t\t\t\tpush @commit_spec, \"-$patch_max\";\n+\t\t\t\t}\n+\t\t\t\tpush @commit_spec, \"-n\";\n+\t\t\t}\n+\t\t\tpush @commit_spec, '--root', $hash;\n \t\t}\n \t\topen $fd, \"-|\", git_cmd(), \"format-patch\", '--encoding=utf8',\n \t\t\t'--stdout', @commit_spec\n@@ -5625,6 +5635,10 @@ sub git_commitdiff_plain {\n \n # format-patch-style patches\n sub git_patch {\n+\tgit_commitdiff('patch', -single=> 1);\n+}\n+\n+sub git_patches {\n \tgit_commitdiff('patch');\n }\n \n-- \n1.5.6.5\n"},{"id":"98060","messageId":"1229422290-6213-4-git-send-email-giuseppe.bilotta@gmail.com","threadId":"16750","inReplyTo":"1229422290-6213-3-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv5 3/3] gitweb: link to patch(es) view in commit(diff) and (short)log view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-16T10:11:30Z","receivedAt":"2008-12-16T10:11:30Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"We link to patch view in commit and commitdiff view, and to patches view\nin log and shortlog view.\n\nIn (short)log view, the link is only offered when the number of commits\nshown is no more than the allowed maximum number of patches.\n\nSigned-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>\n---\n gitweb/gitweb.perl |   30 ++++++++++++++++++++++++++++--\n 1 files changed, 28 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex e7a6477..168deb7 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5024,6 +5024,15 @@ sub git_log {\n \n \tmy $paging_nav = format_paging_nav('log', $hash, $head, $page, $#commitlist >= 100);\n \n+\tmy ($patch_max) = gitweb_get_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=>\"patches\", -replay=>1)},\n+\t\t\t\t\t\"patches\");\n+\t\t}\n+\t}\n+\n \tgit_header_html();\n \tgit_print_page_nav('log','', $hash,undef,undef, $paging_nav);\n \n@@ -5103,6 +5112,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@@ -5416,9 +5430,8 @@ sub git_commitdiff {\n \tmy $format = shift || 'html';\n \tmy %params = @_;\n \n-\tmy $patch_max;\n+\tmy ($patch_max) = gitweb_get_feature('patches');\n \tif ($format eq 'patch') {\n-\t\t($patch_max) = gitweb_get_feature('patches');\n \t\tdie_error(403, \"Patch view not allowed\") unless $patch_max;\n \t}\n \n@@ -5436,6 +5449,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@@ -5992,6 +6010,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=>\"patches\", -replay=>1)},\n+\t\t\t\t\t\"patches\");\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":"98227","messageId":"7v4p12hv5q.fsf@gitster.siamese.dyndns.org","threadId":"16750","inReplyTo":"1229422290-6213-3-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv5 2/3] gitweb: add patches view","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-18T06:09:05Z","receivedAt":"2008-12-18T06:09:05Z","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>  sub git_commitdiff {\n>  \tmy $format = shift || 'html';\n> +\tmy %params = @_;\n> ...  \n> +\t\t\tif ($params{-single}) {\n> +\t\t\t\tpush @commit_spec, '-1';\n> +\t\t\t} else {\n> +\t\t\t\tif ($patch_max > 0) {\n> ...\n> +\t\t\t}\n> @@ -5625,6 +5635,10 @@ sub git_commitdiff_plain {\n>  \n>  # format-patch-style patches\n>  sub git_patch {\n> +\tgit_commitdiff('patch', -single=> 1);\n> +}\n\nHmm, if you are changing the interface this way, wouldn't it make more\nsense to also do this?\n\n\tgit_commitdiff(--format => 'patch', --single => 1);\n\tgit_commitdiff(--format => 'html');\n"},{"id":"98242","messageId":"200812181033.57360.jnareb@gmail.com","threadId":"16750","inReplyTo":"7v4p12hv5q.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCHv5 2/3] gitweb: add patches view","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-18T09:33:55Z","receivedAt":"2008-12-18T09:33:55Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 18 Dec 2008, Junio C Hamano wrote:\n> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n> \n> >  sub git_commitdiff {\n> >  \tmy $format = shift || 'html';\n> > +\tmy %params = @_;\n> > ...  \n> > +\t\t\tif ($params{-single}) {\n> > +\t\t\t\tpush @commit_spec, '-1';\n> > +\t\t\t} else {\n> > +\t\t\t\tif ($patch_max > 0) {\n> > ...\n> > +\t\t\t}\n> > @@ -5625,6 +5635,10 @@ sub git_commitdiff_plain {\n> >  \n> >  # format-patch-style patches\n> >  sub git_patch {\n> > +\tgit_commitdiff('patch', -single=> 1);\n> > +}\n> \n> Hmm, if you are changing the interface this way, wouldn't it make more\n> sense to also do this?\n> \n> \tgit_commitdiff(--format => 'patch', --single => 1);\n> \tgit_commitdiff(--format => 'html');\n\nThe first argument (format) is _required_, second is _optional_;\nI'd rather use named parameters trick only for optional parameters.\nBecause with more than one optional parameter function call begins\nto be cryptic; also flag (boolean) parameters are more readable\nwhen used as named parameters.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"98259","messageId":"cb7bb73a0812180823y5249abcakd419d4fee9942d84@mail.gmail.com","threadId":"16750","inReplyTo":"200812181033.57360.jnareb@gmail.com","subject":"Re: [PATCHv5 2/3] gitweb: add patches view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-18T16:23:43Z","receivedAt":"2008-12-18T16:23:43Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Thu, Dec 18, 2008 at 10:33 AM, Jakub Narebski <jnareb@gmail.com> wrote:\n> On Thu, 18 Dec 2008, Junio C Hamano wrote:\n>> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n>>\n>> >  sub git_commitdiff {\n>> >     my $format = shift || 'html';\n>> > +   my %params = @_;\n>> > ...\n>> > +                   if ($params{-single}) {\n>> > +                           push @commit_spec, '-1';\n>> > +                   } else {\n>> > +                           if ($patch_max > 0) {\n>> > ...\n>> > +                   }\n>> > @@ -5625,6 +5635,10 @@ sub git_commitdiff_plain {\n>> >\n>> >  # format-patch-style patches\n>> >  sub git_patch {\n>> > +   git_commitdiff('patch', -single=> 1);\n>> > +}\n>>\n>> Hmm, if you are changing the interface this way, wouldn't it make more\n>> sense to also do this?\n>>\n>>       git_commitdiff(--format => 'patch', --single => 1);\n>>       git_commitdiff(--format => 'html');\n>\n> The first argument (format) is _required_, second is _optional_;\n> I'd rather use named parameters trick only for optional parameters.\n> Because with more than one optional parameter function call begins\n> to be cryptic; also flag (boolean) parameters are more readable\n> when used as named parameters.\n\nI have mixed feelings about this: on the one hand we have href() (say)\nthat takes all its params from a has, but on the other hand we have\nesc_html() (say) that takes only additional options from a hash. I'm\npersonally more inclined towards the latter usage for git_commitdiff\n(i.e. this patch) but since the other alternative is straightforward I\nsent a v6 of the patchset which implements it.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"98267","messageId":"200812181828.11815.jnareb@gmail.com","threadId":"16750","inReplyTo":"cb7bb73a0812180823y5249abcakd419d4fee9942d84@mail.gmail.com","subject":"Re: [PATCHv5 2/3] gitweb: add patches view","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-18T17:28:10Z","receivedAt":"2008-12-18T17:28:10Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, Dec 18, 2008, Giuseppe Bilotta wrote:\n> On Thu, Dec 18, 2008, Jakub Narebski <jnareb@gmail.com> wrote:\n>> On Thu, 18 Dec 2008, Junio C Hamano wrote:\n\n>>> [...] wouldn't it make more sense to also do this?\n>>>\n>>>       git_commitdiff(--format => 'patch', --single => 1);\n>>>       git_commitdiff(--format => 'html');\n>>\n>> The first argument (format) is _required_, second is _optional_;\n>> I'd rather use named parameters trick only for optional parameters.\n>> Because with more than one optional parameter function call begins\n>> to be cryptic; also flag (boolean) parameters are more readable\n>> when used as named parameters.\n> \n> I have mixed feelings about this: on the one hand we have href() (say)\n> that takes all its params from a has, but on the other hand we have\n> esc_html() (say) that takes only additional options from a hash. [...]\n\nIt is not \"on one hand\".  href() is a bit special case. It has _all_\nparameters optional, especially now that -replay=>1 was introduced.\nIt has no required parameters. Therefore all parameters are named.\n\nBeside, href() mimics a bit CGI interface...\n\n-- \nJakub Narebski\nPoland\n"},{"id":"98271","messageId":"200812181915.48556.jnareb@gmail.com","threadId":"16750","inReplyTo":"7v4p12hv5q.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCHv5 2/3] gitweb: add patches view","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-18T18:15:47Z","receivedAt":"2008-12-18T18:15:47Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 18 Dec 2008, Junio C Hamano wrote:\n> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n> \n> >  sub git_commitdiff {\n> >  \tmy $format = shift || 'html';\n> > +\tmy %params = @_;\n> > ...  \n> > +\t\t\tif ($params{-single}) {\n> > +\t\t\t\tpush @commit_spec, '-1';\n> > +\t\t\t} else {\n> > +\t\t\t\tif ($patch_max > 0) {\n> > ...\n> > +\t\t\t}\n> > @@ -5625,6 +5635,10 @@ sub git_commitdiff_plain {\n> >  \n> >  # format-patch-style patches\n> >  sub git_patch {\n> > +\tgit_commitdiff('patch', -single=> 1);\n> > +}\n> \n> Hmm, if you are changing the interface this way, wouldn't it make more\n> sense to also do this?\n> \n> \tgit_commitdiff(--format => 'patch', --single => 1);\n> \tgit_commitdiff(--format => 'html');\n\nJust a though, but does it really take much sense to have \"patch\" and\n\"patches\" views handled in git_commitdiff? I can understand in the\nfirst version, where it was more about better 'commitdiff_plain', but\nI wonder about now, where patch(es) view is something between git show\nand log_plain format... Wouldn't it be simpler to use separate \nsubroutine, for example git_format_patch or something?\n\n-- \nJakub Narebski\nPoland\n"},{"id":"98272","messageId":"7viqphguqu.fsf@gitster.siamese.dyndns.org","threadId":"16750","inReplyTo":"200812181033.57360.jnareb@gmail.com","subject":"Re: [PATCHv5 2/3] gitweb: add patches view","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-18T19:15:37Z","receivedAt":"2008-12-18T19:15: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> On Thu, 18 Dec 2008, Junio C Hamano wrote:\n>> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n>> \n>> >  sub git_commitdiff {\n>> >  \tmy $format = shift || 'html';\n>> > +\tmy %params = @_;\n>> > ...  \n>> > +\t\t\tif ($params{-single}) {\n>> > +\t\t\t\tpush @commit_spec, '-1';\n>> > +\t\t\t} else {\n>> > +\t\t\t\tif ($patch_max > 0) {\n>> > ...\n>> > +\t\t\t}\n>> > @@ -5625,6 +5635,10 @@ sub git_commitdiff_plain {\n>> >  \n>> >  # format-patch-style patches\n>> >  sub git_patch {\n>> > +\tgit_commitdiff('patch', -single=> 1);\n>> > +}\n>> \n>> Hmm, if you are changing the interface this way, wouldn't it make more\n>> sense to also do this?\n>> \n>> \tgit_commitdiff(--format => 'patch', --single => 1);\n>> \tgit_commitdiff(--format => 'html');\n>\n> The first argument (format) is _required_, second is _optional_;\n\nMy reading of the first line of the sub seems to contradict with your\nstatement.  \"If the first argument is empty, we use 'html' instead\" sounds\npretty optional to me.\n"},{"id":"98275","messageId":"200812182044.32154.jnareb@gmail.com","threadId":"16750","inReplyTo":"7viqphguqu.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCHv5 2/3] gitweb: add patches view","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-18T19:44:31Z","receivedAt":"2008-12-18T19:44:31Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n>> On Thu, 18 Dec 2008, Junio C Hamano wrote:\n>>> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n>>> \n>>>>  sub git_commitdiff {\n>>>>  \tmy $format = shift || 'html';\n>>>> +\tmy %params = @_;\n>>>> ...  \n>>>> +\t\t\tif ($params{-single}) {\n>>>> +\t\t\t\tpush @commit_spec, '-1';\n>>>> +\t\t\t} else {\n>>>> +\t\t\t\tif ($patch_max> 0) {\n>>>> ...\n>>>> +\t\t\t}\n>>>> @@ -5625,6 +5635,10 @@ sub git_commitdiff_plain {\n>>>>  \n>>>>  # format-patch-style patches\n>>>>  sub git_patch {\n>>>> +\tgit_commitdiff('patch', -single=> 1);\n>>>> +}\n>>> \n>>> Hmm, if you are changing the interface this way, wouldn't it make more\n>>> sense to also do this?\n>>> \n>>> \tgit_commitdiff(--format => 'patch', --single => 1);\n>>> \tgit_commitdiff(--format => 'html');\n>>\n>> The first argument (format) is _required_, second is _optional_;\n> \n> My reading of the first line of the sub seems to contradict with your\n> statement.  \"If the first argument is empty, we use 'html' instead\" sounds\n> pretty optional to me.\n \nWell, for me it is more of required argument, but with default value.\nBut now we argue semantics... :-)\n\n-- \nJakub Narebski\nPoland\n"},{"id":"98279","messageId":"cb7bb73a0812181157r20b0cbc3t290e32454a435b0c@mail.gmail.com","threadId":"16750","inReplyTo":"200812181915.48556.jnareb@gmail.com","subject":"Re: [PATCHv5 2/3] gitweb: add patches view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-18T19:57:43Z","receivedAt":"2008-12-18T19:57:43Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Thu, Dec 18, 2008 at 7:15 PM, Jakub Narebski <jnareb@gmail.com> wrote:\n>\n> Just a though, but does it really take much sense to have \"patch\" and\n> \"patches\" views handled in git_commitdiff? I can understand in the\n> first version, where it was more about better 'commitdiff_plain', but\n> I wonder about now, where patch(es) view is something between git show\n> and log_plain format... Wouldn't it be simpler to use separate\n> subroutine, for example git_format_patch or something?\n\nI don't feel like much has changed since the beginning when IYO it\nmade sense to have it part of commitdiff, honestly\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"}]}