From: Kato Kazuyoshi <kato.kazuyoshi@gmail.com>
Date: Mon, 17 Oct 2011 23:58:02 GMT
Subject: Re: [PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class
Message-ID: <CAFo4x0KayxUfnVfO1-Qhu1LiMvwMs6g=bgPXZjqqZUxpUJsDWQ@mail.gmail.com>
In-Reply-To: <7v62jnmog3.fsf@alter.siamese.dyndns.org>

On Tue, Oct 18, 2011 at 4:02 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.
>

Thanks. I couldn't write a good summary for my patch because it was just
an "adjust" for me. However your summary is really clear!

>>  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.
>

Yeah, I want to remove unnecessary SP that you mentioned before.
But well, join(" ", ("frotz", "")) give me "frotz ".
I will add some per-function test cases to gitweb before this patch series.

Thanks,

-- 
Kato Kazuyoshi