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