Re: [PATCH/RFC] gitweb: add the ability to show side-by-side diff on commitdiff.
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 17, 2011, 00:20 UTC
- Message-ID
- <7vobxgpixo.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <CAFo4x0Kb651CyxoP8wxR36aDe5=Md2UV3qjh+HPo4ad6NB=Emg@mail.gmail.com>
Kato Kazuyoshi <kato.kazuyoshi@gmail.com> writes:
Show 41 quoted lines
> @@ -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?
Show 13 quoted lines
> @@ -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.