{"thread":{"id":"30148","subject":"[PATCH v3 0/8] Highlight interesting parts of diff","startedAt":"2012-04-04T19:57:05Z","lastAt":"2012-04-06T08:36:03Z","messageCount":19,"participants":["Michał Kiedrowicz","Junio C Hamano","Jakub Narebski","Michal Kiedrowicz"],"isPatch":true,"patchVersion":3,"patchTotal":8},"messages":[{"id":"188513","messageId":"1333569433-3245-1-git-send-email-michal.kiedrowicz@gmail.com","threadId":"30148","inReplyTo":null,"subject":"[PATCH v3 0/8] Highlight interesting parts of diff","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-04-04T19:57:05Z","receivedAt":"2012-04-04T19:57:05Z","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, especially if\nlines differ only in few characters. This is often true when a commit fixes a\ntypo 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 GitHub.\n\nChanges since v2:\n\n* Added first patch (gitweb: Use descriptive names in esc_html_hl_regions())\n  suggested by Jakub.\n\n* Squashed patches (gitweb: Move HTML-formatting diff line back to\n  process_diff_line()) and (gitweb: Push formatting diff lines to\n  print_diff_chunk())\n\n* Reauthored patch (gitweb: Pass esc_html_hl_regions() options to esc_html())\n  to Jakub.\n\n* Fixed typos in commit messages and added comments based on Jakub's review.\n\n* Added Acked-by from Jakub.\n\n* Fixed minor issues in code pointed by Jakub (hopefully all; I had to drop\n  few due to overlong lines etc).\n\n* Renamed format_rem_add_line() to format_rem_add_lines_pair().\n\n* Renamed $prefix_is_space and $suffix_is_space to $prefix_has_nonspace and\n  $suffix_has_nonspace and simplified a condition\n\n* Moved untabify() and chomp() back to format_diff_line() and introduced in\n  format_rem_add_lines_pair().\n\n* Made red and green background a bit brighter.\n\nJakub Narębski (1):\n  gitweb: Pass esc_html_hl_regions() options to esc_html()\n\nMichał Kiedrowicz (7):\n  gitweb: Use descriptive names in esc_html_hl_regions()\n  gitweb: esc_html_hl_regions(): Don't create empty <span> elements\n  gitweb: Extract print_sidebyside_diff_lines()\n  gitweb: Use print_diff_chunk() for both side-by-side and inline diffs\n  gitweb: Push formatting diff lines to print_diff_chunk()\n  gitweb: Highlight interesting parts of diff\n  gitweb: Refinement highlightning in combined diffs\n\n gitweb/gitweb.perl       |  319 +++++++++++++++++++++++++++++++++-------------\n gitweb/static/gitweb.css |    8 +\n 2 files changed, 241 insertions(+), 86 deletions(-)\n\n-- \n1.7.8.4\n"},{"id":"188516","messageId":"1333569433-3245-2-git-send-email-michal.kiedrowicz@gmail.com","threadId":"30148","inReplyTo":"1333569433-3245-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH v3 1/8] gitweb: Use descriptive names in esc_html_hl_regions()","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-04-04T19:57:06Z","receivedAt":"2012-04-04T19:57:06Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"The $s->[0] and $s->[1] variables look a bit cryptic.  Let's rename them\nto $beg and $end so that it's clear what they do.\n\nSuggested-by: Jakub Narębski <jnareb@gmail.com>\nSigned-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>\n---\n gitweb/gitweb.perl |   10 ++++++----\n 1 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex a8b5fad..a3754ff 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1738,12 +1738,14 @@ sub esc_html_hl_regions {\n \tmy $pos = 0;\n \n \tfor my $s (@sel) {\n-\t\t$out .= esc_html(substr($str, $pos, $s->[0] - $pos))\n-\t\t\tif ($s->[0] - $pos > 0);\n+\t\tmy ($beg, $end) = @$s;\n+\n+\t\t$out .= esc_html(substr($str, $pos, $beg - $pos))\n+\t\t\tif ($beg - $pos > 0);\n \t\t$out .= $cgi->span({-class => $css_class},\n-\t\t                   esc_html(substr($str, $s->[0], $s->[1] - $s->[0])));\n+\t\t                   esc_html(substr($str, $beg, $end - $beg)));\n \n-\t\t$pos = $s->[1];\n+\t\t$pos = $end;\n \t}\n \t$out .= esc_html(substr($str, $pos))\n \t\tif ($pos < length($str));\n-- \n1.7.8.4\n"},{"id":"188514","messageId":"1333569433-3245-3-git-send-email-michal.kiedrowicz@gmail.com","threadId":"30148","inReplyTo":"1333569433-3245-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH v3 2/8] gitweb: esc_html_hl_regions(): Don't create empty <span> elements","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-04-04T19:57:07Z","receivedAt":"2012-04-04T19:57:07Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"If $end is equal to or less than $beg, esc_html_hl_regions()\ngenerates an empty <span> element.  It normally shouldn't be visible in\nthe web browser, but it doesn't look good when looking at page source.\nIt also minimally increases generated page size for no special reason.\n\nSigned-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>\nAcked-by: Jakub Narębski <jnareb@gmail.com>\n---\n gitweb/gitweb.perl |    3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex a3754ff..ca3058c 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1740,6 +1740,9 @@ sub esc_html_hl_regions {\n \tfor my $s (@sel) {\n \t\tmy ($beg, $end) = @$s;\n \n+\t\t# Don't create empty <span> elements.\n+\t\tnext if $end <= $beg; \n+\n \t\t$out .= esc_html(substr($str, $pos, $beg - $pos))\n \t\t\tif ($beg - $pos > 0);\n \t\t$out .= $cgi->span({-class => $css_class},\n-- \n1.7.8.4\n"},{"id":"188515","messageId":"1333569433-3245-4-git-send-email-michal.kiedrowicz@gmail.com","threadId":"30148","inReplyTo":"1333569433-3245-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH v3 3/8] gitweb: Pass esc_html_hl_regions() options to esc_html()","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-04-04T19:57:08Z","receivedAt":"2012-04-04T19:57:08Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"From: Jakub Narębski <jnareb@gmail.com>\n\nWith this change, esc_html_hl_regions() accepts options and passes them\ndown to esc_html().  This may be needed if a caller wants to pass\n-nbsp=>1 to esc_html().\n\nThe idea and implementation example of this change was described in\n337da8d2 (gitweb: Introduce esc_html_match_hl and esc_html_hl_regions,\n2012-02-27).  While other suggestions may be more useful in some cases,\nthere is no need to implement them at the moment.  The\nesc_html_hl_regions() interface may be changed later if it's needed.\n\n[mk: extracted from larger patch and wrote commit message]\n\nSigned-off-by: Jakub Narębski <jnareb@gmail.com>\nSigned-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>\n---\n gitweb/gitweb.perl |   10 ++++++----\n 1 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex ca3058c..d5f802f 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1732,7 +1732,9 @@ sub chop_and_escape_str {\n # '<span class=\"mark\">foo</span>bar'\n sub esc_html_hl_regions {\n \tmy ($str, $css_class, @sel) = @_;\n-\treturn esc_html($str) unless @sel;\n+\tmy %opts = grep { ref($_) ne 'ARRAY' } @sel;\n+\t@sel     = grep { ref($_) eq 'ARRAY' } @sel;\n+\treturn esc_html($str, %opts) unless @sel;\n \n \tmy $out = '';\n \tmy $pos = 0;\n@@ -1743,14 +1745,14 @@ sub esc_html_hl_regions {\n \t\t# Don't create empty <span> elements.\n \t\tnext if $end <= $beg; \n \n-\t\t$out .= esc_html(substr($str, $pos, $beg - $pos))\n+\t\t$out .= esc_html(substr($str, $pos, $beg - $pos), %opts)\n \t\t\tif ($beg - $pos > 0);\n \t\t$out .= $cgi->span({-class => $css_class},\n-\t\t                   esc_html(substr($str, $beg, $end - $beg)));\n+\t\t                   esc_html(substr($str, $beg, $end - $beg), %opts));\n \n \t\t$pos = $end;\n \t}\n-\t$out .= esc_html(substr($str, $pos))\n+\t$out .= esc_html(substr($str, $pos), %opts)\n \t\tif ($pos < length($str));\n \n \treturn $out;\n-- \n1.7.8.4\n"},{"id":"188517","messageId":"1333569433-3245-5-git-send-email-michal.kiedrowicz@gmail.com","threadId":"30148","inReplyTo":"1333569433-3245-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH v3 4/8] gitweb: Extract print_sidebyside_diff_lines()","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-04-04T19:57:09Z","receivedAt":"2012-04-04T19:57:09Z","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 is a preparation patch for diff refinement highlightning.  It should\nnot change the gitweb output, but it slightly changes its behavior.\nBefore this commit, context is printed on the class change. Now,  it's\nprinted just before printing added and removed lines, and at the end of\nchunk.\n\nSigned-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>\nAcked-by: Jakub Narębski <jnareb@gmail.com>\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 d5f802f..56721a3 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5000,6 +5000,53 @@ sub git_difftree_body {\n \tprint \"</table>\\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'<div class=\"chunk_block ctx\">',\n+\t\t\t\t'<div class=\"old\">',\n+\t\t\t\t@$ctx,\n+\t\t\t\t'</div>',\n+\t\t\t\t'<div class=\"new\">',\n+\t\t\t\t@$ctx,\n+\t\t\t\t'</div>',\n+\t\t\t'</div>';\n+\t}\n+\n+\tif (!@$add) {\n+\t\t# pure removal\n+\t\tprint join '',\n+\t\t\t'<div class=\"chunk_block rem\">',\n+\t\t\t\t'<div class=\"old\">',\n+\t\t\t\t@$rem,\n+\t\t\t\t'</div>',\n+\t\t\t'</div>';\n+\t} elsif (!@$rem) {\n+\t\t# pure addition\n+\t\tprint join '',\n+\t\t\t'<div class=\"chunk_block add\">',\n+\t\t\t\t'<div class=\"new\">',\n+\t\t\t\t@$add,\n+\t\t\t\t'</div>',\n+\t\t\t'</div>';\n+\t} else {\n+\t\t# assume that it is change\n+\t\tprint join '',\n+\t\t\t'<div class=\"chunk_block chg\">',\n+\t\t\t\t'<div class=\"old\">',\n+\t\t\t\t@$rem,\n+\t\t\t\t'</div>',\n+\t\t\t\t'<div class=\"new\">',\n+\t\t\t\t@$add,\n+\t\t\t\t'</div>',\n+\t\t\t'</div>';\n+\t}\n+}\n+\n sub print_sidebyside_diff_chunk {\n \tmy @chunk = @_;\n \tmy (@ctx, @rem, @add);\n@@ -5026,51 +5073,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'<div class=\"chunk_block ctx\">',\n-\t\t\t\t\t'<div class=\"old\">',\n-\t\t\t\t\t@ctx,\n-\t\t\t\t\t'</div>',\n-\t\t\t\t\t'<div class=\"new\">',\n-\t\t\t\t\t@ctx,\n-\t\t\t\t\t'</div>',\n-\t\t\t\t'</div>';\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'<div class=\"chunk_block rem\">',\n-\t\t\t\t\t\t'<div class=\"old\">',\n-\t\t\t\t\t\t@rem,\n-\t\t\t\t\t\t'</div>',\n-\t\t\t\t\t'</div>';\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'<div class=\"chunk_block add\">',\n-\t\t\t\t\t\t'<div class=\"new\">',\n-\t\t\t\t\t\t@add,\n-\t\t\t\t\t\t'</div>',\n-\t\t\t\t\t'</div>';\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'<div class=\"chunk_block chg\">',\n-\t\t\t\t\t\t'<div class=\"old\">',\n-\t\t\t\t\t\t@rem,\n-\t\t\t\t\t\t'</div>',\n-\t\t\t\t\t\t'<div class=\"new\">',\n-\t\t\t\t\t\t@add,\n-\t\t\t\t\t\t'</div>',\n-\t\t\t\t\t'</div>';\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.8.4\n"},{"id":"188520","messageId":"1333569433-3245-6-git-send-email-michal.kiedrowicz@gmail.com","threadId":"30148","inReplyTo":"1333569433-3245-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH v3 5/8] gitweb: Use print_diff_chunk() for both side-by-side and inline diffs","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-04-04T19:57:10Z","receivedAt":"2012-04-04T19:57:10Z","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, which is\nneeded for diff refinement highlightning (implemented in incoming\npatches).\n\nIf print_diff_chunk() was left as is, the new function\nprint_inline_diff_lines() could reorder diff lines.  It first prints all\ncontext lines, then all removed lines and finally all added lines.  If\nthe diff output consisted of mixed added and removed lines, gitweb would\nreorder these lines.  This is true for combined diff output, for\nexample:\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 <michal.kiedrowicz@gmail.com>\n---\n gitweb/gitweb.perl |   55 ++++++++++++++++++++++++++++++++++++---------------\n 1 files changed, 39 insertions(+), 16 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 56721a3..4f6b043 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5047,10 +5047,32 @@ 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 @$ctx, @$rem, @$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 \n+\t# The class of the previous line. \n+\tmy $prev_class = '';\n+\n \treturn unless @chunk;\n \n \t# incomplete last line might be among removed or added lines,\n@@ -5074,9 +5096,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    (@rem && @add && $class ne $prev_class)) {\n+\t\t\tprint_diff_lines(\\@ctx, \\@rem, \\@add,\n+\t\t                         $diff_style, $is_combined);\n \t\t\t@ctx = @rem = @add = ();\n \t\t}\n \n@@ -5093,6 +5119,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@@ -5219,22 +5247,17 @@ sub git_patchset_body {\n \t\t\t$diff_classes .= \" $class\" if ($class);\n \t\t\t$line = \"<div class=\\\"$diff_classes\\\">$line</div>\\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\t} else {\n-\t\t\t\t# default 'inline' style and unknown styles\n-\t\t\t\tprint $line;\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 = ();\n \t\t\t}\n+\n+\t\t\tpush @chunk, [ $class, $line ];\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 \"</div>\\n\"; # class=\"patch\"\n-- \n1.7.8.4\n"},{"id":"188518","messageId":"1333569433-3245-7-git-send-email-michal.kiedrowicz@gmail.com","threadId":"30148","inReplyTo":"1333569433-3245-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH v3 6/8] gitweb: Push formatting diff lines to print_diff_chunk()","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-04-04T19:57:11Z","receivedAt":"2012-04-04T19:57:11Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Now lines are formatted closer to place where we actually use HTML\nformatted output.\n\nThis means that we put raw lines in the @chunk accumulator, rather than\nformatted lines.  Because we still need to know class (type) of line\nwhen accumulating data to post-process and print, process_diff_line()\nsubroutine was retired and replaced by diff_line_class() used in\ngit_patchset_body() and new restructured format_diff_line() used in\nprint_diff_chunk().\n\nAs a side effect, we have to pass \\%from and \\%to down to callstack.\n\nThis is a preparation patch for diff refinement highlightning. It's not\nmeant to change gitweb output.\n\n[jn: wrote commit message]\n\nSigned-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>\nAcked-by: Jakub Narębski <jnareb@gmail.com>\n---\n\nJakub, I took your comment as-is as the message for this patch.  It seems to\ndescribe the change much better than my description.\n\n gitweb/gitweb.perl |   37 ++++++++++++++++++-------------------\n 1 files changed, 18 insertions(+), 19 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 4f6b043..926e83c 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2428,26 +2428,26 @@ 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 \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 = \"<div class=\\\"$diff_classes\\\">$line</div>\\n\";\n+\n+\treturn $line;\n }\n \n # Generates undef or something like \"_snapshot_\" or \"snapshot (_tbz2_ _zip_)\",\n@@ -5067,7 +5067,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 \n \t# The class of the previous line. \n@@ -5089,6 +5089,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@@ -5242,22 +5244,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 $diff_classes = \"diff\";\n-\t\t\t$diff_classes .= \" $class\" if ($class);\n-\t\t\t$line = \"<div class=\\\"$diff_classes\\\">$line</div>\\n\";\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\tprint_diff_chunk($diff_style, $is_combined, \\%from, \\%to, @chunk);\n \t\t\t\t@chunk = ();\n \t\t\t}\n \n-\t\t\tpush @chunk, [ $class, $line ];\n+\t\t\tpush @chunk, [ $class, $patch_line ];\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 \"</div>\\n\"; # class=\"patch\"\n-- \n1.7.8.4\n"},{"id":"188519","messageId":"1333569433-3245-8-git-send-email-michal.kiedrowicz@gmail.com","threadId":"30148","inReplyTo":"1333569433-3245-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH v3 7/8] gitweb: Highlight interesting parts of diff","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-04-04T19:57:12Z","receivedAt":"2012-04-04T19:57: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 with a light green/red background.  This should\nwork in the similar manner as in Trac or GitHub.\n\nThe algorithm that compares lines is based on contrib/diff-highlight.\nBasically, it works by determining common prefix/suffix of corresponding\nlines and highlightning only the middle part of lines.  For more\ninformation, see contrib/diff-highlight/README.\n\nCombined diffs are not supported but a following commit will change it.\n\nSince we need to pass esc_html()'ed or esc_html_hl_regions()'ed lines to\nformat_diff_lines(), so it was taught to accept preformatted lines\npassed as a reference.\n\nSigned-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>\nAcked-by: Jakub Narębski <jnareb@gmail.com>\n---\n gitweb/gitweb.perl       |  106 ++++++++++++++++++++++++++++++++++++++++-----\n gitweb/static/gitweb.css |    8 ++++\n 2 files changed, 102 insertions(+), 12 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 926e83c..579592a 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2428,19 +2428,25 @@ sub format_cc_diff_chunk_header {\n }\n \n # process patch (diff) line (not to be used for diff headers),\n-# returning HTML-formatted (but not wrapped) line\n+# returning HTML-formatted (but not wrapped) line.\n+# If the line is passed as a reference, it is treated as HTML and not\n+# 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-\n-\tif ($from && $to && $line =~ m/^\\@{2} /) {\n-\t\t$line = format_unidiff_chunk_header($line, $from, $to);\n-\t} elsif ($from && $to && $line =~ m/^\\@{3}/) {\n-\t\t$line = format_cc_diff_chunk_header($line, $from, $to);\n+\tif (ref($line)) {\n+\t\t$line = $$line;\n \t} else {\n-\t\t$line = esc_html($line, -nbsp=>1);\n+\t\tchomp $line;\n+\t\t$line = untabify($line);\n+\n+\t\tif ($from && $to && $line =~ m/^\\@{2} /) {\n+\t\t\t$line = format_unidiff_chunk_header($line, $from, $to);\n+\t\t} elsif ($from && $to && $line =~ m/^\\@{3}/) {\n+\t\t\t$line = format_cc_diff_chunk_header($line, $from, $to);\n+\t\t} else {\n+\t\t\t$line = esc_html($line, -nbsp=>1);\n+\t\t}\n \t}\n \n \tmy $diff_classes = \"diff\";\n@@ -5054,10 +5060,88 @@ sub print_inline_diff_lines {\n \tprint @$ctx, @$rem, @$add;\n }\n \n+# Format removed and added line, mark changed part and HTML-format them.\n+# Implementation is based on contrib/diff-highlight\n+sub format_rem_add_lines_pair {\n+\tmy ($rem, $add) = @_;\n+\n+\t# We need to untabify lines before split()'ing them;\n+\t# otherwise offsets would be invalid.\n+\tchomp $rem, $add;\n+\t$rem = untabify($rem);\n+\t$add = untabify($add);\n+\n+\tmy @rem = split(//, $rem);\n+\tmy @add = split(//, $add);\n+\tmy ($esc_rem, $esc_add);\n+\t# Ignore +/- character, thus $prefix_len is set to 1.\n+\tmy ($prefix_len, $suffix_len) = (1, 0);\n+\tmy ($prefix_has_nonspace, $suffix_has_nonspace);\n+\n+\tmy $shorter = (@rem < @add) ? @rem : @add;\n+\twhile ($prefix_len < $shorter) {\n+\t\tlast if ($rem[$prefix_len] ne $add[$prefix_len]);\n+\n+\t\t$prefix_has_nonspace = 1 if ($rem[$prefix_len] !~ /\\s/);\n+\t\t$prefix_len++;\n+\t}\n+\n+\twhile ($prefix_len + $suffix_len < $shorter) {\n+\t\tlast if ($rem[-1 - $suffix_len] ne $add[-1 - $suffix_len]);\n+\n+\t\t$suffix_has_nonspace = 1 if ($rem[-1 - $suffix_len] !~ /\\s/);\n+\t\t$suffix_len++;\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_has_nonspace || $suffix_has_nonspace) {\n+\t\t$esc_rem = esc_html_hl_regions($rem, 'marked',\n+\t\t        [$prefix_len, @rem - $suffix_len], -nbsp=>1);\n+\t\t$esc_add = esc_html_hl_regions($add, 'marked',\n+\t\t        [$prefix_len, @add - $suffix_len], -nbsp=>1);\n+\t} else {\n+\t\t$esc_rem = esc_html($rem, -nbsp=>1);\n+\t\t$esc_add = esc_html($add, -nbsp=>1);\n+\t}\n+\n+\treturn format_diff_line(\\$esc_rem, 'rem'),\n+\t       format_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, $is_combined) = @_;\n+\tmy (@new_ctx, @new_rem, @new_add);\n+\n+\t# Highlight if every removed line has a corresponding added line.\n+\t# Combined diffs are not supported at this moment.\n+\tif (!$is_combined && @$add > 0 && @$add == @$rem) {\n+\t\tfor (my $i = 0; $i < @$add; $i++) {\n+\t\t\tmy ($line_rem, $line_add) = format_rem_add_lines_pair(\n+\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($_, 'rem') } @$rem;\n+\t\t@new_add = map { format_diff_line($_, 'add') } @$add;\n+\t}\n+\n+\t@new_ctx = map { format_diff_line($_, 'ctx') } @$ctx;\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+\t        $is_combined);\n+\n \tif ($diff_style eq 'sidebyside' && !$is_combined) {\n \t\tprint_sidebyside_diff_lines($ctx, $rem, $add);\n \t} else {\n@@ -5089,11 +5173,9 @@ 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+\t\t\tprint format_diff_line($line, $class, $from, $to);\n \t\t\tnext;\n \t\t}\n \ndiff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css\nindex c530355..cb86d2d 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: #aaffaa;\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: #ffaaaa;\n+}\n+\n div.diff.chunk_header a,\n div.diff.chunk_header {\n \tcolor: #990099;\n-- \n1.7.8.4\n"},{"id":"188521","messageId":"1333569433-3245-9-git-send-email-michal.kiedrowicz@gmail.com","threadId":"30148","inReplyTo":"1333569433-3245-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH v3 8/8] gitweb: Refinement highlightning in combined diffs","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-04-04T19:57:13Z","receivedAt":"2012-04-04T19:57:13Z","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 is not obvious which removed and added lines should be compared.\n\nCurrent code requires that the number of added lines is equal to the\nnumber of removed lines and only skips first +/- character, treating\nsecond +/- as a line content, Thus, it is not possible to simply use\nexisting algorithm unchanged for combined diffs.\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\nIn other words, we require that pattern of '-'-es in pre-image matches\npattern of '+'-es in post-image.\n\nFurther changes may introduce more intelligent approach that better\nhandles combined diffs.\n\nSigned-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>\nAcked-by: Jakub Narębski <jnareb@gmail.com>\n---\n gitweb/gitweb.perl |   55 +++++++++++++++++++++++++++++++++++++++------------\n 1 files changed, 42 insertions(+), 13 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 579592a..e66aa84 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5063,7 +5063,7 @@ sub print_inline_diff_lines {\n # Format removed and added line, mark changed part and HTML-format them.\n # Implementation is based on contrib/diff-highlight\n sub format_rem_add_lines_pair {\n-\tmy ($rem, $add) = @_;\n+\tmy ($rem, $add, $num_parents) = @_;\n \n \t# We need to untabify lines before split()'ing them;\n \t# otherwise offsets would be invalid.\n@@ -5074,8 +5074,8 @@ sub format_rem_add_lines_pair {\n \tmy @rem = split(//, $rem);\n \tmy @add = split(//, $add);\n \tmy ($esc_rem, $esc_add);\n-\t# Ignore +/- character, thus $prefix_len is set to 1.\n-\tmy ($prefix_len, $suffix_len) = (1, 0);\n+\t# Ignore leading +/- characters for each parent.\n+\tmy ($prefix_len, $suffix_len) = ($num_parents, 0);\n \tmy ($prefix_has_nonspace, $suffix_has_nonspace);\n \n \tmy $shorter = (@rem < @add) ? @rem : @add;\n@@ -5113,15 +5113,43 @@ sub format_rem_add_lines_pair {\n \n # HTML-format diff context, removed and added lines.\n sub format_ctx_rem_add_lines {\n-\tmy ($ctx, $rem, $add, $is_combined) = @_;\n+\tmy ($ctx, $rem, $add, $num_parents) = @_;\n \tmy (@new_ctx, @new_rem, @new_add);\n+\tmy $can_highlight = 0;\n+\tmy $is_combined = ($num_parents > 1);\n \n \t# Highlight if every removed line has a corresponding added line.\n-\t# Combined diffs are not supported at this moment.\n-\tif (!$is_combined && @$add > 0 && @$add == @$rem) {\n+\tif (@$add > 0 && @$add == @$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 < @$add; $i++) {\n+\t\t\t\tmy $prefix_rem = substr($rem->[$i], 0, $num_parents);\n+\t\t\t\tmy $prefix_add = substr($add->[$i], 0, $num_parents);\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}\n+\n+\tif ($can_highlight) {\n \t\tfor (my $i = 0; $i < @$add; $i++) {\n \t\t\tmy ($line_rem, $line_add) = format_rem_add_lines_pair(\n-\t\t\t        $rem->[$i], $add->[$i]);\n+\t\t\t        $rem->[$i], $add->[$i], $num_parents);\n \t\t\tpush @new_rem, $line_rem;\n \t\t\tpush @new_add, $line_add;\n \t\t}\n@@ -5137,10 +5165,11 @@ sub format_ctx_rem_add_lines {\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+\tmy ($ctx, $rem, $add, $diff_style, $num_parents) = @_;\n+\tmy $is_combined = $num_parents > 1;\n \n \t($ctx, $rem, $add) = format_ctx_rem_add_lines($ctx, $rem, $add,\n-\t        $is_combined);\n+\t        $num_parents);\n \n \tif ($diff_style eq 'sidebyside' && !$is_combined) {\n \t\tprint_sidebyside_diff_lines($ctx, $rem, $add);\n@@ -5151,7 +5180,7 @@ sub print_diff_lines {\n }\n \n sub print_diff_chunk {\n-\tmy ($diff_style, $is_combined, $from, $to, @chunk) = @_;\n+\tmy ($diff_style, $num_parents, $from, $to, @chunk) = @_;\n \tmy (@ctx, @rem, @add);\n \n \t# The class of the previous line. \n@@ -5186,7 +5215,7 @@ sub print_diff_chunk {\n \t\tif (((@rem || @add) && $class eq 'ctx') || !$class ||\n \t\t    (@rem && @add && $class ne $prev_class)) {\n \t\t\tprint_diff_lines(\\@ctx, \\@rem, \\@add,\n-\t\t                         $diff_style, $is_combined);\n+\t\t                         $diff_style, $num_parents);\n \t\t\t@ctx = @rem = @add = ();\n \t\t}\n \n@@ -5329,7 +5358,7 @@ sub git_patchset_body {\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, \\%from, \\%to, @chunk);\n+\t\t\t\tprint_diff_chunk($diff_style, scalar @hash_parents, \\%from, \\%to, @chunk);\n \t\t\t\t@chunk = ();\n \t\t\t}\n \n@@ -5338,7 +5367,7 @@ sub git_patchset_body {\n \n \t} continue {\n \t\tif (@chunk) {\n-\t\t\tprint_diff_chunk($diff_style, $is_combined, \\%from, \\%to, @chunk);\n+\t\t\tprint_diff_chunk($diff_style, scalar @hash_parents, \\%from, \\%to, @chunk);\n \t\t\t@chunk = ();\n \t\t}\n \t\tprint \"</div>\\n\"; # class=\"patch\"\n-- \n1.7.8.4\n"},{"id":"188536","messageId":"7vwr5v6uts.fsf@alter.siamese.dyndns.org","threadId":"30148","inReplyTo":"1333569433-3245-2-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH v3 1/8] gitweb: Use descriptive names in esc_html_hl_regions()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-04T21:38:55Z","receivedAt":"2012-04-04T21:38:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:\n\n> The $s->[0] and $s->[1] variables look a bit cryptic.  Let's rename them\n> to $beg and $end so that it's clear what they do.\n\nWhy not $begin and $end?\n\n>\n> Suggested-by: Jakub Narębski <jnareb@gmail.com>\n> Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>\n> ---\n>  gitweb/gitweb.perl |   10 ++++++----\n>  1 files changed, 6 insertions(+), 4 deletions(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index a8b5fad..a3754ff 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1738,12 +1738,14 @@ sub esc_html_hl_regions {\n>  \tmy $pos = 0;\n>  \n>  \tfor my $s (@sel) {\n> -\t\t$out .= esc_html(substr($str, $pos, $s->[0] - $pos))\n> -\t\t\tif ($s->[0] - $pos > 0);\n> +\t\tmy ($beg, $end) = @$s;\n> +\n> +\t\t$out .= esc_html(substr($str, $pos, $beg - $pos))\n> +\t\t\tif ($beg - $pos > 0);\n>  \t\t$out .= $cgi->span({-class => $css_class},\n> -\t\t                   esc_html(substr($str, $s->[0], $s->[1] - $s->[0])));\n> +\t\t                   esc_html(substr($str, $beg, $end - $beg)));\n>  \n> -\t\t$pos = $s->[1];\n> +\t\t$pos = $end;\n>  \t}\n>  \t$out .= esc_html(substr($str, $pos))\n>  \t\tif ($pos < length($str));\n"},{"id":"188538","messageId":"7vsjgj6ufi.fsf@alter.siamese.dyndns.org","threadId":"30148","inReplyTo":"1333569433-3245-5-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH v3 4/8] gitweb: Extract print_sidebyside_diff_lines()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-04T21:47:29Z","receivedAt":"2012-04-04T21:47:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:\n\n> +\tif (!@$add) {\n> +\t\t# pure removal\n> +...\n> +\t} elsif (!@$rem) {\n> +\t\t# pure addition\n> +...\n> +\t} else {\n> +\t\t# assume that it is change\n> +\t\tprint join '',\n\nI know this is not a new problem, but if your patch hunk has both '-' and\n'+' lines, what's there to \"assume\" that it is a change?  Isn't it always?\n\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> -...\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\nThis seems to change the condition.  Earlier, it held true if (there is\nanything to show), and (class is unset or equal to ctx).  The new code\nsays something different.  Also can $class be undef, and if so, doesn't\nit trigger comparison between undef and 'ctx' by having !$class check at\nthe end of || chain?\n"},{"id":"188539","messageId":"201204050047.10357.jnareb@gmail.com","threadId":"30148","inReplyTo":"7vsjgj6ufi.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 4/8] gitweb: Extract print_sidebyside_diff_lines()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-04-04T22:47:09Z","receivedAt":"2012-04-04T22:47:09Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:\n> \n> > +\tif (!@$add) {\n> > +\t\t# pure removal\n> > +...\n> > +\t} elsif (!@$rem) {\n> > +\t\t# pure addition\n> > +...\n> > +\t} else {\n> > +\t\t# assume that it is change\n> > +\t\tprint join '',\n> \n> I know this is not a new problem, but if your patch hunk has both '-' and\n> '+' lines, what's there to \"assume\" that it is a change?  Isn't it always?\n\nWhat I meant here when I was writing it that they are lines that changed\nbetween two versions, like '!' in original (not unified) context format.\n\nWe can omit this comment.\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> > -...\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> This seems to change the condition.  Earlier, it held true if (there is\n> anything to show), and (class is unset or equal to ctx).  The new code\n> says something different.\n\nYes it does, as described in the commit message:\n\n                                                    [...] It should\n  not change the gitweb output, but it **slightly changes its behavior**.\n  Before this commit, context is printed on the class change. Now,  it's\n  printed just before printing added and removed lines, and at the end of\n  chunk.\n\nThe difference is that context lines are also printed accumulated now.\nThough why this change is required for refactoring could have been\ndescribed in more detail...\n\n>                             Also can $class be undef, and if so, doesn't \n> it trigger comparison between undef and 'ctx' by having !$class check at\n> the end of || chain?\n\nThanks for noticing this (I wonder why testsuite didn't caught it).\nIt should be\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 (!$class || ((@rem || @add) && $class eq 'ctx')) {\n\n-- \nJakub Narebski\nPoland\n"},{"id":"188549","messageId":"20120405074619.39174f54@mkiedrowicz.ivo.pl","threadId":"30148","inReplyTo":"7vwr5v6uts.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 1/8] gitweb: Use descriptive names in esc_html_hl_regions()","fromName":"Michal Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-04-05T05:46:19Z","receivedAt":"2012-04-05T05:46:19Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n\n> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:\n> \n> > The $s->[0] and $s->[1] variables look a bit cryptic.  Let's rename\n> > them to $beg and $end so that it's clear what they do.\n> \n> Why not $begin and $end?\n\nFor no special reason.  I just took the names that Jakub proposed in\n\n\thttp://article.gmane.org/gmane.comp.version-control.git/193839/\n> \n> >\n> > Suggested-by: Jakub Narębski <jnareb@gmail.com>\n> > Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>\n> > ---\n> >  gitweb/gitweb.perl |   10 ++++++----\n> >  1 files changed, 6 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> > index a8b5fad..a3754ff 100755\n> > --- a/gitweb/gitweb.perl\n> > +++ b/gitweb/gitweb.perl\n> > @@ -1738,12 +1738,14 @@ sub esc_html_hl_regions {\n> >  \tmy $pos = 0;\n> >  \n> >  \tfor my $s (@sel) {\n> > -\t\t$out .= esc_html(substr($str, $pos, $s->[0] -\n> > $pos))\n> > -\t\t\tif ($s->[0] - $pos > 0);\n> > +\t\tmy ($beg, $end) = @$s;\n> > +\n> > +\t\t$out .= esc_html(substr($str, $pos, $beg - $pos))\n> > +\t\t\tif ($beg - $pos > 0);\n> >  \t\t$out .= $cgi->span({-class => $css_class},\n> > -\t\t                   esc_html(substr($str, $s->[0],\n> > $s->[1] - $s->[0])));\n> > +\t\t                   esc_html(substr($str, $beg,\n> > $end - $beg))); \n> > -\t\t$pos = $s->[1];\n> > +\t\t$pos = $end;\n> >  \t}\n> >  \t$out .= esc_html(substr($str, $pos))\n> >  \t\tif ($pos < length($str));\n"},{"id":"188550","messageId":"20120405080633.3836f49b@mkiedrowicz.ivo.pl","threadId":"30148","inReplyTo":"201204050047.10357.jnareb@gmail.com","subject":"Re: [PATCH v3 4/8] gitweb: Extract print_sidebyside_diff_lines()","fromName":"Michal Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-04-05T06:06:33Z","receivedAt":"2012-04-05T06:06:33Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> wrote:\n\n> Junio C Hamano wrote:\n> > Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:\n> > \n> > > +\tif (!@$add) {\n> > > +\t\t# pure removal\n> > > +...\n> > > +\t} elsif (!@$rem) {\n> > > +\t\t# pure addition\n> > > +...\n> > > +\t} else {\n> > > +\t\t# assume that it is change\n> > > +\t\tprint join '',\n> > \n> > I know this is not a new problem, but if your patch hunk has both\n> > '-' and '+' lines, what's there to \"assume\" that it is a change?\n> > Isn't it always?\n> \n> What I meant here when I was writing it that they are lines that\n> changed between two versions, like '!' in original (not unified)\n> context format.\n> \n> We can omit this comment.\n\nOK.\n\n> \n> > > -\t\t# empty add/rem block on start context block, or\n> > > end of chunk\n> > > -\t\tif ((@rem || @add) && (!$class || $class eq\n> > > 'ctx')) { -...\n> > > +\t\t## print from accumulator when have some add/rem\n> > > lines or end\n> > > +\t\t# of chunk (flush context lines)\n> > > +\t\tif (((@rem || @add) && $class eq 'ctx')\n> > > || !$class) {\n> > \n> > This seems to change the condition.  Earlier, it held true if\n> > (there is anything to show), and (class is unset or equal to ctx).\n> > The new code says something different.\n> \n> Yes it does, as described in the commit message:\n> \n>                                                     [...] It should\n>   not change the gitweb output, but it **slightly changes its\n> behavior**. Before this commit, context is printed on the class\n> change. Now,  it's printed just before printing added and removed\n> lines, and at the end of chunk.\n> \n> The difference is that context lines are also printed accumulated now.\n> Though why this change is required for refactoring could have been\n> described in more detail...\n\nI changed that because I wanted to squash both conditions (the one that\nchecks if @ctx should be printed and the one that prints @add/@rem\nlines) and have just one call to print_sidebyside_diff_lines().  Later,\nthis function is changed to print_diff_lines() and controls whether\n'inline' or 'side-by-side' diff should be printed.  Having two\nconditions and two calls/functions would make the code redundant.  Then\nI thought that instead of calling twice print_sidebyside_diff_lines()\n(for @ctx and @add/@rem lines, like the code from pre-image prints\nthese lines separatedly), I can just call it once.\n\nI can revert this change to previous behavior but I think that would\nmake the condition more complicated.\n\n> \n> >                             Also can $class be undef, and if so,\n> > doesn't it trigger comparison between undef and 'ctx' by\n> > having !$class check at the end of || chain?\n> \n> Thanks for noticing this (I wonder why testsuite didn't caught it).\n> It should be\n> \n>  +\t\t## print from accumulator when have some add/rem\n> lines or end\n>  +\t\t# of chunk (flush context lines)\n>  +\t\tif (!$class || ((@rem || @add) && $class eq 'ctx'))\n> {\n> \n\nOK, I'll fix that.\n"},{"id":"188551","messageId":"20120405082557.7223ff6e@mkiedrowicz.ivo.pl","threadId":"30148","inReplyTo":"1333569433-3245-8-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH v3 7/8] gitweb: Highlight interesting parts of diff","fromName":"Michal Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-04-05T06:25:57Z","receivedAt":"2012-04-05T06:25:57Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Michał Kiedrowicz <michal.kiedrowicz@gmail.com> wrote:\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> \n> This commit teaches gitweb to highlight characters that are different\n> between old and new line with a light green/red background.  This\n> should work in the similar manner as in Trac or GitHub.\n> \n> The algorithm that compares lines is based on contrib/diff-highlight.\n> Basically, it works by determining common prefix/suffix of\n> corresponding lines and highlightning only the middle part of lines.\n> For more information, see contrib/diff-highlight/README.\n> \n> Combined diffs are not supported but a following commit will change\n> it.\n> \n> Since we need to pass esc_html()'ed or esc_html_hl_regions()'ed lines\n> to format_diff_lines(), so it was taught to accept preformatted lines\n> passed as a reference.\n> \n> Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>\n> Acked-by: Jakub Narębski <jnareb@gmail.com>\n\nJunio,\n\ncan you please fixup this patch?  I just noticed \"chomp $rem, $add\" breaks\nthe testsuite.\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex e4351fe..961fbdc 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5067,7 +5067,8 @@ sub format_rem_add_lines_pair {\n\n        # We need to untabify lines before split()'ing them;\n        # otherwise offsets would be invalid.\n-       chomp $rem, $add;\n+       chomp $rem;\n+       chomp $add;\n        $rem = untabify($rem);\n        $add = untabify($add);\n"},{"id":"188602","messageId":"201204060057.34138.jnareb@gmail.com","threadId":"30148","inReplyTo":"20120405080633.3836f49b@mkiedrowicz.ivo.pl","subject":"Re: [PATCH v3 4/8] gitweb: Extract print_sidebyside_diff_lines()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-04-05T22:57:33Z","receivedAt":"2012-04-05T22:57:33Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Michal Kiedrowicz wrote:\n> Jakub Narebski <jnareb@gmail.com> wrote:\n>> Junio C Hamano wrote:\n>>> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:\n\n>>>> -\t\t# empty add/rem block on start context block, or\n>>>> end of chunk\n>>>> -\t\tif ((@rem || @add) && (!$class || $class eq\n>>>> 'ctx')) { -...\n>>>> +\t\t## print from accumulator when have some add/rem\n>>>> lines or end\n>>>> +\t\t# of chunk (flush context lines)\n>>>> +\t\tif (((@rem || @add) && $class eq 'ctx')\n>>>> || !$class) {\n>>> \n>>> This seems to change the condition.  Earlier, it held true if\n>>> (there is anything to show), and (class is unset or equal to ctx).\n>>> The new code says something different.\n>> \n>> Yes it does, as described in the commit message:\n>> \n>>                                                     [...] It should\n>>   not change the gitweb output, but it **slightly changes its\n>> behavior**. Before this commit, context is printed on the class\n>> change. Now,  it's printed just before printing added and removed\n>> lines, and at the end of chunk.\n>> \n>> The difference is that context lines are also printed accumulated now.\n>> Though why this change is required for refactoring could have been\n>> described in more detail...\n> \n> I changed that because I wanted to squash both conditions (the one that\n> checks if @ctx should be printed and the one that prints @add/@rem\n> lines) and have just one call to print_sidebyside_diff_lines().  Later,\n> this function is changed to print_diff_lines() and controls whether\n> 'inline' or 'side-by-side' diff should be printed.  Having two\n> conditions and two calls/functions would make the code redundant.  Then\n> I thought that instead of calling twice print_sidebyside_diff_lines()\n> (for @ctx and @add/@rem lines, like the code from pre-image prints\n> these lines separatedly), I can just call it once.\n> \n> I can revert this change to previous behavior but I think that would\n> make the condition more complicated.\n\nNo, I think that this change is good idea if it simplifies code flow.\nBut it really should be described in commit message, not only \"what\"\n(which you did describe), but also \"whys\".\n\n-- \nJakub Narebski\nPoland\n"},{"id":"188605","messageId":"201204060126.38337.jnareb@gmail.com","threadId":"30148","inReplyTo":"1333569433-3245-6-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH v3 5/8] gitweb: Use print_diff_chunk() for both side-by-side and inline diffs","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-04-05T23:26:37Z","receivedAt":"2012-04-05T23:26:37Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Michał Kiedrowicz wrote:\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, which is\n> needed for diff refinement highlightning (implemented in incoming\n> patches).\n> \n> If print_diff_chunk() was left as is, the new function\n> print_inline_diff_lines() could reorder diff lines.  It first prints all\n> context lines, then all removed lines and finally all added lines.  If\n> the diff output consisted of mixed added and removed lines, gitweb would\n> reorder these lines.  This is true for combined diff output, for\n> 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> 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                                                 , and at the end of hunk.\n\n\nI think it is worth adding the above to the commit message.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"188644","messageId":"20120406103452.1544e88f@mkiedrowicz.ivo.pl","threadId":"30148","inReplyTo":"201204060126.38337.jnareb@gmail.com","subject":"Re: [PATCH v3 5/8] gitweb: Use print_diff_chunk() for both side-by-side and inline diffs","fromName":"Michal Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-04-06T08:34:52Z","receivedAt":"2012-04-06T08:34:52Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> wrote:\n\n> Michał Kiedrowicz wrote:\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>                                                  , and at the end of\n> hunk.\n> \n> \n> I think it is worth adding the above to the commit message.\n> \n\nOK, will do.\n"},{"id":"188645","messageId":"20120406103603.1f1ee90d@mkiedrowicz.ivo.pl","threadId":"30148","inReplyTo":"201204060057.34138.jnareb@gmail.com","subject":"Re: [PATCH v3 4/8] gitweb: Extract print_sidebyside_diff_lines()","fromName":"Michal Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-04-06T08:36:03Z","receivedAt":"2012-04-06T08:36:03Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> wrote:\n\n> Michal Kiedrowicz wrote:\n> > Jakub Narebski <jnareb@gmail.com> wrote:\n> >> Junio C Hamano wrote:\n> >>> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:\n> \n> >>>> -\t\t# empty add/rem block on start context block, or\n> >>>> end of chunk\n> >>>> -\t\tif ((@rem || @add) && (!$class || $class eq\n> >>>> 'ctx')) { -...\n> >>>> +\t\t## print from accumulator when have some add/rem\n> >>>> lines or end\n> >>>> +\t\t# of chunk (flush context lines)\n> >>>> +\t\tif (((@rem || @add) && $class eq 'ctx')\n> >>>> || !$class) {\n> >>> \n> >>> This seems to change the condition.  Earlier, it held true if\n> >>> (there is anything to show), and (class is unset or equal to ctx).\n> >>> The new code says something different.\n> >> \n> >> Yes it does, as described in the commit message:\n> >> \n> >>                                                     [...] It should\n> >>   not change the gitweb output, but it **slightly changes its\n> >> behavior**. Before this commit, context is printed on the class\n> >> change. Now,  it's printed just before printing added and removed\n> >> lines, and at the end of chunk.\n> >> \n> >> The difference is that context lines are also printed accumulated\n> >> now. Though why this change is required for refactoring could have\n> >> been described in more detail...\n> > \n> > I changed that because I wanted to squash both conditions (the one\n> > that checks if @ctx should be printed and the one that prints\n> > @add/@rem lines) and have just one call to\n> > print_sidebyside_diff_lines().  Later, this function is changed to\n> > print_diff_lines() and controls whether 'inline' or 'side-by-side'\n> > diff should be printed.  Having two conditions and two\n> > calls/functions would make the code redundant.  Then I thought that\n> > instead of calling twice print_sidebyside_diff_lines() (for @ctx\n> > and @add/@rem lines, like the code from pre-image prints these\n> > lines separatedly), I can just call it once.\n> > \n> > I can revert this change to previous behavior but I think that would\n> > make the condition more complicated.\n> \n> No, I think that this change is good idea if it simplifies code flow.\n> But it really should be described in commit message, not only \"what\"\n> (which you did describe), but also \"whys\".\n> \n\nSure, I'll try to put my explanation to the commit message.\n"}]}