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

From: Andreas Heiduk <asheiduk@gmail.com>
Date: Mar 5, 2018, 09:37 UTC
Message-ID: <CAFhHFBysKuDO9H4yJtnC6MJ+Jih5q4RsfwHTCsRXhXknapp4xg@mail.gmail.com>
In-Reply-To: <CAPig+cQTpbj6q_s_pX2HjmpYF4W+5wKH7YLQQdZTiMPfK4G2Ng@mail.gmail.com>

2018-03-05 2:42 GMT+01:00 Eric Sunshine <sunshine@sunshineco.com>:

Show 12 quoted lines

> On Sun, Mar 4, 2018 at 6:22 AM, Andreas Heiduk <asheiduk@gmail.com> wrote:
>> The email address in --authors-file and --authors-prog can be empty but
>> git-svn translated it into a syntethic email address in the form
>> $USERNAME@$REPO_UUID. Now git-svn behaves like git-commit: If the email
>> is explicitly set to the empty string, the commit does not contain
>> an email address.
>
> Falling back to "$name@$uuid" was clearly an intentional choice, so
> this seems like a rather significant change of behavior. How likely is
> it that users or scripts relying upon the existing behavior will break
> with this change? If the likelihood is high, should this behavior be
> opt-in?

I don't know nor understand the intial choice. Didn' git-commit support empty emails once upon a time? Or is the SVN-UID important for some SVK/SVM workflows?

My reasoning was: If authors-prog is NOT used, the behaviour is unchanged - the UUID will be generated. If a Script IS used, then I assume that a valid email was generated and this change will not break these scripts. Only scripts intentionally not generating emails will break. But again - was would be the purpose of such a script? And such a script can be easily changed to add the UUID again.

So I think adding an explicit opt-in does not pay off.

> Doesn't such a behavior change deserve being documented (and possibly tests)?

The old behaviour was neither documented nor tested - the change did not break any test after all.

Show 13 quoted lines

>
>> Signed-off-by: Andreas Heiduk <asheiduk@gmail.com>
>> ---
>> 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.

Show 16 quoted lines

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

> Also, the Perl codebase in this project is still at 5.8, whereas the
> // operator (and //=) didn't become available until Perl 5.10.

I can expand these into something like

    $email = defined $email ? $email : "$author\@$uuid";

or

    $email = "$author\@$uuid" unless defined $email;

> (However, there has lately been some talk[1] about bumping the minimum
> Perl version to 5.10.)
>
> [1]: https://public-inbox.org/git/20171223174400.26668-1-avarab@gmail.com/

Did the thread result in some actual action?

Show 23 quoted lines

>>         } elsif ($self->use_svnsync_props) {
>>                 my $full_url = canonicalize_url(
>>                         add_path_to_url( $self->svnsync->{url}, $self->path )
>> @@ -2029,15 +2028,15 @@ sub make_log_entry {
>>                 remove_username($full_url);
>>                 my $uuid = $self->svnsync->{uuid};
>>                 $log_entry{metadata} = "$full_url\@$rev $uuid";
>> -               $email ||= "$author\@$uuid";
>> -               $commit_email ||= "$author\@$uuid";
>> +               $email //= "$author\@$uuid";
>> +               $commit_email //= "$author\@$uuid";
>>         } else {
>>                 my $url = $self->metadata_url;
>>                 remove_username($url);
>>                 my $uuid = $self->rewrite_uuid || $self->ra->get_uuid;
>>                 $log_entry{metadata} = "$url\@$rev " . $uuid;
>> -               $email ||= "$author\@" . $uuid;
>> -               $commit_email ||= "$author\@" . $uuid;
>> +               $email //= "$author\@" . $uuid;
>> +               $commit_email //= "$author\@" . $uuid;
>>         }
>>         $log_entry{name} = $name;
>>         $log_entry{email} = $email;