From: Jakub Narebski <jnareb@gmail.com>
Date: Fri, 10 Jul 2009 17:33:38 GMT
Subject: Re: gitweb: buggy 'commitdiff_plain' output
Message-ID: <200907101933.39424.jnareb@gmail.com>
In-Reply-To: <alpine.LFD.2.01.0907100941060.3352@localhost.localdomain>

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...
 
> ---
>  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') {
> 

-- 
Jakub Narebski
Poland