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

[Jakub Narebski <jnareb@gmail.com>]

On Mon, 17 Oct 2011, Junio C Hamano wrote:
> 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.

Well, cover letter for 2-patch series might be overkill, but having
patches in series connected e.g. chain-replied-to, or all replies to
first patch in series, is IMHO a good idea.

>>>  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

Wouldn't that cover not only combined diff, but an "ordinary" diff as
well then (i.e. comment should change)?  I think that was the intent,
isn't it?

>       my $num_sign = @{$from->{'href'}} + 1;

$from->{'href'} is array reference only for combined diff (>= 2 parents).
For ordinary diff it is a scalar.

> 	my @cc_classifier = (
> 		  ["\@{$num_sign} ", "chunk_header"],

O.K.

Nb., it is "chunk_header", or "hunk_header"?

>                 ['\\', "incomplete"],

O.K.

>                 [" {$num_sign}", ""],

O.K.

>                 ["[+ ]{$num_sign}", "add"],
>                 ["[- ]{$num_sign}", "rem"],

It would be slightly different to what current code does.

Current code for combined diff uses "add" if there is at least one '+',
"rem" if there are no '+' and at least one '-', and context otherwise.


I wonder if with sufficiently evil merge we can have a line that is
added (changed) in some children, and removed in other, i.e. pluses
and minuses combined.

Nb. we can put regexp here, not only stringification of regexp.
i.e.

                  [qr/[+ ]{$num_sign}/, "add"],
                  [qr/[- ]{$num_sign}/, "rem"],


>         );
>         for my $cls (@cc_classifier) {
>                 if ($line =~ /^$cls->[0]$/) {
>                 	$diff_class = $cls->[1];
>                         last;
> 		}
> 	}

Nice trick.
 
> 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?

Right.  We use "diff" class without anything else for context, but probably
it would be better to state this explicitly.

-- 
Jakub Narebski
Poland