Re: [PATCH v2 8/8] gitweb: Refinement highlightning in combined diffs
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Mar 29, 2012, 23:37 UTC
- Message-ID
- <201203300137.11932.jnareb@gmail.com>
- In-Reply-To
- <1332543417-19664-9-git-send-email-michal.kiedrowicz@gmail.com>
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.
Show 25 quoted lines
> > 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@gmail.com>
Acked-by: Jakub Narębski <jnareb@gmail.com>
> ---
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.
Show 10 quoted lines
> 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-highlightImplementation ^---
Show 10 quoted lines
> 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.
[...]
Show 39 quoted lines
> @@ -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.
Show 9 quoted lines
> +
> + 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;
> }[...]
Show 9 quoted lines
> @@ -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