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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 17, 2011, 19:02 UTC
Message-ID: <7v62jnmog3.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CAFo4x0LP4fXgSNAnss_WRLo-TH_qe=esYn7P+=iS6t87tdzcbw@mail.gmail.com>

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

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

An auxiliary piece of information like this is fine at the end of the commit log message, but the patch itself wants to be justified standalone. Perhaps this should be sufficient:

	The $diff_class variable to classify the kind of line in the diff
	output was prefixed with a SP, only so that the code to synthesize
	value for "class" attribute can blindly concatenate it with
	another value "diff". This made the code unnecessarily ugly.

	Instead, add SP that separates the value of $diff_class from
	another class value "diff" where <div class="..."> string is
	created and drop the leading SP from the value of $diff_class.
	
Explained this way, it does not even have to mention that the return value
will be changed in a different patch.

We all know that the hidden motivation of this change is that the caller of this function, after it starts using the returned value of $diff_class, does not want to worry about the ugly SP prefix in that variable. Saying only "This function will return this variable in the future" still does not fully explain that hidden motivation unless you say "and the caller shouldn't have to worry about the leading SP", so let's not even mention it in the log message of this change.

> ---

Sign-off?

Show 11 quoted lines

>  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 {
> ...
> +
> +	my $div_open = '<div class="' . (join ' ', ('diff', $diff_class)) . '">';

I think using a separate helper variable like this is a good change. You do not have to worry about the issue in three different places.

But doesn't join(" ", ("frotz", "")) still give you "frotz "? It is OK to punt and say

	my $div_open = '<div class="diff $diff_class">';

which would be far easier to read. It may sacrifice a bit of tidiness in the resulting HTML but the tidiness of the source outweighs it.

Of course, if you have tons of classes, it may be worth doing something like

	join(" ", grep { defined $_ && $_ ne ""}  @diff_classes);

but I do not think it is worth it in this particular case.

Thanks.