{"thread":{"id":"21101","subject":"[PATCH] gitweb: Do not show 'patch' link in 'commit' view for merges","startedAt":"2009-09-30T20:21:53Z","lastAt":"2009-10-10T00:56:57Z","messageCount":10,"participants":["Jakub Narebski","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"124043","messageId":"20090930201953.22301.73887.stgit@localhost.localdomain","threadId":"21101","inReplyTo":null,"subject":"[PATCH] gitweb: Do not show 'patch' link in 'commit' view for merges","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-09-30T20:21:53Z","receivedAt":"2009-09-30T20:21:53Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Show 'patch' link in the 'commit' view only for commits which have\nexactly one parent, otherwise call to git-format-patch would fail for\nthe hyperlinked 'patch' action.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nJust noticed this issue, thanks indirectly to \n\"Re: How can I download a git commit as a diff patch?\"\nthread.\n\nThis is conservative change: perhaps we could extend 'patch' view to\nwork also for root commit...\n\n gitweb/gitweb.perl |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 24b2193..fb045a1 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5328,7 +5328,7 @@ sub git_commit {\n \t\t\t} @$parents ) .\n \t\t\t')';\n \t}\n-\tif (gitweb_check_feature('patches')) {\n+\tif (gitweb_check_feature('patches') && @$parents == 1) {\n \t\t$formats_nav .= \" | \" .\n \t\t\t$cgi->a({-href => href(action=>\"patch\", -replay=>1)},\n \t\t\t\t\"patch\");\n"},{"id":"124063","messageId":"20091001031140.GA30094@coredump.intra.peff.net","threadId":"21101","inReplyTo":"20090930201953.22301.73887.stgit@localhost.localdomain","subject":"Re: [PATCH] gitweb: Do not show 'patch' link in 'commit' view for merges","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-10-01T03:11:40Z","receivedAt":"2009-10-01T03:11:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 30, 2009 at 10:21:53PM +0200, Jakub Narebski wrote:\n\n> Show 'patch' link in the 'commit' view only for commits which have\n> exactly one parent, otherwise call to git-format-patch would fail for\n> the hyperlinked 'patch' action.\n\nFail in what way? From my cursory reading of the code, it looks like the\n'patch' action calls into git_commitdiff, which handles the multi-parent\ncase.\n\nI assume I'm reading wrong, since you obviously know gitweb much better\nthan I do. :) But can you expand a little on the nature of the problem\nin the commit message?\n\n-Peff\n"},{"id":"124067","messageId":"200910010936.24789.jnareb@gmail.com","threadId":"21101","inReplyTo":"20091001031140.GA30094@coredump.intra.peff.net","subject":"Re: [PATCH] gitweb: Do not show 'patch' link in 'commit' view for merges","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-10-01T07:36:23Z","receivedAt":"2009-10-01T07:36:23Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Dnia czwartek 1. października 2009 05:11, Jeff King napisał:\n> On Wed, Sep 30, 2009 at 10:21:53PM +0200, Jakub Narebski wrote:\n> \n> > Show 'patch' link in the 'commit' view only for commits which have\n> > exactly one parent, otherwise call to git-format-patch would fail for\n> > the hyperlinked 'patch' action.\n> \n> Fail in what way? From my cursory reading of the code, it looks like the\n> 'patch' action calls into git_commitdiff, which handles the multi-parent\n> case.\n> \n> I assume I'm reading wrong, since you obviously know gitweb much better\n> than I do. :) But can you expand a little on the nature of the problem\n> in the commit message?\n\nWell, from the point of view of behavior, 'patch' link in 'commit' view\nfor a merge commit, e.g.\n  gitweb.cgi/git.git/commit/833423ae071ffedb7fbca39789f14f9a45a3d1c4\nleads to the 'patch' view\n  git.git/patch/833423ae071ffedb7fbca39789f14f9a45a3d1c4\nwhich leads to 'text/plain' output with the following contents:\n\n  Reading git-format-patch failed\n\n\nFrom the point of view of code, 'patch' view is handled by git_patch()\nsubroutine, which in turn calls git_commitdiff(-format => 'patch', -single=> 1);\ngit_commitdiff checks if 'patch' view is enabled feature, and then \ncomposes and calls the following command (I have skipped --git-dir=...):\n\n  git format-patch --encoding=utf8 --stdout -1 --root <commit-id>\n\nAnd git-format-patch produces no output for merge commit.  Then \ngit_commitdiff dumps output of git-format-patch\n\n\tlocal $/ = undef;\n\tprint <$fd>;\n\nand somehow fails on closing filehandle\n\n\tclose $fd\n\t\tor print \"Reading git-format-patch failed\\n\";\n\nEven if 'patch' view didn't fail, it is not a good idea to have link\nto an empty page (or page with only error message).  Though probably\ngit_commitdiff could check if it is used for a merge commit...\n\n-- \nJakub Narebski\nPoland\n"},{"id":"124068","messageId":"20091001075540.GA13436@coredump.intra.peff.net","threadId":"21101","inReplyTo":"200910010936.24789.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Do not show 'patch' link in 'commit' view for merges","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-10-01T07:55:40Z","receivedAt":"2009-10-01T07:55:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 01, 2009 at 09:36:23AM +0200, Jakub Narebski wrote:\n\n> From the point of view of code, 'patch' view is handled by git_patch()\n> subroutine, which in turn calls git_commitdiff(-format => 'patch', -single=> 1);\n> git_commitdiff checks if 'patch' view is enabled feature, and then \n> composes and calls the following command (I have skipped --git-dir=...):\n> \n>   git format-patch --encoding=utf8 --stdout -1 --root <commit-id>\n> \n> And git-format-patch produces no output for merge commit.  Then \n> git_commitdiff dumps output of git-format-patch\n\nAh, OK, I see the code path you are talking about now. My first thought\nwas \"shouldn't it be handling the merge case?\" but that really doesn't\nmake any sense. The \"patch\" view is just about something that can be\ngiven to \"git am\", and it doesn't understand merges anyway. And the\n\"commitdiff\" and \"html\" formats already handle it appropriately.\n\nSo I think your patch is the right thing to do. Thanks for the\nexplanation.\n\n-Peff\n"},{"id":"124073","messageId":"200910011118.02875.jnareb@gmail.com","threadId":"21101","inReplyTo":"20091001075540.GA13436@coredump.intra.peff.net","subject":"Re: [PATCH] gitweb: Do not show 'patch' link in 'commit' view for merges","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-10-01T09:18:01Z","receivedAt":"2009-10-01T09:18:01Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Dnia czwartek 1. października 2009 09:55, Jeff King napisał:\n> On Thu, Oct 01, 2009 at 09:36:23AM +0200, Jakub Narebski wrote:\n> \n> > From the point of view of code, 'patch' view is handled by git_patch()\n> > subroutine, which in turn calls git_commitdiff(-format => 'patch', -single=> 1);\n> > git_commitdiff checks if 'patch' view is enabled feature, and then \n> > composes and calls the following command (I have skipped --git-dir=...):\n> > \n> >   git format-patch --encoding=utf8 --stdout -1 --root <commit-id>\n> > \n> > And git-format-patch produces no output for merge commit.  Then \n> > git_commitdiff dumps output of git-format-patch\n> \n> Ah, OK, I see the code path you are talking about now. My first thought\n> was \"shouldn't it be handling the merge case?\" but that really doesn't\n> make any sense. The \"patch\" view is just about something that can be\n> given to \"git am\", and it doesn't understand merges anyway. And the\n> \"commitdiff\" and \"html\" formats already handle it appropriately.\n> \n> So I think your patch is the right thing to do. Thanks for the\n> explanation.\n\nI'll enhance commit message to talk about issues touched here (that\n'patch' is for git-am and doesn't make sense for merges, regardless\nof possible bug in 'patch' view when used for merge commits), and\ncheck if this correction isn't needed also elsewhere.\n\nI'll send v2 of this patch in a bit.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"124479","messageId":"200910091410.15904.jnareb@gmail.com","threadId":"21101","inReplyTo":"200910011118.02875.jnareb@gmail.com","subject":"[PATCH v2] gitweb: Do not show 'patch' link for merge commits","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-10-09T12:10:14Z","receivedAt":"2009-10-09T12:10:14Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"The 'patch' view is about generating text/plain patch that can be\ngiven to \"git am\", and \"git am\" doesn't understand merges anyway.\nTherefore link to 'patch' view should not be shown for merge commits.\n\nAlso call to git-format-patch inside the 'patch' action would fail\nwhen 'patch' action is called for a merge commit, with \"Reading\ngit-format-patch failed\" text as 'patch' view body.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nOn Thu, 1 Oct 2009, Jakub Narebski wrote:\n\n> I'll send v2 of this patch in a bit.\n\nHere it is.\n\nChanges since v1:\n * Do not show 'patch' link for merge commits not only in 'commit'\n   view, but also in 'commitdiff' view (more complete)\n * 'patch' link is shown also for root (parentless) commits; it\n   works correctly thanks to passing '--root' option to git-format-patch\n   (remove unnecessary restriction)\n * better commit message thanks to discussion with Jeff King\n\n gitweb/gitweb.perl |    6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 24b2193..14f31dc 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5328,7 +5328,7 @@ sub git_commit {\n \t\t\t} @$parents ) .\n \t\t\t')';\n \t}\n-\tif (gitweb_check_feature('patches')) {\n+\tif (gitweb_check_feature('patches') && @$parents <= 1) {\n \t\t$formats_nav .= \" | \" .\n \t\t\t$cgi->a({-href => href(action=>\"patch\", -replay=>1)},\n \t\t\t\t\"patch\");\n@@ -5616,7 +5616,7 @@ 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\tif ($patch_max &&  && @{$co{'parents'}} <= 1) {\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@@ -5824,7 +5824,7 @@ sub git_commitdiff_plain {\n \n # format-patch-style patches\n sub git_patch {\n-\tgit_commitdiff(-format => 'patch', -single=> 1);\n+\tgit_commitdiff(-format => 'patch', -single => 1);\n }\n \n sub git_patches {\n-- \n1.6.4.2\n"},{"id":"124480","messageId":"200910091423.51286.jnareb@gmail.com","threadId":"21101","inReplyTo":"200910091410.15904.jnareb@gmail.com","subject":"Re: [PATCH v2] gitweb: Do not show 'patch' link for merge commits","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-10-09T12:23:50Z","receivedAt":"2009-10-09T12:23:50Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Jakub Narebski wrote:\n\n> @@ -5616,7 +5616,7 @@ sub git_commitdiff {\n>                 $formats_nav =\n>                         $cgi->a({-href => href(action=>\"commitdiff_plain\", -replay=>1)},\n>                                 \"raw\");\n> -               if ($patch_max) {\n> +               if ($patch_max &&  && @{$co{'parents'}} <= 1) {\n\nGaaaah. It should be \"&&\", not \"&&  &&\".  I'm extremely sorry!\n\n>                         $formats_nav .= \" | \" .\n>                                 $cgi->a({-href => href(action=>\"patch\", -replay=>1)},\n>                                         \"patch\");\n\n-- \nJakub Narebski\nPoland\n"},{"id":"124481","messageId":"200910091426.44976.jnareb@gmail.com","threadId":"21101","inReplyTo":"200910091423.51286.jnareb@gmail.com","subject":"[PATCH v2 (amend)] gitweb: Do not show 'patch' link for merge commits","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-10-09T12:26:44Z","receivedAt":"2009-10-09T12:26:44Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"The 'patch' view is about generating text/plain patch that can be\ngiven to \"git am\", and \"git am\" doesn't understand merges anyway.\nTherefore link to 'patch' view should not be shown for merge commits.\n\nAlso call to git-format-patch inside the 'patch' action would fail\nwhen 'patch' action is called for a merge commit, with \"Reading\ngit-format-patch failed\" text as 'patch' view body.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nChanges since v1:\n * Do not show 'patch' link for merge commits not only in 'commit'\n   view, but also in 'commitdiff' view (more complete)\n * 'patch' link is shown also for root (parentless) commits; it\n   works correctly thanks to passing '--root' option to git-format-patch\n   (remove unnecessary restriction)\n * better commit message thanks to discussion with Jeff King\n\n gitweb/gitweb.perl |    6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 24b2193..14f31dc 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5328,7 +5328,7 @@ sub git_commit {\n \t\t\t} @$parents ) .\n \t\t\t')';\n \t}\n-\tif (gitweb_check_feature('patches')) {\n+\tif (gitweb_check_feature('patches') && @$parents <= 1) {\n \t\t$formats_nav .= \" | \" .\n \t\t\t$cgi->a({-href => href(action=>\"patch\", -replay=>1)},\n \t\t\t\t\"patch\");\n@@ -5616,7 +5616,7 @@ 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\tif ($patch_max && @{$co{'parents'}} <= 1) {\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@@ -5824,7 +5824,7 @@ sub git_commitdiff_plain {\n \n # format-patch-style patches\n sub git_patch {\n-\tgit_commitdiff(-format => 'patch', -single=> 1);\n+\tgit_commitdiff(-format => 'patch', -single => 1);\n }\n \n sub git_patches {\n-- \n1.6.4.2\n"},{"id":"124520","messageId":"20091009191930.GA14534@coredump.intra.peff.net","threadId":"21101","inReplyTo":"200910091426.44976.jnareb@gmail.com","subject":"Re: [PATCH v2 (amend)] gitweb: Do not show 'patch' link for merge commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-10-09T19:19:30Z","receivedAt":"2009-10-09T19:19:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 09, 2009 at 02:26:44PM +0200, Jakub Narebski wrote:\n\n> Changes since v1:\n>  * Do not show 'patch' link for merge commits not only in 'commit'\n>    view, but also in 'commitdiff' view (more complete)\n>  * 'patch' link is shown also for root (parentless) commits; it\n>    works correctly thanks to passing '--root' option to git-format-patch\n>    (remove unnecessary restriction)\n>  * better commit message thanks to discussion with Jeff King\n\nThanks. I am not a gitweb expert, but those changes look good to me.\n\n-Peff\n"},{"id":"124549","messageId":"7vhbu8f4me.fsf@alter.siamese.dyndns.org","threadId":"21101","inReplyTo":"200910091426.44976.jnareb@gmail.com","subject":"Re: [PATCH v2 (amend)] gitweb: Do not show 'patch' link for merge commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-10T00:56:57Z","receivedAt":"2009-10-10T00:56:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n"}]}