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 <michal.kiedrowicz@xxxxxxxxx>
Acked-by: Jakub Narębski <jnareb@xxxxxxxxx>
> ---
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.
> 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.
[...]
> @@ -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;
> }
[...]
> @@ -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
--
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