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

On Tue, Oct 18, 2011 at 4:02 AM, Junio C Hamano [off-list ref] wrote:

Kato Kazuyoshi [off-list ref] writes:

quoted

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!

quoted

 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