gitweb: buggy 'commitdiff_plain' output

I complained to the CIFS people about their crazy duplicated commit 
message headers: see for example

    [torvalds@nehalem linux]$ git show --stat aeaaf253c4dee7ff9af2f3f0595f3bb66964e944
    commit aeaaf253c4dee7ff9af2f3f0595f3bb66964e944
    Author: Jeff Layton [off-list ref]
    Date:   Thu Jul 9 01:46:39 2009 -0400
    
        cifs: remove cifsInodeInfo->inUse counter
        
        cifs: remove cifsInodeInfo->inUse counter
        
        It was purported to be a refcounter of some sort, but was never
        used that way. It never served any purpose that wasn't served equally well
        by the I_NEW flag.
        
        Signed-off-by: Jeff Layton [off-list ref]
        Reviewed-by: Christoph Hellwig [off-list ref]
        Signed-off-by: Steve French [off-list ref]
    
     fs/cifs/cifsfs.c   |    1 -
     fs/cifs/cifsglob.h |    1 -
     2 files changed, 0 insertions(+), 2 deletions(-)

and note the duplicated "cifs: remove cifsInodeInfo->inUse counter" line.

It turns out that that duplication is because they use gitweb as a strange 
patch distribution system (rather than emailing each other patches), and 
download the 'commitdiff_plain' version of the diff and then apply it with 
'git am -s'.

Ok, so it's a really odd way of doing things, but hey, that gitweb feature 
clearly does try to support that exact workflow, or why would that 
commitdiff_plain output try to look like an email?

But gitweb is a totally buggy piece of trash when it comes to exporting 
commits that way.

Why? Because it first has a 'Subject:' line, and then the "body" of the 
email repeats the raw commit message output. So _of_course_ you get the 
header duplicated.

Now, I asked Steve to not use gitweb (or edit the result some way), but 
this really is a gitweb bug. And since I don't do perl, I can't fix it, 
even though I can pinpoint exactly where the bug is (lines 5732 - 5752 in 
gitweb/gitweb.perl).

I totally untested patch written by a monkey who doesn't actually do perl 
is appended as a purely theoretical pointer in the right direction. But I 
really have no clue about perl, so what the heck do I know? This is like 
my tcl programming - pattern matching rather than any real understanding.

I'm sure there are smarter ways to do this with some simple mapping 
function or whatever.

		Linus

