{"thread":{"id":"16611","subject":"[RFCv4 0/3] gitweb: patch view","startedAt":"2008-12-06T15:02:32Z","lastAt":"2008-12-16T11:14:26Z","messageCount":12,"participants":["Giuseppe Bilotta","Jakub Narebski"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"97263","messageId":"1228575755-13432-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"16611","inReplyTo":null,"subject":"[RFCv4 0/3] gitweb: patch view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-06T15:02:32Z","receivedAt":"2008-12-06T15:02:32Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Fourth iteration of the patch view feature, that exposes git\nformat-patch output in gitweb. The most significant different from the\nprevious revision is the introduction of the 'patches' view, that only\ndiffers from 'patch' view in the treatment of single commits: 'patch'\nview only displays the patch for that specific commit, whereas 'patches'\nfollows the git format-patch M.O.\n\nGiuseppe Bilotta (3):\n  gitweb: add patch view\n  gitweb: add patches view\n  gitweb: link to patch(es) view from commit and log views\n\n gitweb/gitweb.perl |  107 +++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 files changed, 106 insertions(+), 1 deletions(-)\n"},{"id":"97266","messageId":"1228575755-13432-2-git-send-email-giuseppe.bilotta@gmail.com","threadId":"16611","inReplyTo":"1228575755-13432-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[RFCv4 1/3] gitweb: add patch view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-06T15:02:33Z","receivedAt":"2008-12-06T15:02:33Z","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 is imposed on the number of commits which\nwill be included in a patchset, to prevent DoS attacks on the server.\nSetting the limit to 0 will disable the patch view, setting it to a\nnegative number will remove the limit.\n\nSigned-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>\n---\n gitweb/gitweb.perl |   65 +++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 files changed, 64 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 95988fb..71d5af4 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -329,6 +329,14 @@ 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'sub' => \\&feature_patches,\n+\t\t'override' => 0,\n+\t\t'default' => [16]},\n );\n \n sub gitweb_get_feature {\n@@ -410,6 +418,19 @@ sub feature_pickaxe {\n \treturn ($_[0]);\n }\n \n+sub feature_patches {\n+\tmy @val = (git_get_project_config('patches', '--int'));\n+\n+\t# if @val is empty, the config is not (properly)\n+\t# overriding the feature, so we return the default,\n+\t# otherwise we pick the override\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@@ -503,6 +524,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 +5408,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_check_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@@ -5483,7 +5512,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@@ -5532,6 +5577,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@@ -5553,6 +5606,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 +5618,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":"97265","messageId":"1228575755-13432-3-git-send-email-giuseppe.bilotta@gmail.com","threadId":"16611","inReplyTo":"1228575755-13432-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"[RFCv4 2/3] gitweb: add patches view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-06T15:02:34Z","receivedAt":"2008-12-06T15:02:34Z","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---\n gitweb/gitweb.perl |   20 ++++++++++++++++++--\n 1 files changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 71d5af4..dfc7128 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -525,6 +525,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@@ -5408,6 +5409,9 @@ sub git_blobdiff_plain {\n \n sub git_commitdiff {\n \tmy $format = shift || 'html';\n+\t# for patch view: should we limit ourselves to a single patch\n+\t# if only a single commit is passed?\n+\tmy $single_patch = shift && 1;\n \n \tmy $patch_max;\n \tif ($format eq 'patch') {\n@@ -5524,7 +5528,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 ($single_patch) {\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@@ -5620,7 +5632,11 @@ sub git_commitdiff_plain {\n \n # format-patch-style patches\n sub git_patch {\n-\tgit_commitdiff('patch');\n+\tgit_commitdiff('patch', 1);\n+}\n+\n+sub git_patches {\n+\tgit_commitdiff('patch', 0);\n }\n \n sub git_history {\n-- \n1.5.6.5\n"},{"id":"97264","messageId":"1228575755-13432-4-git-send-email-giuseppe.bilotta@gmail.com","threadId":"16611","inReplyTo":"1228575755-13432-3-git-send-email-giuseppe.bilotta@gmail.com","subject":"[RFCv4 3/3] gitweb: link to patch(es) view from commit and log views","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-06T15:02:35Z","receivedAt":"2008-12-06T15:02:35Z","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 dfc7128..8198875 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5019,6 +5019,15 @@ sub git_log {\n \n \tmy $paging_nav = format_paging_nav('log', $hash, $head, $page, $#commitlist >= 100);\n \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@commitlist > 1 ? \"patchset\" : \"patch\");\n+\t\t}\n+\t}\n+\n \tgit_header_html();\n \tgit_print_page_nav('log','', $hash,undef,undef, $paging_nav);\n \n@@ -5098,6 +5107,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@@ -5413,9 +5427,8 @@ sub git_commitdiff {\n \t# if only a single commit is passed?\n \tmy $single_patch = shift && 1;\n \n-\tmy $patch_max;\n+\tmy $patch_max = gitweb_check_feature('patches');\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@@ -5433,6 +5446,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@@ -5989,6 +6007,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@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":"97955","messageId":"200812151417.16372.jnareb@gmail.com","threadId":"16611","inReplyTo":"1228575755-13432-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [RFCv4 1/3] gitweb: add patch view","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-15T13:17:15Z","receivedAt":"2008-12-15T13:17:15Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 6 Dec 2008, Giuseppe Bilotta wrote:\n\n> 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> \n> We thus create a new view that can be fed to git-am directly, allowing\n> patch exchange via gitweb. The new view exposes the output of git\n> format-patch directly, limiting it to a single patch in the case of a\n> single commit.\n> \n> A configurable upper limit is imposed on the number of commits which\n> will be included in a patchset, to prevent DoS attacks on the server.\n> Setting the limit to 0 will disable the patch view, setting it to a\n> negative number will remove the limit.\n\nIt would be good to have in commit mesage what is the default upper\nlimit (or if we decide to have this feature turned off by default,\nproposed limit to choose when enabling this feature).\n\nDoes limit of 16 patches have any numbers behind it? We use page size\nof 100 commits for 'shortlog', 'log' and 'history' views, but for\nthose views we don't need to generate patches (which is slower). From\na few tests \"git log -100\" is faster than \"git format-patch -100 --stdout\"\nabout 30 times in warm cache case, and about 1.7 times in cold cache\ncase.\n\n> \n> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>\n\nOther than that minor detail I like this patch\n\n> ---\n>  gitweb/gitweb.perl |   65 +++++++++++++++++++++++++++++++++++++++++++++++++++-\n>  1 files changed, 64 insertions(+), 1 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 95988fb..71d5af4 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -329,6 +329,14 @@ 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\nI think it would be nice to have standard boilerplate explanation how\nto change it, and how to override it, with the addition on how to\ndisable it from repository config, because it is not very obvious.\n\nSomething like:\n\n+\t# To disable system wide have in $GITWEB_CONFIG\n+\t# $feature{'patches'}{'default'} = [];\n+\t# To have project specific config enable override in $GITWEB_CONFIG\n+\t# $feature{'patches'}{'override'} = 1;\n+\t# and in project config, maximum number of patches in a patchset\n+\t# or 0 to disable.  Example: gitweb.patches = 0\n\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> @@ -410,6 +418,19 @@ sub feature_pickaxe {\n>  \treturn ($_[0]);\n>  }\n>  \n> +sub feature_patches {\n> +\tmy @val = (git_get_project_config('patches', '--int'));\n> +\n> +\t# if @val is empty, the config is not (properly)\n> +\t# overriding the feature, so we return the default,\n> +\t# otherwise we pick the override\n\nVery similar feature_snapshot subroutine doesn't have such comment.\nI don't think it is necessary, and its wording might cause confusion.\n\n> +\tif (@val) {\n> +\t\treturn @val;\n> +\t}\n> +\n> +\treturn ($_[0]);\n> +}\n> +\n\nNice.\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> @@ -503,6 +524,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 +5408,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_check_feature('patches');\n> +\t\tdie_error(403, \"Patch view not allowed\") unless $patch_max;\n> +\t}\n> +\n\nHmmm... gitweb_check_feature\n\n>  \t$hash ||= $hash_base || \"HEAD\";\n>  \tmy %co = parse_commit($hash)\n>  \t    or die_error(404, \"Unknown commit object\");\n> @@ -5483,7 +5512,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\nI think it would be more clear to use\n\n+\t\t# diverging from the git-format-patch default.\n\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\nNice solution. I like it.\n\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> @@ -5532,6 +5577,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\nI am wondering if we could somehow mark (encode) either $hash_parent\nor number of patches in proposed filename... but I don't think it is\nworth it.\n\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> @@ -5553,6 +5606,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\nNice, although... I'd prefer for Perl expert to say if it is better\nto dump file as a whole in such way (it might be quite large), or\nto do it line by line, i.e. without \"local $/ = undef;\", or using\n\"print while <$fd>;\" also without \"local $/ = undef;\".\n\n\n>  \t}\n>  }\n>  \n> @@ -5560,6 +5618,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\nNice.\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":"97957","messageId":"cb7bb73a0812150548w526a8c0eu13ec95785e0ab824@mail.gmail.com","threadId":"16611","inReplyTo":"200812151417.16372.jnareb@gmail.com","subject":"Re: [RFCv4 1/3] gitweb: add patch view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-15T13:48:20Z","receivedAt":"2008-12-15T13:48:20Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Mon, Dec 15, 2008 at 2:17 PM, Jakub Narebski <jnareb@gmail.com> wrote:\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>\n> Hmmm... gitweb_check_feature\n\nYou're right, it's an abuse. I'll make it gitweb_get_feature()[0]\n\n> I am wondering if we could somehow mark (encode) either $hash_parent\n> or number of patches in proposed filename... but I don't think it is\n> worth it.\n\nIncluding hash_parent if defined is quite possible. I'm not sure it's\nreally worth it considering that the typical usage would be to publish\na patchset for a particular feature (in which case the hash/branch\nname would be enough).\n\n>> +     } elsif ($format eq 'patch') {\n>> +             local $/ = undef;\n>> +             print <$fd>;\n>> +             close $fd\n>> +                     or print \"Reading git-format-patch failed\\n\";\n>\n> Nice, although... I'd prefer for Perl expert to say if it is better\n> to dump file as a whole in such way (it might be quite large), or\n> to do it line by line, i.e. without \"local $/ = undef;\", or using\n> \"print while <$fd>;\" also without \"local $/ = undef;\".\n\nI'm just sticking to whatever the existing code does :-)\n\nAs soon as you finish the patchset review I'll have a new version\ntaking into consideration all the other suggestions and remarks I\nsnipped from this reply.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"97959","messageId":"200812151458.55557.jnareb@gmail.com","threadId":"16611","inReplyTo":"cb7bb73a0812150548w526a8c0eu13ec95785e0ab824@mail.gmail.com","subject":"Re: [RFCv4 1/3] gitweb: add patch view","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-15T13:58:55Z","receivedAt":"2008-12-15T13:58:55Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Giuseppe Bilotta wrote:\n> On Mon, Dec 15, 2008 at 2:17 PM, Jakub Narebski <jnareb@gmail.com> wrote:\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>>\n>> Hmmm... gitweb_check_feature\n> \n> You're right, it's an abuse. I'll make it gitweb_get_feature()[0]\n\nOr better\n\n+             ($patch_max) = gitweb_get_feature('patches');\n\n[...]\n> As soon as you finish the patchset review I'll have a new version\n> taking into consideration all the other suggestions and remarks I\n> snipped from this reply.\n\nThank you very much for keeping this patch series alive.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"98019","messageId":"200812160203.29425.jnareb@gmail.com","threadId":"16611","inReplyTo":"1228575755-13432-4-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [RFCv4 3/3] gitweb: link to patch(es) view from commit and log views","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-16T01:03:28Z","receivedAt":"2008-12-16T01:03:28Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 6 Dec 2008, Giuseppe Bilotta wrote:\n\n> We link to patch view in commit and commitdiff view, and to patches view\n> in log and shortlog view.\n> \n> In (short)log view, the link is only offered when the number of commits\n> shown is no more than the allowed maximum number of patches.\n> \n> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>\n\nA few remarks, but otherwise:\n\nAcked-by: Jakub Narebski <jnareb@gmail.com>\n\n> ---\n>  gitweb/gitweb.perl |   30 ++++++++++++++++++++++++++++--\n>  1 files changed, 28 insertions(+), 2 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index dfc7128..8198875 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -5019,6 +5019,15 @@ sub git_log {\n>  \n>  \tmy $paging_nav = format_paging_nav('log', $hash, $head, $page, $#commitlist >= 100);\n>  \n> +\tmy $patch_max = gitweb_check_feature('patches');\n\nIf you change other places to use git_get_feature, then you should\nchange it too. I'm not sure if it is worth it...\n\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@commitlist > 1 ? \"patchset\" : \"patch\");\n\nI think it would be better to always use \"patches\" here, as it is\nseries of patches by design, only by accident it is one commit long.\n\nI wonder if it would make sense to pass\n\n   href(..., hash_parent => $commitlist[-1]{'id'}, ...)\n\nhere. But I think having separate \"patches\" action, with intent being\ndisplaying series of patches, is a better solution. This way you can\nsee in URL and in the page title (thus also in window title, or in\nbookmark name) if it is single patch or patch series (perhaps consisting\nof single patch).\n\n> +\t\t}\n> +\t}\n> +\n>  \tgit_header_html();\n>  \tgit_print_page_nav('log','', $hash,undef,undef, $paging_nav);\n>  \n> @@ -5098,6 +5107,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\nHere using gitweb_check_feature makes perfect sense.\n\n>  \n>  \tif (!defined $parent) {\n>  \t\t$parent = \"--root\";\n> @@ -5413,9 +5427,8 @@ sub git_commitdiff {\n>  \t# if only a single commit is passed?\n>  \tmy $single_patch = shift && 1;\n>  \n> -\tmy $patch_max;\n> +\tmy $patch_max = gitweb_check_feature('patches');\n\ngitweb_check_feature or gitweb_get_feature?...\n\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> @@ -5433,6 +5446,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\nNice reusing $patch_max in different way, as $have_patch if $format is\n'html', and as limiter ($patch_max) if $format is 'patch'/'patches'\n\n>  \n>  \t\tif (defined $hash_parent &&\n>  \t\t    $hash_parent ne '-c' && $hash_parent ne '--cc') {\n> @@ -5989,6 +6007,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@commitlist > 1 ? \"patchset\" : \"patch\");\n> +\t\t}\n> +\t}\n\nSame comment as for git_log...\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":"98030","messageId":"200812160414.56486.jnareb@gmail.com","threadId":"16611","inReplyTo":"1228575755-13432-3-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [RFCv4 2/3] gitweb: add patches view","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-16T03:14:55Z","receivedAt":"2008-12-16T03:14:55Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 6 Dec 2008 16:02, Giuseppe Bilotta wrote:\n\n> The only difference between patch and patches view is in the treatement\n> of single commits: the former only displays a single patch, whereas the\n> latter displays a patchset leading to the specified commit.\n\nI like that fact that we have \"patches\" action which intent is to\nshow series of patches, and \"patch\" action which intent is to show\nsingle patch. I'm just not sure if \"patch\" view should not simply\nignore $hash_parent...\n\nSignoff?\n\n> ---\n>  gitweb/gitweb.perl |   20 ++++++++++++++++++--\n>  1 files changed, 18 insertions(+), 2 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 71d5af4..dfc7128 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -525,6 +525,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> @@ -5408,6 +5409,9 @@ sub git_blobdiff_plain {\n>  \n>  sub git_commitdiff {\n>  \tmy $format = shift || 'html';\n> +\t# for patch view: should we limit ourselves to a single patch\n> +\t# if only a single commit is passed?\n> +\tmy $single_patch = shift && 1;\n\nWhat does this \"shift && 1\" does? Equivalent of \"!!shift\"?\nIs it really needed?\n\nPerhaps it would be better to use %opts trick, like for some other\ngitweb subroutines (-single=>1, or -single_patch=>1, or -nmax=>1)?\nOr perhaps not...\n\n>  \n>  \tmy $patch_max;\n>  \tif ($format eq 'patch') {\n> @@ -5524,7 +5528,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 ($single_patch) {\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\nNice.\n\n>  \t\t}\n>  \t\topen $fd, \"-|\", git_cmd(), \"format-patch\", '--encoding=utf8',\n>  \t\t\t'--stdout', @commit_spec\n> @@ -5620,7 +5632,11 @@ sub git_commitdiff_plain {\n>  \n>  # format-patch-style patches\n>  sub git_patch {\n> -\tgit_commitdiff('patch');\n> +\tgit_commitdiff('patch', 1);\n> +}\n> +\n> +sub git_patches {\n> +\tgit_commitdiff('patch', 0);\n\nI quite like it.\n\n>  }\n>  \n>  sub git_history {\n> -- \n> 1.5.6.5\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"98061","messageId":"200812161114.35336.jnareb@gmail.com","threadId":"16611","inReplyTo":"cb7bb73a0812160202n1f4f7f4fi7f71455eb42bcd31@mail.gmail.com","subject":"Re: [RFCv4 3/3] gitweb: link to patch(es) view from commit and log views","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-16T10:14:34Z","receivedAt":"2008-12-16T10:14:34Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"You lost CC, somehow...\n\nOn Tue, 16 Dec 2008, Giuseppe Bilotta wrote:\n> On Tue, Dec 16, 2008 at 2:03 AM, Jakub Narebski <jnareb@gmail.com> wrote:\n> > On Sat, 6 Dec 2008, Giuseppe Bilotta wrote:\n\n\n>>> +     if ($patch_max) {\n>>> +             if ($patch_max < 0 || @commitlist <= $patch_max) {\n>>> +                     $paging_nav .= \" &sdot; \" .\n>>> +                             $cgi->a({-href => href(action=>\"patches\", -replay=>1)},\n>>> +                                     @commitlist > 1 ? \"patchset\" : \"patch\");\n\n[...]\n>> I wonder if it would make sense to pass\n>>\n>>   href(..., hash_parent => $commitlist[-1]{'id'}, ...)\n>>\n>> here. But I think having separate \"patches\" action, with intent being\n>> displaying series of patches, is a better solution. This way you can\n>> see in URL and in the page title (thus also in window title, or in\n>> bookmark name) if it is single patch or patch series (perhaps consisting\n>> of single patch).\n> \n> I'm not sure I'm following you here. Do you mean as in manually adding\n> the parent endpoint to the URL when it's not specified in the log view\n> itself? I think that would change the behaviour for > 100 patches.\n\nFirst, I meant here that having separate \"patches\" action is a good\nidea in itself, whether we pass explicitly and always $hash_parent\nparameter here or not.\n \nSecond, I haven't thought about interaction with (short)log\npagination; in $patch_max < 0, i.e. unlimited patches, and most\ncommon case of running 'shortlog' action without 'hp' (hash_parent)\nlimiter used, one would make 'patches' limited to page size,\nother unlimited.  On one hand side limiting to page size makes\n\"patches\" be more of equivalent of current \"shortlog\" view; on the\nother hand it makes 'unlimited' actually be limited to page size,\nat least in this situation...\n\n-- \nJakub Narebski\nPoland\n"},{"id":"98062","messageId":"200812161116.45024.jnareb@gmail.com","threadId":"16611","inReplyTo":"cb7bb73a0812160149j1dcaefccv1caf4a2e589ffebb@mail.gmail.com","subject":"Re: [RFCv4 2/3] gitweb: add patches view","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-16T10:16:44Z","receivedAt":"2008-12-16T10:16:44Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Giuseppe Bilotta wrote:\n> On Tue, Dec 16, 2008 at 4:14 AM, Jakub Narebski <jnareb@gmail.com> wrote:\n>> On Sat, 6 Dec 2008 16:02, Giuseppe Bilotta wrote:\n>>\n>>> The only difference between patch and patches view is in the treatement\n>>> of single commits: the former only displays a single patch, whereas the\n>>> latter displays a patchset leading to the specified commit.\n>>\n>> I like that fact that we have \"patches\" action which intent is to\n>> show series of patches, and \"patch\" action which intent is to show\n>> single patch. I'm just not sure if \"patch\" view should not simply\n>> ignore $hash_parent...\n> \n> I had doubts on this myself. In the end I decided to make patch\n> consider hash_parent if present because IMO it's what a user would\n> expect in case e.g. of hand-crafted URLs.\n\nAh. I can understand that.\n\n[...]\n>>>  sub git_commitdiff {\n>>>       my $format = shift || 'html';\n>>> +     # for patch view: should we limit ourselves to a single patch\n>>> +     # if only a single commit is passed?\n>>> +     my $single_patch = shift && 1;\n>>\n>> What does this \"shift && 1\" does? Equivalent of \"!!shift\"?\n>> Is it really needed?\n>>\n>> Perhaps it would be better to use %opts trick, like for some other\n>> gitweb subroutines (-single=>1, or -single_patch=>1, or -nmax=>1)?\n>> Or perhaps not...\n> \n> It would be MUCH better, I'll do it this way. I'll pass the -single\n> param in both cases, having value true/false, even though the false\n> case is not needed since undef is false in perl anyway. (I like\n> symmetry.)\n\n-- \nJakub Narebski\nPoland\n"},{"id":"98063","messageId":"cb7bb73a0812160314t4a74dbb3r4748ad9190a88bd6@mail.gmail.com","threadId":"16611","inReplyTo":"200812161114.35336.jnareb@gmail.com","subject":"Re: [RFCv4 3/3] gitweb: link to patch(es) view from commit and log views","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-16T11:14:26Z","receivedAt":"2008-12-16T11:14:26Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Tue, Dec 16, 2008 at 11:14 AM, Jakub Narebski <jnareb@gmail.com> wrote:\n> You lost CC, somehow...\n\nDuh, clicked the wrong button.\n\n>>> I wonder if it would make sense to pass\n>>>\n>>>   href(..., hash_parent => $commitlist[-1]{'id'}, ...)\n>>>\n>>> here. But I think having separate \"patches\" action, with intent being\n>>> displaying series of patches, is a better solution. This way you can\n>>> see in URL and in the page title (thus also in window title, or in\n>>> bookmark name) if it is single patch or patch series (perhaps consisting\n>>> of single patch).\n>>\n>> I'm not sure I'm following you here. Do you mean as in manually adding\n>> the parent endpoint to the URL when it's not specified in the log view\n>> itself? I think that would change the behaviour for > 100 patches.\n>\n> First, I meant here that having separate \"patches\" action is a good\n> idea in itself, whether we pass explicitly and always $hash_parent\n> parameter here or not.\n\nI'm not too sure about that. Do we pass the actual hash parent, or\njust the last patch that would be included due to the patch limit?\nThis, btw, is an issue that should resolved with patch view: it should\nwarn when the patch limit is reached before all intended patches are\noutput. I'm just not sure about how to do it.\n\n> Second, I haven't thought about interaction with (short)log\n> pagination; in $patch_max < 0, i.e. unlimited patches, and most\n> common case of running 'shortlog' action without 'hp' (hash_parent)\n> limiter used, one would make 'patches' limited to page size,\n> other unlimited.  On one hand side limiting to page size makes\n> \"patches\" be more of equivalent of current \"shortlog\" view; on the\n> other hand it makes 'unlimited' actually be limited to page size,\n> at least in this situation...\n\nConsider that switching from log to shortlog view doesn't respect\npagination (e.g. if you're on page 3 of shortlog and click on log you\ngo to page 1 of log) I would be inclined to keep patches behaviour\nindependent of log view pagination.\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"}]}