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, 22:22 UTC
Message-ID: <CAPig+cSRF0g_+rq_Ga-kg+fh2XnUobVKodkR9Z=pVskq7vRU6w@mail.gmail.com>
In-Reply-To: <CAFhHFBysKuDO9H4yJtnC6MJ+Jih5q4RsfwHTCsRXhXknapp4xg@mail.gmail.com>

On Mon, Mar 5, 2018 at 4:37 AM, Andreas Heiduk <asheiduk@gmail.com> wrote:

Show 17 quoted lines

> 2018-03-05 2:42 GMT+01:00 Eric Sunshine <sunshine@sunshineco.com>:
>> 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?

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.

Show 6 quoted lines

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

Show 27 quoted lines

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

Show 6 quoted lines

>> (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?

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