Re: [PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 17, 2011, 21:22 UTC
Message-ID: <7v62jnl3ef.fsf@alter.siamese.dyndns.org>
In-Reply-To: <m38voj72xy.fsf@localhost.localdomain>

Jakub Narebski <jnareb@gmail.com> writes:

> By the way, it is common to either have following patches to be chain
> reply to first patch, or better provide cover letter for patch series
> and have all patches be reply to cover letter.

It may be a good discipline for 14 patch series, but it doesn't matter for something this small.

Show 26 quoted lines

>>  gitweb/gitweb.perl |   24 +++++++++++++-----------
>>  1 files changed, 13 insertions(+), 11 deletions(-)
>> 
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 85d64b2..095adda 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -2235,28 +2235,30 @@ sub format_diff_line {
>>  		# combined diff
>>  		my $prefix = substr($line, 0, scalar @{$from->{'href'}});
>>  		if ($line =~ m/^\@{3}/) {
>> -			$diff_class = " chunk_header";
>> +			$diff_class = "chunk_header";
>>  		} elsif ($line =~ m/^\\/) {
>> -			$diff_class = " incomplete";
>> +			$diff_class = "incomplete";
>>  		} elsif ($prefix =~ tr/+/+/) {
>> -			$diff_class = " add";
>> +			$diff_class = "add";
>>  		} elsif ($prefix =~ tr/-/-/) {
>> -			$diff_class = " rem";
>> +			$diff_class = "rem";
>>  		}
>
> Hmmm... that reminds me: this if-elsif chain is a bit ugly, but would
> be hard to replace without given ... when, I think.

I wasn't reading the existing context line, but now that you mention it, they are not just ugly but are borderline of being incorrect (e.g. it does not catch a broken hunk-header that begins with "@@@@" for a two-way merge).

Assuming that $from->{'href'} has all the parents (i.e. 2 for two-way merge), shouldn't the code be more like this?

	# combined diff
        my $num_sign = @{$from->{'href'}} + 1;
	my @cc_classifier = (
		["\@{$num_sign} ", "chunk_header"],
                ['\\', "incomplete"],
                [" {$num_sign}", ""],
                ["[+ ]{$num_sign}", "add"],
                ["[- ]{$num_sign}", "rem"],
        );
        for my $cls (@cc_classifier) {
                if ($line =~ /^$cls->[0]$/) {
                	$diff_class = $cls->[1];
                        last;
		}
	}

Also don't we want to use "context" or something for the css class for the context lines, instead of assuming that we won't want to paint it in any special color?