* [PATCH/RFC] gitweb: add the ability to show side-by-side diff on commitdiff.
@ 2011-10-16 23:57 Kato Kazuyoshi
2011-10-17 0:20 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Kato Kazuyoshi @ 2011-10-16 23:57 UTC (permalink / raw)
To: git
---
Hello,
I want to add some features that provided by Trac to Gitweb.
First, I've implemented "side-by-side" style diff.
gitweb/gitweb.perl | 97 ++++++++++++++++++++++++++++++++++++++--------
gitweb/static/gitweb.css | 16 ++++++++
2 files changed, 96 insertions(+), 17 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 85d64b2..9d7609f 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"
);
@@ -1072,6 +1073,8 @@ sub evaluate_and_validate_params {
}
$search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;
}
+
+ $input_params{diff_style} ||= 'inline';
}
# path to the current git repository
@@ -2235,25 +2238,25 @@ sub format_diff_line {
# combined diff
my $prefix = substr($line, 0, scalar @{$from->{'href'}});
if ($line =~ m/^\@{3}/) {
- $diff_class = " chunk_header";
+ $diff_class = "chunk_header";
} elsif ($line =~ m/^\\/) {
- $diff_class = " incomplete";
+ $diff_class = "incomplete";
} elsif ($prefix =~ tr/+/+/) {
- $diff_class = " add";
+ $diff_class = "add";
} elsif ($prefix =~ tr/-/-/) {
- $diff_class = " rem";
+ $diff_class = "rem";
}
} else {
# assume ordinary diff
my $char = substr($line, 0, 1);
if ($char eq '+') {
- $diff_class = " add";
+ $diff_class = "add";
} elsif ($char eq '-') {
- $diff_class = " rem";
+ $diff_class = "rem";
} elsif ($char eq '@') {
- $diff_class = " chunk_header";
+ $diff_class = "chunk_header";
} elsif ($char eq "\\") {
- $diff_class = " incomplete";
+ $diff_class = "incomplete";
}
}
$line = untabify($line);
@@ -2274,7 +2277,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 class=\"diff$diff_class\">$line</div>\n";
+ return $diff_class, "<div class=\"diff $diff_class\">$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);
@@ -2307,9 +2310,9 @@ sub format_diff_line {
}
$line .= " $prefix</span>" .
"<span class=\"section\">" . esc_html($section, -nbsp=>1)
. "</span>";
- return "<div class=\"diff$diff_class\">$line</div>\n";
+ return $diff_class, "<div class=\"diff $diff_class\">$line</div>\n";
}
- return "<div class=\"diff$diff_class\">" . esc_html($line, -nbsp=>1)
. "</div>\n";
+ return $diff_class, "<div class=\"diff $diff_class\">" .
esc_html($line, -nbsp=>1) . "</div>\n";
}
# Generates undef or something like "_snapshot_" or "snapshot (_tbz2_ _zip_)",
@@ -4826,8 +4829,32 @@ sub git_difftree_body {
print "</table>\n";
}
+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) = @_;
my ($hash_parent) = $hash_parents[0];
my $is_combined = (@hash_parents > 1);
@@ -4938,12 +4965,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 q{<div class='chunk'><div class='old'>},
+ $line,
+ q{</div><div class='new'>},
+ $line,
+ q{</div></div>};
+ }
+ }
}
} continue {
@@ -7022,7 +7068,7 @@ sub git_blobdiff {
"raw");
git_header_html(undef, $expires);
if (defined $hash_base && (my %co = parse_commit($hash_base))) {
- git_print_page_nav('','', $hash_base,$co{'tree'},$hash_base, $formats_nav);
+ git_print_page_nav('','', $hash_base,$co{'tree'},$hash_base,
$formats_nav . diff_nav($input_params{diff_style}));
git_print_header_div('commit', esc_html($co{'title'}), $hash_base);
} else {
print "<div class=\"page_nav\"><br/>$formats_nav<br/></div>\n";
@@ -7051,7 +7097,7 @@ 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"
@@ -7076,6 +7122,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);
+}
+
sub git_commitdiff {
my %params = @_;
my $format = $params{-format} || 'html';
@@ -7228,7 +7290,7 @@ 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";
@@ -7282,7 +7344,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"
diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
index 7d88509..a6872f9 100644
--- a/gitweb/static/gitweb.css
+++ b/gitweb/static/gitweb.css
@@ -618,6 +618,22 @@ 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%;
+}
+
/* Style definition generated by highlight 2.4.5, http://www.andre-simon.de/ */
--
1.7.7.213.g8b0e1.dirty
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH/RFC] gitweb: add the ability to show side-by-side diff on commitdiff.
2011-10-16 23:57 [PATCH/RFC] gitweb: add the ability to show side-by-side diff on commitdiff Kato Kazuyoshi
@ 2011-10-17 0:20 ` Junio C Hamano
2011-10-17 1:15 ` Kato Kazuyoshi
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2011-10-17 0:20 UTC (permalink / raw)
To: Kato Kazuyoshi; +Cc: git
Kato Kazuyoshi <kato.kazuyoshi@gmail.com> writes:
> @@ -2235,25 +2238,25 @@ sub format_diff_line {
> # combined diff
> my $prefix = substr($line, 0, scalar @{$from->{'href'}});
> if ($line =~ m/^\@{3}/) {
> - $diff_class = " chunk_header";
> + $diff_class = "chunk_header";
> } elsif ($line =~ m/^\\/) {
> - $diff_class = " incomplete";
> + $diff_class = "incomplete";
> } elsif ($prefix =~ tr/+/+/) {
> - $diff_class = " add";
> + $diff_class = "add";
> } elsif ($prefix =~ tr/-/-/) {
> - $diff_class = " rem";
> + $diff_class = "rem";
> }
> } else {
> # assume ordinary diff
> my $char = substr($line, 0, 1);
> if ($char eq '+') {
> - $diff_class = " add";
> + $diff_class = "add";
> } elsif ($char eq '-') {
> - $diff_class = " rem";
> + $diff_class = "rem";
> } elsif ($char eq '@') {
> - $diff_class = " chunk_header";
> + $diff_class = "chunk_header";
> } elsif ($char eq "\\") {
> - $diff_class = " incomplete";
> + $diff_class = "incomplete";
> }
> }
> $line = untabify($line);
> @@ -2274,7 +2277,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 class=\"diff$diff_class\">$line</div>\n";
> + return $diff_class, "<div class=\"diff $diff_class\">$line</div>\n";
What are these changes for, except perhaps to make the patch larger than
necesssary to make it harder to review?
It would leave an unnecessary SP like 'class="diff "' when all the arms of
if/elsif cascade fall off and $diff_class is left empty. It isn't a major
issue (I suspect that such a case might even be an error), and I even
think the code after the above patch would be easier to read and more
sensible, but shouldn't that kind of "style fix" be in a separate clean-up
patch that does not introduce any new feature?
> @@ -2307,9 +2310,9 @@ sub format_diff_line {
> }
> $line .= " $prefix</span>" .
> "<span class=\"section\">" . esc_html($section, -nbsp=>1)
> . "</span>";
> - return "<div class=\"diff$diff_class\">$line</div>\n";
> + return $diff_class, "<div class=\"diff $diff_class\">$line</div>\n";
> }
> - return "<div class=\"diff$diff_class\">" . esc_html($line, -nbsp=>1)
> . "</div>\n";
> + return $diff_class, "<div class=\"diff $diff_class\">" .
> esc_html($line, -nbsp=>1) . "</div>\n";
> }
And everything else, including this hunk to change what is returned from
the subroutine belongs to a separate patch that implements the side-by-side
feature.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH/RFC] gitweb: add the ability to show side-by-side diff on commitdiff.
2011-10-17 0:20 ` Junio C Hamano
@ 2011-10-17 1:15 ` Kato Kazuyoshi
2011-10-17 1:51 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Kato Kazuyoshi @ 2011-10-17 1:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Mon, Oct 17, 2011 at 9:20 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> What are these changes for, except perhaps to make the patch larger than
> necesssary to make it harder to review?
>
> It would leave an unnecessary SP like 'class="diff "' when all the arms of
> if/elsif cascade fall off and $diff_class is left empty. It isn't a major
> issue (I suspect that such a case might even be an error), and I even
> think the code after the above patch would be easier to read and more
> sensible, but shouldn't that kind of "style fix" be in a separate clean-up
> patch that does not introduce any new feature?
>
Sorry. I will separate my patch into two or three commits. Probably like:
- format_diff_line returns a class with an line.
- remove trailing space from the class.
- add side-by-side feature and CSS.
- add form.
Thank you for your correction!
--
Kato Kazuyoshi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH/RFC] gitweb: add the ability to show side-by-side diff on commitdiff.
2011-10-17 1:15 ` Kato Kazuyoshi
@ 2011-10-17 1:51 ` Junio C Hamano
2011-10-17 2:33 ` Kato Kazuyoshi
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2011-10-17 1:51 UTC (permalink / raw)
To: Kato Kazuyoshi; +Cc: git
Kato Kazuyoshi <kato.kazuyoshi@gmail.com> writes:
> Probably like:
>
> - format_diff_line returns a class with an line.
> - remove trailing space from the class.
> - add side-by-side feature and CSS.
> - add form.
>
> Thank you for your correction!
My wording came out a bit too strong; I didn't mean to "correct" anything.
I think a better organization would be
[1/2] change code so that $diff_class does not have leading SP
optionally catch a case where $diff_class stays empty as an error?
[2/2] add side-by-side feature, which would involve:
- making format_diff_line() to return $diff_class separately;
- necessary addition of CSS;
- addition of form to trigger the feature.
I do not think splitting the second patch into pieces smaller than that
makes sense.
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH/RFC] gitweb: add the ability to show side-by-side diff on commitdiff.
2011-10-17 1:51 ` Junio C Hamano
@ 2011-10-17 2:33 ` Kato Kazuyoshi
2011-10-17 3:57 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Kato Kazuyoshi @ 2011-10-17 2:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Mon, Oct 17, 2011 at 10:51 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> I think a better organization would be
>
> [1/2] change code so that $diff_class does not have leading SP
> optionally catch a case where $diff_class stays empty as an error?
>
I think we don't have to treat empty $diff_class as an error because
$diff_class will be an empty when $line is around modification
(ex. foo or quux).
foo
- bar
+ baz
quuz
And class attributes are CDATA. "diff[SP]" and "diff" have same
meanings.
http://www.w3.org/TR/html401/struct/global.html#h-7.5.2
http://www.w3.org/TR/html401/types.html#type-cdata
--
Kato Kazuyoshi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH/RFC] gitweb: add the ability to show side-by-side diff on commitdiff.
2011-10-17 2:33 ` Kato Kazuyoshi
@ 2011-10-17 3:57 ` Junio C Hamano
0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2011-10-17 3:57 UTC (permalink / raw)
To: Kato Kazuyoshi; +Cc: Junio C Hamano, git
Kato Kazuyoshi <kato.kazuyoshi@gmail.com> writes:
> On Mon, Oct 17, 2011 at 10:51 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> I think a better organization would be
>>
>> [1/2] change code so that $diff_class does not have leading SP
>> optionally catch a case where $diff_class stays empty as an error?
>>
>
> I think we don't have to treat empty $diff_class as an error because
> $diff_class will be an empty when $line is around modification
> (ex. foo or quux).
>
> foo
> - bar
> + baz
> quuz
Ok. then even though class="diff " is not any different from class="diff",
I have to say generated html output becomes aesthetically less nice with
this change; we may want to do something about it _iff_ it does not take
too much change, but as I said I do not care too much about it.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-10-17 3:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-10-16 23:57 [PATCH/RFC] gitweb: add the ability to show side-by-side diff on commitdiff Kato Kazuyoshi
2011-10-17 0:20 ` Junio C Hamano
2011-10-17 1:15 ` Kato Kazuyoshi
2011-10-17 1:51 ` Junio C Hamano
2011-10-17 2:33 ` Kato Kazuyoshi
2011-10-17 3:57 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).