On Sun, Mar 4, 2018 at 6:22 AM, Andreas Heiduk <asheiduk@xxxxxxxxx> 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?
Doesn't such a behavior change deserve being documented (and possibly tests)?
> Signed-off-by: Andreas Heiduk <asheiduk@xxxxxxxxx>
> ---
> 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.
> } 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?
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.
Also, the Perl codebase in this project is still at 5.8, whereas the
// operator (and //=) didn't become available until Perl 5.10.
(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@xxxxxxxxx/
> } 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;
