On Mon, 17 Oct 2011, Junio C Hamano wrote:
> Jakub Narebski <jnareb@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html