Re: [PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class

[Jakub Narebski <jnareb@gmail.com>]

By the way, it is common to either have following patches to be chain
reply to first patch, or better provide cover letter for patch series
and have all patches be reply to cover letter.


Kato Kazuyoshi <kato.kazuyoshi@gmail.com> writes:

This commit needs some more explanation in the commit message, like
Junio said.

> The format_diff_line() will return $diff_class and HTML in upcoming changes.

This is not necessary, I think; this change stands alone as a style
(semantic) cleanup.

Signoff?  See Documentation/SubmittingPatches

> ---
>  gitweb/gitweb.perl |   24 +++++++++++++-----------
>  1 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 85d64b2..095adda 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2235,28 +2235,30 @@ 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";
>  		}

Hmmm... that reminds me: this if-elsif chain is a bit ugly, but would
be hard to replace without given ... when, I think.

>  	} 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";
>  		}

This is also ugly, but this one could in theory be replaced by hash
lookup.  But this would remove symmetry...

>  	}
>  	$line = untabify($line);
> +
> +	my $div_open = '<div class="' . (join ' ', ('diff', $diff_class)) . '">';

Another solution would be to use

  +
  +	my $diff_classes = "diff";
  +	$diff_classes .= " $diff_class" if ($diff_class);
  +

>  	if ($from && $to && $line =~ m/^\@{2} /) {
>  		my ($from_text, $from_start, $from_lines, $to_text, $to_start,
> $to_lines, $section) =
>  			$line =~ m/^\@{2} (-(\d+)(?:,(\d+))?) (\+(\d+)(?:,(\d+))?) \@{2}(.*)$/;
> @@ -2274,7 +2276,7 @@ sub format_diff_line {
>  		}
>  		$line = "<span class=\"chunk_info\">@@ $from_text $to_text @@</span>" .
>  		        "<span class=\"section\">" . esc_html($section, -nbsp=>1) .
> "</span>";

The above is word-wrapped.  Please turn off word wrapping in the
future to avoid such damage to the patch.

> -		return "<div class=\"diff$diff_class\">$line</div>\n";
> +		return "$div_open$line</div>\n";

This would be instead

  +		return "<div class=\"$diff_classes\">$line</div>\n";

which is a bit more symmetric.

But that is just a matter of taste.

>  	} 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 +2309,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 "$div_open$line</div>\n";
>  	}
> -	return "<div class=\"diff$diff_class\">" . esc_html($line, -nbsp=>1)
> . "</div>\n";
> +	return $div_open . esc_html($line, -nbsp=>1) . "</div>\n";
>  }
> 
>  # Generates undef or something like "_snapshot_" or "snapshot (_tbz2_ _zip_)",
> -- 
> 1.7.7.213.g8b0e1.dirty

-- 
Jakub Narębski