Re: [PATCH 2/2] git-svn: allow empty email-address in authors-prog and authors-file
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Mar 5, 2018, 01:42 UTC
- Message-ID
- <CAPig+cQTpbj6q_s_pX2HjmpYF4W+5wKH7YLQQdZTiMPfK4G2Ng@mail.gmail.com>
- In-Reply-To
- <20180304112237.19254-2-asheiduk@gmail.com>
On Sun, Mar 4, 2018 at 6:22 AM, Andreas Heiduk <asheiduk@gmail.com> wrote:
Show 5 quoted lines
> 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)?
Show 9 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.
Show 10 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?
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@gmail.com/
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;