On Thu, Sep 12, 2019 at 10:56 AM Tobias Klauser <tklauser@xxxxxxxxxx> wrote:
> v2:
> - move whitespace trimming below defined'ness check as per Eric Sunshine's
> review comment
> diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
> @@ -1494,6 +1494,7 @@ sub check_author {
> if (!defined $author || length $author == 0) {
> $author = '(no author)';
> }
> + $author =~ s/^\s+|\s+$//g;
Hmm, this still looks questionable. I would have expected the
whitespace trimming to be below the 'defined' check but before the
length($author)==0 check (since the length might become 0 once
whitespace is trimmed).
Also, a minor style/comprehension nit: Perhaps I'm just old-school,
but for me, the idiom:
$author =~ s/^\s+//;
$author =~ s/\s+$//;
is easier to understand at-a-glance as trimming leading and trailing
whitespace than the more compact (noisy) expression this patch uses.
But that's just a subjective review comment, not necessarily
actionable.
