{"thread":{"id":"20078","subject":"gitweb: buggy 'commitdiff_plain' output","startedAt":"2009-07-10T17:04:52Z","lastAt":"2009-07-10T21:50:13Z","messageCount":6,"participants":["Linus Torvalds","Jakub Narebski","Giuseppe Bilotta"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"117779","messageId":"alpine.LFD.2.01.0907100941060.3352@localhost.localdomain","threadId":"20078","inReplyTo":null,"subject":"gitweb: buggy 'commitdiff_plain' output","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-10T17:04:52Z","receivedAt":"2009-07-10T17:04:52Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nI complained to the CIFS people about their crazy duplicated commit \nmessage headers: see for example\n\n [torvalds@nehalem linux]$ git show --stat aeaaf253c4dee7ff9af2f3f0595f3bb66964e944\n commit aeaaf253c4dee7ff9af2f3f0595f3bb66964e944\n Author: Jeff Layton \n Date: Thu Jul 9 01:46:39 2009 -0400\n \n cifs: remove cifsInodeInfo->inUse counter\n \n cifs: remove cifsInodeInfo->inUse counter\n \n It was purported to be a refcounter of some sort, but was never\n used that way. It never served any purpose that wasn't served equally well\n by the I_NEW flag.\n \n Signed-off-by: Jeff Layton \n Reviewed-by: Christoph Hellwig \n Signed-off-by: Steve French \n \n fs/cifs/cifsfs.c | 1 -\n fs/cifs/cifsglob.h | 1 -\n 2 files changed, 0 insertions(+), 2 deletions(-)\n\nand note the duplicated \"cifs: remove cifsInodeInfo->inUse counter\" line.\n\nIt turns out that that duplication is because they use gitweb as a strange \npatch distribution system (rather than emailing each other patches), and \ndownload the 'commitdiff_plain' version of the diff and then apply it with \n'git am -s'.\n\nOk, so it's a really odd way of doing things, but hey, that gitweb feature \nclearly does try to support that exact workflow, or why would that \ncommitdiff_plain output try to look like an email?\n\nBut gitweb is a totally buggy piece of trash when it comes to exporting \ncommits that way.\n\nWhy? Because it first has a 'Subject:' line, and then the \"body\" of the \nemail repeats the raw commit message output. So _of_course_ you get the \nheader duplicated.\n\nNow, I asked Steve to not use gitweb (or edit the result some way), but \nthis really is a gitweb bug. And since I don't do perl, I can't fix it, \neven though I can pinpoint exactly where the bug is (lines 5732 - 5752 in \ngitweb/gitweb.perl).\n\nI totally untested patch written by a monkey who doesn't actually do perl \nis appended as a purely theoretical pointer in the right direction. But I \nreally have no clue about perl, so what the heck do I know? This is like \nmy tcl programming - pattern matching rather than any real understanding.\n\nI'm sure there are smarter ways to do this with some simple mapping \nfunction or whatever.\n\n\t\tLinus\n\n---\n gitweb/gitweb.perl | 7 ++++++-\n 1 files changed, 6 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 6a1b5b5..a809768 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5729,6 +5729,7 @@ sub git_commitdiff {\n \t\t}\n \n \t} elsif ($format eq 'plain') {\n+\t\tmy $show = false;\n \t\tmy $refs = git_get_references(\"tags\");\n \t\tmy $tagname = git_get_rev_name_tags($hash);\n \t\tmy $filename = basename($project) . \"-$hash.patch\";\n@@ -5747,7 +5748,11 @@ sub git_commitdiff {\n \t\tprint \"X-Git-Url: \" . $cgi->self_url() . \"\\n\\n\";\n \n \t\tforeach my $line (@{$co{'comment'}}) {\n-\t\t\tprint to_utf8($line) . \"\\n\";\n+\t\t\tif ($show) {\n+\t\t\t\tprint to_utf8($line) . \"\\n\";\n+\t\t\t} else if ($line ne \"\") {\n+\t\t\t\t$show = true;\n+\t\t\t}\n \t\t}\n \t\tprint \"---\\n\\n\";\n \t} elsif ($format eq 'patch') {\n"},{"id":"117786","messageId":"200907101933.39424.jnareb@gmail.com","threadId":"20078","inReplyTo":"alpine.LFD.2.01.0907100941060.3352@localhost.localdomain","subject":"Re: gitweb: buggy 'commitdiff_plain' output","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-07-10T17:33:38Z","receivedAt":"2009-07-10T17:33:38Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 10 July 2009, Linus Torvalds wrote:\n> \n> I complained to the CIFS people about their crazy duplicated commit \n> message headers: see for example\n[...]\n\n> It turns out that that duplication is because they use gitweb as a strange \n> patch distribution system (rather than emailing each other patches), and \n> download the 'commitdiff_plain' version of the diff and then apply it with \n> 'git am -s'.\n\nFirst question: do they use gitweb from git.git repository, or a custom\nfork of gitweb (like git.kernel.org gitweb, which has caching, but IIRC\ndoes not have all new gitweb features)?\n\n> Ok, so it's a really odd way of doing things, but hey, that gitweb feature \n> clearly does try to support that exact workflow, or why would that \n> commitdiff_plain output try to look like an email?\n> \n> But gitweb is a totally buggy piece of trash when it comes to exporting \n> commits that way.\n\nActually the problem is deeper; both 'commitdiff' and 'commitdiff_plain'\ntry to do two things at once: an equivalent of \"git show \" _and_ \nequivalent of \"git diff \"... and they fail.\n\n> \n> Why? Because it first has a 'Subject:' line, and then the \"body\" of the \n> email repeats the raw commit message output. So _of_course_ you get the \n> header duplicated.\n\nYes, this is bug in 'commitdiff_plain' output. Perhaps it should mimic\n'--pretty=email' format better. Perhaps it shouldn't mimic email format\nat all, but be more like \"git show \".\n\nTHE SOLUTION is to use 'patch' or 'patches' view, introduced by \nGiuseppe Bilotta on 18 Dec 2008 in series of commits from 9872cd6f to\n75bf2cb2. It uses git-format-patch, and can be fed directly to git-am.\nProblem solved.\n\n> Now, I asked Steve to not use gitweb (or edit the result some way), but \n> this really is a gitweb bug. And since I don't do perl, I can't fix it, \n> even though I can pinpoint exactly where the bug is (lines 5732 - 5752 in \n> gitweb/gitweb.perl).\n> \n> I totally untested patch written by a monkey who doesn't actually do perl \n> is appended as a purely theoretical pointer in the right direction. But I \n> really have no clue about perl, so what the heck do I know? This is like \n> my tcl programming - pattern matching rather than any real understanding.\n> \n> I'm sure there are smarter ways to do this with some simple mapping \n> function or whatever.\n> \n> \t\tLinus\n\nI'd have to examine closer what git_commitdiff_plain does, and what\nparse_commit_text does to extract subject from commit message.\nWith the caveat that perhaps it would be better to get rid of mail-like\n'commitdiff_plain' and mimic git-show output...\n \n> ---\n> gitweb/gitweb.perl | 7 ++++++-\n> 1 files changed, 6 insertions(+), 1 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 6a1b5b5..a809768 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -5729,6 +5729,7 @@ sub git_commitdiff {\n> \t\t}\n> \n> \t} elsif ($format eq 'plain') {\n> +\t\tmy $show = false;\n> \t\tmy $refs = git_get_references(\"tags\");\n> \t\tmy $tagname = git_get_rev_name_tags($hash);\n> \t\tmy $filename = basename($project) . \"-$hash.patch\";\n> @@ -5747,7 +5748,11 @@ sub git_commitdiff {\n> \t\tprint \"X-Git-Url: \" . $cgi->self_url() . \"\\n\\n\";\n> \n> \t\tforeach my $line (@{$co{'comment'}}) {\n> -\t\t\tprint to_utf8($line) . \"\\n\";\n> +\t\t\tif ($show) {\n> +\t\t\t\tprint to_utf8($line) . \"\\n\";\n> +\t\t\t} else if ($line ne \"\") {\n> +\t\t\t\t$show = true;\n> +\t\t\t}\n> \t\t}\n> \t\tprint \"---\\n\\n\";\n> \t} elsif ($format eq 'patch') {\n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"117787","messageId":"cb7bb73a0907101034v3e6d655btd28018d24ef6de63@mail.gmail.com","threadId":"20078","inReplyTo":"alpine.LFD.2.01.0907100941060.3352@localhost.localdomain","subject":"Re: gitweb: buggy 'commitdiff_plain' output","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-07-10T17:34:11Z","receivedAt":"2009-07-10T17:34:11Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Fri, Jul 10, 2009 at 7:04 PM, Linus\nTorvalds wrote:\n>\n> I complained to the CIFS people about their crazy duplicated commit\n> message headers: see for example\n>\n>    [torvalds@nehalem linux]$ git show --stat aeaaf253c4dee7ff9af2f3f0595f3bb66964e944\n>    commit aeaaf253c4dee7ff9af2f3f0595f3bb66964e944\n>    Author: Jeff Layton \n>    Date:   Thu Jul 9 01:46:39 2009 -0400\n>\n>        cifs: remove cifsInodeInfo->inUse counter\n>\n>        cifs: remove cifsInodeInfo->inUse counter\n>\n>        It was purported to be a refcounter of some sort, but was never\n>        used that way. It never served any purpose that wasn't served equally well\n>        by the I_NEW flag.\n>\n>        Signed-off-by: Jeff Layton \n>        Reviewed-by: Christoph Hellwig \n>        Signed-off-by: Steve French \n>\n>     fs/cifs/cifsfs.c   |    1 -\n>     fs/cifs/cifsglob.h |    1 -\n>     2 files changed, 0 insertions(+), 2 deletions(-)\n>\n> and note the duplicated \"cifs: remove cifsInodeInfo->inUse counter\" line.\n>\n> It turns out that that duplication is because they use gitweb as a strange\n> patch distribution system (rather than emailing each other patches), and\n> download the 'commitdiff_plain' version of the diff and then apply it with\n> 'git am -s'.\n>\n> Ok, so it's a really odd way of doing things, but hey, that gitweb feature\n> clearly does try to support that exact workflow, or why would that\n> commitdiff_plain output try to look like an email?\n>\n> But gitweb is a totally buggy piece of trash when it comes to exporting\n> commits that way.\n>\n> Why? Because it first has a 'Subject:' line, and then the \"body\" of the\n> email repeats the raw commit message output. So _of_course_ you get the\n> header duplicated.\n>\n> Now, I asked Steve to not use gitweb (or edit the result some way), but\n> this really is a gitweb bug. And since I don't do perl, I can't fix it,\n> even though I can pinpoint exactly where the bug is (lines 5732 - 5752 in\n> gitweb/gitweb.perl).\n>\n> I totally untested patch written by a monkey who doesn't actually do perl\n> is appended as a purely theoretical pointer in the right direction. But I\n> really have no clue about perl, so what the heck do I know? This is like\n> my tcl programming - pattern matching rather than any real understanding.\n>\n> I'm sure there are smarter ways to do this with some simple mapping\n> function or whatever.\n\nThe smarter way is called the 'patches' ('patch' for single ones, e.g.\nhttp://git.example.com/project.git/patch/master, whereas /patches/\nwould display the latest N with N configurable) view that just spouts\nout the git format-patch output in a git-am friendly way. I'm pretty\nsure my patches to implement that view are already upstream ... yes,\nyes it is:\n\nhttp://git.oblomov.eu/git/patches/9872cd..75bf2cb2\n\nTo understand the differences between this and commitdiff_plain, see\n\nhttp://git.oblomov.eu/git/commitdiff_plain/9872cd..75bf2cb2\n\nfor comparison.\n\nAnd yes, commitdiff is just TOTALLY not what you expect it to be,\nESPECIALLY with multiple commits, where it only displays the commit\nmessage for the FIRST commit.\n\nSo tell the CIFS people to use the patch(es) view if they plan to use\ngitweb for patch exchange (which I do all the time, and in fact was\nthe reason why I implemented this view in the first place).\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117788","messageId":"cb7bb73a0907101038t10494806k64380a5e33f1bc16@mail.gmail.com","threadId":"20078","inReplyTo":"200907101933.39424.jnareb@gmail.com","subject":"Re: gitweb: buggy 'commitdiff_plain' output","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-07-10T17:38:11Z","receivedAt":"2009-07-10T17:38:11Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Fri, Jul 10, 2009 at 7:33 PM, Jakub Narebski wrote:\n> Actually the problem is deeper; both 'commitdiff' and 'commitdiff_plain'\n> try to do two things at once: an equivalent of \"git show \" _and_\n> equivalent of \"git diff \"... and they fail.\n\nI think the time might have come to break backwards compatibility and\nfix commitdiff view once and for all ... of course we would have to\nchoose HOW to fix it ...\n\nFor single commits, we could have commitdiff = commit + diff and\ncommitdiff_plain = patch.\n\nFor multiple commits, we could have commitdiff(_plain) output a list\nof subjects for each commit involved, + the output of git diff.\n\nHow does that sound? (read?)\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117798","messageId":"alpine.LFD.2.01.0907101405480.3352@localhost.localdomain","threadId":"20078","inReplyTo":"200907101933.39424.jnareb@gmail.com","subject":"Re: gitweb: buggy 'commitdiff_plain' output","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-10T21:05:56Z","receivedAt":"2009-07-10T21:05:56Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 10 Jul 2009, Jakub Narebski wrote:\n\n> On Fri, 10 July 2009, Linus Torvalds wrote:\n> > \n> > It turns out that that duplication is because they use gitweb as a strange \n> > patch distribution system (rather than emailing each other patches), and \n> > download the 'commitdiff_plain' version of the diff and then apply it with \n> > 'git am -s'.\n> \n> First question: do they use gitweb from git.git repository, or a custom\n> fork of gitweb (like git.kernel.org gitweb, which has caching, but IIRC\n> does not have all new gitweb features)?\n\nI have no idea. Afaik, it was on samba.org:\n\n\thttp://git.samba.org/?p=jlayton/cifs.git;a=commitdiff_plain;h=acc11a88a4cb4ba16777099da00664347e0683f0\n\nand I have no clue whether samba org uses plain git gitweb or something \nfancier.\n\n\t\tLinus\n"},{"id":"117799","messageId":"200907102350.15511.jnareb@gmail.com","threadId":"20078","inReplyTo":"alpine.LFD.2.01.0907101405480.3352@localhost.localdomain","subject":"Re: gitweb: buggy 'commitdiff_plain' output","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-07-10T21:50:13Z","receivedAt":"2009-07-10T21:50:13Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 10 July 2009, Linus Torvalds wrote:\n> On Fri, 10 Jul 2009, Jakub Narebski wrote:\n>> On Fri, 10 July 2009, Linus Torvalds wrote:\n>>> \n>>> It turns out that that duplication is because they use gitweb as a strange \n>>> patch distribution system (rather than emailing each other patches), and \n>>> download the 'commitdiff_plain' version of the diff and then apply it with \n>>> 'git am -s'.\n>> \n>> First question: do they use gitweb from git.git repository, or a custom\n>> fork of gitweb (like git.kernel.org gitweb, which has caching, but IIRC\n>> does not have all new gitweb features)?\n> \n> I have no idea. Afaik, it was on samba.org:\n> \n> \thttp://git.samba.org/?p=jlayton/cifs.git;a=commitdiff_plain;h=acc11a88a4cb4ba16777099da00664347e0683f0\n> \n> and I have no clue whether samba org uses plain git gitweb or something \n> fancier.\n\nThanks. It looks on first glance like ordinary gitweb, only old. It is\ngit and gitweb version 1.5.4.3 from 23 Feb 2008, while Giuseppe's patches\nmade it into git in v1.6.1 or v1.6.2 (commit is from 23 Dec 2008).\n\n-- \nJakub Narebski\nPoland\n"}]}