Re: [PATCH 2/2] gitweb: add a feature to show side-by-side diff
From: Jakub Narebski <hidden>
Date: 2016-06-15 22:52:17
Kato Kazuyoshi [off-list ref] writes:
gitweb currently has a feature to show diff but it doesn't support "side-by-side" style. This modification introduces: * The "ds" query parameter to specify the style of diff. * The format_diff_chunk() to reorganize an output of diff. * The diff_nav() for form.
I would state it a bit differently.
I would say that this patch introduces support for side-by-side diff
in git_patchset_body, and that style of diff is controlled by newly
introduces 'diff_style' ("ds") parameter.
I would say a bit later that navigation bar got extended to make it
easy to switch between 'inline' and 'sidebyside' diff.
quoted hunk ↗ jump to hunk
--- gitweb/gitweb.perl | 81 +++++++++++++++++++++++++++++++++++++++++---- gitweb/static/gitweb.css | 15 ++++++++ 2 files changed, 88 insertions(+), 8 deletions(-)diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 095adda..dca09dc 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl@@ -757,6 +757,7 @@ our @cgi_param_mapping = ( extra_options => "opt", search_use_regexp => "sr", ctag => "by_tag", + diff_style => "ds", # this must be last entry (for manipulation from JavaScript) javascript => "js" );
O.K.
quoted hunk ↗ jump to hunk
@@ -1072,6 +1073,8 @@ sub evaluate_and_validate_params { } $search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext; } + + $input_params{diff_style} ||= 'inline'; }
O.K.
quoted hunk ↗ jump to hunk
# path to the current git repository@@ -2276,7 +2279,7 @@ sub format_diff_line { } $line = "<span class=\"chunk_info\">@@ $from_text $to_text @@</span>" . "<span class=\"section\">" . esc_html($section, -nbsp=>1) ."</span>"; - return "$div_open$line</div>\n"; + return $diff_class, "$div_open$line</div>\n"; } elsif ($from && $to && $line =~ m/^\@{3}/) { my ($prefix, $ranges, $section) = $line =~ m/^(\@+) (.*?) \@+(.*)$/; my (@from_text, @from_start, @from_nlines, $to_text, $to_start, $to_nlines);
If you are changing behavior of format_diff_line, by having it return <class>, <formatted line> line pair, I think you should document it, and change the name of function. All format_* functions in gitweb return a single string, so that print format_*(...); works.
quoted hunk ↗ jump to hunk
@@ -4828,8 +4831,32 @@ sub git_difftree_body { print "</table>\n"; }
It would be good idea to add some comment about this subroutine, e.g. what parameters it accepts. And perhaps how it works, as it is not obvious.
+sub format_diff_chunk {
+ my @chunk = @_;
+
+ my $first_class = $chunk[0]->[0];
+ my @partial = map { $_->[1] } grep { $_->[0] eq $first_class } @chunk;
+
+ if (scalar @partial < scalar @chunk) {
+ return join '', ("<div class='chunk'><div class='old'>",
+ @partial,
+ "</div>",
+ "<div class='new'>",
+ (map {
+ $_->[1];
+ } @chunk[scalar @partial..scalar @chunk-1]),
+ "</div></div>");
+ } else {
+ return join '', ("<div class='chunk'><div class='",
+ ($first_class eq 'add' ? 'new' : 'old'),
+ "'>",
+ @partial,
+ "</div></div>");
+ }
+}
+
sub git_patchset_body {
- my ($fd, $difftree, $hash, @hash_parents) = @_;
+ my ($fd, $is_inline, $difftree, $hash, @hash_parents) = @_;Hmmm... shouldn't changing git_patchset_body signature (calling convention) be mentioned in commit message?
quoted hunk ↗ jump to hunk
my ($hash_parent) = $hash_parents[0]; my $is_combined = (@hash_parents > 1);@@ -4940,12 +4967,31 @@ sub git_patchset_body { # the patch itself LINE: + my @chunk; while ($patch_line = <$fd>) { chomp $patch_line; next PATCH if ($patch_line =~ m/^diff /); - print format_diff_line($patch_line, \%from, \%to); + my ($class, $line) = format_diff_line($patch_line, \%from, \%to); + if ($is_inline) { + print $line; + } elsif ($class eq 'add' || $class eq 'rem') { + push @chunk, [ $class, $line ]; + } else { + if (@chunk) { + print format_diff_chunk(@chunk); + @chunk = (); + } elsif ($class eq 'chunk_header') { + print $line; + } else { + print '<div class="chunk"><div class="old">', + $line, + '</div><div class="new">', + $line, + '</div></div>'; + } + }
I wonder why you have output of context lines in side-by-side diff in git_patchset_body, instead of having it all in format_diff_chunk. Unless by chunk you mean something different than "unified format hunk" that is defined e.g. in (diff.info)"Detailed Unified".
quoted hunk ↗ jump to hunk
} } continue {@@ -7053,7 +7099,8 @@ sub git_blobdiff { if ($format eq 'html') { print "<div class=\"page_body\">\n"; - git_patchset_body($fd, [ \%diffinfo ], $hash_base, $hash_parent_base); + git_patchset_body($fd, $input_params{diff_style} eq 'inline', + [ \%diffinfo ], $hash_base, $hash_parent_base); close $fd; print "</div>\n"; # class="page_body"@@ -7078,6 +7125,22 @@ sub git_blobdiff_plain { git_blobdiff('plain'); } +sub diff_nav { + my ($style) = @_; + + my %pairs = (inline => 'inline', 'sidebyside' => 'side by side'); + join '', ($cgi->start_form({ method => 'get' }), + $cgi->hidden('p'), + $cgi->hidden('a'), + $cgi->hidden('h'), + $cgi->hidden('hp'), + $cgi->hidden('hb'), + $cgi->hidden('hpb'), + $cgi->popup_menu('ds', [keys %pairs], $style, \%pairs), + $cgi->submit('change'), + $cgi->end_form); +}
Why do you feel the need for form to select between diff formats, instead of links switching between them, like for interactive and non-interactive blame?
quoted hunk ↗ jump to hunk
+ sub git_commitdiff { my %params = @_; my $format = $params{-format} || 'html';@@ -7230,7 +7293,8 @@ sub git_commitdiff { my $ref = format_ref_marker($refs, $co{'id'}); git_header_html(undef, $expires); - git_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, $formats_nav); + git_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, + $formats_nav . diff_nav($input_params{diff_style})); git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash); print "<div class=\"title_text\">\n" . "<table class=\"object_header\">\n";@@ -7284,7 +7348,8 @@ sub git_commitdiff { $use_parents ? @{$co{'parents'}} : $hash_parent); print "<br/>\n"; - git_patchset_body($fd, \@difftree, $hash, + git_patchset_body($fd, $input_params{diff_style} eq 'inline', + \@difftree, $hash, $use_parents ? @{$co{'parents'}} : $hash_parent); close $fd; print "</div>\n"; # class="page_body"
Only commitdiff? What about blobdiff?
quoted hunk ↗ jump to hunk
diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css index 7d88509..dc84db2 100644 --- a/gitweb/static/gitweb.css +++ b/gitweb/static/gitweb.css@@ -618,6 +618,21 @@ div.remote { cursor: pointer; } +/* side-by-side diff */ +div.chunk { + overflow: hidden;
???
+}
+
+div.chunk div.old {
+ float: left;
+ width: 50%;
+ overflow: hidden;
+}
+
+div.chunk div.new {
+ margin-left: 50%;
+ width: 50%;
+}Hmmm... I think the way side-by-side diff is styled should be explained in commit message, or in comments. I also wonder if it wouldn't be simpler to use table here, instead of fiddling with floats, widths and margins. -- Jakub Narębski