From: Eric Sunshine <sunshine@sunshineco.com>
Date: Thu, 12 Sep 2019 18:20:11 GMT
Subject: Re: [PATCH v2] git-svn: trim leading and trailing whitespaces in author name
Message-ID: <CAPig+cTAY=4cuDyrsPiDH+MUvz4+H4eMKAsmTAETep2On5=q3g@mail.gmail.com>
In-Reply-To: <20190912145638.32192-1-tklauser@distanz.ch>

On Thu, Sep 12, 2019 at 10:56 AM Tobias Klauser <tklauser@distanz.ch> 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.