Re: [PATCH v2 8/8] gitweb: Refinement highlightning in combined diffs
From: Jakub Narebski <hidden>
Date: 2016-06-15 22:53:25
On Fri, 23 Mar 2012, Michał Kiedrowicz wrote:
The highlightning of combined diffs is currently disabled. This is because output from a combined diff is much harder to highlight because it's not obvious which removed and added lines should be compared.
Is -> was?
Moreover, code that compares added and removed lines doesn't care about combined diffs. It only skips first +/- character, treating second +/- as a line content.
Well, we explicitly skip combined diffs. I think what you want to say here is that it is not possible to simply use existing algorithm unchanged for combined diffs.
Let's start with a simple case: only highlight changes that come from one parent, i.e. when every removed line has a corresponding added line for the same parent. This way the highlightning cannot get wrong. For example, following diffs would be highlighted: - removed line for first parent + added line for first parent context line -removed line for second parent +added line for second parent or - removed line for first parent -removed line for second parent + added line for first parent +added line for second parent but following output will not: - removed line for first parent -removed line for second parent +added line for second parent ++added line for both parents
O.K., that's a nice and sensible first step. I wonder if it would be worth to specify that we currently require that pattern of '-'-es in pre-image match pattern of '+'-es in postimage. Nb. the prefix of combined diff would either include '+', or '-', but never mixed (this is documented, but I had trouble with this).
Further changes may introduce more intelligent approach that better handles combined diffs.
Very sensible approach.
Signed-off-by: Michał Kiedrowicz <redacted>
Acked-by: Jakub Narębski [off-list ref]
---
BTW. I went and checked if this approach helps for non-trivial merges in git.git history: * b10656c - helps a bit, though one can see limitation of pre/post-fix matching here, but it is present also for non-combined diff. * 8b132bc - helps a bit, though char-interdiff or word-interdiff might be better. Nb. I think that red background for 'marked' is a bit too dark (intensive). * c58499c - doesn't help too much. * f629c23, aa145bf - helps.
quoted hunk ↗ jump to hunk
gitweb/gitweb.perl | 57 +++++++++++++++++++++++++++++++++++++++------------ 1 files changed, 43 insertions(+), 14 deletions(-)diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 872ba12..c056e83 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl@@ -5057,12 +5057,12 @@ sub print_inline_diff_lines { # Format removed and added line, mark changed part and HTML-format them. # Impementation is based on contrib/diff-highlight
Implementation ^---
sub format_rem_add_line {
- my ($rem, $add) = @_;
+ my ($rem, $add, $num_parents) = @_;
my @rem = split(//, $rem);
my @add = split(//, $add);
my ($esc_rem, $esc_add);
- # Ignore +/- character, thus $prefix_len is set to 1.
- my ($prefix_len, $suffix_len) = (1, 0);
+ # Ignore leading +/- characters for each parent.
+ my ($prefix_len, $suffix_len) = ($num_parents, 0);Nice. [...]
quoted hunk ↗ jump to hunk
@@ -5099,15 +5099,43 @@ sub format_rem_add_line { # HTML-format diff context, removed and added lines. sub format_ctx_rem_add_lines { - my ($ctx, $rem, $add, $is_combined) = @_; + my ($ctx, $rem, $add, $num_parents) = @_; my (@new_ctx, @new_rem, @new_add); + my $can_highlight = 0; + my $is_combined = ($num_parents > 1); # Highlight if every removed line has a corresponding added line. - # Combined diffs are not supported ATM. - if (!$is_combined && @$add > 0 && @$add == @$rem) { + if (@$add > 0 && @$add == @$rem) { + $can_highlight = 1; + + # Highlight lines in combined diff only if the chunk contains + # diff between the same version, e.g. + # + # - a + # - b + # + c + # + d + # + # Otherwise the highlightling would be confusing. + if ($is_combined) { + for (my $i = 0; $i < @$add; $i++) { + my $prefix_rem = substr($rem->[$i], 0, $num_parents); + my $prefix_add = substr($add->[$i], 0, $num_parents); + + $prefix_rem =~ s/-/+/g; + + if ($prefix_rem ne $prefix_add) { + $can_highlight = 0; + last; + } + } + } + }
Good.
+
+ if ($can_highlight) {
for (my $i = 0; $i < @$add; $i++) {
my ($line_rem, $line_add) = format_rem_add_line(
- $rem->[$i], $add->[$i]);
+ $rem->[$i], $add->[$i], $num_parents);
push @new_rem, $line_rem;
push @new_add, $line_add;
}[...]
quoted hunk ↗ jump to hunk
@@ -5326,7 +5355,7 @@ sub git_patchset_body { } continue { if (@chunk) { - print_diff_chunk($diff_style, $is_combined, \%from, \%to, @chunk); + print_diff_chunk($diff_style, scalar @hash_parents, \%from, \%to, @chunk); @chunk = (); } print "</div>\n"; # class="patch"
I was wondering about 'commitdiff' between two commits, which is not combined even ifany of those commits is a merge commit... but it looks like it works all right. -- Jakub Narebski Poland