---
 gitweb/gitweb.perl |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 6a1b5b5..a809768 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -5729,6 +5729,7 @@ sub git_commitdiff {
 		}
 
 	} elsif ($format eq 'plain') {
+		my $show = false;
 		my $refs = git_get_references("tags");
 		my $tagname = git_get_rev_name_tags($hash);
 		my $filename = basename($project) . "-$hash.patch";

@@ -5747,7 +5748,11 @@ sub git_commitdiff {
 		print "X-Git-Url: " . $cgi->self_url() . "\n\n";
 
 		foreach my $line (@{$co{'comment'}}) {
-			print to_utf8($line) . "\n";
+			if ($show) {
+				print to_utf8($line) . "\n";
+			} else if ($line ne "") {
+				$show = true;
+			}
 		}
 		print "---\n\n";
 	} elsif ($format eq 'patch') {

On Fri, 10 July 2009, Linus Torvalds wrote:

I complained to the CIFS people about their crazy duplicated commit 
message headers: see for example

[...]

It turns out that that duplication is because they use gitweb as a strange 
patch distribution system (rather than emailing each other patches), and 
download the 'commitdiff_plain' version of the diff and then apply it with 
'git am -s'.

First question: do they use gitweb from git.git repository, or a custom
fork of gitweb (like git.kernel.org gitweb, which has caching, but IIRC
does not have all new gitweb features)?

Ok, so it's a really odd way of doing things, but hey, that gitweb feature 
clearly does try to support that exact workflow, or why would that 
commitdiff_plain output try to look like an email?

But gitweb is a totally buggy piece of trash when it comes to exporting 
commits that way.

Actually the problem is deeper; both 'commitdiff' and 'commitdiff_plain'
try to do two things at once: an equivalent of "git show <commit>" _and_ 
equivalent of "git diff <commit 1> <commit 2>"... and they fail.

Why? Because it first has a 'Subject:' line, and then the "body" of the 
email repeats the raw commit message output. So _of_course_ you get the 
header duplicated.

Yes, this is bug in 'commitdiff_plain' output.  Perhaps it should mimic
'--pretty=email' format better.  Perhaps it shouldn't mimic email format
at all, but be more like "git show <commit>".

THE SOLUTION is to use 'patch' or 'patches' view, introduced by 
Giuseppe Bilotta on 18 Dec 2008 in series of commits from 9872cd6f to
75bf2cb2.  It uses git-format-patch, and can be fed directly to git-am.
Problem solved.

Now, I asked Steve to not use gitweb (or edit the result some way), but 
this really is a gitweb bug. And since I don't do perl, I can't fix it, 
even though I can pinpoint exactly where the bug is (lines 5732 - 5752 in 
gitweb/gitweb.perl).

I totally untested patch written by a monkey who doesn't actually do perl 
is appended as a purely theoretical pointer in the right direction. But I 
really have no clue about perl, so what the heck do I know? This is like 
my tcl programming - pattern matching rather than any real understanding.

I'm sure there are smarter ways to do this with some simple mapping 
function or whatever.

		Linus

I'd have to examine closer what git_commitdiff_plain does, and what
parse_commit_text does to extract subject from commit message.
With the caveat that perhaps it would be better to get rid of mail-like
'commitdiff_plain' and mimic git-show output...
 

-- 
Jakub Narebski
Poland

On Fri, Jul 10, 2009 at 7:04 PM, Linus
Torvalds[off-list ref] wrote:

I complained to the CIFS people about their crazy duplicated commit
message headers: see for example

   [torvalds@nehalem linux]$ git show --stat aeaaf253c4dee7ff9af2f3f0595f3bb66964e944
   commit aeaaf253c4dee7ff9af2f3f0595f3bb66964e944
   Author: Jeff Layton [off-list ref]
   Date:   Thu Jul 9 01:46:39 2009 -0400

       cifs: remove cifsInodeInfo->inUse counter

       cifs: remove cifsInodeInfo->inUse counter

       It was purported to be a refcounter of some sort, but was never
       used that way. It never served any purpose that wasn't served equally well
       by the I_NEW flag.

       Signed-off-by: Jeff Layton [off-list ref]
       Reviewed-by: Christoph Hellwig [off-list ref]
       Signed-off-by: Steve French [off-list ref]

    fs/cifs/cifsfs.c   |    1 -
    fs/cifs/cifsglob.h |    1 -
    2 files changed, 0 insertions(+), 2 deletions(-)

and note the duplicated "cifs: remove cifsInodeInfo->inUse counter" line.

It turns out that that duplication is because they use gitweb as a strange
patch distribution system (rather than emailing each other patches), and
download the 'commitdiff_plain' version of the diff and then apply it with
'git am -s'.

Ok, so it's a really odd way of doing things, but hey, that gitweb feature
clearly does try to support that exact workflow, or why would that
commitdiff_plain output try to look like an email?

But gitweb is a totally buggy piece of trash when it comes to exporting
commits that way.

Why? Because it first has a 'Subject:' line, and then the "body" of the
email repeats the raw commit message output. So _of_course_ you get the
header duplicated.

Now, I asked Steve to not use gitweb (or edit the result some way), but
this really is a gitweb bug. And since I don't do perl, I can't fix it,
even though I can pinpoint exactly where the bug is (lines 5732 - 5752 in
gitweb/gitweb.perl).

I totally untested patch written by a monkey who doesn't actually do perl
is appended as a purely theoretical pointer in the right direction. But I
really have no clue about perl, so what the heck do I know? This is like
my tcl programming - pattern matching rather than any real understanding.

I'm sure there are smarter ways to do this with some simple mapping
function or whatever.

The smarter way is called the 'patches' ('patch' for single ones, e.g.
http://git.example.com/project.git/patch/master, whereas /patches/
would display the latest N with N configurable) view that just spouts
out the git format-patch output in a git-am friendly way. I'm pretty
sure my patches to implement that view are already upstream ... yes,
yes it is:

http://git.oblomov.eu/git/patches/9872cd..75bf2cb2

To understand the differences between this and commitdiff_plain, see

http://git.oblomov.eu/git/commitdiff_plain/9872cd..75bf2cb2

for comparison.

And yes, commitdiff is just TOTALLY not what you expect it to be,
ESPECIALLY with multiple commits, where it only displays the commit
message for the FIRST commit.

So tell the CIFS people to use the patch(es) view if they plan to use
gitweb for patch exchange (which I do all the time, and in fact was
the reason why I implemented this view in the first place).

-- 
Giuseppe "Oblomov" Bilotta

On Fri, Jul 10, 2009 at 7:33 PM, Jakub Narebski[off-list ref] wrote:

Actually the problem is deeper; both 'commitdiff' and 'commitdiff_plain'
try to do two things at once: an equivalent of "git show <commit>" _and_
equivalent of "git diff <commit 1> <commit 2>"... and they fail.

I think the time might have come to break backwards compatibility and
fix commitdiff view once and for all ... of course we would have to
choose HOW to fix it ...

For single commits, we could have commitdiff = commit + diff and
commitdiff_plain = patch.

For multiple commits, we could have commitdiff(_plain) output a list
of subjects for each commit involved, + the output of git diff.

How does that sound? (read?)

-- 
Giuseppe "Oblomov" Bilotta

On Fri, 10 Jul 2009, Jakub Narebski wrote:

On Fri, 10 July 2009, Linus Torvalds wrote:

quoted

It turns out that that duplication is because they use gitweb as a strange 
patch distribution system (rather than emailing each other patches), and 
download the 'commitdiff_plain' version of the diff and then apply it with 
'git am -s'.

First question: do they use gitweb from git.git repository, or a custom
fork of gitweb (like git.kernel.org gitweb, which has caching, but IIRC
does not have all new gitweb features)?

I have no idea. Afaik, it was on samba.org:

	http://git.samba.org/?p=jlayton/cifs.git;a=commitdiff_plain;h=acc11a88a4cb4ba16777099da00664347e0683f0

and I have no clue whether samba org uses plain git gitweb or something 
fancier.

		Linus

On Fri, 10 July 2009, Linus Torvalds wrote:

On Fri, 10 Jul 2009, Jakub Narebski wrote:

quoted

On Fri, 10 July 2009, Linus Torvalds wrote:

quoted

It turns out that that duplication is because they use gitweb as a strange 
patch distribution system (rather than emailing each other patches), and 
download the 'commitdiff_plain' version of the diff and then apply it with 
'git am -s'.

First question: do they use gitweb from git.git repository, or a custom
fork of gitweb (like git.kernel.org gitweb, which has caching, but IIRC
does not have all new gitweb features)?

I have no idea. Afaik, it was on samba.org:

	http://git.samba.org/?p=jlayton/cifs.git;a=commitdiff_plain;h=acc11a88a4cb4ba16777099da00664347e0683f0

and I have no clue whether samba org uses plain git gitweb or something 
fancier.

Thanks. It looks on first glance like ordinary gitweb, only old.  It is
git and gitweb version 1.5.4.3 from 23 Feb 2008, while Giuseppe's patches
made it into git in v1.6.1 or v1.6.2 (commit is from 23 Dec 2008).

-- 
Jakub Narebski
Poland