{"thread":{"id":"29593","subject":"[PATCH 3/8] gitweb: Move HTML-formatting diff line back to process_diff_line()","startedAt":"2012-02-10T09:18:06Z","lastAt":"2012-02-25T09:00:19Z","messageCount":67,"participants":["Michał Kiedrowicz","Jakub Narebski","Jeff King","Michal Kiedrowicz","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"184338","messageId":"1328865494-24415-1-git-send-email-michal.kiedrowicz@gmail.com","threadId":"29593","inReplyTo":null,"subject":"[PATCH 0/8] gitweb: Highlight interesting parts of diff","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-10T09:18:06Z","receivedAt":"2012-02-10T09:18:06Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Reading diff output is sometimes very hard, even if it's colored,\nespecially if lines differ only in few characters. This is often true when\na commit fixes a typo or renames some variables or functions.\n\nThis patch series teaches gitweb to highlight fragments that are different\nbetween old and new line. This should mimic the same feature in Trac or\nGitHub.\n\nTo achieve that, added and removed lines must be accumulated and compared.\nThe current code in print_sidebyside_diff_chunk() already does this, so\nthis patch series reuses it in commits:\n\n gitweb: Extract print_sidebyside_diff_lines()\n gitweb: Use print_diff_chunk() for both side-by-side and inline diffs\n\nNext, HTML-formatting diff lines is pushed down to the place where they\nare about to be printed. This is required because comparision must be\nperformened on raw git-diff output and not on HTML-formatted lines. This\nis done in commits:\n\n gitweb: Move HTML-formatting diff line back to process_diff_line()\n gitweb: Push formatting diff lines to print_diff_chunk()\n gitweb: Format diff lines just before printing\n\nThe last three commits implement the advertised feature. They could be\nsquashed together but that would make them harder to review (I think).\n\n gitweb: Highlight interesting parts of diff\n gitweb: Use different colors to present marked changes\n gitweb: Highlight combined diffs\n\nThis series is based on v1.7.9.\n\nMichał Kiedrowicz (8):\n gitweb: Extract print_sidebyside_diff_lines()\n gitweb: Use print_diff_chunk() for both side-by-side and inline diffs\n gitweb: Move HTML-formatting diff line back to process_diff_line()\n gitweb: Push formatting diff lines to print_diff_chunk()\n gitweb: Format diff lines just before printing\n gitweb: Highlight interesting parts of diff\n gitweb: Use different colors to present marked changes\n gitweb: Highlight combined diffs\n\n gitweb/gitweb.perl | 304 +++++++++++++++++++++++++++++++++++-----------\n gitweb/static/gitweb.css | 8 ++\n 2 files changed, 238 insertions(+), 74 deletions(-)\n\n-- \n1.7.3.4\n"},{"id":"184339","messageId":"1328865494-24415-2-git-send-email-michal.kiedrowicz@gmail.com","threadId":"29593","inReplyTo":"1328865494-24415-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH 1/8] gitweb: Extract print_sidebyside_diff_lines()","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-10T09:18:07Z","receivedAt":"2012-02-10T09:18:07Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Currently, print_sidebyside_diff_chunk() does two things: it\naccumulates diff lines and prints them. Accumulation may be used to\nperform additional operations on diff lines, so it makes sense to split\nthese two things. Thus, the code that prints diff lines in a side-by-side\nmanner is moved out of print_sidebyside_diff_chunk() to a separate\nsubroutine.\n\nThe outcome of this patch is that print_sidebyside_diff_chunk() is now\nmuch shorter and easier to read.\n\nThis change is meant to be a simple code movement. No behavior change is\nintended.\n\nSigned-off-by: Michał Kiedrowicz \n---\n gitweb/gitweb.perl | 97 ++++++++++++++++++++++++++++------------------------\n 1 files changed, 52 insertions(+), 45 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex abb5a79..1247607 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -4861,6 +4861,53 @@ sub git_difftree_body {\n \tprint \"\\n\";\n }\n \n+# Print context lines and then rem/add lines in a side-by-side manner.\n+sub print_sidebyside_diff_lines {\n+\tmy ($ctx, $rem, $add) = @_;\n+\n+\t# print context block before add/rem block\n+\tif (@$ctx) {\n+\t\tprint join '',\n+\t\t\t'
',\n+\t\t\t\t'
',\n+\t\t\t\t@$ctx,\n+\t\t\t\t'
',\n+\t\t\t\t'
',\n+\t\t\t\t@$ctx,\n+\t\t\t\t'
',\n+\t\t\t'
';\n+\t}\n+\n+\tif (!@$add) {\n+\t\t# pure removal\n+\t\tprint join '',\n+\t\t\t'
',\n+\t\t\t\t'
',\n+\t\t\t\t@$rem,\n+\t\t\t\t'
',\n+\t\t\t'
';\n+\t} elsif (!@$rem) {\n+\t\t# pure addition\n+\t\tprint join '',\n+\t\t\t'
',\n+\t\t\t\t'
',\n+\t\t\t\t@$add,\n+\t\t\t\t'
',\n+\t\t\t'
';\n+\t} else {\n+\t\t# assume that it is change\n+\t\tprint join '',\n+\t\t\t'
',\n+\t\t\t\t'
',\n+\t\t\t\t@$rem,\n+\t\t\t\t'
',\n+\t\t\t\t'
',\n+\t\t\t\t@$add,\n+\t\t\t\t'
',\n+\t\t\t'
';\n+\t}\n+}\n+\n sub print_sidebyside_diff_chunk {\n \tmy @chunk = @_;\n \tmy (@ctx, @rem, @add);\n@@ -4887,51 +4934,11 @@ sub print_sidebyside_diff_chunk {\n \t\t\tnext;\n \t\t}\n \n-\t\t## print from accumulator when type of class of lines change\n-\t\t# empty contents block on start rem/add block, or end of chunk\n-\t\tif (@ctx && (!$class || $class eq 'rem' || $class eq 'add')) {\n-\t\t\tprint join '',\n-\t\t\t\t'
',\n-\t\t\t\t\t'
',\n-\t\t\t\t\t@ctx,\n-\t\t\t\t\t'
',\n-\t\t\t\t\t'
',\n-\t\t\t\t\t@ctx,\n-\t\t\t\t\t'
',\n-\t\t\t\t'
';\n-\t\t\t@ctx = ();\n-\t\t}\n-\t\t# empty add/rem block on start context block, or end of chunk\n-\t\tif ((@rem || @add) && (!$class || $class eq 'ctx')) {\n-\t\t\tif (!@add) {\n-\t\t\t\t# pure removal\n-\t\t\t\tprint join '',\n-\t\t\t\t\t'
',\n-\t\t\t\t\t\t'
',\n-\t\t\t\t\t\t@rem,\n-\t\t\t\t\t\t'
',\n-\t\t\t\t\t'
';\n-\t\t\t} elsif (!@rem) {\n-\t\t\t\t# pure addition\n-\t\t\t\tprint join '',\n-\t\t\t\t\t'
',\n-\t\t\t\t\t\t'
',\n-\t\t\t\t\t\t@add,\n-\t\t\t\t\t\t'
',\n-\t\t\t\t\t'
';\n-\t\t\t} else {\n-\t\t\t\t# assume that it is change\n-\t\t\t\tprint join '',\n-\t\t\t\t\t'
',\n-\t\t\t\t\t\t'
',\n-\t\t\t\t\t\t@rem,\n-\t\t\t\t\t\t'
',\n-\t\t\t\t\t\t'
',\n-\t\t\t\t\t\t@add,\n-\t\t\t\t\t\t'
',\n-\t\t\t\t\t'
';\n-\t\t\t}\n-\t\t\t@rem = @add = ();\n+\t\t## print from accumulator when have some add/rem lines or end\n+\t\t# of chunk (flush context lines)\n+\t\tif (((@rem || @add) && $class eq 'ctx') || !$class) {\n+\t\t\tprint_sidebyside_diff_lines(\\@ctx, \\@rem, \\@add);\n+\t\t\t@ctx = @rem = @add = ();\n \t\t}\n \n \t\t## adding lines to accumulator\n-- \n1.7.3.4\n"},{"id":"184335","messageId":"1328865494-24415-3-git-send-email-michal.kiedrowicz@gmail.com","threadId":"29593","inReplyTo":"1328865494-24415-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH 2/8] gitweb: Use print_diff_chunk() for both side-by-side and inline diffs","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-10T09:18:08Z","receivedAt":"2012-02-10T09:18:08Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"This renames print_sidebyside_diff_chunk() to print_diff_chunk() and\nmakes use of it for both side-by-side and inline diffs. Now diff lines\nare always accumulated before they are printed. This opens the\npossibility to preprocess diff output before it's printed.\n\nThe new function print_inline_diff_lines() could reorder diff lines. It\nfirst prints all context lines, then all removed lines and finally all\nadded lines. If the diff output consisted of mixed added and removed\nlines, gitweb would reorder these lines. This is especially true for\ncombined diff output, for example:\n\n\t - removed line for first parent\n\t + added line for first parent\n\t -removed line for second parent\n\t ++added line for both parents\n\nwould be rendered as:\n\n\t- removed line for first parent\n\t -removed line for second parent\n\t+ added line for first parent\n\t++added line for both parents\n\nTo prevent gitweb from reordering lines, print_diff_chunk() calls\nprint_diff_lines() as soon as it detects that both added and removed\nlines are present and there was a class change.\n\nSigned-off-by: Michał Kiedrowicz \n---\n gitweb/gitweb.perl | 53 +++++++++++++++++++++++++++++++++++++--------------\n 1 files changed, 38 insertions(+), 15 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 1247607..ed63261 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -4908,9 +4908,31 @@ sub print_sidebyside_diff_lines {\n \t}\n }\n \n-sub print_sidebyside_diff_chunk {\n-\tmy @chunk = @_;\n+# Print context lines and then rem/add lines in inline manner.\n+sub print_inline_diff_lines {\n+\tmy ($ctx, $rem, $add) = @_;\n+\n+\tprint foreach (@$ctx);\n+\tprint foreach (@$rem);\n+\tprint foreach (@$add);\n+}\n+\n+# Print context lines and then rem/add lines.\n+sub print_diff_lines {\n+\tmy ($ctx, $rem, $add, $diff_style, $is_combined) = @_;\n+\n+\tif ($diff_style eq 'sidebyside' && !$is_combined) {\n+\t\tprint_sidebyside_diff_lines($ctx, $rem, $add);\n+\t} else {\n+\t\t# default 'inline' style and unknown styles\n+\t\tprint_inline_diff_lines($ctx, $rem, $add);\n+\t}\n+}\n+\n+sub print_diff_chunk {\n+\tmy ($diff_style, $is_combined, @chunk) = @_;\n \tmy (@ctx, @rem, @add);\n+\tmy $prev_class = '';\n \n \treturn unless @chunk;\n \n@@ -4935,9 +4957,13 @@ sub print_sidebyside_diff_chunk {\n \t\t}\n \n \t\t## print from accumulator when have some add/rem lines or end\n-\t\t# of chunk (flush context lines)\n-\t\tif (((@rem || @add) && $class eq 'ctx') || !$class) {\n-\t\t\tprint_sidebyside_diff_lines(\\@ctx, \\@rem, \\@add);\n+\t\t# of chunk (flush context lines), or when have add and rem\n+\t\t# lines and new block is reached (otherwise add/rem lines could\n+\t\t# be reordered)\n+\t\tif (((@rem || @add) && $class eq 'ctx') || !$class ||\n+\t\t\t\t(@rem && @add && $class ne $prev_class)) {\n+\t\t\tprint_diff_lines(\\@ctx, \\@rem, \\@add, $diff_style,\n+\t\t\t\t\t$is_combined);\n \t\t\t@ctx = @rem = @add = ();\n \t\t}\n \n@@ -4954,6 +4980,8 @@ sub print_sidebyside_diff_chunk {\n \t\tif ($class eq 'ctx') {\n \t\t\tpush @ctx, $line;\n \t\t}\n+\n+\t\t$prev_class = $class;\n \t}\n }\n \n@@ -5080,22 +5108,17 @@ sub git_patchset_body {\n \t\t\t$diff_classes .= \" $class\" if ($class);\n \t\t\t$line = \"
$line
\\n\";\n \n-\t\t\tif ($diff_style eq 'sidebyside' && !$is_combined) {\n-\t\t\t\tif ($class eq 'chunk_header') {\n-\t\t\t\t\tprint_sidebyside_diff_chunk(@chunk);\n-\t\t\t\t\t@chunk = ( [ $class, $line ] );\n-\t\t\t\t} else {\n-\t\t\t\t\tpush @chunk, [ $class, $line ];\n-\t\t\t\t}\n+\t\t\tif ($class eq 'chunk_header') {\n+\t\t\t\tprint_diff_chunk($diff_style, $is_combined, @chunk);\n+\t\t\t\t@chunk = ( [ $class, $line ] );\n \t\t\t} else {\n-\t\t\t\t# default 'inline' style and unknown styles\n-\t\t\t\tprint $line;\n+\t\t\t\tpush @chunk, [ $class, $line ];\n \t\t\t}\n \t\t}\n \n \t} continue {\n \t\tif (@chunk) {\n-\t\t\tprint_sidebyside_diff_chunk(@chunk);\n+\t\t\tprint_diff_chunk($diff_style, $is_combined, @chunk);\n \t\t\t@chunk = ();\n \t\t}\n \t\tprint \"\\n\"; # class=\"patch\"\n-- \n1.7.3.4\n"},{"id":"184331","messageId":"1328865494-24415-4-git-send-email-michal.kiedrowicz@gmail.com","threadId":"29593","inReplyTo":"1328865494-24415-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH 3/8] gitweb: Move HTML-formatting diff line back to process_diff_line()","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-10T09:18:09Z","receivedAt":"2012-02-10T09:18:09Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Commit 6ba1eb51b (gitweb: Add a feature to show side-by-side diff,\n2011-10-31) for no special reason moved wrapping diff line in
out\nof format_diff_line(). Bring back old behavior.\n\nThis simplifies code in git_patchset_body() and keeps formatting of a\ndiff line in one place.\n\nThe more long-term purpose of this patch is to move formatting diff\nlines down to print_diff_chunk(), to allow processing lines without\nHTML-formatting.\n\nThis is just a refactoring patch. It's not meant to change gitweb\noutput.\n\nSigned-off-by: Michał Kiedrowicz \n---\n gitweb/gitweb.perl | 15 ++++++++-------\n 1 files changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex ed63261..d2f75c4 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2332,14 +2332,18 @@ sub process_diff_line {\n \n \tif ($from && $to && $line =~ m/^\\@{2} /) {\n \t\t$line = format_unidiff_chunk_header($line, $from, $to);\n-\t\treturn $diff_class, $line;\n \n \t} elsif ($from && $to && $line =~ m/^\\@{3}/) {\n \t\t$line = format_cc_diff_chunk_header($line, $from, $to);\n-\t\treturn $diff_class, $line;\n-\n+\t} else {\n+\t\t$line = esc_html($line, -nbsp=>1);\n \t}\n-\treturn $diff_class, esc_html($line, -nbsp=>1);\n+\n+\tmy $diff_classes = \"diff\";\n+\t$diff_classes .= \" $diff_class\" if ($diff_class);\n+\t$line = \"
$line
\\n\";\n+\n+\treturn $diff_class, $line;\n }\n \n # Generates undef or something like \"_snapshot_\" or \"snapshot (_tbz2_ _zip_)\",\n@@ -5104,9 +5108,6 @@ sub git_patchset_body {\n \t\t\tnext PATCH if ($patch_line =~ m/^diff /);\n \n \t\t\tmy ($class, $line) = process_diff_line($patch_line, \\%from, \\%to);\n-\t\t\tmy $diff_classes = \"diff\";\n-\t\t\t$diff_classes .= \" $class\" if ($class);\n-\t\t\t$line = \"
$line
\\n\";\n \n \t\t\tif ($class eq 'chunk_header') {\n \t\t\t\tprint_diff_chunk($diff_style, $is_combined, @chunk);\n-- \n1.7.3.4\n"},{"id":"184332","messageId":"1328865494-24415-5-git-send-email-michal.kiedrowicz@gmail.com","threadId":"29593","inReplyTo":"1328865494-24415-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH 4/8] gitweb: Push formatting diff lines to print_diff_chunk()","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-10T09:18:10Z","receivedAt":"2012-02-10T09:18:10Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Now git_patchset_body() only calls diff_line_class() (removed from\nprocess_diff_line()). The latter function is renamed to\nformat_diff_line() and is called from print_diff_chunk().\n\nThis is a pure code movement, needed for processing raw diff lines in\nthe accumulator in print_diff_chunk(). No behavior change is intended by\nthis change.\n\nSigned-off-by: Michał Kiedrowicz \n---\n gitweb/gitweb.perl | 25 ++++++++++++-------------\n 1 files changed, 12 insertions(+), 13 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex d2f75c4..cae9dfa 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2320,12 +2320,9 @@ sub format_cc_diff_chunk_header {\n }\n \n # process patch (diff) line (not to be used for diff headers),\n-# returning class and HTML-formatted (but not wrapped) line\n-sub process_diff_line {\n-\tmy $line = shift;\n-\tmy ($from, $to) = @_;\n-\n-\tmy $diff_class = diff_line_class($line, $from, $to);\n+# returning HTML-formatted (but not wrapped) line\n+sub format_diff_line {\n+\tmy ($line, $diff_class, $from, $to) = @_;\n \n \tchomp $line;\n \t$line = untabify($line);\n@@ -2343,7 +2340,7 @@ sub process_diff_line {\n \t$diff_classes .= \" $diff_class\" if ($diff_class);\n \t$line = \"
$line
\\n\";\n \n-\treturn $diff_class, $line;\n+\treturn $line;\n }\n \n # Generates undef or something like \"_snapshot_\" or \"snapshot (_tbz2_ _zip_)\",\n@@ -4934,7 +4931,7 @@ sub print_diff_lines {\n }\n \n sub print_diff_chunk {\n-\tmy ($diff_style, $is_combined, @chunk) = @_;\n+\tmy ($diff_style, $is_combined, $from, $to, @chunk) = @_;\n \tmy (@ctx, @rem, @add);\n \tmy $prev_class = '';\n \n@@ -4954,6 +4951,8 @@ sub print_diff_chunk {\n \tforeach my $line_info (@chunk) {\n \t\tmy ($class, $line) = @$line_info;\n \n+\t\t$line = format_diff_line($line, $class, $from, $to);\n+\n \t\t# print chunk headers\n \t\tif ($class && $class eq 'chunk_header') {\n \t\t\tprint $line;\n@@ -5107,19 +5106,19 @@ sub git_patchset_body {\n \n \t\t\tnext PATCH if ($patch_line =~ m/^diff /);\n \n-\t\t\tmy ($class, $line) = process_diff_line($patch_line, \\%from, \\%to);\n+\t\t\tmy $class = diff_line_class($patch_line, \\%from, \\%to);\n \n \t\t\tif ($class eq 'chunk_header') {\n-\t\t\t\tprint_diff_chunk($diff_style, $is_combined, @chunk);\n-\t\t\t\t@chunk = ( [ $class, $line ] );\n+\t\t\t\tprint_diff_chunk($diff_style, $is_combined, \\%from, \\%to, @chunk);\n+\t\t\t\t@chunk = ( [ $class, $patch_line ] );\n \t\t\t} else {\n-\t\t\t\tpush @chunk, [ $class, $line ];\n+\t\t\t\tpush @chunk, [ $class, $patch_line ];\n \t\t\t}\n \t\t}\n \n \t} continue {\n \t\tif (@chunk) {\n-\t\t\tprint_diff_chunk($diff_style, $is_combined, @chunk);\n+\t\t\tprint_diff_chunk($diff_style, $is_combined, \\%from, \\%to, @chunk);\n \t\t\t@chunk = ();\n \t\t}\n \t\tprint \"
\\n\"; # class=\"patch\"\n-- \n1.7.3.4\n"},{"id":"184334","messageId":"1328865494-24415-6-git-send-email-michal.kiedrowicz@gmail.com","threadId":"29593","inReplyTo":"1328865494-24415-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH 5/8] gitweb: Format diff lines just before printing","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-10T09:18:11Z","receivedAt":"2012-02-10T09:18:11Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Now we're ready to insert highlightning to diff output.\n\nThe call to untabify() remains in the main loop in print_diff_chunk().\nThe motivation is that it must be called before any call to esc_html()\n(because that converts spaces to  ) and to call it only once.\n\nThis is a refactoring patch. It's not meant to change gitweb output.\n\nSigned-off-by: Michał Kiedrowicz \n---\n gitweb/gitweb.perl | 42 +++++++++++++++++++++++++++++++-----------\n 1 files changed, 31 insertions(+), 11 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex cae9dfa..db61553 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2319,13 +2319,22 @@ sub format_cc_diff_chunk_header {\n \treturn $line;\n }\n \n-# process patch (diff) line (not to be used for diff headers),\n-# returning HTML-formatted (but not wrapped) line\n+# wrap patch (diff) line into a
(not to be used for diff headers),\n+# the line must be esc_html()'ed\n sub format_diff_line {\n \tmy ($line, $diff_class, $from, $to) = @_;\n \n-\tchomp $line;\n-\t$line = untabify($line);\n+\tmy $diff_classes = \"diff\";\n+\t$diff_classes .= \" $diff_class\" if ($diff_class);\n+\t$line = \"
$line
\\n\";\n+\n+\treturn $line;\n+}\n+\n+# format diff chunk header line (not to be used for diff headers),\n+# returning HTML-formatted line\n+sub format_diff_chunk_header {\n+\tmy ($line, $diff_class, $from, $to) = @_;\n \n \tif ($from && $to && $line =~ m/^\\@{2} /) {\n \t\t$line = format_unidiff_chunk_header($line, $from, $to);\n@@ -2336,11 +2345,7 @@ sub format_diff_line {\n \t\t$line = esc_html($line, -nbsp=>1);\n \t}\n \n-\tmy $diff_classes = \"diff\";\n-\t$diff_classes .= \" $diff_class\" if ($diff_class);\n-\t$line = \"
$line
\\n\";\n-\n-\treturn $line;\n+\treturn format_diff_line($line, $diff_class);\n }\n \n # Generates undef or something like \"_snapshot_\" or \"snapshot (_tbz2_ _zip_)\",\n@@ -4918,10 +4923,24 @@ sub print_inline_diff_lines {\n \tprint foreach (@$add);\n }\n \n+# HTML-format diff context, removed and added lines.\n+sub format_ctx_rem_add_lines {\n+\tmy ($ctx, $rem, $add) = @_;\n+\tmy (@new_ctx, @new_rem, @new_add);\n+\n+\t@new_ctx = map { format_diff_line(esc_html($_, -nbsp=>1), 'ctx') } @$ctx;\n+\t@new_rem = map { format_diff_line(esc_html($_, -nbsp=>1), 'rem') } @$rem;\n+\t@new_add = map { format_diff_line(esc_html($_, -nbsp=>1), 'add') } @$add;\n+\n+\treturn (\\@new_ctx, \\@new_rem, \\@new_add);\n+}\n+\n # Print context lines and then rem/add lines.\n sub print_diff_lines {\n \tmy ($ctx, $rem, $add, $diff_style, $is_combined) = @_;\n \n+\t($ctx, $rem, $add) = format_ctx_rem_add_lines($ctx, $rem, $add);\n+\n \tif ($diff_style eq 'sidebyside' && !$is_combined) {\n \t\tprint_sidebyside_diff_lines($ctx, $rem, $add);\n \t} else {\n@@ -4951,11 +4970,12 @@ sub print_diff_chunk {\n \tforeach my $line_info (@chunk) {\n \t\tmy ($class, $line) = @$line_info;\n \n-\t\t$line = format_diff_line($line, $class, $from, $to);\n+\t\t$line = untabify($line);\n \n \t\t# print chunk headers\n \t\tif ($class && $class eq 'chunk_header') {\n-\t\t\tprint $line;\n+\t\t\tprint format_diff_chunk_header($line, $class, $from,\n+\t\t\t\t$to);\n \t\t\tnext;\n \t\t}\n \n-- \n1.7.3.4\n"},{"id":"184333","messageId":"1328865494-24415-7-git-send-email-michal.kiedrowicz@gmail.com","threadId":"29593","inReplyTo":"1328865494-24415-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-10T09:18:12Z","receivedAt":"2012-02-10T09:18:12Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Reading diff output is sometimes very hard, even if it's colored,\nespecially if lines differ only in few characters. This is often true\nwhen a commit fixes a typo or renames some variables or functions.\n\nThis commit teaches gitweb to highlight characters that are different\nbetween old and new line. This should work in the similar manner as in\nTrac or GitHub.\n\nThe code that comares lines is based on\ncontrib/diff-highlight/diff-highlight, except that it works with\nmultiline changes too. It also won't highlight lines that are\ncompletely different because that would only make the output unreadable.\nCombined diffs are not supported but a following commit will change it.\n\nSigned-off-by: Michał Kiedrowicz \n---\n gitweb/gitweb.perl | 82 ++++++++++++++++++++++++++++++++++++++++++++++++---\n 1 files changed, 77 insertions(+), 5 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex db61553..1a5b454 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2322,7 +2322,7 @@ sub format_cc_diff_chunk_header {\n # wrap patch (diff) line into a
(not to be used for diff headers),\n # the line must be esc_html()'ed\n sub format_diff_line {\n-\tmy ($line, $diff_class, $from, $to) = @_;\n+\tmy ($line, $diff_class) = @_;\n \n \tmy $diff_classes = \"diff\";\n \t$diff_classes .= \" $diff_class\" if ($diff_class);\n@@ -4923,14 +4923,85 @@ sub print_inline_diff_lines {\n \tprint foreach (@$add);\n }\n \n+# Highlight characters from $prefix to $suffix and escape HTML.\n+# $str is a reference to the array of characters.\n+sub esc_html_mark_range {\n+\tmy ($str, $prefix, $suffix) = @_;\n+\n+\t# Don't generate empty element.\n+\tif ($prefix == $suffix + 1) {\n+\t\treturn esc_html(join('', @$str), -nbsp=>1);\n+\t}\n+\n+\tmy $before = join('', @{$str}[0..($prefix - 1)]);\n+\tmy $marked = join('', @{$str}[$prefix..$suffix]);\n+\tmy $after = join('', @{$str}[($suffix + 1)..$#{$str}]);\n+\n+\treturn esc_html($before, -nbsp=>1) .\n+\t\t$cgi->span({-class=>'marked'}, esc_html($marked, -nbsp=>1)) .\n+\t\tesc_html($after,-nbsp=>1);\n+}\n+\n+# Format removed and added line, mark changed part and HTML-format them.\n+sub format_rem_add_line {\n+\tmy ($rem, $add) = @_;\n+\tmy @r = split(//, $rem);\n+\tmy @a = split(//, $add);\n+\tmy ($esc_rem, $esc_add);\n+\tmy ($prefix, $suffix_rem, $suffix_add) = (1, $#r, $#a);\n+\tmy ($prefix_is_space, $suffix_is_space) = (1, 1);\n+\n+\twhile ($prefix < @r && $prefix < @a) {\n+\t\tlast if ($r[$prefix] ne $a[$prefix]);\n+\n+\t\t$prefix_is_space = 0 if ($r[$prefix] !~ /\\s/);\n+\t\t$prefix++;\n+\t}\n+\n+\twhile ($suffix_rem >= $prefix && $suffix_add >= $prefix) {\n+\t\tlast if ($r[$suffix_rem] ne $a[$suffix_add]);\n+\n+\t\t$suffix_is_space = 0 if ($r[$suffix_rem] !~ /\\s/);\n+\t\t$suffix_rem--;\n+\t\t$suffix_add--;\n+\t}\n+\n+\t# Mark lines that are different from each other, but have some common\n+\t# part that isn't whitespace. If lines are completely different, don't\n+\t# mark them because that would make output unreadable, especially if\n+\t# diff consists of multiple lines.\n+\tif (($prefix == 1 && $suffix_rem == $#r && $suffix_add == $#a)\n+\t\t|| ($prefix_is_space && $suffix_is_space)) {\n+\t\t$esc_rem = esc_html($rem);\n+\t\t$esc_add = esc_html($add);\n+\t} else {\n+\t\t$esc_rem = esc_html_mark_range(\\@r, $prefix, $suffix_rem);\n+\t\t$esc_add = esc_html_mark_range(\\@a, $prefix, $suffix_add);\n+\t}\n+\n+\treturn format_diff_line($esc_rem, 'rem'),\n+\t\tformat_diff_line($esc_add, 'add');\n+}\n+\n # HTML-format diff context, removed and added lines.\n sub format_ctx_rem_add_lines {\n-\tmy ($ctx, $rem, $add) = @_;\n+\tmy ($ctx, $rem, $add, $is_combined) = @_;\n \tmy (@new_ctx, @new_rem, @new_add);\n+\tmy $num_add_lines = @$add;\n+\n+\tif (!$is_combined && $num_add_lines > 0 && $num_add_lines == @$rem) {\n+\t\tfor (my $i = 0; $i < $num_add_lines; $i++) {\n+\t\t\tmy ($line_rem, $line_add) = format_rem_add_line(\n+\t\t\t\t$rem->[$i], $add->[$i]);\n+\t\t\tpush @new_rem, $line_rem;\n+\t\t\tpush @new_add, $line_add;\n+\t\t}\n+\t} else {\n+\t\t@new_rem = map { format_diff_line(esc_html($_, -nbsp=>1), 'rem') } @$rem;\n+\t\t@new_add = map { format_diff_line(esc_html($_, -nbsp=>1), 'add') } @$add;\n+\t}\n \n \t@new_ctx = map { format_diff_line(esc_html($_, -nbsp=>1), 'ctx') } @$ctx;\n-\t@new_rem = map { format_diff_line(esc_html($_, -nbsp=>1), 'rem') } @$rem;\n-\t@new_add = map { format_diff_line(esc_html($_, -nbsp=>1), 'add') } @$add;\n \n \treturn (\\@new_ctx, \\@new_rem, \\@new_add);\n }\n@@ -4939,7 +5010,8 @@ sub format_ctx_rem_add_lines {\n sub print_diff_lines {\n \tmy ($ctx, $rem, $add, $diff_style, $is_combined) = @_;\n \n-\t($ctx, $rem, $add) = format_ctx_rem_add_lines($ctx, $rem, $add);\n+\t($ctx, $rem, $add) = format_ctx_rem_add_lines($ctx, $rem, $add,\n+\t\t$is_combined);\n \n \tif ($diff_style eq 'sidebyside' && !$is_combined) {\n \t\tprint_sidebyside_diff_lines($ctx, $rem, $add);\n-- \n1.7.3.4\n"},{"id":"184336","messageId":"1328865494-24415-8-git-send-email-michal.kiedrowicz@gmail.com","threadId":"29593","inReplyTo":"1328865494-24415-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH 7/8] gitweb: Use different colors to present marked changes","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-10T09:18:13Z","receivedAt":"2012-02-10T09:18:13Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"This makes use of the highlight diff feature.\n\nSigned-off-by: Michał Kiedrowicz \n---\nI decided to split mechanism (generate HTML page with elements that\nmark interesting fragments of diff output) from politics (use these\nparticular colors for this elements), but otherwise this commit\nmay be squashed with the previous one. These colors work for me but if\nsomeone comes out with better ones, I'd be happy.\n\n gitweb/static/gitweb.css | 8 ++++++++\n 1 files changed, 8 insertions(+), 0 deletions(-)\n\ndiff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css\nindex c7827e8..4f87d16 100644\n--- a/gitweb/static/gitweb.css\n+++ b/gitweb/static/gitweb.css\n@@ -438,6 +438,10 @@ div.diff.add {\n \tcolor: #008800;\n }\n \n+div.diff.add span.marked {\n+\tbackground-color: #77ff77;\n+}\n+\n div.diff.from_file a.path,\n div.diff.from_file {\n \tcolor: #aa0000;\n@@ -447,6 +451,10 @@ div.diff.rem {\n \tcolor: #cc0000;\n }\n \n+div.diff.rem span.marked {\n+\tbackground-color: #ff7777;\n+}\n+\n div.diff.chunk_header a,\n div.diff.chunk_header {\n \tcolor: #990099;\n-- \n1.7.3.4\n"},{"id":"184337","messageId":"1328865494-24415-9-git-send-email-michal.kiedrowicz@gmail.com","threadId":"29593","inReplyTo":"1328865494-24415-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH 8/8] gitweb: Highlight combined diffs","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-10T09:18:14Z","receivedAt":"2012-02-10T09:18:14Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"The highlightning of combined diffs is currently disabled. This is\nbecause output from a combined diff is much harder to highlight because\nit's not obvious which removed and added lines should be compared.\n\nMoreover, code that compares added and removed lines doesn't care about\ncombined diffs. It only skips first +/- character, treating second +/-\nas a line content.\n\nLet's start with a simple case: only highlight changes that come from\none parent, i.e. when every removed line has a corresponding added line\nfor the same parent. This way the highlightning cannot get wrong. For\nexample, following diffs would be highlighted:\n\n\t- removed line for first parent\n\t+ added line for first parent\n\t context line\n\t -removed line for second parent\n\t +added line for second parent\n\nor\n\n\t- removed line for first parent\n\t -removed line for second parent\n\t+ added line for first parent\n\t +added line for second parent\n\nbut following output will not:\n\n\t- removed line for first parent\n\t -removed line for second parent\n\t +added line for second parent\n\t++added line for both parents\n\nFurther changes may introduce more intelligent approach that better\nhandles combined diffs.\n\nSigned-off-by: Michał Kiedrowicz \n---\n gitweb/gitweb.perl | 40 +++++++++++++++++++++++++++++++++++++---\n 1 files changed, 37 insertions(+), 3 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 1a5b454..2b6cb9e 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -4944,13 +4944,16 @@ sub esc_html_mark_range {\n \n # Format removed and added line, mark changed part and HTML-format them.\n sub format_rem_add_line {\n-\tmy ($rem, $add) = @_;\n+\tmy ($rem, $add, $is_combined) = @_;\n \tmy @r = split(//, $rem);\n \tmy @a = split(//, $add);\n \tmy ($esc_rem, $esc_add);\n \tmy ($prefix, $suffix_rem, $suffix_add) = (1, $#r, $#a);\n \tmy ($prefix_is_space, $suffix_is_space) = (1, 1);\n \n+\t# In combined diff we must ignore two +/- characters.\n+\t$prefix = 2 if ($is_combined);\n+\n \twhile ($prefix < @r && $prefix < @a) {\n \t\tlast if ($r[$prefix] ne $a[$prefix]);\n \n@@ -4988,11 +4991,42 @@ sub format_ctx_rem_add_lines {\n \tmy ($ctx, $rem, $add, $is_combined) = @_;\n \tmy (@new_ctx, @new_rem, @new_add);\n \tmy $num_add_lines = @$add;\n+\tmy $can_highlight;\n+\n+\t# Highlight if every removed line has a corresponding added line.\n+\tif ($num_add_lines > 0 && $num_add_lines == @$rem) {\n+\t\t$can_highlight = 1;\n+\n+\t\t# Highlight lines in combined diff only if the chunk contains\n+\t\t# diff between the same version, e.g.\n+\t\t#\n+\t\t# - a\n+\t\t# - b\n+\t\t# + c\n+\t\t# + d\n+\t\t#\n+\t\t# Otherwise the highlightling would be confusing.\n+\t\tif ($is_combined) {\n+\t\t\tfor (my $i = 0; $i < $num_add_lines; $i++) {\n+\t\t\t\tmy $prefix_rem = substr($rem->[$i], 0, 2);\n+\t\t\t\tmy $prefix_add = substr($add->[$i], 0, 2);\n+\n+\t\t\t\t$prefix_rem =~ s/-/+/g;\n+\n+\t\t\t\tif ($prefix_rem ne $prefix_add) {\n+\t\t\t\t\t$can_highlight = 0;\n+\t\t\t\t\tlast;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t}\n+\t} else {\n+\t\t$can_highlight = 0;\n+\t}\n \n-\tif (!$is_combined && $num_add_lines > 0 && $num_add_lines == @$rem) {\n+\tif ($can_highlight) {\n \t\tfor (my $i = 0; $i < $num_add_lines; $i++) {\n \t\t\tmy ($line_rem, $line_add) = format_rem_add_line(\n-\t\t\t\t$rem->[$i], $add->[$i]);\n+\t\t\t\t$rem->[$i], $add->[$i], $is_combined);\n \t\t\tpush @new_rem, $line_rem;\n \t\t\tpush @new_add, $line_add;\n \t\t}\n-- \n1.7.3.4\n"},{"id":"184351","messageId":"m339aivn4z.fsf@localhost.localdomain","threadId":"29593","inReplyTo":"1328865494-24415-7-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-10T13:23:10Z","receivedAt":"2012-02-10T13:23:10Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Michał Kiedrowicz writes:\n\n> The code that comares lines is based on\n> contrib/diff-highlight/diff-highlight, except that it works with\n> multiline changes too. It also won't highlight lines that are\n> completely different because that would only make the output unreadable.\n> Combined diffs are not supported but a following commit will change it.\n\nI was thinking that if gitweb were to support \"diff refinement\nhighlighting\", it would either use one of *Diff* packages from CPAN,\nor \"git diff --word-diff\" output.\n \n-- \nJakub Narębski\n"},{"id":"184354","messageId":"20120210151528.56145e0c@gmail.com","threadId":"29593","inReplyTo":"m339aivn4z.fsf@localhost.localdomain","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-10T14:15:28Z","receivedAt":"2012-02-10T14:15:28Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jakub Narebski wrote:\n\n> Michał Kiedrowicz writes:\n> \n> > The code that comares lines is based on\n> > contrib/diff-highlight/diff-highlight, except that it works with\n> > multiline changes too. It also won't highlight lines that are\n> > completely different because that would only make the output unreadable.\n> > Combined diffs are not supported but a following commit will change it.\n> \n> I was thinking that if gitweb were to support \"diff refinement\n> highlighting\", it would either use one of *Diff* packages from CPAN,\n> or \"git diff --word-diff\" output.\n> \n\nI think highlighting inline and side-by-side diff outputs is\nsomething different from \"git diff --word-diff\". I find it useful for\npeople who are used to these diff formats (i.e. me :).\n\nOTOH I'm not against using a dedicated package from CPAN. But I think\nmy approach is proven to work (I use contrib/diff-highlight as a\nfilter) and more lightweight (doesn't add another dependency to\ngitweb). Moreover, adding support for some Diff package may be done\nlater, at any moment. It's just a matter of replacing one function\n(format_rem_add_line()) with the one that uses Diff. \n"},{"id":"184357","messageId":"201202101555.20163.jnareb@gmail.com","threadId":"29593","inReplyTo":"20120210151528.56145e0c@gmail.com","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-10T14:55:19Z","receivedAt":"2012-02-10T14:55:19Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 10 Feb 2012, Michał Kiedrowicz wrote:\n> Jakub Narebski wrote:\n> > Michał Kiedrowicz writes:\n> > \n> > > The code that comares lines is based on\n> > > contrib/diff-highlight/diff-highlight, except that it works with\n> > > multiline changes too. It also won't highlight lines that are\n> > > completely different because that would only make the output unreadable.\n> > > Combined diffs are not supported but a following commit will change it.\n> > \n> > I was thinking that if gitweb were to support \"diff refinement\n> > highlighting\", it would either use one of *Diff* packages from CPAN,\n> > or \"git diff --word-diff\" output.\n> \n> I think highlighting inline and side-by-side diff outputs is\n> something different from \"git diff --word-diff\". I find it useful for\n> people who are used to these diff formats (i.e. me :).\n\nI was thinking about *using* \"git diff --word-diff\" for diff refinement\nhighlighting of inline (unified) and side-by-side diff... \n\nthough having an option of showing word-diff would be I think a good\nidea in some cases, like e.g. documentation changes.\n\n> OTOH I'm not against using a dedicated package from CPAN. But I think\n> my approach is proven to work (I use contrib/diff-highlight as a\n> filter) and more lightweight (doesn't add another dependency to\n> gitweb). Moreover, adding support for some Diff package may be done\n> later, at any moment. It's just a matter of replacing one function\n> (format_rem_add_line()) with the one that uses Diff. \n\nO.K., if it is tested code, then all is good. Well, except the fact\nthat I'm rather wary about adding more code to gitweb when it is still\nsingle monolithic script, rather than split into packages.\n\nAnyway, I'll try to review those patches soon. I like the refactoring\nwork (that is from what I had chance to examine).\n\n-- \nJakub Narebski\nPoland\n"},{"id":"184367","messageId":"20120210183319.2f56ff88@gmail.com","threadId":"29593","inReplyTo":"201202101555.20163.jnareb@gmail.com","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-10T17:33:19Z","receivedAt":"2012-02-10T17:33:19Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jakub Narebski wrote:\n\n> On Fri, 10 Feb 2012, Michał Kiedrowicz wrote:\n> > Jakub Narebski wrote:\n> > > Michał Kiedrowicz writes:\n> > > \n> > > > The code that comares lines is based on\n> > > > contrib/diff-highlight/diff-highlight, except that it works with\n> > > > multiline changes too. It also won't highlight lines that are\n> > > > completely different because that would only make the output unreadable.\n> > > > Combined diffs are not supported but a following commit will change it.\n> > > \n> > > I was thinking that if gitweb were to support \"diff refinement\n> > > highlighting\", it would either use one of *Diff* packages from CPAN,\n> > > or \"git diff --word-diff\" output.\n> > \n> > I think highlighting inline and side-by-side diff outputs is\n> > something different from \"git diff --word-diff\". I find it useful for\n> > people who are used to these diff formats (i.e. me :).\n> \n> I was thinking about *using* \"git diff --word-diff\" for diff refinement\n> highlighting of inline (unified) and side-by-side diff... \n> \n\nThen I must have misunderstood you. \n\n> though having an option of showing word-diff would be I think a good\n> idea in some cases, like e.g. documentation changes.\n> \n> > OTOH I'm not against using a dedicated package from CPAN. But I think\n> > my approach is proven to work (I use contrib/diff-highlight as a\n> > filter) and more lightweight (doesn't add another dependency to\n> > gitweb). Moreover, adding support for some Diff package may be done\n> > later, at any moment. It's just a matter of replacing one function\n> > (format_rem_add_line()) with the one that uses Diff. \n> \n> O.K., if it is tested code, then all is good. \n\nAs I wrote, I haven't taken the code as-is (for example, original\ncode only works for oneline changes). But the general approach is the\nsame.\n\n> Well, except the fact\n> that I'm rather wary about adding more code to gitweb when it is still\n> single monolithic script, rather than split into packages.\n> \n\nYeah, jumping between 2k'th and 5k'th line isn't a great fun. Do you\nhave any roadmap how to split gitweb?\n\n> Anyway, I'll try to review those patches soon. I like the refactoring\n> work (that is from what I had chance to examine).\n\nThanks.\n"},{"id":"184391","messageId":"20120210202008.GA5874@sigill.intra.peff.net","threadId":"29593","inReplyTo":"1328865494-24415-7-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-10T20:20:08Z","receivedAt":"2012-02-10T20:20:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 10, 2012 at 10:18:12AM +0100, Michał Kiedrowicz wrote:\n\n> The code that comares lines is based on\n> contrib/diff-highlight/diff-highlight, except that it works with\n> multiline changes too. It also won't highlight lines that are\n> completely different because that would only make the output unreadable.\n> Combined diffs are not supported but a following commit will change it.\n\nHave you considered contributing back the enhancements to\ncontrib/diff-highlight? I took a look at handling multi-line changes\nwhen I originally wrote it, but I was worried too much about failing to\nmatch up lines properly, and ending up with too much noise in the diff.\nMaybe your \"don't highlight lines that are completely different\" rule\nhelps that, though.\n\nDo you have any examples handy? (I was hoping not to need to get a\nrunning gitweb installation in order to see the output).\n\n-Peff\n"},{"id":"184392","messageId":"20120210202450.GC5504@sigill.intra.peff.net","threadId":"29593","inReplyTo":"201202101555.20163.jnareb@gmail.com","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-10T20:24:50Z","receivedAt":"2012-02-10T20:24:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 10, 2012 at 03:55:19PM +0100, Jakub Narebski wrote:\n\n> > I think highlighting inline and side-by-side diff outputs is\n> > something different from \"git diff --word-diff\". I find it useful for\n> > people who are used to these diff formats (i.e. me :).\n> \n> I was thinking about *using* \"git diff --word-diff\" for diff refinement\n> highlighting of inline (unified) and side-by-side diff... \n\nYeah, you might be able to get better results out of a real diff engine\n(i.e., having \"git diff\" do the color by doing a real LCS-match) than\nout of diff-highlight (which is really just a couple of simple\nheuristics). OTOH, the heuristics sometimes make the result \"less noisy\"\nbecause they don't find little bits of commonality, and instead\nhighlight only a single chunk per line.\n\nSo if somebody wanted to work on that, I'd be really curious to see if\nthey could get better results. It's not high enough priority for me\npersonally, as I find diff-highlight is \"good enough\".\n\n-Peff\n"},{"id":"184400","messageId":"20120210222916.2721e9e6@gmail.com","threadId":"29593","inReplyTo":"20120210202008.GA5874@sigill.intra.peff.net","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-10T21:29:16Z","receivedAt":"2012-02-10T21:29:16Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jeff King wrote:\n\n> On Fri, Feb 10, 2012 at 10:18:12AM +0100, Michał Kiedrowicz wrote:\n> \n> > The code that comares lines is based on\n> > contrib/diff-highlight/diff-highlight, except that it works with\n> > multiline changes too. It also won't highlight lines that are\n> > completely different because that would only make the output unreadable.\n> > Combined diffs are not supported but a following commit will change it.\n> \n> Have you considered contributing back the enhancements to\n> contrib/diff-highlight? \n\nYeah, I did. In fact, at work I have a hacked version of your\ndiff-highlight that supports multiline changes and I use it every day.\nBut I just couldn't make myself fix your long README and send a\npatch :).\n\nMaybe I'll cook something in my spare time.\n\n> I took a look at handling multi-line changes\n> when I originally wrote it, but I was worried too much about failing to\n> match up lines properly, and ending up with too much noise in the diff.\n> Maybe your \"don't highlight lines that are completely different\" rule\n> helps that, though.\n\nI must say that it works great for me. Most often it's very helping.\nLike every heuristics it sometimes goes the wrong way, but it's so rare\nthat I don't find it disturbing.\n\n> \n> Do you have any examples handy? (I was hoping not to need to get a\n> running gitweb installation in order to see the output).\n> \n> -Peff\n\nNope. Except for comparing diffs in various commits in gitweb-1.7.9 and\nfrom my branch, I just created a dummy commit with different kinds of\nchanges to check if they are properly colorized. \n"},{"id":"184402","messageId":"20120210213209.GA7582@sigill.intra.peff.net","threadId":"29593","inReplyTo":"20120210222916.2721e9e6@gmail.com","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-10T21:32:09Z","receivedAt":"2012-02-10T21:32:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 10, 2012 at 10:29:16PM +0100, Michał Kiedrowicz wrote:\n\n> > Have you considered contributing back the enhancements to\n> > contrib/diff-highlight? \n> \n> Yeah, I did. In fact, at work I have a hacked version of your\n> diff-highlight that supports multiline changes and I use it every day.\n> But I just couldn't make myself fix your long README and send a\n> patch :).\n\nHeh. Why don't you show your hack in the meantime. Even if the code is\nugly or the documentation is missing, I'd like to see the output, and\nmaybe we can fix those other things together.\n\n-Peff\n"},{"id":"184403","messageId":"20120210223631.17f4edad@gmail.com","threadId":"29593","inReplyTo":"20120210213209.GA7582@sigill.intra.peff.net","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-10T21:36:31Z","receivedAt":"2012-02-10T21:36:31Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jeff King wrote:\n\n> On Fri, Feb 10, 2012 at 10:29:16PM +0100, Michał Kiedrowicz wrote:\n> \n> > > Have you considered contributing back the enhancements to\n> > > contrib/diff-highlight? \n> > \n> > Yeah, I did. In fact, at work I have a hacked version of your\n> > diff-highlight that supports multiline changes and I use it every day.\n> > But I just couldn't make myself fix your long README and send a\n> > patch :).\n> \n> Heh. Why don't you show your hack in the meantime. Even if the code is\n> ugly or the documentation is missing, I'd like to see the output, and\n> maybe we can fix those other things together.\n> \n> -Peff\n\nThen just give must few minutes.\n"},{"id":"184406","messageId":"1328910433-2539-1-git-send-email-michal.kiedrowicz@gmail.com","threadId":"29593","inReplyTo":"20120210213209.GA7582@sigill.intra.peff.net","subject":"[PATCH] diff-highlight: Work for multiline changes too","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-10T21:47:13Z","receivedAt":"2012-02-10T21:47:13Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Signed-off-by: Michał Kiedrowicz \n---\n\nAfter looking at outputs I noticed that it can also ignore lines with\nprefixes/suffixes that consist only of punctuation (asterisk, semicolon, dot,\netc), because otherwise whole line is highlighted except for terminating\npunctuation.\n\n contrib/diff-highlight/diff-highlight | 96 ++++++++++++++++++++++-----------\n 1 files changed, 65 insertions(+), 31 deletions(-)\n\ndiff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight\nindex d893898..4811550 100755\n--- a/contrib/diff-highlight/diff-highlight\n+++ b/contrib/diff-highlight/diff-highlight\n@@ -1,28 +1,40 @@\n #!/usr/bin/perl\n \n+use warnings;\n+use strict;\n+\n # Highlight by reversing foreground and background. You could do\n # other things like bold or underline if you prefer.\n my $HIGHLIGHT = \"\\x1b[7m\";\n my $UNHIGHLIGHT = \"\\x1b[27m\";\n my $COLOR = qr/\\x1b\\[[0-9;]*m/;\n \n-my @window;\n+my $context;\n+my @removed = ();\n+my @added = ();\n+my $started = 0;\n \n while (<>) {\n-\t# We highlight only single-line changes, so we need\n-\t# a 4-line window to make a decision on whether\n-\t# to highlight.\n-\tpush @window, $_;\n-\tnext if @window < 4;\n-\tif ($window[0] =~ /^$COLOR*(\\@| )/ &&\n-\t $window[1] =~ /^$COLOR*-/ &&\n-\t $window[2] =~ /^$COLOR*\\+/ &&\n-\t $window[3] !~ /^$COLOR*\\+/) {\n-\t\tprint shift @window;\n-\t\tshow_pair(shift @window, shift @window);\n-\t}\n-\telse {\n-\t\tprint shift @window;\n+\tif (/^$COLOR*-/) {\n+\t\tpush @removed, $_;\n+\t} elsif (/^$COLOR*\\+/) {\n+\t\tpush @added, $_;\n+\t} else {\n+\t\tif ($started == 1 ) {\n+\t\t\tshow_pairs(\\@removed, \\@added);\n+\t\t} else {\n+\t\t\tprint @removed;\n+\t\t\tprint @added;\n+\t\t}\n+\t\t@removed = ();\n+\t\t@added = ();\n+\t\tprint $_;\n+\n+\t\tif (/^$COLOR*(\\@| )/) {\n+\t\t\t$started = 1;\n+\t\t} else {\n+\t\t\t$started = 0;\n+\t\t}\n \t}\n \n \t# Most of the time there is enough output to keep things streaming,\n@@ -38,23 +50,33 @@ while (<>) {\n \t}\n }\n \n-# Special case a single-line hunk at the end of file.\n-if (@window == 3 &&\n- $window[0] =~ /^$COLOR*(\\@| )/ &&\n- $window[1] =~ /^$COLOR*-/ &&\n- $window[2] =~ /^$COLOR*\\+/) {\n-\tprint shift @window;\n-\tshow_pair(shift @window, shift @window);\n-}\n-\n-# And then flush any remaining lines.\n-while (@window) {\n-\tprint shift @window;\n-}\n+show_pairs(\\@removed, \\@added);\n \n exit 0;\n \n-sub show_pair {\n+sub show_pairs {\n+\tmy $a = shift;\n+\tmy $b = shift;\n+\n+\tif (scalar(@{$a}) == scalar(@{$b}) && scalar(@${a}) > 0) {\n+\t\tmy @removed;\n+\t\tmy @added;\n+\n+\t\tfor(my $i = 0; $i < scalar(@{$a}); $i++) {\n+\t\t\tmy ($rm, $add) = highlight_pair($a->[$i], $b->[$i]);\n+\t\t\tpush @removed, $rm;\n+\t\t\tpush @added, $add;\n+\t\t}\n+\n+\t\tprint @removed;\n+\t\tprint @added;\n+\t} else {\n+\t\tprint @{$a};\n+\t\tprint @{$b};\n+\t}\n+}\n+\n+sub highlight_pair {\n \tmy @a = split_line(shift);\n \tmy @b = split_line(shift);\n \n@@ -101,8 +123,20 @@ sub show_pair {\n \t\t}\n \t}\n \n-\tprint highlight(\\@a, $pa, $sa);\n-\tprint highlight(\\@b, $pb, $sb);\n+\tmy $prefa = join('', @a[0..($pa-1)]);\n+\tmy $prefb = join('', @b[0..($pb-1)]);\n+\tmy $sufa = join('', @a[($sa+1)..$#a]);\n+\tmy $sufb = join('', @b[($sb+1)..$#b]);\n+\n+\t# Highlight only if prefix or suffix is interesting (i.e. not consisting\n+\t# of color and (for prefix) +/-). Otherwise we would highlight whole\n+\t# lines.\n+\tif ($prefa =~ /^($COLOR)*-(\\s|$COLOR)*$/ && $sufa =~ /^(\\s|$COLOR)*$/\n+\t\t&& $prefb =~ /^($COLOR)*\\+(\\s|$COLOR)*$/ && $sufb =~ /^(\\s|$COLOR)*$/) {\n+\t\treturn join('', @a), join('', @b);\n+\t} else {\n+\t\treturn highlight(\\@a, $pa, $sa), highlight(\\@b, $pb, $sb);\n+\t}\n }\n \n sub split_line {\n-- \n1.7.3.4\n"},{"id":"184407","messageId":"m3y5satksm.fsf@localhost.localdomain","threadId":"29593","inReplyTo":"20120210202008.GA5874@sigill.intra.peff.net","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-10T21:56:11Z","receivedAt":"2012-02-10T21:56:11Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Jeff King writes:\n\n> (I was hoping not to need to get a running gitweb installation in\n> order to see the output).\n\nWell, there is always git-instaweb ;-)\n\n-- \nJakub Narębski\n"},{"id":"184414","messageId":"201202102352.42101.jnareb@gmail.com","threadId":"29593","inReplyTo":"20120210183319.2f56ff88@gmail.com","subject":"Re: Splitting gitweb (was: Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff)","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-10T22:52:41Z","receivedAt":"2012-02-10T22:52:41Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Michał Kiedrowicz wrote:\n> Jakub Narebski wrote:\n\n> > Well, except the fact\n> > that I'm rather wary about adding more code to gitweb when it is still\n> > single monolithic script, rather than split into packages.\n> > \n> \n> Yeah, jumping between 2k'th and 5k'th line isn't a great fun. Do you\n\nThere is an easy part, an almost easy part, and a hard part.\n\nThe easy part of splitting gitweb is creating infrastructure for it,\nat least in the basic case. The 'gitweb/split' branch in my git forks:\n\n http://repo.or.cz/w/git/jnareb-git.git\n https://github.com/jnareb/git\n\ncontains changes to gitweb and gitweb/Makefile, and splitting off\nGitweb::Util as an example; I'd have to update this branch to current\nstate of gitweb.\n\nThe almost easy part is to come up with a way to split gitweb. Do we\nfollow SVN::Web (Subversion web interface in Perl), or maybe Gitalist\n(git web interface in Perl, using Catalyst MVC framework)? Do we use\nMVC paradigm? Or do we split on functionality: diffs, blobs, trees,\nlogs, etc.?\n\n\nThe hard part is about splitting main parts of gitweb. It is easy to\nput generic subroutines that are not specific to git or gitweb in\nGitweb::Util (or Gitweb::Util::* submodules). It would be almost as\neasy to put parsing of git command output in Gitweb::Parse (or \nGitweb::Parse::* submodules).\n\nThe problem is with putting actual actions in separate submodules.\nFor that we would need to replace our hacky \"longjmp\"-based error handling\n(nonlocal goto in Perl is roughly equivalent to longjmp() in C) to\nexception-based one, as I don't think going back to exit-based error\nahndling is a good idea. We would need exception-based error handling\nif we want to implement HTTP output caching anyway, I think.\n\nNot to not reimplement the wheel, badly, we will do better to use some\nnon-core Perl modules, namely Try::Tiny for capturing exceptions, and\nHTTP::Exception (based on Exception::Class) for throwing exceptions.\nSo we would have to add a way to handle such non-core modules, perhaps\nbundling them with gitweb, like Error is bundled with Git.pm and used\nif it is not present on install.\n\nMaybe Module::Install would help us there with bundling of such\ndependencies for install; maybe \"push @INC, __DIR__.'/inc' would be\na good idea.\n\nIt needs thinking about.\n-- \nJakub Narebski\nPoland\n"},{"id":"184457","messageId":"m3obt5tn0g.fsf@localhost.localdomain","threadId":"29593","inReplyTo":"1328865494-24415-2-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH 1/8] gitweb: Extract print_sidebyside_diff_lines()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-11T15:20:35Z","receivedAt":"2012-02-11T15:20:35Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Michał Kiedrowicz writes:\n\n> Currently, print_sidebyside_diff_chunk() does two things: it\n> accumulates diff lines and prints them. Accumulation may be used to\n> perform additional operations on diff lines, so it makes sense to split\n> these two things. Thus, the code that prints diff lines in a side-by-side\n> manner is moved out of print_sidebyside_diff_chunk() to a separate\n> subroutine.\n> \n> The outcome of this patch is that print_sidebyside_diff_chunk() is now\n> much shorter and easier to read.\n> \n> This change is meant to be a simple code movement. No behavior change is\n> intended.\n> \n> Signed-off-by: Michał Kiedrowicz \n\nAs it is pure refactoring, and improves readibility of code quite a\nbit, I am all for it, but...\n\nYou replace the following set of conditions\n\n> -\t\t## print from accumulator when type of class of lines change\n> -\t\t# empty contents block on start rem/add block, or end of chunk\n> -\t\tif (@ctx && (!$class || $class eq 'rem' || $class eq 'add')) {\n\nand\n\n> -\t\t# empty add/rem block on start context block, or end of chunk\n> -\t\tif ((@rem || @add) && (!$class || $class eq 'ctx')) {\n> -\t\t\tif (!@add) {\n[...]\n> -\t\t\t} elsif (!@rem) {\n[...]\n> -\t\t\t} else {\n\n\nwith these (I have reindented the code to match)\n\n> +\t\t## print from accumulator when have some add/rem lines or end\n> +\t\t# of chunk (flush context lines)\n> +\t\tif (((@rem || @add) && $class eq 'ctx') || !$class) {\n\n> + if (@$ctx) {\n[...]\n> + }\n> + if (!@$add) {\n[...]\n> + } elsif (!@$rem) {\n[...]\n> + } else {\n\nIt is not obvious that the final result is the same.\n\nBTW doesn't new code print context when printing added and removed\nlines, and not as soon as class changes? This doesn't change the\noutput, but it slightly changes behavior.\n\n-- \nJakub Narębski\n"},{"id":"184458","messageId":"m3k43ttlh9.fsf@localhost.localdomain","threadId":"29593","inReplyTo":"1328865494-24415-3-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH 2/8] gitweb: Use print_diff_chunk() for both side-by-side and inline diffs","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-11T15:53:41Z","receivedAt":"2012-02-11T15:53:41Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Michał Kiedrowicz writes:\n\n> This renames print_sidebyside_diff_chunk() to print_diff_chunk() and\n> makes use of it for both side-by-side and inline diffs. Now diff lines\n> are always accumulated before they are printed. This opens the\n> possibility to preprocess diff output before it's printed.\n> \n> The new function print_inline_diff_lines() could reorder diff lines.\n\nI think you meant here \"if left as is\", isn't it?\n\n> It first prints all context lines, then all removed lines and\n> finally all added lines. If the diff output consisted of mixed added\n> and removed lines, gitweb would reorder these lines. This is\n> especially true for combined diff output, for example:\n> \n> \t - removed line for first parent\n> \t + added line for first parent\n> \t -removed line for second parent\n> \t ++added line for both parents\n> \n> would be rendered as:\n> \n> \t- removed line for first parent\n> \t -removed line for second parent\n> \t+ added line for first parent\n> \t++added line for both parents\n\nThis is not \"is especially true\"; it can happen (though: can it\nreally?) in combined diff output; in ordinary diff you would always\nhave context or end/beginning of chunk between added and removed\nlines.\n\n> \n> To prevent gitweb from reordering lines, print_diff_chunk() calls\n> print_diff_lines() as soon as it detects that both added and removed\n> lines are present and there was a class change.\n\nYou really should mention, in my opinion, that this change is\npreparation for diff refinement highlighting (higlighting the\ndiffering segments).\n\nOtherwise I don't see the reason for it: ordinary diff didn't need the\nfancy stuff with accumulating then printing, and could be send\nstraightly to output. This adds complexity for non-sidebyside diff,\nunnecessary if not for diff refinement highlighting.\n \n> Signed-off-by: Michał Kiedrowicz \n> ---\n> gitweb/gitweb.perl | 53 +++++++++++++++++++++++++++++++++++++--------------\n> 1 files changed, 38 insertions(+), 15 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 1247607..ed63261 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -4908,9 +4908,31 @@ sub print_sidebyside_diff_lines {\n> \t}\n> }\n> \n> -sub print_sidebyside_diff_chunk {\n> -\tmy @chunk = @_;\n> +# Print context lines and then rem/add lines in inline manner.\n> +sub print_inline_diff_lines {\n> +\tmy ($ctx, $rem, $add) = @_;\n> +\n> +\tprint foreach (@$ctx);\n> +\tprint foreach (@$rem);\n> +\tprint foreach (@$add);\n> +}\n> +\n> +# Print context lines and then rem/add lines.\n> +sub print_diff_lines {\n> +\tmy ($ctx, $rem, $add, $diff_style, $is_combined) = @_;\n> +\n> +\tif ($diff_style eq 'sidebyside' && !$is_combined) {\n> +\t\tprint_sidebyside_diff_lines($ctx, $rem, $add);\n> +\t} else {\n> +\t\t# default 'inline' style and unknown styles\n> +\t\tprint_inline_diff_lines($ctx, $rem, $add);\n> +\t}\n> +}\n\nThat looks nice.\n\nBTW. I didn't examine the final code, but what happens for binary\ndiffs that git supports? Is it handled outside print_diff_chunk()?\n\n> +\n> +sub print_diff_chunk {\n> +\tmy ($diff_style, $is_combined, @chunk) = @_;\n> \tmy (@ctx, @rem, @add);\n> +\tmy $prev_class = '';\n\nIs $prev_class here class of previous chunk fragment or _previous class_\n(i.e. always different from $class), or is it class of previous line?\nIt is not obvious here, from variable name. Better name or at least\na comment would be appreciated.\n\n> \n> \treturn unless @chunk;\n> \n> @@ -4935,9 +4957,13 @@ sub print_sidebyside_diff_chunk {\n> \t\t}\n> \n> \t\t## print from accumulator when have some add/rem lines or end\n> -\t\t# of chunk (flush context lines)\n> -\t\tif (((@rem || @add) && $class eq 'ctx') || !$class) {\n> -\t\t\tprint_sidebyside_diff_lines(\\@ctx, \\@rem, \\@add);\n> +\t\t# of chunk (flush context lines), or when have add and rem\n> +\t\t# lines and new block is reached (otherwise add/rem lines could\n> +\t\t# be reordered)\n> +\t\tif (((@rem || @add) && $class eq 'ctx') || !$class ||\n> +\t\t\t\t(@rem && @add && $class ne $prev_class)) {\n\nWe usually use tabs to align, but spaces to indent in gitweb, so it\nwould be\n\n> +\t\tif (((@rem || @add) && $class eq 'ctx') || !$class ||\n> +\t\t (@rem && @add && $class ne $prev_class)) {\n\nAnyway, this conditional gets ridicously complicated. I wonder if we\ncould simplify it.\n\nIf we allow printing context together widh added/removed lines, and\nnot as soon as possible, perhaps good conditional would be: class\nchanged, and accumulator for current class is full:\n\n> +\t\tif ($class ne $prev_class && @{ $acc{$class} }) {\n\nOr something like that, where\n\n my %acc = (ctx => \\@ctx, add => \\@add, rem => \\@rem);\n\nIf we want to print context as soon as possible, like it was before\n1st patch in this series, we could uuse the following conditional\n\n> +\t\tif ($class ne $prev_class && \n> + ($prev_class eq 'ctx' || @{ $acc{$class} })) {\n\nPerhaps with \"scalar @{ $acc{$class} }\" if it would make things more\nclear.\n\nThough probably to avoid \"Use of uninitialized value in string ne\"\nwe would probably have to use empty string instead of undefined\nvalue for \"no class\" case, i.e. beginning or end of chunk.\n\n> +\t\t\tprint_diff_lines(\\@ctx, \\@rem, \\@add, $diff_style,\n> +\t\t\t\t\t$is_combined);\n> \t\t\t@ctx = @rem = @add = ();\n> \t\t}\n> \n> @@ -4954,6 +4980,8 @@ sub print_sidebyside_diff_chunk {\n> \t\tif ($class eq 'ctx') {\n> \t\t\tpush @ctx, $line;\n> \t\t}\n> +\n> +\t\t$prev_class = $class;\n> \t}\n> }\n[...] \n\n-- \nJakub Narębski\n"},{"id":"184459","messageId":"m3fwehtl1t.fsf@localhost.localdomain","threadId":"29593","inReplyTo":"1328865494-24415-4-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH 3/8] gitweb: Move HTML-formatting diff line back to process_diff_line()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-11T16:02:57Z","receivedAt":"2012-02-11T16:02:57Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Michał Kiedrowicz writes:\n\n> Commit 6ba1eb51b (gitweb: Add a feature to show side-by-side diff,\n> 2011-10-31) for no special reason moved wrapping diff line in
out\n> of format_diff_line(). Bring back old behavior.\n> \n> This simplifies code in git_patchset_body() and keeps formatting of a\n> diff line in one place.\n> \n> The more long-term purpose of this patch is to move formatting diff\n> lines down to print_diff_chunk(), to allow processing lines without\n> HTML-formatting.\n> \n> This is just a refactoring patch. It's not meant to change gitweb\n> output.\n\nGah, I don't remember why we ended with change in behavior when adding\nside-by-side diff support to gitweb.\n \n> Signed-off-by: Michał Kiedrowicz \n\n Acked-by: Jakub Narębski \n\n> ---\n> gitweb/gitweb.perl | 15 ++++++++-------\n> 1 files changed, 8 insertions(+), 7 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index ed63261..d2f75c4 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -2332,14 +2332,18 @@ sub process_diff_line {\n> \n> \tif ($from && $to && $line =~ m/^\\@{2} /) {\n> \t\t$line = format_unidiff_chunk_header($line, $from, $to);\n> -\t\treturn $diff_class, $line;\n> \n> \t} elsif ($from && $to && $line =~ m/^\\@{3}/) {\n> \t\t$line = format_cc_diff_chunk_header($line, $from, $to);\n> -\t\treturn $diff_class, $line;\n> -\n> +\t} else {\n> +\t\t$line = esc_html($line, -nbsp=>1);\n> \t}\n> -\treturn $diff_class, esc_html($line, -nbsp=>1);\n> +\n> +\tmy $diff_classes = \"diff\";\n> +\t$diff_classes .= \" $diff_class\" if ($diff_class);\n> +\t$line = \"
$line
\\n\";\n> +\n> +\treturn $diff_class, $line;\n> }\n> \n> # Generates undef or something like \"_snapshot_\" or \"snapshot (_tbz2_ _zip_)\",\n> @@ -5104,9 +5108,6 @@ sub git_patchset_body {\n> \t\t\tnext PATCH if ($patch_line =~ m/^diff /);\n> \n> \t\t\tmy ($class, $line) = process_diff_line($patch_line, \\%from, \\%to);\n> -\t\t\tmy $diff_classes = \"diff\";\n> -\t\t\t$diff_classes .= \" $class\" if ($class);\n> -\t\t\t$line = \"
$line
\\n\";\n> \n> \t\t\tif ($class eq 'chunk_header') {\n> \t\t\t\tprint_diff_chunk($diff_style, $is_combined, @chunk);\n> -- \n> 1.7.3.4\n> \n\n-- \nJakub Narębski\n"},{"id":"184460","messageId":"m3bop5tjt6.fsf@localhost.localdomain","threadId":"29593","inReplyTo":"1328865494-24415-5-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH 4/8] gitweb: Push formatting diff lines to print_diff_chunk()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-11T16:29:45Z","receivedAt":"2012-02-11T16:29:45Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Michał Kiedrowicz writes:\n\n> Now git_patchset_body() only calls diff_line_class() (removed from\n> process_diff_line()). The latter function is renamed to\n> format_diff_line() and is called from print_diff_chunk().\n> \n> This is a pure code movement, needed for processing raw diff lines in\n> the accumulator in print_diff_chunk(). No behavior change is intended by\n> this change.\n\nWell, this is not \"pure code movement\" per se; it is meant to be\nrefactoring that doesn't change gitweb output nor behavior.\n\nIf I understand correctly the change is from\n\n read\n format\n accumulate\n print\n \nto\n\n read\n accumulate\n format\n print\n\nIsn't it?\n\nAs a note I would add also that process_diff_line got renamed to\nformat_diff_line, and its output changed to returning only\nHTML-formatted line, which bringg it in line with other format_*\nsubroutines.\n\n> Signed-off-by: Michał Kiedrowicz \n\nI think it is a good change even without subsequent patches.\n\n Acked-by: Jakub Narębski \n\n> ---\n> gitweb/gitweb.perl | 25 ++++++++++++-------------\n> 1 files changed, 12 insertions(+), 13 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index d2f75c4..cae9dfa 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -2320,12 +2320,9 @@ sub format_cc_diff_chunk_header {\n> }\n> \n> # process patch (diff) line (not to be used for diff headers),\n> -# returning class and HTML-formatted (but not wrapped) line\n> -sub process_diff_line {\n> -\tmy $line = shift;\n> -\tmy ($from, $to) = @_;\n> -\n> -\tmy $diff_class = diff_line_class($line, $from, $to);\n> +# returning HTML-formatted (but not wrapped) line\n> +sub format_diff_line {\n> +\tmy ($line, $diff_class, $from, $to) = @_;\n> \n> \tchomp $line;\n> \t$line = untabify($line);\n> @@ -2343,7 +2340,7 @@ sub process_diff_line {\n> \t$diff_classes .= \" $diff_class\" if ($diff_class);\n> \t$line = \"
$line
\\n\";\n> \n> -\treturn $diff_class, $line;\n> +\treturn $line;\n> }\n> \n> # Generates undef or something like \"_snapshot_\" or \"snapshot (_tbz2_ _zip_)\",\n> @@ -4934,7 +4931,7 @@ sub print_diff_lines {\n> }\n> \n> sub print_diff_chunk {\n> -\tmy ($diff_style, $is_combined, @chunk) = @_;\n> +\tmy ($diff_style, $is_combined, $from, $to, @chunk) = @_;\n> \tmy (@ctx, @rem, @add);\n> \tmy $prev_class = '';\n> \n> @@ -4954,6 +4951,8 @@ sub print_diff_chunk {\n> \tforeach my $line_info (@chunk) {\n> \t\tmy ($class, $line) = @$line_info;\n> \n> +\t\t$line = format_diff_line($line, $class, $from, $to);\n> +\n> \t\t# print chunk headers\n> \t\tif ($class && $class eq 'chunk_header') {\n> \t\t\tprint $line;\n> @@ -5107,19 +5106,19 @@ sub git_patchset_body {\n> \n> \t\t\tnext PATCH if ($patch_line =~ m/^diff /);\n> \n> -\t\t\tmy ($class, $line) = process_diff_line($patch_line, \\%from, \\%to);\n> +\t\t\tmy $class = diff_line_class($patch_line, \\%from, \\%to);\n> \n> \t\t\tif ($class eq 'chunk_header') {\n> -\t\t\t\tprint_diff_chunk($diff_style, $is_combined, @chunk);\n> -\t\t\t\t@chunk = ( [ $class, $line ] );\n> +\t\t\t\tprint_diff_chunk($diff_style, $is_combined, \\%from, \\%to, @chunk);\n> +\t\t\t\t@chunk = ( [ $class, $patch_line ] );\n> \t\t\t} else {\n> -\t\t\t\tpush @chunk, [ $class, $line ];\n> +\t\t\t\tpush @chunk, [ $class, $patch_line ];\n> \t\t\t}\n> \t\t}\n> \n> \t} continue {\n> \t\tif (@chunk) {\n> -\t\t\tprint_diff_chunk($diff_style, $is_combined, @chunk);\n> +\t\t\tprint_diff_chunk($diff_style, $is_combined, \\%from, \\%to, @chunk);\n> \t\t\t@chunk = ();\n> \t\t}\n> \t\tprint \"
\\n\"; # class=\"patch\"\n> -- \n> 1.7.3.4\n> \n\nNice!\n\n-- \nJakub Narębski\n"},{"id":"184461","messageId":"m37gztthrx.fsf@localhost.localdomain","threadId":"29593","inReplyTo":"1328865494-24415-6-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH 5/8] gitweb: Format diff lines just before printing","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-11T17:14:11Z","receivedAt":"2012-02-11T17:14:11Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Michał Kiedrowicz writes:\n\n> Now we're ready to insert highlightning to diff output.\n> \n> The call to untabify() remains in the main loop in print_diff_chunk().\n> The motivation is that it must be called before any call to esc_html()\n> (because that converts spaces to  ) and to call it only once.\n> \n> This is a refactoring patch. It's not meant to change gitweb output.\n\nI'm not sure about this patch.\n\nFirst, in my opinion it doesn't make much sense standalone, and not\nsquashed with subsequent patch.\n\nSecond, it makes format_diff_line an odd duck among all other format_*\nsubroutines in that it post-processes HTML output, rather than\ngenerating HTML from data.\n \nWhy \"diff refinement highlighting\" cannot be part of format_diff_line()?\nIf it does need additional data, just pass it as additional parameters\nto this subroutine.\n\nAnother solution could be to borrow idea from stalled and inactive\ncommittags feature, namely that parts that are HTML are to be passed\nas scalar reference (\\$str), while plain text (unescaped yet) is to be\npassed as string ($str).\n\n> -# process patch (diff) line (not to be used for diff headers),\n> -# returning HTML-formatted (but not wrapped) line\n> +# wrap patch (diff) line into a
(not to be used for diff headers),\n> +# the line must be esc_html()'ed\n> sub format_diff_line {\n\nI just don't like this error-prone \"the line must be esc_html()'ed\".\n\n> +# HTML-format diff context, removed and added lines.\n> +sub format_ctx_rem_add_lines {\n> +\tmy ($ctx, $rem, $add) = @_;\n> +\tmy (@new_ctx, @new_rem, @new_add);\n> +\n> +\t@new_ctx = map { format_diff_line(esc_html($_, -nbsp=>1), 'ctx') } @$ctx;\n> +\t@new_rem = map { format_diff_line(esc_html($_, -nbsp=>1), 'rem') } @$rem;\n> +\t@new_add = map { format_diff_line(esc_html($_, -nbsp=>1), 'add') } @$add;\n> +\n> +\treturn (\\@new_ctx, \\@new_rem, \\@new_add);\n> +}\n> +\n> # Print context lines and then rem/add lines.\n> sub print_diff_lines {\n> \tmy ($ctx, $rem, $add, $diff_style, $is_combined) = @_;\n> \n> +\t($ctx, $rem, $add) = format_ctx_rem_add_lines($ctx, $rem, $add);\n> +\n\nNice trick.\n\n-- \nJakub Narębski\n"},{"id":"184468","messageId":"m31uq1te51.fsf@localhost.localdomain","threadId":"29593","inReplyTo":"1328865494-24415-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH 0/8] gitweb: Highlight interesting parts of diff","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-11T18:32:12Z","receivedAt":"2012-02-11T18:32:12Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Michał Kiedrowicz writes:\n\n\n> gitweb: Use print_diff_chunk() for both side-by-side and inline diffs\n> gitweb: Move HTML-formatting diff line back to process_diff_line()\n> gitweb: Push formatting diff lines to print_diff_chunk()\n> gitweb: Format diff lines just before printing\n\nThose patshes are expected to not change gitweb output. Did you check\nthat gitweb after those changes handles incomplete line marker\ncorrectly? print_sidebyside_diff_chunk() ealt with 'incomplete' class\nin slightly hacky way, assuming side-by-side output.\n\nNamely, does gitweb after those patches print incomplete line marker\ncorrectly, i.e. only once for \"inline\" output while it prints it for\nboth sides in \"side-by-side\" output, and using 'incomplete' class?\n\nYou have to treat correctly all the following situations:\n\n* removing end of line at end of file (eol-at-eof):\n\n diff --git a/a b/b\n index 257cc56..1910281 100644\n --- a/a\n +++ b/b\n @@ -1 +1 @@\n -foo\n +foo\n \\ No newline at end of file\n\n* adding eol-at-eof:\n\n diff --git b/b a/a\n index 1910281..257cc56 100644\n --- b/b\n +++ a/a\n @@ -1 +1 @@\n -foo\n \\ No newline at end of file\n +foo\n\n* removing from end of file, preserving lack of eol-at-eof:\n\n diff --git b/c a/a\n index a5d4a2f..257cc56 100644\n --- b/c\n +++ a/a\n @@ -1,2 +1 @@\n foo\n -foo\n \\ No newline at end of file\n\n* change at the end of file, preserving lack of eol-at-eof:\n\n diff --git a/b b/d\n index 1910281..ba0e162 100644\n --- a/b\n +++ b/d\n @@ -1 +1 @@\n -foo\n \\ No newline at end of file\n +bar\n \\ No newline at end of file\n\n* change near the end of file, with incomplete line marker in context\n\n diff --git b/c a/b\n index a5d4a2f..1910281 100644\n --- b/c\n +++ a/b\n @@ -1,2 +1 @@\n -foo\n foo\n \\ No newline at end of file\n\n\n-- \nJakub Narębski\n"},{"id":"184478","messageId":"20120211235637.5ae5e9f7@gmail.com","threadId":"29593","inReplyTo":"m31uq1te51.fsf@localhost.localdomain","subject":"Re: [PATCH 0/8] gitweb: Highlight interesting parts of diff","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-11T22:56:37Z","receivedAt":"2012-02-11T22:56:37Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jakub Narebski wrote:\n\n> Michał Kiedrowicz writes:\n> \n> \n> > gitweb: Use print_diff_chunk() for both side-by-side and inline diffs\n> > gitweb: Move HTML-formatting diff line back to process_diff_line()\n> > gitweb: Push formatting diff lines to print_diff_chunk()\n> > gitweb: Format diff lines just before printing\n> \n> Those patshes are expected to not change gitweb output. \n\nYes.\n\n> Did you check\n> that gitweb after those changes handles incomplete line marker\n> correctly? print_sidebyside_diff_chunk() ealt with 'incomplete' class\n> in slightly hacky way, assuming side-by-side output.\n> \n\nYeah, I saw the 'for' loop at the beginning of\nprint_sidebyside_diff_chunk() but I didn't look at it very closely.\nI'll make sure 'incomplete' lines are properly printed.\n"},{"id":"184479","messageId":"20120212000325.2f6bc9f0@gmail.com","threadId":"29593","inReplyTo":"m3obt5tn0g.fsf@localhost.localdomain","subject":"Re: [PATCH 1/8] gitweb: Extract print_sidebyside_diff_lines()","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-11T23:03:25Z","receivedAt":"2012-02-11T23:03:25Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jakub Narebski wrote:\n\n> Michał Kiedrowicz writes:\n> \n> > Currently, print_sidebyside_diff_chunk() does two things: it\n> > accumulates diff lines and prints them. Accumulation may be used to\n> > perform additional operations on diff lines, so it makes sense to split\n> > these two things. Thus, the code that prints diff lines in a side-by-side\n> > manner is moved out of print_sidebyside_diff_chunk() to a separate\n> > subroutine.\n> > \n> > The outcome of this patch is that print_sidebyside_diff_chunk() is now\n> > much shorter and easier to read.\n> > \n> > This change is meant to be a simple code movement. No behavior change is\n> > intended.\n> > \n> > Signed-off-by: Michał Kiedrowicz \n> \n> As it is pure refactoring, and improves readibility of code quite a\n> bit, I am all for it, but...\n> \n> You replace the following set of conditions\n> \n> > -\t\t## print from accumulator when type of class of lines change\n> > -\t\t# empty contents block on start rem/add block, or end of chunk\n> > -\t\tif (@ctx && (!$class || $class eq 'rem' || $class eq 'add')) {\n> \n> and\n> \n> > -\t\t# empty add/rem block on start context block, or end of chunk\n> > -\t\tif ((@rem || @add) && (!$class || $class eq 'ctx')) {\n> > -\t\t\tif (!@add) {\n> [...]\n> > -\t\t\t} elsif (!@rem) {\n> [...]\n> > -\t\t\t} else {\n> \n> \n> with these (I have reindented the code to match)\n> \n> > +\t\t## print from accumulator when have some add/rem lines or end\n> > +\t\t# of chunk (flush context lines)\n> > +\t\tif (((@rem || @add) && $class eq 'ctx') || !$class) {\n> \n> > + if (@$ctx) {\n> [...]\n> > + }\n> > + if (!@$add) {\n> [...]\n> > + } elsif (!@$rem) {\n> [...]\n> > + } else {\n> \n> It is not obvious that the final result is the same.\n\nYou are right. I should have described it in the commit message.\n\n> \n> BTW doesn't new code print context when printing added and removed\n> lines, and not as soon as class changes? This doesn't change the\n> output, but it slightly changes behavior.\n> \n\nThis is also true. I'll describe it in the commit message. I could also\ncreate two functions: one that prints @ctx and second one that prints\n@rem and @add but then I would have to create two other functions for\ninline diff.\n"},{"id":"184480","messageId":"20120212001624.4083c3b6@gmail.com","threadId":"29593","inReplyTo":"m3k43ttlh9.fsf@localhost.localdomain","subject":"Re: [PATCH 2/8] gitweb: Use print_diff_chunk() for both side-by-side and inline diffs","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-11T23:16:24Z","receivedAt":"2012-02-11T23:16:24Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jakub Narebski wrote:\n\n> Michał Kiedrowicz writes:\n> \n> > This renames print_sidebyside_diff_chunk() to print_diff_chunk() and\n> > makes use of it for both side-by-side and inline diffs. Now diff lines\n> > are always accumulated before they are printed. This opens the\n> > possibility to preprocess diff output before it's printed.\n> > \n> > The new function print_inline_diff_lines() could reorder diff lines.\n> \n> I think you meant here \"if left as is\", isn't it?\n\nYes.\n\n> > It first prints all context lines, then all removed lines and\n> > finally all added lines. If the diff output consisted of mixed added\n> > and removed lines, gitweb would reorder these lines. This is\n> > especially true for combined diff output, for example:\n> > \n> > \t - removed line for first parent\n> > \t + added line for first parent\n> > \t -removed line for second parent\n> > \t ++added line for both parents\n> > \n> > would be rendered as:\n> > \n> > \t- removed line for first parent\n> > \t -removed line for second parent\n> > \t+ added line for first parent\n> > \t++added line for both parents\n> \n> This is not \"is especially true\"; it can happen (though: can it\n> really?) \n\nYeah. See\nhttp://git.kernel.org/?p=git/git.git;a=commitdiff;h=5de89d3abfca98b0dfd0280d28576940c913d60d \n\n\n> in combined diff output; in ordinary diff you would always\n> have context or end/beginning of chunk between added and removed\n> lines.\n\nOK, I'll reword this part of the commit message.\n> \n> > \n> > To prevent gitweb from reordering lines, print_diff_chunk() calls\n> > print_diff_lines() as soon as it detects that both added and removed\n> > lines are present and there was a class change.\n> \n> You really should mention, in my opinion, that this change is\n> preparation for diff refinement highlighting (higlighting the\n> differing segments).\n> \n> Otherwise I don't see the reason for it: ordinary diff didn't need the\n> fancy stuff with accumulating then printing, and could be send\n> straightly to output. This adds complexity for non-sidebyside diff,\n> unnecessary if not for diff refinement highlighting.\n\nI completely agree with that. This patch doesn't makes sense without\nlater patches.\n\n> \n> > Signed-off-by: Michał Kiedrowicz \n> > ---\n> > gitweb/gitweb.perl | 53 +++++++++++++++++++++++++++++++++++++--------------\n> > 1 files changed, 38 insertions(+), 15 deletions(-)\n> > \n> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> > index 1247607..ed63261 100755\n> > --- a/gitweb/gitweb.perl\n> > +++ b/gitweb/gitweb.perl\n> > @@ -4908,9 +4908,31 @@ sub print_sidebyside_diff_lines {\n> > \t}\n> > }\n> > \n> > -sub print_sidebyside_diff_chunk {\n> > -\tmy @chunk = @_;\n> > +# Print context lines and then rem/add lines in inline manner.\n> > +sub print_inline_diff_lines {\n> > +\tmy ($ctx, $rem, $add) = @_;\n> > +\n> > +\tprint foreach (@$ctx);\n> > +\tprint foreach (@$rem);\n> > +\tprint foreach (@$add);\n> > +}\n> > +\n> > +# Print context lines and then rem/add lines.\n> > +sub print_diff_lines {\n> > +\tmy ($ctx, $rem, $add, $diff_style, $is_combined) = @_;\n> > +\n> > +\tif ($diff_style eq 'sidebyside' && !$is_combined) {\n> > +\t\tprint_sidebyside_diff_lines($ctx, $rem, $add);\n> > +\t} else {\n> > +\t\t# default 'inline' style and unknown styles\n> > +\t\tprint_inline_diff_lines($ctx, $rem, $add);\n> > +\t}\n> > +}\n> \n> That looks nice.\n> \n> BTW. I didn't examine the final code, but what happens for binary\n> diffs that git supports? Is it handled outside print_diff_chunk()?\n\nGood question. Will check.\n\n> \n> > +\n> > +sub print_diff_chunk {\n> > +\tmy ($diff_style, $is_combined, @chunk) = @_;\n> > \tmy (@ctx, @rem, @add);\n> > +\tmy $prev_class = '';\n> \n> Is $prev_class here class of previous chunk fragment or _previous class_\n> (i.e. always different from $class), or is it class of previous line?\n> It is not obvious here, from variable name. Better name or at least\n> a comment would be appreciated.\n> \n\nIt's a class of previous line.\n\n> > \n> > \treturn unless @chunk;\n> > \n> > @@ -4935,9 +4957,13 @@ sub print_sidebyside_diff_chunk {\n> > \t\t}\n> > \n> > \t\t## print from accumulator when have some add/rem lines or end\n> > -\t\t# of chunk (flush context lines)\n> > -\t\tif (((@rem || @add) && $class eq 'ctx') || !$class) {\n> > -\t\t\tprint_sidebyside_diff_lines(\\@ctx, \\@rem, \\@add);\n> > +\t\t# of chunk (flush context lines), or when have add and rem\n> > +\t\t# lines and new block is reached (otherwise add/rem lines could\n> > +\t\t# be reordered)\n> > +\t\tif (((@rem || @add) && $class eq 'ctx') || !$class ||\n> > +\t\t\t\t(@rem && @add && $class ne $prev_class)) {\n> \n> We usually use tabs to align, but spaces to indent in gitweb, so it\n> would be\n> \n> > +\t\tif (((@rem || @add) && $class eq 'ctx') || !$class ||\n> > +\t\t (@rem && @add && $class ne $prev_class)) {\n> \n\nOK.\n\n> Anyway, this conditional gets ridicously complicated. I wonder if we\n> could simplify it.\n> \n> If we allow printing context together widh added/removed lines, and\n> not as soon as possible, perhaps good conditional would be: class\n> changed, and accumulator for current class is full:\n> \n> > +\t\tif ($class ne $prev_class && @{ $acc{$class} }) {\n> \n> Or something like that, where\n> \n> my %acc = (ctx => \\@ctx, add => \\@add, rem => \\@rem);\n> \n> If we want to print context as soon as possible, like it was before\n> 1st patch in this series, we could uuse the following conditional\n> \n> > +\t\tif ($class ne $prev_class && \n> > + ($prev_class eq 'ctx' || @{ $acc{$class} })) {\n> \n> Perhaps with \"scalar @{ $acc{$class} }\" if it would make things more\n> clear.\n> \n> Though probably to avoid \"Use of uninitialized value in string ne\"\n> we would probably have to use empty string instead of undefined\n> value for \"no class\" case, i.e. beginning or end of chunk.\n>\n\nOK, I'll try to work out something.\n \n> > +\t\t\tprint_diff_lines(\\@ctx, \\@rem, \\@add, $diff_style,\n> > +\t\t\t\t\t$is_combined);\n> > \t\t\t@ctx = @rem = @add = ();\n> > \t\t}\n> > \n> > @@ -4954,6 +4980,8 @@ sub print_sidebyside_diff_chunk {\n> > \t\tif ($class eq 'ctx') {\n> > \t\t\tpush @ctx, $line;\n> > \t\t}\n> > +\n> > +\t\t$prev_class = $class;\n> > \t}\n> > }\n> [...] \n> \n"},{"id":"184481","messageId":"20120212002025.214b3fb3@gmail.com","threadId":"29593","inReplyTo":"m3bop5tjt6.fsf@localhost.localdomain","subject":"Re: [PATCH 4/8] gitweb: Push formatting diff lines to print_diff_chunk()","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-11T23:20:25Z","receivedAt":"2012-02-11T23:20:25Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jakub Narebski wrote:\n\n> Michał Kiedrowicz writes:\n> \n> > Now git_patchset_body() only calls diff_line_class() (removed from\n> > process_diff_line()). The latter function is renamed to\n> > format_diff_line() and is called from print_diff_chunk().\n> > \n> > This is a pure code movement, needed for processing raw diff lines in\n> > the accumulator in print_diff_chunk(). No behavior change is intended by\n> > this change.\n> \n> Well, this is not \"pure code movement\" per se; it is meant to be\n> refactoring that doesn't change gitweb output nor behavior.\n> \n> If I understand correctly the change is from\n> \n> read\n> format\n> accumulate\n> print\n> \n> to\n> \n> read\n> accumulate\n> format\n> print\n> \n> Isn't it?\n\nYeah, this is what I meant :).\n\n> \n> As a note I would add also that process_diff_line got renamed to\n> format_diff_line, \n\nI thought I wrote that (\"The latter function is renamed...\")?\n\n> and its output changed to returning only\n> HTML-formatted line, which bringg it in line with other format_*\n> subroutines.\n> \n\nOK.\n\n> > Signed-off-by: Michał Kiedrowicz \n> \n> I think it is a good change even without subsequent patches.\n> \n> Acked-by: Jakub Narębski \n> \n\nThanks.\n\n> > ---\n> > gitweb/gitweb.perl | 25 ++++++++++++-------------\n> > 1 files changed, 12 insertions(+), 13 deletions(-)\n> > \n> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> > index d2f75c4..cae9dfa 100755\n> > --- a/gitweb/gitweb.perl\n> > +++ b/gitweb/gitweb.perl\n> > @@ -2320,12 +2320,9 @@ sub format_cc_diff_chunk_header {\n> > }\n> > \n> > # process patch (diff) line (not to be used for diff headers),\n> > -# returning class and HTML-formatted (but not wrapped) line\n> > -sub process_diff_line {\n> > -\tmy $line = shift;\n> > -\tmy ($from, $to) = @_;\n> > -\n> > -\tmy $diff_class = diff_line_class($line, $from, $to);\n> > +# returning HTML-formatted (but not wrapped) line\n> > +sub format_diff_line {\n> > +\tmy ($line, $diff_class, $from, $to) = @_;\n> > \n> > \tchomp $line;\n> > \t$line = untabify($line);\n> > @@ -2343,7 +2340,7 @@ sub process_diff_line {\n> > \t$diff_classes .= \" $diff_class\" if ($diff_class);\n> > \t$line = \"
$line
\\n\";\n> > \n> > -\treturn $diff_class, $line;\n> > +\treturn $line;\n> > }\n> > \n> > # Generates undef or something like \"_snapshot_\" or \"snapshot (_tbz2_ _zip_)\",\n> > @@ -4934,7 +4931,7 @@ sub print_diff_lines {\n> > }\n> > \n> > sub print_diff_chunk {\n> > -\tmy ($diff_style, $is_combined, @chunk) = @_;\n> > +\tmy ($diff_style, $is_combined, $from, $to, @chunk) = @_;\n> > \tmy (@ctx, @rem, @add);\n> > \tmy $prev_class = '';\n> > \n> > @@ -4954,6 +4951,8 @@ sub print_diff_chunk {\n> > \tforeach my $line_info (@chunk) {\n> > \t\tmy ($class, $line) = @$line_info;\n> > \n> > +\t\t$line = format_diff_line($line, $class, $from, $to);\n> > +\n> > \t\t# print chunk headers\n> > \t\tif ($class && $class eq 'chunk_header') {\n> > \t\t\tprint $line;\n> > @@ -5107,19 +5106,19 @@ sub git_patchset_body {\n> > \n> > \t\t\tnext PATCH if ($patch_line =~ m/^diff /);\n> > \n> > -\t\t\tmy ($class, $line) = process_diff_line($patch_line, \\%from, \\%to);\n> > +\t\t\tmy $class = diff_line_class($patch_line, \\%from, \\%to);\n> > \n> > \t\t\tif ($class eq 'chunk_header') {\n> > -\t\t\t\tprint_diff_chunk($diff_style, $is_combined, @chunk);\n> > -\t\t\t\t@chunk = ( [ $class, $line ] );\n> > +\t\t\t\tprint_diff_chunk($diff_style, $is_combined, \\%from, \\%to, @chunk);\n> > +\t\t\t\t@chunk = ( [ $class, $patch_line ] );\n> > \t\t\t} else {\n> > -\t\t\t\tpush @chunk, [ $class, $line ];\n> > +\t\t\t\tpush @chunk, [ $class, $patch_line ];\n> > \t\t\t}\n> > \t\t}\n> > \n> > \t} continue {\n> > \t\tif (@chunk) {\n> > -\t\t\tprint_diff_chunk($diff_style, $is_combined, @chunk);\n> > +\t\t\tprint_diff_chunk($diff_style, $is_combined, \\%from, \\%to, @chunk);\n> > \t\t\t@chunk = ();\n> > \t\t}\n> > \t\tprint \"
\\n\"; # class=\"patch\"\n> > -- \n> > 1.7.3.4\n> > \n> \n> Nice!\n> \n"},{"id":"184482","messageId":"20120212003054.63255bea@gmail.com","threadId":"29593","inReplyTo":"20120212002025.214b3fb3@gmail.com","subject":"Re: [PATCH 4/8] gitweb: Push formatting diff lines to print_diff_chunk()","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-11T23:30:54Z","receivedAt":"2012-02-11T23:30:54Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Michał Kiedrowicz wrote:\n\n> Jakub Narebski wrote:\n> \n> > Michał Kiedrowicz writes:\n> > \n> > > Now git_patchset_body() only calls diff_line_class() (removed from\n> > > process_diff_line()). The latter function is renamed to\n> > > format_diff_line() and is called from print_diff_chunk().\n> > > \n> > > This is a pure code movement, needed for processing raw diff lines in\n> > > the accumulator in print_diff_chunk(). No behavior change is intended by\n> > > this change.\n> > \n> > Well, this is not \"pure code movement\" per se; it is meant to be\n> > refactoring that doesn't change gitweb output nor behavior.\n> > \n> > If I understand correctly the change is from\n> > \n> > read\n> > format\n> > accumulate\n> > print\n> > \n> > to\n> > \n> > read\n> > accumulate\n> > format\n> > print\n> > \n> > Isn't it?\n> \n> Yeah, this is what I meant :).\n\nMinor correction: The order *does* change, in the sense that first a\nchunk is read, then (in print_diff_chunk) formatted and printed,\nbut in print_diff_chunk() the order remains:\n\n\tread line\n\tformat\n\tput into accumulator\n\tprint\n> \n> > \n> > As a note I would add also that process_diff_line got renamed to\n> > format_diff_line, \n> \n> I thought I wrote that (\"The latter function is renamed...\")?\n> \n> > and its output changed to returning only\n> > HTML-formatted line, which bringg it in line with other format_*\n> > subroutines.\n> > \n> \n> OK.\n> \n> > > Signed-off-by: Michał Kiedrowicz \n> > \n> > I think it is a good change even without subsequent patches.\n> > \n> > Acked-by: Jakub Narębski \n> > \n> \n> Thanks.\n> \n> > > ---\n> > > gitweb/gitweb.perl | 25 ++++++++++++-------------\n> > > 1 files changed, 12 insertions(+), 13 deletions(-)\n> > > \n> > > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> > > index d2f75c4..cae9dfa 100755\n> > > --- a/gitweb/gitweb.perl\n> > > +++ b/gitweb/gitweb.perl\n> > > @@ -2320,12 +2320,9 @@ sub format_cc_diff_chunk_header {\n> > > }\n> > > \n> > > # process patch (diff) line (not to be used for diff headers),\n> > > -# returning class and HTML-formatted (but not wrapped) line\n> > > -sub process_diff_line {\n> > > -\tmy $line = shift;\n> > > -\tmy ($from, $to) = @_;\n> > > -\n> > > -\tmy $diff_class = diff_line_class($line, $from, $to);\n> > > +# returning HTML-formatted (but not wrapped) line\n> > > +sub format_diff_line {\n> > > +\tmy ($line, $diff_class, $from, $to) = @_;\n> > > \n> > > \tchomp $line;\n> > > \t$line = untabify($line);\n> > > @@ -2343,7 +2340,7 @@ sub process_diff_line {\n> > > \t$diff_classes .= \" $diff_class\" if ($diff_class);\n> > > \t$line = \"
$line
\\n\";\n> > > \n> > > -\treturn $diff_class, $line;\n> > > +\treturn $line;\n> > > }\n> > > \n> > > # Generates undef or something like \"_snapshot_\" or \"snapshot (_tbz2_ _zip_)\",\n> > > @@ -4934,7 +4931,7 @@ sub print_diff_lines {\n> > > }\n> > > \n> > > sub print_diff_chunk {\n> > > -\tmy ($diff_style, $is_combined, @chunk) = @_;\n> > > +\tmy ($diff_style, $is_combined, $from, $to, @chunk) = @_;\n> > > \tmy (@ctx, @rem, @add);\n> > > \tmy $prev_class = '';\n> > > \n> > > @@ -4954,6 +4951,8 @@ sub print_diff_chunk {\n> > > \tforeach my $line_info (@chunk) {\n> > > \t\tmy ($class, $line) = @$line_info;\n> > > \n> > > +\t\t$line = format_diff_line($line, $class, $from, $to);\n> > > +\n> > > \t\t# print chunk headers\n> > > \t\tif ($class && $class eq 'chunk_header') {\n> > > \t\t\tprint $line;\n> > > @@ -5107,19 +5106,19 @@ sub git_patchset_body {\n> > > \n> > > \t\t\tnext PATCH if ($patch_line =~ m/^diff /);\n> > > \n> > > -\t\t\tmy ($class, $line) = process_diff_line($patch_line, \\%from, \\%to);\n> > > +\t\t\tmy $class = diff_line_class($patch_line, \\%from, \\%to);\n> > > \n> > > \t\t\tif ($class eq 'chunk_header') {\n> > > -\t\t\t\tprint_diff_chunk($diff_style, $is_combined, @chunk);\n> > > -\t\t\t\t@chunk = ( [ $class, $line ] );\n> > > +\t\t\t\tprint_diff_chunk($diff_style, $is_combined, \\%from, \\%to, @chunk);\n> > > +\t\t\t\t@chunk = ( [ $class, $patch_line ] );\n> > > \t\t\t} else {\n> > > -\t\t\t\tpush @chunk, [ $class, $line ];\n> > > +\t\t\t\tpush @chunk, [ $class, $patch_line ];\n> > > \t\t\t}\n> > > \t\t}\n> > > \n> > > \t} continue {\n> > > \t\tif (@chunk) {\n> > > -\t\t\tprint_diff_chunk($diff_style, $is_combined, @chunk);\n> > > +\t\t\tprint_diff_chunk($diff_style, $is_combined, \\%from, \\%to, @chunk);\n> > > \t\t\t@chunk = ();\n> > > \t\t}\n> > > \t\tprint \"
\\n\"; # class=\"patch\"\n> > > -- \n> > > 1.7.3.4\n> > > \n> > \n> > Nice!\n> > \n"},{"id":"184483","messageId":"20120212003821.276f2c5a@gmail.com","threadId":"29593","inReplyTo":"m37gztthrx.fsf@localhost.localdomain","subject":"Re: [PATCH 5/8] gitweb: Format diff lines just before printing","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-11T23:38:21Z","receivedAt":"2012-02-11T23:38:21Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jakub Narebski wrote:\n\n> Michał Kiedrowicz writes:\n> \n> > Now we're ready to insert highlightning to diff output.\n> > \n> > The call to untabify() remains in the main loop in print_diff_chunk().\n> > The motivation is that it must be called before any call to esc_html()\n> > (because that converts spaces to  ) and to call it only once.\n> > \n> > This is a refactoring patch. It's not meant to change gitweb output.\n> \n> I'm not sure about this patch.\n> \n> First, in my opinion it doesn't make much sense standalone, and not\n> squashed with subsequent patch.\n\nTrue. I wanted to separate \"preparation\" from \"adding new feature\" but\nmaybe went few steps too far.\n\n> \n> Second, it makes format_diff_line an odd duck among all other format_*\n> subroutines in that it post-processes HTML output, rather than\n> generating HTML from data.\n> \n> Why \"diff refinement highlighting\" cannot be part of format_diff_line()?\n> If it does need additional data, just pass it as additional parameters\n> to this subroutine.\n> \n> Another solution could be to borrow idea from stalled and inactive\n> committags feature, namely that parts that are HTML are to be passed\n> as scalar reference (\\$str), while plain text (unescaped yet) is to be\n> passed as string ($str).\n\nI'll look into it.\n\n> \n> > -# process patch (diff) line (not to be used for diff headers),\n> > -# returning HTML-formatted (but not wrapped) line\n> > +# wrap patch (diff) line into a
(not to be used for diff headers),\n> > +# the line must be esc_html()'ed\n> > sub format_diff_line {\n> \n> I just don't like this error-prone \"the line must be esc_html()'ed\".\n> \n> > +# HTML-format diff context, removed and added lines.\n> > +sub format_ctx_rem_add_lines {\n> > +\tmy ($ctx, $rem, $add) = @_;\n> > +\tmy (@new_ctx, @new_rem, @new_add);\n> > +\n> > +\t@new_ctx = map { format_diff_line(esc_html($_, -nbsp=>1), 'ctx') } @$ctx;\n> > +\t@new_rem = map { format_diff_line(esc_html($_, -nbsp=>1), 'rem') } @$rem;\n> > +\t@new_add = map { format_diff_line(esc_html($_, -nbsp=>1), 'add') } @$add;\n> > +\n> > +\treturn (\\@new_ctx, \\@new_rem, \\@new_add);\n> > +}\n> > +\n> > # Print context lines and then rem/add lines.\n> > sub print_diff_lines {\n> > \tmy ($ctx, $rem, $add, $diff_style, $is_combined) = @_;\n> > \n> > +\t($ctx, $rem, $add) = format_ctx_rem_add_lines($ctx, $rem, $add);\n> > +\n> \n> Nice trick.\n> \n"},{"id":"184487","messageId":"m3y5s9rl3g.fsf@localhost.localdomain","threadId":"29593","inReplyTo":"1328865494-24415-7-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-11T23:45:29Z","receivedAt":"2012-02-11T23:45:29Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Michał Kiedrowicz writes:\n\n> Reading diff output is sometimes very hard, even if it's colored,\n> especially if lines differ only in few characters. This is often true\n> when a commit fixes a typo or renames some variables or functions.\n>\nThis is certainly nice feature to have. I think most tools that\nimplement side-by-side diff implement this too.\n\n> This commit teaches gitweb to highlight characters that are different\n> between old and new line. This should work in the similar manner as in\n> Trac or GitHub.\n>\nDoe you have URLs with good examples of such diff refinement\nhighlighting (GNU Emacs ediff/emerge terminology) / highlighting\ndiffering segments (diff-highlight terminology)?\n \n> The code that compares lines is based on\n> contrib/diff-highlight/diff-highlight, except that it works with\n> multiline changes too. It also won't highlight lines that are\n> completely different because that would only make the output unreadable.\n>\nSo what does it look like? From the contrib/diff-highlight/README I\nguess that it finds common prefix and common suffix, and highlights\nthe rest, which includes change. It doesn't implement LCS / diff\nalgorithm like e.g. tkdiff does for its diff refinement highlighting,\nisn't it?\n\nBy completly different you mean that they do not have common prefix or\ncommon suffix (at least one of them), isn't it?\n\n> Combined diffs are not supported but a following commit will change it.\n> \nO.K.\n\n> Signed-off-by: Michał Kiedrowicz \n> ---\n> gitweb/gitweb.perl | 82 ++++++++++++++++++++++++++++++++++++++++++++++++---\n> 1 files changed, 77 insertions(+), 5 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index db61553..1a5b454 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -2322,7 +2322,7 @@ sub format_cc_diff_chunk_header {\n> # wrap patch (diff) line into a
(not to be used for diff headers),\n> # the line must be esc_html()'ed\n> sub format_diff_line {\n> -\tmy ($line, $diff_class, $from, $to) = @_;\n> +\tmy ($line, $diff_class) = @_;\n\nWhy that change?\n\n> \n> \tmy $diff_classes = \"diff\";\n> \t$diff_classes .= \" $diff_class\" if ($diff_class);\n> @@ -4923,14 +4923,85 @@ sub print_inline_diff_lines {\n> \tprint foreach (@$add);\n> }\n> \n> +# Highlight characters from $prefix to $suffix and escape HTML.\n> +# $str is a reference to the array of characters.\n> +sub esc_html_mark_range {\n> +\tmy ($str, $prefix, $suffix) = @_;\n> +\n> +\t# Don't generate empty element.\n> +\tif ($prefix == $suffix + 1) {\n> +\t\treturn esc_html(join('', @$str), -nbsp=>1);\n> +\t}\n> +\n> +\tmy $before = join('', @{$str}[0..($prefix - 1)]);\n> +\tmy $marked = join('', @{$str}[$prefix..$suffix]);\n> +\tmy $after = join('', @{$str}[($suffix + 1)..$#{$str}]);\n\nEeeeeek! First you split into letters, in caller at that, then join?\nWhy not pass striung ($str suggests string not array of characters),\nand use substr instead?\n\n[Please disregard this and the next paragraph at first reading]\n\n> +\n> +\treturn esc_html($before, -nbsp=>1) .\n> +\t\t$cgi->span({-class=>'marked'}, esc_html($marked, -nbsp=>1)) .\n> +\t\tesc_html($after,-nbsp=>1);\n> +}\n\nAnyway I have send to git mailing list a patch series, which in one of\npatches adds esc_html_match_hl($str, $regexp) to highlight matches in\na string. Your esc_html_mark_range(), after a generalization, could\nbe used as underlying \"engine\".\n\nSomething like this, perhaps (untested):\n\n # Highlight selected fragments of string, using given CSS class,\n # and escape HTML. It is assumed that fragments do not overlap.\n # Regions are passed as list of pairs (array references).\n sub esc_html_hl {\n my ($str, $css_class, @sel) = @_;\n return esc_html($str) unless @sel;\n \n my $out = '';\n my $pos = 0;\n \n for my $s (@sel) {\n $out .= esc_html(substr($str, $pos, $s->[0] - $pos))\n if ($s->[0] - $pos > 0);\n $out .= $cgi->span({-class => $css_class},\n esc_html(substr($str, $s->[0], $s->[1] - $s->[0])));\n\n $pos = $m->[1];\n }\n $out .= esc_html(substr($str, $pos))\n if ($pos < length($str));\n \n return $out;\n }\n\n> +\n> +# Format removed and added line, mark changed part and HTML-format them.\n\nYou should probably ad here that this code is taken from\ndiff-highlight in contrib area, isn't it?\n\n> +sub format_rem_add_line {\n> +\tmy ($rem, $add) = @_;\n> +\tmy @r = split(//, $rem);\n> +\tmy @a = split(//, $add);\n> +\tmy ($esc_rem, $esc_add);\n> +\tmy ($prefix, $suffix_rem, $suffix_add) = (1, $#r, $#a);\n\nIt is not as much $prefix, as $prefix_len, isn't it? Shouldn't\n$prefix / $prefix_len start from 0, not from 1?\n\n> +\tmy ($prefix_is_space, $suffix_is_space) = (1, 1);\n> +\n> +\twhile ($prefix < @r && $prefix < @a) {\n> +\t\tlast if ($r[$prefix] ne $a[$prefix]);\n> +\n> +\t\t$prefix_is_space = 0 if ($r[$prefix] !~ /\\s/);\n> +\t\t$prefix++;\n> +\t}\n\nAh, I see that it is easier to find common prefix by treating string\nas array of characters.\n\nThough I wonder if it wouldn't be easier to use trick of XOR-ing two\nstrings (see \"Bitwise String Operators\" in perlop(1)):\n\n my $xor = \"$rem\" ^ \"$add\";\n\nand finding starting sequence of \"\\0\", which denote common prefix.\n\n\nThough this and the following is a nice implementation of\nalgorithm... as it would be implemented in C. Nevermind, it might be\ngood enough...\n\n> +\n> +\twhile ($suffix_rem >= $prefix && $suffix_add >= $prefix) {\n> +\t\tlast if ($r[$suffix_rem] ne $a[$suffix_add]);\n> +\n> +\t\t$suffix_is_space = 0 if ($r[$suffix_rem] !~ /\\s/);\n> +\t\t$suffix_rem--;\n> +\t\t$suffix_add--;\n> +\t}\n\nBTW., perhaps using single negative $suffix_len instead of separate\n$suffix_rem_pos and $suffix_add_pos would make code simpler and easier\nto understand?\n\n> +\n> +\t# Mark lines that are different from each other, but have some common\n> +\t# part that isn't whitespace. If lines are completely different, don't\n> +\t# mark them because that would make output unreadable, especially if\n> +\t# diff consists of multiple lines.\n> +\tif (($prefix == 1 && $suffix_rem == $#r && $suffix_add == $#a)\n> +\t\t|| ($prefix_is_space && $suffix_is_space)) {\n\nMicronit about style: in gitweb we put boolean operator at the end of\ncontinued line, not at beginning of next one.\n\nSo this would be:\n\n +\tif (($prefix == 1 && $suffix_rem == $#r && $suffix_add == $#a) ||\n +\t ($prefix_is_space && $suffix_is_space)) {\n\n> +\t\t$esc_rem = esc_html($rem);\n> +\t\t$esc_add = esc_html($add);\n> +\t} else {\n> +\t\t$esc_rem = esc_html_mark_range(\\@r, $prefix, $suffix_rem);\n> +\t\t$esc_add = esc_html_mark_range(\\@a, $prefix, $suffix_add);\n> +\t}\n> +\n> +\treturn format_diff_line($esc_rem, 'rem'),\n> +\t\tformat_diff_line($esc_add, 'add');\n\nPlease use spaces to align.\n\n> +}\n> +\n> # HTML-format diff context, removed and added lines.\n> sub format_ctx_rem_add_lines {\n> -\tmy ($ctx, $rem, $add) = @_;\n> +\tmy ($ctx, $rem, $add, $is_combined) = @_;\n> \tmy (@new_ctx, @new_rem, @new_add);\n> +\tmy $num_add_lines = @$add;\n\nWhy is this temporary variable needed? If you are not sure that ==\noperator enforces scalar context on both arguments you can always\nwrite\n\n scalar @$add == scalar @$rem\n\n> +\n> +\tif (!$is_combined && $num_add_lines > 0 && $num_add_lines == @$rem) {\n> +\t\tfor (my $i = 0; $i < $num_add_lines; $i++) {\n> +\t\t\tmy ($line_rem, $line_add) = format_rem_add_line(\n> +\t\t\t\t$rem->[$i], $add->[$i]);\n> +\t\t\tpush @new_rem, $line_rem;\n> +\t\t\tpush @new_add, $line_add;\n\nThe original contrib/diff-highlight works only for single changed line\n(single removed and single added). You make this code work also for\nthe case where number of aded lines is equal to the number of removed\nlines, and assume that subsequent changed lines in preimage\ncorrecponds to subsequent changed lines in postimage, which is not\nalways true:\n\n -foo\n -bar\n +baz\n +fooo\n\nThis is not described in commit message, I think.\n\n> +\t\t}\n> +\t} else {\n> +\t\t@new_rem = map { format_diff_line(esc_html($_, -nbsp=>1), 'rem') } @$rem;\n> +\t\t@new_add = map { format_diff_line(esc_html($_, -nbsp=>1), 'add') } @$add;\n> +\t}\n> \n> \t@new_ctx = map { format_diff_line(esc_html($_, -nbsp=>1), 'ctx') } @$ctx;\n> -\t@new_rem = map { format_diff_line(esc_html($_, -nbsp=>1), 'rem') } @$rem;\n> -\t@new_add = map { format_diff_line(esc_html($_, -nbsp=>1), 'add') } @$add;\n> \n> \treturn (\\@new_ctx, \\@new_rem, \\@new_add);\n> }\n> @@ -4939,7 +5010,8 @@ sub format_ctx_rem_add_lines {\n> sub print_diff_lines {\n> \tmy ($ctx, $rem, $add, $diff_style, $is_combined) = @_;\n> \n> -\t($ctx, $rem, $add) = format_ctx_rem_add_lines($ctx, $rem, $add);\n> +\t($ctx, $rem, $add) = format_ctx_rem_add_lines($ctx, $rem, $add,\n> +\t\t$is_combined);\n\nO.K., now the code depends on $is_combined\n\n-- \nJakub Narębski\n"},{"id":"184488","messageId":"m3ty2wsyt6.fsf@localhost.localdomain","threadId":"29593","inReplyTo":"1328865494-24415-9-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH 8/8] gitweb: Highlight combined diffs","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-12T00:03:19Z","receivedAt":"2012-02-12T00:03:19Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Michał Kiedrowicz writes:\n\n> The highlightning of combined diffs is currently disabled. This is\n> because output from a combined diff is much harder to highlight because\n> it's not obvious which removed and added lines should be compared.\n> \n> Moreover, code that compares added and removed lines doesn't care about\n> combined diffs. It only skips first +/- character, treating second +/-\n> as a line content.\n> \n> Let's start with a simple case: only highlight changes that come from\n> one parent, i.e. when every removed line has a corresponding added line\n> for the same parent. This way the highlightning cannot get wrong. For\n> example, following diffs would be highlighted:\n> \n> \t- removed line for first parent\n> \t+ added line for first parent\n> \t context line\n> \t -removed line for second parent\n> \t +added line for second parent\n> \n> or\n> \n> \t- removed line for first parent\n> \t -removed line for second parent\n> \t+ added line for first parent\n> \t +added line for second parent\n> \n> but following output will not:\n> \n> \t- removed line for first parent\n> \t -removed line for second parent\n> \t +added line for second parent\n> \t++added line for both parents\n> \nThat is a very reasonable approach.\n\n> Further changes may introduce more intelligent approach that better\n> handles combined diffs.\n> \n> Signed-off-by: Michał Kiedrowicz \n> ---\n> gitweb/gitweb.perl | 40 +++++++++++++++++++++++++++++++++++++---\n> 1 files changed, 37 insertions(+), 3 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 1a5b454..2b6cb9e 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -4944,13 +4944,16 @@ sub esc_html_mark_range {\n> \n> # Format removed and added line, mark changed part and HTML-format them.\n> sub format_rem_add_line {\n> -\tmy ($rem, $add) = @_;\n> +\tmy ($rem, $add, $is_combined) = @_;\n> \tmy @r = split(//, $rem);\n> \tmy @a = split(//, $add);\n> \tmy ($esc_rem, $esc_add);\n> \tmy ($prefix, $suffix_rem, $suffix_add) = (1, $#r, $#a);\n> \tmy ($prefix_is_space, $suffix_is_space) = (1, 1);\n> \n> +\t# In combined diff we must ignore two +/- characters.\n> +\t$prefix = 2 if ($is_combined);\n> +\n\nErrr... actually number of prefix is equalt to number of parents, so\nit might be in case of octopus merge more than 2.\n\n> \twhile ($prefix < @r && $prefix < @a) {\n> \t\tlast if ($r[$prefix] ne $a[$prefix]);\n> \n> @@ -4988,11 +4991,42 @@ sub format_ctx_rem_add_lines {\n> \tmy ($ctx, $rem, $add, $is_combined) = @_;\n> \tmy (@new_ctx, @new_rem, @new_add);\n> \tmy $num_add_lines = @$add;\n> +\tmy $can_highlight;\n> +\n> +\t# Highlight if every removed line has a corresponding added line.\n> +\tif ($num_add_lines > 0 && $num_add_lines == @$rem) {\n> +\t\t$can_highlight = 1;\n> +\n> +\t\t# Highlight lines in combined diff only if the chunk contains\n> +\t\t# diff between the same version, e.g.\n> +\t\t#\n> +\t\t# - a\n> +\t\t# - b\n> +\t\t# + c\n> +\t\t# + d\n> +\t\t#\n> +\t\t# Otherwise the highlightling would be confusing.\n> +\t\tif ($is_combined) {\n> +\t\t\tfor (my $i = 0; $i < $num_add_lines; $i++) {\n> +\t\t\t\tmy $prefix_rem = substr($rem->[$i], 0, 2);\n> +\t\t\t\tmy $prefix_add = substr($add->[$i], 0, 2);\n> +\n> +\t\t\t\t$prefix_rem =~ s/-/+/g;\n> +\n> +\t\t\t\tif ($prefix_rem ne $prefix_add) {\n> +\t\t\t\t\t$can_highlight = 0;\n> +\t\t\t\t\tlast;\n\nNb. this assumes that we cannot refine and highlight something like\nthis:\n\n \t\t# - a\n \t\t# - b\n \t\t# + c\n \t\t# ++ d\n\nBut perhaps this is better left for future improvemnt.\n\n> +\t\t\t\t}\n> +\t\t\t}\n> +\t\t}\n> +\t} else {\n> +\t\t$can_highlight = 0;\n> +\t}\n\nThis 'else' would be not necessary if $can_highlight was initialized\nto 0.\n\n> \n> -\tif (!$is_combined && $num_add_lines > 0 && $num_add_lines == @$rem) {\n> +\tif ($can_highlight) {\n> \t\tfor (my $i = 0; $i < $num_add_lines; $i++) {\n> \t\t\tmy ($line_rem, $line_add) = format_rem_add_line(\n> -\t\t\t\t$rem->[$i], $add->[$i]);\n> +\t\t\t\t$rem->[$i], $add->[$i], $is_combined);\n> \t\t\tpush @new_rem, $line_rem;\n> \t\t\tpush @new_add, $line_add;\n\nO.K.\n\n-- \nJakub Narębski\n"},{"id":"184491","messageId":"m3pqdksyg1.fsf@localhost.localdomain","threadId":"29593","inReplyTo":"1328865494-24415-8-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH 7/8] gitweb: Use different colors to present marked changes","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-12T00:11:11Z","receivedAt":"2012-02-12T00:11:11Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Michał Kiedrowicz writes:\n\n> This makes use of the highlight diff feature.\n> \n> Signed-off-by: Michał Kiedrowicz \n> ---\n> I decided to split mechanism (generate HTML page with elements that\n> mark interesting fragments of diff output) from politics (use these\n> particular colors for this elements), but otherwise this commit\n> may be squashed with the previous one. These colors work for me but if\n> someone comes out with better ones, I'd be happy.\n\nI think it would be better squashed with previous patch, otherwise it\nis a bit not visible change...\n \n> gitweb/static/gitweb.css | 8 ++++++++\n> 1 files changed, 8 insertions(+), 0 deletions(-)\n> \n> diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css\n> index c7827e8..4f87d16 100644\n> --- a/gitweb/static/gitweb.css\n> +++ b/gitweb/static/gitweb.css\n> @@ -438,6 +438,10 @@ div.diff.add {\n> \tcolor: #008800;\n> }\n> \n> +div.diff.add span.marked {\n> +\tbackground-color: #77ff77;\n> +}\n> +\n> div.diff.from_file a.path,\n> div.diff.from_file {\n> \tcolor: #aa0000;\n> @@ -447,6 +451,10 @@ div.diff.rem {\n> \tcolor: #cc0000;\n> }\n> \n> +div.diff.rem span.marked {\n> +\tbackground-color: #ff7777;\n> +}\n> +\n> div.diff.chunk_header a,\n> div.diff.chunk_header {\n> \tcolor: #990099;\n> -- \n\nI'd have to see those colors in use. BTW what colors other\nhighlighting diff GUIs use?\n\n-- \nJakub Narębski\n"},{"id":"184506","messageId":"m3lio8s57v.fsf@localhost.localdomain","threadId":"29593","inReplyTo":"m3y5s9rl3g.fsf@localhost.localdomain","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-12T10:42:34Z","receivedAt":"2012-02-12T10:42:34Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Jakub Narebski writes:\n> Michał Kiedrowicz writes:\n \n> > +# Highlight characters from $prefix to $suffix and escape HTML.\n> > +# $str is a reference to the array of characters.\n> > +sub esc_html_mark_range {\n> > +\tmy ($str, $prefix, $suffix) = @_;\n> > +\n> > +\t# Don't generate empty element.\n> > +\tif ($prefix == $suffix + 1) {\n> > +\t\treturn esc_html(join('', @$str), -nbsp=>1);\n> > +\t}\n> > +\n> > +\tmy $before = join('', @{$str}[0..($prefix - 1)]);\n> > +\tmy $marked = join('', @{$str}[$prefix..$suffix]);\n> > +\tmy $after = join('', @{$str}[($suffix + 1)..$#{$str}]);\n> \n> Eeeeeek! First you split into letters, in caller at that, then join?\n> Why not pass striung ($str suggests string not array of characters),\n> and use substr instead?\n> \n> [Please disregard this and the next paragraph at first reading]\n> \n> > +\n> > +\treturn esc_html($before, -nbsp=>1) .\n> > +\t\t$cgi->span({-class=>'marked'}, esc_html($marked, -nbsp=>1)) .\n> > +\t\tesc_html($after,-nbsp=>1);\n> > +}\n> \n> Anyway I have send to git mailing list a patch series, which in one of\n> patches adds esc_html_match_hl($str, $regexp) to highlight matches in\n> a string. Your esc_html_mark_range(), after a generalization, could\n> be used as underlying \"engine\".\n> \n> Something like this, perhaps (untested):\n> \n> # Highlight selected fragments of string, using given CSS class,\n> # and escape HTML. It is assumed that fragments do not overlap.\n> # Regions are passed as list of pairs (array references).\n> sub esc_html_hl {\n> my ($str, $css_class, @sel) = @_;\n> return esc_html($str) unless @sel;\n> \n> my $out = '';\n> my $pos = 0;\n> \n> for my $s (@sel) {\n> $out .= esc_html(substr($str, $pos, $s->[0] - $pos))\n> if ($s->[0] - $pos > 0);\n> $out .= $cgi->span({-class => $css_class},\n> esc_html(substr($str, $s->[0], $s->[1] - $s->[0])));\n> \n> $pos = $m->[1];\n> }\n> $out .= esc_html(substr($str, $pos))\n> if ($pos < length($str));\n> \n> return $out;\n> }\n\nActually we can accomodate both operating on string and operating on\narray of characters in a single subroutine. Though it can be left for\nlater commit, anyway...\n\n # Highlight selected fragments of string, using given CSS class,\n # and escape HTML. It is assumed that fragments do not overlap.\n # Regions are passed as list of pairs (array references).\n sub esc_html_hl {\n my ($sth, $css_class, @sel) = @_;\n\n if (!@sel) {\n if (ref($sth) eq \"ARRAY\") {\n return esc_html(join('', @$sth), -nbsp=>1);\n } else {\n return esc_html($sth, -nbsp=>1);\n }\n\n if (ref($sth) eq \"ARRAY\") {\n return esc_html_hl_gen($sth,\n sub { \n my ($arr, $from, $to) = @_;\n return join('', @{$arr}[$from..$to]);\n },\n scalar @{$arr}, $css_class, @sel);\n } else {\n return esc_html_hl_gen($sth,\n sub {\n my ($str, $from, $to) = @_;\n if ($to < 0) { $to += lenght($str); };\n return substr($str, $from, $to - $from);\n },\n length($sth), $css_class, @sel);\n }\n }\n\n # Highlight selected fragments of string or array of characters\n # with given length, using provided $extr subroutine to extract\n # fragment (substring)\n sub esc_html_hl_gen {\n my ($sth, $extr, $len, $css_class, @sel) = @_;\n \n my $out = '';\n my $pos = 0;\n \n for my $s (@sel) {\n $out .= esc_html($extr->($str, $pos, $s->[0]))\n if ($s->[0] - $pos > 0);\n $out .= $cgi->span({-class => $css_class},\n esc_html($extr->($str, $s->[0], $s->[1])));\n \n $pos = $s->[1];\n }\n $out .= esc_html($extr->($str, $pos, $len))\n if ($pos < $len);\n \n return $out;\n }\n\nOr maybe I have read \"Higher-Order Perl\" one time too many ;-))))\n\n-- \nJakub Narębski\n"},{"id":"184549","messageId":"20120213074125.3e8793ad@mkiedrowicz.ivo.pl","threadId":"29593","inReplyTo":"m3y5s9rl3g.fsf@localhost.localdomain","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Michal Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-13T06:41:25Z","receivedAt":"2012-02-13T06:41:25Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jakub Narebski wrote:\n\n> Michał Kiedrowicz writes:\n> \n> > Reading diff output is sometimes very hard, even if it's colored,\n> > especially if lines differ only in few characters. This is often\n> > true when a commit fixes a typo or renames some variables or\n> > functions.\n> >\n> This is certainly nice feature to have. I think most tools that\n> implement side-by-side diff implement this too.\n> \n> > This commit teaches gitweb to highlight characters that are\n> > different between old and new line. This should work in the\n> > similar manner as in Trac or GitHub.\n> >\n> Doe you have URLs with good examples of such diff refinement\n> highlighting (GNU Emacs ediff/emerge terminology) / highlighting\n> differing segments (diff-highlight terminology)?\n\nI haven't found *examples* on GitHub and Trac sites, but what about\nthese ones:\n\nhttps://github.com/gitster/git/commit/8cad4744ee37ebec1d9491a1381ec1771a1ba795\nhttp://trac.edgewall.org/changeset/10973\n\n> \n> > The code that compares lines is based on\n> > contrib/diff-highlight/diff-highlight, except that it works with\n> > multiline changes too. It also won't highlight lines that are\n> > completely different because that would only make the output\n> > unreadable.\n> >\n> So what does it look like? From the contrib/diff-highlight/README I\n> guess that it finds common prefix and common suffix, and highlights\n> the rest, which includes change. \n\nYes.\n\n> It doesn't implement LCS / diff\n> algorithm like e.g. tkdiff does for its diff refinement highlighting,\n> isn't it?\n> \n\nI doesn't. I share the Jeff's opinion that:\na) Jeff's approach is \"good enough\"\nb) LCS on bytes could be very confusing if it marked few sets of\ncharacters.\n\n> By completly different you mean that they do not have common prefix or\n> common suffix (at least one of them), isn't it?\n\nYes, but I also don't highlight lines which prefix/suffix consists only\nof whitespace (This is quite common). I would also consider ignoring\nprefixes/suffixes with punctuation, like:\n\n\t- * I like you.\n\t+ * Alice had a little lamb.\n\n> \n> > Combined diffs are not supported but a following commit will change\n> > it.\n> > \n> O.K.\n> \n> > Signed-off-by: Michał Kiedrowicz \n> > ---\n> > gitweb/gitweb.perl | 82\n> > ++++++++++++++++++++++++++++++++++++++++++++++++--- 1 files\n> > changed, 77 insertions(+), 5 deletions(-)\n> > \n> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> > index db61553..1a5b454 100755\n> > --- a/gitweb/gitweb.perl\n> > +++ b/gitweb/gitweb.perl\n> > @@ -2322,7 +2322,7 @@ sub format_cc_diff_chunk_header {\n> > # wrap patch (diff) line into a
(not to be used for diff\n> > headers), # the line must be esc_html()'ed\n> > sub format_diff_line {\n> > -\tmy ($line, $diff_class, $from, $to) = @_;\n> > +\tmy ($line, $diff_class) = @_;\n> \n> Why that change?\n\nI think it fallout from previous patch where format_diff_line() stopped\nusing $from and $to. Will fix that.\n\n> \n> > \n> > \tmy $diff_classes = \"diff\";\n> > \t$diff_classes .= \" $diff_class\" if ($diff_class);\n> > @@ -4923,14 +4923,85 @@ sub print_inline_diff_lines {\n> > \tprint foreach (@$add);\n> > }\n> > \n> > +# Highlight characters from $prefix to $suffix and escape HTML.\n> > +# $str is a reference to the array of characters.\n> > +sub esc_html_mark_range {\n> > +\tmy ($str, $prefix, $suffix) = @_;\n> > +\n> > +\t# Don't generate empty element.\n> > +\tif ($prefix == $suffix + 1) {\n> > +\t\treturn esc_html(join('', @$str), -nbsp=>1);\n> > +\t}\n> > +\n> > +\tmy $before = join('', @{$str}[0..($prefix - 1)]);\n> > +\tmy $marked = join('', @{$str}[$prefix..$suffix]);\n> > +\tmy $after = join('', @{$str}[($suffix + 1)..$#{$str}]);\n> \n> Eeeeeek! First you split into letters, in caller at that, then join?\n> Why not pass striung ($str suggests string not array of characters),\n> and use substr instead?\n> \n> [Please disregard this and the next paragraph at first reading]\n\nI will rename $str to something more self describing.\n\n> \n> > +\n> > +\treturn esc_html($before, -nbsp=>1) .\n> > +\t\t$cgi->span({-class=>'marked'}, esc_html($marked,\n> > -nbsp=>1)) .\n> > +\t\tesc_html($after,-nbsp=>1);\n> > +}\n> \n> Anyway I have send to git mailing list a patch series, which in one of\n> patches adds esc_html_match_hl($str, $regexp) to highlight matches in\n> a string. Your esc_html_mark_range(), after a generalization, could\n> be used as underlying \"engine\".\n> \n> Something like this, perhaps (untested):\n> \n> # Highlight selected fragments of string, using given CSS class,\n> # and escape HTML. It is assumed that fragments do not overlap.\n> # Regions are passed as list of pairs (array references).\n> sub esc_html_hl {\n> my ($str, $css_class, @sel) = @_;\n> return esc_html($str) unless @sel;\n> \n> my $out = '';\n> my $pos = 0;\n> \n> for my $s (@sel) {\n> $out .= esc_html(substr($str, $pos, $s->[0] - $pos))\n> if ($s->[0] - $pos > 0);\n> $out .= $cgi->span({-class => $css_class},\n> esc_html(substr($str, $s->[0],\n> $s->[1] - $s->[0])));\n> \n> $pos = $m->[1];\n> }\n> $out .= esc_html(substr($str, $pos))\n> if ($pos < length($str));\n> \n> return $out;\n> }\n> \n> > +\n> > +# Format removed and added line, mark changed part and HTML-format\n> > them.\n> \n> You should probably ad here that this code is taken from\n> diff-highlight in contrib area, isn't it?\n\nTrue.\n\n> \n> > +sub format_rem_add_line {\n> > +\tmy ($rem, $add) = @_;\n> > +\tmy @r = split(//, $rem);\n> > +\tmy @a = split(//, $add);\n> > +\tmy ($esc_rem, $esc_add);\n> > +\tmy ($prefix, $suffix_rem, $suffix_add) = (1, $#r, $#a);\n> \n> It is not as much $prefix, as $prefix_len, isn't it? \n\nYes.\n\n> Shouldn't\n> $prefix / $prefix_len start from 0, not from 1?\n\nIt starts from 1 because it skips first +/-. It should become obvious\nafter reading the comment from last patch :).\n\n+\t# In combined diff we must ignore two +/- characters.\n+\t$prefix = 2 if ($is_combined);\n+\n\n> \n> > +\tmy ($prefix_is_space, $suffix_is_space) = (1, 1);\n> > +\n> > +\twhile ($prefix < @r && $prefix < @a) {\n> > +\t\tlast if ($r[$prefix] ne $a[$prefix]);\n> > +\n> > +\t\t$prefix_is_space = 0 if ($r[$prefix] !~ /\\s/);\n> > +\t\t$prefix++;\n> > +\t}\n> \n> Ah, I see that it is easier to find common prefix by treating string\n> as array of characters.\n> \n> Though I wonder if it wouldn't be easier to use trick of XOR-ing two\n> strings (see \"Bitwise String Operators\" in perlop(1)):\n> \n> my $xor = \"$rem\" ^ \"$add\";\n> \n> and finding starting sequence of \"\\0\", which denote common prefix.\n> \n> \n> Though this and the following is a nice implementation of\n> algorithm... as it would be implemented in C. Nevermind, it might be\n> good enough...\n\nThe splitting and comparing by characters is taken from diff-highlight.\nI don't think it's worth changing here.\n\n> \n> > +\n> > +\twhile ($suffix_rem >= $prefix && $suffix_add >= $prefix) {\n> > +\t\tlast if ($r[$suffix_rem] ne $a[$suffix_add]);\n> > +\n> > +\t\t$suffix_is_space = 0 if ($r[$suffix_rem] !~ /\\s/);\n> > +\t\t$suffix_rem--;\n> > +\t\t$suffix_add--;\n> > +\t}\n> \n> BTW., perhaps using single negative $suffix_len instead of separate\n> $suffix_rem_pos and $suffix_add_pos would make code simpler and easier\n> to understand?\n\nI'll try that.\n\n> \n> > +\n> > +\t# Mark lines that are different from each other, but have\n> > some common\n> > +\t# part that isn't whitespace. If lines are completely\n> > different, don't\n> > +\t# mark them because that would make output unreadable,\n> > especially if\n> > +\t# diff consists of multiple lines.\n> > +\tif (($prefix == 1 && $suffix_rem == $#r && $suffix_add ==\n> > $#a)\n> > +\t\t|| ($prefix_is_space && $suffix_is_space)) {\n> \n> Micronit about style: in gitweb we put boolean operator at the end of\n> continued line, not at beginning of next one.\n> \n> So this would be:\n> \n> +\tif (($prefix == 1 && $suffix_rem == $#r && $suffix_add ==\n> $#a) ||\n> +\t ($prefix_is_space && $suffix_is_space)) {\n> \n\nOK\n\n> > +\t\t$esc_rem = esc_html($rem);\n> > +\t\t$esc_add = esc_html($add);\n> > +\t} else {\n> > +\t\t$esc_rem = esc_html_mark_range(\\@r, $prefix,\n> > $suffix_rem);\n> > +\t\t$esc_add = esc_html_mark_range(\\@a, $prefix,\n> > $suffix_add);\n> > +\t}\n> > +\n> > +\treturn format_diff_line($esc_rem, 'rem'),\n> > +\t\tformat_diff_line($esc_add, 'add');\n> \n> Please use spaces to align.\n\nOK\n\n> \n> > +}\n> > +\n> > # HTML-format diff context, removed and added lines.\n> > sub format_ctx_rem_add_lines {\n> > -\tmy ($ctx, $rem, $add) = @_;\n> > +\tmy ($ctx, $rem, $add, $is_combined) = @_;\n> > \tmy (@new_ctx, @new_rem, @new_add);\n> > +\tmy $num_add_lines = @$add;\n> \n> Why is this temporary variable needed? If you are not sure that ==\n> operator enforces scalar context on both arguments you can always\n> write\n> \n> scalar @$add == scalar @$rem\n> \n\nYou read my mind.\n\n> > +\n> > +\tif (!$is_combined && $num_add_lines > 0 && $num_add_lines\n> > == @$rem) {\n> > +\t\tfor (my $i = 0; $i < $num_add_lines; $i++) {\n> > +\t\t\tmy ($line_rem, $line_add) =\n> > format_rem_add_line(\n> > +\t\t\t\t$rem->[$i], $add->[$i]);\n> > +\t\t\tpush @new_rem, $line_rem;\n> > +\t\t\tpush @new_add, $line_add;\n> \n> The original contrib/diff-highlight works only for single changed line\n> (single removed and single added). You make this code work also for\n> the case where number of aded lines is equal to the number of removed\n> lines, and assume that subsequent changed lines in preimage\n> correcponds to subsequent changed lines in postimage, which is not\n> always true:\n> \n> -foo\n> -bar\n> +baz\n> +fooo\n> \n> This is not described in commit message, I think.\n\nTrue. Note that in this particular case nothing should be highlighted\nbecause corresponding lines don't have common prefix/suffix.\n\n> \n> > +\t\t}\n> > +\t} else {\n> > +\t\t@new_rem = map { format_diff_line(esc_html($_,\n> > -nbsp=>1), 'rem') } @$rem;\n> > +\t\t@new_add = map { format_diff_line(esc_html($_,\n> > -nbsp=>1), 'add') } @$add;\n> > +\t}\n> > \n> > \t@new_ctx = map { format_diff_line(esc_html($_, -nbsp=>1),\n> > 'ctx') } @$ctx;\n> > -\t@new_rem = map { format_diff_line(esc_html($_, -nbsp=>1),\n> > 'rem') } @$rem;\n> > -\t@new_add = map { format_diff_line(esc_html($_, -nbsp=>1),\n> > 'add') } @$add; \n> > \treturn (\\@new_ctx, \\@new_rem, \\@new_add);\n> > }\n> > @@ -4939,7 +5010,8 @@ sub format_ctx_rem_add_lines {\n> > sub print_diff_lines {\n> > \tmy ($ctx, $rem, $add, $diff_style, $is_combined) = @_;\n> > \n> > -\t($ctx, $rem, $add) = format_ctx_rem_add_lines($ctx, $rem,\n> > $add);\n> > +\t($ctx, $rem, $add) = format_ctx_rem_add_lines($ctx, $rem,\n> > $add,\n> > +\t\t$is_combined);\n> \n> O.K., now the code depends on $is_combined\n> \n"},{"id":"184550","messageId":"20120213074609.2222bd6a@mkiedrowicz.ivo.pl","threadId":"29593","inReplyTo":"m3pqdksyg1.fsf@localhost.localdomain","subject":"Re: [PATCH 7/8] gitweb: Use different colors to present marked changes","fromName":"Michal Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-13T06:46:09Z","receivedAt":"2012-02-13T06:46:09Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jakub Narebski wrote:\n\n> Michał Kiedrowicz writes:\n> \n> > This makes use of the highlight diff feature.\n> > \n> > Signed-off-by: Michał Kiedrowicz \n> > ---\n> > I decided to split mechanism (generate HTML page with \n> > elements that mark interesting fragments of diff output) from\n> > politics (use these particular colors for this elements),\n> > but otherwise this commit may be squashed with the previous one.\n> > These colors work for me but if someone comes out with better ones,\n> > I'd be happy.\n> \n> I think it would be better squashed with previous patch, otherwise it\n> is a bit not visible change...\n\nOK, but please note that since previous patch HTML contains \nelements around differing segments of diff so the change exists. It\njust isn't reflected by CSS.\n\n> \n> > gitweb/static/gitweb.css | 8 ++++++++\n> > 1 files changed, 8 insertions(+), 0 deletions(-)\n> > \n> > diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css\n> > index c7827e8..4f87d16 100644\n> > --- a/gitweb/static/gitweb.css\n> > +++ b/gitweb/static/gitweb.css\n> > @@ -438,6 +438,10 @@ div.diff.add {\n> > \tcolor: #008800;\n> > }\n> > \n> > +div.diff.add span.marked {\n> > +\tbackground-color: #77ff77;\n> > +}\n> > +\n> > div.diff.from_file a.path,\n> > div.diff.from_file {\n> > \tcolor: #aa0000;\n> > @@ -447,6 +451,10 @@ div.diff.rem {\n> > \tcolor: #cc0000;\n> > }\n> > \n> > +div.diff.rem span.marked {\n> > +\tbackground-color: #ff7777;\n> > +}\n> > +\n> > div.diff.chunk_header a,\n> > div.diff.chunk_header {\n> > \tcolor: #990099;\n> > -- \n> \n> I'd have to see those colors in use. \n\nThen why don't you try it?\n\n> BTW what colors other\n> highlighting diff GUIs use?\n> \n\nAFAIK they just use darker red/green background than for the rest of\nline.\n"},{"id":"184551","messageId":"20120213074856.50b1bf37@mkiedrowicz.ivo.pl","threadId":"29593","inReplyTo":"m3ty2wsyt6.fsf@localhost.localdomain","subject":"Re: [PATCH 8/8] gitweb: Highlight combined diffs","fromName":"Michal Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-13T06:48:56Z","receivedAt":"2012-02-13T06:48:56Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jakub Narebski wrote:\n\n> Michał Kiedrowicz writes:\n> \n> > The highlightning of combined diffs is currently disabled. This is\n> > because output from a combined diff is much harder to highlight\n> > because it's not obvious which removed and added lines should be\n> > compared.\n> > \n> > Moreover, code that compares added and removed lines doesn't care\n> > about combined diffs. It only skips first +/- character, treating\n> > second +/- as a line content.\n> > \n> > Let's start with a simple case: only highlight changes that come\n> > from one parent, i.e. when every removed line has a corresponding\n> > added line for the same parent. This way the highlightning cannot\n> > get wrong. For example, following diffs would be highlighted:\n> > \n> > \t- removed line for first parent\n> > \t+ added line for first parent\n> > \t context line\n> > \t -removed line for second parent\n> > \t +added line for second parent\n> > \n> > or\n> > \n> > \t- removed line for first parent\n> > \t -removed line for second parent\n> > \t+ added line for first parent\n> > \t +added line for second parent\n> > \n> > but following output will not:\n> > \n> > \t- removed line for first parent\n> > \t -removed line for second parent\n> > \t +added line for second parent\n> > \t++added line for both parents\n> > \n> That is a very reasonable approach.\n\nThanks.\n\n> \n> > Further changes may introduce more intelligent approach that better\n> > handles combined diffs.\n> > \n> > Signed-off-by: Michał Kiedrowicz \n> > ---\n> > gitweb/gitweb.perl | 40 +++++++++++++++++++++++++++++++++++++---\n> > 1 files changed, 37 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> > index 1a5b454..2b6cb9e 100755\n> > --- a/gitweb/gitweb.perl\n> > +++ b/gitweb/gitweb.perl\n> > @@ -4944,13 +4944,16 @@ sub esc_html_mark_range {\n> > \n> > # Format removed and added line, mark changed part and HTML-format\n> > them. sub format_rem_add_line {\n> > -\tmy ($rem, $add) = @_;\n> > +\tmy ($rem, $add, $is_combined) = @_;\n> > \tmy @r = split(//, $rem);\n> > \tmy @a = split(//, $add);\n> > \tmy ($esc_rem, $esc_add);\n> > \tmy ($prefix, $suffix_rem, $suffix_add) = (1, $#r, $#a);\n> > \tmy ($prefix_is_space, $suffix_is_space) = (1, 1);\n> > \n> > +\t# In combined diff we must ignore two +/- characters.\n> > +\t$prefix = 2 if ($is_combined);\n> > +\n> \n> Errr... actually number of prefix is equalt to number of parents, so\n> it might be in case of octopus merge more than 2.\n\nOK\n\n> \n> > \twhile ($prefix < @r && $prefix < @a) {\n> > \t\tlast if ($r[$prefix] ne $a[$prefix]);\n> > \n> > @@ -4988,11 +4991,42 @@ sub format_ctx_rem_add_lines {\n> > \tmy ($ctx, $rem, $add, $is_combined) = @_;\n> > \tmy (@new_ctx, @new_rem, @new_add);\n> > \tmy $num_add_lines = @$add;\n> > +\tmy $can_highlight;\n> > +\n> > +\t# Highlight if every removed line has a corresponding\n> > added line.\n> > +\tif ($num_add_lines > 0 && $num_add_lines == @$rem) {\n> > +\t\t$can_highlight = 1;\n> > +\n> > +\t\t# Highlight lines in combined diff only if the\n> > chunk contains\n> > +\t\t# diff between the same version, e.g.\n> > +\t\t#\n> > +\t\t# - a\n> > +\t\t# - b\n> > +\t\t# + c\n> > +\t\t# + d\n> > +\t\t#\n> > +\t\t# Otherwise the highlightling would be confusing.\n> > +\t\tif ($is_combined) {\n> > +\t\t\tfor (my $i = 0; $i < $num_add_lines; $i++)\n> > {\n> > +\t\t\t\tmy $prefix_rem =\n> > substr($rem->[$i], 0, 2);\n> > +\t\t\t\tmy $prefix_add =\n> > substr($add->[$i], 0, 2); +\n> > +\t\t\t\t$prefix_rem =~ s/-/+/g;\n> > +\n> > +\t\t\t\tif ($prefix_rem ne $prefix_add) {\n> > +\t\t\t\t\t$can_highlight = 0;\n> > +\t\t\t\t\tlast;\n> \n> Nb. this assumes that we cannot refine and highlight something like\n> this:\n> \n> \t\t# - a\n> \t\t# - b\n> \t\t# + c\n> \t\t# ++ d\n> \n> But perhaps this is better left for future improvemnt.\n\nI can add a patch for that at the end of this series.\n\n> \n> > +\t\t\t\t}\n> > +\t\t\t}\n> > +\t\t}\n> > +\t} else {\n> > +\t\t$can_highlight = 0;\n> > +\t}\n> \n> This 'else' would be not necessary if $can_highlight was initialized\n> to 0.\n> \n\nOK.\n\n> > \n> > -\tif (!$is_combined && $num_add_lines > 0 && $num_add_lines\n> > == @$rem) {\n> > +\tif ($can_highlight) {\n> > \t\tfor (my $i = 0; $i < $num_add_lines; $i++) {\n> > \t\t\tmy ($line_rem, $line_add) =\n> > format_rem_add_line(\n> > -\t\t\t\t$rem->[$i], $add->[$i]);\n> > +\t\t\t\t$rem->[$i], $add->[$i],\n> > $is_combined); push @new_rem, $line_rem;\n> > \t\t\tpush @new_add, $line_add;\n> \n> O.K.\n> \n\nThanks for your thorough review.\n"},{"id":"184552","messageId":"20120213075451.1bc20885@mkiedrowicz.ivo.pl","threadId":"29593","inReplyTo":"m3lio8s57v.fsf@localhost.localdomain","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Michal Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-13T06:54:51Z","receivedAt":"2012-02-13T06:54:51Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jakub Narebski wrote:\n\n> Jakub Narebski writes:\n> > Michał Kiedrowicz writes:\n> \n> > > +# Highlight characters from $prefix to $suffix and escape HTML.\n> > > +# $str is a reference to the array of characters.\n> > > +sub esc_html_mark_range {\n> > > +\tmy ($str, $prefix, $suffix) = @_;\n> > > +\n> > > +\t# Don't generate empty element.\n> > > +\tif ($prefix == $suffix + 1) {\n> > > +\t\treturn esc_html(join('', @$str), -nbsp=>1);\n> > > +\t}\n> > > +\n> > > +\tmy $before = join('', @{$str}[0..($prefix - 1)]);\n> > > +\tmy $marked = join('', @{$str}[$prefix..$suffix]);\n> > > +\tmy $after = join('', @{$str}[($suffix + 1)..$#{$str}]);\n> > \n> > Eeeeeek! First you split into letters, in caller at that, then\n> > join? Why not pass striung ($str suggests string not array of\n> > characters), and use substr instead?\n> > \n> > [Please disregard this and the next paragraph at first reading]\n> > \n> > > +\n> > > +\treturn esc_html($before, -nbsp=>1) .\n> > > +\t\t$cgi->span({-class=>'marked'}, esc_html($marked,\n> > > -nbsp=>1)) .\n> > > +\t\tesc_html($after,-nbsp=>1);\n> > > +}\n> > \n> > Anyway I have send to git mailing list a patch series, which in one\n> > of patches adds esc_html_match_hl($str, $regexp) to highlight\n> > matches in a string. \n\nYeah, I saw that but after seeing that they accept different arguments\nI decided to leave them alone.\n\n> Your esc_html_mark_range(), after a\n> > generalization, could be used as underlying \"engine\".\n> > \n> > Something like this, perhaps (untested):\n\nI think I'll leave it to you after merging both these series to\nmaster :)\n\n> > \n> > # Highlight selected fragments of string, using given CSS class,\n> > # and escape HTML. It is assumed that fragments do not overlap.\n> > # Regions are passed as list of pairs (array references).\n> > sub esc_html_hl {\n> > my ($str, $css_class, @sel) = @_;\n> > return esc_html($str) unless @sel;\n> > \n> > my $out = '';\n> > my $pos = 0;\n> > \n> > for my $s (@sel) {\n> > $out .= esc_html(substr($str, $pos, $s->[0] - $pos))\n> > if ($s->[0] - $pos > 0);\n> > $out .= $cgi->span({-class => $css_class},\n> > esc_html(substr($str, $s->[0],\n> > $s->[1] - $s->[0])));\n> > \n> > $pos = $m->[1];\n> > }\n> > $out .= esc_html(substr($str, $pos))\n> > if ($pos < length($str));\n> > \n> > return $out;\n> > }\n> \n> Actually we can accomodate both operating on string and operating on\n> array of characters in a single subroutine. Though it can be left for\n> later commit, anyway...\n\n> \n> # Highlight selected fragments of string, using given CSS class,\n> # and escape HTML. It is assumed that fragments do not overlap.\n> # Regions are passed as list of pairs (array references).\n> sub esc_html_hl {\n> my ($sth, $css_class, @sel) = @_;\n> \n> if (!@sel) {\n> if (ref($sth) eq \"ARRAY\") {\n> return esc_html(join('', @$sth), -nbsp=>1);\n> } else {\n> return esc_html($sth, -nbsp=>1);\n> }\n> \n> if (ref($sth) eq \"ARRAY\") {\n> return esc_html_hl_gen($sth,\n> sub { \n> my ($arr, $from, $to) = @_;\n> return join('', @{$arr}[$from..$to]);\n> },\n> scalar @{$arr}, $css_class, @sel);\n> } else {\n> return esc_html_hl_gen($sth,\n> sub {\n> my ($str, $from, $to) = @_;\n> if ($to < 0) { $to += lenght($str); };\n> return substr($str, $from, $to -\n> $from); },\n> length($sth), $css_class, @sel);\n> }\n> }\n> \n> # Highlight selected fragments of string or array of characters\n> # with given length, using provided $extr subroutine to extract\n> # fragment (substring)\n> sub esc_html_hl_gen {\n> my ($sth, $extr, $len, $css_class, @sel) = @_;\n> \n> my $out = '';\n> my $pos = 0;\n> \n> for my $s (@sel) {\n> $out .= esc_html($extr->($str, $pos, $s->[0]))\n> if ($s->[0] - $pos > 0);\n> $out .= $cgi->span({-class => $css_class},\n> esc_html($extr->($str, $s->[0],\n> $s->[1]))); \n> $pos = $s->[1];\n> }\n> $out .= esc_html($extr->($str, $pos, $len))\n> if ($pos < $len);\n> \n> return $out;\n> }\n> \n> Or maybe I have read \"Higher-Order Perl\" one time too many ;-))))\n> \n"},{"id":"184580","messageId":"201202131944.50886.jnareb@gmail.com","threadId":"29593","inReplyTo":"20120213074125.3e8793ad@mkiedrowicz.ivo.pl","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-13T18:44:50Z","receivedAt":"2012-02-13T18:44:50Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 13 Feb 2012, Michal Kiedrowicz wrote:\n> Jakub Narebski wrote:\n> \n> > Michał Kiedrowicz writes:\n> > \n> > > Reading diff output is sometimes very hard, even if it's colored,\n> > > especially if lines differ only in few characters. This is often\n> > > true when a commit fixes a typo or renames some variables or\n> > > functions.\n> > >\n> > This is certainly nice feature to have. I think most tools that\n> > implement side-by-side diff implement this too.\n> > \n> > > This commit teaches gitweb to highlight characters that are\n> > > different between old and new line. This should work in the\n> > > similar manner as in Trac or GitHub.\n> > >\n> > Doe you have URLs with good examples of such diff refinement\n> > highlighting (GNU Emacs ediff/emerge terminology) / highlighting\n> > differing segments (diff-highlight terminology)?\n> \n> I haven't found *examples* on GitHub and Trac sites, but what about\n> these ones:\n> \n> https://github.com/gitster/git/commit/8cad4744ee37ebec1d9491a1381ec1771a1ba795\n> http://trac.edgewall.org/changeset/10973\n\nThanks. That is what I meant by \"good examples\". Perhaps they should\nbe put in the commit message?\n\nBTW GitHub is closed source, but we can check what algorithm does Trac\nuse for diff refinement highlighting (highlighting changed portions of\ndiff).\n\n> > > The code that compares lines is based on\n> > > contrib/diff-highlight/diff-highlight, except that it works with\n> > > multiline changes too. It also won't highlight lines that are\n> > > completely different because that would only make the output\n> > > unreadable.\n> > >\n> > So what does it look like? From the contrib/diff-highlight/README I\n> > guess that it finds common prefix and common suffix, and highlights\n> > the rest, which includes change. \n> \n> Yes.\n> \n> > It doesn't implement LCS / diff\n> > algorithm like e.g. tkdiff does for its diff refinement highlighting,\n> > isn't it?\n> \n> I doesn't. I share the Jeff's opinion that:\n> a) Jeff's approach is \"good enough\"\n> b) LCS on bytes could be very confusing if it marked few sets of\n> characters.\n\nI wonder if we can use --diff-words for diff refinement highlighting,\ni.e. LCS on words.\n\nAnyway Jeff's approach is a bit limited, in that it would work only for\nchange that does not involve adding newlines, for example splitting\noverly long line when changing something.\n\nSee for example line 1786 (in pre-image) in http://trac.edgewall.org/changeset/10973\n \n> > By completly different you mean that they do not have common prefix or\n> > common suffix (at least one of them), isn't it?\n\nBTW. is it \"at least one of prefix or suffix are non-empty\" or \"both prefix\nand suffix are non-empty\"?\n\n> Yes, but I also don't highlight lines which prefix/suffix consists only\n> of whitespace (This is quite common). \n\nO.K., that is quite sensible.\n\n> I would also consider ignoring prefixes/suffixes with punctuation, like:\n> \n> \t- * I like you.\n> \t+ * Alice had a little lamb.\n\nBut this patch doesn't implement this feature yet, isn't it?\n\nWell, here is another idea: do not highlight if sum of prefix and suffix\nlengths are less than some threshold, e.g. 2 characters not including \nwhitespace, or some percentage with respect to total line length.\n\n> > > +# Highlight characters from $prefix to $suffix and escape HTML.\n> > > +# $str is a reference to the array of characters.\n> > > +sub esc_html_mark_range {\n> > > +\tmy ($str, $prefix, $suffix) = @_;\n> > > +\n> > > +\t# Don't generate empty element.\n> > > +\tif ($prefix == $suffix + 1) {\n> > > +\t\treturn esc_html(join('', @$str), -nbsp=>1);\n> > > +\t}\n> > > +\n> > > +\tmy $before = join('', @{$str}[0..($prefix - 1)]);\n> > > +\tmy $marked = join('', @{$str}[$prefix..$suffix]);\n> > > +\tmy $after = join('', @{$str}[($suffix + 1)..$#{$str}]);\n\nIt would be nicer with 'part' from List::MoreUtils... but that module\nis unfortunately not in core.\n\n> > Eeeeeek! First you split into letters, in caller at that, then join?\n> > Why not pass striung ($str suggests string not array of characters),\n> > and use substr instead?\n> > \n> > [Please disregard this and the next paragraph at first reading]\n> \n> I will rename $str to something more self describing.\n\nPlease do.\n\nBTW. don't you assume here that both common prefix and common suffix\nare non-empty?\n \n[...]\n> > > +\n> > > +# Format removed and added line, mark changed part and HTML-format\n> > > them.\n> > \n> > You should probably add here that this code is taken from\n> > diff-highlight in contrib area, isn't it?\n> \n> True.\n\nAnd perhaps not the changes from diff-highlight, unless you meant to\nsend them upstream for inclusion.\n\n> > > +sub format_rem_add_line {\n> > > +\tmy ($rem, $add) = @_;\n> > > +\tmy @r = split(//, $rem);\n> > > +\tmy @a = split(//, $add);\n\nBTW the name of variable can be just @add and @rem.\n\n> > > +\tmy ($esc_rem, $esc_add);\n> > > +\tmy ($prefix, $suffix_rem, $suffix_add) = (1, $#r, $#a);\n> > \n> > It is not as much $prefix, as $prefix_len, isn't it? \n> \n> Yes.\n> \n> > Shouldn't\n> > $prefix / $prefix_len start from 0, not from 1?\n> \n> It starts from 1 because it skips first +/-. It should become obvious\n> after reading the comment from last patch :).\n> \n> +\t# In combined diff we must ignore two +/- characters.\n> +\t$prefix = 2 if ($is_combined);\n\nAnyway comment about that fact would be nice.\n \n> > > +\tmy ($prefix_is_space, $suffix_is_space) = (1, 1);\n> > > +\n> > > +\twhile ($prefix < @r && $prefix < @a) {\n> > > +\t\tlast if ($r[$prefix] ne $a[$prefix]);\n> > > +\n> > > +\t\t$prefix_is_space = 0 if ($r[$prefix] !~ /\\s/);\n> > > +\t\t$prefix++;\n> > > +\t}\n> > \n> > Ah, I see that it is easier to find common prefix by treating string\n> > as array of characters.\n> > \n> > Though I wonder if it wouldn't be easier to use trick of XOR-ing two\n> > strings (see \"Bitwise String Operators\" in perlop(1)):\n> > \n> > my $xor = \"$rem\" ^ \"$add\";\n> > \n> > and finding starting sequence of \"\\0\", which denote common prefix.\n> > \n> > \n> > Though this and the following is a nice implementation of\n> > algorithm... as it would be implemented in C. Nevermind, it might be\n> > good enough...\n> \n> The splitting and comparing by characters is taken from diff-highlight.\n> I don't think it's worth changing here.\n\nYou are right.\n\nI'll try to come with hacky algorithm using string bitwise xor and regexp,\nand benchmark it comparing to your C-like solution, but it can be left for\nlater (simple is better than clever, usually).\n \n> > > +\twhile ($suffix_rem >= $prefix && $suffix_add >= $prefix) {\n> > > +\t\tlast if ($r[$suffix_rem] ne $a[$suffix_add]);\n> > > +\n> > > +\t\t$suffix_is_space = 0 if ($r[$suffix_rem] !~ /\\s/);\n> > > +\t\t$suffix_rem--;\n> > > +\t\t$suffix_add--;\n> > > +\t}\n> > \n> > BTW., perhaps using single negative $suffix_len instead of separate\n> > $suffix_rem_pos and $suffix_add_pos would make code simpler and easier\n> > to understand?\n> \n> I'll try that.\n\nI think this should just work, and it would be one variable less to\ntrack.\n \n> > > # HTML-format diff context, removed and added lines.\n> > > sub format_ctx_rem_add_lines {\n> > > -\tmy ($ctx, $rem, $add) = @_;\n> > > +\tmy ($ctx, $rem, $add, $is_combined) = @_;\n> > > \tmy (@new_ctx, @new_rem, @new_add);\n> > > +\tmy $num_add_lines = @$add;\n> > \n> > Why is this temporary variable needed? If you are not sure that ==\n> > operator enforces scalar context on both arguments you can always\n> > write\n> > \n> > scalar @$add == scalar @$rem\n> \n> You read my mind.\n\nBTW. the same comment applies to patch adding support for highlighting\nchanged part in combined diff.\n \n> > The original contrib/diff-highlight works only for single changed line\n> > (single removed and single added). You make this code work also for\n> > the case where number of aded lines is equal to the number of removed\n> > lines, and assume that subsequent changed lines in preimage\n> > correcponds to subsequent changed lines in postimage, [...]\n[...]\n> > This is not described in commit message, I think.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"184591","messageId":"201202132058.18001.jnareb@gmail.com","threadId":"29593","inReplyTo":"20120213075451.1bc20885@mkiedrowicz.ivo.pl","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-13T19:58:17Z","receivedAt":"2012-02-13T19:58:17Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Michal Kiedrowicz wrote:\n> Jakub Narebski wrote:\n> > Jakub Narebski writes:\n[...]\n> > > Anyway I have send to git mailing list a patch series, which in one\n> > > of patches adds esc_html_match_hl($str, $regexp) to highlight\n> > > matches in a string. \n> \n> Yeah, I saw that but after seeing that they accept different arguments\n> I decided to leave them alone.\n> \n> > > Your esc_html_mark_range(), after a\n> > > generalization, could be used as underlying \"engine\".\n> > > \n> > > Something like this, perhaps (untested):\n> \n> I think I'll leave it to you after merging both these series to\n> master :)\n \nYes, you are right. Let's do it when we actually need it.\n\n[...]\n> > > # Highlight selected fragments of string, using given CSS class,\n> > > # and escape HTML. It is assumed that fragments do not overlap.\n> > > # Regions are passed as list of pairs (array references).\n> > > sub esc_html_hl {\n[...]\n> > Actually we can accomodate both operating on string and operating on\n> > array of characters in a single subroutine. Though it can be left for\n> > later commit, anyway...\n\n-- \nJakub Narebski\nPoland\n"},{"id":"184606","messageId":"20120213220917.4cf14eb1@gmail.com","threadId":"29593","inReplyTo":"201202131944.50886.jnareb@gmail.com","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-13T21:09:17Z","receivedAt":"2012-02-13T21:09:17Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jakub Narebski wrote:\n\n> On Mon, 13 Feb 2012, Michal Kiedrowicz wrote:\n> > Jakub Narebski wrote:\n> > > Michał Kiedrowicz writes:\n> > I haven't found *examples* on GitHub and Trac sites, but what about\n> > these ones:\n> > \n> > https://github.com/gitster/git/commit/8cad4744ee37ebec1d9491a1381ec1771a1ba795\n> > http://trac.edgewall.org/changeset/10973\n> \n> Thanks. That is what I meant by \"good examples\". Perhaps they should\n> be put in the commit message?\n\nOK\n\n> \n> BTW GitHub is closed source, but we can check what algorithm does Trac\n> use for diff refinement highlighting (highlighting changed portions of\n> diff).\n> \n\nI think it's\nhttp://trac.edgewall.org/browser/trunk/trac/versioncontrol/diff.py\n(see markup intraline_changes()).\n\n\n> > > It doesn't implement LCS / diff\n> > > algorithm like e.g. tkdiff does for its diff refinement highlighting,\n> > > isn't it?\n> > \n> > I doesn't. I share the Jeff's opinion that:\n> > a) Jeff's approach is \"good enough\"\n> > b) LCS on bytes could be very confusing if it marked few sets of\n> > characters.\n> \n> I wonder if we can use --diff-words for diff refinement highlighting,\n> i.e. LCS on words.\n\nI think we can try it, but I worry about performance of running `git\ndiff` on every diff chunk.\n\n\n> \n> Anyway Jeff's approach is a bit limited, in that it would work only for\n> change that does not involve adding newlines, for example splitting\n> overly long line when changing something.\n> \n> See for example line 1786 (in pre-image) in http://trac.edgewall.org/changeset/10973\n> \n\nYes, I'm aware of that. I was thinking about improving it later (\"Let's\nstart with a simple refinment highlightning and maybe later add more\nsophisticated algorithms\").\n\n> > > By completly different you mean that they do not have common prefix or\n> > > common suffix (at least one of them), isn't it?\n> \n> BTW. is it \"at least one of prefix or suffix are non-empty\" or \"both prefix\n> and suffix are non-empty\"?\n> \n\nAt least one. See:\n\n\t-a = 42;\n\t+b = 42;\n\nHere prefix is empty but suffix is not.\n\n> > I would also consider ignoring prefixes/suffixes with punctuation, like:\n> > \n> > \t- * I like you.\n> > \t+ * Alice had a little lamb.\n> \n> But this patch doesn't implement this feature yet, isn't it?\n\nNo, but is a matter of adding\n\n\t-$prefix_is_space = 0 if ($r[$prefix] !~ /\\s/);\n\t+$prefix_is_space = 0 if ($r[$prefix] !~ /\\s|[[:punct:]]/);\n\n(and the same for suffix)\n\n> Well, here is another idea: do not highlight if sum of prefix and suffix\n> lengths are less than some threshold, e.g. 2 characters not including \n> whitespace, or some percentage with respect to total line length.\n> \n\nThat might be a good idea.\n\n> > > Eeeeeek! First you split into letters, in caller at that, then join?\n> > > Why not pass striung ($str suggests string not array of characters),\n> > > and use substr instead?\n> > > \n> > > [Please disregard this and the next paragraph at first reading]\n> > \n> > I will rename $str to something more self describing.\n> \n> Please do.\n> \n> BTW. don't you assume here that both common prefix and common suffix\n> are non-empty?\n\nYes. The caller makes sure they are correct (at least \n\n> > > > +sub format_rem_add_line {\n> > > > +\tmy ($rem, $add) = @_;\n> > > > +\tmy @r = split(//, $rem);\n> > > > +\tmy @a = split(//, $add);\n> \n> BTW the name of variable can be just @add and @rem.\n>\n\nI know they are different scopes but I don't like it. It makes the code\nmore confusing IMO. But I won't insist.\n\n> > > Shouldn't\n> > > $prefix / $prefix_len start from 0, not from 1?\n> > \n> > It starts from 1 because it skips first +/-. It should become obvious\n> > after reading the comment from last patch :).\n> > \n> > +\t# In combined diff we must ignore two +/- characters.\n> > +\t$prefix = 2 if ($is_combined);\n> \n> Anyway comment about that fact would be nice.\n\nWill do.\n\n> \n> > > > +\tmy ($prefix_is_space, $suffix_is_space) = (1, 1);\n> > > > +\n> > > > +\twhile ($prefix < @r && $prefix < @a) {\n> > > > +\t\tlast if ($r[$prefix] ne $a[$prefix]);\n> > > > +\n> > > > +\t\t$prefix_is_space = 0 if ($r[$prefix] !~ /\\s/);\n> > > > +\t\t$prefix++;\n> > > > +\t}\n> > > \n> > > Ah, I see that it is easier to find common prefix by treating string\n> > > as array of characters.\n> > > \n> > > Though I wonder if it wouldn't be easier to use trick of XOR-ing two\n> > > strings (see \"Bitwise String Operators\" in perlop(1)):\n> > > \n> > > my $xor = \"$rem\" ^ \"$add\";\n> > > \n> > > and finding starting sequence of \"\\0\", which denote common prefix.\n> > > \n> > > \n> > > Though this and the following is a nice implementation of\n> > > algorithm... as it would be implemented in C. Nevermind, it might be\n> > > good enough...\n> > \n> > The splitting and comparing by characters is taken from diff-highlight.\n> > I don't think it's worth changing here.\n> \n> You are right.\n> \n> I'll try to come with hacky algorithm using string bitwise xor and regexp,\n> and benchmark it comparing to your C-like solution, but it can be left for\n> later (simple is better than clever, usually).\n\nIf you have time :).\n\n> > > > # HTML-format diff context, removed and added lines.\n> > > > sub format_ctx_rem_add_lines {\n> > > > -\tmy ($ctx, $rem, $add) = @_;\n> > > > +\tmy ($ctx, $rem, $add, $is_combined) = @_;\n> > > > \tmy (@new_ctx, @new_rem, @new_add);\n> > > > +\tmy $num_add_lines = @$add;\n> > > \n> > > Why is this temporary variable needed? If you are not sure that ==\n> > > operator enforces scalar context on both arguments you can always\n> > > write\n> > > \n> > > scalar @$add == scalar @$rem\n> > \n> > You read my mind.\n> \n> BTW. the same comment applies to patch adding support for highlighting\n> changed part in combined diff.\n>\n\nOK\n \n"},{"id":"184607","messageId":"20120213221024.7315d3c0@gmail.com","threadId":"29593","inReplyTo":"201202132058.18001.jnareb@gmail.com","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-13T21:10:24Z","receivedAt":"2012-02-13T21:10:24Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jakub Narebski wrote:\n\n> Michal Kiedrowicz wrote:\n> > Jakub Narebski wrote:\n> > > Jakub Narebski writes:\n> [...]\n> > > > Anyway I have send to git mailing list a patch series, which in one\n> > > > of patches adds esc_html_match_hl($str, $regexp) to highlight\n> > > > matches in a string. \n> > \n> > Yeah, I saw that but after seeing that they accept different arguments\n> > I decided to leave them alone.\n> > \n> > > > Your esc_html_mark_range(), after a\n> > > > generalization, could be used as underlying \"engine\".\n> > > > \n> > > > Something like this, perhaps (untested):\n> > \n> > I think I'll leave it to you after merging both these series to\n> > master :)\n> \n> Yes, you are right. Let's do it when we actually need it.\n> \n\nGood :)\n"},{"id":"184616","messageId":"20120213222702.GA19393@sigill.intra.peff.net","threadId":"29593","inReplyTo":"1328910433-2539-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH] diff-highlight: Work for multiline changes too","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-13T22:27:02Z","receivedAt":"2012-02-13T22:27:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 10, 2012 at 10:47:13PM +0100, Michał Kiedrowicz wrote:\n\n> contrib/diff-highlight/diff-highlight | 96 ++++++++++++++++++++++-----------\n> 1 files changed, 65 insertions(+), 31 deletions(-)\n\nThanks for sending. I looked at a whole bunch of patches, and I was\npleasantly surprised to find how infrequently we hit false positives in\npractice. In fact, the only things that looked worse with your patch\nwere places where your patch happened to turn on highlighting for lines\nwhere the existing heuristics already were a little ugly (i.e., the\nproblem was not your patch, but that the existing heuristic is sometimes\nnon-optimal).\n\nI ended up pulling your changes out into a few distinct commits. That\nmade it easier for me to review and understand what was going on (and\nhopefully ditto for other reviewers, or people who end up bisecting or\nreading the log later). I'll post that series in a moment.\n\n> After looking at outputs I noticed that it can also ignore lines with\n> prefixes/suffixes that consist only of punctuation (asterisk, semicolon, dot,\n> etc), because otherwise whole line is highlighted except for terminating\n> punctuation.\n\nI missed this note when I applied the patch and started looking at the\noutputs, and ended up having a similar thought. However, I don't know\nthat it buys much in practice, and it's nice to be fairly agnostic about\ncontent. I did leave that open to easy tweaking in my series, though.\n\n [1/5]: diff-highlight: make perl strict and warnings fatal\n [2/5]: diff-highlight: don't highlight whole lines\n [3/5]: diff-highlight: refactor to prepare for multi-line hunks\n [4/5]: diff-highlight: match multi-line hunks\n [5/5]: diff-highlight: document some non-optimal cases\n\n-Peff\n"},{"id":"184617","messageId":"20120213222810.GA19521@sigill.intra.peff.net","threadId":"29593","inReplyTo":"20120213222702.GA19393@sigill.intra.peff.net","subject":"[PATCH 1/5] diff-highlight: make perl strict and warnings fatal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-13T22:28:10Z","receivedAt":"2012-02-13T22:28:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These perl features can catch bugs, and we shouldn't be\nviolating any of the strict rules or creating any warnings,\nso let's turn them on.\n\nSigned-off-by: Jeff King \n---\n contrib/diff-highlight/diff-highlight | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight\nindex d893898..c3302dd 100755\n--- a/contrib/diff-highlight/diff-highlight\n+++ b/contrib/diff-highlight/diff-highlight\n@@ -1,5 +1,8 @@\n #!/usr/bin/perl\n \n+use warnings FATAL => 'all';\n+use strict;\n+\n # Highlight by reversing foreground and background. You could do\n # other things like bold or underline if you prefer.\n my $HIGHLIGHT = \"\\x1b[7m\";\n-- \n1.7.8.4.17.g2df81\n"},{"id":"184618","messageId":"20120213223247.GB19521@sigill.intra.peff.net","threadId":"29593","inReplyTo":"20120213222702.GA19393@sigill.intra.peff.net","subject":"[PATCH 2/5] diff-highlight: don't highlight whole lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-13T22:32:47Z","receivedAt":"2012-02-13T22:32:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If you have a change like:\n\n -foo\n +bar\n\nwe end up highlighting the entirety of both lines (since the\nwhole thing is changed). But the point of diff highlighting\nis to pinpoint the specific change in a pair of lines that\nare mostly identical. In this case, the highlighting is just\nnoise, since there is nothing to pinpoint, and we are better\noff doing nothing.\n\nThe implementation looks for \"interesting\" pairs by checking\nto see whether they actually have a matching prefix or\nsuffix that does not simply consist of colorization and\nwhitespace. However, the implementation makes it easy to\nplug in other heuristics, too, like:\n\n 1. Depending on the source material, the set of \"boring\"\n characters could be tweaked to include language-specific\n stuff (like braces or semicolons for C).\n\n 2. Instead of saying \"an interesting line has at least one\n character of prefix or suffix\", we could require that\n less than N percent of the line be highlighted.\n\nThe simple \"ignore whitespace, and highlight if there are\nany matched characters\" implemented by this patch seems to\ngive good results on git.git. I'll leave experimentation\nwith other heuristics to somebody who has a dataset that\ndoes not look good with the current code.\n\nBased on an original idea and implementation by Michał\nKiedrowicz.\n\nSigned-off-by: Jeff King \n---\nRegarding attribution: I kept myself as author, because I rewrote it a\nbit and figured that I would take primary responsibility for bugs in\nthis patch, and in the long run would be responsible for maintaining it.\nBut the idea and the substance of the patch are yours, and I would be\nhappy to list you as author if you prefer getting the credit that way\n(after all, it bumps your shortlog numbers :) ).\n\nThe implementation is similar to yours, but pulls some of the decisions\nout into a separate function to make tweaking the above heuristics\neasier.\n\n contrib/diff-highlight/diff-highlight | 28 ++++++++++++++++++++++++++--\n 1 file changed, 26 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight\nindex c3302dd..0d8df84 100755\n--- a/contrib/diff-highlight/diff-highlight\n+++ b/contrib/diff-highlight/diff-highlight\n@@ -8,6 +8,7 @@ use strict;\n my $HIGHLIGHT = \"\\x1b[7m\";\n my $UNHIGHLIGHT = \"\\x1b[27m\";\n my $COLOR = qr/\\x1b\\[[0-9;]*m/;\n+my $BORING = qr/$COLOR|\\s/;\n \n my @window;\n \n@@ -104,8 +105,14 @@ sub show_pair {\n \t\t}\n \t}\n \n-\tprint highlight(\\@a, $pa, $sa);\n-\tprint highlight(\\@b, $pb, $sb);\n+\tif (is_pair_interesting(\\@a, $pa, $sa, \\@b, $pb, $sb)) {\n+\t\tprint highlight(\\@a, $pa, $sa);\n+\t\tprint highlight(\\@b, $pb, $sb);\n+\t}\n+\telse {\n+\t\tprint join('', @a);\n+\t\tprint join('', @b);\n+\t}\n }\n \n sub split_line {\n@@ -125,3 +132,20 @@ sub highlight {\n \t\t@{$line}[($suffix+1)..$#$line]\n \t);\n }\n+\n+# Pairs are interesting to highlight only if we are going to end up\n+# highlighting a subset (i.e., not the whole line). Otherwise, the highlighting\n+# is just useless noise. We can detect this by finding either a matching prefix\n+# or suffix (disregarding boring bits like whitespace and colorization).\n+sub is_pair_interesting {\n+\tmy ($a, $pa, $sa, $b, $pb, $sb) = @_;\n+\tmy $prefix_a = join('', @$a[0..($pa-1)]);\n+\tmy $prefix_b = join('', @$b[0..($pb-1)]);\n+\tmy $suffix_a = join('', @$a[($sa+1)..$#$a]);\n+\tmy $suffix_b = join('', @$b[($sb+1)..$#$b]);\n+\n+\treturn $prefix_a !~ /^$COLOR*-$BORING*$/ ||\n+\t $prefix_b !~ /^$COLOR*\\+$BORING*$/ ||\n+\t $suffix_a !~ /^$BORING*$/ ||\n+\t $suffix_b !~ /^$BORING*$/;\n+}\n-- \n1.7.8.4.17.g2df81\n"},{"id":"184619","messageId":"20120213223310.GC19521@sigill.intra.peff.net","threadId":"29593","inReplyTo":"20120213222702.GA19393@sigill.intra.peff.net","subject":"[PATCH 3/5] diff-highlight: refactor to prepare for multi-line hunks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-13T22:33:10Z","receivedAt":"2012-02-13T22:33:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The current code structure assumes that we will only look at\na pair of lines at any given time, and that the end result\nshould always be to output that pair. However, we want to\neventually handle multi-line hunks, which will involve\ncollating pairs of removed/added lines. Let's refactor the\ncode to return highlighted pairs instead of printing them.\n\nSigned-off-by: Jeff King \n---\nYou did a similar refactoring in your patch, but I found pulling it out\nmade the next patch a lot more readable.\n\n contrib/diff-highlight/diff-highlight | 22 ++++++++++++++--------\n 1 file changed, 14 insertions(+), 8 deletions(-)\n\ndiff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight\nindex 0d8df84..279d211 100755\n--- a/contrib/diff-highlight/diff-highlight\n+++ b/contrib/diff-highlight/diff-highlight\n@@ -23,7 +23,7 @@ while (<>) {\n \t $window[2] =~ /^$COLOR*\\+/ &&\n \t $window[3] !~ /^$COLOR*\\+/) {\n \t\tprint shift @window;\n-\t\tshow_pair(shift @window, shift @window);\n+\t\tshow_hunk(shift @window, shift @window);\n \t}\n \telse {\n \t\tprint shift @window;\n@@ -48,7 +48,7 @@ if (@window == 3 &&\n $window[1] =~ /^$COLOR*-/ &&\n $window[2] =~ /^$COLOR*\\+/) {\n \tprint shift @window;\n-\tshow_pair(shift @window, shift @window);\n+\tshow_hunk(shift @window, shift @window);\n }\n \n # And then flush any remaining lines.\n@@ -58,7 +58,13 @@ while (@window) {\n \n exit 0;\n \n-sub show_pair {\n+sub show_hunk {\n+\tmy ($a, $b) = @_;\n+\n+\tprint highlight_pair($a, $b);\n+}\n+\n+sub highlight_pair {\n \tmy @a = split_line(shift);\n \tmy @b = split_line(shift);\n \n@@ -106,12 +112,12 @@ sub show_pair {\n \t}\n \n \tif (is_pair_interesting(\\@a, $pa, $sa, \\@b, $pb, $sb)) {\n-\t\tprint highlight(\\@a, $pa, $sa);\n-\t\tprint highlight(\\@b, $pb, $sb);\n+\t\treturn highlight_line(\\@a, $pa, $sa),\n+\t\t highlight_line(\\@b, $pb, $sb);\n \t}\n \telse {\n-\t\tprint join('', @a);\n-\t\tprint join('', @b);\n+\t\treturn join('', @a),\n+\t\t join('', @b);\n \t}\n }\n \n@@ -121,7 +127,7 @@ sub split_line {\n \t split /($COLOR*)/;\n }\n \n-sub highlight {\n+sub highlight_line {\n \tmy ($line, $prefix, $suffix) = @_;\n \n \treturn join('',\n-- \n1.7.8.4.17.g2df81\n"},{"id":"184620","messageId":"20120213223636.GD19521@sigill.intra.peff.net","threadId":"29593","inReplyTo":"20120213222702.GA19393@sigill.intra.peff.net","subject":"[PATCH 4/5] diff-highlight: match multi-line hunks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-13T22:36:36Z","receivedAt":"2012-02-13T22:36:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Currently we only bother highlighting single-line hunks. The\nrationale was that the purpose of highlighting is to point\nout small changes between two similar lines that are\notherwise hard to see. However, that meant we missed similar\ncases where two lines were changed together, like:\n\n -foo(buf);\n -bar(buf);\n +foo(obj->buf);\n +bar(obj->buf);\n\nEach of those changes is simple, and would benefit from\nhighlighting (the \"obj->\" parts in this case).\n\nThis patch considers whole hunks at a time. For now, we\nconsider only the case where the hunk has the same number of\nremoved and added lines, and assume that the lines from each\nsegment correspond one-to-one. While this is just a\nheuristic, in practice it seems to generate sensible\nresults (especially because we now omit highlighting on\ncompletely-changed lines, so when our heuristic is wrong, we\ntend to avoid highlighting at all).\n\nBased on an original idea and implementation by Michał\nKiedrowicz.\n\nSigned-off-by: Jeff King \n---\nSame attribution statement applies as to patch 2 (in fact, patches 1 and\n3 could be attributed to you, too).\n\nThis version has the missing documentation fixes. The implementation is\na little different than yours. I rearranged the parsing in a manner that\nwas a little more obvious to me, and I pulled out the \"don't highlight\nif the number of lines don't match\" case into its own conditional, which\nmakes it more obvious where to work if somebody wants to try doing\nsomething fancier.\n\n contrib/diff-highlight/README | 16 ++++---\n contrib/diff-highlight/diff-highlight | 70 ++++++++++++++++++++-------------\n 2 files changed, 52 insertions(+), 34 deletions(-)\n\ndiff --git a/contrib/diff-highlight/README b/contrib/diff-highlight/README\nindex 1b7b6df..4a58579 100644\n--- a/contrib/diff-highlight/README\n+++ b/contrib/diff-highlight/README\n@@ -14,13 +14,15 @@ Instead, this script post-processes the line-oriented diff, finds pairs\n of lines, and highlights the differing segments. It's currently very\n simple and stupid about doing these tasks. In particular:\n \n- 1. It will only highlight a pair of lines if they are the only two\n- lines in a hunk. It could instead try to match up \"before\" and\n- \"after\" lines for a given hunk into pairs of similar lines.\n- However, this may end up visually distracting, as the paired\n- lines would have other highlighted lines in between them. And in\n- practice, the lines which most need attention called to their\n- small, hard-to-see changes are touching only a single line.\n+ 1. It will only highlight hunks in which the number of removed and\n+ added lines is the same, and it will pair lines within the hunk by\n+ position (so the first removed line is compared to the first added\n+ line, and so forth). This is simple and tends to work well in\n+ practice. More complex changes don't highlight well, so we tend to\n+ exclude them due to the \"same number of removed and added lines\"\n+ restriction. Or even if we do try to highlight them, they end up\n+ not highlighting because of our \"don't highlight if the whole line\n+ would be highlighted\" rule.\n \n 2. It will find the common prefix and suffix of two lines, and\n consider everything in the middle to be \"different\". It could\ndiff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight\nindex 279d211..c4404d4 100755\n--- a/contrib/diff-highlight/diff-highlight\n+++ b/contrib/diff-highlight/diff-highlight\n@@ -10,23 +10,28 @@ my $UNHIGHLIGHT = \"\\x1b[27m\";\n my $COLOR = qr/\\x1b\\[[0-9;]*m/;\n my $BORING = qr/$COLOR|\\s/;\n \n-my @window;\n+my @removed;\n+my @added;\n+my $in_hunk;\n \n while (<>) {\n-\t# We highlight only single-line changes, so we need\n-\t# a 4-line window to make a decision on whether\n-\t# to highlight.\n-\tpush @window, $_;\n-\tnext if @window < 4;\n-\tif ($window[0] =~ /^$COLOR*(\\@| )/ &&\n-\t $window[1] =~ /^$COLOR*-/ &&\n-\t $window[2] =~ /^$COLOR*\\+/ &&\n-\t $window[3] !~ /^$COLOR*\\+/) {\n-\t\tprint shift @window;\n-\t\tshow_hunk(shift @window, shift @window);\n+\tif (!$in_hunk) {\n+\t\tprint;\n+\t\t$in_hunk = /^$COLOR*\\@/;\n+\t}\n+\telsif (/^$COLOR*-/) {\n+\t\tpush @removed, $_;\n+\t}\n+\telsif (/^$COLOR*\\+/) {\n+\t\tpush @added, $_;\n \t}\n \telse {\n-\t\tprint shift @window;\n+\t\tshow_hunk(\\@removed, \\@added);\n+\t\t@removed = ();\n+\t\t@added = ();\n+\n+\t\tprint;\n+\t\t$in_hunk = /^$COLOR*[\\@ ]/;\n \t}\n \n \t# Most of the time there is enough output to keep things streaming,\n@@ -42,26 +47,37 @@ while (<>) {\n \t}\n }\n \n-# Special case a single-line hunk at the end of file.\n-if (@window == 3 &&\n- $window[0] =~ /^$COLOR*(\\@| )/ &&\n- $window[1] =~ /^$COLOR*-/ &&\n- $window[2] =~ /^$COLOR*\\+/) {\n-\tprint shift @window;\n-\tshow_hunk(shift @window, shift @window);\n-}\n-\n-# And then flush any remaining lines.\n-while (@window) {\n-\tprint shift @window;\n-}\n+# Flush any queued hunk (this can happen when there is no trailing context in\n+# the final diff of the input).\n+show_hunk(\\@removed, \\@added);\n \n exit 0;\n \n sub show_hunk {\n \tmy ($a, $b) = @_;\n \n-\tprint highlight_pair($a, $b);\n+\t# If one side is empty, then there is nothing to compare or highlight.\n+\tif (!@$a || !@$b) {\n+\t\tprint @$a, @$b;\n+\t\treturn;\n+\t}\n+\n+\t# If we have mismatched numbers of lines on each side, we could try to\n+\t# be clever and match up similar lines. But for now we are simple and\n+\t# stupid, and only handle multi-line hunks that remove and add the same\n+\t# number of lines.\n+\tif (@$a != @$b) {\n+\t\tprint @$a, @$b;\n+\t\treturn;\n+\t}\n+\n+\tmy @queue;\n+\tfor (my $i = 0; $i < @$a; $i++) {\n+\t\tmy ($rm, $add) = highlight_pair($a->[$i], $b->[$i]);\n+\t\tprint $rm;\n+\t\tpush @queue, $add;\n+\t}\n+\tprint @queue;\n }\n \n sub highlight_pair {\n-- \n1.7.8.4.17.g2df81\n"},{"id":"184621","messageId":"20120213223733.GE19521@sigill.intra.peff.net","threadId":"29593","inReplyTo":"20120213222702.GA19393@sigill.intra.peff.net","subject":"[PATCH 5/5] diff-highlight: document some non-optimal cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-13T22:37:33Z","receivedAt":"2012-02-13T22:37:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The diff-highlight script works on heuristics, so it can be\nwrong. Let's document some of the wrong-ness in case\nsomebody feels like working on it.\n\nSigned-off-by: Jeff King \n---\nThese were just some that I considered while looking at the output of\nthe original and the current code. Suggestions are welcome for more.\n\n contrib/diff-highlight/README | 93 +++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 93 insertions(+)\n\ndiff --git a/contrib/diff-highlight/README b/contrib/diff-highlight/README\nindex 4a58579..502e03b 100644\n--- a/contrib/diff-highlight/README\n+++ b/contrib/diff-highlight/README\n@@ -57,3 +57,96 @@ following in your git configuration:\n \tshow = diff-highlight | less\n \tdiff = diff-highlight | less\n ---------------------------------------------\n+\n+Bugs\n+----\n+\n+Because diff-highlight relies on heuristics to guess which parts of\n+changes are important, there are some cases where the highlighting is\n+more distracting than useful. Fortunately, these cases are rare in\n+practice, and when they do occur, the worst case is simply a little\n+extra highlighting. This section documents some cases known to be\n+sub-optimal, in case somebody feels like working on improving the\n+heuristics.\n+\n+1. Two changes on the same line get highlighted in a blob. For example,\n+ highlighting:\n+\n+----------------------------------------------\n+-foo(buf, size);\n++foo(obj->buf, obj->size);\n+----------------------------------------------\n+\n+ yields (where the inside of \"+{}\" would be highlighted):\n+\n+----------------------------------------------\n+-foo(buf, size);\n++foo(+{obj->buf, obj->}size);\n+----------------------------------------------\n+\n+ whereas a more semantically meaningful output would be:\n+\n+----------------------------------------------\n+-foo(buf, size);\n++foo(+{obj->}buf, +{obj->}size);\n+----------------------------------------------\n+\n+ Note that doing this right would probably involve a set of\n+ content-specific boundary patterns, similar to word-diff. Otherwise\n+ you get junk like:\n+\n+-----------------------------------------------------\n+-this line has some -{i}nt-{ere}sti-{ng} text on it\n++this line has some +{fa}nt+{a}sti+{c} text on it\n+-----------------------------------------------------\n+\n+ which is less readable than the current output.\n+\n+2. The multi-line matching assumes that lines in the pre- and post-image\n+ match by position. This is often the case, but can be fooled when a\n+ line is removed from the top and a new one added at the bottom (or\n+ vice versa). Unless the lines in the middle are also changed, diffs\n+ will show this as two hunks, and it will not get highlighted at all\n+ (which is good). But if the lines in the middle are changed, the\n+ highlighting can be misleading. Here's a pathological case:\n+\n+-----------------------------------------------------\n+-one\n+-two\n+-three\n+-four\n++two 2\n++three 3\n++four 4\n++five 5\n+-----------------------------------------------------\n+\n+ which gets highlighted as:\n+\n+-----------------------------------------------------\n+-one\n+-t-{wo}\n+-three\n+-f-{our}\n++two 2\n++t+{hree 3}\n++four 4\n++f+{ive 5}\n+-----------------------------------------------------\n+\n+ because it matches \"two\" to \"three 3\", and so forth. It would be\n+ nicer as:\n+\n+-----------------------------------------------------\n+-one\n+-two\n+-three\n+-four\n++two +{2}\n++three +{3}\n++four +{4}\n++five 5\n+-----------------------------------------------------\n+\n+ which would probably involve pre-matching the lines into pairs\n+ according to some heuristic.\n-- \n1.7.8.4.17.g2df81\n"},{"id":"184626","messageId":"7v8vk6b7pq.fsf@alter.siamese.dyndns.org","threadId":"29593","inReplyTo":"20120213222702.GA19393@sigill.intra.peff.net","subject":"Re: [PATCH] diff-highlight: Work for multiline changes too","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-14T00:05:05Z","receivedAt":"2012-02-14T00:05:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I ended up pulling your changes out into a few distinct commits. That\n> made it easier for me to review and understand what was going on (and\n> hopefully ditto for other reviewers, or people who end up bisecting or\n> reading the log later). I'll post that series in a moment.\n\nThis is all nice. As I am lazy and this is a long neglected contrib/\nmaterial I didn't even know it existed ;-), I am tempted to apply them\ndirectly on top of 'master'.\n\nThis shows the first hunk of your \"diff-highlight: refactor to prepare for\nmulti-line hunks\" like this to me, by the way.\n\n@@ -23,7 +23,7 @@ while (<>) {\n $window[2] =~ /^$COLOR*\\+/ &&\n $window[3] !~ /^$COLOR*\\+/) {\n print shift @window;\n{- show_pair}(shift @window, shift @window);\n+ show_{hunk}(shift @window, shift @window);\n }\n else {\n print shift @window;\n\nIs this intended, or is setting \"diff.color.old = red reverse\" not\nsupported (without the custom configuration, the leading blank on the old\nline is not highlighted)?\n"},{"id":"184628","messageId":"20120214002209.GA23171@sigill.intra.peff.net","threadId":"29593","inReplyTo":"7v8vk6b7pq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] diff-highlight: Work for multiline changes too","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-14T00:22:09Z","receivedAt":"2012-02-14T00:22:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 13, 2012 at 04:05:05PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I ended up pulling your changes out into a few distinct commits. That\n> > made it easier for me to review and understand what was going on (and\n> > hopefully ditto for other reviewers, or people who end up bisecting or\n> > reading the log later). I'll post that series in a moment.\n> \n> This is all nice. As I am lazy and this is a long neglected contrib/\n> material I didn't even know it existed ;-), I am tempted to apply them\n> directly on top of 'master'.\n\nYeah, given the contrib nature, I'm comfortable just applying. I wanted\nto wait on the attribution question from Michał before sending a final\nversion, though.\n\n> This shows the first hunk of your \"diff-highlight: refactor to prepare for\n> multi-line hunks\" like this to me, by the way.\n> \n> @@ -23,7 +23,7 @@ while (<>) {\n> $window[2] =~ /^$COLOR*\\+/ &&\n> $window[3] !~ /^$COLOR*\\+/) {\n> print shift @window;\n> {- show_pair}(shift @window, shift @window);\n> + show_{hunk}(shift @window, shift @window);\n> }\n> else {\n> print shift @window;\n> \n> Is this intended, or is setting \"diff.color.old = red reverse\" not\n> supported (without the custom configuration, the leading blank on the old\n> line is not highlighted)?\n\nNo, it's not intended, and should be:\n\n - show-{pair}...\n + show+{hunk}...\n\n(and appears that way with my config). I suspect it is because the\ndefault highlight color is a subset of your \"old\" configured color. It\nis ANSI \"reverse video\", but sadly that does not work as a toggle (i.e.,\nit does not reverse your reverse, but simply is a no-op).\n\nI chose reverse because I like the way it looks, and because it should\nJust Work if people have selected alternate colors (I never dreamed\nsomebody would use \"reverse\" all the time, as I find it horribly ugly.\nBut to each his own). We could read the highlight color from\ndiff.highlight{Old,New}. That would also let people highlight in purple\nor something if they care.\n\n-Peff\n"},{"id":"184631","messageId":"7vk43q9pp6.fsf@alter.siamese.dyndns.org","threadId":"29593","inReplyTo":"20120214002209.GA23171@sigill.intra.peff.net","subject":"Re: [PATCH] diff-highlight: Work for multiline changes too","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-14T01:19:33Z","receivedAt":"2012-02-14T01:19:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I chose reverse because I like the way it looks, and because it should\n> Just Work if people have selected alternate colors (I never dreamed\n> somebody would use \"reverse\" all the time, as I find it horribly ugly.\n> But to each his own).\n\nI also find it ugly, but I am on black-letters on white background window,\nand I do not see my terminal's red very well, so it is hard to tell the\nold from the context if I use the \"diff.color.old=red\" default; that is\nthe primary reason for my setting.\n"},{"id":"184636","messageId":"20120214060404.GA25996@sigill.intra.peff.net","threadId":"29593","inReplyTo":"7vk43q9pp6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] diff-highlight: Work for multiline changes too","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-14T06:04:04Z","receivedAt":"2012-02-14T06:04:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 13, 2012 at 05:19:33PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I chose reverse because I like the way it looks, and because it should\n> > Just Work if people have selected alternate colors (I never dreamed\n> > somebody would use \"reverse\" all the time, as I find it horribly ugly.\n> > But to each his own).\n> \n> I also find it ugly, but I am on black-letters on white background window,\n> and I do not see my terminal's red very well, so it is hard to tell the\n> old from the context if I use the \"diff.color.old=red\" default; that is\n> the primary reason for my setting.\n\nI find black-on-white ugly, too, but I have heard some people find it\nmore readable. You might try setting color.diff.old to \"bold red\" to\nmake it more readable. Depending on your terminal, that may end up as a\nbrighter shade of red.\n\nConfigurable colors for diff-highlight would look like the patch below:\n\ndiff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight\nindex c4404d4..f43832b 100755\n--- a/contrib/diff-highlight/diff-highlight\n+++ b/contrib/diff-highlight/diff-highlight\n@@ -2,11 +2,13 @@\n \n use warnings FATAL => 'all';\n use strict;\n+use Git;\n+\n+my $repo = Git->repository;\n+my $color_old = $repo->get_color('color.diff.highlightold', 'reverse');\n+my $color_new = $repo->get_color('color.diff.highlightnew', 'reverse');\n+my $color_end = $repo->get_color('color.diff.highlightend', 'noreverse');\n \n-# Highlight by reversing foreground and background. You could do\n-# other things like bold or underline if you prefer.\n-my $HIGHLIGHT = \"\\x1b[7m\";\n-my $UNHIGHLIGHT = \"\\x1b[27m\";\n my $COLOR = qr/\\x1b\\[[0-9;]*m/;\n my $BORING = qr/$COLOR|\\s/;\n \n@@ -128,8 +130,8 @@ sub highlight_pair {\n \t}\n \n \tif (is_pair_interesting(\\@a, $pa, $sa, \\@b, $pb, $sb)) {\n-\t\treturn highlight_line(\\@a, $pa, $sa),\n-\t\t highlight_line(\\@b, $pb, $sb);\n+\t\treturn highlight_line(\\@a, $pa, $sa, $color_old, $color_end),\n+\t\t highlight_line(\\@b, $pb, $sb, $color_new, $color_end);\n \t}\n \telse {\n \t\treturn join('', @a),\n@@ -144,13 +146,13 @@ sub split_line {\n }\n \n sub highlight_line {\n-\tmy ($line, $prefix, $suffix) = @_;\n+\tmy ($line, $prefix, $suffix, $highlight, $unhighlight) = @_;\n \n \treturn join('',\n \t\t@{$line}[0..($prefix-1)],\n-\t\t$HIGHLIGHT,\n+\t\t$highlight,\n \t\t@{$line}[$prefix..$suffix],\n-\t\t$UNHIGHLIGHT,\n+\t\t$unhighlight,\n \t\t@{$line}[($suffix+1)..$#$line]\n \t);\n }\n\nHowever, there are two problems:\n\n 1. Git's color-parsing support does not understand the \"noreverse\"\n attribute. There is no way to turn off attributes short of doing a\n whole \"reset\". But we don't want to do that here, because we want\n whatever other colors were in effect to continue after the\n highlight ends. The patch to teach \"noreverse\" is below (though it\n should probably also teach \"normal\" and \"noblink\", too).\n\n 2. The Git.pm:get_color code requires that we have a repository\n object, which means we will die() if we are not in a git\n repository. Yet \"git config\" will do the right thing whether we are\n in a repository or not. I would have thought all of the _maybe_self\n stuff in Git.pm would handle \"Git->get_color\" properly, but it\n doesn't. That's probably a bug that should be fixed.\n\nI don't especially care about this feature, as I won't use it. But if\nyou are interested in using diff-highlight and the lack of configurable\ncolors is blocking you, then I at least know there will be one user and\nI don't mind putting a little bit of time into it.\n\nHere's the patch for (1) above.\n\ndiff --git a/color.c b/color.c\nindex e8e2681..b0f53a7 100644\n--- a/color.c\n+++ b/color.c\n@@ -47,9 +47,9 @@ static int parse_color(const char *name, int len)\n \n static int parse_attr(const char *name, int len)\n {\n-\tstatic const int attr_values[] = { 1, 2, 4, 5, 7 };\n+\tstatic const int attr_values[] = { 1, 2, 4, 5, 7, 27 };\n \tstatic const char * const attr_names[] = {\n-\t\t\"bold\", \"dim\", \"ul\", \"blink\", \"reverse\"\n+\t\t\"bold\", \"dim\", \"ul\", \"blink\", \"reverse\", \"noreverse\"\n \t};\n \tint i;\n \tfor (i = 0; i < ARRAY_SIZE(attr_names); i++) {\n@@ -128,7 +128,7 @@ void color_parse_mem(const char *value, int value_len, const char *var,\n \t\t\tattr &= ~bit;\n \t\t\tif (sep++)\n \t\t\t\t*dst++ = ';';\n-\t\t\t*dst++ = '0' + i;\n+\t\t\tdst += sprintf(dst, \"%d\", i);\n \t\t}\n \t\tif (fg >= 0) {\n \t\t\tif (sep++)\n"},{"id":"184637","messageId":"20120214072848.724c1725@mkiedrowicz.ivo.pl","threadId":"29593","inReplyTo":"20120213222702.GA19393@sigill.intra.peff.net","subject":"Re: [PATCH] diff-highlight: Work for multiline changes too","fromName":"Michal Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-14T06:28:48Z","receivedAt":"2012-02-14T06:28:48Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jeff King wrote:\n\n> On Fri, Feb 10, 2012 at 10:47:13PM +0100, Michał Kiedrowicz wrote:\n> \n> > contrib/diff-highlight/diff-highlight | 96\n> > ++++++++++++++++++++++----------- 1 files changed, 65\n> > insertions(+), 31 deletions(-)\n> \n> Thanks for sending. I looked at a whole bunch of patches, and I was\n> pleasantly surprised to find how infrequently we hit false positives\n> in practice. \n\nYeah, I completely agree with that.\n\n> In fact, the only things that looked worse with your\n> patch were places where your patch happened to turn on highlighting\n> for lines where the existing heuristics already were a little ugly\n> (i.e., the problem was not your patch, but that the existing\n> heuristic is sometimes non-optimal).\n> \n> I ended up pulling your changes out into a few distinct commits. That\n> made it easier for me to review and understand what was going on (and\n> hopefully ditto for other reviewers, or people who end up bisecting or\n> reading the log later). I'll post that series in a moment.\n\nVery nicely done.\n\n> \n> > After looking at outputs I noticed that it can also ignore lines\n> > with prefixes/suffixes that consist only of punctuation (asterisk,\n> > semicolon, dot, etc), because otherwise whole line is highlighted\n> > except for terminating punctuation.\n> \n> I missed this note when I applied the patch and started looking at the\n> outputs, and ended up having a similar thought. However, I don't know\n> that it buys much in practice, and it's nice to be fairly agnostic\n> about content. I did leave that open to easy tweaking in my series,\n> though.\n> \n> [1/5]: diff-highlight: make perl strict and warnings fatal\n> [2/5]: diff-highlight: don't highlight whole lines\n> [3/5]: diff-highlight: refactor to prepare for multi-line hunks\n> [4/5]: diff-highlight: match multi-line hunks\n> [5/5]: diff-highlight: document some non-optimal cases\n> \n> -Peff\n"},{"id":"184638","messageId":"20120214073515.3f2b6f59@mkiedrowicz.ivo.pl","threadId":"29593","inReplyTo":"20120213223247.GB19521@sigill.intra.peff.net","subject":"Re: [PATCH 2/5] diff-highlight: don't highlight whole lines","fromName":"Michal Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-14T06:35:15Z","receivedAt":"2012-02-14T06:35:15Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jeff King wrote:\n\n> Regarding attribution: I kept myself as author, because I rewrote it a\n> bit and figured that I would take primary responsibility for bugs in\n> this patch, and in the long run would be responsible for maintaining\n> it. But the idea and the substance of the patch are yours, and I\n> would be happy to list you as author if you prefer getting the credit\n> that way (after all, it bumps your shortlog numbers :) ).\n\nI'm completely OK with that. I don't care much about shortlog :). It's\nnice enough to see that you liked my idea :) \n"},{"id":"184640","messageId":"20120214074845.513bccf6@mkiedrowicz.ivo.pl","threadId":"29593","inReplyTo":"20120213223733.GE19521@sigill.intra.peff.net","subject":"Re: [PATCH 5/5] diff-highlight: document some non-optimal cases","fromName":"Michal Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-14T06:48:45Z","receivedAt":"2012-02-14T06:48:45Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jeff King wrote:\n\n> The diff-highlight script works on heuristics, so it can be\n> wrong. Let's document some of the wrong-ness in case\n> somebody feels like working on it.\n> \n> Signed-off-by: Jeff King \n> ---\n> These were just some that I considered while looking at the output of\n> the original and the current code. Suggestions are welcome for more.\n\nI would also add (feel free to reword if my English is not very clear):\n\n3. Sometimes the prefix or suffix is very small but because it exists,\nalmost whole line is highlighted. For example:\n\n----------------------------------------------\n--{This is a very long line}.\n++{I like apples}.\n----------------------------------------------\n\nor:\n\n----------------------------------------------\n-Th-{is is an apple}\n+Th+{at was a car}\n----------------------------------------------\n\n\n> \n> contrib/diff-highlight/README | 93\n> +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 93\n> insertions(+)\n> \n> diff --git a/contrib/diff-highlight/README\n> b/contrib/diff-highlight/README index 4a58579..502e03b 100644\n> --- a/contrib/diff-highlight/README\n> +++ b/contrib/diff-highlight/README\n> @@ -57,3 +57,96 @@ following in your git configuration:\n> \tshow = diff-highlight | less\n> \tdiff = diff-highlight | less\n> ---------------------------------------------\n> +\n> +Bugs\n> +----\n> +\n> +Because diff-highlight relies on heuristics to guess which parts of\n> +changes are important, there are some cases where the highlighting is\n> +more distracting than useful. Fortunately, these cases are rare in\n> +practice, and when they do occur, the worst case is simply a little\n> +extra highlighting. This section documents some cases known to be\n> +sub-optimal, in case somebody feels like working on improving the\n> +heuristics.\n> +\n> +1. Two changes on the same line get highlighted in a blob. For\n> example,\n> + highlighting:\n> +\n> +----------------------------------------------\n> +-foo(buf, size);\n> ++foo(obj->buf, obj->size);\n> +----------------------------------------------\n> +\n> + yields (where the inside of \"+{}\" would be highlighted):\n> +\n> +----------------------------------------------\n> +-foo(buf, size);\n> ++foo(+{obj->buf, obj->}size);\n> +----------------------------------------------\n> +\n> + whereas a more semantically meaningful output would be:\n> +\n> +----------------------------------------------\n> +-foo(buf, size);\n> ++foo(+{obj->}buf, +{obj->}size);\n> +----------------------------------------------\n> +\n> + Note that doing this right would probably involve a set of\n> + content-specific boundary patterns, similar to word-diff.\n> Otherwise\n> + you get junk like:\n> +\n> +-----------------------------------------------------\n> +-this line has some -{i}nt-{ere}sti-{ng} text on it\n> ++this line has some +{fa}nt+{a}sti+{c} text on it\n> +-----------------------------------------------------\n> +\n> + which is less readable than the current output.\n> +\n> +2. The multi-line matching assumes that lines in the pre- and\n> post-image\n> + match by position. This is often the case, but can be fooled when\n> a\n> + line is removed from the top and a new one added at the bottom (or\n> + vice versa). Unless the lines in the middle are also changed,\n> diffs\n> + will show this as two hunks, and it will not get highlighted at\n> all\n> + (which is good). But if the lines in the middle are changed, the\n> + highlighting can be misleading. Here's a pathological case:\n> +\n> +-----------------------------------------------------\n> +-one\n> +-two\n> +-three\n> +-four\n> ++two 2\n> ++three 3\n> ++four 4\n> ++five 5\n> +-----------------------------------------------------\n> +\n> + which gets highlighted as:\n> +\n> +-----------------------------------------------------\n> +-one\n> +-t-{wo}\n> +-three\n> +-f-{our}\n> ++two 2\n> ++t+{hree 3}\n> ++four 4\n> ++f+{ive 5}\n> +-----------------------------------------------------\n> +\n> + because it matches \"two\" to \"three 3\", and so forth. It would be\n> + nicer as:\n> +\n> +-----------------------------------------------------\n> +-one\n> +-two\n> +-three\n> +-four\n> ++two +{2}\n> ++three +{3}\n> ++four +{4}\n> ++five 5\n> +-----------------------------------------------------\n> +\n> + which would probably involve pre-matching the lines into pairs\n> + according to some heuristic.\n"},{"id":"184639","messageId":"20120214075439.14f1d2b7@mkiedrowicz.ivo.pl","threadId":"29593","inReplyTo":"m339aivn4z.fsf@localhost.localdomain","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Michal Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-14T06:54:39Z","receivedAt":"2012-02-14T06:54:39Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jakub Narebski wrote:\n\n> Michał Kiedrowicz writes:\n> \n> > The code that comares lines is based on\n> > contrib/diff-highlight/diff-highlight, except that it works with\n> > multiline changes too. It also won't highlight lines that are\n> > completely different because that would only make the output\n> > unreadable. Combined diffs are not supported but a following commit\n> > will change it.\n> \n> I was thinking that if gitweb were to support \"diff refinement\n> highlighting\", it would either use one of *Diff* packages from CPAN,\n> or \"git diff --word-diff\" output.\n> \n\nI just started to wonder if we couldn't use output from Jeff's\ndiff-highlight for gitweb. We could tech diff-highlight to produce diffs\nmarked with -{} and +{} (this is the notation used by Jeff in one of his\nrecent patches) or something like this and then just convert that into\nHTML markup. Changes in gitweb would be minimal, we would reduce\nredundancy and could focus on improving matching algorithm in one place.\n"},{"id":"184641","messageId":"7vpqdh999t.fsf@alter.siamese.dyndns.org","threadId":"29593","inReplyTo":"20120214075439.14f1d2b7@mkiedrowicz.ivo.pl","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-14T07:14:22Z","receivedAt":"2012-02-14T07:14:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michal Kiedrowicz writes:\n\n> I just started to wonder if we couldn't use output from Jeff's\n> diff-highlight for gitweb.\n\nThat could be a sensible approach, but\n\n> We could tech diff-highlight to produce diffs\n> marked with -{} and +{} (this is the notation used by Jeff in one of his\n> recent patches) or something like this and then just convert that into\n> HTML markup.\n\nthis implementation strategy would not work well, given that the payload\ncan contain arbitrary letter sequence (e.g. a Perl script that wants be\nexplicit when writing a hashref literal write +{...} to disambiguate it\nfrom a block). If you are going to modularize diff-highlight and reuse\nit, it needs to learn how to talk HTML to properly escape the payload.\n"},{"id":"184643","messageId":"20120214082051.GA9911@sigill.intra.peff.net","threadId":"29593","inReplyTo":"7vpqdh999t.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-14T08:20:51Z","receivedAt":"2012-02-14T08:20:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 13, 2012 at 11:14:22PM -0800, Junio C Hamano wrote:\n\n> > We could tech diff-highlight to produce diffs\n> > marked with -{} and +{} (this is the notation used by Jeff in one of his\n> > recent patches) or something like this and then just convert that into\n> > HTML markup.\n> \n> this implementation strategy would not work well, given that the payload\n> can contain arbitrary letter sequence (e.g. a Perl script that wants be\n> explicit when writing a hashref literal write +{...} to disambiguate it\n> from a block). If you are going to modularize diff-highlight and reuse\n> it, it needs to learn how to talk HTML to properly escape the payload.\n\nThey're both written in perl; perhaps a more sensible solution would be\nto lib-ify diff-highlight and use it directly inside gitweb.\n\n-Peff\n"},{"id":"184662","messageId":"201202141831.59699.jnareb@gmail.com","threadId":"29593","inReplyTo":"20120213220917.4cf14eb1@gmail.com","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-14T17:31:58Z","receivedAt":"2012-02-14T17:31:58Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"A few issues I have just noticed about this series.\n\n\nFirst, about naming. \"Highlighting interesting parts of diff\" is\nacceptable name, but \"syntax highlighting for diff\" is not: gitweb\nalready use syntax highlighting in diff views. Call it \"diff refinement\nhighlighting\", \"highlighting changes / changed sections\", \"intraline\nhighlighting\".\n\n\nSecond, I think (but I am not sure) that there is a bug in code that\nfinds common suffix and prefix.\n\nIf I understand correctly the idea is to highlight changed part if\nthere is at least one of common non-whitespace suffix or prefix.\nSo syntax highlighting should look like this:\n\n1. Both prefix and suffix are non empty and non whitespace only\n\n -foo -{bar} baz\n +foo +{quux} baz\n\n2. Non empty and non whitespace only prefix\n\n -foo -{bar}\n +foo +{quux}\n\n2. Non empty and non whitespace only suffix\n\n --{bar} baz\n ++{quux} baz\n\nBut in your code $prefix is not the length of common prefix, but\nthe position of end of prefix in the original line of diff. So\nyou start with $prefix = 1... even though the prefix is empty.\n\nHow is that supposed to work?\n\n\nOn Mon, 13 Feb 2012, Michał Kiedrowicz wrote:\n> Jakub Narebski wrote:\n> > On Mon, 13 Feb 2012, Michal Kiedrowicz wrote:\n\n> > > I haven't found *examples* on GitHub and Trac sites, but what about\n> > > these ones:\n> > > \n> > > https://github.com/gitster/git/commit/8cad4744ee37ebec1d9491a1381ec1771a1ba795\n> > > http://trac.edgewall.org/changeset/10973\n[...]\n> > BTW GitHub is closed source, but we can check what algorithm does Trac\n> > use for diff refinement highlighting (highlighting changed portions of\n> > diff).\n> > \n> \n> I think it's\n> http://trac.edgewall.org/browser/trunk/trac/versioncontrol/diff.py\n> (see markup intraline_changes()).\n \nIt is get_change_extent() that finds extent of changes, as a pair\ncontaining the offset at which the changes start, and the negative offset\nat which the changes end. So it is the same solution you use, only\nwithout ignoring whitespace-only prefixes and suffixes... This code can\nbe easily ported to Perl, BTW.\n\nThe markup_intraline_changes() function compares lines from preimage and\nfrom postimage pairwise, requiring that number of lines matches, the same\nlike in your algorithm.\n\n[...]\n> > I wonder if we can use --diff-words for diff refinement highlighting,\n> > i.e. LCS on words.\n> \n> I think we can try it, but I worry about performance of running `git\n> diff` on every diff chunk.\n\nI was thinking about one single additional run of git-diff-tree with\n`--diff-words`, not one per chunk.\n\nOr perhaps even put it together in one git-diff-tree invocation, just like\n'commitdiff' action / git_commitdiff() subroutine uses single git-diff-tree\ninvocation, with the option \"--patch-with-raw\", to generate both raw diff\nfor difftree and patchset.\n \n> > Anyway Jeff's approach is a bit limited, in that it would work only for\n> > change that does not involve adding newlines, for example splitting\n> > overly long line when changing something.\n> > \n> > See for example line 1786 (in pre-image) in http://trac.edgewall.org/changeset/10973\n> \n> Yes, I'm aware of that. I was thinking about improving it later (\"Let's\n> start with a simple refinment highlightning and maybe later add more\n> sophisticated algorithms\").\n\nRight.\n \n[...]\n> > BTW. is it \"at least one of prefix or suffix are non-empty\" or \"both prefix\n> > and suffix are non-empty\"?\n> > \n> \n> At least one. See:\n> \n> \t-a = 42;\n> \t+b = 42;\n> \n> Here prefix is empty but suffix is not.\n\nNb. prefix is empty but $prefix == 1, and is boolean true.\n\n> > > I would also consider ignoring prefixes/suffixes with punctuation, like:\n> > > \n> > > \t- * I like you.\n> > > \t+ * Alice had a little lamb.\n> > \n> > But this patch doesn't implement this feature yet, isn't it?\n> \n> No, but is a matter of adding\n> \n> \t-$prefix_is_space = 0 if ($r[$prefix] !~ /\\s/);\n> \t+$prefix_is_space = 0 if ($r[$prefix] !~ /\\s|[[:punct:]]/);\n> \n> (and the same for suffix)\n\nAll right. But it is better added as separate patch. Perhaps even\nrequiring that not only there is at least one of common prefix or common\nsuffix, but at least one of them is not whitespace only could be put\nin a separate commit...\n\n> > > > > +sub format_rem_add_line {\n> > > > > +\tmy ($rem, $add) = @_;\n> > > > > +\tmy @r = split(//, $rem);\n> > > > > +\tmy @a = split(//, $add);\n> > \n> > BTW the name of variable can be just @add and @rem.\n> >\n> \n> I know they are different scopes but I don't like it. It makes the code\n> more confusing IMO. But I won't insist.\n\nIn my opinion if the variable refers to the same entity in different\nforms, using @foo and %foo (used in gitweb), or $foo and @foo (could be\nused here) is all right, and even better than trying to come up with\ndifferent name for the same thing because of sigil.\n \n> > > > Shouldn't\n> > > > $prefix / $prefix_len start from 0, not from 1?\n> > > \n> > > It starts from 1 because it skips first +/-. It should become obvious\n> > > after reading the comment from last patch :).\n\nThis means that $prefix is true even if prefix is empty ($prefix == 1).\nWouldn't it be better for $prefix_len to count length of true prefix,\nwithout diff adornment? Or make @r / @rem skip initial characters...\n\n> > > +\t# In combined diff we must ignore two +/- characters.\n> > > +\t$prefix = 2 if ($is_combined);\n> > \n> > Anyway comment about that fact would be nice.\n> \n> Will do.\n\nBTW. it is not \"2\" but \"scalar @{$co{'parents'}}\".\n\n\n> > > The splitting and comparing by characters is taken from diff-highlight.\n> > > I don't think it's worth changing here.\n> > \n> > You are right.\n> > \n> > I'll try to come with hacky algorithm using string bitwise xor and regexp,\n> > and benchmark it comparing to your C-like solution, but it can be left for\n> > later (simple is better than clever, usually).\n> \n> If you have time :).\n\nAnyway it would be separate commit. Better to just copy tested code\nfrom contrib/diff-highlight\n\nBTW. would \"git blame -C -C -C -w\" detect this correctly as code\nmovement^W copying?\n \n-- \nJakub Narebski\nPoland\n"},{"id":"184664","messageId":"20120214192340.2d473866@gmail.com","threadId":"29593","inReplyTo":"201202141831.59699.jnareb@gmail.com","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-14T18:23:40Z","receivedAt":"2012-02-14T18:23:40Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jakub Narebski wrote:\n\n> A few issues I have just noticed about this series.\n> \n> \n> First, about naming. \"Highlighting interesting parts of diff\" is\n> acceptable name, but \"syntax highlighting for diff\" is not: gitweb\n> already use syntax highlighting in diff views. Call it \"diff refinement\n> highlighting\", \"highlighting changes / changed sections\", \"intraline\n> highlighting\".\n\nOK, I'll make sure the naming is correct and consistent.\n> \n> \n> Second, I think (but I am not sure) that there is a bug in code that\n> finds common suffix and prefix.\n> \n> If I understand correctly the idea is to highlight changed part if\n> there is at least one of common non-whitespace suffix or prefix.\n> So syntax highlighting should look like this:\n> \n> 1. Both prefix and suffix are non empty and non whitespace only\n> \n> -foo -{bar} baz\n> +foo +{quux} baz\n> \n> 2. Non empty and non whitespace only prefix\n> \n> -foo -{bar}\n> +foo +{quux}\n> \n> 2. Non empty and non whitespace only suffix\n> \n> --{bar} baz\n> ++{quux} baz\n> \n> But in your code $prefix is not the length of common prefix, but\n> the position of end of prefix in the original line of diff. So\n> you start with $prefix = 1... even though the prefix is empty.\n> \n> How is that supposed to work?\n\nBut see the check:\n\n+\t# Mark lines that are different from each other, but have some common\n+\t# part that isn't whitespace. If lines are completely different, don't\n+\t# mark them because that would make output unreadable, especially if\n+\t# diff consists of multiple lines.\n+\tif (($prefix == 1 && $suffix_rem == $#r && $suffix_add == $#a)\n ^\n |\n This part.\n\n+\t\t|| ($prefix_is_space && $suffix_is_space)) {\n+\t\t$esc_rem = esc_html($rem);\n+\t\t$esc_add = esc_html($add);\n+\t} else {\n+\t\t$esc_rem = esc_html_mark_range(\\@r, $prefix, $suffix_rem);\n+\t\t$esc_add = esc_html_mark_range(\\@a, $prefix, $suffix_add);\n+\t}\n\nI guess it's still not correct because it should be equal to number of\nparents or $prefix_length should start from 0 like you wrote later.\n\n> \n> \n> On Mon, 13 Feb 2012, Michał Kiedrowicz wrote:\n> > Jakub Narebski wrote:\n> > > On Mon, 13 Feb 2012, Michal Kiedrowicz wrote:\n> \n> > > > I haven't found *examples* on GitHub and Trac sites, but what about\n> > > > these ones:\n> > > > \n> > > > https://github.com/gitster/git/commit/8cad4744ee37ebec1d9491a1381ec1771a1ba795\n> > > > http://trac.edgewall.org/changeset/10973\n> [...]\n> > > BTW GitHub is closed source, but we can check what algorithm does Trac\n> > > use for diff refinement highlighting (highlighting changed portions of\n> > > diff).\n> > > \n> > \n> > I think it's\n> > http://trac.edgewall.org/browser/trunk/trac/versioncontrol/diff.py\n> > (see markup intraline_changes()).\n> \n> It is get_change_extent() that finds extent of changes, as a pair\n> containing the offset at which the changes start, and the negative offset\n> at which the changes end. So it is the same solution you use, only\n> without ignoring whitespace-only prefixes and suffixes... This code can\n> be easily ported to Perl, BTW.\n> \n> The markup_intraline_changes() function compares lines from preimage and\n> from postimage pairwise, requiring that number of lines matches, the same\n> like in your algorithm.\n> \n\nSo using Jeff's diff-highlight we remain quite consistent with Trac\noutput. There's nothing we can \"steal\" from it. \n\n> > > > I would also consider ignoring prefixes/suffixes with punctuation, like:\n> > > > \n> > > > \t- * I like you.\n> > > > \t+ * Alice had a little lamb.\n> > > \n> > > But this patch doesn't implement this feature yet, isn't it?\n> > \n> > No, but is a matter of adding\n> > \n> > \t-$prefix_is_space = 0 if ($r[$prefix] !~ /\\s/);\n> > \t+$prefix_is_space = 0 if ($r[$prefix] !~ /\\s|[[:punct:]]/);\n> > \n> > (and the same for suffix)\n> \n> All right. But it is better added as separate patch.\n\nSure \n\n> Perhaps even\n> requiring that not only there is at least one of common prefix or common\n> suffix, but at least one of them is not whitespace only could be put\n> in a separate commit...\n> \n> > > > > > +sub format_rem_add_line {\n> > > > > > +\tmy ($rem, $add) = @_;\n> > > > > > +\tmy @r = split(//, $rem);\n> > > > > > +\tmy @a = split(//, $add);\n> > > \n> > > BTW the name of variable can be just @add and @rem.\n> > >\n> > \n> > I know they are different scopes but I don't like it. It makes the code\n> > more confusing IMO. But I won't insist.\n> \n> In my opinion if the variable refers to the same entity in different\n> forms, using @foo and %foo (used in gitweb), or $foo and @foo (could be\n> used here) is all right, and even better than trying to come up with\n> different name for the same thing because of sigil.\n\nOK. I'll follow that convention then.\n\n> \n> > > > > Shouldn't\n> > > > > $prefix / $prefix_len start from 0, not from 1?\n> > > > \n> > > > It starts from 1 because it skips first +/-. It should become obvious\n> > > > after reading the comment from last patch :).\n> \n> This means that $prefix is true even if prefix is empty ($prefix == 1).\n> Wouldn't it be better for $prefix_len to count length of true prefix,\n> without diff adornment? Or make @r / @rem skip initial characters...\n> \n> > > > +\t# In combined diff we must ignore two +/- characters.\n> > > > +\t$prefix = 2 if ($is_combined);\n> > > \n> > > Anyway comment about that fact would be nice.\n> > \n> > Will do.\n> \n> BTW. it is not \"2\" but \"scalar @{$co{'parents'}}\".\n> \n\nOK.\n\n> \n> > > > The splitting and comparing by characters is taken from diff-highlight.\n> > > > I don't think it's worth changing here.\n> > > \n> > > You are right.\n> > > \n> > > I'll try to come with hacky algorithm using string bitwise xor and regexp,\n> > > and benchmark it comparing to your C-like solution, but it can be left for\n> > > later (simple is better than clever, usually).\n> > \n> > If you have time :).\n> \n> Anyway it would be separate commit. Better to just copy tested code\n> from contrib/diff-highlight\n> \n> BTW. would \"git blame -C -C -C -w\" detect this correctly as code\n> movement^W copying?\n> \n\nCannot say. Have you considered what I wrote in a separate e-mail,\nabout using diff-highlight output directly / as a library? \n"},{"id":"184668","messageId":"20120214185233.GA12072@sigill.intra.peff.net","threadId":"29593","inReplyTo":"20120214192340.2d473866@gmail.com","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-14T18:52:33Z","receivedAt":"2012-02-14T18:52:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 14, 2012 at 07:23:40PM +0100, Michał Kiedrowicz wrote:\n\n> > > > BTW GitHub is closed source, but we can check what algorithm does Trac\n> > > > use for diff refinement highlighting (highlighting changed portions of\n> > > > diff).\n> > > > \n> > > I think it's\n> > > http://trac.edgewall.org/browser/trunk/trac/versioncontrol/diff.py\n> > > (see markup intraline_changes()).\n> [...]\n> [...]\n> > The markup_intraline_changes() function compares lines from preimage and\n> > from postimage pairwise, requiring that number of lines matches, the same\n> > like in your algorithm.\n> \n> So using Jeff's diff-highlight we remain quite consistent with Trac\n> output. There's nothing we can \"steal\" from it.\n\nNeat. When I originally wrote diff-highlight, I was inspired by seeing\nwhat Trac and GitHub did, but came up with the algorithm on my own.\nLearning that Trac does the multiline thing (as we started doing with\nrecent patches) makes me feel even better that it's a good strategy.\n\nAs an aside, I looked at what GitHub does. It turns on highlighting only\nfor the single-line case, and does the same prefix/suffix thing.\n\nIt's really not very complex code; most of the hassle in diff-highlight\nis ignoring (but preserving) embedded colors, so that we build on top of\nexisting colorization. A web tool will be doing the line coloring itself\nanyway, so it can be much simpler. Elsewhere, I suggested lib-ifying\ndiff-highlight for gitweb to use. But once you remove the embedded color\nhandling, there really is not that much code, and it's probably simpler\nto just rewrite it.\n\n-Peff\n"},{"id":"184678","messageId":"20120214210453.16c6bd2e@gmail.com","threadId":"29593","inReplyTo":"20120214185233.GA12072@sigill.intra.peff.net","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-14T20:04:53Z","receivedAt":"2012-02-14T20:04:53Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jeff King wrote:\n\n> On Tue, Feb 14, 2012 at 07:23:40PM +0100, Michał Kiedrowicz wrote:\n> \n> > > > > BTW GitHub is closed source, but we can check what algorithm does Trac\n> > > > > use for diff refinement highlighting (highlighting changed portions of\n> > > > > diff).\n> > > > > \n> > > > I think it's\n> > > > http://trac.edgewall.org/browser/trunk/trac/versioncontrol/diff.py\n> > > > (see markup intraline_changes()).\n> > [...]\n> > [...]\n> > > The markup_intraline_changes() function compares lines from preimage and\n> > > from postimage pairwise, requiring that number of lines matches, the same\n> > > like in your algorithm.\n> > \n> > So using Jeff's diff-highlight we remain quite consistent with Trac\n> > output. There's nothing we can \"steal\" from it.\n> \n> Neat. When I originally wrote diff-highlight, I was inspired by seeing\n> what Trac and GitHub did, but came up with the algorithm on my own.\n> Learning that Trac does the multiline thing (as we started doing with\n> recent patches) makes me feel even better that it's a good strategy.\n> \n> As an aside, I looked at what GitHub does. It turns on highlighting only\n> for the single-line case, and does the same prefix/suffix thing.\n> \n> It's really not very complex code; most of the hassle in diff-highlight\n> is ignoring (but preserving) embedded colors, so that we build on top of\n> existing colorization. A web tool will be doing the line coloring itself\n> anyway, so it can be much simpler. Elsewhere, I suggested lib-ifying\n> diff-highlight for gitweb to use. But once you remove the embedded color\n> handling, there really is not that much code, and it's probably simpler\n> to just rewrite it.\n> \n> -Peff\n\nSure, now it's a simple algorithm, but if we add more code, we will\nhave problems with making it consistent in both gitweb and\ndiff-highlight (which is nice-to-have IMO). Note that my patches to\ngitweb already support combined diffs (in obvious cases) while\ndiff-highlight will fail on them badly (see for example 09bb4eb4f14c).\nI haven't done it in diff-highlight because I noticed that problem\nwhile working on patches for gitweb.\n\nAnyway, thanks for looking at Trac/GitHub code and sharing your opinion.\n"},{"id":"184691","messageId":"20120214203801.GB23291@sigill.intra.peff.net","threadId":"29593","inReplyTo":"20120214210453.16c6bd2e@gmail.com","subject":"Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-14T20:38:01Z","receivedAt":"2012-02-14T20:38:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 14, 2012 at 09:04:53PM +0100, Michał Kiedrowicz wrote:\n\n> Sure, now it's a simple algorithm, but if we add more code, we will\n> have problems with making it consistent in both gitweb and\n> diff-highlight (which is nice-to-have IMO).\n\nTrue. I was thinking they would stay simple enough that porting features\nwouldn't be too painful. But that might be overly optimistic.\n\n> Note that my patches to gitweb already support combined diffs (in\n> obvious cases) while diff-highlight will fail on them badly (see for\n> example 09bb4eb4f14c). I haven't done it in diff-highlight because I\n> noticed that problem while working on patches for gitweb.\n\nHmm. 09bb4eb4f14c looks fine to me, because all of the combined lines\nare from the left-hand side. A better example is 4802997, where the\nfirst hunk properly highlights, but the second does not (because we\ninterpret the lines as context lines).\n\nEven worse is \"git show 5de89d3 -- notes-cache.c\", where we match a \"- \"\nline with a \"++\" line, and erroneously think the changes begin in the\nsecond character (even though it is simply a combined-diff marker).\n\nThese are reasonably rare, so I don't consider them critical bugs, but\nyeah, it would be nice to fix.\n\n-Peff\n"},{"id":"185405","messageId":"20120225100019.719105c4@gmail.com","threadId":"29593","inReplyTo":"m3k43ttlh9.fsf@localhost.localdomain","subject":"Re: [PATCH 2/8] gitweb: Use print_diff_chunk() for both side-by-side and inline diffs","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-02-25T09:00:19Z","receivedAt":"2012-02-25T09:00:19Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jakub Narebski wrote:\n\n> BTW. I didn't examine the final code, but what happens for binary\n> diffs that git supports? Is it handled outside print_diff_chunk()?\n\ngitweb doesn't produce binary diffs at all except for \"?a=patch\", but\nit isn't HTML'ed. So I don't think there anything I can do with it.\n"}]}