On Tue, Oct 18, 2011 at 4:02 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Kato Kazuyoshi <kato.kazuyoshi@xxxxxxxxx> 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
--
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