Re: [PATCH 2/2] git-svn: allow empty email-address in authors-prog and authors-file
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;
Andreas Heiduk