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

2018-03-06 Thread Andreas Heiduk 
Am 05.03.2018 um 10:37 schrieb Andreas Heiduk:
> 2018-03-05 2:42 GMT+01:00 Eric Sunshine :
>> On Sun, Mar 4, 2018 at 6:22 AM, Andreas Heiduk  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.

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

2018-03-05 Thread Eric Sunshine 
On Mon, Mar 5, 2018 at 4:37 AM, Andreas Heiduk  wrote:
> 2018-03-05 2:42 GMT+01:00 Eric Sunshine :
>> On Sun, Mar 4, 2018 at 6:22 AM, Andreas Heiduk  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?

I don't know the answer to either question. As I've only ever used
git-svn a few times many years ago, I can't speak of the reason behind
the name@uuid fallback, but that it was intentional suggests that
there may have been a good reason for it.

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

As I'm not the author of every script in the wild, I can't answer as
to the purpose of a script working in such a way, but that there may
be such scripts makes me cautious. We don't know how easy it would be
for a script to be "fixed" for this new behavior or even how soon the
"breakage" would be noticed for scripts which have been working
properly and quietly in the background for years.

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

I defer judgment to Eric W.'s area expertise.

>>> 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 {
>>> -   $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

Okay.

>> (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-ava...@gmail.com/
>
> Did the thread result in some actual action?

Not to my knowledge. Scanning the thread, it looks like the issue is
still hanging.

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

2018-03-05 Thread Eric Wong 
Andreas Heiduk  wrote:
> 2018-03-05 2:42 GMT+01:00 Eric Sunshine :
> > 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.

I consider that too low of a bar to justify a behavior change
which can negatively affect users.

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

2018-03-05 Thread Andreas Heiduk 
2018-03-05 2:42 GMT+01:00 Eric Sunshine :
> On Sun, Mar 4, 2018 at 6:22 AM, Andreas Heiduk  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.

>
>> Signed-off-by: Andreas Heiduk 
>> ---
>> 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.

> 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-ava...@gmail.com/

Did the thread result in some actual action?


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

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

2018-03-04 Thread Eric Sunshine 
On Sun, Mar 4, 2018 at 6:22 AM, Andreas Heiduk  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 
> ---
> 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-ava...@gmail.com/

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

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

2018-03-04 Thread Andreas Heiduk 
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.

Signed-off-by: Andreas Heiduk 
---
 perl/Git/SVN.pm | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index bc4eed3d75..b0a340b294 100644
--- 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];
} 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";
} 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;
-- 
2.16.2