Jakub Narebski [off-list ref] 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.
quoted
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?
On Mon, 17 Oct 2011, Junio C Hamano wrote:
Jakub Narebski [off-list ref] writes:
quoted
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.
quoted
quoted
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
Simplify classification of diff line body in format_diff_line(),
replacing two if-elsif chains (one for ordinary diff and one for
combined diff of a merge commit) with regexp match. Refactor this
code into diff_line_class() function.
While at it:
* Fix an artifact in that $diff_class included leading space to be
able to compose classes like this "class=\"diff$diff_class\"', even
when $diff_class was an empty string. This made code unnecessary
ugly: $diff_class is now just class name or an empty string.
* Introduce "ctx" class for context lines ($diff_class was set to ""
in this case before this commit).
Idea and initial code by Junio C Hamano, polish and testing by Jakub
Narebski. Inspired by patch adding side-by-side diff by Kato Kazuyoshi,
which required $diff_class to be name of class without extra space.
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Jakub Narebski <redacted>
---
Junio C Hamano wrote:
Jakub Narebski [off-list ref] writes:
quoted
Kato Kazuyoshi wrote:
quoted
quoted
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?
How about this?
Kato Kazuyoshi, this should replace your
[PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class
gitweb/gitweb.perl | 67 ++++++++++++++++++++++++++++-----------------------
1 files changed, 37 insertions(+), 30 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index b0eaad7..b3284d4 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2247,40 +2247,47 @@ sub format_diff_cc_simplified {
return $result;
}
+sub diff_line_class {
+ my ($line, $from, $to) = @_;
+
+ # ordinary diff
+ my $num_sign = 1;
+ # combined diff
+ if ($from && $to && ref($from->{'href'}) eq "ARRAY") {
+ $num_sign = scalar @{$from->{'href'}};
+ }
+
+ my @diff_line_classifier = (
+ { regexp => qr/^\@\@{$num_sign} /, class => "chunk_header"},
+ { regexp => qr/^\\/, class => "incomplete" },
+ { regexp => qr/^ {$num_sign}/, class => "ctx" },
+ # classifier for context must come before classifier add/rem,
+ # or we would have to use more complicated regexp, for example
+ # qr/(?= {0,$m}\+)[+ ]{$num_sign}/, where $m = $num_sign - 1;
+ { regexp => qr/^[+ ]{$num_sign}/, class => "add" },
+ { regexp => qr/^[- ]{$num_sign}/, class => "rem" },
+ );
+ for my $clsfy (@diff_line_classifier) {
+ return $clsfy->{'class'}
+ if ($line =~ $clsfy->{'regexp'});
+ }
+
+ # fallback
+ return "";
+}
+
# format patch (diff) line (not to be used for diff headers)
sub format_diff_line {
my $line = shift;
my ($from, $to) = @_;
- my $diff_class = "";
- chomp $line;
+ my $diff_class = diff_line_class($line, $from, $to);
+ my $diff_classes = "diff";
+ $diff_classes .= " $diff_class" if ($diff_class);
- if ($from && $to && ref($from->{'href'}) eq "ARRAY") {
- # combined diff
- my $prefix = substr($line, 0, scalar @{$from->{'href'}});
- if ($line =~ m/^\@{3}/) {
- $diff_class = " chunk_header";
- } elsif ($line =~ m/^\\/) {
- $diff_class = " incomplete";
- } elsif ($prefix =~ tr/+/+/) {
- $diff_class = " add";
- } elsif ($prefix =~ tr/-/-/) {
- $diff_class = " rem";
- }
- } else {
- # assume ordinary diff
- my $char = substr($line, 0, 1);
- if ($char eq '+') {
- $diff_class = " add";
- } elsif ($char eq '-') {
- $diff_class = " rem";
- } elsif ($char eq '@') {
- $diff_class = " chunk_header";
- } elsif ($char eq "\\") {
- $diff_class = " incomplete";
- }
- }
+ chomp $line;
$line = untabify($line);
+
if ($from && $to && $line =~ m/^\@{2} /) {
my ($from_text, $from_start, $from_lines, $to_text, $to_start, $to_lines, $section) =
$line =~ m/^\@{2} (-(\d+)(?:,(\d+))?) (\+(\d+)(?:,(\d+))?) \@{2}(.*)$/;@@ -2298,7 +2305,7 @@ sub format_diff_line {
}
$line = "<span class=\"chunk_info\">@@ $from_text $to_text @@</span>" .
"<span class=\"section\">" . esc_html($section, -nbsp=>1) . "</span>";
- return "<div class=\"diff$diff_class\">$line</div>\n";
+ return "<div class=\"$diff_classes\">$line</div>\n";
} elsif ($from && $to && $line =~ m/^\@{3}/) {
my ($prefix, $ranges, $section) = $line =~ m/^(\@+) (.*?) \@+(.*)$/;
my (@from_text, @from_start, @from_nlines, $to_text, $to_start, $to_nlines);@@ -2331,9 +2338,9 @@ sub format_diff_line {
}
$line .= " $prefix</span>" .
"<span class=\"section\">" . esc_html($section, -nbsp=>1) . "</span>";
- return "<div class=\"diff$diff_class\">$line</div>\n";
+ return "<div class=\"$diff_classes\">$line</div>\n";
}
- return "<div class=\"diff$diff_class\">" . esc_html($line, -nbsp=>1) . "</div>\n";
+ return "<div class=\"$diff_classes\">" . esc_html($line, -nbsp=>1) . "</div>\n";
}
# Generates undef or something like "_snapshot_" or "snapshot (_tbz2_ _zip_)",--
1.7.6