{"thread":{"id":"28700","subject":"[PATCH 2/2] gitweb: add a feature to show side-by-side diff","startedAt":"2011-10-17T07:00:18Z","lastAt":"2011-10-30T18:56:39Z","messageCount":5,"participants":["Kato Kazuyoshi","Junio C Hamano","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"177836","messageId":"CAFo4x0L4BAKnCDa1uEK0Rskd9kTsR-94D4mkYKnLGqVDnuyuBA@mail.gmail.com","threadId":"28700","inReplyTo":null,"subject":"[PATCH 2/2] gitweb: add a feature to show side-by-side diff","fromName":"Kato Kazuyoshi","fromEmail":"kato.kazuyoshi@gmail.com","sentAt":"2011-10-17T07:00:18Z","receivedAt":"2011-10-17T07:00:18Z","isPatch":true,"sender":{"key":"kato.kazuyoshi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/19111?v=4"},"body":"gitweb currently has a feature to show diff but it doesn't support\n\"side-by-side\" style. This modification introduces:\n\n * The \"ds\" query parameter to specify the style of diff.\n * The format_diff_chunk() to reorganize an output of diff.\n * The diff_nav() for form.\n---\n gitweb/gitweb.perl       |   81 +++++++++++++++++++++++++++++++++++++++++----\n gitweb/static/gitweb.css |   15 ++++++++\n 2 files changed, 88 insertions(+), 8 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 095adda..dca09dc 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -757,6 +757,7 @@ our @cgi_param_mapping = (\n \textra_options => \"opt\",\n \tsearch_use_regexp => \"sr\",\n \tctag => \"by_tag\",\n+\tdiff_style => \"ds\",\n \t# this must be last entry (for manipulation from JavaScript)\n \tjavascript => \"js\"\n );\n@@ -1072,6 +1073,8 @@ sub evaluate_and_validate_params {\n \t\t}\n \t\t$search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;\n \t}\n+\n+\t$input_params{diff_style} ||= 'inline';\n }\n\n # path to the current git repository\n@@ -2276,7 +2279,7 @@ sub format_diff_line {\n \t\t}\n \t\t$line = \"<span class=\\\"chunk_info\\\">@@ $from_text $to_text @@</span>\" .\n \t\t        \"<span class=\\\"section\\\">\" . esc_html($section, -nbsp=>1) .\n\"</span>\";\n-\t\treturn \"$div_open$line</div>\\n\";\n+\t\treturn $diff_class, \"$div_open$line</div>\\n\";\n \t} elsif ($from && $to && $line =~ m/^\\@{3}/) {\n \t\tmy ($prefix, $ranges, $section) = $line =~ m/^(\\@+) (.*?) \\@+(.*)$/;\n \t\tmy (@from_text, @from_start, @from_nlines, $to_text, $to_start, $to_nlines);\n@@ -2309,9 +2312,9 @@ sub format_diff_line {\n \t\t}\n \t\t$line .= \" $prefix</span>\" .\n \t\t         \"<span class=\\\"section\\\">\" . esc_html($section, -nbsp=>1)\n. \"</span>\";\n-\t\treturn \"$div_open$line</div>\\n\";\n+\t\treturn $diff_class, \"$div_open$line</div>\\n\";\n \t}\n-\treturn $div_open . esc_html($line, -nbsp=>1) . \"</div>\\n\";\n+\treturn $diff_class, $div_open . esc_html($line, -nbsp=>1) . \"</div>\\n\";\n }\n\n # Generates undef or something like \"_snapshot_\" or \"snapshot (_tbz2_ _zip_)\",\n@@ -4828,8 +4831,32 @@ sub git_difftree_body {\n \tprint \"</table>\\n\";\n }\n\n+sub format_diff_chunk {\n+\tmy @chunk = @_;\n+\n+\tmy $first_class = $chunk[0]->[0];\n+\tmy @partial = map { $_->[1] } grep { $_->[0] eq $first_class } @chunk;\n+\n+\tif (scalar @partial < scalar @chunk) {\n+\t\treturn join '', (\"<div class='chunk'><div class='old'>\",\n+\t\t             @partial,\n+\t\t             \"</div>\",\n+\t\t             \"<div class='new'>\",\n+\t\t             (map {\n+\t\t                 $_->[1];\n+\t\t             } @chunk[scalar @partial..scalar @chunk-1]),\n+\t\t             \"</div></div>\");\n+\t} else {\n+\t\treturn join '', (\"<div class='chunk'><div class='\",\n+\t\t             ($first_class eq 'add' ? 'new' : 'old'),\n+\t\t             \"'>\",\n+\t\t             @partial,\n+\t\t             \"</div></div>\");\n+\t}\n+}\n+\n sub git_patchset_body {\n-\tmy ($fd, $difftree, $hash, @hash_parents) = @_;\n+\tmy ($fd, $is_inline, $difftree, $hash, @hash_parents) = @_;\n \tmy ($hash_parent) = $hash_parents[0];\n\n \tmy $is_combined = (@hash_parents > 1);\n@@ -4940,12 +4967,31 @@ sub git_patchset_body {\n\n \t\t# the patch itself\n \tLINE:\n+\t\tmy @chunk;\n \t\twhile ($patch_line = <$fd>) {\n \t\t\tchomp $patch_line;\n\n \t\t\tnext PATCH if ($patch_line =~ m/^diff /);\n\n-\t\t\tprint format_diff_line($patch_line, \\%from, \\%to);\n+\t\t\tmy ($class, $line) = format_diff_line($patch_line, \\%from, \\%to);\n+\t\t\tif ($is_inline) {\n+\t\t\t\tprint $line;\n+\t\t\t} elsif ($class eq 'add' || $class eq 'rem') {\n+\t\t\t\tpush @chunk, [ $class, $line ];\n+\t\t\t} else {\n+\t\t\t\tif (@chunk) {\n+\t\t\t\t\tprint format_diff_chunk(@chunk);\n+\t\t\t\t\t@chunk = ();\n+\t\t\t\t} elsif ($class eq 'chunk_header') {\n+\t\t\t\t\tprint $line;\n+\t\t\t\t} else {\n+\t\t\t\t\tprint '<div class=\"chunk\"><div class=\"old\">',\n+\t\t\t\t\t      $line,\n+\t\t\t\t\t      '</div><div class=\"new\">',\n+\t\t\t\t\t      $line,\n+\t\t\t\t\t      '</div></div>';\n+\t\t\t\t}\n+\t\t\t}\n \t\t}\n\n \t} continue {\n@@ -7053,7 +7099,8 @@ sub git_blobdiff {\n \tif ($format eq 'html') {\n \t\tprint \"<div class=\\\"page_body\\\">\\n\";\n\n-\t\tgit_patchset_body($fd, [ \\%diffinfo ], $hash_base, $hash_parent_base);\n+\t\tgit_patchset_body($fd, $input_params{diff_style} eq 'inline',\n+\t\t                  [ \\%diffinfo ], $hash_base, $hash_parent_base);\n \t\tclose $fd;\n\n \t\tprint \"</div>\\n\"; # class=\"page_body\"\n@@ -7078,6 +7125,22 @@ sub git_blobdiff_plain {\n \tgit_blobdiff('plain');\n }\n\n+sub diff_nav {\n+\tmy ($style) = @_;\n+\n+\tmy %pairs = (inline => 'inline', 'sidebyside' => 'side by side');\n+\tjoin '', ($cgi->start_form({ method => 'get' }),\n+\t          $cgi->hidden('p'),\n+\t          $cgi->hidden('a'),\n+\t          $cgi->hidden('h'),\n+\t          $cgi->hidden('hp'),\n+\t          $cgi->hidden('hb'),\n+\t          $cgi->hidden('hpb'),\n+\t          $cgi->popup_menu('ds', [keys %pairs], $style, \\%pairs),\n+\t          $cgi->submit('change'),\n+\t          $cgi->end_form);\n+}\n+\n sub git_commitdiff {\n \tmy %params = @_;\n \tmy $format = $params{-format} || 'html';\n@@ -7230,7 +7293,8 @@ sub git_commitdiff {\n \t\tmy $ref = format_ref_marker($refs, $co{'id'});\n\n \t\tgit_header_html(undef, $expires);\n-\t\tgit_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, $formats_nav);\n+\t\tgit_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash,\n+\t\t                   $formats_nav . diff_nav($input_params{diff_style}));\n \t\tgit_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash);\n \t\tprint \"<div class=\\\"title_text\\\">\\n\" .\n \t\t      \"<table class=\\\"object_header\\\">\\n\";\n@@ -7284,7 +7348,8 @@ sub git_commitdiff {\n \t\t                  $use_parents ? @{$co{'parents'}} : $hash_parent);\n \t\tprint \"<br/>\\n\";\n\n-\t\tgit_patchset_body($fd, \\@difftree, $hash,\n+\t\tgit_patchset_body($fd, $input_params{diff_style} eq 'inline',\n+\t\t                  \\@difftree, $hash,\n \t\t                  $use_parents ? @{$co{'parents'}} : $hash_parent);\n \t\tclose $fd;\n \t\tprint \"</div>\\n\"; # class=\"page_body\"\ndiff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css\nindex 7d88509..dc84db2 100644\n--- a/gitweb/static/gitweb.css\n+++ b/gitweb/static/gitweb.css\n@@ -618,6 +618,21 @@ div.remote {\n \tcursor: pointer;\n }\n\n+/* side-by-side diff */\n+div.chunk {\n+\toverflow: hidden;\n+}\n+\n+div.chunk div.old {\n+\tfloat: left;\n+\twidth: 50%;\n+\toverflow: hidden;\n+}\n+\n+div.chunk div.new {\n+\tmargin-left: 50%;\n+\twidth: 50%;\n+}\n\n /* Style definition generated by highlight 2.4.5, http://www.andre-simon.de/ */\n\n-- \n1.7.7.213.g8b0e1.dirty\n"},{"id":"177878","messageId":"7vwrc3l898.fsf@alter.siamese.dyndns.org","threadId":"28700","inReplyTo":"CAFo4x0L4BAKnCDa1uEK0Rskd9kTsR-94D4mkYKnLGqVDnuyuBA@mail.gmail.com","subject":"Re: [PATCH 2/2] gitweb: add a feature to show side-by-side diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-17T19:37:23Z","receivedAt":"2011-10-17T19:37:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kato Kazuyoshi <kato.kazuyoshi@gmail.com> writes:\n\n> @@ -2276,7 +2279,7 @@ sub format_diff_line {\n>  \t\t}\n>  \t\t$line = \"<span class=\\\"chunk_info\\\">@@ $from_text $to_text @@</span>\" .\n>  \t\t        \"<span class=\\\"section\\\">\" . esc_html($section, -nbsp=>1) .\n> \"</span>\";\n\nCorrupt patch (wrapped long line); please fix.\n\nI suspect you have similar issues in the [PATCH 1/2] message, too.\n"},{"id":"177892","messageId":"m34nz771mj.fsf@localhost.localdomain","threadId":"28700","inReplyTo":"CAFo4x0L4BAKnCDa1uEK0Rskd9kTsR-94D4mkYKnLGqVDnuyuBA@mail.gmail.com","subject":"Re: [PATCH 2/2] gitweb: add a feature to show side-by-side diff","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-10-17T21:24:51Z","receivedAt":"2011-10-17T21:24:51Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Kato Kazuyoshi <kato.kazuyoshi@gmail.com> writes:\n\n> gitweb currently has a feature to show diff but it doesn't support\n> \"side-by-side\" style. This modification introduces:\n> \n>  * The \"ds\" query parameter to specify the style of diff.\n>  * The format_diff_chunk() to reorganize an output of diff.\n>  * The diff_nav() for form.\n\nI would state it a bit differently.\n\nI would say that this patch introduces support for side-by-side diff\nin git_patchset_body, and that style of diff is controlled by newly\nintroduces 'diff_style' (\"ds\") parameter.\n\nI would say a bit later that navigation bar got extended to make it\neasy to switch between 'inline' and 'sidebyside' diff.\n\n> ---\n>  gitweb/gitweb.perl       |   81 +++++++++++++++++++++++++++++++++++++++++----\n>  gitweb/static/gitweb.css |   15 ++++++++\n>  2 files changed, 88 insertions(+), 8 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 095adda..dca09dc 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -757,6 +757,7 @@ our @cgi_param_mapping = (\n>  \textra_options => \"opt\",\n>  \tsearch_use_regexp => \"sr\",\n>  \tctag => \"by_tag\",\n> +\tdiff_style => \"ds\",\n>  \t# this must be last entry (for manipulation from JavaScript)\n>  \tjavascript => \"js\"\n>  );\n\nO.K.\n\n> @@ -1072,6 +1073,8 @@ sub evaluate_and_validate_params {\n>  \t\t}\n>  \t\t$search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;\n>  \t}\n> +\n> +\t$input_params{diff_style} ||= 'inline';\n>  }\n\nO.K.\n \n>  # path to the current git repository\n> @@ -2276,7 +2279,7 @@ sub format_diff_line {\n>  \t\t}\n>  \t\t$line = \"<span class=\\\"chunk_info\\\">@@ $from_text $to_text @@</span>\" .\n>  \t\t        \"<span class=\\\"section\\\">\" . esc_html($section, -nbsp=>1) .\n> \"</span>\";\n> -\t\treturn \"$div_open$line</div>\\n\";\n> +\t\treturn $diff_class, \"$div_open$line</div>\\n\";\n>  \t} elsif ($from && $to && $line =~ m/^\\@{3}/) {\n>  \t\tmy ($prefix, $ranges, $section) = $line =~ m/^(\\@+) (.*?) \\@+(.*)$/;\n>  \t\tmy (@from_text, @from_start, @from_nlines, $to_text, $to_start, $to_nlines);\n\nIf you are changing behavior of format_diff_line, by having it return\n<class>, <formatted line> line pair, I think you should document it,\nand change the name of function.\n\nAll format_* functions in gitweb return a single string, so that\n\n  print format_*(...);\n\nworks.\n\n> @@ -4828,8 +4831,32 @@ sub git_difftree_body {\n>  \tprint \"</table>\\n\";\n>  }\n> \n\nIt would be good idea to add some comment about this subroutine,\ne.g. what parameters it accepts.  And perhaps how it works, as it is\nnot obvious.\n\n> +sub format_diff_chunk {\n> +\tmy @chunk = @_;\n> +\n> +\tmy $first_class = $chunk[0]->[0];\n> +\tmy @partial = map { $_->[1] } grep { $_->[0] eq $first_class } @chunk;\n> +\n> +\tif (scalar @partial < scalar @chunk) {\n> +\t\treturn join '', (\"<div class='chunk'><div class='old'>\",\n> +\t\t             @partial,\n> +\t\t             \"</div>\",\n> +\t\t             \"<div class='new'>\",\n> +\t\t             (map {\n> +\t\t                 $_->[1];\n> +\t\t             } @chunk[scalar @partial..scalar @chunk-1]),\n> +\t\t             \"</div></div>\");\n> +\t} else {\n> +\t\treturn join '', (\"<div class='chunk'><div class='\",\n> +\t\t             ($first_class eq 'add' ? 'new' : 'old'),\n> +\t\t             \"'>\",\n> +\t\t             @partial,\n> +\t\t             \"</div></div>\");\n> +\t}\n> +}\n> +\n>  sub git_patchset_body {\n> -\tmy ($fd, $difftree, $hash, @hash_parents) = @_;\n> +\tmy ($fd, $is_inline, $difftree, $hash, @hash_parents) = @_;\n\nHmmm... shouldn't changing git_patchset_body signature (calling\nconvention) be mentioned in commit message?\n\n>  \tmy ($hash_parent) = $hash_parents[0];\n> \n>  \tmy $is_combined = (@hash_parents > 1);\n> @@ -4940,12 +4967,31 @@ sub git_patchset_body {\n> \n>  \t\t# the patch itself\n>  \tLINE:\n> +\t\tmy @chunk;\n>  \t\twhile ($patch_line = <$fd>) {\n>  \t\t\tchomp $patch_line;\n> \n>  \t\t\tnext PATCH if ($patch_line =~ m/^diff /);\n> \n> -\t\t\tprint format_diff_line($patch_line, \\%from, \\%to);\n> +\t\t\tmy ($class, $line) = format_diff_line($patch_line, \\%from, \\%to);\n> +\t\t\tif ($is_inline) {\n> +\t\t\t\tprint $line;\n> +\t\t\t} elsif ($class eq 'add' || $class eq 'rem') {\n> +\t\t\t\tpush @chunk, [ $class, $line ];\n> +\t\t\t} else {\n> +\t\t\t\tif (@chunk) {\n> +\t\t\t\t\tprint format_diff_chunk(@chunk);\n> +\t\t\t\t\t@chunk = ();\n> +\t\t\t\t} elsif ($class eq 'chunk_header') {\n> +\t\t\t\t\tprint $line;\n> +\t\t\t\t} else {\n> +\t\t\t\t\tprint '<div class=\"chunk\"><div class=\"old\">',\n> +\t\t\t\t\t      $line,\n> +\t\t\t\t\t      '</div><div class=\"new\">',\n> +\t\t\t\t\t      $line,\n> +\t\t\t\t\t      '</div></div>';\n> +\t\t\t\t}\n> +\t\t\t}\n\nI wonder why you have output of context lines in side-by-side diff in\ngit_patchset_body, instead of having it all in format_diff_chunk.\n\nUnless by chunk you mean something different than \"unified format hunk\"\nthat is defined e.g. in (diff.info)\"Detailed Unified\".\n\n>  \t\t}\n> \n>  \t} continue {\n> @@ -7053,7 +7099,8 @@ sub git_blobdiff {\n>  \tif ($format eq 'html') {\n>  \t\tprint \"<div class=\\\"page_body\\\">\\n\";\n> \n> -\t\tgit_patchset_body($fd, [ \\%diffinfo ], $hash_base, $hash_parent_base);\n> +\t\tgit_patchset_body($fd, $input_params{diff_style} eq 'inline',\n> +\t\t                  [ \\%diffinfo ], $hash_base, $hash_parent_base);\n>  \t\tclose $fd;\n> \n>  \t\tprint \"</div>\\n\"; # class=\"page_body\"\n> @@ -7078,6 +7125,22 @@ sub git_blobdiff_plain {\n>  \tgit_blobdiff('plain');\n>  }\n> \n> +sub diff_nav {\n> +\tmy ($style) = @_;\n> +\n> +\tmy %pairs = (inline => 'inline', 'sidebyside' => 'side by side');\n> +\tjoin '', ($cgi->start_form({ method => 'get' }),\n> +\t          $cgi->hidden('p'),\n> +\t          $cgi->hidden('a'),\n> +\t          $cgi->hidden('h'),\n> +\t          $cgi->hidden('hp'),\n> +\t          $cgi->hidden('hb'),\n> +\t          $cgi->hidden('hpb'),\n> +\t          $cgi->popup_menu('ds', [keys %pairs], $style, \\%pairs),\n> +\t          $cgi->submit('change'),\n> +\t          $cgi->end_form);\n> +}\n\nWhy do you feel the need for form to select between diff formats,\ninstead of links switching between them, like for interactive and\nnon-interactive blame?\n\n> +\n>  sub git_commitdiff {\n>  \tmy %params = @_;\n>  \tmy $format = $params{-format} || 'html';\n> @@ -7230,7 +7293,8 @@ sub git_commitdiff {\n>  \t\tmy $ref = format_ref_marker($refs, $co{'id'});\n> \n>  \t\tgit_header_html(undef, $expires);\n> -\t\tgit_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, $formats_nav);\n> +\t\tgit_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash,\n> +\t\t                   $formats_nav . diff_nav($input_params{diff_style}));\n>  \t\tgit_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash);\n>  \t\tprint \"<div class=\\\"title_text\\\">\\n\" .\n>  \t\t      \"<table class=\\\"object_header\\\">\\n\";\n> @@ -7284,7 +7348,8 @@ sub git_commitdiff {\n>  \t\t                  $use_parents ? @{$co{'parents'}} : $hash_parent);\n>  \t\tprint \"<br/>\\n\";\n> \n> -\t\tgit_patchset_body($fd, \\@difftree, $hash,\n> +\t\tgit_patchset_body($fd, $input_params{diff_style} eq 'inline',\n> +\t\t                  \\@difftree, $hash,\n>  \t\t                  $use_parents ? @{$co{'parents'}} : $hash_parent);\n>  \t\tclose $fd;\n>  \t\tprint \"</div>\\n\"; # class=\"page_body\"\n\nOnly commitdiff?  What about blobdiff?\n\n> diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css\n> index 7d88509..dc84db2 100644\n> --- a/gitweb/static/gitweb.css\n> +++ b/gitweb/static/gitweb.css\n> @@ -618,6 +618,21 @@ div.remote {\n>  \tcursor: pointer;\n>  }\n> \n> +/* side-by-side diff */\n> +div.chunk {\n> +\toverflow: hidden;\n\n???\n\n> +}\n> +\n> +div.chunk div.old {\n> +\tfloat: left;\n> +\twidth: 50%;\n> +\toverflow: hidden;\n> +}\n> +\n> +div.chunk div.new {\n> +\tmargin-left: 50%;\n> +\twidth: 50%;\n> +}\n\nHmmm... I think the way side-by-side diff is styled should be\nexplained in commit message, or in comments.\n\nI also wonder if it wouldn't be simpler to use table here, instead of\nfiddling with floats, widths and margins.\n\n-- \nJakub Narębski\n"},{"id":"177942","messageId":"m3zkgy6103.fsf@localhost.localdomain","threadId":"28700","inReplyTo":"m34nz771mj.fsf@localhost.localdomain","subject":"Re: [PATCH 2/2] gitweb: add a feature to show side-by-side diff","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-10-18T10:36:02Z","receivedAt":"2011-10-18T10:36:02Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> Kato Kazuyoshi <kato.kazuyoshi@gmail.com> writes:\n> \n> > gitweb currently has a feature to show diff but it doesn't support\n> > \"side-by-side\" style. This modification introduces:\n> > \n> >  * The \"ds\" query parameter to specify the style of diff.\n> >  * The format_diff_chunk() to reorganize an output of diff.\n> >  * The diff_nav() for form.\n> \n> I would state it a bit differently.\n> \n> I would say that this patch introduces support for side-by-side diff\n> in git_patchset_body, and that style of diff is controlled by newly\n> introduces 'diff_style' (\"ds\") parameter.\n> \n> I would say a bit later that navigation bar got extended to make it\n> easy to switch between 'inline' and 'sidebyside' diff.\n\nI think it would be good idea to explain algorithm here, and perhaps\nalso layout used.\n\nWhen I was thinking about implementing side-by-side diff in gitweb, I\nwas always stopped by the problem of aligning changes.  In your\nsolution changes are always aligned to top, which is a simple\nsolution.\n\n> > +sub format_diff_chunk {\n> > +\tmy @chunk = @_;\n> > +\n> > +\tmy $first_class = $chunk[0]->[0];\n> > +\tmy @partial = map { $_->[1] } grep { $_->[0] eq $first_class } @chunk;\n> > +\n> > +\tif (scalar @partial < scalar @chunk) {\n> > +\t\treturn join '', (\"<div class='chunk'><div class='old'>\",\n> > +\t\t             @partial,\n> > +\t\t             \"</div>\",\n> > +\t\t             \"<div class='new'>\",\n> > +\t\t             (map {\n> > +\t\t                 $_->[1];\n> > +\t\t             } @chunk[scalar @partial..scalar @chunk-1]),\n> > +\t\t             \"</div></div>\");\n> > +\t} else {\n> > +\t\treturn join '', (\"<div class='chunk'><div class='\",\n> > +\t\t             ($first_class eq 'add' ? 'new' : 'old'),\n> > +\t\t             \"'>\",\n> > +\t\t             @partial,\n> > +\t\t             \"</div></div>\");\n> > +\t}\n> > +}\n\nThis is I think unnecessary complicated.  What you do here is\nseparating removed and added lines (either can be empty), and putting\nremoved on left (as \"old\"), and added on right (as \"new\").\n\nIt means that the following unified diff:\n\n     --- a/foo\n     +++ b/foo\n     @@ -1,5 +1,4 @@\n     -foo\n     -FOO\n      bar\n     -baz\n     +b\n     +baZ\n      quux\n\nwould be turned into following side by side diff:\n\n      foo             | \n      FOO             |\n      bar             |  bar\n      baz             |  b\n                      |  baZ\n      quux            |  quux\n\nIt's a bit strange that context is put line by line, and changed lines\nare put in \"blocks\".\n\n> > @@ -4940,12 +4967,31 @@ sub git_patchset_body {\n> > \n> >  \t\t# the patch itself\n> >  \tLINE:\n> > +\t\tmy @chunk;\n> >  \t\twhile ($patch_line = <$fd>) {\n> >  \t\t\tchomp $patch_line;\n> > \n> >  \t\t\tnext PATCH if ($patch_line =~ m/^diff /);\n> > \n> > -\t\t\tprint format_diff_line($patch_line, \\%from, \\%to);\n> > +\t\t\tmy ($class, $line) = format_diff_line($patch_line, \\%from, \\%to);\n> > +\t\t\tif ($is_inline) {\n> > +\t\t\t\tprint $line;\n> > +\t\t\t} elsif ($class eq 'add' || $class eq 'rem') {\n> > +\t\t\t\tpush @chunk, [ $class, $line ];\n> > +\t\t\t} else {\n> > +\t\t\t\tif (@chunk) {\n> > +\t\t\t\t\tprint format_diff_chunk(@chunk);\n> > +\t\t\t\t\t@chunk = ();\n> > +\t\t\t\t} elsif ($class eq 'chunk_header') {\n> > +\t\t\t\t\tprint $line;\n> > +\t\t\t\t} else {\n> > +\t\t\t\t\tprint '<div class=\"chunk\"><div class=\"old\">',\n> > +\t\t\t\t\t      $line,\n> > +\t\t\t\t\t      '</div><div class=\"new\">',\n> > +\t\t\t\t\t      $line,\n> > +\t\t\t\t\t      '</div></div>';\n> > +\t\t\t\t}\n> > +\t\t\t}\n\nNote that your side by side diff wouldn't work for combined diff,\nwhich gitweb supports.  You should show 'unified' / 'inline' format\nfor combined diff (more than one parent / from).\n\n-- \nJakub Narębski\n"},{"id":"178533","messageId":"m3sjma48ce.fsf@localhost.localdomain","threadId":"28700","inReplyTo":"CAFo4x0L4BAKnCDa1uEK0Rskd9kTsR-94D4mkYKnLGqVDnuyuBA@mail.gmail.com","subject":"Re: [PATCH 2/2] gitweb: add a feature to show side-by-side diff","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-10-30T18:56:39Z","receivedAt":"2011-10-30T18:56:39Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Those are additional comments, most of which I have come about when\nrewriting this series and testing it.\n\nThere are to complement existing comments in other post(s).\n\nKato Kazuyoshi <kato.kazuyoshi@gmail.com> writes:\n\n> ---\n>  gitweb/gitweb.perl       |   81 +++++++++++++++++++++++++++++++++++++++++----\n>  gitweb/static/gitweb.css |   15 ++++++++\n>  2 files changed, 88 insertions(+), 8 deletions(-)\n\nNo tests.\n \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 095adda..dca09dc 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -757,6 +757,7 @@ our @cgi_param_mapping = (\n>  \textra_options => \"opt\",\n>  \tsearch_use_regexp => \"sr\",\n>  \tctag => \"by_tag\",\n> +\tdiff_style => \"ds\",\n>  \t# this must be last entry (for manipulation from JavaScript)\n>  \tjavascript => \"js\"\n>  );\n\nAlternate solution would be to use 'extra_options' (\"opt\") for\nthat... though current use of it in gitweb seems to suggest that it is\nmore about passing extra options to underlying git commands; and \"git\ndiff\" doesn't support '--side-by-side' like GNU diff does, (yet?).\n\nSo currently I favor neither.\n\n> @@ -1072,6 +1073,8 @@ sub evaluate_and_validate_params {\n>  \t\t}\n>  \t\t$search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;\n>  \t}\n> +\n> +\t$input_params{diff_style} ||= 'inline';\n>  }\n\nHmmm... similar option 'order' (\"o\") had default value set in action\nsubroutine.  I wonder if it wouldn't be better to do the same also in\nthis situation.\n\n> @@ -2276,7 +2279,7 @@ sub format_diff_line {\n>  \t\t}\n>  \t\t$line = \"<span class=\\\"chunk_info\\\">@@ $from_text $to_text @@</span>\" .\n>  \t\t        \"<span class=\\\"section\\\">\" . esc_html($section, -nbsp=>1) .\n> \"</span>\";\n> -\t\treturn \"$div_open$line</div>\\n\";\n> +\t\treturn $diff_class, \"$div_open$line</div>\\n\";\n>  \t} elsif ($from && $to && $line =~ m/^\\@{3}/) {\n>  \t\tmy ($prefix, $ranges, $section) = $line =~ m/^(\\@+) (.*?) \\@+(.*)$/;\n>  \t\tmy (@from_text, @from_start, @from_nlines, $to_text, $to_start, $to_nlines);\n\nThis subroutine no longer *format* line to be printed, isn't it?\n\n> @@ -4828,8 +4831,32 @@ sub git_difftree_body {\n>  \tprint \"</table>\\n\";\n>  }\n> \n> +sub format_diff_chunk {\n\nName: it is not about diff chunk (or hunk), but about a block of lines\nin a chunk; in this case block of change lines (rem / add).  Also, it\nis not about generic diff, only about sidebyside one.\n\nBTW. I think it would be better if this subroutine also managed\ncontext lines.\n\n> +\tmy @chunk = @_;\n> +\n> +\tmy $first_class = $chunk[0]->[0];\n\nStyle: You can use simply $chunk[0][0] here.  perlref(1) says:\n\n  \"The arrow is optional between brackets subscripts, [...]\" \n\n> +\tmy @partial = map { $_->[1] } grep { $_->[0] eq $first_class } @chunk;\n> +\n> +\tif (scalar @partial < scalar @chunk) {\n\nStyle: you can write simply\n\n  +\tif (@partial < @chunk) {\n\n> +\t\treturn join '', (\"<div class='chunk'><div class='old'>\",\n> +\t\t             @partial,\n> +\t\t             \"</div>\",\n> +\t\t             \"<div class='new'>\",\n> +\t\t             (map {\n> +\t\t                 $_->[1];\n> +\t\t             } @chunk[scalar @partial..scalar @chunk-1]),\n> +\t\t             \"</div></div>\");\n> +\t} else {\n> +\t\treturn join '', (\"<div class='chunk'><div class='\",\n> +\t\t             ($first_class eq 'add' ? 'new' : 'old'),\n> +\t\t             \"'>\",\n> +\t\t             @partial,\n> +\t\t             \"</div></div>\");\n> +\t}\n> +}\n\nAnyway this code is not very clear.  You rely on the fact that if\nthere are two classes, then they are \"rem\" first, and \"add\" second.\n\nAlso, it is I think overly complicated.\n\n> +\n>  sub git_patchset_body {\n> -\tmy ($fd, $difftree, $hash, @hash_parents) = @_;\n> +\tmy ($fd, $is_inline, $difftree, $hash, @hash_parents) = @_;\n\nRather than passing $is_inline, I think it would be better to pass\n$diff_style (with default value filled in)\n\n>  \tmy ($hash_parent) = $hash_parents[0];\n> \n>  \tmy $is_combined = (@hash_parents > 1);\n> @@ -4940,12 +4967,31 @@ sub git_patchset_body {\n> \n>  \t\t# the patch itself\n>  \tLINE:\n> +\t\tmy @chunk;\n>  \t\twhile ($patch_line = <$fd>) {\n>  \t\t\tchomp $patch_line;\n> \n>  \t\t\tnext PATCH if ($patch_line =~ m/^diff /);\n\nHere is a bug.  If patchset consists of more than one patch, if\nnot-last patches have change that does not have trailing context lines\n(changed, added or removed lines at the end of file), then the last\nblock will be lost (@chunk can be non-empty here).\n\n> \n> -\t\t\tprint format_diff_line($patch_line, \\%from, \\%to);\n> +\t\t\tmy ($class, $line) = format_diff_line($patch_line, \\%from, \\%to);\n> +\t\t\tif ($is_inline) {\n\nThat is wrong to test for.  You should test if you can use\nside-by-side diff, not if you use default output.  \n\nEspecially that diff can be combined diff of a merge commit, which\ncannot be represented as 2-sided side-by-side diff; for such diff\ngitweb needs to use inline diff.\n\n> +\t\t\t\tprint $line;\n> +\t\t\t} elsif ($class eq 'add' || $class eq 'rem') {\n> +\t\t\t\tpush @chunk, [ $class, $line ];\n> +\t\t\t} else {\n> +\t\t\t\tif (@chunk) {\n> +\t\t\t\t\tprint format_diff_chunk(@chunk);\n> +\t\t\t\t\t@chunk = ();\n> +\t\t\t\t} elsif ($class eq 'chunk_header') {\n> +\t\t\t\t\tprint $line;\n> +\t\t\t\t} else {\n> +\t\t\t\t\tprint '<div class=\"chunk\"><div class=\"old\">',\n> +\t\t\t\t\t      $line,\n> +\t\t\t\t\t      '</div><div class=\"new\">',\n> +\t\t\t\t\t      $line,\n> +\t\t\t\t\t      '</div></div>';\n\nAll of this should in my opinion be done in format_diff_chunk(), not\nin caller.  This also introduces a bit of inconsistency in that\nadded/removed lines are in single block and context lines are each in\nits own block.\n\nAdditionally you forgot about incomplete lines here, which can apply\neither to added lines, removed lines, both of added and removed lines,\nand to context lines.  Your code generates incorrect info in the case\nif incomplete line is either removed line only, or added line only.\n\n[Nb. I have to check my code yet again.]\n\n> +sub diff_nav {\n> +\tmy ($style) = @_;\n> +\n> +\tmy %pairs = (inline => 'inline', 'sidebyside' => 'side by side');\n> +\tjoin '', ($cgi->start_form({ method => 'get' }),\n> +\t          $cgi->hidden('p'),\n> +\t          $cgi->hidden('a'),\n> +\t          $cgi->hidden('h'),\n> +\t          $cgi->hidden('hp'),\n> +\t          $cgi->hidden('hb'),\n> +\t          $cgi->hidden('hpb'),\n> +\t          $cgi->popup_menu('ds', [keys %pairs], $style, \\%pairs),\n> +\t          $cgi->submit('change'),\n> +\t          $cgi->end_form);\n> +}\n\nWhat about 'f' and 'fp' for \"blobdiff\" view?\n\n> diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css\n> index 7d88509..dc84db2 100644\n> --- a/gitweb/static/gitweb.css\n> +++ b/gitweb/static/gitweb.css\n> @@ -618,6 +618,21 @@ div.remote {\n>  \tcursor: pointer;\n>  }\n> \n> +/* side-by-side diff */\n> +div.chunk {\n> +\toverflow: hidden;\n> +}\n> +\n> +div.chunk div.old {\n> +\tfloat: left;\n> +\twidth: 50%;\n> +\toverflow: hidden;\n> +}\n> +\n> +div.chunk div.new {\n> +\tmargin-left: 50%;\n> +\twidth: 50%;\n> +}\n\nNice trick of composing CSS layout... though I wonder if there is\nperhaps a better solution.\n\nAnyway, I think this addition should be put near style for div.diff\netc.\n\n-- \nJakub Narębski\n"}]}