Re: [PATCH 2/2] git-svn: allow empty email-address in authors-prog and authors-file

From: Andreas Heiduk <asheiduk@gmail.com>
Date: Mar 6, 2018, 22:24 UTC
Message-ID: <426e019a-cfb2-0e3a-b9b4-9d94f4f79312@gmail.com>
In-Reply-To: <CAFhHFBysKuDO9H4yJtnC6MJ+Jih5q4RsfwHTCsRXhXknapp4xg@mail.gmail.com>

Am 05.03.2018 um 10:37 schrieb Andreas Heiduk:

Show 42 quoted lines

> 2018-03-05 2:42 GMT+01:00 Eric Sunshine <sunshine@sunshineco.com>:
>> On Sun, Mar 4, 2018 at 6:22 AM, Andreas Heiduk <asheiduk@gmail.com> wrote:
>>> ---
>>> diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
>>> @@ -1482,7 +1482,6 @@ sub call_authors_prog {
>>>         }
>>>         if ($author =~ /^\s*(.+?)\s*<(.*)>\s*$/) {
>>>                 my ($name, $email) = ($1, $2);
>>> -               $email = undef if length $2 == 0;
>>>                 return [$name, $email];
>>
>> Mental note: existing behavior intentionally makes $email undefined if
>> not present in $author; revised behavior leaves it defined.
> 
> But only if the data comes from authors-prog. authors-file is unaffected.
> 
>>
>>>         } else {
>>>                 die "Author: $orig_author: $::_authors_prog returned "
>>> @@ -2020,8 +2019,8 @@ sub make_log_entry {
>>>                 remove_username($full_url);
>>>                 $log_entry{metadata} = "$full_url\@$r $uuid";
>>>                 $log_entry{svm_revision} = $r;
>>> -               $email ||= "$author\@$uuid";
>>> -               $commit_email ||= "$author\@$uuid";
>>> +               $email //= "$author\@$uuid";
>>> +               $commit_email //= "$author\@$uuid";
>>
>> With the revised behavior (above), $email is unconditionally defined,
>> so these //= expressions will _never_ assign "$author\@$uuid" to
>> $email. Am I reading that correctly? If so, then isn't this now just
>> dead code? Wouldn't it be clearer to remove these lines altogether?
> 
> The olf behaviour still kicks in if
>  - neither authors-file nor authors-prog is used
>  - only authors-file is used
> 
>> I see from reading the code that there is a "if (!defined $email)"
>> earlier in the function which becomes misleading with this change. I'd
>> have expected the patch to modify that, as well.
> 
> I will look into that one later.

I don't want to let that slip through the cracks: The `if` statement still makes a difference if:

 - neither ` --authors-prog` nor `--authors-file` is active,
 - but `--use-log-author` is active and
 - the commit at hand does not contain a `From:` or `Signed-off-by:`
   trailer.

In that case the result was and still is `$user <$user>` for the author and `$user <$user@$uuid>` for the comitter. That doesn't make sense to me but doesn't concern me right now.