Re: [PATCH/RFC] gitweb: add the ability to show side-by-side diff on commitdiff.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Kato Kazuyoshi <kato.kazuyoshi@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]