From: Jakub Narebski <jnareb@gmail•com>
To: "Michał Kiedrowicz" <michal.kiedrowicz@gmail•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH 8/8] gitweb: Highlight combined diffs
Date: Sat, 11 Feb 2012 16:03:19 -0800 (PST) [thread overview]
Message-ID: <m3ty2wsyt6.fsf@localhost.localdomain> (raw)
In-Reply-To: <1328865494-24415-9-git-send-email-michal.kiedrowicz@gmail.com>
Michał Kiedrowicz <michal.kiedrowicz@gmail•com> writes:
> 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.
>
> 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.
>
> 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
>
That is a very reasonable approach.
> Further changes may introduce more intelligent approach that better
> handles combined diffs.
>
> Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail•com>
> ---
> gitweb/gitweb.perl | 40 +++++++++++++++++++++++++++++++++++++---
> 1 files changed, 37 insertions(+), 3 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 1a5b454..2b6cb9e 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -4944,13 +4944,16 @@ sub esc_html_mark_range {
>
> # Format removed and added line, mark changed part and HTML-format them.
> sub format_rem_add_line {
> - my ($rem, $add) = @_;
> + my ($rem, $add, $is_combined) = @_;
> my @r = split(//, $rem);
> my @a = split(//, $add);
> my ($esc_rem, $esc_add);
> my ($prefix, $suffix_rem, $suffix_add) = (1, $#r, $#a);
> my ($prefix_is_space, $suffix_is_space) = (1, 1);
>
> + # In combined diff we must ignore two +/- characters.
> + $prefix = 2 if ($is_combined);
> +
Errr... actually number of prefix is equalt to number of parents, so
it might be in case of octopus merge more than 2.
> while ($prefix < @r && $prefix < @a) {
> last if ($r[$prefix] ne $a[$prefix]);
>
> @@ -4988,11 +4991,42 @@ sub format_ctx_rem_add_lines {
> my ($ctx, $rem, $add, $is_combined) = @_;
> my (@new_ctx, @new_rem, @new_add);
> my $num_add_lines = @$add;
> + my $can_highlight;
> +
> + # Highlight if every removed line has a corresponding added line.
> + if ($num_add_lines > 0 && $num_add_lines == @$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 < $num_add_lines; $i++) {
> + my $prefix_rem = substr($rem->[$i], 0, 2);
> + my $prefix_add = substr($add->[$i], 0, 2);
> +
> + $prefix_rem =~ s/-/+/g;
> +
> + if ($prefix_rem ne $prefix_add) {
> + $can_highlight = 0;
> + last;
Nb. this assumes that we cannot refine and highlight something like
this:
# - a
# - b
# + c
# ++ d
But perhaps this is better left for future improvemnt.
> + }
> + }
> + }
> + } else {
> + $can_highlight = 0;
> + }
This 'else' would be not necessary if $can_highlight was initialized
to 0.
>
> - if (!$is_combined && $num_add_lines > 0 && $num_add_lines == @$rem) {
> + if ($can_highlight) {
> for (my $i = 0; $i < $num_add_lines; $i++) {
> my ($line_rem, $line_add) = format_rem_add_line(
> - $rem->[$i], $add->[$i]);
> + $rem->[$i], $add->[$i], $is_combined);
> push @new_rem, $line_rem;
> push @new_add, $line_add;
O.K.
--
Jakub Narębski
next prev parent reply other threads:[~2012-02-12 0:03 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-02-10 9:18 [PATCH 0/8] gitweb: Highlight interesting parts of diff Michał Kiedrowicz
2012-02-10 9:18 ` [PATCH 1/8] gitweb: Extract print_sidebyside_diff_lines() Michał Kiedrowicz
2012-02-11 15:20 ` Jakub Narebski
2012-02-11 23:03 ` Michał Kiedrowicz
2012-02-10 9:18 ` [PATCH 2/8] gitweb: Use print_diff_chunk() for both side-by-side and inline diffs Michał Kiedrowicz
2012-02-11 15:53 ` Jakub Narebski
2012-02-11 23:16 ` Michał Kiedrowicz
2012-02-25 9:00 ` Michał Kiedrowicz
2012-02-10 9:18 ` [PATCH 3/8] gitweb: Move HTML-formatting diff line back to process_diff_line() Michał Kiedrowicz
2012-02-11 16:02 ` Jakub Narebski
2012-02-10 9:18 ` [PATCH 4/8] gitweb: Push formatting diff lines to print_diff_chunk() Michał Kiedrowicz
2012-02-11 16:29 ` Jakub Narebski
2012-02-11 23:20 ` Michał Kiedrowicz
2012-02-11 23:30 ` Michał Kiedrowicz
2012-02-10 9:18 ` [PATCH 5/8] gitweb: Format diff lines just before printing Michał Kiedrowicz
2012-02-11 17:14 ` Jakub Narebski
2012-02-11 23:38 ` Michał Kiedrowicz
2012-02-10 9:18 ` [PATCH 6/8] gitweb: Highlight interesting parts of diff Michał Kiedrowicz
2012-02-10 13:23 ` Jakub Narebski
2012-02-10 14:15 ` Michał Kiedrowicz
2012-02-10 14:55 ` Jakub Narebski
2012-02-10 17:33 ` Michał Kiedrowicz
2012-02-10 22:52 ` Splitting gitweb (was: Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff) Jakub Narebski
2012-02-10 20:24 ` [PATCH 6/8] gitweb: Highlight interesting parts of diff Jeff King
2012-02-14 6:54 ` Michal Kiedrowicz
2012-02-14 7:14 ` Junio C Hamano
2012-02-14 8:20 ` Jeff King
2012-02-10 20:20 ` Jeff King
2012-02-10 21:29 ` Michał Kiedrowicz
2012-02-10 21:32 ` Jeff King
2012-02-10 21:36 ` Michał Kiedrowicz
2012-02-10 21:47 ` [PATCH] diff-highlight: Work for multiline changes too Michał Kiedrowicz
2012-02-13 22:27 ` Jeff King
2012-02-13 22:28 ` [PATCH 1/5] diff-highlight: make perl strict and warnings fatal Jeff King
2012-02-13 22:32 ` [PATCH 2/5] diff-highlight: don't highlight whole lines Jeff King
2012-02-14 6:35 ` Michal Kiedrowicz
2012-02-13 22:33 ` [PATCH 3/5] diff-highlight: refactor to prepare for multi-line hunks Jeff King
2012-02-13 22:36 ` [PATCH 4/5] diff-highlight: match " Jeff King
2012-02-13 22:37 ` [PATCH 5/5] diff-highlight: document some non-optimal cases Jeff King
2012-02-14 6:48 ` Michal Kiedrowicz
2012-02-14 0:05 ` [PATCH] diff-highlight: Work for multiline changes too Junio C Hamano
2012-02-14 0:22 ` Jeff King
2012-02-14 1:19 ` Junio C Hamano
2012-02-14 6:04 ` Jeff King
2012-02-14 6:28 ` Michal Kiedrowicz
2012-02-10 21:56 ` [PATCH 6/8] gitweb: Highlight interesting parts of diff Jakub Narebski
2012-02-11 23:45 ` Jakub Narebski
2012-02-12 10:42 ` Jakub Narebski
2012-02-13 6:54 ` Michal Kiedrowicz
2012-02-13 19:58 ` Jakub Narebski
2012-02-13 21:10 ` Michał Kiedrowicz
2012-02-13 6:41 ` Michal Kiedrowicz
2012-02-13 18:44 ` Jakub Narebski
2012-02-13 21:09 ` Michał Kiedrowicz
2012-02-14 17:31 ` Jakub Narebski
2012-02-14 18:23 ` Michał Kiedrowicz
2012-02-14 18:52 ` Jeff King
2012-02-14 20:04 ` Michał Kiedrowicz
2012-02-14 20:38 ` Jeff King
2012-02-10 9:18 ` [PATCH 7/8] gitweb: Use different colors to present marked changes Michał Kiedrowicz
2012-02-12 0:11 ` Jakub Narebski
2012-02-13 6:46 ` Michal Kiedrowicz
2012-02-10 9:18 ` [PATCH 8/8] gitweb: Highlight combined diffs Michał Kiedrowicz
2012-02-12 0:03 ` Jakub Narebski [this message]
2012-02-13 6:48 ` Michal Kiedrowicz
2012-02-11 18:32 ` [PATCH 0/8] gitweb: Highlight interesting parts of diff Jakub Narebski
2012-02-11 22:56 ` Michał Kiedrowicz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=m3ty2wsyt6.fsf@localhost.localdomain \
--to=jnareb@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=michal.kiedrowicz@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox