{"thread":{"id":"30043","subject":"[PATCH v2 0/8] gitweb: Highlight interesting parts of diff","startedAt":"2012-03-23T22:56:49Z","lastAt":"2012-03-30T13:37:38Z","messageCount":30,"participants":["Michał Kiedrowicz","Jakub Narebski","Michal Kiedrowicz"],"isPatch":true,"patchVersion":2,"patchTotal":8},"messages":[{"id":"187613","messageId":"1332543417-19664-1-git-send-email-michal.kiedrowicz@gmail.com","threadId":"30043","inReplyTo":null,"subject":"[PATCH v2 0/8] gitweb: Highlight interesting parts of diff","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-03-23T22:56:49Z","receivedAt":"2012-03-23T22:56:49Z","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 v1:\n\n* Rebased on origin/master.\n\n* Added two patches at the beginning of the series. First one is a small fix\n and second one adds a feature to esc_html_hl_regions() that will be needed\n by later patches.\n\n* Squashed patch \"gitweb: Format diff lines just before printing\" and reworked\n changes to format_diff_line(). Now it accepts both raw line that needs to be\n html_esc()'ed and already escaped line.\n\n* Squashed changes in CSS with previous patch as proposed by Jakub.\n\n* Dropped esc_html_mark_change() and started using esc_html_hl_regions()\n because it was merged to origin/master.\n\n* Fixed highlightning for combined diffs with more than two parents.\n\n* Fixed indentation (tabs -> spaces) in conditions.\n\n* Ensured that patches with \"\\ No newline at end of file\" are properly\n displayed.\n\n* Ensured that binary patches are not supported in HTML format, thus this\n series canot break it :) (answering Jakub's questions) \n\n* Reworded some commit messages and fixed typos.\n\n* Renamed few variables after comments from Jakub.\n\n* Added some comments to the code.\n\n* Added Acked-by from Jakub for few patches.\n\nMichał Kiedrowicz (8):\n gitweb: esc_html_hl_regions(): Don't create empty elements\n gitweb: Pass esc_html_hl_regions() options to esc_html()\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: Highlight interesting parts of diff\n gitweb: Refinement highlightning in combined diffs\n\n gitweb/gitweb.perl | 299 +++++++++++++++++++++++++++++++++-------------\n gitweb/static/gitweb.css | 8 ++\n 2 files changed, 225 insertions(+), 82 deletions(-)\n\n-- \n1.7.8.4\n"},{"id":"187615","messageId":"1332543417-19664-2-git-send-email-michal.kiedrowicz@gmail.com","threadId":"30043","inReplyTo":"1332543417-19664-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH v2 1/8] gitweb: esc_html_hl_regions(): Don't create empty elements","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-03-23T22:56:50Z","receivedAt":"2012-03-23T22:56:50Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"if $s->[1] is equal to or less than $s->[0], esc_html_hl_regions()\ngenerates an empty element. It normally shouldn't be visible in\nthe web broweser, 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 \n---\n\nI don't know if any code currently can produce empty elements,\nbut diff refinement highlighning does it.\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 a8b5fad..af645e5 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1738,6 +1738,9 @@ sub esc_html_hl_regions {\n \tmy $pos = 0;\n \n \tfor my $s (@sel) {\n+\t\t# Don't craete empty elements.\n+\t\tnext if $s->[1] <= $s->[0];\n+\n \t\t$out .= esc_html(substr($str, $pos, $s->[0] - $pos))\n \t\t\tif ($s->[0] - $pos > 0);\n \t\t$out .= $cgi->span({-class => $css_class},\n-- \n1.7.8.4\n"},{"id":"187614","messageId":"1332543417-19664-3-git-send-email-michal.kiedrowicz@gmail.com","threadId":"30043","inReplyTo":"1332543417-19664-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH v2 2/8] gitweb: Pass esc_html_hl_regions() options to esc_html()","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-03-23T22:56:51Z","receivedAt":"2012-03-23T22:56:51Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"With 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\nSigned-off-by: Michał Kiedrowicz \n---\n\nJakub, The code was in fact stolen from a yours patch posted to the\nlist (gitweb: Use esc_html_match_hl() in 'grep' search) that wasn't\nmerged so maybe I should pass the authorship to you?\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 af645e5..1744c40 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1732,7 +1732,9 @@ sub chop_and_escape_str {\n # 'foobar'\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@@ -1741,14 +1743,14 @@ sub esc_html_hl_regions {\n \t\t# Don't craete empty elements.\n \t\tnext if $s->[1] <= $s->[0];\n \n-\t\t$out .= esc_html(substr($str, $pos, $s->[0] - $pos))\n+\t\t$out .= esc_html(substr($str, $pos, $s->[0] - $pos), %opts)\n \t\t\tif ($s->[0] - $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, $s->[0], $s->[1] - $s->[0]), %opts));\n \n \t\t$pos = $s->[1];\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":"187620","messageId":"1332543417-19664-4-git-send-email-michal.kiedrowicz@gmail.com","threadId":"30043","inReplyTo":"1332543417-19664-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH v2 3/8] gitweb: Extract print_sidebyside_diff_lines()","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-03-23T22:56:52Z","receivedAt":"2012-03-23T22:56:52Z","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'it\nprinted just before printing added and removed lines.\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 1744c40..86d5a02 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -4998,6 +4998,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@@ -5024,51 +5071,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.8.4\n"},{"id":"187616","messageId":"1332543417-19664-5-git-send-email-michal.kiedrowicz@gmail.com","threadId":"30043","inReplyTo":"1332543417-19664-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH v2 4/8] gitweb: Use print_diff_chunk() for both side-by-side and inline diffs","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-03-23T22:56:53Z","receivedAt":"2012-03-23T22:56:53Z","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 left as is, the new function print_inline_diff_lines() could reorder\ndiff lines. It first prints all context lines, then all removed lines\nand finally all added lines. If the diff output consisted of mixed added\nand removed lines, gitweb would reorder these lines. This is 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 | 55 +++++++++++++++++++++++++++++++++++++--------------\n 1 files changed, 40 insertions(+), 15 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 86d5a02..1a2f258 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5045,10 +5045,34 @@ 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 \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@@ -5072,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, $diff_style,\n+\t\t\t\t\t$is_combined);\n \t\t\t@ctx = @rem = @add = ();\n \t\t}\n \n@@ -5091,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@@ -5217,22 +5247,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.8.4\n"},{"id":"187617","messageId":"1332543417-19664-6-git-send-email-michal.kiedrowicz@gmail.com","threadId":"30043","inReplyTo":"1332543417-19664-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH v2 5/8] gitweb: Move HTML-formatting diff line back to process_diff_line()","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-03-23T22:56:54Z","receivedAt":"2012-03-23T22:56:54Z","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 \nAcked-by: Jakub Narębski \n---\n gitweb/gitweb.perl | 16 ++++++++--------\n 1 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 1a2f258..aacf518 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2438,14 +2438,17 @@ 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@@ -5243,9 +5246,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.8.4\n"},{"id":"187621","messageId":"1332543417-19664-7-git-send-email-michal.kiedrowicz@gmail.com","threadId":"30043","inReplyTo":"1332543417-19664-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH v2 6/8] gitweb: Push formatting diff lines to print_diff_chunk()","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-03-23T22:56:55Z","receivedAt":"2012-03-23T22:56:55Z","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(), which is removed\nfrom process_diff_line(). The latter function is renamed to\nformat_diff_line() and its output is changed to return only\nHTML-formatted line, which brings it in line with outher format_*\nsubroutined.\n\nThis slightly changes the order of operations performed on diff lines.\nBefore this commit, each read line was formatted and then put to the\n@chunk accumulator. Now, lines are formatted inside print_diff_chunk(),\n\nThis is a preparation patch for diff refinement highlightning. It's not\nmeant to change gitweb output.\n\nSigned-off-by: Michał Kiedrowicz \nAcked-by: Jakub Narębski \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 aacf518..db32588 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2426,12 +2426,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@@ -2448,7 +2445,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@@ -5070,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@@ -5092,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@@ -5245,19 +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 $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.8.4\n"},{"id":"187618","messageId":"1332543417-19664-8-git-send-email-michal.kiedrowicz@gmail.com","threadId":"30043","inReplyTo":"1332543417-19664-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH v2 7/8] gitweb: Highlight interesting parts of diff","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-03-23T22:56:56Z","receivedAt":"2012-03-23T22:56:56Z","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\nTwo changes in format_diff_line() were needed to allow diff refinement\nhighlightning to work. Firstly, format_diff_line() was taught to not\nesc_html() line that was passed as a reference. This was needed because\nrefining the highlight must be performed on unescaped lines and it uses\na element to mark interesting parts of the line. Secondly, the\nlines are untabified before comparing because calling untabify()\nafter inserting 's could improperly convert tabs to spaces.\n\nSigned-off-by: Michał Kiedrowicz \n---\n gitweb/gitweb.perl | 84 ++++++++++++++++++++++++++++++++++++++++++----\n gitweb/static/gitweb.css | 8 ++++\n 2 files changed, 85 insertions(+), 7 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex db32588..872ba12 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2426,14 +2426,14 @@ 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 already esc_html()'ed, pass it as a reference.\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+\tif (ref($line)) {\n+\t\t$line = $$line;\n+\t} elsif ($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@@ -5054,10 +5054,80 @@ sub print_inline_diff_lines {\n \tprint foreach (@$add);\n }\n \n+# Format removed and added line, mark changed part and HTML-format them.\n+# Impementation is based on contrib/diff-highlight\n+sub format_rem_add_line {\n+\tmy ($rem, $add) = @_;\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_is_space, $suffix_is_space) = (1, 1);\n+\n+\twhile ($prefix_len < @rem && $prefix_len < @add) {\n+\t\tlast if ($rem[$prefix_len] ne $add[$prefix_len]);\n+\n+\t\t$prefix_is_space = 0 if ($rem[$prefix_len] !~ /\\s/);\n+\t\t$prefix_len++;\n+\t}\n+\n+\tmy $shorter = (@rem < @add) ? @rem : @add;\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_is_space = 0 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_len == 1 && $suffix_len == 0) ||\n+\t ($prefix_is_space && $suffix_is_space)) {\n+\t\t$esc_rem = esc_html($rem, -nbsp=>1);\n+\t\t$esc_add = esc_html($add, -nbsp=>1);\n+\t} else {\n+\t\t$esc_rem = esc_html_hl_regions($rem, 'marked', [$prefix_len, @rem - $suffix_len], -nbsp=>1);\n+\t\t$esc_add = esc_html_hl_regions($add, 'marked', [$prefix_len, @add - $suffix_len], -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 ATM.\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_line(\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\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 +5159,11 @@ 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_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..3c4a3c9 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.8.4\n"},{"id":"187619","messageId":"1332543417-19664-9-git-send-email-michal.kiedrowicz@gmail.com","threadId":"30043","inReplyTo":"1332543417-19664-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"[PATCH v2 8/8] gitweb: Refinement highlightning in combined diffs","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-03-23T22:56:57Z","receivedAt":"2012-03-23T22:56:57Z","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 | 57 +++++++++++++++++++++++++++++++++++++++------------\n 1 files changed, 43 insertions(+), 14 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 872ba12..c056e83 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5057,12 +5057,12 @@ sub print_inline_diff_lines {\n # Format removed and added line, mark changed part and HTML-format them.\n # Impementation is based on contrib/diff-highlight\n sub format_rem_add_line {\n-\tmy ($rem, $add) = @_;\n+\tmy ($rem, $add, $num_parents) = @_;\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_is_space, $suffix_is_space) = (1, 1);\n \n \twhile ($prefix_len < @rem && $prefix_len < @add) {\n@@ -5084,7 +5084,7 @@ sub format_rem_add_line {\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_len == 1 && $suffix_len == 0) ||\n+\tif (($prefix_len == $num_parents && $suffix_len == 0) ||\n \t ($prefix_is_space && $suffix_is_space)) {\n \t\t$esc_rem = esc_html($rem, -nbsp=>1);\n \t\t$esc_add = esc_html($add, -nbsp=>1);\n@@ -5099,15 +5099,43 @@ sub format_rem_add_line {\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 ATM.\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_line(\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@@ -5123,10 +5151,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\t$is_combined);\n+\t\t$num_parents);\n \n \tif ($diff_style eq 'sidebyside' && !$is_combined) {\n \t\tprint_sidebyside_diff_lines($ctx, $rem, $add);\n@@ -5137,7 +5166,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@@ -5174,7 +5203,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, $diff_style,\n-\t\t\t\t\t$is_combined);\n+\t\t\t\t\t$num_parents);\n \t\t\t@ctx = @rem = @add = ();\n \t\t}\n \n@@ -5317,7 +5346,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 = ( [ $class, $patch_line ] );\n \t\t\t} else {\n \t\t\t\tpush @chunk, [ $class, $patch_line ];\n@@ -5326,7 +5355,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 \"\\n\"; # class=\"patch\"\n-- \n1.7.8.4\n"},{"id":"187648","messageId":"201203241958.23835.jnareb@gmail.com","threadId":"30043","inReplyTo":"1332543417-19664-2-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH v2 1/8] gitweb: esc_html_hl_regions(): Don't create empty elements","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-24T18:58:23Z","receivedAt":"2012-03-24T18:58:23Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Note: all those minor issues can be fixed while applying, I think.\n\nOn Fri, 23 Mar 2012, Michał Kiedrowicz wrote:\n\n> if $s->[1] is equal to or less than $s->[0], esc_html_hl_regions()\n ^^\ns/if/If/\n\n> generates an empty element. It normally shouldn't be visible in\n> the web broweser, but it doesn't look good when looking at page source.\n ^^^^^^^^ \ns/broweser/browser/\n\n> It also minimally increases generated page size for no special reason.\n> \n> Signed-off-by: Michał Kiedrowicz \n> ---\n> \n> I don't know if any code currently can produce empty elements,\n> but diff refinement highlighning does it.\n\nI didn't want to make esc_html_hl_regions() paranoid and boggle it down\nwith checking that gitweb called it with sane values of parameters,\nbut this one is cheap and simple. \n\nIt might be better to make esc_html_hl_regions() more robust instead\nof modifying future caller to skip empty regions.\n\nI have not read the rest of this series; for the time being conditional\nACK from me.\n\n> gitweb/gitweb.perl | 3 +++\n> 1 files changed, 3 insertions(+), 0 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index a8b5fad..af645e5 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1738,6 +1738,9 @@ sub esc_html_hl_regions {\n> \tmy $pos = 0;\n> \n> \tfor my $s (@sel) {\n> +\t\t# Don't craete empty elements.\n ^^^^^^\ns/craete/create/\n\n> +\t\tnext if $s->[1] <= $s->[0];\n> +\n> \t\t$out .= esc_html(substr($str, $pos, $s->[0] - $pos))\n> \t\t\tif ($s->[0] - $pos > 0);\n> \t\t$out .= $cgi->span({-class => $css_class},\n> -- \n> 1.7.8.4\n \nP.S. I wonder if it wouldn't be better if we created and used loop-local\nvariables with descriptive names, e.g.\n\n my ($beg, $end) = @$s;\n\nand use $beg in place of $s->[0] and $end in place of $s->[1], which are\na bit cryptic.\n\nThis of course doesn't affect this patch.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"187650","messageId":"201203242015.50421.jnareb@gmail.com","threadId":"30043","inReplyTo":"1332543417-19664-3-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH v2 2/8] gitweb: Pass esc_html_hl_regions() options to esc_html()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-24T19:15:50Z","receivedAt":"2012-03-24T19:15:50Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 23 Mar 2012, Michał Kiedrowicz wrote:\n\n> With this change, esc_html_hl_regions() accepts options and passes them\n> down to esc_html(). This may be needed if a caller wants to pass\n> -nbsp=>1 to esc_html().\n> \n> The idea and implementation example of this change was described in\n> 337da8d2 (gitweb: Introduce esc_html_match_hl and esc_html_hl_regions,\n> 2012-02-27). While other suggestions may be more useful in some cases,\n> there is no need to implement them at the moment. The\n> esc_html_hl_regions() interface may be changed later if it's needed.\n> \n> Signed-off-by: Michał Kiedrowicz \n> ---\n> \n> Jakub, The code was in fact stolen from a yours patch posted to the\n> list (gitweb: Use esc_html_match_hl() in 'grep' search) that wasn't\n> merged so maybe I should pass the authorship to you?\n\nEither passing authorship, with double signoff (mine and yours), and\nnote explaining modification, e.g.\n\n [mk: extracted from larger patch and wrote commit message]\n\nor courtesy contribution in the form of signoff-like annotation just\nbefore signoff, e.g.\n\n Based-on-patch-by: Jakub Narębski \n\nis fine by me.\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 af645e5..1744c40 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1732,7 +1732,9 @@ sub chop_and_escape_str {\n> # 'foobar'\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> @@ -1741,14 +1743,14 @@ sub esc_html_hl_regions {\n> \t\t# Don't craete empty elements.\n> \t\tnext if $s->[1] <= $s->[0];\n> \n> -\t\t$out .= esc_html(substr($str, $pos, $s->[0] - $pos))\n> +\t\t$out .= esc_html(substr($str, $pos, $s->[0] - $pos), %opts)\n> \t\t\tif ($s->[0] - $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, $s->[0], $s->[1] - $s->[0]), %opts));\n> \n> \t\t$pos = $s->[1];\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> -- \n> 1.7.8.4\n"},{"id":"187659","messageId":"20120325003134.69378c2d@gmail.com","threadId":"30043","inReplyTo":"201203242015.50421.jnareb@gmail.com","subject":"Re: [PATCH v2 2/8] gitweb: Pass esc_html_hl_regions() options to esc_html()","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-03-24T23:31:34Z","receivedAt":"2012-03-24T23:31:34Z","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, 23 Mar 2012, Michał Kiedrowicz wrote:\n> \n> > With this change, esc_html_hl_regions() accepts options and passes them\n> > down to esc_html(). This may be needed if a caller wants to pass\n> > -nbsp=>1 to esc_html().\n> > \n> > The idea and implementation example of this change was described in\n> > 337da8d2 (gitweb: Introduce esc_html_match_hl and esc_html_hl_regions,\n> > 2012-02-27). While other suggestions may be more useful in some cases,\n> > there is no need to implement them at the moment. The\n> > esc_html_hl_regions() interface may be changed later if it's needed.\n> > \n> > Signed-off-by: Michał Kiedrowicz \n> > ---\n> > \n> > Jakub, The code was in fact stolen from a yours patch posted to the\n> > list (gitweb: Use esc_html_match_hl() in 'grep' search) that wasn't\n> > merged so maybe I should pass the authorship to you?\n> \n> Either passing authorship, with double signoff (mine and yours), and\n> note explaining modification, e.g.\n> \n> [mk: extracted from larger patch and wrote commit message]\n> \n> or courtesy contribution in the form of signoff-like annotation just\n> before signoff, e.g.\n> \n> Based-on-patch-by: Jakub Narębski \n> \n> is fine by me.\n\nOK, will do.\n\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 af645e5..1744c40 100755\n> > --- a/gitweb/gitweb.perl\n> > +++ b/gitweb/gitweb.perl\n> > @@ -1732,7 +1732,9 @@ sub chop_and_escape_str {\n> > # 'foobar'\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> > @@ -1741,14 +1743,14 @@ sub esc_html_hl_regions {\n> > \t\t# Don't craete empty elements.\n> > \t\tnext if $s->[1] <= $s->[0];\n> > \n> > -\t\t$out .= esc_html(substr($str, $pos, $s->[0] - $pos))\n> > +\t\t$out .= esc_html(substr($str, $pos, $s->[0] - $pos), %opts)\n> > \t\t\tif ($s->[0] - $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, $s->[0], $s->[1] - $s->[0]), %opts));\n> > \n> > \t\t$pos = $s->[1];\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> > -- \n> > 1.7.8.4\n"},{"id":"187660","messageId":"20120325003854.7a4e309c@gmail.com","threadId":"30043","inReplyTo":"201203241958.23835.jnareb@gmail.com","subject":"Re: [PATCH v2 1/8] gitweb: esc_html_hl_regions(): Don't create empty elements","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-03-24T23:38:54Z","receivedAt":"2012-03-24T23:38:54Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jakub Narebski wrote:\n\n> Note: all those minor issues can be fixed while applying, I think.\n> \n> On Fri, 23 Mar 2012, Michał Kiedrowicz wrote:\n> \n> > if $s->[1] is equal to or less than $s->[0], esc_html_hl_regions()\n> ^^\n> s/if/If/\n> \n> > generates an empty element. It normally shouldn't be visible in\n> > the web broweser, but it doesn't look good when looking at page source.\n> ^^^^^^^^ \n> s/broweser/browser/\n\nThanks. I'm terribly sorry for these typos. I really must tripple\ncheck everything I write.\n\n> \n> > It also minimally increases generated page size for no special reason.\n> > \n> > Signed-off-by: Michał Kiedrowicz \n> > ---\n> > \n> > I don't know if any code currently can produce empty elements,\n> > but diff refinement highlighning does it.\n> \n> I didn't want to make esc_html_hl_regions() paranoid and boggle it down\n> with checking that gitweb called it with sane values of parameters,\n> but this one is cheap and simple. \n> \n> It might be better to make esc_html_hl_regions() more robust instead\n> of modifying future caller to skip empty regions.\n> \n\nYeah, the caller is just\n\n+$esc_rem = esc_html_hl_regions($rem, 'marked', [$prefix_len, @rem - $suffix_len], -nbsp=>1);\n+$esc_add = esc_html_hl_regions($add, 'marked', [$prefix_len, @add - $suffix_len], -nbsp=>1);\n\nWithout this patch I would have to do (untested)\n\n+# Create only if there is anything to mark.\n+if($prefix_len < @rem - $suffix_len) {\n+\t$esc_rem = esc_html_hl_regions($rem, 'marked', [$prefix_len, @rem - $suffix_len], -nbsp=>1);\n+} else {\n+\t$esc_rem = esc_html($rem, -nbsp=1);\n+}\n+if($prefix_len < @add - $suffix_len) {\n+\t$esc_add = esc_html_hl_regions($add, 'marked', [$prefix_len, @add - $suffix_len], -nbsp=>1);\n+} else {\n+\t$esc_add = esc_html($add, -nbsp=1);\n+}\n\nor something like that.\n\n> I have not read the rest of this series; for the time being conditional\n> ACK from me.\n> \n> > gitweb/gitweb.perl | 3 +++\n> > 1 files changed, 3 insertions(+), 0 deletions(-)\n> > \n> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> > index a8b5fad..af645e5 100755\n> > --- a/gitweb/gitweb.perl\n> > +++ b/gitweb/gitweb.perl\n> > @@ -1738,6 +1738,9 @@ sub esc_html_hl_regions {\n> > \tmy $pos = 0;\n> > \n> > \tfor my $s (@sel) {\n> > +\t\t# Don't craete empty elements.\n> ^^^^^^\n> s/craete/create/\n> \n> > +\t\tnext if $s->[1] <= $s->[0];\n> > +\n> > \t\t$out .= esc_html(substr($str, $pos, $s->[0] - $pos))\n> > \t\t\tif ($s->[0] - $pos > 0);\n> > \t\t$out .= $cgi->span({-class => $css_class},\n> > -- \n> > 1.7.8.4\n> \n> P.S. I wonder if it wouldn't be better if we created and used loop-local\n> variables with descriptive names, e.g.\n> \n> my ($beg, $end) = @$s;\n> \n> and use $beg in place of $s->[0] and $end in place of $s->[1], which are\n> a bit cryptic.\n> \n> This of course doesn't affect this patch.\n> \n"},{"id":"187931","messageId":"201203281633.53916.jnareb@gmail.com","threadId":"30043","inReplyTo":"1332543417-19664-4-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH v2 3/8] gitweb: Extract print_sidebyside_diff_lines()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-28T14:33:53Z","receivedAt":"2012-03-28T14:33:53Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 23 Mar 2012, Michał Kiedrowicz wrote:\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>\nRight, that is quite sensible.\n\n> The outcome of this patch is that print_sidebyside_diff_chunk() is now\n> much shorter and easier to read.\n> \nNice effect.\n\n> This is a preparation patch for diff refinement highlightning. 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'it\n> printed just before printing added and removed lines.\n\n , and at the end\n of chunk.\n\nIMVHO such change is irrelevant.\n\nAcked-by: Jakub Narębski \n\n> Signed-off-by: Michał Kiedrowicz \n> ---\n> gitweb/gitweb.perl | 97 ++++++++++++++++++++++++++++------------------------\n> 1 files changed, 52 insertions(+), 45 deletions(-)\n\nNice code movement.\n \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 1744c40..86d5a02 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -4998,6 +4998,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> @@ -5024,51 +5071,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\n-- \nJakub Narebski\nPoland\n"},{"id":"187947","messageId":"201203281756.42653.jnareb@gmail.com","threadId":"30043","inReplyTo":"1332543417-19664-5-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH v2 4/8] gitweb: Use print_diff_chunk() for both side-by-side and inline diffs","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-28T15:56:41Z","receivedAt":"2012-03-28T15:56:41Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 23 Mar 2012, 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> \nWhile I don't like that we use accumulator for something that was \nstraightforwardly printed, I understand that it is necessary preprocessing\nrequired for diff refinement highlighting. Having refactoring upfront\nis a good idea, making for easier review.\n\n> If left as is, the new function print_inline_diff_lines() could reorder\n\nI think the previous paragraph is long enough that it would be better\nto repeat subject of this sentence, i.e. start with\n\n \"If print_diff_chunk() was left as is, the new function ...\"\n\n> diff lines. It first prints all context lines, then all removed lines\n> and finally all added lines. If the diff output consisted of mixed added\n> and removed lines, gitweb would reorder these lines. This is true for\n> 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> 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> \nO.K.\n\n> Signed-off-by: Michał Kiedrowicz \n> ---\n[...]\n\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\nWhy not simply\n\n +\tprint @$ctx, @$rem, @$add;\n\nIt is \"print LIST\" for a reason (and $\\ is empty here in gitweb, as it\nis empty by default).\n\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\nVery nice subroutine!\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> @@ -5072,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, $diff_style,\n> +\t\t\t\t\t$is_combined);\n\nNitpick: the following _might_ be a tiny little bit more readable:\n\n +\t\t\tprint_diff_lines(\\@ctx, \\@rem, \\@add,\n +\t\t\t $diff_style, $is_combined);\n\n> \t\t\t@ctx = @rem = @add = ();\n> \t\t}\n> \n> @@ -5091,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\nAnyway nice change.\n \n> @@ -5217,22 +5247,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\nNice, pushing acting on $diff_style down to print_diff_chunk(), which\nsimplifies code a bit.\n\n> +\t\t\t\t@chunk = ( [ $class, $line ] );\n\nBTW. this could be simplified to\n\n +\t\t\t\t@chunk = ();\n +\t\t\t\tpush @chunk, [ $class, $line ];\n\nWell, simplified after noticing the common part of those two branches\nof a conditional.\n\nBut it really doesn't matter.\n\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> -- \n\nBTW. I was wondering about binary files, but this commit wouldn't make\nit worse anyway as we parse diff output assuming unified-like diff for\ndiff syntax highlighting... and binary diffs are shown as\n\n \"Binary files a/foo.png and b/foo.png differ\"\n\nin extended diff header.\n-- \nJakub Narebski\nPoland\n"},{"id":"188070","messageId":"201203291814.42803.jnareb@gmail.com","threadId":"30043","inReplyTo":"1332543417-19664-6-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH v2 5/8] gitweb: Move HTML-formatting diff line back to process_diff_line()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-29T16:14:42Z","receivedAt":"2012-03-29T16:14:42Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 23 Mar 2012, Michał Kiedrowicz wrote:\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> \nI remember that originally process_diff_line was format_diff_line... \n\n> This simplifies code in git_patchset_body() and keeps formatting of a\n> diff line in one place.\n> \nThat is a good enough reason for me.\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> \nExcuse me, but from this commit message (and from the patch itseld)\nI don't see how this commit (patch) can help with this goal (and\ndon't remember details of discussion).\n\nPlease explain it in more detail, or simply remove above paragraph.\n\n> This is just a refactoring patch. It's not meant to change gitweb\n> output.\n> \n> Signed-off-by: Michał Kiedrowicz \n> Acked-by: Jakub Narębski \n> ---\n[...]\n-- \nJakub Narebski\nPoland\n"},{"id":"188073","messageId":"201203291849.42583.jnareb@gmail.com","threadId":"30043","inReplyTo":"201203291814.42803.jnareb@gmail.com","subject":"Re: [PATCH v2 5/8] gitweb: Move HTML-formatting diff line back to process_diff_line()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-29T16:49:42Z","receivedAt":"2012-03-29T16:49:42Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Jakub Narebski wrote:\n> On Fri, 23 Mar 2012, Michał Kiedrowicz wrote:\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> Excuse me, but from this commit message (and from the patch itseld)\n> I don't see how this commit (patch) can help with this goal (and\n> don't remember details of discussion).\n> \n> Please explain it in more detail, or simply remove above paragraph.\n\nOr just squash it with next patch in this series.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"188074","messageId":"201203291859.44891.jnareb@gmail.com","threadId":"30043","inReplyTo":"1332543417-19664-7-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH v2 6/8] gitweb: Push formatting diff lines to print_diff_chunk()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-29T16:59:44Z","receivedAt":"2012-03-29T16:59:44Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 23 Mon 2012, Michał Kiedrowicz wrote:\n\n> Now git_patchset_body() only calls diff_line_class(), which is removed\n> from process_diff_line(). The latter function is renamed to\n> format_diff_line() and its output is changed to return only\n> HTML-formatted line, which brings it in line with outher format_*\n> subroutined.\n> \n> This slightly changes the order of operations performed on diff lines.\n> Before this commit, each read line was formatted and then put to the\n> @chunk accumulator. Now, lines are formatted inside print_diff_chunk(),\n\nThis is a bit convoluted description.\n\n\nAs I understand it, what happens here is that formatting lines is\npushed down to print_diff_chunk(), closer to the place where we\nactually use HTML formatted output.\n\nThis means that we put raw lines in the @chunk accumulator, rather\nthan formatted lines. Because we still need to know class (type)\nof line when accumulating data to post-process and print, \nprocess_diff_line() subroutine was retired and replaced by \ndiff_line_class() used in git_patchset_body() and new / resurrected\nformat_diff_line() used in print_diff_chunk().\n\nIsn't it?\n \n\nA side effect is that we have to pass \\%from and \\%to down the\ncallstack.\n\n> This is a preparation patch for diff refinement highlightning. It's not\n> meant to change gitweb output.\n> \nThis is a very nice refactoring. I was never really comfortable with\nthe API of process_diff_line(), which was different from all other\nsubroutines in gitweb, and error prone to call. I wish we used this\nsolution presented in this commit from the very beginning.\n\nBTW. I think we can simply squash this commit with previous one; no\nneed to improve process_diff_line() if we are retiring it.\n\n> Signed-off-by: Michał Kiedrowicz \n> Acked-by: Jakub Narębski \n> ---\n> gitweb/gitweb.perl | 25 ++++++++++++-------------\n> 1 files changed, 12 insertions(+), 13 deletions(-)\n\n-- \nJakub Narebski\nPoland\n"},{"id":"188079","messageId":"20120329192557.4bd45322@gmail.com","threadId":"30043","inReplyTo":"201203281633.53916.jnareb@gmail.com","subject":"Re: [PATCH v2 3/8] gitweb: Extract print_sidebyside_diff_lines()","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-03-29T17:25:57Z","receivedAt":"2012-03-29T17:25:57Z","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, 23 Mar 2012, Michał Kiedrowicz wrote:\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> Right, that is quite sensible.\n> \n> > The outcome of this patch is that print_sidebyside_diff_chunk() is now\n> > much shorter and easier to read.\n> > \n> Nice effect.\n> \n> > This is a preparation patch for diff refinement highlightning. 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'it\n> > printed just before printing added and removed lines.\n> \n> , and at the end\n> of chunk.\n> \n> IMVHO such change is irrelevant.\n> \n> Acked-by: Jakub Narębski \n\nThanks.\n\n> \n> > Signed-off-by: Michał Kiedrowicz \n> > ---\n> > gitweb/gitweb.perl | 97 ++++++++++++++++++++++++++++------------------------\n> > 1 files changed, 52 insertions(+), 45 deletions(-)\n> \n> Nice code movement.\n> \n"},{"id":"188080","messageId":"20120329193152.3d0f27e2@gmail.com","threadId":"30043","inReplyTo":"201203281756.42653.jnareb@gmail.com","subject":"Re: [PATCH v2 4/8] gitweb: Use print_diff_chunk() for both side-by-side and inline diffs","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-03-29T17:31:52Z","receivedAt":"2012-03-29T17:31:52Z","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, 23 Mar 2012, 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> While I don't like that we use accumulator for something that was \n> straightforwardly printed, I understand that it is necessary preprocessing\n> required for diff refinement highlighting. Having refactoring upfront\n> is a good idea, making for easier review.\n\nYeah, this change doesn't make sense on its own but is required by later\npatch that needs whole chunk at once.\n\n> \n> > If left as is, the new function print_inline_diff_lines() could reorder\n> \n> I think the previous paragraph is long enough that it would be better\n> to repeat subject of this sentence, i.e. start with\n> \n> \"If print_diff_chunk() was left as is, the new function ...\"\n\nOK.\n\n> \n> > diff lines. It first prints all context lines, then all removed lines\n> > and finally all added lines. If the diff output consisted of mixed added\n> > and removed lines, gitweb would reorder these lines. This is true for\n> > 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> > 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> O.K.\n> \n> > Signed-off-by: Michał Kiedrowicz \n> > ---\n> [...]\n> \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> Why not simply\n> \n> +\tprint @$ctx, @$rem, @$add;\n> \n> It is \"print LIST\" for a reason (and $\\ is empty here in gitweb, as it\n> is empty by default).\n> \n\nOK, that's much better. I really didn't like those 3 prints.\n\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> Very nice subroutine!\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> > @@ -5072,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, $diff_style,\n> > +\t\t\t\t\t$is_combined);\n> \n> Nitpick: the following _might_ be a tiny little bit more readable:\n> \n> +\t\t\tprint_diff_lines(\\@ctx, \\@rem, \\@add,\n> +\t\t\t $diff_style, $is_combined);\n> \n\nOK.\n\n> > \t\t\t@ctx = @rem = @add = ();\n> > \t\t}\n> > \n> > @@ -5091,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> Anyway nice change.\n> \n> > @@ -5217,22 +5247,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> \n> Nice, pushing acting on $diff_style down to print_diff_chunk(), which\n> simplifies code a bit.\n> \n> > +\t\t\t\t@chunk = ( [ $class, $line ] );\n> \n> BTW. this could be simplified to\n> \n> +\t\t\t\t@chunk = ();\n> +\t\t\t\tpush @chunk, [ $class, $line ];\n> \n> Well, simplified after noticing the common part of those two branches\n> of a conditional.\n> \n> But it really doesn't matter.\n\nSounds sensible.\n\n> \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> > -- \n> \n> BTW. I was wondering about binary files, but this commit wouldn't make\n> it worse anyway as we parse diff output assuming unified-like diff for\n> diff syntax highlighting... and binary diffs are shown as\n> \n> \"Binary files a/foo.png and b/foo.png differ\"\n> \n> in extended diff header.\n\nYeah, this is what I wrote in the cover letter:\n\n\t* Ensured that binary patches are not supported in HTML format,\n\t thus this series canot break it :) (answering Jakub's questions)\n\nbut maybe I wasn't clear enough.\n"},{"id":"188081","messageId":"20120329193658.035caa3a@gmail.com","threadId":"30043","inReplyTo":"201203291814.42803.jnareb@gmail.com","subject":"Re: [PATCH v2 5/8] gitweb: Move HTML-formatting diff line back to process_diff_line()","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-03-29T17:36:58Z","receivedAt":"2012-03-29T17:36:58Z","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, 23 Mar 2012, Michał Kiedrowicz wrote:\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> I remember that originally process_diff_line was format_diff_line... \n> \n> > This simplifies code in git_patchset_body() and keeps formatting of a\n> > diff line in one place.\n> > \n> That is a good enough reason for me.\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> Excuse me, but from this commit message (and from the patch itseld)\n> I don't see how this commit (patch) can help with this goal (and\n> don't remember details of discussion).\n> \n> Please explain it in more detail, or simply remove above paragraph.\n\nThe important part in this patch is that it removes some of HTML\nformatting from git_patchset_body(). I need this because I want to\nprocess whole chunk before formatting. So I must push all lines in a\nchunk to print_diff_chunk(). \n\nBut I may remove this paragraph as well.\n\n> \n> > This is just a refactoring patch. It's not meant to change gitweb\n> > output.\n> > \n> > Signed-off-by: Michał Kiedrowicz \n> > Acked-by: Jakub Narębski \n> > ---\n> [...]\n"},{"id":"188082","messageId":"20120329194153.2e1ad827@gmail.com","threadId":"30043","inReplyTo":"201203291859.44891.jnareb@gmail.com","subject":"Re: [PATCH v2 6/8] gitweb: Push formatting diff lines to print_diff_chunk()","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-03-29T17:41:53Z","receivedAt":"2012-03-29T17:41:53Z","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, 23 Mon 2012, Michał Kiedrowicz wrote:\n> \n> > Now git_patchset_body() only calls diff_line_class(), which is removed\n> > from process_diff_line(). The latter function is renamed to\n> > format_diff_line() and its output is changed to return only\n> > HTML-formatted line, which brings it in line with outher format_*\n> > subroutined.\n> > \n> > This slightly changes the order of operations performed on diff lines.\n> > Before this commit, each read line was formatted and then put to the\n> > @chunk accumulator. Now, lines are formatted inside print_diff_chunk(),\n> \n> This is a bit convoluted description.\n> \n> \n> As I understand it, what happens here is that formatting lines is\n> pushed down to print_diff_chunk(), closer to the place where we\n> actually use HTML formatted output.\n\nYes.\n\n> \n> This means that we put raw lines in the @chunk accumulator, rather\n> than formatted lines. Because we still need to know class (type)\n> of line when accumulating data to post-process and print, \n> process_diff_line() subroutine was retired and replaced by \n> diff_line_class() used in git_patchset_body() and new / resurrected\n> format_diff_line() used in print_diff_chunk().\n> \n> Isn't it?\n\nVery true.\n\n> \n> \n> A side effect is that we have to pass \\%from and \\%to down the\n> callstack.\n\nYes.\n\n> \n> > This is a preparation patch for diff refinement highlightning. It's not\n> > meant to change gitweb output.\n> > \n> This is a very nice refactoring. I was never really comfortable with\n> the API of process_diff_line(), which was different from all other\n> subroutines in gitweb, and error prone to call. I wish we used this\n> solution presented in this commit from the very beginning.\n> \n> BTW. I think we can simply squash this commit with previous one; no\n> need to improve process_diff_line() if we are retiring it.\n\nOK, will do.\n\n> \n> > Signed-off-by: Michał Kiedrowicz \n> > Acked-by: Jakub Narębski \n> > ---\n> > gitweb/gitweb.perl | 25 ++++++++++++-------------\n> > 1 files changed, 12 insertions(+), 13 deletions(-)\n> \n"},{"id":"188084","messageId":"1333044260-31278-1-git-send-email-michal.kiedrowicz@gmail.com","threadId":"30043","inReplyTo":"201203241958.23835.jnareb@gmail.com","subject":"[PATCH] gitweb: Use descriptive names in esc_html_hl_regions()","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-03-29T18:04:20Z","receivedAt":"2012-03-29T18:04:20Z","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\nSigned-off-by: Michał Kiedrowicz \n---\n\n> P.S. I wonder if it wouldn't be better if we created and used loop-local\n> variables with descriptive names, e.g.\n> \n> my ($beg, $end) = @$s;\n> \n> and use $beg in place of $s->[0] and $end in place of $s->[1], which are\n> a bit cryptic.\n> \n> This of course doesn't affect this patch.\n\nSomething like this? (patch not based on this series, may be applied\nindependently).\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":"188090","messageId":"201203292142.21680.jnareb@gmail.com","threadId":"30043","inReplyTo":"1332543417-19664-8-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH v2 7/8] gitweb: Highlight interesting parts of diff","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-29T19:42:20Z","receivedAt":"2012-03-29T19:42:20Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 23 Mar 2012, Michał Kiedrowicz 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 should\n> 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 corresponding\n> lines and highlightning only the middle part of lines. For more\n> information, see contrib/diff-highlight/README.\n>\nNice description.\n \n> Combined diffs are not supported but a following commit will change it.\n> \nO.K.\n\n> Two changes in format_diff_line() were needed to allow diff refinement\n> highlightning to work. Firstly, format_diff_line() was taught to not\n> esc_html() line that was passed as a reference. This was needed because\n> refining the highlight must be performed on unescaped lines and it uses\n> a element to mark interesting parts of the line.\n\nWell, actually we just need to make format_diff_line to not esc_html\npassed line which is already esc_html'ed or esc_html_hl_regions'ed,\ni.e. to avoid double escaping. Passing line as reference if it is\nto be treated as HTML is one possibility, and I think it is a good\nconvention to start to use.\n\n> Secondly, the \n> lines are untabified before comparing because calling untabify()\n> after inserting 's could improperly convert tabs to spaces.\n\nWell, it is actually because untabify() must work on original text to\nwork correctly, i.e. to convert tabs to spaces according to tab stops.\nIt must precede esc_html'ing, and even more esc_html_hl_regions'ing.\n\n> \n> Signed-off-by: Michał Kiedrowicz \n\nAnyway this is a good change, much cleaner than previous version\n\n Acked-by: Jakub Narębski \n\n> ---\n> gitweb/gitweb.perl | 84 ++++++++++++++++++++++++++++++++++++++++++----\n> gitweb/static/gitweb.css | 8 ++++\n> 2 files changed, 85 insertions(+), 7 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index db32588..872ba12 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -2426,14 +2426,14 @@ 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 already esc_html()'ed, pass it as a reference.\n\nErrr... I think the explanation here must be in reverse:\n\n +# If the line is passed as a reference, it is treated as HTML,\n +# and not esc_html()'ed.\n\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> +\tif (ref($line)) {\n> +\t\t$line = $$line;\n> +\t} elsif ($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> @@ -5054,10 +5054,80 @@ sub print_inline_diff_lines {\n> \tprint foreach (@$add);\n> }\n> \n> +# Format removed and added line, mark changed part and HTML-format them.\n> +# Impementation is based on contrib/diff-highlight\n> +sub format_rem_add_line {\n\nWouldn't a better name be format_rem_add_pair() or format_rem_add_lines(),\nor format_rem_add_lines_pair()?\n\n> +\tmy ($rem, $add) = @_;\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\nI wonder if it wouldn't be slightly cleaner to count $prefix_len from\n+/- rather than start of line. It means\n\n +\t# Prefix does not count initial +/- character.\n +\tmy ($prefix_len, $suffix_len) = (0, 0);\n\nPerhaps even remove initial +/- from @add and @rem.\n\n +\tshift @rem;\n +\tshift @add;\n\nEhh... now I see that starting $prefix_len with 1 might be not so bad\nidea after all.\n\n> +\tmy ($prefix_is_space, $suffix_is_space) = (1, 1);\n\nIt is not entirely true: $prefix_is_space is really $prefix_is_space_or_empty.\nIt might be a better idea to use\n\n +\tmy ($prefix_has_nonspace, $suffix_has_nonspace);\n\nusing the fact that undefined value is false.\n\n> +\n> +\twhile ($prefix_len < @rem && $prefix_len < @add) {\n> +\t\tlast if ($rem[$prefix_len] ne $add[$prefix_len]);\n> +\n> +\t\t$prefix_is_space = 0 if ($rem[$prefix_len] !~ /\\s/);\n\n +\t\t$prefix_has_nonspace = 1 if ($rem[$prefix_len] =~ /\\s/);\n\n> +\t\t$prefix_len++;\n> +\t}\n> +\n> +\tmy $shorter = (@rem < @add) ? @rem : @add;\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_is_space = 0 if ($rem[-1 - $suffix_len] !~ /\\s/);\n\n +\t\t$suffix_has_nonspace = 1 if ($rem[-1 - $suffix_len] =~ /\\s/);\n\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_len == 1 && $suffix_len == 0) ||\n> +\t ($prefix_is_space && $suffix_is_space)) {\n\nActually ($prefix_is_space && $suffix_is_space) is enough, but cryptic.\nWith $prefix_is_space (== $prefix_is_space_or_empty) -> $prefix_has_nonspace\nit would be\n\n +\tif (not ($prefix_has_nonspace || $suffix_has_nonspace)) {\n\nAnyway, it is not as if it is not clear enough.\n\n> +\t\t$esc_rem = esc_html($rem, -nbsp=>1);\n> +\t\t$esc_add = esc_html($add, -nbsp=>1);\n> +\t} else {\n> +\t\t$esc_rem = esc_html_hl_regions($rem, 'marked', [$prefix_len, @rem - $suffix_len], -nbsp=>1);\n> +\t\t$esc_add = esc_html_hl_regions($add, 'marked', [$prefix_len, @add - $suffix_len], -nbsp=>1);\n\n +\t\t$esc_rem = esc_html_hl_regions($rem, 'marked', [$prefix_len+1, @rem+1 - $suffix_len], -nbsp=>1);\n +\t\t$esc_add = esc_html_hl_regions($add, 'marked', [$prefix_len+1, @add+1 - $suffix_len], -nbsp=>1);\n\nSo probably not worth it.\n\n> +\t}\n> +\n> +\treturn format_diff_line(\\$esc_rem, 'rem'),\n> +\t format_diff_line(\\$esc_add, 'add');\n\n +\treturn format_diff_line(\\$esc_rem, 'rem'),\n +\t format_diff_line(\\$esc_add, 'add');\n\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 ATM.\n\nATM == at this moment? Please say so.\n\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_line(\n> +\t\t\t $rem->[$i], $add->[$i]);\n\n +\t\t\tmy ($line_rem, $line_add) =\n +\t\t\t format_rem_add_line($rem->[$i], $add->[$i]);\n\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\nNice.\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\t$is_combined);\n> +\n\n +\t($ctx, $rem, $add) = format_ctx_rem_add_lines($ctx, $rem, $add,\t$is_combined);\n +\n\nUnless the line is too long.\n\n> \tif ($diff_style eq 'sidebyside' && !$is_combined) {\n> \t\tprint_sidebyside_diff_lines($ctx, $rem, $add);\n> \t} else {\n> @@ -5089,11 +5159,11 @@ 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\nO.K. you move untabify() out of format_diff_line(), and must now\nensure that caller uses it before calling format_diff_line() or \nformat_ctx_rem_add_lines() (which uses format_diff_line() itself).\n\nI wonder if leaving untabify() (and chomp) in format_diff_line(),\nbut not running it if passed reference, and running untabify() in\nformat_ctx_rem_add_lines() wouldn't be a better, less fragile code.\n\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\nO.K., only 'chunk_header' are not formatted.\n\n> \t\t\tnext;\n> \t\t}\n\n[I have to take a look how final version of print_diff_lines() looks\n like in this commit; the patch alone is not enough]\n\nRight, so we format just before printing, and print_* do formatting\nitself before printing. Nice and clean.\n\n> \n> diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css\n> index c530355..3c4a3c9 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\nNice styling.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"188091","messageId":"20120329215922.606f54d4@gmail.com","threadId":"30043","inReplyTo":"201203292142.21680.jnareb@gmail.com","subject":"Re: [PATCH v2 7/8] gitweb: Highlight interesting parts of diff","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-03-29T19:59:22Z","receivedAt":"2012-03-29T19:59:22Z","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, 23 Mar 2012, Michał Kiedrowicz 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 should\n> > 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 corresponding\n> > lines and highlightning only the middle part of lines. For more\n> > information, see contrib/diff-highlight/README.\n> >\n> Nice description.\n> \n> > Combined diffs are not supported but a following commit will change it.\n> > \n> O.K.\n> \n> > Two changes in format_diff_line() were needed to allow diff refinement\n> > highlightning to work. Firstly, format_diff_line() was taught to not\n> > esc_html() line that was passed as a reference. This was needed because\n> > refining the highlight must be performed on unescaped lines and it uses\n> > a element to mark interesting parts of the line.\n> \n> Well, actually we just need to make format_diff_line to not esc_html\n> passed line which is already esc_html'ed or esc_html_hl_regions'ed,\n> i.e. to avoid double escaping. Passing line as reference if it is\n> to be treated as HTML is one possibility, and I think it is a good\n> convention to start to use.\n> \n> > Secondly, the \n> > lines are untabified before comparing because calling untabify()\n> > after inserting 's could improperly convert tabs to spaces.\n> \n> Well, it is actually because untabify() must work on original text to\n> work correctly, i.e. to convert tabs to spaces according to tab stops.\n> It must precede esc_html'ing, and even more esc_html_hl_regions'ing.\n> \n> > \n> > Signed-off-by: Michał Kiedrowicz \n> \n> Anyway this is a good change, much cleaner than previous version\n> \n> Acked-by: Jakub Narębski \n\nThanks.\n\n> \n> > ---\n> > gitweb/gitweb.perl | 84 ++++++++++++++++++++++++++++++++++++++++++----\n> > gitweb/static/gitweb.css | 8 ++++\n> > 2 files changed, 85 insertions(+), 7 deletions(-)\n> > \n> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> > index db32588..872ba12 100755\n> > --- a/gitweb/gitweb.perl\n> > +++ b/gitweb/gitweb.perl\n> > @@ -2426,14 +2426,14 @@ 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 already esc_html()'ed, pass it as a reference.\n> \n> Errr... I think the explanation here must be in reverse:\n> \n> +# If the line is passed as a reference, it is treated as HTML,\n> +# and not esc_html()'ed.\n\nYeah, I thought about that option too :).\n\n> \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> > +\tif (ref($line)) {\n> > +\t\t$line = $$line;\n> > +\t} elsif ($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> > @@ -5054,10 +5054,80 @@ sub print_inline_diff_lines {\n> > \tprint foreach (@$add);\n> > }\n> > \n> > +# Format removed and added line, mark changed part and HTML-format them.\n> > +# Impementation is based on contrib/diff-highlight\n> > +sub format_rem_add_line {\n> \n> Wouldn't a better name be format_rem_add_pair() or format_rem_add_lines(),\n> or format_rem_add_lines_pair()?\n\nOK.\n\n> \n> > +\tmy ($rem, $add) = @_;\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> \n> I wonder if it wouldn't be slightly cleaner to count $prefix_len from\n> +/- rather than start of line. It means\n> \n> +\t# Prefix does not count initial +/- character.\n> +\tmy ($prefix_len, $suffix_len) = (0, 0);\n> \n> Perhaps even remove initial +/- from @add and @rem.\n> \n> +\tshift @rem;\n> +\tshift @add;\n> \n> Ehh... now I see that starting $prefix_len with 1 might be not so bad\n> idea after all.\n> \n> > +\tmy ($prefix_is_space, $suffix_is_space) = (1, 1);\n> \n> It is not entirely true: $prefix_is_space is really $prefix_is_space_or_empty.\n> It might be a better idea to use\n> \n> +\tmy ($prefix_has_nonspace, $suffix_has_nonspace);\n> \n> using the fact that undefined value is false.\n> \n> > +\n> > +\twhile ($prefix_len < @rem && $prefix_len < @add) {\n> > +\t\tlast if ($rem[$prefix_len] ne $add[$prefix_len]);\n> > +\n> > +\t\t$prefix_is_space = 0 if ($rem[$prefix_len] !~ /\\s/);\n> \n> +\t\t$prefix_has_nonspace = 1 if ($rem[$prefix_len] =~ /\\s/);\n> \n> > +\t\t$prefix_len++;\n> > +\t}\n> > +\n> > +\tmy $shorter = (@rem < @add) ? @rem : @add;\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_is_space = 0 if ($rem[-1 - $suffix_len] !~ /\\s/);\n> \n> +\t\t$suffix_has_nonspace = 1 if ($rem[-1 - $suffix_len] =~ /\\s/);\n> \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_len == 1 && $suffix_len == 0) ||\n> > +\t ($prefix_is_space && $suffix_is_space)) {\n> \n> Actually ($prefix_is_space && $suffix_is_space) is enough, but cryptic.\n\nYes, you caught was I thought too but hadn't wrote explicitly. The\nfirst condition ($prefix_len == 1 && $suffix_len == 0) is redundant but\nmakes the condition easier to understand.\n\n> With $prefix_is_space (== $prefix_is_space_or_empty) -> $prefix_has_nonspace\n> it would be\n> \n> +\tif (not ($prefix_has_nonspace || $suffix_has_nonspace)) {\n> \n> Anyway, it is not as if it is not clear enough.\n\nI may play with this for a while to see which version looks best.\n\n> \n> > +\t\t$esc_rem = esc_html($rem, -nbsp=>1);\n> > +\t\t$esc_add = esc_html($add, -nbsp=>1);\n> > +\t} else {\n> > +\t\t$esc_rem = esc_html_hl_regions($rem, 'marked', [$prefix_len, @rem - $suffix_len], -nbsp=>1);\n> > +\t\t$esc_add = esc_html_hl_regions($add, 'marked', [$prefix_len, @add - $suffix_len], -nbsp=>1);\n> \n> +\t\t$esc_rem = esc_html_hl_regions($rem, 'marked', [$prefix_len+1, @rem+1 - $suffix_len], -nbsp=>1);\n> +\t\t$esc_add = esc_html_hl_regions($add, 'marked', [$prefix_len+1, @add+1 - $suffix_len], -nbsp=>1);\n> \n> So probably not worth it.\n> \n> > +\t}\n> > +\n> > +\treturn format_diff_line(\\$esc_rem, 'rem'),\n> > +\t format_diff_line(\\$esc_add, 'add');\n> \n> +\treturn format_diff_line(\\$esc_rem, 'rem'),\n> +\t format_diff_line(\\$esc_add, 'add');\n> \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 ATM.\n> \n> ATM == at this moment? Please say so.\n\nOK.\n\n> \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_line(\n> > +\t\t\t $rem->[$i], $add->[$i]);\n> \n> +\t\t\tmy ($line_rem, $line_add) =\n> +\t\t\t format_rem_add_line($rem->[$i], $add->[$i]);\n> \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> Nice.\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\t$is_combined);\n> > +\n> \n> +\t($ctx, $rem, $add) = format_ctx_rem_add_lines($ctx, $rem, $add,\t$is_combined);\n> +\n> \n> Unless the line is too long.\n> \n> > \tif ($diff_style eq 'sidebyside' && !$is_combined) {\n> > \t\tprint_sidebyside_diff_lines($ctx, $rem, $add);\n> > \t} else {\n> > @@ -5089,11 +5159,11 @@ 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> O.K. you move untabify() out of format_diff_line(), and must now\n> ensure that caller uses it before calling format_diff_line() or \n> format_ctx_rem_add_lines() (which uses format_diff_line() itself).\n> \n> I wonder if leaving untabify() (and chomp) in format_diff_line(),\n> but not running it if passed reference, and running untabify() in\n> format_ctx_rem_add_lines() wouldn't be a better, less fragile code.\n> \n\nI can try that.\n\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> \n> O.K., only 'chunk_header' are not formatted.\n> \n> > \t\t\tnext;\n> > \t\t}\n> \n> [I have to take a look how final version of print_diff_lines() looks\n> like in this commit; the patch alone is not enough]\n> \n> Right, so we format just before printing, and print_* do formatting\n> itself before printing. Nice and clean.\n\nThanks.\n\n> \n> > \n> > diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css\n> > index c530355..3c4a3c9 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> Nice styling.\n> \n"},{"id":"188107","messageId":"201203300137.11932.jnareb@gmail.com","threadId":"30043","inReplyTo":"1332543417-19664-9-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH v2 8/8] gitweb: Refinement highlightning in combined diffs","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-29T23:37:10Z","receivedAt":"2012-03-29T23:37:10Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 23 Mar 2012, Michał Kiedrowicz wrote:\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> \nIs -> was?\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\nWell, we explicitly skip combined diffs. I think what you want to say\nhere is that it is not possible to simply use existing algorithm\nunchanged for combined diffs.\n\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\nO.K., that's a nice and sensible first step.\n\nI wonder if it would be worth to specify that we currently require that\npattern of '-'-es in pre-image match pattern of '+'-es in postimage.\n\nNb. the prefix of combined diff would either include '+', or '-',\nbut never mixed (this is documented, but I had trouble with this).\n\n> \n> Further changes may introduce more intelligent approach that better\n> handles combined diffs.\n\nVery sensible approach.\n> \n> Signed-off-by: Michał Kiedrowicz \n\n Acked-by: Jakub Narębski \n\n> ---\n\nBTW. I went and checked if this approach helps for non-trivial merges\nin git.git history:\n\n* b10656c - helps a bit, though one can see limitation of pre/post-fix\n matching here, but it is present also for non-combined diff.\n\n* 8b132bc - helps a bit, though char-interdiff or word-interdiff might\n be better. Nb. I think that red background for 'marked' is a bit\n too dark (intensive).\n\n* c58499c - doesn't help too much.\n\n* f629c23, aa145bf - helps.\n\n> gitweb/gitweb.perl | 57 +++++++++++++++++++++++++++++++++++++++------------\n> 1 files changed, 43 insertions(+), 14 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 872ba12..c056e83 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -5057,12 +5057,12 @@ sub print_inline_diff_lines {\n> # Format removed and added line, mark changed part and HTML-format them.\n> # Impementation is based on contrib/diff-highlight\n\nImplementation\n ^---\n\n> sub format_rem_add_line {\n> -\tmy ($rem, $add) = @_;\n> +\tmy ($rem, $add, $num_parents) = @_;\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\nNice.\n\n[...]\n> @@ -5099,15 +5099,43 @@ sub format_rem_add_line {\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 ATM.\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\nGood.\n\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_line(\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\n[...]\n> @@ -5326,7 +5355,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 \"
\\n\"; # class=\"patch\"\n\nI was wondering about 'commitdiff' between two commits, which is not\ncombined even ifany of those commits is a merge commit... but it looks\nlike it works all right.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"188131","messageId":"20120330084929.3c4fa23e@mkiedrowicz.ivo.pl","threadId":"30043","inReplyTo":"201203300137.11932.jnareb@gmail.com","subject":"Re: [PATCH v2 8/8] gitweb: Refinement highlightning in combined diffs","fromName":"Michal Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-03-30T06:49:29Z","receivedAt":"2012-03-30T06:49:29Z","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, 23 Mar 2012, Michał Kiedrowicz wrote:\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> Is -> was?\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> Well, we explicitly skip combined diffs. I think what you want to say\n> here is that it is not possible to simply use existing algorithm\n> unchanged for combined diffs.\n\nYep, your wording is much better.\n\n> \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> O.K., that's a nice and sensible first step.\n> \n> I wonder if it would be worth to specify that we currently require\n> that pattern of '-'-es in pre-image match pattern of '+'-es in\n> postimage.\n> \n> Nb. the prefix of combined diff would either include '+', or '-',\n> but never mixed (this is documented, but I had trouble with this).\n> \n> > \n> > Further changes may introduce more intelligent approach that better\n> > handles combined diffs.\n> \n> Very sensible approach.\n> > \n> > Signed-off-by: Michał Kiedrowicz \n> \n> Acked-by: Jakub Narębski \n> \n> > ---\n> \n> BTW. I went and checked if this approach helps for non-trivial merges\n> in git.git history:\n> \n> * b10656c - helps a bit, though one can see limitation of pre/post-fix\n> matching here, but it is present also for non-combined diff.\n> \n> * 8b132bc - helps a bit, though char-interdiff or word-interdiff might\n> be better. Nb. I think that red background for 'marked' is a bit\n> too dark (intensive).\n> \n> * c58499c - doesn't help too much.\n> \n> * f629c23, aa145bf - helps.\n> \n\nThanks for testing. I'll experiment with less intensive backgrounds.\n\n> > gitweb/gitweb.perl | 57\n> > +++++++++++++++++++++++++++++++++++++++------------ 1 files\n> > changed, 43 insertions(+), 14 deletions(-)\n> > \n> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> > index 872ba12..c056e83 100755\n> > --- a/gitweb/gitweb.perl\n> > +++ b/gitweb/gitweb.perl\n> > @@ -5057,12 +5057,12 @@ sub print_inline_diff_lines {\n> > # Format removed and added line, mark changed part and HTML-format\n> > them. # Impementation is based on contrib/diff-highlight\n> \n> Implementation\n> ^---\n> \n> > sub format_rem_add_line {\n> > -\tmy ($rem, $add) = @_;\n> > +\tmy ($rem, $add, $num_parents) = @_;\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> \n> Nice.\n> \n> [...]\n> > @@ -5099,15 +5099,43 @@ sub format_rem_add_line {\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\n> > added line.\n> > -\t# Combined diffs are not supported ATM.\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\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 < @$add; $i++) {\n> > +\t\t\t\tmy $prefix_rem =\n> > substr($rem->[$i], 0, $num_parents);\n> > +\t\t\t\tmy $prefix_add =\n> > substr($add->[$i], 0, $num_parents); +\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> Good.\n> \n> > +\n> > +\tif ($can_highlight) {\n> > \t\tfor (my $i = 0; $i < @$add; $i++) {\n> > \t\t\tmy ($line_rem, $line_add) =\n> > format_rem_add_line(\n> > -\t\t\t $rem->[$i], $add->[$i]);\n> > +\t\t\t $rem->[$i], $add->[$i],\n> > $num_parents); push @new_rem, $line_rem;\n> > \t\t\tpush @new_add, $line_add;\n> > \t\t}\n> \n> [...]\n> > @@ -5326,7 +5355,7 @@ sub git_patchset_body {\n> > \n> > \t} continue {\n> > \t\tif (@chunk) {\n> > -\t\t\tprint_diff_chunk($diff_style,\n> > $is_combined, \\%from, \\%to, @chunk);\n> > +\t\t\tprint_diff_chunk($diff_style, scalar\n> > @hash_parents, \\%from, \\%to, @chunk); @chunk = ();\n> > \t\t}\n> > \t\tprint \"\\n\"; # class=\"patch\"\n> \n> I was wondering about 'commitdiff' between two commits, which is not\n> combined even ifany of those commits is a merge commit... but it looks\n> like it works all right.\n> \n"},{"id":"188158","messageId":"201203301534.35484.jnareb@gmail.com","threadId":"30043","inReplyTo":"20120329193152.3d0f27e2@gmail.com","subject":"Re: [PATCH v2 4/8] gitweb: Use print_diff_chunk() for both side-by-side and inline diffs","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-30T13:34:34Z","receivedAt":"2012-03-30T13:34:34Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 29 Mar 2012, Michał Kiedrowicz wrote:\n> Jakub Narebski wrote:\n\n> > BTW. I was wondering about binary files, but this commit wouldn't make\n> > it worse anyway as we parse diff output assuming unified-like diff for\n> > diff syntax highlighting... and binary diffs are shown as\n> > \n> > \"Binary files a/foo.png and b/foo.png differ\"\n> > \n> > in extended diff header.\n> \n> Yeah, this is what I wrote in the cover letter:\n> \n> \t* Ensured that binary patches are not supported in HTML format,\n> \t thus this series canot break it :) (answering Jakub's questions)\n> \n> but maybe I wasn't clear enough.\n\nEh, sorry, this was more of reminder to myself, rather than questioning\nyou if you really checked that. Sorry for that. \n\n-- \nJakub Narebski\nPoland\n"},{"id":"188159","messageId":"20120330153710.3a165beb@mkiedrowicz.ivo.pl","threadId":"30043","inReplyTo":"201203301534.35484.jnareb@gmail.com","subject":"Re: [PATCH v2 4/8] gitweb: Use print_diff_chunk() for both side-by-side and inline diffs","fromName":"Michal Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-03-30T13:37:10Z","receivedAt":"2012-03-30T13:37:10Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jakub Narebski wrote:\n\n> On Thu, 29 Mar 2012, Michał Kiedrowicz wrote:\n> > Jakub Narebski wrote:\n> \n> > > BTW. I was wondering about binary files, but this commit wouldn't\n> > > make it worse anyway as we parse diff output assuming\n> > > unified-like diff for diff syntax highlighting... and binary\n> > > diffs are shown as\n> > > \n> > > \"Binary files a/foo.png and b/foo.png differ\"\n> > > \n> > > in extended diff header.\n> > \n> > Yeah, this is what I wrote in the cover letter:\n> > \n> > \t* Ensured that binary patches are not supported in HTML\n> > format, thus this series canot break it :) (answering Jakub's\n> > questions)\n> > \n> > but maybe I wasn't clear enough.\n> \n> Eh, sorry, this was more of reminder to myself, rather than\n> questioning you if you really checked that. Sorry for that. \n> \n\nOK, no problem. Thanks for your very thorough review.\n"},{"id":"188160","messageId":"201203301537.39429.jnareb@gmail.com","threadId":"30043","inReplyTo":"20120329192557.4bd45322@gmail.com","subject":"Re: [PATCH v2 3/8] gitweb: Extract print_sidebyside_diff_lines()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-30T13:37:38Z","receivedAt":"2012-03-30T13:37:38Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 29 Mar 2012, Michał Kiedrowicz wrote:\n> Jakub Narebski wrote:\n> > On Fri, 23 Mar 2012, Michał Kiedrowicz wrote:\n\n> > > This is a preparation patch for diff refinement highlightning. 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'it\n> > > printed just before printing added and removed lines.\n> > \n> > , and at the end\n> > of chunk.\n\nPlease don't forget to add that information. I was wondering about this,\nand checked code that it works all right.\n\n> > IMVHO such change is irrelevant.\n\nThis refers to \"but it slightly changes its behavior\", which might be\nnot clear from the position of this sentence.\n\n> > Acked-by: Jakub Narębski \n> \n> Thanks.\n\n-- \nJakub Narebski\nPoland\n"}]}